search

microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.87k stars 12.36k forks source link

JSDoc @link not resolved across modules #43869

Open connor4312 opened 3 years ago

connor4312 commented 3 years ago

Bug Report

While verifying https://github.com/microsoft/vscode/issues/119357

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

Workbench Repro

💻 Code

// @filename: a.ts
import { Model } from './b';

/**
 * {@link Model}
 */

// @filename: b.ts
export class Model {}

🙁 Actual behavior

@link Model is not clickable

🙂 Expected behavior

I expected to be able to click on @link Model

connor4312 commented 3 years ago

Actually, this may be wrong -- the workbench seems like it might be slightly buggy. This works only if I import the referenced type. However, with the compiler flag disallowed unused locals, this produces a warning if I don't use the type elsewhere in the file.

One fix for that would be to mark linked types as "Used". C# does that in their xml docs. Or just link against all exported globals, but this might direct to the wrong types if they share names.

This is the link where I see it in a real program -- which typedoc, when generated, resolves fine https://github.com/connor4312/cosmonaut/blob/18a954f6184173fff999a4616b7cf8554addd966/src/baseModel.ts#L45

orta commented 3 years ago

Cool, I agree that this should probably work, and should probably look through the whole known list of symbols in the current project (that's what I'd expect as a JS user, and I think it matches how the web version of this works).

From a dive into this it looks like https://github.com/microsoft/TypeScript/pull/41877 never made changes to the gotoDefinition service meaning that it will only work with identifiers in the current scope, and doesn't reach out to search the whole project for an identifier

A failing fourslash test:

///<reference path="fourslash.ts" />

// @Filename: foo.ts
//// export interface [|/*def1*/Foo|] {
////     foo: string
//// }

// @Filename: importee.ts
//// /** {@link /*otherFile*/[|Foo|] } not explicitly linked */
//// const f = ""

goTo.marker("otherFile");
verify.goToDefinitionIs("def1");
orta commented 3 years ago

This had a bit of chatter about whether expanding the scope to all identifiers is the right call, so I'll wait till @sandersn is back to chat about that

Otherwise, we'll make the un-used import as used if it's in the @link

sandersn commented 3 years ago

After discussing this with @orta, we switched our opinions. Now I think that expanding the scope to all identifiers is the right call and he doesn't. However: it would be difficult to decide on the precise semantics and difficult to implement efficiently, so I don't think it's worthwhile right now.

What should work is import references: import('./b').Model. I don't think that @link even parses this reference right now.

Edit: @mjbvz beat me to it: #43950

pokey commented 2 years ago

I'd lean towards solving this one by treating imports that are referenced in a @link as used variables so that @typescript-eslint/no-unused-vars doesn't complain. This way the developer can just import variables for links like normal, so they only need to know one mechanism for importing variables.

And then for extra credit, to make the experience even more consistent between @link and normal references: