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 19 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
5 changes: 5 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,11 @@ namespace ts {

getLocalTypeParametersOfClassOrInterfaceOrTypeAlias,
isDeclarationVisible,
containsMatchingReference: (nodeIn, targetIn) => {
const node = getParseTreeNode(nodeIn);
const target = getParseTreeNode(targetIn);
return !!node && !!target && containsMatchingReference(node, target);
},
};

function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined {
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -5799,6 +5799,10 @@
"category": "Message",
"code": 95137
},
"Convert to optional chain expression": {
"category": "Message",
"code": 95138
},

"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/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4120,6 +4120,7 @@ namespace ts {

/* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined;
/* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean;
/* @internal */ containsMatchingReference(node: Node, target: Node): boolean;
}

/* @internal */
Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3254,9 +3254,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
126 changes: 126 additions & 0 deletions src/services/refactors/convertToOptionalChainExpression.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/* @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 {
fullPropertyAccess: PropertyAccessExpression,
firstOccurrence: Node,
expression: BinaryExpression | ConditionalExpression
}

function getInfo(context: RefactorContext, considerEmptySpans = true): Info | undefined {
const { file, program } = context;
const span = getRefactorContextSpan(context);

if (span.length === 0 && !considerEmptySpans) return undefined;
const forEmptySpan = span.length === 0 && considerEmptySpans;
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved

const startToken = getTokenAtPosition(file, span.start);

const containingNode = forEmptySpan ? findAncestor(startToken, (node) => { return isExpressionStatement(node) && (isBinaryExpression(node.expression) || isConditionalExpression(node.expression)); }) : getParentNodeInSpan(startToken, file, span);
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
const expression = containingNode && isExpressionStatement(containingNode) ? containingNode.expression : containingNode;
if (!expression) return undefined;

const checker = program.getTypeChecker();


if (isBinaryExpression(expression)) {
sandersn marked this conversation as resolved.
Show resolved Hide resolved
const start = forEmptySpan ? expression.pos : startToken.pos;

const fullPropertyAccess = getFullPropertyAccessChain(expression);
if (!fullPropertyAccess) return undefined;
if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= fullPropertyAccess.pos) return undefined;

// ensure that each sequential operand in range matches the longest acceess chain
let checkNode = expression.left;
let firstOccurrence: PropertyAccessExpression | Identifier = fullPropertyAccess;
while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= start) {
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) {
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
return undefined;
}
firstOccurrence = checkNode.right;
checkNode = checkNode.left;
}
// check final identifier
if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= start) {
firstOccurrence = checkNode;
}
return firstOccurrence ? { fullPropertyAccess, firstOccurrence, expression } : undefined;
}

if (isConditionalExpression(expression)) {
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
const whenTrue = expression.whenTrue;
const condition = expression.condition;

if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) {
return { fullPropertyAccess: whenTrue, firstOccurrence: condition, expression };
}
}
return undefined;
}

function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined {
if (isCallExpression(node) && isPropertyAccessExpression(node.expression)) {
// a && |a.b|();
return node.expression;
}
else if (isBinaryExpression(node)) {
if (isPropertyAccessExpression(node.left)) {
// a && |a.b| == 1;
return node.left;
}
else if (isCallExpression(node.left) && isPropertyAccessExpression(node.left.expression)) {
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
// a && |a.b|() == 1;
return node.left.expression;
}
}
return undefined;
}

function getFullPropertyAccessChain(node: BinaryExpression): PropertyAccessExpression | undefined {
return isBinaryExpression(node.right) || isCallExpression(node.right)
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
? getRightHandSidePropertyAccess(node.right) : node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && isPropertyAccessExpression(node.right) && !isOptionalChain(node.right)
? node.right : undefined;
}

function convertPropertyAccessToOptionalChain(checker: TypeChecker, toConvert: PropertyAccessExpression, until: Identifier | PrivateIdentifier): PropertyAccessExpression {
if (isPropertyAccessExpression(toConvert.expression) && checker.getSymbolAtLocation(toConvert.expression.name) !== checker.getSymbolAtLocation(until)) {
return factory.createPropertyAccessChain(convertPropertyAccessToOptionalChain(checker, toConvert.expression, until), factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name);
}
return factory.createPropertyAccessChain(toConvert.expression, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name);
}

function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void {
const { fullPropertyAccess, firstOccurrence, expression } = info;
const until = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.name : isIdentifier(firstOccurrence) ? firstOccurrence : fullPropertyAccess.name;
if (isBinaryExpression(expression)) {
changes.replaceNodeRange(sourceFile, firstOccurrence, fullPropertyAccess, convertPropertyAccessToOptionalChain(checker, fullPropertyAccess, until));
}
else if (isConditionalExpression(expression)) {
changes.replaceNode(sourceFile, expression, factory.createBinaryExpression(convertPropertyAccessToOptionalChain(checker, fullPropertyAccess, until), 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 @@ -106,6 +106,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
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts
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,7 @@
/// <reference path='fourslash.ts' />

////let a = { b: { c: 0 } };
/////*a*/a && a.b && a?.b?.c;/*b*/

goTo.select("a", "b");
verify.not.refactorAvailable("Convert to optional chain expression");
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

////let a = { b: 0 };
/////*a*/a ? a.b : "whenFalse";/*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: 0 };
a?.b ?? "whenFalse";`
});
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.b ? a.b.c : "whenFalse";/*b*/
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved

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

////let a = { b: { c: () => { } } };
/////*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: () => { } } };
a?.b?.c();`
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts
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 === 1;/*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 === 1;`
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

////let a = { b: { c: () => { } } };
/////*a*/a && a.b && a.b.c() === 1;/*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: () => { } } };
a?.b?.c() === 1;`
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

////let a = { b: { c: 0 } };
////if(/*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 } };
if(a?.b?.c){};`
});
24 changes: 24 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path='fourslash.ts' />

////interface Foo {
//// bar?:{
//// baz?: string;
//// }
////}
////declare let foo: Foo | undefined;
/////*a*/foo && foo.bar && foo.bar.baz;/*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:
`interface Foo {
bar?:{
baz?: string;
}
}
declare let foo: Foo | undefined;
foo?.bar?.baz;`
});
16 changes: 16 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// <reference path='fourslash.ts' />

////let a = { b: 0 };
////let x = { y: 0 };
/////*a*/a && a.b()/*b*/ && x && x.y();

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: 0 };
let x = { y: 0 };
a?.b() && x && x.y();`
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts
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.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;`
});
Loading