- Notifications
You must be signed in to change notification settings - Fork12.9k
fix(61867): fix find-all-refs for constructors involving string literals#61869
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 ourterms of service andprivacy 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?
Conversation
| isLiteralNameOfPropertyDeclarationOrIndexAccess(str) || | ||
| isNameOfModuleDeclaration(node) || | ||
| isExpressionOfExternalModuleImportEqualsDeclaration(node) || | ||
| (isCallExpression(node.parent) && isBindableObjectDefinePropertyCall(node.parent) && node.parent.arguments[1] === node) || | ||
| isImportOrExportSpecifier(node.parent) | ||
| ); | ||
| ) && str.text.length === searchSymbolName.length; |
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.
this might fix the issue at hand but this check could be seen as (minor) perf optimization - it should be cheaper to perform this length-based check first
the true issue here, I feel, is that something that claims to be a StringLiteral... doesn't behave like one. AStringLiteralLike must have.text - it's an invariant that is broken here.
I feel like this is closer to what the proper fix should look like:Andarist@38551b1
But I'd fully expect it to need some adjustments. What I don't feel confident enough:
- should this be somehow also passed to object allocators?
- maybe the
textassignment should be handled byaddSyntheticNodesas that's a function that deals with the scanner - and maybe then a custom/extra
StringLiteralObjectwouldn't be needed? One could cast and assign.textto the createdTokenObject - obviously the proposed fix might require some extra other properties to be set - what about
singleQuote? Is it safely omittable? - it feels like string literals wouldn't necessarily be the only token kinds that might be broken right now
Fixes#61867
Fixregression infix-all-refs for constructors with string literals by moving the length check after node kind checks