Skip to content

Add inline variable refactor #54281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
Jun 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
308e643
Add inline variable refactor
May 16, 2023
307779e
Basic tests
May 17, 2023
007a0e1
Offer refactor in reference site
May 17, 2023
c39c34c
Do not offer refactor if there is a write
May 19, 2023
e477282
Add parenthesize logic
May 19, 2023
32234c0
More tests
May 19, 2023
7245421
Update baselines
May 22, 2023
6d83ed0
Another test
May 22, 2023
5579a48
Define the refactor kind
May 23, 2023
3ad106d
Limit FAR to one file
May 25, 2023
c0e302a
Handle some more invalid cases
May 25, 2023
095d3b4
Use eachSymbolReferenceInFile
May 25, 2023
ce734ff
Another error msg
May 25, 2023
c679717
Exported variable test
May 25, 2023
5e0dfe6
Update src/compiler/diagnosticMessages.json
MariaSolOs May 25, 2023
225b759
Update messages
May 25, 2023
2fb8b04
Handle case where declaration has export
May 29, 2023
1e6d8cd
More tests
May 29, 2023
b2d9018
Add inline callback case
May 30, 2023
9bc6c05
Handle Daniel's recursive case
May 31, 2023
a259b3a
Improve comments
MariaSolOs Jun 7, 2023
b308e99
Use deep clones for replacements
MariaSolOs Jun 7, 2023
76bd29a
Use textRangeContainsPositionInclusive
MariaSolOs Jun 7, 2023
4080541
deep clone for each replacement
MariaSolOs Jun 9, 2023
9f39bfb
Update src/services/refactors/inlineVariable.ts
MariaSolOs Jun 9, 2023
4fa54f7
Clone the replacement node
Jun 12, 2023
4e608ef
Restrict availability to identifier only
Jun 12, 2023
ca090ef
Improve exported variable checks
Jun 12, 2023
4c8335f
Handle functions
Jun 12, 2023
206a2df
Look at the merged symbol
Jun 12, 2023
49b38db
More comments
Jun 12, 2023
67a5880
Improve error message
Jun 12, 2023
7c7202a
control flow nits
Jun 12, 2023
4539610
Use getTouchingPropertyName
Jun 12, 2023
b040942
Add availability test
MariaSolOs Jun 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -7632,6 +7632,18 @@
"category": "Message",
"code": 95183
},
"Inline variable": {
"category": "Message",
"code": 95184
},
"Could not find variable to inline.": {
"category": "Message",
"code": 95185
},
"Variables with multiple declarations cannot 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",
Expand Down
1 change: 1 addition & 0 deletions src/services/_namespaces/ts.refactor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
8 changes: 6 additions & 2 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
236 changes: 236 additions & 0 deletions src/services/refactors/inlineVariable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
import {
cast,
Debug,
Diagnostics,
emptyArray,
Expression,
factory,
FindAllReferences,
getExpressionPrecedence,
getLocaleSpecificMessage,
getSynthesizedDeepClone,
getTouchingPropertyName,
Identifier,
InitializedVariableDeclaration,
isCallLikeExpression,
isExportAssignment,
isExportModifier,
isExportSpecifier,
isExpression,
isFunctionLike,
isIdentifier,
isInitializedVariable,
isTypeQueryNode,
isVariableDeclarationInVariableStatement,
isVariableStatement,
needsParentheses,
Node,
Program,
refactor,
some,
SourceFile,
SymbolFlags,
textChanges,
textRangeContainsPositionInclusive,
TypeChecker,
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.inline.variable"
};

interface InliningInfo {
references: Node[];
declaration: VariableDeclaration;
replacement: Expression;
}

registerRefactor(refactorName, {
kinds: [inlineVariableAction.kind],

getAvailableActions(context) {
const {
file,
program,
preferences,
startPosition,
triggerReason
} = context;

// 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);
if (!info) {
return emptyArray;
}

if (!refactor.isRefactorErrorInfo(info)) {
return [{
name: refactorName,
description: refactorDescription,
actions: [inlineVariableAction]
}];
}

if (preferences.provideRefactorNotApplicableReason) {
return [{
name: refactorName,
description: refactorDescription,
actions: [{
...inlineVariableAction,
notApplicableReason: info.error
}]
}];
}

return emptyArray;
},

getEditsForAction(context, actionName) {
Debug.assert(actionName === refactorName, "Unexpected refactor invoked");

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;
}

const { references, declaration, replacement } = info;
const edits = textChanges.ChangeTracker.with(context, tracker => {
for (const node of references) {
tracker.replaceNode(file, node, getReplacementExpression(node, replacement));
}
tracker.delete(file, declaration);
});

return { edits };
}
});

function getInliningInfo(file: SourceFile, startPosition: number, tryWithReferenceToken: boolean, program: Program): InliningInfo | RefactorErrorInfo | undefined {
const checker = program.getTypeChecker();
const token = getTouchingPropertyName(file, startPosition);
const parent = token.parent;

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)) {
// Don't inline the variable if it has multiple declarations.
if (checker.getMergedSymbol(parent.symbol).declarations?.length !== 1) {
return { error: getLocaleSpecificMessage(Diagnostics.Variables_with_multiple_declarations_cannot_be_inlined) };
}

// Do not inline if the variable is exported.
if (isDeclarationExported(parent)) {
return undefined;
}

// Find all references to the variable in the current file.
const references = getReferenceNodes(parent, checker, file);
return references && { references, declaration: parent, replacement: parent.initializer };
}

// Try finding the declaration and nodes to replace via the reference token.
if (tryWithReferenceToken) {
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_with_multiple_declarations_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;
}

// 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 };
}

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 => {
// 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)) {
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;
}

// Cannot inline recursive declarations (e.g. const foo = () => foo();)
if (textRangeContainsPositionInclusive(declaration, ref.pos)) {
return true;
}

references.push(ref);
});

return references.length === 0 || cannotInline ? undefined : references;
}

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."
//
// 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.
if (isExpression(parent) && (getExpressionPrecedence(replacement) < getExpressionPrecedence(parent) || needsParentheses(parent))) {
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;
}
18 changes: 18 additions & 0 deletions tests/cases/fourslash/inlineVariableExportedVariable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path="fourslash.ts" />

////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");
17 changes: 17 additions & 0 deletions tests/cases/fourslash/inlineVariableFunctionResult.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />

////function inc(x: number) { return x + 1; }
////const /*a*/y/*b*/ = inc(1);
////const z = y + 1;
////const w = inc(y);

goTo.select("a", "b");
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));`
});
35 changes: 35 additions & 0 deletions tests/cases/fourslash/inlineVariableJsxCallback.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/// <reference path='fourslash.ts'/>

//@module: commonjs
//@jsx: preserve

// @Filename: file.tsx
////function Button() {
//// const /*a*/onClick/*b*/ = () => {
//// console.log("clicked");
//// };
////
//// return (
//// <button onClick={onClick}>
//// Click me!
//// </button>
//// );
////}

goTo.select("a", "b");
verify.refactorAvailable("Inline variable");
edit.applyRefactor({
refactorName: "Inline variable",
actionName: "Inline variable",
actionDescription: "Inline variable",
newContent: `function Button() {

return (
<button onClick={() => {
console.log("clicked");
}}>
Click me!
</button>
);
}`
});
11 changes: 11 additions & 0 deletions tests/cases/fourslash/inlineVariableMultipleDeclarations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path="fourslash.ts" />

////const /*a1*/foo/*b1*/ = "foo";
////type /*a2*/foo/*b2*/ = string;
////type bar = foo;

goTo.select("a1", "b1");
verify.not.refactorAvailable("Inline variable");

goTo.select("a2", "b2");
verify.not.refactorAvailable("Inline variable");
19 changes: 19 additions & 0 deletions tests/cases/fourslash/inlineVariableMultipleScopes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// <reference path="fourslash.ts" />

////let /*a*/x/*b*/ = 1;
////function foo() {
//// console.log(x);
////}
////const y = x + 2;

goTo.select("a", "b");
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;`
});
Loading