Skip to content
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

Add refactor convertToOptionalChainExpression #39135

Merged
merged 49 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
77a49c3
add convertOptionalChain
Jun 12, 2020
9cf07cf
cover more cases
Jun 16, 2020
80bf5d1
expose containsMatchingReference
Jun 17, 2020
4e64236
clean up performing edits
Jun 17, 2020
9a59e13
bound start position
Jun 17, 2020
6c1bccf
add tests
Jun 17, 2020
2a355e4
refactor and handle edge cases
Jun 17, 2020
3dac9c6
update tests
Jun 18, 2020
b403037
consider explicit requests for empty spans
Jun 18, 2020
1ae5500
update fourslash to use trigger reason
Jun 18, 2020
bf28673
add tests cases for trigger reason
Jun 18, 2020
fa3f9c8
fix errors
Jun 18, 2020
8794154
remove type assertion
Jun 18, 2020
3d8fed1
fix non ampersand chains
Jun 18, 2020
b5c833c
clean up some logic
Jun 18, 2020
796b2bf
add ternary case
Jun 20, 2020
37004b7
add diagnostic message
Jun 20, 2020
3584bae
resolve merge conflict
Jun 20, 2020
1b2e86b
Merge remote-tracking branch 'upstream/master' into refactorOptionalC…
Jun 22, 2020
afb0e44
add nullish check for ternary expressions
Jun 22, 2020
65ca81e
Update src/services/refactors/convertToOptionalChainExpression.ts
jessetrinity Jun 22, 2020
d9c34ff
Update src/services/refactors/convertToOptionalChainExpression.ts
jessetrinity Jun 22, 2020
c2b9924
Update tests/cases/fourslash/refactorConvertToOptionalChainExpression…
jessetrinity Jun 22, 2020
1d51dab
Update tests/cases/fourslash/refactorConvertToOptionalChainExpression…
jessetrinity Jun 22, 2020
adbd586
reformat and remove unused checks
Jun 22, 2020
fb6b831
allow any for ternary refactor
Jun 23, 2020
8184ecf
add tests
Jun 23, 2020
5ac29a0
add tests
Jun 23, 2020
a0708be
check return and variable statements
Jun 23, 2020
6810cea
use isMatchingReference instead of containsMatchingReference
Jun 23, 2020
5634a4c
allow partial selections
Jun 23, 2020
e6e54cb
fine tune selection ranges
Jun 24, 2020
77f47e7
recurse for call expressions
Jun 24, 2020
1ba5fd0
fix spellings
Jun 24, 2020
6095e95
add recursive cases
Jun 25, 2020
89c8c9d
remove isOrContainsMatchingReference
Jun 26, 2020
b20cd95
cleanup
Jun 26, 2020
15176b2
more refactoring
Jun 29, 2020
2de5e28
cleanup
Jun 29, 2020
4126a46
rename tests
Jun 29, 2020
8f65c02
address PR comments
Jul 1, 2020
d4f6f52
Merge remote-tracking branch 'upstream/master' into refactorOptionalC…
Jul 1, 2020
8aed315
check match syntactically
Jul 2, 2020
602075f
handle another call expression case
Jul 7, 2020
a4cc060
some renames
Jul 7, 2020
e89ae08
inline some checks
Jul 7, 2020
2cdb5e1
add test
Jul 7, 2020
fd64b14
address comments
Jul 13, 2020
01854bb
add refactorNotAvailableReason
Jul 13, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -5827,6 +5827,10 @@
"category": "Message",
"code": 95138
},
"Convert to optional chain expression": {
"category": "Message",
"code": 95139
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2463,7 +2463,7 @@ namespace ts {
}
}

function getSingleVariableOfVariableStatement(node: Node): VariableDeclaration | undefined {
export function getSingleVariableOfVariableStatement(node: Node): VariableDeclaration | undefined {
return isVariableStatement(node) ? firstOrUndefined(node.declarationList.declarations) : undefined;
}

Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3252,9 +3252,9 @@ namespace FourSlash {
}
}

public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker }: FourSlashInterface.ApplyRefactorOptions) {
public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker, triggerReason }: FourSlashInterface.ApplyRefactorOptions) {
const range = this.getSelection();
const refactors = this.getApplicableRefactorsAtSelection();
const refactors = this.getApplicableRefactorsAtSelection(triggerReason);
const refactorsWithName = refactors.filter(r => r.name === refactorName);
if (refactorsWithName.length === 0) {
this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.\nAvailable refactors: ${refactors.map(r => r.name)}`);
Expand Down
1 change: 1 addition & 0 deletions src/harness/fourslashInterfaceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1483,6 +1483,7 @@ namespace FourSlashInterface {
actionName: string;
actionDescription: string;
newContent: NewFileContent;
triggerReason?: ts.RefactorTriggerReason;
}

export type ExpectedCompletionEntry = string | ExpectedCompletionEntryObject;
Expand Down
251 changes: 251 additions & 0 deletions src/services/refactors/convertToOptionalChainExpression.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
/* @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;
return [{
name: refactorName,
description: convertToOptionalChainExpressionMessage,
actions: [{
name: refactorName,
description: convertToOptionalChainExpressionMessage
}]
}];
}

function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined {
const info = getInfo(context);
if (!info) return undefined;
const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program.getTypeChecker(), t, info, actionName));
return { edits, renameFilename: undefined, renameLocation: undefined };
}

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): Info | 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 undefined;

const checker = program.getTypeChecker();
return isConditionalExpression(expression) ? getConditionalInfo(expression, checker) : getBinaryInfo(expression);
}

function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined {
const condition = expression.condition;
const finalExpression = getFinalExpressionInChain(expression.whenTrue);

if (!finalExpression || checker.isNullableType(checker.getTypeAtLocation(finalExpression))) return undefined;

if ((isPropertyAccessExpression(condition) || isIdentifier(condition))
&& getMatchingStart(condition, finalExpression.expression)) {
return { finalExpression, occurrences:[condition], expression };
}
else if (isBinaryExpression(condition)) {
const occurrences = getOccurrencesInExpression(finalExpression.expression, condition);
return occurrences ? { finalExpression, occurrences, expression } : undefined;
}
}

function getBinaryInfo(expression: BinaryExpression): Info | undefined {
if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined;
const finalExpression = getFinalExpressionInChain(expression.right);

if (!finalExpression) return undefined;

const occurrences = getOccurrencesInExpression(finalExpression.expression, expression.left);
return occurrences ? { finalExpression, occurrences, expression } : undefined;
}

/**
* 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(matchTo, 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 {
if (!isIdentifier(subchain) && !isPropertyAccessExpression(subchain)) return undefined;
return chainStartsWith(chain, subchain) ? subchain : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work as well? Seems easier to read than double !

Suggested change
return chainStartsWith(chain, subchain) ? subchain : 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.
if (isBinaryExpression(node)) {
Copy link
Member

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 here

Copy link
Contributor Author

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?

Copy link
Member

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 put skipParentheses in the right places, you can trivially handle foo && (((foo).bar) === 1) 😁

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: lastPropertyAccessChain, occurrences, expression } = info;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just change the name to finalExpression here too?

const firstOccurrence = occurrences[occurrences.length - 1];
const convertedChain = convertOccurrences(checker, lastPropertyAccessChain, occurrences);
if (convertedChain && (isPropertyAccessExpression(convertedChain) || isCallExpression(convertedChain))) {
if (isBinaryExpression(expression)) {
changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertedChain);
}
else if (isConditionalExpression(expression)) {
changes.replaceNode(sourceFile, expression,
factory.createBinaryExpression(convertedChain, factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse)
);
}
}
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
"codefixes/fixExpectedComma.ts",
"refactors/convertExport.ts",
"refactors/convertImport.ts",
"refactors/convertToOptionalChainExpression.ts",
"refactors/convertOverloadListToSingleSignature.ts",
"refactors/extractSymbol.ts",
"refactors/extractType.ts",
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ declare namespace FourSlashInterface {
enableFormatting(): void;
disableFormatting(): void;

applyRefactor(options: { refactorName: string, actionName: string, actionDescription: string, newContent: NewFileContent }): void;
applyRefactor(options: { refactorName: string, actionName: string, actionDescription: string, newContent: NewFileContent, triggerReason?: RefactorTriggerReason }): void;
}
class debug {
printCurrentParameterHelp(): void;
Expand Down
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;`
});
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;`
});
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?.();`
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

////let a = { b: { 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: { c: 0 } };
a?.b?.c;`
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

////let foo = { bar: { baz: 0 } };
////f/*a*/oo && foo.bar && foo.bar.ba/*b*/z;

// allow partial spans
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 foo = { bar: { baz: 0 } };
foo?.bar?.baz;`
});
Loading