Fixed crashes when looking up symbols of jsdoc nodes in TS files #57110

Andarist · 2024-01-20T23:55:08Z

Andarist · 2024-01-21T10:00:19Z

src/services/findAllReferences.ts

@@ -1837,6 +1837,9 @@ export namespace Core {
                }
                // falls through I guess
            case SyntaxKind.Identifier:
+                if (node.flags & NodeFlags.JSDoc && !isInJSFile(node)) {


I need to additionally check if this is in an "any type tag" as those should be ignored whereas nodes in semantic~ jsdoc tags shouldn't (like in @see, @link, etc). I couldn't find any utility that would check something like this - I could create such and manually curate the list of those tags in it.

But there is another question: should those references in type-like tags always be ignored? Or should TS try to resolve corresponding TS types? There is always a risk that those won't match but in such a case displaying any (likely as a result of displaying errorType) could actually be OK.

An alternative fix for the crashes would be to ignore symbol-less declarations for now but I intentionally didn't go that route. It seemed like a bandaid for a deeper problem and I think handling this at a higher level would be better. I'm just not sure now what's the exact desired behavior.

cc @sandersn

I think you're asking whether find-all-refs should skip the references in @see et al? Is that right? Ideally the answer is No. Symbol resolution for those tags works outside the normal name resolution code, as I recall, via resolveJSDocMemberName. That function is called from getSymbolAtLocation, the usual entrypoint for public users of the API. You might want to call one of those two functions.

A utility to identify tags with semantics in Typescript would be useful, and I don't think it exists yet. scanner.ts does have a regex, but that's not useful for a parse tree. (and, besides, @deprecated should be included in the post-parser predicate, but not the scanner one.)

Please add a test either way.

sandersn · 2024-02-01T23:54:41Z

tests/cases/fourslash/findReferencesBindingPatternInJsdocNoCrash1.ts

+////  data: string[];
+////};
+
+// @Filename: node_modules/use-query/package.json


This should be other to match the following index.d.ts
Although I don't know if package.json is really needed for either package.
And they might not need to be in separate packages for the problem to reproduce, right? It seems like this should even work inside a single file, but at least should repro between files of a single package.

Suggested change

// @Filename: node_modules/use-query/package.json

// @Filename: node_modules/other/package.json

yes, it should - the package split shouldn't be required for this to be reproduced at all, it was just faster for me to keep the package structure as-is (like in the original real-world code)

If u feel strongly about it, I can simplify this further or add an extra test case. I need to refresh myself on the whole situation (which I'll do soon since I plan to finish this PR) - but the file split itself might have been important for this (I'm not 100% sure about it right now though)

tests/baselines/reference/myFindAllRefs.baseline.jsonc

sandersn · 2024-02-02T00:21:22Z

tests/cases/fourslash/findReferencesBindingPatternInJsdocNoCrash2.ts

+////interface BottomSheetModalProps {
+////  /**
+////   * A scrollable node or normal view.
+////   * @type null | (({ data: any }?) => any)


is null | the only difference between NoCrash2 and 1? If so, I don't see what value it adds above NoCrash1, especially since the new code doesn't reference Typescript types really.

This test case exercises a braceless @type which was somewhat important when I started to reduce this and for some moments I couldn't figure out why the crash went away on certain reduction attempts. So I included that here to be extra sure that it's gonna be fixed in such a scenario too

sandersn · 2024-02-02T00:58:22Z

src/services/findAllReferences.ts

@@ -1837,6 +1837,9 @@ export namespace Core {
                }
                // falls through I guess
            case SyntaxKind.Identifier:
+                if (node.flags & NodeFlags.JSDoc && !isInJSFile(node)) {


I think you're asking whether find-all-refs should skip the references in @see et al? Is that right? Ideally the answer is No. Symbol resolution for those tags works outside the normal name resolution code, as I recall, via resolveJSDocMemberName. That function is called from getSymbolAtLocation, the usual entrypoint for public users of the API. You might want to call one of those two functions.

A utility to identify tags with semantics in Typescript would be useful, and I don't think it exists yet. scanner.ts does have a regex, but that's not useful for a parse tree. (and, besides, @deprecated should be included in the post-parser predicate, but not the scanner one.)

Please add a test either way.

Andarist · 2024-03-05T10:32:12Z

src/compiler/checker.ts

@@ -11027,6 +11027,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
        }

        if (isParameter(declaration)) {
+            if (!declaration.symbol) {


I decided to include this here because it's just always possible to call checker.getTypeAtLocation or checker.getTypeAtLocation and those shouldn't crash or do something overly goofy - so the safest bet is to actually be on the safe side of things here + fine-tune existing call sites to those 2 if it's absolutely necessary to avoid getting errorType back when it's obvious that it will be returned back

Andarist · 2024-03-05T10:34:39Z

src/services/services.ts

+                    ((node.parent.kind === SyntaxKind.PropertySignature && (node.parent as PropertySignature).name === node) ||
+                        findAncestor(node, (n) => n.kind === SyntaxKind.Parameter))
+                    ) {
+                    // if we'd request type at those locations we'd get `errorType` that displays confusingly as `any`
+                    return false;
+                }


I don't like how specific this is but I concluded that some condition like this is the best solution for #57108 . I'm not 100% sure if I'm missing some other node types that should be ignored here or not though. The worst case scenario quick info will continue to display any when it shouldn't so I feel it's fine. If you come up with any extra situations that should be handled here I can tweak this and add more tests

Note that it's still good to display any here - and that's why I have to prevent the type request conditionally instead of just ignoring return values that are error types or smth like that

Andarist · 2024-03-05T10:41:54Z

tests/cases/fourslash/quickInfoInJsdocInTsFile1.ts

+//// const test3 = { bar: '' };
+////
+//// type SomeObj = { bar: string; };
+//// /** @type {SomeObj/*4*/} */


In TS files this is just ignored, in the sense that it doesn't impact types at all. So technically we could just avoid displaying anything here. I concluded though that it would actually be less useful and that it would be confusing - usually the user expects some feedback in form of a tooltip and not returning any info here could feel like something is off, like it's broken.

Since this isn't used for type-checking there is no guarantee that it "matches" the declaration below - I think it's an acceptable tradeoff. If somebody is writing JSDoc types in TS files then it's their responsibility to make them match.

cc @jakebailey since we were talking about this a few hours ago

Andarist · 2024-03-05T10:43:22Z

tests/cases/fourslash/quickInfoInJsdocInTsFile1.ts

+//// /** @type {(a/*8*/: string) => void} */
+//// function test2(a: string) {}


In situations like this and more, we could probably try to find the matching symbol and display that. That could be converted to a feature request - I don't think it's important enough for me to work on it though. Using both types of annotations in TS files shouldn't be common anyway.

sandersn · 2024-03-27T20:24:57Z

Can you merge from main before I merge this? We've had a lot of baseline churn recently.

jakebailey · 2024-03-27T20:25:54Z

We'll still not done, forgot one PR is left...

Andarist · 2024-03-28T07:03:35Z

@sandersn done

Fixed crashes when looking up symbols of jsdoc nodes in TS files

84a8002

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 20, 2024

Andarist commented Jan 21, 2024

View reviewed changes

sandersn requested review from sandersn and DanielRosenwasser February 1, 2024 23:36

sandersn self-assigned this Feb 1, 2024

sandersn requested a review from jakebailey February 2, 2024 00:36

sandersn requested changes Feb 2, 2024

View reviewed changes

Andarist mentioned this pull request Mar 3, 2024

Fixed braceless type tags with types starting with an open parenthesis #57167

Merged

Andarist added 5 commits March 3, 2024 11:57

Merge remote-tracking branch 'origin/main' into fix/jsdoc-in-ts-crash

7cbd5db

remove accidentally committed file

25435c3

fix filename in a test case

38b1f18

just avoid locations without symbols

a3f6994

fine tune the logic

f6a76e8

Andarist requested a review from sandersn March 5, 2024 10:29

Andarist commented Mar 5, 2024

View reviewed changes

add extra test case

834d281

Andarist commented Mar 5, 2024

View reviewed changes

format

6f3dd91

sandersn approved these changes Mar 27, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/jsdoc-in-ts-crash

cb73700

Merge branch 'main' into fix/jsdoc-in-ts-crash

f5719b4

sandersn merged commit 69e7e57 into microsoft:main Apr 4, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed crashes when looking up symbols of jsdoc nodes in TS files #57110

Fixed crashes when looking up symbols of jsdoc nodes in TS files #57110

Andarist commented Jan 20, 2024 •

edited

Loading

Andarist Jan 21, 2024

sandersn Feb 2, 2024

sandersn Feb 1, 2024

Andarist Mar 3, 2024

Andarist Mar 3, 2024

sandersn Feb 2, 2024

Andarist Mar 4, 2024

sandersn Feb 2, 2024

Andarist Mar 5, 2024

Andarist Mar 5, 2024

Andarist Mar 5, 2024

Andarist Mar 5, 2024

Andarist Mar 5, 2024

Andarist Mar 5, 2024

sandersn commented Mar 27, 2024

jakebailey commented Mar 27, 2024

Andarist commented Mar 28, 2024

	// @Filename: node_modules/use-query/package.json
	// @Filename: node_modules/other/package.json

		//// /** @type {(a/8/: string) => void} */
		//// function test2(a: string) {}

Fixed crashes when looking up symbols of jsdoc nodes in TS files #57110

Fixed crashes when looking up symbols of jsdoc nodes in TS files #57110

Conversation

Andarist commented Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Mar 27, 2024

jakebailey commented Mar 27, 2024

Andarist commented Mar 28, 2024

Andarist commented Jan 20, 2024 •

edited

Loading