Jsdoc ... binds tighter than postfix ?!#18117
Conversation
1. Previously ...X? mistakenly parsed as ...(X?) instead of (...X)? 2. Previously X?!?!? mistakenly failed to parse the postfix tokens ? ! ? ! ? at the same level of precedence. The fix is to 1. Make ... parsing call parseNonArrayType instead of parseType. 2. Make postfix jsdoc parsing parse in a loop instead of only parsing one token.
weswigham
left a comment
There was a problem hiding this comment.
Certainly an improvement - do we have tests covering cases like ...foo[], ...foo?[]!, and ...foo![]? (mostly so we know what they parse to)?
This combines parseArrayType and parseJSDocPostfixType into parsePostfixType.
|
Good idea. Those tests failed to parse the trailing postfix jsdoc type, so I combined |
|
I don't know how to read the title of this issue without laughing. |
| function parsePostfixTypeOrHigher(): TypeNode { | ||
| let kind: SyntaxKind | undefined; | ||
| let type = parseNonArrayType(); | ||
| while (!scanner.hasPrecedingLineBreak() && (kind = getPostfixTypeKind(token()))) { |
There was a problem hiding this comment.
Do an explicit === undefined check here because who knows what a SyntaxKind of 0 is.
There was a problem hiding this comment.
Potentally just move the defined-ness check into the body.
There was a problem hiding this comment.
who knows what a SyntaxKind of 0 is
SyntaxKind.Unknown
(good idea, I'll switch to an undefined check)
There was a problem hiding this comment.
Actually, returning 0 is even better since 0 is falsy too.
| case SyntaxKind.QuestionToken: | ||
| return SyntaxKind.JSDocNullableType; | ||
| case SyntaxKind.OpenBracketToken: | ||
| return SyntaxKind.ArrayType; |
There was a problem hiding this comment.
This is slightly misleading - in actuality, you are potentially parsing out an IndexedAccessTypeNode. Is it fair to say that this function determines gives you a JSDocPostfixType, a TypeScriptPostfixType, or None?
There was a problem hiding this comment.
What if you just inlined this switch/case to the function itself?
There was a problem hiding this comment.
Inlining makes the jsdoc cases really inconvenient because the mapping between tokens and jsdoc types has to happen in the switch, but the createNode code is shared between all three. I could separate out just the token → jsdoc-type mapping code, but that would essentially duplicate the switch.
|
@DanielRosenwasser can you take another look? |
| switch (tokenKind) { | ||
| case SyntaxKind.EqualsToken: | ||
| // only parse postfix = inside jsdoc, because it's ambiguous elsewhere | ||
| return contextFlags & NodeFlags.JSDoc ? SyntaxKind.JSDocOptionalType : 0; |
There was a problem hiding this comment.
If you are trying to prevent boxing, there's a SyntaxKind.Unknown
There was a problem hiding this comment.
I was trying to prevent boxing and SyntaxKind.Unknown at the same time. Actually, there's not much point, because 0 requires a falsy check later, and functions automatically union their return types these days. I might as well return false here.
Inline the switch, but extract the function that creates jsdoc nodes.
|
Thanks to some discussion with @rbuckton, I inlined the switch into |
|
Travis failed because my quickfix-fix now reports more fixes in a fourslash test that has precedence changes from this PR. I need to merge from master and update the test. |
|
OK, I merged from master and updated the new test. |
Fixes #18104
? ! ? ! ? at the same level of precedence.
The fix is to