-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add refactor convertToOptionalChainExpression #39135
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
jessetrinity
merged 49 commits into
microsoft:master
from
jessetrinity:refactorOptionalChain
Jul 14, 2020
Merged
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
77a49c3
add convertOptionalChain
9cf07cf
cover more cases
80bf5d1
expose containsMatchingReference
4e64236
clean up performing edits
9a59e13
bound start position
6c1bccf
add tests
2a355e4
refactor and handle edge cases
3dac9c6
update tests
b403037
consider explicit requests for empty spans
1ae5500
update fourslash to use trigger reason
bf28673
add tests cases for trigger reason
fa3f9c8
fix errors
8794154
remove type assertion
3d8fed1
fix non ampersand chains
b5c833c
clean up some logic
796b2bf
add ternary case
37004b7
add diagnostic message
3584bae
resolve merge conflict
1b2e86b
Merge remote-tracking branch 'upstream/master' into refactorOptionalC…
afb0e44
add nullish check for ternary expressions
65ca81e
Update src/services/refactors/convertToOptionalChainExpression.ts
jessetrinity d9c34ff
Update src/services/refactors/convertToOptionalChainExpression.ts
jessetrinity c2b9924
Update tests/cases/fourslash/refactorConvertToOptionalChainExpression…
jessetrinity 1d51dab
Update tests/cases/fourslash/refactorConvertToOptionalChainExpression…
jessetrinity adbd586
reformat and remove unused checks
fb6b831
allow any for ternary refactor
8184ecf
add tests
5ac29a0
add tests
a0708be
check return and variable statements
6810cea
use isMatchingReference instead of containsMatchingReference
5634a4c
allow partial selections
e6e54cb
fine tune selection ranges
77f47e7
recurse for call expressions
1ba5fd0
fix spellings
6095e95
add recursive cases
89c8c9d
remove isOrContainsMatchingReference
b20cd95
cleanup
15176b2
more refactoring
2de5e28
cleanup
4126a46
rename tests
8f65c02
address PR comments
d4f6f52
Merge remote-tracking branch 'upstream/master' into refactorOptionalC…
8aed315
check match syntactically
602075f
handle another call expression case
a4cc060
some renames
e89ae08
inline some checks
2cdb5e1
add test
fd64b14
address comments
01854bb
add refactorNotAvailableReason
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
284 changes: 284 additions & 0 deletions
284
src/services/refactors/convertToOptionalChainExpression.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,284 @@ | ||
/* @internal */ | ||
namespace ts.refactor.convertToOptionalChainExpression { | ||
const refactorName = "Convert to optional chain expression"; | ||
const convertToOptionalChainExpressionMessage = getLocaleSpecificMessage(Diagnostics.Convert_to_optional_chain_expression); | ||
|
||
registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); | ||
|
||
function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { | ||
const info = getInfo(context, context.triggerReason === "invoked"); | ||
if (!info) return emptyArray; | ||
|
||
if (!info.error) { | ||
return [{ | ||
name: refactorName, | ||
description: convertToOptionalChainExpressionMessage, | ||
actions: [{ | ||
name: refactorName, | ||
description: convertToOptionalChainExpressionMessage | ||
}] | ||
}]; | ||
} | ||
|
||
if (context.preferences.provideRefactorNotApplicableReason) { | ||
return [{ | ||
name: refactorName, | ||
description: convertToOptionalChainExpressionMessage, | ||
actions: [{ | ||
name: refactorName, | ||
description: convertToOptionalChainExpressionMessage, | ||
notApplicableReason: info.error | ||
}] | ||
}]; | ||
} | ||
return emptyArray; | ||
} | ||
|
||
function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { | ||
const info = getInfo(context); | ||
if (!info || !info.info) return undefined; | ||
const edits = textChanges.ChangeTracker.with(context, t => | ||
doChange(context.file, context.program.getTypeChecker(), t, Debug.checkDefined(info.info, "context must have info"), actionName) | ||
); | ||
return { edits, renameFilename: undefined, renameLocation: undefined }; | ||
} | ||
|
||
type InfoOrError = { | ||
info: Info, | ||
error?: never; | ||
} | { | ||
info?: never, | ||
error: string; | ||
}; | ||
|
||
interface Info { | ||
finalExpression: PropertyAccessExpression | CallExpression, | ||
occurrences: (PropertyAccessExpression | Identifier)[], | ||
expression: ValidExpression, | ||
}; | ||
|
||
type ValidExpressionOrStatement = ValidExpression | ValidStatement; | ||
|
||
/** | ||
* Types for which a "Convert to optional chain refactor" are offered. | ||
*/ | ||
type ValidExpression = BinaryExpression | ConditionalExpression; | ||
|
||
/** | ||
* Types of statements which are likely to include a valid expression for extraction. | ||
*/ | ||
type ValidStatement = ExpressionStatement | ReturnStatement | VariableStatement; | ||
|
||
function isValidExpression(node: Node): node is ValidExpression { | ||
return isBinaryExpression(node) || isConditionalExpression(node); | ||
} | ||
|
||
function isValidStatement(node: Node): node is ValidStatement { | ||
return isExpressionStatement(node) || isReturnStatement(node) || isVariableStatement(node); | ||
} | ||
|
||
function isValidExpressionOrStatement(node: Node): node is ValidExpressionOrStatement { | ||
return isValidExpression(node) || isValidStatement(node); | ||
} | ||
|
||
function getInfo(context: RefactorContext, considerEmptySpans = true): InfoOrError | undefined { | ||
const { file, program } = context; | ||
const span = getRefactorContextSpan(context); | ||
|
||
const forEmptySpan = span.length === 0; | ||
if (forEmptySpan && !considerEmptySpans) return undefined; | ||
|
||
// selecting fo[|o && foo.ba|]r should be valid, so adjust span to fit start and end tokens | ||
const startToken = getTokenAtPosition(file, span.start); | ||
const endToken = findTokenOnLeftOfPosition(file, span.start + span.length); | ||
const adjustedSpan = createTextSpanFromBounds(startToken.pos, endToken && endToken.end >= startToken.pos ? endToken.getEnd() : startToken.getEnd()); | ||
|
||
const parent = forEmptySpan ? getValidParentNodeOfEmptySpan(startToken) : getValidParentNodeContainingSpan(startToken, adjustedSpan); | ||
const expression = parent && isValidExpressionOrStatement(parent) ? getExpression(parent) : undefined; | ||
if (!expression) return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_convertible_access_expression) }; | ||
|
||
const checker = program.getTypeChecker(); | ||
return isConditionalExpression(expression) ? getConditionalInfo(expression, checker) : getBinaryInfo(expression); | ||
} | ||
|
||
function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): InfoOrError | undefined { | ||
const condition = expression.condition; | ||
const finalExpression = getFinalExpressionInChain(expression.whenTrue); | ||
|
||
if (!finalExpression || checker.isNullableType(checker.getTypeAtLocation(finalExpression))) { | ||
return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_convertible_access_expression) }; | ||
}; | ||
|
||
if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) | ||
&& getMatchingStart(condition, finalExpression.expression)) { | ||
return { info: { finalExpression, occurrences: [condition], expression } }; | ||
} | ||
else if (isBinaryExpression(condition)) { | ||
const occurrences = getOccurrencesInExpression(finalExpression.expression, condition); | ||
return occurrences ? { info: { finalExpression, occurrences, expression } } : | ||
{ error: getLocaleSpecificMessage(Diagnostics.Could_not_find_matching_access_expressions) }; | ||
} | ||
} | ||
|
||
function getBinaryInfo(expression: BinaryExpression): InfoOrError | undefined { | ||
if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) { | ||
return { error: getLocaleSpecificMessage(Diagnostics.Can_only_convert_logical_AND_access_chains) }; | ||
}; | ||
const finalExpression = getFinalExpressionInChain(expression.right); | ||
|
||
if (!finalExpression) return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_convertible_access_expression) }; | ||
|
||
const occurrences = getOccurrencesInExpression(finalExpression.expression, expression.left); | ||
return occurrences ? { info: { finalExpression, occurrences, expression } } : | ||
{ error: getLocaleSpecificMessage(Diagnostics.Could_not_find_matching_access_expressions) }; | ||
} | ||
|
||
/** | ||
* Gets a list of property accesses that appear in matchTo and occur in sequence in expression. | ||
*/ | ||
function getOccurrencesInExpression(matchTo: Expression, expression: Expression): (PropertyAccessExpression | Identifier)[] | undefined { | ||
const occurrences: (PropertyAccessExpression | Identifier)[] = []; | ||
while (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { | ||
const match = getMatchingStart(skipParentheses(matchTo), skipParentheses(expression.right)); | ||
if (!match) { | ||
break; | ||
} | ||
occurrences.push(match); | ||
matchTo = match; | ||
expression = expression.left; | ||
} | ||
const finalMatch = getMatchingStart(matchTo, expression); | ||
if (finalMatch) { | ||
occurrences.push(finalMatch); | ||
} | ||
return occurrences.length > 0 ? occurrences: undefined; | ||
} | ||
|
||
/** | ||
* Returns subchain if chain begins with subchain syntactically. | ||
*/ | ||
function getMatchingStart(chain: Expression, subchain: Expression): PropertyAccessExpression | Identifier | undefined { | ||
return (isIdentifier(subchain) || isPropertyAccessExpression(subchain)) && | ||
chainStartsWith(chain, subchain) ? subchain : undefined; | ||
} | ||
|
||
/** | ||
* Returns true if chain begins with subchain syntactically. | ||
*/ | ||
function chainStartsWith(chain: Node, subchain: Node): boolean { | ||
// skip until we find a matching identifier. | ||
while (isCallExpression(chain) || isPropertyAccessExpression(chain)) { | ||
const subchainName = isPropertyAccessExpression(subchain) ? subchain.name.getText() : subchain.getText(); | ||
if (isPropertyAccessExpression(chain) && chain.name.getText() === subchainName) break; | ||
chain = chain.expression; | ||
} | ||
// check that the chains match at each access. Call chains in subchain are not valid. | ||
while (isPropertyAccessExpression(chain) && isPropertyAccessExpression(subchain)) { | ||
if (chain.name.getText() !== subchain.name.getText()) return false; | ||
chain = chain.expression; | ||
subchain = subchain.expression; | ||
} | ||
// check if we have reached a final identifier. | ||
return isIdentifier(chain) && isIdentifier(subchain) && chain.getText() === subchain.getText(); | ||
} | ||
|
||
/** | ||
* Find the least ancestor of the input node that is a valid type for extraction and contains the input span. | ||
*/ | ||
function getValidParentNodeContainingSpan(node: Node, span: TextSpan): ValidExpressionOrStatement | undefined { | ||
while (node.parent) { | ||
if (isValidExpressionOrStatement(node) && span.length !== 0 && node.end >= span.start + span.length) { | ||
return node; | ||
} | ||
node = node.parent; | ||
} | ||
return undefined; | ||
} | ||
|
||
/** | ||
* Finds an ancestor of the input node that is a valid type for extraction, skipping subexpressions. | ||
*/ | ||
function getValidParentNodeOfEmptySpan(node: Node): ValidExpressionOrStatement | undefined { | ||
while (node.parent) { | ||
if (isValidExpressionOrStatement(node) && !isValidExpressionOrStatement(node.parent)) { | ||
return node; | ||
} | ||
node = node.parent; | ||
} | ||
return undefined; | ||
} | ||
|
||
/** | ||
* Gets an expression of valid extraction type from a valid statement or expression. | ||
*/ | ||
function getExpression(node: ValidExpressionOrStatement): ValidExpression | undefined { | ||
if (isValidExpression(node)) { | ||
return node; | ||
} | ||
if (isVariableStatement(node)) { | ||
const variable = getSingleVariableOfVariableStatement(node); | ||
const initializer = variable?.initializer; | ||
return initializer && isValidExpression(initializer) ? initializer : undefined; | ||
} | ||
return node.expression && isValidExpression(node.expression) ? node.expression : undefined; | ||
} | ||
|
||
/** | ||
* Gets a property access expression which may be nested inside of a binary expression. The final | ||
* expression in an && chain will occur as the right child of the parent binary expression, unless | ||
* it is followed by a different binary operator. | ||
* @param node the right child of a binary expression or a call expression. | ||
*/ | ||
function getFinalExpressionInChain(node: Expression): CallExpression | PropertyAccessExpression | undefined { | ||
// foo && |foo.bar === 1|; - here the right child of the && binary expression is another binary expression. | ||
// the rightmost member of the && chain should be the leftmost child of that expression. | ||
node = skipParentheses(node); | ||
if (isBinaryExpression(node)) { | ||
return getFinalExpressionInChain(node.left); | ||
} | ||
// foo && |foo.bar()()| - nested calls are treated like further accesses. | ||
else if ((isPropertyAccessExpression(node) || isCallExpression(node)) && !isOptionalChain(node)) { | ||
return node; | ||
} | ||
return undefined; | ||
} | ||
|
||
/** | ||
* Creates an access chain from toConvert with '?.' accesses at expressions appearing in occurrences. | ||
*/ | ||
function convertOccurrences(checker: TypeChecker, toConvert: Expression, occurrences: (PropertyAccessExpression | Identifier)[]): Expression { | ||
if (isPropertyAccessExpression(toConvert) || isCallExpression(toConvert)) { | ||
const chain = convertOccurrences(checker, toConvert.expression, occurrences); | ||
const lastOccurrence = occurrences.length > 0 ? occurrences[occurrences.length - 1] : undefined; | ||
const isOccurrence = lastOccurrence?.getText() === toConvert.expression.getText(); | ||
if (isOccurrence) occurrences.pop(); | ||
if (isCallExpression(toConvert)) { | ||
return isOccurrence ? | ||
factory.createCallChain(chain, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.typeArguments, toConvert.arguments) : | ||
factory.createCallChain(chain, toConvert.questionDotToken, toConvert.typeArguments, toConvert.arguments); | ||
} | ||
else if (isPropertyAccessExpression(toConvert)) { | ||
return isOccurrence ? | ||
factory.createPropertyAccessChain(chain, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name) : | ||
factory.createPropertyAccessChain(chain, toConvert.questionDotToken, toConvert.name); | ||
} | ||
} | ||
return toConvert; | ||
} | ||
|
||
function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { | ||
const { finalExpression, occurrences, expression } = info; | ||
const firstOccurrence = occurrences[occurrences.length - 1]; | ||
const convertedChain = convertOccurrences(checker, finalExpression, occurrences); | ||
if (convertedChain && (isPropertyAccessExpression(convertedChain) || isCallExpression(convertedChain))) { | ||
if (isBinaryExpression(expression)) { | ||
changes.replaceNodeRange(sourceFile, firstOccurrence, finalExpression, convertedChain); | ||
} | ||
else if (isConditionalExpression(expression)) { | ||
changes.replaceNode(sourceFile, expression, | ||
factory.createBinaryExpression(convertedChain, factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse) | ||
); | ||
} | ||
} | ||
} | ||
} |
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
14 changes: 14 additions & 0 deletions
14
tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.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,14 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
////let a = { b: () => { return () => { c: 0 } } } | ||
/////*a*/a && a.b && a.b()().c/*b*/; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Convert to optional chain expression", | ||
actionName: "Convert to optional chain expression", | ||
actionDescription: "Convert to optional chain expression", | ||
newContent: | ||
`let a = { b: () => { return () => { c: 0 } } } | ||
a?.b?.()().c;` | ||
}); |
14 changes: 14 additions & 0 deletions
14
tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.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,14 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
////let a = { b: () => { return { c: 0 } } } | ||
/////*a*/a && a.b && a.b().c/*b*/; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Convert to optional chain expression", | ||
actionName: "Convert to optional chain expression", | ||
actionDescription: "Convert to optional chain expression", | ||
newContent: | ||
`let a = { b: () => { return { c: 0 } } } | ||
a?.b?.().c;` | ||
}); |
12 changes: 12 additions & 0 deletions
12
tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessThenCall.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,12 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
/////*a*/a && a.b && a.b()/*b*/; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Convert to optional chain expression", | ||
actionName: "Convert to optional chain expression", | ||
actionDescription: "Convert to optional chain expression", | ||
newContent: | ||
`a?.b?.();` | ||
}); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also
node = skipParentheses(node)
at the top hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be an example we care to test, something like
a && (a).b
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep,
foo && (foo.bar === 1)
seems more realistic since the precedence between different binary operators can be hard to remember, but I think if you putskipParentheses
in the right places, you can trivially handlefoo && (((foo).bar) === 1)
😁