-
Notifications
You must be signed in to change notification settings - Fork 12.8k
fix(46433): Unhandled Error for using "enum" as a parameter #46459
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
Conversation
@@ -2611,7 +2611,10 @@ namespace ts { | |||
case ParsingContext.ObjectLiteralMembers: return parseErrorAtCurrentToken(Diagnostics.Property_assignment_expected); | |||
case ParsingContext.ArrayLiteralMembers: return parseErrorAtCurrentToken(Diagnostics.Expression_or_comma_expected); | |||
case ParsingContext.JSDocParameters: return parseErrorAtCurrentToken(Diagnostics.Parameter_declaration_expected); | |||
case ParsingContext.Parameters: return parseErrorAtCurrentToken(Diagnostics.Parameter_declaration_expected); | |||
case ParsingContext.Parameters: | |||
return isKeyword(token()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a possible reason why we didn't do this in another PR? Maybe @JoshuaKGoldberg knows?
I'm suspicious that it had something to do with modifiers and/or contextual keywords (e.g. public
/private
/protected
/readonly
(/override
?) on parameter properties).
Otherwise the change seems good except for the fact that we bail out and start trying to parse an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't a case I considered in #43005, no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... except for the fact that we bail out and start trying to parse an enum.
@DanielRosenwasser Yes, you are right, I thought about this case and decided to follow behavior similar to variable declarations - Playground. Where TS tries to parse the enum
(or other keywords) keyword as a variable name and then tries to find a name for the enum
. Do we need to change that?
@@ -10,4 +10,4 @@ var [debugger, if] = [1, 2]; | |||
enum void {} | |||
function f(default: number) {} | |||
class C { m(null: string) {} } | |||
|
|||
function f1(enum) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact, this PR also changes the error message for a parameter named class
in a similar way: a better error message, then an attempt to parse a class.
function f2(class) {}
- Parameter declaration expected.
+ 'class' is not allowed as a parameter name.
'{' expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. @a-tarasyuk can we include this as a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewbranch Sure. I've updated the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, I thought about this case and decided to follow behavior similar to variable declarations - Playground. Where TS tries to parse the enum (or other keywords) keyword as a variable name and then tries to find a name for the enum. Do we need to change that?
I wouldn't be comfortable making a bigger change at this point in the release cycle, but this change looks safe and 100% better than before.
FIxes #46433