Skip to content

JSDoc Instantiation Fixes#17553

Merged
weswigham merged 4 commits into
microsoft:masterfrom
weswigham:fix-jsdoc-instantiate-builtin-nongeneric
Aug 2, 2017
Merged

JSDoc Instantiation Fixes#17553
weswigham merged 4 commits into
microsoft:masterfrom
weswigham:fix-jsdoc-instantiate-builtin-nongeneric

Conversation

@weswigham
Copy link
Copy Markdown
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.

Comment thread src/compiler/checker.ts Outdated
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

Comment thread src/compiler/checker.ts
return;
}
let typeParameters = symbol.flags & SymbolFlags.TypeAlias && getSymbolLinks(symbol).typeParameters;
if (!typeParameters && getObjectFlags(type) & ObjectFlags.Reference) {
Copy link
Copy Markdown
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
Copy Markdown
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