Elide in/out variance modifiers from emitted JS#4156
Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
in/out variance modifiers from emitted JS
There was a problem hiding this comment.
Pull request overview
Fixes a regression where in/out variance modifiers (grammar errors when placed on class members rather than type parameters) were copied verbatim into emitted JavaScript. The TypeEraserTransformer now elides these tokens while preserving the in binary operator and for…in semantics.
Changes:
- Add an
InKeyword/OutKeywordcase inTypeEraserTransformer.visitthat elides the token unless its parent is aBinaryExpression. - Add a compiler test case (with
.js,.types,.symbols,.errors.txtbaselines) covering misplaced variance modifiers alongsidein/for…inusage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/transformers/tstransforms/typeeraser.go | Elides KindInKeyword/KindOutKeyword modifier tokens, guarded against the in binary operator via parent-node check. |
| testdata/tests/cases/compiler/varianceModifiersOnClassMembers.ts | New test covering elision plus in operator/for…in preservation. |
| testdata/baselines/reference/compiler/varianceModifiersOnClassMembers.js | Baseline showing variance modifiers stripped, in/for…in retained. |
| testdata/baselines/reference/compiler/varianceModifiersOnClassMembers.{types,symbols,errors.txt} | Type/symbol/diagnostic baselines (TS1274 still reported). |
|
Seems fine but I don't think we guarantee syntactic validity of output JS in the presence of malformed TS, which this is? I would have punted the original bug Same category as which emits as-is (syntax error in, syntax error out) |
|
A while back I added something to our test harness that verified that all output code parses, and it does; I think surprisingly it was an invariant, we just were lacking many tests. |
|
Yeah seems like a desirable property to have, even if Strada didn't have it |
in/outvariance modifiers are only valid on type parameters, but when written (a grammar error) on a class member they were copied verbatim into the emitted JavaScript:Changes
internal/transformers/tstransforms/typeeraser.go:TypeEraserTransformernow elidesKindInKeyword/KindOutKeywordmodifier tokens alongside the other TypeScript-only modifiers. SinceKindInKeywordis shared with theinbinary operator, the token is only elided when its parent is not aBinaryExpression, soa in bandfor…inare left untouched.testdata/tests/cases/compiler/varianceModifiersOnClassMembers.ts(+ baselines): covers variance modifiers on class members together with theinoperator andfor…into guard against over-elision.The grammar diagnostic (
TS1274) is unaffected and still reported.