improve error info for arrow function at right of binary expression#26085
Closed
Kingwl wants to merge 1 commit intomicrosoft:masterfrom
Closed
improve error info for arrow function at right of binary expression#26085Kingwl wants to merge 1 commit intomicrosoft:masterfrom
Kingwl wants to merge 1 commit intomicrosoft:masterfrom
Conversation
DanielRosenwasser
requested changes
Aug 22, 2018
| return nextToken() === SyntaxKind.SlashToken; | ||
| } | ||
|
|
||
| function isStartOfSimpleArrowFunction () { |
Member
There was a problem hiding this comment.
Nit: not space before parentheses
| function isStartOfSimpleArrowFunction () { | ||
| if (token() === SyntaxKind.AsyncKeyword) { | ||
| nextToken(); | ||
| return isIdentifier() && nextToken() === SyntaxKind.EqualsGreaterThanToken || token() === SyntaxKind.EqualsGreaterThanToken; |
Member
There was a problem hiding this comment.
Can you leave a comment on the intent here? Is this supposed to cover async => 100?
| "category": "Error", | ||
| "code": 17017 | ||
| }, | ||
| "Invalid arrow-function arguments, parentheses around the arrow-function may help.": { |
Member
There was a problem hiding this comment.
Arrow functions are not syntactically valid here. Consider wrapping the function in parentheses.
| ~ | ||
| !!! error TS1109: Expression expected. | ||
| true || async => 1; | ||
| ~~~~~~~~~~~ |
Member
There was a problem hiding this comment.
You really don't want the error span to include whitespace.
| const maybeArrow = tryParseAsyncSimpleArrowFunctionExpression() || lookAhead(() => parseSimpleArrowFunctionExpression(<Identifier>parseBinaryExpressionOrHigher(/*precedence*/ 0))); | ||
| parseErrorAtRange(maybeArrow, Diagnostics.Invalid_arrow_function_arguments_parentheses_around_the_arrow_function_may_help); | ||
| } | ||
| leftOperand = makeBinaryExpression(leftOperand, op, parseBinaryExpressionOrHigher(newPrecedence)); |
Member
There was a problem hiding this comment.
You're currently ignoring it if it's present, but I think that you actually want to gracefully parse the arrow functions you've been able to find above.
Throwing away the result and trying to parse as before actually provides a slightly more confusing error experience.
| } | ||
| else { | ||
| leftOperand = makeBinaryExpression(leftOperand, parseTokenNode(), parseBinaryExpressionOrHigher(newPrecedence)); | ||
| const op = parseTokenNode<Token<BinaryOperator>>(); |
Member
There was a problem hiding this comment.
Leave a comment like
// Much of the time, users will write code along the lines of
//
// let x = foo || () => { /*...*/ }
//
// However, arrow functions aren't valid in those positions in the ECMAScript grammar.
// Despite that, we'll try to parse it out anyway and give a decent error message.
Member
|
Many unaddressed comments - please open a fresh PR if desired. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25898