fix(22682): Fail on class declaration in statement position for ES2015 or higher #47216
Conversation
|
|
src/compiler/checker.ts
Outdated
| @@ -42870,6 +42870,9 @@ namespace ts { | |||
| } | |||
|
|
|||
| function checkGrammarClassLikeDeclaration(node: ClassLikeDeclaration): boolean { | |||
| if (languageVersion >= ScriptTarget.ES2015 && node.parent && isIfStatement(node.parent)) { | |||
There was a problem hiding this comment.
I don't think we want to gate this on the target.
There was a problem hiding this comment.
Can you provide some more guidance? :)
There was a problem hiding this comment.
Do you mean I should simply check always?
|
Hi I am sorry for the long time, I had examination session and some other stuff which took priority unfortunately :( I will start working back on this tomorrow, thanks also a lot for the review! |
|
Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs. |
Yes sure I'd like to try to solve the issue:) however I didn't fully understand the feedback, especially the first one, can you provide more guidance? |
nicdard
force-pushed
the
fix/error-for-class-declaration-in-statement
branch
4 times, most recently
from
97beb77 to
87ac77b
Compare
|
@DanielRosenwasser I think I addressed all the issues you mentioned in your review (thanks again), still not entirely sure on the first suggestion, but in case let me know so I will fix it :) |
nicdard
force-pushed
the
fix/error-for-class-declaration-in-statement
branch
from
87ac77b to
b0f56dd
Compare
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at b0f56dd. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based community code test suite on this PR at b0f56dd. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at b0f56dd. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at b0f56dd. You can monitor the build here. Update: The results are in! |
|
@jakebailey |
|
@jakebailey Here they are:Comparison Report - main..47216
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
nicdard
force-pushed
the
fix/error-for-class-declaration-in-statement
branch
from
b0f56dd to
8afc088
Compare
|
@typescript-bot user test this inline |
|
Heya @jakebailey, I've started to run the diff-based community code test suite on this PR at 8afc088. You can monitor the build here. |
|
Hm, this fails quite a bit; I don't think you ran the tests before sending. I think there's more things to check than just |
Yeah sorry for this, I was warning about in the thread: Oh cool thanks for the hint! |
Actually I have taken a better look at the tests that are failing, and there are cases such: /// anonClassDeclarationEmitsAnon.ts
// @declaration: true
// @filename: wrapClass.ts
export function wrapClass(param: any) {
return class Wrapped {
foo() {
return param;
}
}
}or /// classExpressionWithStaticPropertiesES61.ts
//@target: es6
var v = class C {
static a = 1;
static b = 2;
static c = C.a + 3;
};I think we should test first if the node is a class statement then, so we avoid all those errors on class expressions, am I on the right track? |
|
Clearly I was wrong about the rules, then. This code executes today: Playground Link Probably worth testing all of the cases and see what's actually disallowed? |
Maybe the intution was ok, but just regarding |
nicdard
force-pushed
the
fix/error-for-class-declaration-in-statement
branch
from
8afc088 to
721a4b5
Compare
src/compiler/checker.ts
Outdated
| function isBlockLike(node: Node): node is BlockLike { | ||
| switch (node.kind) { | ||
| case SyntaxKind.Block: | ||
| case SyntaxKind.SourceFile: | ||
| case SyntaxKind.ModuleBlock: | ||
| case SyntaxKind.CaseClause: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
This should probably have been moved from extractSymbol.ts into src/compiler/utilities.ts rather than being copied and pasted here.
There was a problem hiding this comment.
Yeah I was trying the tests, then fixing the PR, maybe I can change it to Draft while experimenting :) Sorry for this, it's just my computer really cannot run the tests without exploding 😅 Is there any tip to run them locally without overwhelming it?
|
|
||
| while (class C {}) { // Not an error. | ||
| break; | ||
| } |
There was a problem hiding this comment.
This file should also test the other missing parents, e.g. case, default, etc.
|
I'll change this to draft so I can work on it without disturbing, as soon as I think it is ready I will reopen it |
nicdard
force-pushed
the
fix/error-for-class-declaration-in-statement
branch
from
721a4b5 to
24ba578
Compare
Fixes #22682
The error should be reported only in ES2015 or higher, therefore I changed the
binderinstead of theparserwhich needs still to parse this sequence to allow older standards.In this PR I am checking only class declarations, since I am not sure from the conversation in the issue which is the required behaviour for enums.
Since this is my first contribution, I am not sure about the test coverage. In particular, I am not sure on how to generate the
.error.txtfile which I need to generate by running directly (calling tsc) the updated version of the compiler against the newly createdscopeClassDeclaration.tstest file.