-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Fix 'arguments' check in class field initializer or static initialization block #44393
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
src/compiler/checker.ts
Outdated
|
|
||
| const firstFunctionLikeContainerOrClassLikeContainer = findAncestor(node.parent, node => isClassLike(node) || isFunctionLike(node))!; | ||
| if (firstFunctionLikeContainerOrClassLikeContainer.kind === SyntaxKind.ClassExpression || firstFunctionLikeContainerOrClassLikeContainer.kind === SyntaxKind.ClassDeclaration) { | ||
| error(node, Diagnostics.arguments_is_not_allowed_in_class_field_initializer_or_static_initialization_block); |
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.
And it should not execute the following two lines.
You should add
return errorType;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.
LGTM
DanielRosenwasser
left a comment
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.
Technically there's an isInPropertyInitializer but the suggestions I've given seem cheaper.
src/compiler/diagnosticMessages.json
Outdated
| "category": "Error", | ||
| "code": 2812 | ||
| }, | ||
| "'arguments' is not allowed in class field initializer or static initialization block.": { |
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.
There's no static initialization block, right?
| "'arguments' is not allowed in class field initializer or static initialization block.": { | |
| "'arguments' cannot be referenced in property initializers.": { |
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.
Oh, the error message is copied from Chrome. But Chrome does not support static initialization block either, that's strange.
src/compiler/checker.ts
Outdated
| } | ||
| } | ||
|
|
||
| const firstFunctionLikeContainerOrClassLikeContainer = findAncestor(node.parent, node => isClassLike(node) || isFunctionLike(node))!; |
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.
Consider moving this higher and reusing it for container
const firstFunctionLikeContainerOrClassLikeContainer = findAncestor(node.parent, node => isClassLike(node) || isFunctionLike(node))!;
const container = isClassLike(firstFunctionLikeContainerOrClassLikeContainer) ?
findAncestor(firstFunctionLikeContainerOrClassLikeContainer.parent, isFunctionLike)! :
firstFunctionLikeContainerOrClassLikeContainer;fix Diagnostics message
|
@DanielRosenwasser |
|
only node14 failed,werid |
sandersn
left a comment
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.
Looks good to me -- what do you think @DanielRosenwasser ?
@exoticknight you can merge now from main to get diagnosticMessages up to date, or you can wait until @DanielRosenwasser signs off, it's up to you.
# Conflicts: # src/compiler/diagnosticMessages.json
|
I looked over the comment history and I think this is ready to go once CI finishes one more time. |
Fixes #44269