fix(4048): fix declaration emit for new keyword used as property names#4052
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage for declaration emit around keyword property names and adjusts type-node building to ensure a method named new is emitted as a quoted property name (not a construct signature), updating/removing related baselines.
Changes:
- Added a new compiler test case for keyword property names (
new,delete,break,continue) in object literals andas const. - Updated pseudo type-to-AST node construction to special-case method names equal to
newand emit them as string-literal property names. - Cleaned up accepted/baseline outputs related to
emitMethodCalledNew.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/tests/cases/compiler/declarationEmitKeywordPropertyNames.ts | New test to validate declaration emit for keyword property names. |
| testdata/submoduleAccepted.txt | Removes an accepted baseline diff entry that should no longer be produced. |
| testdata/baselines/reference/submoduleAccepted/compiler/emitMethodCalledNew.js.diff | Removes an obsolete accepted diff baseline. |
| testdata/baselines/reference/submodule/compiler/emitMethodCalledNew.js | Updates expected output to emit "new" instead of ["new"]. |
| testdata/baselines/reference/compiler/declarationEmitKeywordPropertyNames.* | Adds new baselines for the new keyword-property-names test. |
| internal/checker/pseudotypenodebuilder.go | Special-cases method name new to emit a string literal property name node. |
| internal/checker/nodecopy.go | Simplifies reuseName logic by removing new-specific handling and always normalizing string-literals to identifiers. |
Comments suppressed due to low confidence (1)
internal/checker/nodecopy.go:1
- The previous logic intentionally avoided normalizing the string literal
"new"into an identifier because, in method-signature contexts,new()is interpreted as a construct signature (different meaning than a method named"new"). The PR moves the special-casing intopseudoTypeToNodefor one specific path, butreuseNameis a shared helper; with this change, any other caller that reuses"new"as a method name risks emitting a construct signature again. Consider restoring an explicit guard inreuseNameto not normalize"new"(and/or introducing a context-aware helper likereusePropertyNameForMethodvsreusePropertyNameForProperty) so the invariant is enforced centrally rather than depending on individual call sites.
package checker
weswigham
left a comment
There was a problem hiding this comment.
I'm not sure this is quite right - or rather, sufficient. reuseNode is over-quoting new right now, sure, and this does cut it down - but only quoting news that come from method names in the pseudochecker presupposes that we only transform like for like, which is probably mostly true, but might not be always end up being true. I'd rather the approach we use make it harder to miss if we add more method-transformed reuse locations. Also, I can already see this PR still over-quoting by quoting the new property signature in the isConst case when it doesn't need really to.
Honestly, if we want the reused node to always match the rules we use for synthesized nodes, we should be mirroring createPropertyNameNodeForIdentifierOrLiteral exactly inside reuseName, including passing in if the target location is a method. There's definitely a part of me that would love to extract the actual logic (vs the resultant building of new vs reused nodes) into a helper and using it for both (eg, a function that returns an enum indicating if the name can safely be be an Identifier, NumericLiteral, or StringLiteral). I know numeric names are probably in a weird place re: node reuse matching synthetic output right now because of the diff between the two.
…deForIdentifierOrLiteral
Fixes #4048