Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −2
Open

fix(61867): fix find-all-refs for constructors involving string literals #61869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/services/findAllReferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1838,13 +1838,13 @@ export namespace Core {
case SyntaxKind.NoSubstitutionTemplateLiteral:
case SyntaxKind.StringLiteral: {
const str = node as StringLiteralLike;
return str.text.length === searchSymbolName.length && (
return (
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;
Copy link
Contributor

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. A StringLiteralLike 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 text assignment should be handled by addSyntheticNodes as that's a function that deals with the scanner
  • and maybe then a custom/extra StringLiteralObject wouldn't be needed? One could cast and assign .text to the created TokenObject
  • 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

}

case SyntaxKind.NumericLiteral:
Expand Down
82 changes: 82 additions & 0 deletions tests/baselines/reference/findAllRefsConstructor.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// === findAllReferences ===
// === lib.d.ts ===
// --- (line: --) skipped ---
//
// interface Object {
// /** The initial value of Object.prototype.constructor is the standard built-in Object constructor. */
// <|[|{| isWriteAccess: true |}constructor|]: Function;|>
//
// /** Returns a string representation of an object. */
// toString(): string;
// --- (line: --) skipped ---

// === /tests/cases/fourslash/findAllRefsConstructor.ts ===
// class A {
// 'constructor'() { }
// }
// const a = new A()
// console.log(a.[|constructor|]/*FIND ALL REFS*/)

// === Definitions ===
// === lib.d.ts ===
// --- (line: --) skipped ---
//
// interface Object {
// /** The initial value of Object.prototype.constructor is the standard built-in Object constructor. */
// <|[|constructor|]: Function;|>
//
// /** Returns a string representation of an object. */
// toString(): string;
// --- (line: --) skipped ---

// === Details ===
[
{
"containerKind": "",
"containerName": "",
"kind": "property",
"name": "(property) Object.constructor: Function",
"displayParts": [
{
"text": "(",
"kind": "punctuation"
},
{
"text": "property",
"kind": "text"
},
{
"text": ")",
"kind": "punctuation"
},
{
"text": " ",
"kind": "space"
},
{
"text": "Object",
"kind": "localName"
},
{
"text": ".",
"kind": "punctuation"
},
{
"text": "constructor",
"kind": "propertyName"
},
{
"text": ":",
"kind": "punctuation"
},
{
"text": " ",
"kind": "space"
},
{
"text": "Function",
"kind": "localName"
}
]
}
]
10 changes: 10 additions & 0 deletions tests/cases/fourslash/findAllRefsConstructor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path="fourslash.ts" />
// @target: esnext

////class A {
//// 'constructor'() { }
////}
////const a = new A()
////console.log(a.constructor/**/)

verify.baselineFindAllReferences('');