-
-
Notifications
You must be signed in to change notification settings - Fork 726
fix(semantic): handle edge cases checking super
#13499
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
fix(semantic): handle edge cases checking super
#13499
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #13499 will not alter performanceComparing Summary
Benchmarks breakdown
Footnotes |
7f600f3 to
51da4a6
Compare
185fc16 to
dc9645f
Compare
51da4a6 to
f3fed3e
Compare
f3fed3e to
5a4d4a5
Compare
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.
Pull Request Overview
This PR fixes semantic analysis of super() and super.prop usage in JavaScript/TypeScript classes by implementing a complete rewrite of the super checking logic. The fix addresses edge cases where super was incorrectly allowed or disallowed in various contexts.
Key changes:
- Unified the checking logic for
superinside and outside classes into a single comprehensive function - Added proper handling of nested classes and their scope interactions
- Implemented correct validation for
superin class property values, computed keys, decorators, and static blocks
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
06d849e to
e5df62c
Compare
|
I think we should run The question is can class S { foo: string = 'bar'; }
class C extends S { [keys: typeof super.foo]: typeof super.foo }At present our parser parses this without error, but parses |
|
No new errors in |
07afd4a to
74ec2d3
Compare
|
OK, I found one weird case where class C {
[keys: string]: typeof import('x', { with: super.foo }).y;
}So we do need to handle it (which this PR now does). |
1767a6b to
8f0d4e6
Compare
ea9f930 to
3f62444
Compare
|
I've looked into the mismatch case, which is related to the legacy decorator, and it probably doesn't matter now because it only affects class expressions, but the legacy decorator doesn't support transforming class expressions with decorators. |
Merge activity
|
Fixes #13284. Fix semantic's checking of `super()` and `super.prop`. This fixes various TS test cases, notably correctly identifying that these are all illegal: ```js class C { prop = function() { super.foo }; [super.foo]() {} @super.foo method() {} }; ``` It also fixes various edge cases related to classes nested within other classes that aren't covered by existing conformance tests. It's pretty much a complete re-write. I've combined the 2 paths for in a class and not in a class into one, to avoid repeated code. This PR includes pass + fail test fixtures which include `super` in every conceivable position I could think of. There are so many errors in the fail test fixtures, that I had to write a script to check the snapshot includes them all! Hopefully this solves this issue for once and for all! Note: The large pass fixture seems to be revealing bugs somewhere in transformer - there's a mismatch, as well as lots of wrong scopes. But that's incidental to this PR, and can be addressed separately.
3f62444 to
9fa551a
Compare

Fixes #13284.
Fix semantic's checking of
super()andsuper.prop.This fixes various TS test cases, notably correctly identifying that these are all illegal:
It also fixes various edge cases related to classes nested within other classes that aren't covered by existing conformance tests.
It's pretty much a complete re-write. I've combined the 2 paths for in a class and not in a class into one, to avoid repeated code.
This PR includes pass + fail test fixtures which include
superin every conceivable position I could think of. There are so many errors in the fail test fixtures, that I had to write a script to check the snapshot includes them all!Hopefully this solves this issue for once and for all!
Note: The large pass fixture seems to be revealing bugs somewhere in transformer - there's a mismatch, as well as lots of wrong scopes. But that's incidental to this PR, and can be addressed separately.