-
Notifications
You must be signed in to change notification settings - Fork 765
Add token flags parameter to token creation functions #2333
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
Conversation
| >T2 : Symbol(T2, Decl(assignmentCompatWithObjectMembersStringNumericNames.ts, 9, 46)) | ||
| ->'1.0' : Symbol(T2['1.0'], Decl(assignmentCompatWithObjectMembersStringNumericNames.ts, 10, 18)) | ||
| +>'1.0' : Symbol(T2["10"], Decl(assignmentCompatWithObjectMembersStringNumericNames.ts, 10, 18)) | ||
| +>'1.0' : Symbol(T2['10'], Decl(assignmentCompatWithObjectMembersStringNumericNames.ts, 10, 18)) |
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.
Nothing you did in this PR, but I bet this is a very simple oops somewhere
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.
Looking
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 adds a TokenFlags parameter to token creation functions in the AST factory to properly propagate quote styles and other token-specific formatting information throughout the codebase. Previously, token flags were set after creation, which could lead to inconsistencies.
Key Changes
- Modified token creation functions (
NewStringLiteral,NewNumericLiteral,NewBigIntLiteral,NewRegularExpressionLiteral,NewNoSubstitutionTemplateLiteral) to accept aflagsparameter - Token flags are now masked at construction time to preserve only relevant flags for each token type
- Updated all call sites to either propagate existing flags or use
TokenFlagsNonefor synthetic tokens - Test baselines now show consistent quote style preservation (single vs double quotes) in symbol output
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/ast/ast.go |
Core API changes: added flags parameter to token creation functions, updated Clone methods, and refactored createToken function to pass flags directly |
internal/parser/parser.go |
Parser now passes full token flags from scanner to token creation functions, removing post-creation flag assignments |
internal/transformers/tstransforms/runtimesyntax.go |
Propagates token flags when creating string/numeric literals from enum members |
internal/transformers/tstransforms/utilities.go |
Uses TokenFlagsNone for synthetic constant expressions |
internal/transformers/moduletransforms/utilities.go |
Preserves token flags when rewriting module specifiers |
internal/transformers/moduletransforms/externalmoduleinfo.go |
Uses TokenFlagsNone for synthetic helper imports |
internal/transformers/moduletransforms/esmodule.go |
Uses TokenFlagsNone for synthetic "module" string literal |
internal/transformers/moduletransforms/commonjsmodule.go |
Uses TokenFlagsNone for synthetic "__esModule" string |
internal/transformers/jsxtransforms/jsx.go |
Uses TokenFlagsNone for all synthetic JSX-related string literals |
internal/transformers/inliners/constenum.go |
Uses TokenFlagsNone for inlined const enum values |
internal/transformers/estransforms/objectrestspread.go |
Uses TokenFlagsNone for synthetic array index literals |
internal/transformers/estransforms/namedevaluation.go |
Uses TokenFlagsNone for synthetic name evaluation strings |
internal/transformers/declarations/transform.go |
Uses TokenFlagsNone for enum declaration values in .d.ts files |
internal/printer/factory.go |
Updated helper methods to use TokenFlagsNone for synthetic literals |
internal/printer/printer_test.go |
Updated test code to pass TokenFlagsNone |
internal/ls/codeactions_importfixes.go |
Preserves token flags when rewriting import module specifiers |
internal/ls/autoimports.go |
Uses quote preference to set appropriate token flags for new imports |
internal/ls/autoimportfixes.go |
Uses quote preference to set appropriate token flags for new imports |
internal/compiler/fileloader.go |
Uses TokenFlagsNone for synthetic helper imports |
internal/checker/nodebuilderimpl.go |
Properly handles quote style when creating property name literals from symbols |
internal/checker/emitresolver.go |
Uses TokenFlagsNone for literal constant values |
internal/checker/checker.go |
Uses TokenFlagsNone for synthetic element access literals |
testdata/baselines/reference/**/*.symbols |
Baseline updates showing consistent quote style preservation in symbol output |
testdata/baselines/reference/**/enumWithQuotedElementName1.js |
Shows proper quote preservation in emitted enum code |
testdata/baselines/reference/**/enumWithUnicodeEscape1.js |
Shows proper quote preservation in emitted enum code with unicode escapes |
| case ast.KindIdentifier: | ||
| return tx.Factory().NewStringLiteralFromNode(name) | ||
| case ast.KindStringLiteral: | ||
| return tx.Factory().NewStringLiteral(name.Text()) |
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.
We could preserve flags here, but we didn't in Strada (I think because cloneNode doesn't copy the singleQuote property, but haven't looked deeply), so if we propagate flags here it ends up producing a couple diffs.
| name = tryRenameExternalModule(factory, moduleName, sourceFile) | ||
| } | ||
| if name == nil { | ||
| name = factory.NewStringLiteral(moduleName.Text()) |
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.
Same as other comment above, we could preserve flags here but since Strada didn't, we end up with 190 new diffs if we try and preserve them now.
jakebailey
left a comment
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.
Worth leaving comments about those cases?
jakebailey
left a comment
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.
Awesome, all diff deletions
Also propagates token flags in the places where Strada did.
This could have helped prevent #2326, probably.