Skip to content

JSDoc Instantiation Fixes #17553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Aug 1, 2017

Fixes #17383. - We now assume that if a symbol does not exist, then we found a builtin, but tried to instantiate it, which does not work.
Fixes #17377. - We now assume that instantiation can fail (ie, if the type doesn't exist as a type) and only look up the type arguments if instantiation was a success.
Fixes #17525. - We now consider * the potential start of a type.

@@ -18592,6 +18592,11 @@ namespace ts {
forEach(node.typeArguments, checkSourceElement);
if (produceDiagnostics) {
const symbol = getNodeLinks(node).resolvedSymbol;
if (!symbol) {
// There is no resolved symbol cached if the type resolved to a builtin via jsdoc type reference resolution, none of which are generic when they have no associated symbol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap comment, jsdoc -> JSDoc

Give a specific example of what you mean by JSDoc type reference resolution (e.g. a snippet of when this can happen)

@Jessidhia
Copy link

This same crash is also happening in a snippet that has {Array<*>} as its type, which IIUC is supposed to be generic.

See #17525

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weswigham Can you verify whether this addresses #17525 as well?

@weswigham
Copy link
Member Author

@rbuckton It prevents it from throwing, but doesn't give it the correct behavior. We seem to be parsing @param {Array<*>} list as a parameter tag with type Array, no type arguments, and a comment on the type of >} list. The root issue with that scenario would appear to be in the parser, not here. I'll fix it separately.

@RyanCavanaugh
Copy link
Member

I think we have three different repros on this, it'd be good to have all of them in the TC

@weswigham
Copy link
Member Author

I've also bundled a fix for #17377, which was actually another issue which manifested as a crash on the same line. We assumed that if there was no resolved symbol that we successfully instantiated the type, which is, of course, predicated on the type being instantiable; which in erroneous cases (such as the example), it is not.

@weswigham weswigham changed the title Fix #17383 - Issue an error when jsdoc attempts to instantiate a builtin JSDoc Instantiation Fixes Aug 2, 2017
@weswigham
Copy link
Member Author

And now this fixes #17525, since they were all hidden behind a crash on the same line.

return;
}
let typeParameters = symbol.flags & SymbolFlags.TypeAlias && getSymbolLinks(symbol).typeParameters;
if (!typeParameters && getObjectFlags(type) & ObjectFlags.Reference) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check for objectFlags.Reference is new. What if it's false? Won't you still see a crash? When could it be false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously it was blindly assumed that if it wasn't a type alias, that it must be a type reference. This is not the case - it can just be erroneous (since we just mark a lookup error and return the type for the alternate meaning if the lookup fails); so we must check that we have a type reference before treating the type as such.

@weswigham weswigham merged commit c06a30a into microsoft:master Aug 2, 2017
@weswigham weswigham deleted the fix-jsdoc-instantiate-builtin-nongeneric branch August 2, 2017 20:55
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants