Cannot navigate JSDoc links in a hover

[pisv]

Bug Description:

Attempting to open a JSDoc link displayed in a hover fails with a logged error.

Steps to Reproduce:

1. Open a JS/TS file and hover over a symbol that has a documentation with JSDoc links, e.g.

2. Click on a JSDoc link in the hover.

Expected behavior: The JSDoc link is opened.

Actual behavior: Clicking on the JSDoc link has no effect. An error is logged like that:

ERROR Fail to open 'command:_typescript.openJsDocLink?%5B%7B%22file%22%3A%7B%22path%22%3A%22%2F...%2Fnode_modules%2F%40types%2Fvscode%2Findex.d.ts%22%2C%22scheme%22%3A%22file%22%7D%2C%22position%22%3A%7B%22line%22%3A9414%2C%22character%22%3A8%7D%7D%5D': Error: Invalid arguments
    at new Range (/.../theia/examples/browser/lib/backend/packages_plugin-ext_lib_common_plugin-api-rpc_js.js:2913:19)
    ...

Additional Information

• Operating System: macOS 13.6.6
• Theia Version: 1.48.0

[pisv]

The error is thrown from the constructor of the Range class: https://github.com/eclipse-theia/theia/blob/9b0e47f23661070e15602c90dd68a298aced2902/packages/plugin-ext/src/plugin/types-impl.ts#L457-L459

[pisv]

The error is thrown because the arguments passed to the Range constructor are in this case Position-like objects deserialized from the command URI query at https://github.com/eclipse-theia/theia/blob/09051c419afb34dc0b2b10d4cb4f0e19962d593f/packages/core/src/browser/command-open-handler.ts#L38 while the constructor expects instances of the Position itself: https://github.com/eclipse-theia/theia/blob/9b0e47f23661070e15602c90dd68a298aced2902/packages/plugin-ext/src/plugin/types-impl.ts#L452-L455

[pisv]

Note that the code in types-impl.ts was initially copied from https://github.com/microsoft/vscode/blob/1.37.0/src/vs/workbench/api/common/extHostTypes.ts.

Since that time, VSCode has evolved to relax the constraint in the Range constructor: https://github.com/microsoft/vscode/commit/0fd15bb21d6132ff3514a9df465cd8e07f22346b.

It looks like the same thing needs to be done in Theia.

[JonasHelming]

@pisv Thank you for the analysis. Are you up to do a contribution to fix this?

[pisv]

@pisv Thank you for the analysis. Are you up to do a contribution to fix this?

Sure. I think that essentially the above mentioned change from VSCode (https://github.com/microsoft/vscode/commit/0fd15bb21d6132ff3514a9df465cd8e07f22346b) needs to be copied to Theia. I can do it, if there are no objections.

[tsmaeder]

In principle, I'd argue that the VS Code folks are dead wrong, because their own typing says the only thing you can create a Range with is an instanceof Position and not some random object that happens to look like a position. In practice, I guess it's the smart thing to be "bug-compatible" in this instance. Can we have an overload of the constructor in types-impl.ts, though that documents that parameter typing? If you want, request me as a review on the PR to avoid having this discussion with another person. Otherwise, this calls for a comment in the constructor.

[pisv]

After looking into it further, this issue is actually caused by the implementation of the _typescript.openJsDocLink command (typescript-language-features/src/commands/openJsDocLink.ts), which is not type safe. It declares the type of the command parameter as

export interface OpenJsDocLinkCommand_Args {
	readonly file: vscode.Uri;
	readonly position: vscode.Position;
}

where actually the command argument type is in this case

{
	readonly file: vscode.UriComponents;
	readonly position: { line: number; character: number; };
}

Note that the implementation of this command converts args.file with vscode.Uri.from(args.file), but misses a similar conversion for args.position.

So, basically, I think this is a bug in typescript-language-features rather than in Theia itself.

[pisv]

In principle, I'd argue that the VS Code folks are dead wrong, because their own typing says the only thing you can create a Range with is an instanceof Position and not some random object that happens to look like a position.

You are right; I was dead wrong in my initial analysis.

In practice, I guess it's the smart thing to be "bug-compatible" in this instance.

I no longer think it would be a good idea. As evidenced by this issue, it might have masked the actual bug in a plug-in, as it did in VSCode in this case.

Anyway, the issue has been fixed in typescript-language-features: https://github.com/microsoft/vscode/pull/209872, and I don't plan on doing anything about it in Theia itself. From my POV, this issue can be closed, but I'm leaving it to the discretion of committers.

[tsmaeder]

We can close this one once we have reached VS Code 1.89, then. Thanks for the analysis.

[pisv]

Closing as per the comment above.