-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add inline variable refactor #54281
Changes from 34 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
308e643
Add inline variable refactor
307779e
Basic tests
007a0e1
Offer refactor in reference site
c39c34c
Do not offer refactor if there is a write
e477282
Add parenthesize logic
32234c0
More tests
7245421
Update baselines
6d83ed0
Another test
5579a48
Define the refactor kind
3ad106d
Limit FAR to one file
c0e302a
Handle some more invalid cases
095d3b4
Use eachSymbolReferenceInFile
ce734ff
Another error msg
c679717
Exported variable test
5e0dfe6
Update src/compiler/diagnosticMessages.json
MariaSolOs 225b759
Update messages
2fb8b04
Handle case where declaration has export
1e6d8cd
More tests
b2d9018
Add inline callback case
9bc6c05
Handle Daniel's recursive case
a259b3a
Improve comments
MariaSolOs b308e99
Use deep clones for replacements
MariaSolOs 76bd29a
Use textRangeContainsPositionInclusive
MariaSolOs 4080541
deep clone for each replacement
MariaSolOs 9f39bfb
Update src/services/refactors/inlineVariable.ts
MariaSolOs 4fa54f7
Clone the replacement node
4e608ef
Restrict availability to identifier only
ca090ef
Improve exported variable checks
4c8335f
Handle functions
206a2df
Look at the merged symbol
49b38db
More comments
67a5880
Improve error message
7c7202a
control flow nits
4539610
Use getTouchingPropertyName
b040942
Add availability test
MariaSolOs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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));` | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
11
tests/cases/fourslash/inlineVariableMultipleDeclarations.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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;` | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.