Skip to content

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Aug 31, 2025

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:

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.

@github-actions github-actions bot added A-semantic Area - Semantic C-bug Category - Bug labels Aug 31, 2025
Copy link
Member Author

overlookmotel commented Aug 31, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@graphite-app graphite-app bot changed the base branch from 08-31-fix_semantic_allow_super_in_object_literal_method_inside_a_class to graphite-base/13499 August 31, 2025 21:06
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 31, 2025

CodSpeed Instrumentation Performance Report

Merging #13499 will not alter performance

Comparing 08-31-fix_semantic_handle_edge_cases_checking_super_ (9fa551a) with main (bf50a02)1

Summary

✅ 9 untouched benchmarks
🆕 28 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 formatter[RadixUIAdoptionSection.jsx] N/A 572 µs N/A
🆕 formatter[binder.ts] N/A 22.6 ms N/A
🆕 formatter[cal.com.tsx] N/A 206.8 ms N/A
🆕 formatter[react.development.js] N/A 11.5 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 20 µs N/A
🆕 lexer[binder.ts] N/A 870.1 µs N/A
🆕 lexer[cal.com.tsx] N/A 5.3 ms N/A
🆕 lexer[react.development.js] N/A 356.8 µs N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.4 ms N/A
🆕 linter[binder.ts] N/A 141.9 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[react.development.js] N/A 50.3 ms N/A
🆕 mangler[RadixUIAdoptionSection.jsx] N/A 11.5 µs N/A
🆕 mangler[binder.ts] N/A 734.1 µs N/A
🆕 mangler[cal.com.tsx] N/A 2.8 ms N/A
🆕 mangler[react.development.js] N/A 259.8 µs N/A
🆕 minifier[binder.ts] N/A 3.8 ms N/A
🆕 minifier[cal.com.tsx] N/A 31 ms N/A
🆕 minifier[react.development.js] N/A 2.4 ms N/A
🆕 semantic[RadixUIAdoptionSection.jsx] N/A 77.7 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Footnotes

  1. No successful run was found on main (9fa551a) during the generation of this report, so bf50a02 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@graphite-app graphite-app bot force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from 7f600f3 to 51da4a6 Compare August 31, 2025 21:13
@graphite-app graphite-app bot force-pushed the graphite-base/13499 branch from 185fc16 to dc9645f Compare August 31, 2025 21:13
@graphite-app graphite-app bot changed the base branch from graphite-base/13499 to main August 31, 2025 21:14
@graphite-app graphite-app bot force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from 51da4a6 to f3fed3e Compare August 31, 2025 21:14
@overlookmotel overlookmotel force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from f3fed3e to 5a4d4a5 Compare September 1, 2025 00:41
@overlookmotel overlookmotel marked this pull request as ready for review September 1, 2025 00:56
Copilot AI review requested due to automatic review settings September 1, 2025 00:56
Copy link
Contributor

Copilot AI left a 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 super inside and outside classes into a single comprehensive function
  • Added proper handling of nested classes and their scope interactions
  • Implemented correct validation for super in 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.

@overlookmotel overlookmotel force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch 2 times, most recently from 06d849e to e5df62c Compare September 1, 2025 01:52
@overlookmotel
Copy link
Member Author

overlookmotel commented Sep 1, 2025

I think we should run monitor-oxc on this PR before merging.

The question is can TSTypeAnnotation contain Super? e.g. something like:

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 super as an IdentifierReference, not Super. But if it's possible that super in a type annotation can get parsed as Super in any weird edge case, we should handle it in semantic to avoid a panic for such code. I have a commit ready which adds handling for it, but I'd like to see if any such patterns exists in ecosystem first, to inform whether the handling logic is correct.

@overlookmotel overlookmotel marked this pull request as draft September 1, 2025 09:14
@overlookmotel
Copy link
Member Author

overlookmotel commented Sep 1, 2025

No new errors in monitor-oxc, so Super in TSIndexSignature isn't present in any of those examples. But I've added a commit which handles it, just to be on safe side.

@overlookmotel overlookmotel marked this pull request as ready for review September 1, 2025 10:47
@overlookmotel overlookmotel force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from 07afd4a to 74ec2d3 Compare September 1, 2025 11:30
@overlookmotel
Copy link
Member Author

OK, I found one weird case where super in a TypeAnnotation is parsed as Super (though I don't know if it should be):

class C {
  [keys: string]: typeof import('x', { with: super.foo }).y;
}

So we do need to handle it (which this PR now does).

@Dunqing Dunqing self-assigned this Sep 8, 2025
@overlookmotel overlookmotel force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from 1767a6b to 8f0d4e6 Compare September 8, 2025 10:49
@overlookmotel overlookmotel force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch 2 times, most recently from ea9f930 to 3f62444 Compare September 9, 2025 02:01
@Dunqing
Copy link
Member

Dunqing commented Sep 10, 2025

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.

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Sep 10, 2025
Copy link
Member

Dunqing commented Sep 10, 2025

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.
@graphite-app graphite-app bot force-pushed the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch from 3f62444 to 9fa551a Compare September 10, 2025 03:23
@graphite-app graphite-app bot merged commit 9fa551a into main Sep 10, 2025
28 checks passed
@graphite-app graphite-app bot deleted the 08-31-fix_semantic_handle_edge_cases_checking_super_ branch September 10, 2025 03:28
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-semantic Area - Semantic C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

semantic: discrepancies in super diagnostics compared to TypeScript

3 participants