-
Notifications
You must be signed in to change notification settings - Fork 685
Port class fields transformer #1542
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
base: main
Are you sure you want to change the base?
Port class fields transformer #1542
Conversation
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.
It's ambitious to take this on - this is one of the largest transforms in the TS codebase. Correspondingly, it's kinda difficult to judge how "right" this is without all of it in place. I can say "great, one less diff" - but with tests failing with unaccepted diffs it's a 🤷♂️ if it's actually net-positive, even for just the currently claimed logic. Still, aside from the spelling errors copilot picked up on, I see a few other fixable issues:
- The substitution pass is no more (intentionally). Anything referencing them needs to be reimplemented with direct transformations. Usually not a problem, but it's not a direct port of the substitution logic like I partially see here.
- Closures. Closures need to be avoided, generally - we used them liberally in TS, but in the
go
codebase they've proven to generally have an outsized performance cost. Most, if not all closures, and auto-closing function/method references, need to be replaced with either static function refs or captured a single time on transform creation. Or just eliminated entirely. - This falls more into the "open questions" category - in the TS repo, we unconditionally execute a single class fields transform that has a bunch of internal switches for param properties,get/set semantics, auto accessors, private name support, static private name support, etc. While I did stub out a single empty transform, it might ultimately make more sense to have each of those sub-features all in separate transformers for the sake of being able to modularly compose them sensibly (moving the configuration mostly out of the transformer logic and into initial transformer selection). We've broken down other transforms by reasonably logical feature in this way already, and some functionality has already moved, as noted, to other transformers, like parameter properties. I'd probably prefer more subdivision here, but it's a bit of extra work. Doing so might make piecemeal implementations like this a bit easier to digest one bit at a time, though.
Hey @weswigham ,thanks for the detailed feedback! As I mentioned earlier, I only realized during implementation just how complex this transformer is — far beyond my initial expectation. As the code size grew, I started to suspect I might be heading in the wrong direction. My initial intent was to fix a single test case and then immediately pause, using this PR as a way to “test the waters” and invite feedback from existing contributors so we could adjust the approach early. Even if this PR doesn’t get merged, I completely understand. I really appreciate the suggestions and review. I’ll take some time to get a deeper feel for the current design philosophy, and then explore whether it’s possible to narrow this down to a well-scoped subdivision so that any contribution stays aligned and ensures we’re on the same page. |
78458b3
to
3784bab
Compare
3784bab
to
fdab161
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 ports the classFieldsTransformer
from Strada into the current codebase to improve TypeScript private field transformation compatibility. The primary change involves transforming private class fields from ES class field syntax to WeakMap-based implementations with constructor initialization. This addresses compatibility issues identified in test case privateNameHashCharName
and brings the output closer to the TypeScript reference implementation.
Key changes:
- Ports class fields transformer logic for private field handling
- Transforms private field declarations to WeakMap declarations and constructor initialization
- Updates helper function calls for private field access
- Reduces diff between local and reference test outputs
Reviewed Changes
Copilot reviewed 165 out of 167 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
testdata/baselines/reference/submodule/conformance/privateNameHashCharName.js.diff | Shows the removal of diff output, indicating successful transformation alignment |
testdata/baselines/reference/submodule/conformance/privateNameHashCharName.js | Main test case demonstrating transformed private field implementation |
Various other test baseline files | Additional test cases showing consistent private field transformation patterns |
Comments suppressed due to low confidence (2)
testdata/baselines/reference/submodule/conformance/privateNameFieldDestructuredBinding(target=es2015).js:38
- Using
__classPrivateFieldGet
in destructuring assignment target is invalid syntax. Destructuring assignments to private fields should use setter patterns or be transformed to regular assignments after destructuring.
if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot write private member to an object whose class did not declare it");
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
const g = ++(__classPrivateFieldGet(this, _C_test, "f")); | ||
const h = --(__classPrivateFieldGet(this, _C_test, "f")); | ||
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; ++(__classPrivateFieldGet(this, _C_test, "f"))) { } | ||
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; (__classPrivateFieldGet(this, _C_test, "f"))++) { } |
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.
The unary increment operation __classPrivateFieldGet(this, _C_test, "f")++;
is invalid JavaScript syntax. Private field mutations should use the get/set helper pattern like __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f")
to properly update the WeakMap.
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; (__classPrivateFieldGet(this, _C_test, "f"))++) { } | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
const a = __classPrivateFieldGet(this, _C_test, "f"); | |
__classPrivateFieldSet(this, _C_test, a + 1, "f"); | |
const b = __classPrivateFieldGet(this, _C_test, "f"); | |
__classPrivateFieldSet(this, _C_test, b - 1, "f"); | |
const c = __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
const d = __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; ++__classPrivateFieldGet(this, _C_test, "f")) { } | |
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; __classPrivateFieldGet(this, _C_test, "f")++) { } | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
__classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
const e = __classPrivateFieldGet(this, _C_test, "f"); | |
__classPrivateFieldSet(this, _C_test, e + 1, "f"); | |
const f = __classPrivateFieldGet(this, _C_test, "f"); | |
__classPrivateFieldSet(this, _C_test, f - 1, "f"); | |
const g = __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f"); | |
const h = __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") - 1, "f"); | |
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f")) { } | |
for (__classPrivateFieldSet(this, _C_test, 0, "f"); __classPrivateFieldGet(this, _C_test, "f") < 10; __classPrivateFieldSet(this, _C_test, __classPrivateFieldGet(this, _C_test, "f") + 1, "f")) { } |
Copilot uses AI. Check for mistakes.
} | ||
t2(p) { | ||
this.#p = p; | ||
__classPrivateFieldGet(this, _Foo_p, "f") = p; | ||
} |
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.
The assignment __classPrivateFieldGet(this, _Foo_p, "f") = p;
is invalid JavaScript syntax. Private field assignments should use __classPrivateFieldSet(this, _Foo_p, p, "f")
instead.
Copilot uses AI. Check for mistakes.
this.#fieldFunc(); | ||
(_a = this.#fieldFunc) === null || _a === void 0 ? void 0 : _a.call(this); | ||
const func = this.#fieldFunc; | ||
__classPrivateFieldGet(this, _A_fieldFunc, "f")(); |
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.
The function call pattern __classPrivateFieldGet(this, _A_fieldFunc, "f")()
loses the correct this
binding. It should be __classPrivateFieldGet(this, _A_fieldFunc, "f").call(this)
to maintain proper context.
__classPrivateFieldGet(this, _A_fieldFunc, "f")(); | |
__classPrivateFieldGet(this, _A_fieldFunc, "f").call(this); |
Copilot uses AI. Check for mistakes.
Hi @weswigham I’ve updated this PR based on the earlier review feedback. The key change is introducing a dedicated At this stage, I’ve only implemented part of the logic as a proof of concept (not all cases yet). Could you take another look and let me know if this seems to be heading in the right direction? |
Summary
This PR ports the
classFieldsTransformer
from Strada into the current codebase.Details
The motivation for this change originates from this issue. However, after digging into the implementation, it became clear that the scope of work is significantly more complex than initially expected.
At this stage, the PR fixes the transformation logic for the
privateNameHashCharName
test case. Please check 78458b3Before proceeding further, I'd like to confirm that this direction aligns with project goals and that this contribution is welcome. Once confirmed, I plan to continue addressing the remaining related test cases.
Test
This PR reduces the test result difference between the Strada and Corsa, brining them closer to parity.