Fix #47753 - Organize imports removes type imports that are only referenced in @link (jsdoc)

[komyg]

Fixes #47753, #47558

[sandersn]

This works, but it should actually happen in FindAllReferences, and use the normal FindAllReference machinery.

[komyg]

Hi @sandersn, thanks for the review. I will update the PR accordingly, but I just need a little time, because things are kind of crazy right now.

[DanielSWolf]

This is related to the larger issue that TS considers @linked imports as unused, as described in #47558. I wonder whether it might make more sense to solve the root cause rather than fixing this specific symptom.

[komyg]

Hey @DanielSWolf, I think I found the root cause of both problems in the compiler. I will try to solve both issues.

[DanielSWolf]

@komyg That would be great! The current behavior makes it really hard to write links that consistently work.

[komyg]

Hi @sandersn, I've fixed the FindAllReference function as you requested and all test cases for organize imports have passed.

However, I also got an error for a JSDoc baseline that I didn't understand, so I don't know if I inadvertently broke something or if the fix also corrected it.

Could you please review the PR again?

[sandersn]

The JSDoc baseline change shows that the link target is now different for imports. For this code:

// @filename: one.ts
export class C { }
// @filename: two.ts
import { C } from './one'
/** {@link C} */
var x = 1

{@link C} used to link to the actual declaration of C in one.ts (export class C). Now, because aliases are allowed, it returns C from two.ts instead: import { C }. In effect, it stops at the alias instead of following it like it used to.

That's not right, so this PR needs more work. I'll see if I can figure out what.

[sandersn]

WIthout debugging the newly-failing test case, I don't know how the old code resolves C to the original class. What tests fail if you change /*dontResolveAlias*/ !isJSDoc to /*dontResolveAlias*/ false? It's possible that it doesn't actually ever need to be true.

[komyg]

WIthout debugging the newly-failing test case, I don't know how the old code resolves C to the original class. What tests fail if you change /*dontResolveAlias*/ !isJSDoc to /*dontResolveAlias*/ false? It's possible that it doesn't actually ever need to be true.

Well, I since the dontResolveAlias param is !JSDoc it will probably be always false.

I think that the problem lies in the resolveEntityName function. I've followed its execution through the debugger, and it resolves the symbol for the {@link ZodType}, however at its end there is the following conditional:

return (symbol.flags & meaning) || dontResolveAlias ? symbol : resolveAlias(symbol);

By setting dontResolveAlias to true or by adding the Symbol.Alias meaning when calling resolveEntityName, we fall into the first part of the ternary and just return the symbol we found. Which in our case means the import statement that is referenced by the JSDoc {@link ZodType} and the import from the C type in the failing test.

Since the original code did not have the Symbol.Alias in the meaning and dontResolveAlias was false, we fell into the second part of the ternary, which meant we tried to resolve our JSDoc {@link ZodType} import alias twice, resulting in an unknown type and the bug that was reported.

A possible solution would be for us to find what is unique about each situation to make a better choice whether to set dontResolveAlias to true or false or to update the meaning accordingly. But I don't know if this is possible, since we have two different use cases of the same {@link} element.

[sandersn]

So: in the old code, jsdoc references, like {@link C} in my recent simplified example, do resolve aliases? Then why doesn't calling resolveAlias on import { C } return export class C? It seems like that should succeed.

You are right, though: for the purpose of organise imports, we don't actually want to resolve aliases. It's enough to know that {@link C} resolves to an imported alias, even if it's an incorrect import. But the code is so distant from the actual name resolution, I don't see how you could differentiate that use from the normal find-all-references use.

[komyg]

How can we fix this, then? It seems that organize imports is an "edgier" case than resolving aliases that you mentioned, so maybe we could do some logic outside the findAllReferences code.

My original solution did not rely on the findAllReferences code. Maybe we can repurpose that somehow or add it to the checker code? What do you think?

Note: this is the commit in which I erased my previous attempt at a solution.

[sandersn]

I looked again at your test cases, and the first problem is that they import things that aren't present in the program. The test cases need to have multiple files. See jsdocLink3.ts for an example.

It might be nice if organise imports didn't rely on find-all-refs, and therefore on the the target of the alias being defined.

[sandersn]

From manual testing, I can see that find-all-references doesn't work correctly for @link references, so it should be fixed too. I still believe that fixing find-all-refs will fix organise-imports. Here's the test I used:

// @filename:ex.d.ts
export type A = 1 | 2
export type B = 3 | 4
// @filename: main.ts
import { A, B } from './ex'
/** {@link A} */
class C {
  b: B = 3
}

Find-all-refs on main's B finds 3 refs; find-all-refs on main's A only finds the import alias.

[sandersn]

WIthout debugging the newly-failing test case, I don't know how the old code resolves C to the original class. What tests fail if you change /dontResolveAlias/ !isJSDoc to /dontResolveAlias/ false? It's possible that it doesn't actually ever need to be true.

I tested this locally and it is the solution. It does change the baselines for jsdocLink3, but I'm pretty sure those were wrong, because I manually tested the test code in jsdocLink3 and it behaves right with false, wrong with !isJSDoc. When I wrote the code, I didn't understand how it was supposed to follow (or not) aliases.

[komyg]

Hey @sandersn, from your comments, I understood that the solution that we've implemented here is correct, and I just have to make the following changes:

1. Update my test cases, so that they have types defined in multiple files, as per your example and the jsdocLink3.ts test.

2. Change the call to findAllReferences so that the dontResolveAlias is always false, but also keep the change that I made that adds Symbol.Alias to the meaning.

3. Update the jsdocLink3.ts baseline.

4. Add the test case that you created above.

Am I right?

[sandersn]

Everything is right except skipping the addition of Symbol.Alias. You only need dontResolveAlias: false.

Edit: Also I think jsdoclink3 covers enough that you don't need to add the test case in (4).

[sandersn]

Sorry, true. dontResolveAlias should be true.
(Just like it is a few lines below in the isTypeReferenceIdentifier branch)

[komyg]

Hi @sandersn, I've done the changes we've discussed. Could you review again, please?

[komyg]

Hi @sandersn, sorry to bother you. I've done the changes we've discussed. Could you review again, please?

[sandersn]

Woops, sorry this fell off my list. Thanks for your patience!

[komyg]

No worries! I am happy to have contributed!