From 308e6438708a1961794f4572dbe5160830ed1c26 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Tue, 16 May 2023 15:20:45 -0700 Subject: [PATCH 01/35] Add inline variable refactor --- src/compiler/diagnosticMessages.json | 8 ++ src/services/_namespaces/ts.refactor.ts | 1 + src/services/refactors/inlineVariable.ts | 131 +++++++++++++++++++++++ src/services/services.ts | 2 +- 4 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 src/services/refactors/inlineVariable.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 102be920acf0e..5fdd77d3bfec0 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -7632,6 +7632,14 @@ "category": "Message", "code": 95183 }, + "Inline variable": { + "category": "Message", + "code": 95184 + }, + "Could not find variable to inline.": { + "category": "Message", + "code": 95185 + }, "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": { "category": "Error", diff --git a/src/services/_namespaces/ts.refactor.ts b/src/services/_namespaces/ts.refactor.ts index 2edf81a5b8caa..e397f484766ba 100644 --- a/src/services/_namespaces/ts.refactor.ts +++ b/src/services/_namespaces/ts.refactor.ts @@ -5,6 +5,7 @@ export * from "../refactors/convertExport"; export * from "../refactors/convertImport"; export * from "../refactors/extractType"; export * from "../refactors/helpers"; +export * from "../refactors/inlineVariable"; export * from "../refactors/moveToNewFile"; export * from "../refactors/moveToFile"; import * as addOrRemoveBracesToArrowFunction from "./ts.refactor.addOrRemoveBracesToArrowFunction"; diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts new file mode 100644 index 0000000000000..09bbf3aeb44c5 --- /dev/null +++ b/src/services/refactors/inlineVariable.ts @@ -0,0 +1,131 @@ +import { + CancellationToken, + Debug, + Diagnostics, + emptyArray, + Expression, + FindAllReferences, + getLocaleSpecificMessage, + getTokenAtPosition, + isVariableDeclaration, + isVariableDeclarationInVariableStatement, + mapDefined, + Node, + NoopCancellationToken, + Program, + refactor, + SourceFile, + textChanges, + VariableDeclaration, +} from "../_namespaces/ts"; +import { RefactorErrorInfo, registerRefactor } from "../_namespaces/ts.refactor"; + +const refactorName = "Inline variable"; +const refactorDescription = getLocaleSpecificMessage(Diagnostics.Inline_variable); + +const inlineVariableAction = { + name: refactorName, + description: refactorDescription, + kind: "refactor" // TODO: Not sure what the logic for defining these is. +}; + +interface InliningInfo { + references: Node[]; + declaration: VariableDeclaration; + replacement: Expression; +} + +registerRefactor(refactorName, { + kinds: [ inlineVariableAction.kind ], + + getAvailableActions(context) { + const { + cancellationToken, + file, + program, + preferences, + startPosition + } = context; + + const info = getInliningInfo(file, startPosition, program, cancellationToken); + + if (refactor.isRefactorErrorInfo(info) && preferences.provideRefactorNotApplicableReason) { + return [{ + name: refactorName, + description: refactorDescription, + actions: [{ + ...inlineVariableAction, + notApplicableReason: info.error + }] + }]; + } + + if (info) { + return [{ + name: refactorName, + description: refactorDescription, + actions: [inlineVariableAction] + }]; + } + + return emptyArray; + }, + + getEditsForAction(context, actionName) { + Debug.assert(actionName === refactorName, "Unexpected refactor invoked"); + + const { + cancellationToken, + file, + program, + startPosition + } = context; + + const info = getInliningInfo(file, startPosition, program, cancellationToken); + + if (!info || refactor.isRefactorErrorInfo(info)) { + return undefined; + } + + const { references, declaration, replacement } = info; + const edits = textChanges.ChangeTracker.with(context, tracker => { + for (const node of references) { + tracker.replaceNode(file, node, replacement); + } + tracker.delete(file, declaration); + }); + + return { edits }; + } +}); + +function getInliningInfo(file: SourceFile, startPosition: number, program: Program, cancellationToken: CancellationToken = NoopCancellationToken): InliningInfo | RefactorErrorInfo | undefined { + const token = getTokenAtPosition(file, startPosition); + const parent = token.parent; + + // Make sure the token is inside a variable declaration and the declaration + // is not in a catch clause or for-loop. + if (isVariableDeclaration(parent) && isVariableDeclarationInVariableStatement(parent)) { + // We only care if the declaration has a value (since that's the thing we're inlining). + if (!parent.initializer) { + return undefined; + } + + // Find all references to the variable. + const name = parent.name; + const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(name.pos, name, program, program.getSourceFiles(), cancellationToken); + if (!referencedSymbols || referencedSymbols.length !== 1) { + return undefined; + } + + // Only replace node references, and exclude the original variable too. + const references = mapDefined(referencedSymbols[0].references, entry => + entry.kind === FindAllReferences.EntryKind.Node ? entry.node === name ? undefined : entry.node : undefined + ); + + return references.length === 0 ? undefined : { references, declaration: parent, replacement: parent.initializer }; + } + + // TODO: Do we want to have other errors too? + return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; +} \ No newline at end of file diff --git a/src/services/services.ts b/src/services/services.ts index d631ce9a0a130..ae7997b538b32 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1452,7 +1452,7 @@ export function updateLanguageServiceSourceFile(sourceFile: SourceFile, scriptSn return createLanguageServiceSourceFile(sourceFile.fileName, scriptSnapshot, options, version, /*setNodeParents*/ true, sourceFile.scriptKind); } -const NoopCancellationToken: CancellationToken = { +export const NoopCancellationToken: CancellationToken = { isCancellationRequested: returnFalse, throwIfCancellationRequested: noop, }; From 307779edca86d600c8a62423d2616fa59851ed5c Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Wed, 17 May 2023 16:34:30 -0700 Subject: [PATCH 02/35] Basic tests --- .../fourslash/inlineVariableNoReplacementValue.ts | 7 +++++++ .../fourslash/inlineVariableOnlyDeclaration.ts | 7 +++++++ .../fourslash/inlineVariableSingleReference.ts | 13 +++++++++++++ 3 files changed, 27 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableNoReplacementValue.ts create mode 100644 tests/cases/fourslash/inlineVariableOnlyDeclaration.ts create mode 100644 tests/cases/fourslash/inlineVariableSingleReference.ts diff --git a/tests/cases/fourslash/inlineVariableNoReplacementValue.ts b/tests/cases/fourslash/inlineVariableNoReplacementValue.ts new file mode 100644 index 0000000000000..09e06c80f3581 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableNoReplacementValue.ts @@ -0,0 +1,7 @@ +/// + +////let x/**/; +////const y = x + 1; + +goTo.marker(""); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts b/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts new file mode 100644 index 0000000000000..728342edf69fe --- /dev/null +++ b/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts @@ -0,0 +1,7 @@ +/// + +////const x/**/ = 0; +////const y = 1 + 2; + +goTo.marker(""); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableSingleReference.ts b/tests/cases/fourslash/inlineVariableSingleReference.ts new file mode 100644 index 0000000000000..e68bc69edb828 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableSingleReference.ts @@ -0,0 +1,13 @@ +/// + +////const x/**/ = 0; +////const y = x + 1; + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: "const y = 0 + 1;" +}); \ No newline at end of file From 007a0e1b5a8995494ba780f033e97a23b5f45dfe Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Wed, 17 May 2023 16:38:35 -0700 Subject: [PATCH 03/35] Offer refactor in reference site --- src/services/refactors/inlineVariable.ts | 55 +++++++++++++++++------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 09bbf3aeb44c5..a3baca0410e66 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -7,7 +7,8 @@ import { FindAllReferences, getLocaleSpecificMessage, getTokenAtPosition, - isVariableDeclaration, + isIdentifier, + isInitializedVariable, isVariableDeclarationInVariableStatement, mapDefined, Node, @@ -44,10 +45,17 @@ registerRefactor(refactorName, { file, program, preferences, - startPosition + startPosition, + triggerReason } = context; - const info = getInliningInfo(file, startPosition, program, cancellationToken); + // tryWithReferenceToken is true below when triggerReason === "invoked", since we want to + // always provide the refactor in the declaration site but only show it in references when + // the refactor is explicitly invoked. + const info = getInliningInfo(file, startPosition, triggerReason === "invoked", program, cancellationToken); + if (!info) { + return emptyArray; + } if (refactor.isRefactorErrorInfo(info) && preferences.provideRefactorNotApplicableReason) { return [{ @@ -60,7 +68,7 @@ registerRefactor(refactorName, { }]; } - if (info) { + if (!refactor.isRefactorErrorInfo(info)) { return [{ name: refactorName, description: refactorDescription, @@ -78,10 +86,10 @@ registerRefactor(refactorName, { cancellationToken, file, program, - startPosition + startPosition, } = context; - const info = getInliningInfo(file, startPosition, program, cancellationToken); + const info = getInliningInfo(file, startPosition, /*tryWithReferenceToken*/ true, program, cancellationToken); if (!info || refactor.isRefactorErrorInfo(info)) { return undefined; @@ -99,18 +107,13 @@ registerRefactor(refactorName, { } }); -function getInliningInfo(file: SourceFile, startPosition: number, program: Program, cancellationToken: CancellationToken = NoopCancellationToken): InliningInfo | RefactorErrorInfo | undefined { +function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferenceToken: boolean, program: Program, cancellationToken: CancellationToken = NoopCancellationToken): InliningInfo | RefactorErrorInfo | undefined { const token = getTokenAtPosition(file, startPosition); const parent = token.parent; - // Make sure the token is inside a variable declaration and the declaration - // is not in a catch clause or for-loop. - if (isVariableDeclaration(parent) && isVariableDeclarationInVariableStatement(parent)) { - // We only care if the declaration has a value (since that's the thing we're inlining). - if (!parent.initializer) { - return undefined; - } - + // If the node is a variable declaration, make sure it's not in a catch clause or for-loop + // and that it has a value. + if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { // Find all references to the variable. const name = parent.name; const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(name.pos, name, program, program.getSourceFiles(), cancellationToken); @@ -126,6 +129,28 @@ function getInliningInfo(file: SourceFile, startPosition: number, program: Progr return references.length === 0 ? undefined : { references, declaration: parent, replacement: parent.initializer }; } + if (tryWithReferenceToken && isIdentifier(token)) { + // Try finding the declaration and nodes to replace via the reference token. + const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(token.pos, token, program, program.getSourceFiles(), cancellationToken); + if (!referencedSymbols || referencedSymbols.length !== 1) { + return undefined; + } + + const { definition } = referencedSymbols[0]; + if (definition?.type !== FindAllReferences.DefinitionKind.Symbol) { + return undefined; + } + + const { valueDeclaration } = definition.symbol; + if (valueDeclaration && isInitializedVariable(valueDeclaration) && isVariableDeclarationInVariableStatement(valueDeclaration)) { + const references = mapDefined(referencedSymbols[0].references, entry => + entry.kind === FindAllReferences.EntryKind.Node ? entry.node === valueDeclaration.name ? undefined : entry.node : undefined + ); + + return references.length === 0 ? undefined : { references, declaration: valueDeclaration, replacement: valueDeclaration.initializer }; + } + } + // TODO: Do we want to have other errors too? return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } \ No newline at end of file From c39c34c7d25c64f01d682ff84957f549daf16e6b Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Fri, 19 May 2023 13:31:25 -0700 Subject: [PATCH 04/35] Do not offer refactor if there is a write --- src/services/findAllReferences.ts | 8 +++-- src/services/refactors/inlineVariable.ts | 37 +++++++++++++++++------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index e19d22cd509cc..1aa55a6cbc2cf 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -849,8 +849,12 @@ export function getTextSpanOfEntry(entry: Entry) { getTextSpan(entry.node, entry.node.getSourceFile()); } -/** A node is considered a writeAccess iff it is a name of a declaration or a target of an assignment */ -function isWriteAccessForReference(node: Node): boolean { +/** + * A node is considered a writeAccess iff it is a name of a declaration or a target of an assignment. + * + * @internal + */ +export function isWriteAccessForReference(node: Node): boolean { const decl = getDeclarationFromName(node); return !!decl && declarationIsWriteAccess(decl) || node.kind === SyntaxKind.DefaultKeyword || isWriteAccess(node); } diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index a3baca0410e66..8f6f91b41ccba 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -120,13 +120,9 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (!referencedSymbols || referencedSymbols.length !== 1) { return undefined; } + const referenceNodes = getReferenceNodes(referencedSymbols[0].references, name); - // Only replace node references, and exclude the original variable too. - const references = mapDefined(referencedSymbols[0].references, entry => - entry.kind === FindAllReferences.EntryKind.Node ? entry.node === name ? undefined : entry.node : undefined - ); - - return references.length === 0 ? undefined : { references, declaration: parent, replacement: parent.initializer }; + return referenceNodes && { references: referenceNodes, declaration: parent, replacement: parent.initializer }; } if (tryWithReferenceToken && isIdentifier(token)) { @@ -136,21 +132,40 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return undefined; } - const { definition } = referencedSymbols[0]; + const { definition, references } = referencedSymbols[0]; if (definition?.type !== FindAllReferences.DefinitionKind.Symbol) { return undefined; } const { valueDeclaration } = definition.symbol; if (valueDeclaration && isInitializedVariable(valueDeclaration) && isVariableDeclarationInVariableStatement(valueDeclaration)) { - const references = mapDefined(referencedSymbols[0].references, entry => - entry.kind === FindAllReferences.EntryKind.Node ? entry.node === valueDeclaration.name ? undefined : entry.node : undefined - ); + const referenceNodes = getReferenceNodes(references, valueDeclaration.name); - return references.length === 0 ? undefined : { references, declaration: valueDeclaration, replacement: valueDeclaration.initializer }; + return referenceNodes && { references: referenceNodes, declaration: valueDeclaration, replacement: valueDeclaration.initializer }; } } // TODO: Do we want to have other errors too? return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; +} + +function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declaration: Node) { + const referenceNodes = mapDefined(entries, entry => { + // Only replace node references, and exclude the original variable too. + if (entry.kind !== FindAllReferences.EntryKind.Node || entry.node === declaration) { + return undefined; + } + + // Only inline if all references are reads. Else we might end up with an invalid scenario like: + // const y = x++ + 1 -> const y = 2++ + 1 + if (FindAllReferences.isWriteAccessForReference(entry.node)) { + return undefined; + } + + return entry.node; + }); + + // Return undefined if the only reference is the declaration itself, or if a reference + // isn't applicable for inlining. + return referenceNodes.length > 0 && referenceNodes.length === entries.length - 1 ? referenceNodes : undefined; } \ No newline at end of file From e477282f6aa6d4bce3dae912cf6dd2166e5c1239 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Fri, 19 May 2023 14:56:14 -0700 Subject: [PATCH 05/35] Add parenthesize logic --- src/services/refactors/inlineVariable.ts | 25 ++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 8f6f91b41ccba..fe36b935c1ee8 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -4,9 +4,12 @@ import { Diagnostics, emptyArray, Expression, + factory, FindAllReferences, + getExpressionPrecedence, getLocaleSpecificMessage, getTokenAtPosition, + isExpression, isIdentifier, isInitializedVariable, isVariableDeclarationInVariableStatement, @@ -98,7 +101,7 @@ registerRefactor(refactorName, { const { references, declaration, replacement } = info; const edits = textChanges.ChangeTracker.with(context, tracker => { for (const node of references) { - tracker.replaceNode(file, node, replacement); + tracker.replaceNode(file, node, getReplacementExpression(node, replacement)); } tracker.delete(file, declaration); }); @@ -149,7 +152,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } -function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declaration: Node) { +function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declaration: Node): Node[] | undefined { const referenceNodes = mapDefined(entries, entry => { // Only replace node references, and exclude the original variable too. if (entry.kind !== FindAllReferences.EntryKind.Node || entry.node === declaration) { @@ -168,4 +171,22 @@ function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declarat // Return undefined if the only reference is the declaration itself, or if a reference // isn't applicable for inlining. return referenceNodes.length > 0 && referenceNodes.length === entries.length - 1 ? referenceNodes : undefined; +} + +function getReplacementExpression(reference: Node, replacement: Expression): Expression { + // Logic from binaryOperandNeedsParentheses: "If the operand has lower precedence, + // then it needs to be parenthesized to preserve the intent of the expression. + // If the operand has higher precedence, then it does not need to be parenthesized." + // + // Note that binaryOperandNeedsParentheses has further logic when the precedences + // are equal, but for the purposes of this refactor we keep things simple and always + // parenthesize. + const { parent } = reference; + if (isExpression(parent)) { + if (getExpressionPrecedence(replacement) <= getExpressionPrecedence(parent)) { + return factory.createParenthesizedExpression(replacement); + } + } + + return replacement; } \ No newline at end of file From 32234c0bd066292370e115427b71371d969f0372 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Fri, 19 May 2023 15:26:51 -0700 Subject: [PATCH 06/35] More tests --- src/services/refactors/inlineVariable.ts | 14 ++++++++------ .../fourslash/inlineVariableMultipleScopes.ts | 19 +++++++++++++++++++ .../fourslash/inlineVariableNeedsParens.ts | 13 +++++++++++++ .../inlineVariableTriggerInReference.ts | 15 +++++++++++++++ .../fourslash/inlineVariableWriteReference.ts | 7 +++++++ 5 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 tests/cases/fourslash/inlineVariableMultipleScopes.ts create mode 100644 tests/cases/fourslash/inlineVariableNeedsParens.ts create mode 100644 tests/cases/fourslash/inlineVariableTriggerInReference.ts create mode 100644 tests/cases/fourslash/inlineVariableWriteReference.ts diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index fe36b935c1ee8..3ca0080988adf 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -6,6 +6,7 @@ import { Expression, factory, FindAllReferences, + flatMap, getExpressionPrecedence, getLocaleSpecificMessage, getTokenAtPosition, @@ -23,6 +24,7 @@ import { VariableDeclaration, } from "../_namespaces/ts"; import { RefactorErrorInfo, registerRefactor } from "../_namespaces/ts.refactor"; +import { getReferenceEntriesForNode } from "../findAllReferences"; const refactorName = "Inline variable"; const refactorDescription = getLocaleSpecificMessage(Diagnostics.Inline_variable); @@ -119,11 +121,11 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { // Find all references to the variable. const name = parent.name; - const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(name.pos, name, program, program.getSourceFiles(), cancellationToken); - if (!referencedSymbols || referencedSymbols.length !== 1) { + const referenceEntries = getReferenceEntriesForNode(name.pos, name, program, program.getSourceFiles(), cancellationToken); + if (!referenceEntries) { return undefined; } - const referenceNodes = getReferenceNodes(referencedSymbols[0].references, name); + const referenceNodes = getReferenceNodes(referenceEntries, name); return referenceNodes && { references: referenceNodes, declaration: parent, replacement: parent.initializer }; } @@ -131,18 +133,18 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (tryWithReferenceToken && isIdentifier(token)) { // Try finding the declaration and nodes to replace via the reference token. const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(token.pos, token, program, program.getSourceFiles(), cancellationToken); - if (!referencedSymbols || referencedSymbols.length !== 1) { + if (!referencedSymbols) { return undefined; } - const { definition, references } = referencedSymbols[0]; + const { definition } = referencedSymbols[0]; if (definition?.type !== FindAllReferences.DefinitionKind.Symbol) { return undefined; } const { valueDeclaration } = definition.symbol; if (valueDeclaration && isInitializedVariable(valueDeclaration) && isVariableDeclarationInVariableStatement(valueDeclaration)) { - const referenceNodes = getReferenceNodes(references, valueDeclaration.name); + const referenceNodes = getReferenceNodes(flatMap(referencedSymbols, ({ references }) => references), valueDeclaration.name); return referenceNodes && { references: referenceNodes, declaration: valueDeclaration, replacement: valueDeclaration.initializer }; } diff --git a/tests/cases/fourslash/inlineVariableMultipleScopes.ts b/tests/cases/fourslash/inlineVariableMultipleScopes.ts new file mode 100644 index 0000000000000..7b6d76348de03 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableMultipleScopes.ts @@ -0,0 +1,19 @@ +/// + +////let x/**/ = 1; +////function foo() { +//// console.log(x); +////} +////const y = x + 2; + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: `function foo() { + console.log(1); +} +const y = 1 + 2;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableNeedsParens.ts b/tests/cases/fourslash/inlineVariableNeedsParens.ts new file mode 100644 index 0000000000000..41afad4022f94 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableNeedsParens.ts @@ -0,0 +1,13 @@ +/// + +////const x/**/ = 1 + 2; +////const y = x * 3; + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: "const y = (1 + 2) * 3;" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableTriggerInReference.ts b/tests/cases/fourslash/inlineVariableTriggerInReference.ts new file mode 100644 index 0000000000000..b192c7ab3cce8 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableTriggerInReference.ts @@ -0,0 +1,15 @@ +/// + +////const x = 0; +////const y = /*a*/x/*b*/ + 1; + +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Inline variable"); +verify.refactorAvailableForTriggerReason("invoked", "Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: "const y = 0 + 1;", + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableWriteReference.ts b/tests/cases/fourslash/inlineVariableWriteReference.ts new file mode 100644 index 0000000000000..ead2754d70bdd --- /dev/null +++ b/tests/cases/fourslash/inlineVariableWriteReference.ts @@ -0,0 +1,7 @@ +/// + +////const x/**/ = 0; +////const y = x++ + 1; + +goTo.marker(""); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From 72454218489dac08f1c16e458691a22f9c16aeb1 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 22 May 2023 11:50:56 -0700 Subject: [PATCH 07/35] Update baselines --- tests/baselines/reference/api/tsserverlibrary.d.ts | 1 + tests/baselines/reference/api/typescript.d.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 56c4b49ff6df2..d08e950bf1531 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -11232,6 +11232,7 @@ declare namespace ts { function getDefaultLibFilePath(options: CompilerOptions): string; /** The version of the language service API */ const servicesVersion = "0.8"; + const NoopCancellationToken: CancellationToken; /** * Transform one or more nodes using the supplied transformers. * @param source A single `Node` or an array of `Node` objects. diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 2b642a091ce2e..b01a4786b6534 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -7263,6 +7263,7 @@ declare namespace ts { function getDefaultLibFilePath(options: CompilerOptions): string; /** The version of the language service API */ const servicesVersion = "0.8"; + const NoopCancellationToken: CancellationToken; /** * Transform one or more nodes using the supplied transformers. * @param source A single `Node` or an array of `Node` objects. From 6d83ed07d28ef69f5a938653b6c4891fe1dabe75 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 22 May 2023 11:51:15 -0700 Subject: [PATCH 08/35] Another test --- .../fourslash/inlineVariableNotAVariableStatement.ts | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableNotAVariableStatement.ts diff --git a/tests/cases/fourslash/inlineVariableNotAVariableStatement.ts b/tests/cases/fourslash/inlineVariableNotAVariableStatement.ts new file mode 100644 index 0000000000000..b2efbaeec74bb --- /dev/null +++ b/tests/cases/fourslash/inlineVariableNotAVariableStatement.ts @@ -0,0 +1,8 @@ +/// + +////for (let /*a*/i/*b*/ = 0; i < 5; i++) { +//// console.log(i) +////} + +goTo.select("a", "b"); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From 5579a481d8867987ff0482881c66e501a0c9c4ab Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Tue, 23 May 2023 12:20:52 -0700 Subject: [PATCH 09/35] Define the refactor kind --- src/services/refactors/inlineVariable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 3ca0080988adf..051a05e7e5639 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -32,7 +32,7 @@ const refactorDescription = getLocaleSpecificMessage(Diagnostics.Inline_variable const inlineVariableAction = { name: refactorName, description: refactorDescription, - kind: "refactor" // TODO: Not sure what the logic for defining these is. + kind: "refactor.inline.variable" }; interface InliningInfo { From 3ad106d25c463bf150a34dc8f8b0cedf3b878f5d Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 10:51:12 -0700 Subject: [PATCH 10/35] Limit FAR to one file --- src/services/refactors/inlineVariable.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 051a05e7e5639..94dcc81018ae1 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -119,9 +119,9 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // If the node is a variable declaration, make sure it's not in a catch clause or for-loop // and that it has a value. if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { - // Find all references to the variable. + // Find all references to the variable in the current file. const name = parent.name; - const referenceEntries = getReferenceEntriesForNode(name.pos, name, program, program.getSourceFiles(), cancellationToken); + const referenceEntries = getReferenceEntriesForNode(name.pos, name, program, [file], cancellationToken); if (!referenceEntries) { return undefined; } @@ -132,7 +132,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (tryWithReferenceToken && isIdentifier(token)) { // Try finding the declaration and nodes to replace via the reference token. - const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(token.pos, token, program, program.getSourceFiles(), cancellationToken); + const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(token.pos, token, program, [file], cancellationToken); if (!referencedSymbols) { return undefined; } @@ -184,10 +184,8 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp // are equal, but for the purposes of this refactor we keep things simple and always // parenthesize. const { parent } = reference; - if (isExpression(parent)) { - if (getExpressionPrecedence(replacement) <= getExpressionPrecedence(parent)) { - return factory.createParenthesizedExpression(replacement); - } + if (isExpression(parent) && (getExpressionPrecedence(replacement) <= getExpressionPrecedence(parent))) { + return factory.createParenthesizedExpression(replacement); } return replacement; From c0e302a31f4a7e53f6c3feea31c642421da1601d Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 11:30:35 -0700 Subject: [PATCH 11/35] Handle some more invalid cases --- src/services/refactors/inlineVariable.ts | 16 ++++++++++++++++ .../inlineVariableMultipleDeclarations.ts | 11 +++++++++++ tests/cases/fourslash/inlineVariableTypeof.ts | 7 +++++++ 3 files changed, 34 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableMultipleDeclarations.ts create mode 100644 tests/cases/fourslash/inlineVariableTypeof.ts diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 94dcc81018ae1..8e67c85a60808 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -13,6 +13,7 @@ import { isExpression, isIdentifier, isInitializedVariable, + isTypeQueryNode, isVariableDeclarationInVariableStatement, mapDefined, Node, @@ -119,6 +120,11 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // If the node is a variable declaration, make sure it's not in a catch clause or for-loop // and that it has a value. if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { + // Don't inline the variable if it isn't declared exactly once. + if (parent.symbol.declarations?.length !== 1) { + return undefined; + } + // Find all references to the variable in the current file. const name = parent.name; const referenceEntries = getReferenceEntriesForNode(name.pos, name, program, [file], cancellationToken); @@ -142,6 +148,11 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return undefined; } + // Don't inline the variable if it isn't declared exactly once. + if (definition.symbol.declarations?.length !== 1) { + return undefined; + } + const { valueDeclaration } = definition.symbol; if (valueDeclaration && isInitializedVariable(valueDeclaration) && isVariableDeclarationInVariableStatement(valueDeclaration)) { const referenceNodes = getReferenceNodes(flatMap(referencedSymbols, ({ references }) => references), valueDeclaration.name); @@ -167,6 +178,11 @@ function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declarat return undefined; } + // typeof needs an identifier, so we can't inline a value in there. + if (isTypeQueryNode(entry.node.parent)) { + return undefined; + } + return entry.node; }); diff --git a/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts b/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts new file mode 100644 index 0000000000000..4eb294cc8f8ba --- /dev/null +++ b/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts @@ -0,0 +1,11 @@ +/// + +////const foo/*a*/ = "foo"; +////type foo/*b*/ = string; +////type bar = foo; + +goTo.marker("a"); +verify.not.refactorAvailable("Inline variable"); + +goTo.marker("b"); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableTypeof.ts b/tests/cases/fourslash/inlineVariableTypeof.ts new file mode 100644 index 0000000000000..5ef20f4d068e0 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableTypeof.ts @@ -0,0 +1,7 @@ +/// + +////const Foo/**/ = class Foo {} +////type FooConstructor = typeof Foo; + +goTo.marker(""); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From 095d3b45582c6c26ce34f77d4c0f1e82f680cbdb Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 13:48:59 -0700 Subject: [PATCH 12/35] Use eachSymbolReferenceInFile --- src/services/refactors/inlineVariable.ts | 81 ++++++------------- src/services/services.ts | 2 +- .../reference/api/tsserverlibrary.d.ts | 1 - tests/baselines/reference/api/typescript.d.ts | 1 - 4 files changed, 27 insertions(+), 58 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 8e67c85a60808..c7b3887c8f062 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -1,31 +1,29 @@ import { - CancellationToken, Debug, Diagnostics, emptyArray, Expression, factory, FindAllReferences, - flatMap, getExpressionPrecedence, getLocaleSpecificMessage, getTokenAtPosition, + Identifier, isExpression, isIdentifier, isInitializedVariable, isTypeQueryNode, isVariableDeclarationInVariableStatement, - mapDefined, Node, - NoopCancellationToken, Program, refactor, SourceFile, + SymbolFlags, textChanges, + TypeChecker, VariableDeclaration, } from "../_namespaces/ts"; import { RefactorErrorInfo, registerRefactor } from "../_namespaces/ts.refactor"; -import { getReferenceEntriesForNode } from "../findAllReferences"; const refactorName = "Inline variable"; const refactorDescription = getLocaleSpecificMessage(Diagnostics.Inline_variable); @@ -47,7 +45,6 @@ registerRefactor(refactorName, { getAvailableActions(context) { const { - cancellationToken, file, program, preferences, @@ -58,7 +55,7 @@ registerRefactor(refactorName, { // tryWithReferenceToken is true below when triggerReason === "invoked", since we want to // always provide the refactor in the declaration site but only show it in references when // the refactor is explicitly invoked. - const info = getInliningInfo(file, startPosition, triggerReason === "invoked", program, cancellationToken); + const info = getInliningInfo(file, startPosition, triggerReason === "invoked", program); if (!info) { return emptyArray; } @@ -88,15 +85,9 @@ registerRefactor(refactorName, { getEditsForAction(context, actionName) { Debug.assert(actionName === refactorName, "Unexpected refactor invoked"); - const { - cancellationToken, - file, - program, - startPosition, - } = context; - - const info = getInliningInfo(file, startPosition, /*tryWithReferenceToken*/ true, program, cancellationToken); + const { file, program, startPosition } = context; + const info = getInliningInfo(file, startPosition, /*tryWithReferenceToken*/ true, program); if (!info || refactor.isRefactorErrorInfo(info)) { return undefined; } @@ -113,82 +104,62 @@ registerRefactor(refactorName, { } }); -function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferenceToken: boolean, program: Program, cancellationToken: CancellationToken = NoopCancellationToken): InliningInfo | RefactorErrorInfo | undefined { +function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferenceToken: boolean, program: Program): InliningInfo | RefactorErrorInfo | undefined { + const checker = program.getTypeChecker(); const token = getTokenAtPosition(file, startPosition); const parent = token.parent; // If the node is a variable declaration, make sure it's not in a catch clause or for-loop // and that it has a value. - if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { + if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent) && isIdentifier(parent.name)) { // Don't inline the variable if it isn't declared exactly once. if (parent.symbol.declarations?.length !== 1) { return undefined; } // Find all references to the variable in the current file. - const name = parent.name; - const referenceEntries = getReferenceEntriesForNode(name.pos, name, program, [file], cancellationToken); - if (!referenceEntries) { - return undefined; - } - const referenceNodes = getReferenceNodes(referenceEntries, name); - - return referenceNodes && { references: referenceNodes, declaration: parent, replacement: parent.initializer }; + const references = getReferenceNodes(parent.name, checker, file); + return references && { references, declaration: parent, replacement: parent.initializer }; } if (tryWithReferenceToken && isIdentifier(token)) { // Try finding the declaration and nodes to replace via the reference token. - const referencedSymbols = FindAllReferences.Core.getReferencedSymbolsForNode(token.pos, token, program, [file], cancellationToken); - if (!referencedSymbols) { - return undefined; - } - - const { definition } = referencedSymbols[0]; - if (definition?.type !== FindAllReferences.DefinitionKind.Symbol) { + const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); + if (definition?.declarations?.length !== 1) { return undefined; } - // Don't inline the variable if it isn't declared exactly once. - if (definition.symbol.declarations?.length !== 1) { + const declaration = definition.declarations[0]; + if (!isInitializedVariable(declaration) || !isVariableDeclarationInVariableStatement(declaration) || !isIdentifier(declaration.name)) { return undefined; } - const { valueDeclaration } = definition.symbol; - if (valueDeclaration && isInitializedVariable(valueDeclaration) && isVariableDeclarationInVariableStatement(valueDeclaration)) { - const referenceNodes = getReferenceNodes(flatMap(referencedSymbols, ({ references }) => references), valueDeclaration.name); - - return referenceNodes && { references: referenceNodes, declaration: valueDeclaration, replacement: valueDeclaration.initializer }; - } + const references = getReferenceNodes(declaration.name, checker, file); + return references && { references, declaration, replacement: declaration.initializer }; } // TODO: Do we want to have other errors too? return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } -function getReferenceNodes(entries: readonly FindAllReferences.Entry[], declaration: Node): Node[] | undefined { - const referenceNodes = mapDefined(entries, entry => { - // Only replace node references, and exclude the original variable too. - if (entry.kind !== FindAllReferences.EntryKind.Node || entry.node === declaration) { - return undefined; - } - +function getReferenceNodes(declaration: Identifier, checker: TypeChecker, file: SourceFile): Identifier[] | undefined { + const references: Identifier[] = []; + const cannotInline = FindAllReferences.Core.eachSymbolReferenceInFile(declaration, checker, file, ref => { // Only inline if all references are reads. Else we might end up with an invalid scenario like: // const y = x++ + 1 -> const y = 2++ + 1 - if (FindAllReferences.isWriteAccessForReference(entry.node)) { - return undefined; + if (FindAllReferences.isWriteAccessForReference(ref)) { + return true; } // typeof needs an identifier, so we can't inline a value in there. - if (isTypeQueryNode(entry.node.parent)) { - return undefined; + if (isTypeQueryNode(ref.parent)) { + return true; } - return entry.node; + references.push(ref); }); - // Return undefined if the only reference is the declaration itself, or if a reference - // isn't applicable for inlining. - return referenceNodes.length > 0 && referenceNodes.length === entries.length - 1 ? referenceNodes : undefined; + return references.length === 0 || cannotInline ? undefined : references; } function getReplacementExpression(reference: Node, replacement: Expression): Expression { diff --git a/src/services/services.ts b/src/services/services.ts index ae7997b538b32..d631ce9a0a130 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1452,7 +1452,7 @@ export function updateLanguageServiceSourceFile(sourceFile: SourceFile, scriptSn return createLanguageServiceSourceFile(sourceFile.fileName, scriptSnapshot, options, version, /*setNodeParents*/ true, sourceFile.scriptKind); } -export const NoopCancellationToken: CancellationToken = { +const NoopCancellationToken: CancellationToken = { isCancellationRequested: returnFalse, throwIfCancellationRequested: noop, }; diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index d08e950bf1531..56c4b49ff6df2 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -11232,7 +11232,6 @@ declare namespace ts { function getDefaultLibFilePath(options: CompilerOptions): string; /** The version of the language service API */ const servicesVersion = "0.8"; - const NoopCancellationToken: CancellationToken; /** * Transform one or more nodes using the supplied transformers. * @param source A single `Node` or an array of `Node` objects. diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index b01a4786b6534..2b642a091ce2e 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -7263,7 +7263,6 @@ declare namespace ts { function getDefaultLibFilePath(options: CompilerOptions): string; /** The version of the language service API */ const servicesVersion = "0.8"; - const NoopCancellationToken: CancellationToken; /** * Transform one or more nodes using the supplied transformers. * @param source A single `Node` or an array of `Node` objects. From ce734fff5da0ea5749327e22ee34247b804da75b Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 14:10:41 -0700 Subject: [PATCH 13/35] Another error msg --- src/compiler/diagnosticMessages.json | 4 ++++ src/services/refactors/inlineVariable.ts | 5 ++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 5fdd77d3bfec0..f85cc2599895d 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -7640,6 +7640,10 @@ "category": "Message", "code": 95185 }, + "Only variables declared exactly once can be inlined.": { + "category": "Message", + "code": 95186 + }, "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": { "category": "Error", diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index c7b3887c8f062..5bcba2f0273c2 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -114,7 +114,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent) && isIdentifier(parent.name)) { // Don't inline the variable if it isn't declared exactly once. if (parent.symbol.declarations?.length !== 1) { - return undefined; + return { error: getLocaleSpecificMessage(Diagnostics.Only_variables_declared_exactly_once_can_be_inlined) }; } // Find all references to the variable in the current file. @@ -126,7 +126,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Try finding the declaration and nodes to replace via the reference token. const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); if (definition?.declarations?.length !== 1) { - return undefined; + return { error: getLocaleSpecificMessage(Diagnostics.Only_variables_declared_exactly_once_can_be_inlined) }; } const declaration = definition.declarations[0]; @@ -138,7 +138,6 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return references && { references, declaration, replacement: declaration.initializer }; } - // TODO: Do we want to have other errors too? return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } From c679717448b166a0497cb39cdf726079b22816ae Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 14:18:17 -0700 Subject: [PATCH 14/35] Exported variable test --- .../cases/fourslash/inlineVariableExportedVariable.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableExportedVariable.ts diff --git a/tests/cases/fourslash/inlineVariableExportedVariable.ts b/tests/cases/fourslash/inlineVariableExportedVariable.ts new file mode 100644 index 0000000000000..c909e1e79a113 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableExportedVariable.ts @@ -0,0 +1,11 @@ +/// + +////export const x/*a*/ = 1; +////const y/*b*/ = 2; +////export { y }; + +goTo.marker("a"); +verify.not.refactorAvailable("Inline variable"); + +goTo.marker("b"); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From 5e0dfe6a5f4fe1a19a95c1b310df9505d5a2d0af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Thu, 25 May 2023 16:37:50 -0700 Subject: [PATCH 15/35] Update src/compiler/diagnosticMessages.json Co-authored-by: Andrew Branch --- src/compiler/diagnosticMessages.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index f85cc2599895d..2e97e4229d757 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -7640,7 +7640,7 @@ "category": "Message", "code": 95185 }, - "Only variables declared exactly once can be inlined.": { + "Variables that share a name with a type or namespace in the same scope cannot be inlined.": { "category": "Message", "code": 95186 }, From 225b759b16e9d71ec7f383dbe9ef829f9d0fcbdc Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Thu, 25 May 2023 16:41:15 -0700 Subject: [PATCH 16/35] Update messages --- src/services/refactors/inlineVariable.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 5bcba2f0273c2..b8f5cf4ef0eca 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -114,7 +114,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent) && isIdentifier(parent.name)) { // Don't inline the variable if it isn't declared exactly once. if (parent.symbol.declarations?.length !== 1) { - return { error: getLocaleSpecificMessage(Diagnostics.Only_variables_declared_exactly_once_can_be_inlined) }; + return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } // Find all references to the variable in the current file. @@ -126,7 +126,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Try finding the declaration and nodes to replace via the reference token. const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); if (definition?.declarations?.length !== 1) { - return { error: getLocaleSpecificMessage(Diagnostics.Only_variables_declared_exactly_once_can_be_inlined) }; + return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } const declaration = definition.declarations[0]; From 2fb8b04f89221179c514198493924b7f9d84748c Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 29 May 2023 11:39:12 -0700 Subject: [PATCH 17/35] Handle case where declaration has export --- src/services/refactors/inlineVariable.ts | 9 +++++++++ tests/cases/fourslash/inlineVariableExportedVariable.ts | 2 ++ 2 files changed, 11 insertions(+) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index b8f5cf4ef0eca..45c4210cec3b1 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -1,14 +1,17 @@ import { + canHaveModifiers, Debug, Diagnostics, emptyArray, Expression, factory, FindAllReferences, + findAncestor, getExpressionPrecedence, getLocaleSpecificMessage, getTokenAtPosition, Identifier, + isExportModifier, isExpression, isIdentifier, isInitializedVariable, @@ -17,6 +20,7 @@ import { Node, Program, refactor, + some, SourceFile, SymbolFlags, textChanges, @@ -117,6 +121,11 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } + // Do not inline if the variable is exported. + if (findAncestor(parent, node => canHaveModifiers(node) && some(node.modifiers, isExportModifier))) { + return undefined; + } + // Find all references to the variable in the current file. const references = getReferenceNodes(parent.name, checker, file); return references && { references, declaration: parent, replacement: parent.initializer }; diff --git a/tests/cases/fourslash/inlineVariableExportedVariable.ts b/tests/cases/fourslash/inlineVariableExportedVariable.ts index c909e1e79a113..51eb0c8625055 100644 --- a/tests/cases/fourslash/inlineVariableExportedVariable.ts +++ b/tests/cases/fourslash/inlineVariableExportedVariable.ts @@ -2,6 +2,8 @@ ////export const x/*a*/ = 1; ////const y/*b*/ = 2; +////const z = x + 1; +////const w = 2 + y; ////export { y }; goTo.marker("a"); From 1e6d8cdc005cb2207c912f6ce8aae060ecc4341d Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 29 May 2023 12:49:51 -0700 Subject: [PATCH 18/35] More tests --- src/services/refactors/inlineVariable.ts | 7 ++++--- .../fourslash/inlineVariableFunctionResult.ts | 17 +++++++++++++++++ .../fourslash/inlineVariableNullCoalescing.ts | 15 +++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 tests/cases/fourslash/inlineVariableFunctionResult.ts create mode 100644 tests/cases/fourslash/inlineVariableNullCoalescing.ts diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 45c4210cec3b1..56db264b58967 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -17,6 +17,7 @@ import { isInitializedVariable, isTypeQueryNode, isVariableDeclarationInVariableStatement, + needsParentheses, Node, Program, refactor, @@ -176,10 +177,10 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp // If the operand has higher precedence, then it does not need to be parenthesized." // // Note that binaryOperandNeedsParentheses has further logic when the precedences - // are equal, but for the purposes of this refactor we keep things simple and always - // parenthesize. + // are equal, but for the purposes of this refactor we keep things simple and + // instead just check for special cases with needsParentheses. const { parent } = reference; - if (isExpression(parent) && (getExpressionPrecedence(replacement) <= getExpressionPrecedence(parent))) { + if (isExpression(parent) && (getExpressionPrecedence(replacement) < getExpressionPrecedence(parent) || needsParentheses(parent))) { return factory.createParenthesizedExpression(replacement); } diff --git a/tests/cases/fourslash/inlineVariableFunctionResult.ts b/tests/cases/fourslash/inlineVariableFunctionResult.ts new file mode 100644 index 0000000000000..b2b0d3d8562b5 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableFunctionResult.ts @@ -0,0 +1,17 @@ +/// + +////function inc(x: number) { return x + 1; } +////const y/**/ = inc(1); +////const z = y + 1; +////const w = inc(y); + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: `function inc(x: number) { return x + 1; } +const z = inc(1) + 1; +const w = inc(inc(1));` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableNullCoalescing.ts b/tests/cases/fourslash/inlineVariableNullCoalescing.ts new file mode 100644 index 0000000000000..568b1171fceeb --- /dev/null +++ b/tests/cases/fourslash/inlineVariableNullCoalescing.ts @@ -0,0 +1,15 @@ +/// + +////function foo(): number | undefined { return Math.random() > 0.5 ? 1 : undefined; } +////const x/**/ = foo(); +////const y = x?.toString(); + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: `function foo(): number | undefined { return Math.random() > 0.5 ? 1 : undefined; } +const y = (foo())?.toString();` +}); \ No newline at end of file From b2d90182fa39566593b8c1c01f3690e312460659 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Tue, 30 May 2023 16:27:33 -0700 Subject: [PATCH 19/35] Add inline callback case --- .../fourslash/inlineVariableJsxCallback.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableJsxCallback.ts diff --git a/tests/cases/fourslash/inlineVariableJsxCallback.ts b/tests/cases/fourslash/inlineVariableJsxCallback.ts new file mode 100644 index 0000000000000..77fc7d435ee79 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableJsxCallback.ts @@ -0,0 +1,35 @@ +/// + +//@module: commonjs +//@jsx: preserve + +// @Filename: file.tsx +////function Button() { +//// const onClick /**/ = () => { +//// console.log("clicked"); +//// }; +//// +//// return ( +//// +//// ); +////} + +goTo.marker(""); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: `function Button() { + + return ( + + ); +}` +}); \ No newline at end of file From 9bc6c05233a585d0c66a212579e81b100457a3a2 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Wed, 31 May 2023 15:20:12 -0700 Subject: [PATCH 20/35] Handle Daniel's recursive case --- src/services/refactors/inlineVariable.ts | 14 ++++++++++---- .../inlineVariableRecursiveInitializer.ts | 6 ++++++ 2 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 tests/cases/fourslash/inlineVariableRecursiveInitializer.ts diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 56db264b58967..95c0e4f94a2a8 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -11,6 +11,7 @@ import { getLocaleSpecificMessage, getTokenAtPosition, Identifier, + InitializedVariableDeclaration, isExportModifier, isExpression, isIdentifier, @@ -128,7 +129,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen } // Find all references to the variable in the current file. - const references = getReferenceNodes(parent.name, checker, file); + const references = getReferenceNodes(parent, checker, file); return references && { references, declaration: parent, replacement: parent.initializer }; } @@ -144,16 +145,16 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return undefined; } - const references = getReferenceNodes(declaration.name, checker, file); + const references = getReferenceNodes(declaration, checker, file); return references && { references, declaration, replacement: declaration.initializer }; } return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } -function getReferenceNodes(declaration: Identifier, checker: TypeChecker, file: SourceFile): Identifier[] | undefined { +function getReferenceNodes(declaration: InitializedVariableDeclaration, checker: TypeChecker, file: SourceFile): Identifier[] | undefined { const references: Identifier[] = []; - const cannotInline = FindAllReferences.Core.eachSymbolReferenceInFile(declaration, checker, file, ref => { + const cannotInline = FindAllReferences.Core.eachSymbolReferenceInFile(declaration.name as Identifier, checker, file, ref => { // Only inline if all references are reads. Else we might end up with an invalid scenario like: // const y = x++ + 1 -> const y = 2++ + 1 if (FindAllReferences.isWriteAccessForReference(ref)) { @@ -165,6 +166,11 @@ function getReferenceNodes(declaration: Identifier, checker: TypeChecker, file: return true; } + // Cannot inline recursive declarations (e.g. const foo = () => foo();) + if (findAncestor(ref, node => node === declaration.initializer)) { + return true; + } + references.push(ref); }); diff --git a/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts b/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts new file mode 100644 index 0000000000000..3e6dbeb4501f6 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts @@ -0,0 +1,6 @@ +/// + +////const foo/**/ = () => foo(); + +goTo.marker(""); +verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From a259b3a67635261e70c49e96c0944f8b36c7de4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Wed, 7 Jun 2023 09:46:25 -0500 Subject: [PATCH 21/35] Improve comments --- src/services/refactors/inlineVariable.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 95c0e4f94a2a8..2c788da8e9830 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -118,7 +118,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // If the node is a variable declaration, make sure it's not in a catch clause or for-loop // and that it has a value. if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent) && isIdentifier(parent.name)) { - // Don't inline the variable if it isn't declared exactly once. + // Don't inline the variable if it has multiple declarations. if (parent.symbol.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } @@ -133,8 +133,8 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return references && { references, declaration: parent, replacement: parent.initializer }; } + // Try finding the declaration and nodes to replace via the reference token. if (tryWithReferenceToken && isIdentifier(token)) { - // Try finding the declaration and nodes to replace via the reference token. const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); if (definition?.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; From b308e999d6decc0430fa9b3fdcceadc37ae9b332 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Wed, 7 Jun 2023 16:06:43 -0500 Subject: [PATCH 22/35] Use deep clones for replacements --- src/services/refactors/inlineVariable.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 2c788da8e9830..2d36ef99ce5c3 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -9,6 +9,7 @@ import { findAncestor, getExpressionPrecedence, getLocaleSpecificMessage, + getSynthesizedDeepClone, getTokenAtPosition, Identifier, InitializedVariableDeclaration, @@ -130,7 +131,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Find all references to the variable in the current file. const references = getReferenceNodes(parent, checker, file); - return references && { references, declaration: parent, replacement: parent.initializer }; + return references && { references, declaration: parent, replacement: getSynthesizedDeepClone(parent.initializer) }; } // Try finding the declaration and nodes to replace via the reference token. @@ -146,7 +147,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen } const references = getReferenceNodes(declaration, checker, file); - return references && { references, declaration, replacement: declaration.initializer }; + return references && { references, declaration, replacement: getSynthesizedDeepClone(declaration.initializer) }; } return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; @@ -191,4 +192,4 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp } return replacement; -} \ No newline at end of file +} From 76bd29a99430c27b5f033dbded62da09d2edb60e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Wed, 7 Jun 2023 16:44:53 -0500 Subject: [PATCH 23/35] Use textRangeContainsPositionInclusive --- src/services/refactors/inlineVariable.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 2d36ef99ce5c3..350d0b0da1acd 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -27,6 +27,7 @@ import { SourceFile, SymbolFlags, textChanges, + textRangeContainsPositionInclusive, TypeChecker, VariableDeclaration, } from "../_namespaces/ts"; @@ -168,7 +169,7 @@ function getReferenceNodes(declaration: InitializedVariableDeclaration, checker: } // Cannot inline recursive declarations (e.g. const foo = () => foo();) - if (findAncestor(ref, node => node === declaration.initializer)) { + if (textRangeContainsPositionInclusive(declaration, ref.pos)) { return true; } From 4080541ff9017f90ab866b5b5a20bc031dd0c701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Fri, 9 Jun 2023 11:21:54 -0500 Subject: [PATCH 24/35] deep clone for each replacement --- src/services/refactors/inlineVariable.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 350d0b0da1acd..16fe8de5a8184 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -132,7 +132,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Find all references to the variable in the current file. const references = getReferenceNodes(parent, checker, file); - return references && { references, declaration: parent, replacement: getSynthesizedDeepClone(parent.initializer) }; + return references && { references, declaration: parent, replacement: parent.initializer }; } // Try finding the declaration and nodes to replace via the reference token. @@ -148,7 +148,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen } const references = getReferenceNodes(declaration, checker, file); - return references && { references, declaration, replacement: getSynthesizedDeepClone(declaration.initializer) }; + return references && { references, declaration, replacement: declaration.initializer }; } return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; @@ -189,8 +189,8 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp // instead just check for special cases with needsParentheses. const { parent } = reference; if (isExpression(parent) && (getExpressionPrecedence(replacement) < getExpressionPrecedence(parent) || needsParentheses(parent))) { - return factory.createParenthesizedExpression(replacement); + return getSynthesizedDeepClone(factory.createParenthesizedExpression(replacement)); } - return replacement; + return getSynthesizedDeepClone(replacement); } From 9f39bfb87e9898153244df5e8d79a2bf3dc13d35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Fri, 9 Jun 2023 09:26:50 -0700 Subject: [PATCH 25/35] Update src/services/refactors/inlineVariable.ts Co-authored-by: Daniel Rosenwasser --- src/services/refactors/inlineVariable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 16fe8de5a8184..0675ce005cc34 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -49,7 +49,7 @@ interface InliningInfo { } registerRefactor(refactorName, { - kinds: [ inlineVariableAction.kind ], + kinds: [inlineVariableAction.kind], getAvailableActions(context) { const { From 4fa54f733ea0c32f03887229d52060185c368da9 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 11:27:08 -0700 Subject: [PATCH 26/35] Clone the replacement node --- src/services/refactors/inlineVariable.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 0675ce005cc34..1c8f9c5da90bd 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -180,6 +180,10 @@ function getReferenceNodes(declaration: InitializedVariableDeclaration, checker: } function getReplacementExpression(reference: Node, replacement: Expression): Expression { + // Make sure each reference site gets its own copy of the replacement node. + replacement = getSynthesizedDeepClone(replacement); + const { parent } = reference; + // Logic from binaryOperandNeedsParentheses: "If the operand has lower precedence, // then it needs to be parenthesized to preserve the intent of the expression. // If the operand has higher precedence, then it does not need to be parenthesized." @@ -187,10 +191,9 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp // Note that binaryOperandNeedsParentheses has further logic when the precedences // are equal, but for the purposes of this refactor we keep things simple and // instead just check for special cases with needsParentheses. - const { parent } = reference; if (isExpression(parent) && (getExpressionPrecedence(replacement) < getExpressionPrecedence(parent) || needsParentheses(parent))) { - return getSynthesizedDeepClone(factory.createParenthesizedExpression(replacement)); + return factory.createParenthesizedExpression(replacement); } - return getSynthesizedDeepClone(replacement); + return replacement; } From 4e608efd7e98ba8900638442591e1445c163e12f Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 12:23:04 -0700 Subject: [PATCH 27/35] Restrict availability to identifier only --- src/services/refactors/inlineVariable.ts | 10 +++++++--- .../fourslash/inlineVariableExportedVariable.ts | 12 ++++++------ .../cases/fourslash/inlineVariableFunctionResult.ts | 4 ++-- tests/cases/fourslash/inlineVariableJsxCallback.ts | 4 ++-- .../fourslash/inlineVariableMultipleDeclarations.ts | 8 ++++---- .../cases/fourslash/inlineVariableMultipleScopes.ts | 4 ++-- tests/cases/fourslash/inlineVariableNeedsParens.ts | 4 ++-- .../fourslash/inlineVariableNoReplacementValue.ts | 4 ++-- .../cases/fourslash/inlineVariableNullCoalescing.ts | 4 ++-- .../cases/fourslash/inlineVariableOnlyDeclaration.ts | 4 ++-- .../fourslash/inlineVariableRecursiveInitializer.ts | 4 ++-- .../cases/fourslash/inlineVariableSingleReference.ts | 4 ++-- tests/cases/fourslash/inlineVariableTypeof.ts | 4 ++-- .../cases/fourslash/inlineVariableWriteReference.ts | 4 ++-- 14 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 1c8f9c5da90bd..e4adb058755fd 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -117,9 +117,13 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen const token = getTokenAtPosition(file, startPosition); const parent = token.parent; - // If the node is a variable declaration, make sure it's not in a catch clause or for-loop + if (!isIdentifier(token)) { + return undefined; + } + + // If triggered in a variable declaration, make sure it's not in a catch clause or for-loop // and that it has a value. - if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent) && isIdentifier(parent.name)) { + if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { // Don't inline the variable if it has multiple declarations. if (parent.symbol.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; @@ -136,7 +140,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen } // Try finding the declaration and nodes to replace via the reference token. - if (tryWithReferenceToken && isIdentifier(token)) { + if (tryWithReferenceToken) { const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); if (definition?.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; diff --git a/tests/cases/fourslash/inlineVariableExportedVariable.ts b/tests/cases/fourslash/inlineVariableExportedVariable.ts index 51eb0c8625055..9b9d0bd31a81d 100644 --- a/tests/cases/fourslash/inlineVariableExportedVariable.ts +++ b/tests/cases/fourslash/inlineVariableExportedVariable.ts @@ -1,13 +1,13 @@ /// -////export const x/*a*/ = 1; -////const y/*b*/ = 2; -////const z = x + 1; -////const w = 2 + y; +////export const /*a1*/x/*b1*/ = 1; +////const /*a2*/y/*b2*/ = 2; +////const u = x + 1; +////const v = 2 + y; ////export { y }; -goTo.marker("a"); +goTo.select("a1", "b1"); verify.not.refactorAvailable("Inline variable"); -goTo.marker("b"); +goTo.select("a2", "b2"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableFunctionResult.ts b/tests/cases/fourslash/inlineVariableFunctionResult.ts index b2b0d3d8562b5..e72dc8376fdae 100644 --- a/tests/cases/fourslash/inlineVariableFunctionResult.ts +++ b/tests/cases/fourslash/inlineVariableFunctionResult.ts @@ -1,11 +1,11 @@ /// ////function inc(x: number) { return x + 1; } -////const y/**/ = inc(1); +////const /*a*/y/*b*/ = inc(1); ////const z = y + 1; ////const w = inc(y); -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableJsxCallback.ts b/tests/cases/fourslash/inlineVariableJsxCallback.ts index 77fc7d435ee79..a386f17ee6761 100644 --- a/tests/cases/fourslash/inlineVariableJsxCallback.ts +++ b/tests/cases/fourslash/inlineVariableJsxCallback.ts @@ -5,7 +5,7 @@ // @Filename: file.tsx ////function Button() { -//// const onClick /**/ = () => { +//// const /*a*/onClick/*b*/ = () => { //// console.log("clicked"); //// }; //// @@ -16,7 +16,7 @@ //// ); ////} -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts b/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts index 4eb294cc8f8ba..704c076632234 100644 --- a/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts +++ b/tests/cases/fourslash/inlineVariableMultipleDeclarations.ts @@ -1,11 +1,11 @@ /// -////const foo/*a*/ = "foo"; -////type foo/*b*/ = string; +////const /*a1*/foo/*b1*/ = "foo"; +////type /*a2*/foo/*b2*/ = string; ////type bar = foo; -goTo.marker("a"); +goTo.select("a1", "b1"); verify.not.refactorAvailable("Inline variable"); -goTo.marker("b"); +goTo.select("a2", "b2"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableMultipleScopes.ts b/tests/cases/fourslash/inlineVariableMultipleScopes.ts index 7b6d76348de03..56845b330098f 100644 --- a/tests/cases/fourslash/inlineVariableMultipleScopes.ts +++ b/tests/cases/fourslash/inlineVariableMultipleScopes.ts @@ -1,12 +1,12 @@ /// -////let x/**/ = 1; +////let /*a*/x/*b*/ = 1; ////function foo() { //// console.log(x); ////} ////const y = x + 2; -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableNeedsParens.ts b/tests/cases/fourslash/inlineVariableNeedsParens.ts index 41afad4022f94..a83d34a7630cd 100644 --- a/tests/cases/fourslash/inlineVariableNeedsParens.ts +++ b/tests/cases/fourslash/inlineVariableNeedsParens.ts @@ -1,9 +1,9 @@ /// -////const x/**/ = 1 + 2; +////const /*a*/x/*b*/ = 1 + 2; ////const y = x * 3; -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableNoReplacementValue.ts b/tests/cases/fourslash/inlineVariableNoReplacementValue.ts index 09e06c80f3581..7c7f2b9ce9a06 100644 --- a/tests/cases/fourslash/inlineVariableNoReplacementValue.ts +++ b/tests/cases/fourslash/inlineVariableNoReplacementValue.ts @@ -1,7 +1,7 @@ /// -////let x/**/; +////let /*a*/x/*b*/; ////const y = x + 1; -goTo.marker(""); +goTo.select("a", "b"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableNullCoalescing.ts b/tests/cases/fourslash/inlineVariableNullCoalescing.ts index 568b1171fceeb..7efc9ace0a6f1 100644 --- a/tests/cases/fourslash/inlineVariableNullCoalescing.ts +++ b/tests/cases/fourslash/inlineVariableNullCoalescing.ts @@ -1,10 +1,10 @@ /// ////function foo(): number | undefined { return Math.random() > 0.5 ? 1 : undefined; } -////const x/**/ = foo(); +////const /*a*/x/*b*/ = foo(); ////const y = x?.toString(); -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts b/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts index 728342edf69fe..cc31af2778d6e 100644 --- a/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts +++ b/tests/cases/fourslash/inlineVariableOnlyDeclaration.ts @@ -1,7 +1,7 @@ /// -////const x/**/ = 0; +////const /*a*/x/*b*/ = 0; ////const y = 1 + 2; -goTo.marker(""); +goTo.select("a", "b"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts b/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts index 3e6dbeb4501f6..8ce59e9d77407 100644 --- a/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts +++ b/tests/cases/fourslash/inlineVariableRecursiveInitializer.ts @@ -1,6 +1,6 @@ /// -////const foo/**/ = () => foo(); +////const /*a*/foo/*b*/ = () => foo(); -goTo.marker(""); +goTo.select("a", "b"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableSingleReference.ts b/tests/cases/fourslash/inlineVariableSingleReference.ts index e68bc69edb828..79e05c6d139bf 100644 --- a/tests/cases/fourslash/inlineVariableSingleReference.ts +++ b/tests/cases/fourslash/inlineVariableSingleReference.ts @@ -1,9 +1,9 @@ /// -////const x/**/ = 0; +////const /*a*/x/*b*/ = 0; ////const y = x + 1; -goTo.marker(""); +goTo.select("a", "b"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", diff --git a/tests/cases/fourslash/inlineVariableTypeof.ts b/tests/cases/fourslash/inlineVariableTypeof.ts index 5ef20f4d068e0..9af8f951a33e9 100644 --- a/tests/cases/fourslash/inlineVariableTypeof.ts +++ b/tests/cases/fourslash/inlineVariableTypeof.ts @@ -1,7 +1,7 @@ /// -////const Foo/**/ = class Foo {} +////const /*a*/Foo/*b*/ = class Foo {} ////type FooConstructor = typeof Foo; -goTo.marker(""); +goTo.select("a", "b"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file diff --git a/tests/cases/fourslash/inlineVariableWriteReference.ts b/tests/cases/fourslash/inlineVariableWriteReference.ts index ead2754d70bdd..9c0228f6ce91d 100644 --- a/tests/cases/fourslash/inlineVariableWriteReference.ts +++ b/tests/cases/fourslash/inlineVariableWriteReference.ts @@ -1,7 +1,7 @@ /// -////const x/**/ = 0; +////const /*a*/x/*b*/ = 0; ////const y = x++ + 1; -goTo.marker(""); +goTo.select("a", "b"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From ca090ef66893dae8eb44347730e046589eda42b1 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 13:00:45 -0700 Subject: [PATCH 28/35] Improve exported variable checks --- src/services/refactors/inlineVariable.ts | 25 ++++++++++++++++--- .../inlineVariableExportedVariable.ts | 5 ++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index e4adb058755fd..93bedaa22962f 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -1,24 +1,26 @@ import { - canHaveModifiers, + cast, Debug, Diagnostics, emptyArray, Expression, factory, FindAllReferences, - findAncestor, getExpressionPrecedence, getLocaleSpecificMessage, getSynthesizedDeepClone, getTokenAtPosition, Identifier, InitializedVariableDeclaration, + isExportAssignment, isExportModifier, + isExportSpecifier, isExpression, isIdentifier, isInitializedVariable, isTypeQueryNode, isVariableDeclarationInVariableStatement, + isVariableStatement, needsParentheses, Node, Program, @@ -130,7 +132,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen } // Do not inline if the variable is exported. - if (findAncestor(parent, node => canHaveModifiers(node) && some(node.modifiers, isExportModifier))) { + if (isDeclarationExported(parent)) { return undefined; } @@ -151,6 +153,11 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return undefined; } + // Do not inline if the variable is exported. + if (isDeclarationExported(declaration)) { + return undefined; + } + const references = getReferenceNodes(declaration, checker, file); return references && { references, declaration, replacement: declaration.initializer }; } @@ -158,6 +165,13 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_variable_to_inline) }; } +function isDeclarationExported(declaration: InitializedVariableDeclaration): boolean { + // We use this function after isVariableDeclarationInVariableStatement, which ensures + // that the cast below succeeds. + const variableStatement = cast(declaration.parent.parent, isVariableStatement); + return some(variableStatement.modifiers, isExportModifier); +} + function getReferenceNodes(declaration: InitializedVariableDeclaration, checker: TypeChecker, file: SourceFile): Identifier[] | undefined { const references: Identifier[] = []; const cannotInline = FindAllReferences.Core.eachSymbolReferenceInFile(declaration.name as Identifier, checker, file, ref => { @@ -167,6 +181,11 @@ function getReferenceNodes(declaration: InitializedVariableDeclaration, checker: return true; } + // We cannot inline exported variables like "export { x as y }" or "export default foo". + if (isExportSpecifier(ref.parent) || isExportAssignment(ref.parent)) { + return true; + } + // typeof needs an identifier, so we can't inline a value in there. if (isTypeQueryNode(ref.parent)) { return true; diff --git a/tests/cases/fourslash/inlineVariableExportedVariable.ts b/tests/cases/fourslash/inlineVariableExportedVariable.ts index 9b9d0bd31a81d..26ef4f7034b85 100644 --- a/tests/cases/fourslash/inlineVariableExportedVariable.ts +++ b/tests/cases/fourslash/inlineVariableExportedVariable.ts @@ -2,12 +2,17 @@ ////export const /*a1*/x/*b1*/ = 1; ////const /*a2*/y/*b2*/ = 2; +////const /*a3*/z/*b3*/ = 3; ////const u = x + 1; ////const v = 2 + y; ////export { y }; +////export { z as w }; goTo.select("a1", "b1"); verify.not.refactorAvailable("Inline variable"); goTo.select("a2", "b2"); +verify.not.refactorAvailable("Inline variable"); + +goTo.select("a3", "b3"); verify.not.refactorAvailable("Inline variable"); \ No newline at end of file From 4c8335fc912c3ca980a208ccdb1e939beca35ada Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 14:03:53 -0700 Subject: [PATCH 29/35] Handle functions --- src/services/refactors/inlineVariable.ts | 8 ++++++++ .../fourslash/inlineVariableNeedsParens.ts | 20 ++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 93bedaa22962f..a40fc9ef9cec8 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -12,10 +12,12 @@ import { getTokenAtPosition, Identifier, InitializedVariableDeclaration, + isCallLikeExpression, isExportAssignment, isExportModifier, isExportSpecifier, isExpression, + isFunctionLike, isIdentifier, isInitializedVariable, isTypeQueryNode, @@ -218,5 +220,11 @@ function getReplacementExpression(reference: Node, replacement: Expression): Exp return factory.createParenthesizedExpression(replacement); } + // Functions also need to be parenthesized. + // E.g.: const f = () => {}; f(); -> (() => {})(); + if (isFunctionLike(replacement) && isCallLikeExpression(parent)) { + return factory.createParenthesizedExpression(replacement); + } + return replacement; } diff --git a/tests/cases/fourslash/inlineVariableNeedsParens.ts b/tests/cases/fourslash/inlineVariableNeedsParens.ts index a83d34a7630cd..6d4edfa1288fe 100644 --- a/tests/cases/fourslash/inlineVariableNeedsParens.ts +++ b/tests/cases/fourslash/inlineVariableNeedsParens.ts @@ -1,13 +1,27 @@ /// -////const /*a*/x/*b*/ = 1 + 2; +////const /*a1*/x/*b1*/ = 1 + 2; ////const y = x * 3; +////const /*a2*/f/*b2*/ = () => { }; +////f(); -goTo.select("a", "b"); +goTo.select("a1", "b1"); verify.refactorAvailable("Inline variable"); edit.applyRefactor({ refactorName: "Inline variable", actionName: "Inline variable", actionDescription: "Inline variable", - newContent: "const y = (1 + 2) * 3;" + newContent: `const y = (1 + 2) * 3; +const f = () => { }; +f();` +}); + +goTo.select("a2", "b2"); +verify.refactorAvailable("Inline variable"); +edit.applyRefactor({ + refactorName: "Inline variable", + actionName: "Inline variable", + actionDescription: "Inline variable", + newContent: `const y = (1 + 2) * 3; +(() => { })();` }); \ No newline at end of file From 206a2df26ab27f3559362cf53228c20bfba9a76c Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 14:15:20 -0700 Subject: [PATCH 30/35] Look at the merged symbol --- src/services/refactors/inlineVariable.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index a40fc9ef9cec8..74b8101093f2a 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -129,7 +129,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // and that it has a value. if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { // Don't inline the variable if it has multiple declarations. - if (parent.symbol.declarations?.length !== 1) { + if (checker.getMergedSymbol(parent.symbol).declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } @@ -145,7 +145,9 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Try finding the declaration and nodes to replace via the reference token. if (tryWithReferenceToken) { - const definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); + let definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); + definition = definition && checker.getMergedSymbol(definition); + if (definition?.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } From 49b38dbef72f1d073edad76c765d8c92576fdc56 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 14:21:06 -0700 Subject: [PATCH 31/35] More comments --- src/services/refactors/inlineVariable.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 74b8101093f2a..34d97d65bf308 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -99,6 +99,8 @@ registerRefactor(refactorName, { const { file, program, startPosition } = context; + // tryWithReferenceToken is true below since here we're already performing the refactor. + // The trigger kind was only relevant when checking the availability of the refactor. const info = getInliningInfo(file, startPosition, /*tryWithReferenceToken*/ true, program); if (!info || refactor.isRefactorErrorInfo(info)) { return undefined; @@ -148,10 +150,12 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen let definition = checker.resolveName(token.text, token, SymbolFlags.Value, /*excludeGlobals*/ false); definition = definition && checker.getMergedSymbol(definition); + // Don't inline the variable if it has multiple declarations. if (definition?.declarations?.length !== 1) { return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; } + // Make sure we're not inlining something like "let foo;" or "for (let i = 0; i < 5; i++) {}". const declaration = definition.declarations[0]; if (!isInitializedVariable(declaration) || !isVariableDeclarationInVariableStatement(declaration) || !isIdentifier(declaration.name)) { return undefined; From 67a5880ccdbedd52e2131eec2a421b6e70803c20 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 15:04:44 -0700 Subject: [PATCH 32/35] Improve error message --- src/compiler/diagnosticMessages.json | 2 +- src/services/refactors/inlineVariable.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 2e97e4229d757..65ccc4cb18fdc 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -7640,7 +7640,7 @@ "category": "Message", "code": 95185 }, - "Variables that share a name with a type or namespace in the same scope cannot be inlined.": { + "Variables with multiple declarations cannot be inlined.": { "category": "Message", "code": 95186 }, diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index 34d97d65bf308..e430f7bd23c2b 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -132,7 +132,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen if (isInitializedVariable(parent) && isVariableDeclarationInVariableStatement(parent)) { // Don't inline the variable if it has multiple declarations. if (checker.getMergedSymbol(parent.symbol).declarations?.length !== 1) { - return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; + return { error: getLocaleSpecificMessage(Diagnostics.Variables_with_multiple_declarations_cannot_be_inlined) }; } // Do not inline if the variable is exported. @@ -152,7 +152,7 @@ function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferen // Don't inline the variable if it has multiple declarations. if (definition?.declarations?.length !== 1) { - return { error: getLocaleSpecificMessage(Diagnostics.Variables_that_share_a_name_with_a_type_or_namespace_in_the_same_scope_cannot_be_inlined) }; + return { error: getLocaleSpecificMessage(Diagnostics.Variables_with_multiple_declarations_cannot_be_inlined) }; } // Make sure we're not inlining something like "let foo;" or "for (let i = 0; i < 5; i++) {}". From 7c7202a453c8a098b1dfcb4cb5b995db6495a2ea Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 15:48:47 -0700 Subject: [PATCH 33/35] control flow nits --- src/services/refactors/inlineVariable.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index e430f7bd23c2b..d21c2a1ed58e0 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -72,22 +72,22 @@ registerRefactor(refactorName, { return emptyArray; } - if (refactor.isRefactorErrorInfo(info) && preferences.provideRefactorNotApplicableReason) { + if (!refactor.isRefactorErrorInfo(info)) { return [{ name: refactorName, description: refactorDescription, - actions: [{ - ...inlineVariableAction, - notApplicableReason: info.error - }] + actions: [inlineVariableAction] }]; } - if (!refactor.isRefactorErrorInfo(info)) { + if (preferences.provideRefactorNotApplicableReason) { return [{ name: refactorName, description: refactorDescription, - actions: [inlineVariableAction] + actions: [{ + ...inlineVariableAction, + notApplicableReason: info.error + }] }]; } From 45396103940a5f5dd2ce644cc5942e7c0abf74b5 Mon Sep 17 00:00:00 2001 From: Maria Solano Date: Mon, 12 Jun 2023 16:18:31 -0700 Subject: [PATCH 34/35] Use getTouchingPropertyName --- src/services/refactors/inlineVariable.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/refactors/inlineVariable.ts b/src/services/refactors/inlineVariable.ts index d21c2a1ed58e0..2b0b43bf426a4 100644 --- a/src/services/refactors/inlineVariable.ts +++ b/src/services/refactors/inlineVariable.ts @@ -9,7 +9,7 @@ import { getExpressionPrecedence, getLocaleSpecificMessage, getSynthesizedDeepClone, - getTokenAtPosition, + getTouchingPropertyName, Identifier, InitializedVariableDeclaration, isCallLikeExpression, @@ -120,7 +120,7 @@ registerRefactor(refactorName, { function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferenceToken: boolean, program: Program): InliningInfo | RefactorErrorInfo | undefined { const checker = program.getTypeChecker(); - const token = getTokenAtPosition(file, startPosition); + const token = getTouchingPropertyName(file, startPosition); const parent = token.parent; if (!isIdentifier(token)) { From b040942d1d74c6eb6bdc8a31c1c3a1210a9446c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Mon, 12 Jun 2023 20:02:14 -0700 Subject: [PATCH 35/35] Add availability test --- .../inlineVariableRefactorAvailability.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 tests/cases/fourslash/inlineVariableRefactorAvailability.ts diff --git a/tests/cases/fourslash/inlineVariableRefactorAvailability.ts b/tests/cases/fourslash/inlineVariableRefactorAvailability.ts new file mode 100644 index 0000000000000..67348ba42fc54 --- /dev/null +++ b/tests/cases/fourslash/inlineVariableRefactorAvailability.ts @@ -0,0 +1,16 @@ +/// + +////const/*a*/ /*b*/x/*c*/ = /*d*/1; +////const y = x + 2; + +goTo.marker("a"); +verify.not.refactorAvailable("Inline variable"); + +goTo.marker("b"); +verify.refactorAvailable("Inline variable"); + +goTo.marker("c"); +verify.refactorAvailable("Inline variable"); + +goTo.marker("d"); +verify.not.refactorAvailable("Inline variable");