From 77a49c35c09afe53509a287428e5e340032c792c Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Fri, 12 Jun 2020 15:23:17 -0700 Subject: [PATCH 01/46] add convertOptionalChain --- .../convertOptionalChainExpression.ts | 61 +++++++++++++++++++ src/services/tsconfig.json | 1 + 2 files changed, 62 insertions(+) create mode 100644 src/services/refactors/convertOptionalChainExpression.ts diff --git a/src/services/refactors/convertOptionalChainExpression.ts b/src/services/refactors/convertOptionalChainExpression.ts new file mode 100644 index 0000000000000..6b6a627efaed8 --- /dev/null +++ b/src/services/refactors/convertOptionalChainExpression.ts @@ -0,0 +1,61 @@ +/* @internal */ +namespace ts.refactor.convertOptionalChainExpression { + const refactorName = "Convert optional chain or && expression"; + const convertToOptionalChainExpression = "Convert to optional chain expression"; + // @ts-ignore + const convertToLogicalAndExpression = "Convert to logical && expression" + registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); + + function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { + const { file, startPosition, program } = context; + const info = getInfo(file, startPosition, program); + if (!info) return emptyArray; + return [{ + name: refactorName, + description: convertToOptionalChainExpression, + actions: [{ + name: refactorName, + description: convertToOptionalChainExpression + }] + }]; + } + + type Info = BinaryExpression; + + // @ts-ignore + function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { + const { file, startPosition, program } = context; + const info = getInfo(file, startPosition, program); + if (!info) return undefined; + } + + function isParentBinaryExpression(node: Node): boolean { + return isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken; + } + + function getFullPropertyAccessChain(node: BinaryExpression, checker: TypeChecker): Node | undefined { + if (isIdentifier(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { + return node.right + } else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { + const previousOperand = getFullPropertyAccessChain(node.left, checker); + const previousSymbol = previousOperand ? checker.getSymbolAtLocation(previousOperand) : undefined; + const currentSymbol = isCallExpression(node.right) && isPropertyAccessExpression(node.right.expression) ? + checker.getSymbolAtLocation(node.right.expression.expression) : checker.getSymbolAtLocation(node.right.expression); + if (previousSymbol && previousSymbol === currentSymbol) { + return node.right; + } + } + return undefined; + } + + // @ts-ignore + function getInfo(file: SourceFile, startPosition: number, program: Program): Info | undefined { + const node = getTokenAtPosition(file, startPosition); + const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + const checker = program.getTypeChecker(); + const expression = getFullPropertyAccessChain(parent as BinaryExpression, checker); + if (!expression) { + return undefined; + } + } +} \ No newline at end of file diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 1fd5ba3058165..62271c1f3de92 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -105,6 +105,7 @@ "codefixes/fixExpectedComma.ts", "refactors/convertExport.ts", "refactors/convertImport.ts", + "refactors/convertOptionalChainExpression.ts", "refactors/convertOverloadListToSingleSignature.ts", "refactors/extractSymbol.ts", "refactors/extractType.ts", From 9cf07cffcf1bd17be05507b114dc536141cc0cf3 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Jun 2020 14:45:11 -0700 Subject: [PATCH 02/46] cover more cases --- .../convertOptionalChainExpression.ts | 61 ------------- .../convertToOptionalChainExpression.ts | 90 +++++++++++++++++++ src/services/tsconfig.json | 2 +- 3 files changed, 91 insertions(+), 62 deletions(-) delete mode 100644 src/services/refactors/convertOptionalChainExpression.ts create mode 100644 src/services/refactors/convertToOptionalChainExpression.ts diff --git a/src/services/refactors/convertOptionalChainExpression.ts b/src/services/refactors/convertOptionalChainExpression.ts deleted file mode 100644 index 6b6a627efaed8..0000000000000 --- a/src/services/refactors/convertOptionalChainExpression.ts +++ /dev/null @@ -1,61 +0,0 @@ -/* @internal */ -namespace ts.refactor.convertOptionalChainExpression { - const refactorName = "Convert optional chain or && expression"; - const convertToOptionalChainExpression = "Convert to optional chain expression"; - // @ts-ignore - const convertToLogicalAndExpression = "Convert to logical && expression" - registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); - - function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - const { file, startPosition, program } = context; - const info = getInfo(file, startPosition, program); - if (!info) return emptyArray; - return [{ - name: refactorName, - description: convertToOptionalChainExpression, - actions: [{ - name: refactorName, - description: convertToOptionalChainExpression - }] - }]; - } - - type Info = BinaryExpression; - - // @ts-ignore - function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { - const { file, startPosition, program } = context; - const info = getInfo(file, startPosition, program); - if (!info) return undefined; - } - - function isParentBinaryExpression(node: Node): boolean { - return isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken; - } - - function getFullPropertyAccessChain(node: BinaryExpression, checker: TypeChecker): Node | undefined { - if (isIdentifier(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { - return node.right - } else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { - const previousOperand = getFullPropertyAccessChain(node.left, checker); - const previousSymbol = previousOperand ? checker.getSymbolAtLocation(previousOperand) : undefined; - const currentSymbol = isCallExpression(node.right) && isPropertyAccessExpression(node.right.expression) ? - checker.getSymbolAtLocation(node.right.expression.expression) : checker.getSymbolAtLocation(node.right.expression); - if (previousSymbol && previousSymbol === currentSymbol) { - return node.right; - } - } - return undefined; - } - - // @ts-ignore - function getInfo(file: SourceFile, startPosition: number, program: Program): Info | undefined { - const node = getTokenAtPosition(file, startPosition); - const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); - const checker = program.getTypeChecker(); - const expression = getFullPropertyAccessChain(parent as BinaryExpression, checker); - if (!expression) { - return undefined; - } - } -} \ No newline at end of file diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts new file mode 100644 index 0000000000000..56e94475fc836 --- /dev/null +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -0,0 +1,90 @@ +/* @internal */ +namespace ts.refactor.convertToOptionalChainExpression { + const refactorName = "Convert to optional chain expression"; + const convertToOptionalChainExpression = "Convert to optional chain expression"; + + registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); + + function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { + const { file, startPosition, program } = context; + const convertableExpression = getExpressionToConvert(file, startPosition, program); + if (!convertableExpression) return emptyArray; + return [{ + name: refactorName, + description: convertToOptionalChainExpression, + actions: [{ + name: refactorName, + description: convertToOptionalChainExpression + }] + }]; + } + + type ConvertableExpression = BinaryExpression | PropertyAccessExpression | CallExpression | undefined; + + // @ts-ignore + function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { + const { file, startPosition, program } = context; + const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, t, getExpressionToConvert(file, startPosition, program))); + return { edits, renameFilename: undefined, renameLocation: undefined } + } + + function isParentBinaryExpression(node: Node): boolean { + // stop at the LHS binary expression in cases like a && a.b && a.b.c == 1 + return isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken; + } + + function getPropertyAccessChain(node: BinaryExpression, checker: TypeChecker): PropertyAccessExpression | CallExpression | undefined { + if (isIdentifier(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { + return node.right; + } else if (isBinaryExpression(node.right) && isPropertyAccessExpression(node.right.left)) { + return node.right.left; + } else if (isBinaryExpression(node.right) && isCallExpression(node.right.left)) { + return node.right.left.expression; + } else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { + const previousOperand = getPropertyAccessChain(node.left, checker); + const previousSymbol = previousOperand ? checker.getSymbolAtLocation(previousOperand) : undefined; + const currentSymbol = isCallExpression(node.right) && isPropertyAccessExpression(node.right.expression) ? + checker.getSymbolAtLocation(node.right.expression.expression) : checker.getSymbolAtLocation(node.right.expression); + if (previousSymbol && previousSymbol === currentSymbol) { + return node.right; + } + } + return undefined; + } + + function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, toConvert: ConvertableExpression): void { + if (toConvert?.kind === SyntaxKind.BinaryExpression) { + if (isBinaryExpression(toConvert.right)) { + const rightHandSideExpression = toConvert.right.left; + // in the case of (a && a.b && a.b.c == 1), a.b.c is in the RHS binary expression. + if (isPropertyAccessExpression(rightHandSideExpression)) { + changes.replaceNode(sourceFile, toConvert, createBinary(convertPropertyAccessToOptionalChain(rightHandSideExpression), toConvert.right.operatorToken, toConvert.right.right)); + } else if (isCallExpression(rightHandSideExpression) && isPropertyAccessExpression(rightHandSideExpression.expression)) { + changes.replaceNode(sourceFile, toConvert, createBinary(createCall(convertPropertyAccessToOptionalChain(rightHandSideExpression.expression), rightHandSideExpression.typeArguments, rightHandSideExpression.arguments), toConvert.right.operatorToken, toConvert.right.right)); + } + } else if (isPropertyAccessExpression(toConvert.right)) { + changes.replaceNode(sourceFile, toConvert, convertPropertyAccessToOptionalChain(toConvert.right)); + } else if (isCallExpression(toConvert.right) && isPropertyAccessExpression(toConvert.right.expression)) { + changes.replaceNode(sourceFile, toConvert, createCall(convertPropertyAccessToOptionalChain(toConvert.right.expression), toConvert.right.typeArguments, toConvert.right.arguments)); + } + } + } + + function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { + if (toConvert.expression.kind === SyntaxKind.PropertyAccessExpression) { + return createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), createToken(SyntaxKind.QuestionDotToken), toConvert.name); + } + return createPropertyAccessChain(toConvert.expression, createToken(SyntaxKind.QuestionDotToken), toConvert.name); + } + + // returns the final property access chain in a binary expression e.g. a && a.b && a.b.c -> a.b.c + function getExpressionToConvert(file: SourceFile, startPosition: number, program: Program): BinaryExpression | undefined { + const node = getTokenAtPosition(file, startPosition); + const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + + if (!parent || parent.kind !== SyntaxKind.BinaryExpression) return undefined; + + const checker = program.getTypeChecker(); + return getPropertyAccessChain(parent, checker) ? parent : undefined; + } +} \ No newline at end of file diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 62271c1f3de92..99e3c80c83071 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -105,7 +105,7 @@ "codefixes/fixExpectedComma.ts", "refactors/convertExport.ts", "refactors/convertImport.ts", - "refactors/convertOptionalChainExpression.ts", + "refactors/convertToOptionalChainExpression.ts", "refactors/convertOverloadListToSingleSignature.ts", "refactors/extractSymbol.ts", "refactors/extractType.ts", From 80bf5d196dd60a61257ff79ea000051b6394ee3e Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Jun 2020 20:20:53 -0700 Subject: [PATCH 03/46] expose containsMatchingReference --- src/compiler/checker.ts | 5 +++++ src/compiler/types.ts | 1 + 2 files changed, 6 insertions(+) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 93b2834178eec..2dec7f13a805c 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -674,6 +674,11 @@ namespace ts { getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, isDeclarationVisible, + containsMatchingReference: (node, target) =>{ + node = getParseTreeNode(node); + target = getParseTreeNode(target); + return !!node && !!target && containsMatchingReference(node, target); + }, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 5c4e6f0c1592b..b0e8ff3e2738b 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4110,6 +4110,7 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; + /* @internal */ containsMatchingReference(node: Node, target: Node): boolean; } /* @internal */ From 4e642364cd5d54c88889b825d2e4db10a0131be6 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Jun 2020 20:21:23 -0700 Subject: [PATCH 04/46] clean up performing edits --- .../convertToOptionalChainExpression.ts | 100 ++++++++++++------ 1 file changed, 65 insertions(+), 35 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 56e94475fc836..077e2a2755d2b 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -2,7 +2,7 @@ namespace ts.refactor.convertToOptionalChainExpression { const refactorName = "Convert to optional chain expression"; const convertToOptionalChainExpression = "Convert to optional chain expression"; - + registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { @@ -19,28 +19,43 @@ namespace ts.refactor.convertToOptionalChainExpression { }]; } - type ConvertableExpression = BinaryExpression | PropertyAccessExpression | CallExpression | undefined; - - // @ts-ignore function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { const { file, startPosition, program } = context; - const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, t, getExpressionToConvert(file, startPosition, program))); - return { edits, renameFilename: undefined, renameLocation: undefined } + const checker = program.getTypeChecker(); + const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, checker, t, getExpressionToConvert(file, startPosition, program), actionName)); + return { edits, renameFilename: undefined, renameLocation: undefined }; } + type ConvertableExpression = PropertyAccessExpression | CallExpression | undefined; + + // returns the final property access chain in a binary expression e.g. a && a.b && a.b.c -> a.b.c + function getExpressionToConvert(file: SourceFile, startPosition: number, program: Program): ConvertableExpression { + const node = getTokenAtPosition(file, startPosition); + const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + + if (!parent || parent.kind !== SyntaxKind.BinaryExpression) return undefined; + + const checker = program.getTypeChecker(); + return getPropertyAccessChain(parent, checker); + } + + // finds the parent binary expression with && operatorToken function isParentBinaryExpression(node: Node): boolean { - // stop at the LHS binary expression in cases like a && a.b && a.b.c == 1 return isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken; } + // Checks the symbol in each operand of a && a.b && a.b.c appears in the next. function getPropertyAccessChain(node: BinaryExpression, checker: TypeChecker): PropertyAccessExpression | CallExpression | undefined { if (isIdentifier(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { return node.right; - } else if (isBinaryExpression(node.right) && isPropertyAccessExpression(node.right.left)) { + } + else if (isBinaryExpression(node.right) && isPropertyAccessExpression(node.right.left)) { return node.right.left; - } else if (isBinaryExpression(node.right) && isCallExpression(node.right.left)) { + } + else if (isBinaryExpression(node.right) && isCallExpression(node.right.left)) { return node.right.left.expression; - } else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { + } + else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { const previousOperand = getPropertyAccessChain(node.left, checker); const previousSymbol = previousOperand ? checker.getSymbolAtLocation(previousOperand) : undefined; const currentSymbol = isCallExpression(node.right) && isPropertyAccessExpression(node.right.expression) ? @@ -52,21 +67,47 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } - function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, toConvert: ConvertableExpression): void { - if (toConvert?.kind === SyntaxKind.BinaryExpression) { - if (isBinaryExpression(toConvert.right)) { - const rightHandSideExpression = toConvert.right.left; - // in the case of (a && a.b && a.b.c == 1), a.b.c is in the RHS binary expression. - if (isPropertyAccessExpression(rightHandSideExpression)) { - changes.replaceNode(sourceFile, toConvert, createBinary(convertPropertyAccessToOptionalChain(rightHandSideExpression), toConvert.right.operatorToken, toConvert.right.right)); - } else if (isCallExpression(rightHandSideExpression) && isPropertyAccessExpression(rightHandSideExpression.expression)) { - changes.replaceNode(sourceFile, toConvert, createBinary(createCall(convertPropertyAccessToOptionalChain(rightHandSideExpression.expression), rightHandSideExpression.typeArguments, rightHandSideExpression.arguments), toConvert.right.operatorToken, toConvert.right.right)); - } - } else if (isPropertyAccessExpression(toConvert.right)) { - changes.replaceNode(sourceFile, toConvert, convertPropertyAccessToOptionalChain(toConvert.right)); - } else if (isCallExpression(toConvert.right) && isPropertyAccessExpression(toConvert.right.expression)) { - changes.replaceNode(sourceFile, toConvert, createCall(convertPropertyAccessToOptionalChain(toConvert.right.expression), toConvert.right.typeArguments, toConvert.right.arguments)); + function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: ConvertableExpression, _actionName: string): void { + if (!toConvert) return; + if (isCallExpression(toConvert)) { + doCallExpression(sourceFile, checker, changes, toConvert); + } + else if (isPropertyAccessExpression(toConvert)) { + doPropertyAccessExpression(sourceFile, checker, changes, toConvert); + } + } + + function doPropertyAccessExpression(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: PropertyAccessExpression): void { + let checkNode = findAncestor(toConvert, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + let startNode: Node = toConvert; + while (isBinaryExpression(checkNode.left)) { + if (isPropertyAccessExpression(checkNode.right) && checker.containsMatchingReference(toConvert, checkNode.right)) { + startNode = checkNode.right; } + checkNode = checkNode.left; + } + if (isIdentifier(checkNode.left) && checker.containsMatchingReference(toConvert, checkNode.right)) { + startNode = checkNode.left; + } + changes.replaceNodeRange(sourceFile, startNode, toConvert, convertPropertyAccessToOptionalChain(toConvert)); + } + + function doCallExpression(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: CallExpression) { + if (!toConvert || !isBinaryExpression(toConvert.parent)) return; + + let checkNode = findAncestor(toConvert, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + let startNode: Node = toConvert; + while (isBinaryExpression(checkNode.left)) { + if (isPropertyAccessExpression(checkNode.right) && checker.containsMatchingReference(toConvert, checkNode.right)) { + startNode = checkNode.right; + } + checkNode = checkNode.left; + } + if (isIdentifier(checkNode.left) && checker.containsMatchingReference(toConvert.expression, checkNode.right)) { + startNode = checkNode.left; + } + if (isPropertyAccessExpression(toConvert.expression)) { + changes.replaceNodeRange(sourceFile, startNode, toConvert, createCall(convertPropertyAccessToOptionalChain(toConvert.expression), toConvert.typeArguments, toConvert.arguments)); } } @@ -76,15 +117,4 @@ namespace ts.refactor.convertToOptionalChainExpression { } return createPropertyAccessChain(toConvert.expression, createToken(SyntaxKind.QuestionDotToken), toConvert.name); } - - // returns the final property access chain in a binary expression e.g. a && a.b && a.b.c -> a.b.c - function getExpressionToConvert(file: SourceFile, startPosition: number, program: Program): BinaryExpression | undefined { - const node = getTokenAtPosition(file, startPosition); - const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); - - if (!parent || parent.kind !== SyntaxKind.BinaryExpression) return undefined; - - const checker = program.getTypeChecker(); - return getPropertyAccessChain(parent, checker) ? parent : undefined; - } } \ No newline at end of file From 9a59e1343dcea374971908e0ef98d34ccc302ef4 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Jun 2020 20:39:03 -0700 Subject: [PATCH 05/46] bound start position --- src/services/refactors/convertToOptionalChainExpression.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 077e2a2755d2b..e70af66fb4a75 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -36,7 +36,7 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!parent || parent.kind !== SyntaxKind.BinaryExpression) return undefined; const checker = program.getTypeChecker(); - return getPropertyAccessChain(parent, checker); + return parent.getStart() <= startPosition ? getPropertyAccessChain(parent, checker) : undefined; } // finds the parent binary expression with && operatorToken From 6c1bccf29dee9a649f8449cdf99a543edfcd8a4e Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 16 Jun 2020 21:10:44 -0700 Subject: [PATCH 06/46] add tests --- .../convertToOptionalChainExpression.ts | 2 +- ...factorConvertToOptionalChainExpression1.ts | 14 +++++++++++ ...factorConvertToOptionalChainExpression2.ts | 14 +++++++++++ ...factorConvertToOptionalChainExpression3.ts | 14 +++++++++++ ...factorConvertToOptionalChainExpression4.ts | 14 +++++++++++ ...factorConvertToOptionalChainExpression5.ts | 14 +++++++++++ ...factorConvertToOptionalChainExpression6.ts | 24 +++++++++++++++++++ 7 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index e70af66fb4a75..02355c9a5a55c 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -1,7 +1,7 @@ /* @internal */ namespace ts.refactor.convertToOptionalChainExpression { const refactorName = "Convert to optional chain expression"; - const convertToOptionalChainExpression = "Convert to optional chain expression"; + const convertToOptionalChainExpression = "Convert && chain to optional chain expression"; registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts new file mode 100644 index 0000000000000..4308adee80ccd --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +a?.b?.c;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts new file mode 100644 index 0000000000000..ecc4a7b550c14 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: () => { } } }; +a?.b?.c();` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts new file mode 100644 index 0000000000000..beb40c26d52fe --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +a?.b?.c === 1;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts new file mode 100644 index 0000000000000..a9c9e6cb3f05f --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: () => { } } }; +a?.b?.c() === 1;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts new file mode 100644 index 0000000000000..bcfddd8cc19cd --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +if(a?.b?.c){};` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts new file mode 100644 index 0000000000000..452bb72183ada --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts @@ -0,0 +1,24 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`interface Foo { + bar?:{ + baz?: string; + } +} +declare let foo: Foo | undefined; +foo?.bar?.baz;` +}); \ No newline at end of file From 2a355e4b29f693195463e15d729b7b2c50ee9f12 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 15:24:37 -0700 Subject: [PATCH 07/46] refactor and handle edge cases --- .../convertToOptionalChainExpression.ts | 130 ++++++++---------- ...factorConvertToOptionalChainExpression7.ts | 16 +++ ...factorConvertToOptionalChainExpression8.ts | 16 +++ 3 files changed, 89 insertions(+), 73 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 02355c9a5a55c..813031a076aaa 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -6,9 +6,9 @@ namespace ts.refactor.convertToOptionalChainExpression { registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - const { file, startPosition, program } = context; - const convertableExpression = getExpressionToConvert(file, startPosition, program); - if (!convertableExpression) return emptyArray; + const { file, startPosition, endPosition, program } = context; + const info = getExpressionToConvert(file, startPosition, endPosition, program); + if (!info) return emptyArray; return [{ name: refactorName, description: convertToOptionalChainExpression, @@ -20,95 +20,79 @@ namespace ts.refactor.convertToOptionalChainExpression { } function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { - const { file, startPosition, program } = context; - const checker = program.getTypeChecker(); - const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, checker, t, getExpressionToConvert(file, startPosition, program), actionName)); + const { file, startPosition, endPosition, program } = context; + const info = getExpressionToConvert(file, startPosition, endPosition, program); + if (!info) return undefined; + const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, t, info, actionName)); return { edits, renameFilename: undefined, renameLocation: undefined }; } - type ConvertableExpression = PropertyAccessExpression | CallExpression | undefined; + interface Info { + fullPropertyAccess: PropertyAccessExpression, + firstOccurence: Node, + containingExpression: BinaryExpression + } + + function getExpressionToConvert(file: SourceFile, startPosition: number, endPosition: number | undefined, program: Program): Info | undefined { + const startToken = getTokenAtPosition(file, startPosition); + const endToken = endPosition ? findTokenOnLeftOfPosition(file, endPosition) : startToken; + const startBinary = findAncestor(startToken, (node) => { return isBinaryExpression(node); }); + const endBinary = endToken !== startToken ? findAncestor(endToken, (node) => { return isBinaryExpression(node); }) : startBinary; - // returns the final property access chain in a binary expression e.g. a && a.b && a.b.c -> a.b.c - function getExpressionToConvert(file: SourceFile, startPosition: number, program: Program): ConvertableExpression { - const node = getTokenAtPosition(file, startPosition); - const parent = findAncestor(node, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); + if (!startBinary || !endBinary || !isBinaryExpression(startBinary) || !isBinaryExpression(endBinary)) return undefined; + const parent = getParentNodeInSpan(startBinary, file, createTextSpanFromBounds(startBinary.pos, endBinary.end)); - if (!parent || parent.kind !== SyntaxKind.BinaryExpression) return undefined; + if (!parent || !isBinaryExpression(parent)) return undefined; const checker = program.getTypeChecker(); - return parent.getStart() <= startPosition ? getPropertyAccessChain(parent, checker) : undefined; - } - // finds the parent binary expression with && operatorToken - function isParentBinaryExpression(node: Node): boolean { - return isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken; - } + const fullPropertyAccess = getFullPropertyAccessChain(parent, checker); + if (!fullPropertyAccess) return undefined; - // Checks the symbol in each operand of a && a.b && a.b.c appears in the next. - function getPropertyAccessChain(node: BinaryExpression, checker: TypeChecker): PropertyAccessExpression | CallExpression | undefined { - if (isIdentifier(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { - return node.right; - } - else if (isBinaryExpression(node.right) && isPropertyAccessExpression(node.right.left)) { - return node.right.left; - } - else if (isBinaryExpression(node.right) && isCallExpression(node.right.left)) { - return node.right.left.expression; - } - else if (isBinaryExpression(node.left) && (isPropertyAccessExpression(node.right) || isCallExpression(node.right))) { - const previousOperand = getPropertyAccessChain(node.left, checker); - const previousSymbol = previousOperand ? checker.getSymbolAtLocation(previousOperand) : undefined; - const currentSymbol = isCallExpression(node.right) && isPropertyAccessExpression(node.right.expression) ? - checker.getSymbolAtLocation(node.right.expression.expression) : checker.getSymbolAtLocation(node.right.expression); - if (previousSymbol && previousSymbol === currentSymbol) { - return node.right; + // ensure that each sequential operand in range matches the longest acceess chain + let checkNode = parent.left; + let firstOccurence: PropertyAccessExpression | Identifier = fullPropertyAccess; + while (isBinaryExpression(checkNode) && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= startBinary.right.pos) { + if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { + return undefined; } + firstOccurence = checkNode.right; + checkNode = checkNode.left; } - return undefined; - } - - function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: ConvertableExpression, _actionName: string): void { - if (!toConvert) return; - if (isCallExpression(toConvert)) { - doCallExpression(sourceFile, checker, changes, toConvert); - } - else if (isPropertyAccessExpression(toConvert)) { - doPropertyAccessExpression(sourceFile, checker, changes, toConvert); + // check final identifier + if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= startBinary.pos) { + firstOccurence = checkNode; } + return firstOccurence ? { fullPropertyAccess, firstOccurence, containingExpression:parent } : undefined; } - function doPropertyAccessExpression(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: PropertyAccessExpression): void { - let checkNode = findAncestor(toConvert, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); - let startNode: Node = toConvert; - while (isBinaryExpression(checkNode.left)) { - if (isPropertyAccessExpression(checkNode.right) && checker.containsMatchingReference(toConvert, checkNode.right)) { - startNode = checkNode.right; - } - checkNode = checkNode.left; + function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { + if (isCallExpression(node) && isPropertyAccessExpression(node.expression)) { + // a && |a.b|(); + return node.expression; } - if (isIdentifier(checkNode.left) && checker.containsMatchingReference(toConvert, checkNode.right)) { - startNode = checkNode.left; + else if (isBinaryExpression(node)) { + if (isPropertyAccessExpression(node.left)) { + // a && |a.b| == 1; + return node.left; + } + else if (isCallExpression(node.left) && isPropertyAccessExpression(node.left.expression)) { + // a && |a.b|() == 1; + return node.left.expression; + } } - changes.replaceNodeRange(sourceFile, startNode, toConvert, convertPropertyAccessToOptionalChain(toConvert)); + return undefined; } - function doCallExpression(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: CallExpression) { - if (!toConvert || !isBinaryExpression(toConvert.parent)) return; + function getFullPropertyAccessChain(node: BinaryExpression, _checker: TypeChecker): PropertyAccessExpression | undefined { + return isBinaryExpression(node.right) || isCallExpression(node.right) + ? getRightHandSidePropertyAccess(node.right) : isPropertyAccessExpression(node.right) + ? node.right : undefined; + } - let checkNode = findAncestor(toConvert, (node) => { return node.parent && isParentBinaryExpression(node) && !isParentBinaryExpression(node.parent); }); - let startNode: Node = toConvert; - while (isBinaryExpression(checkNode.left)) { - if (isPropertyAccessExpression(checkNode.right) && checker.containsMatchingReference(toConvert, checkNode.right)) { - startNode = checkNode.right; - } - checkNode = checkNode.left; - } - if (isIdentifier(checkNode.left) && checker.containsMatchingReference(toConvert.expression, checkNode.right)) { - startNode = checkNode.left; - } - if (isPropertyAccessExpression(toConvert.expression)) { - changes.replaceNodeRange(sourceFile, startNode, toConvert, createCall(convertPropertyAccessToOptionalChain(toConvert.expression), toConvert.typeArguments, toConvert.arguments)); - } + function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { + const { fullPropertyAccess, firstOccurence } = info; + changes.replaceNodeRange(sourceFile, firstOccurence, fullPropertyAccess, convertPropertyAccessToOptionalChain(fullPropertyAccess)); } function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts new file mode 100644 index 0000000000000..0c59737bb2c25 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts @@ -0,0 +1,16 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: 0 }; +let x = { y: 0 }; +a?.b && x && x.y;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts new file mode 100644 index 0000000000000..3d45c0c4104ac --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts @@ -0,0 +1,16 @@ +/// + +////let a = { b: 0 }; +////let x = { y: 0 }; +////a && a.b && /*a*/x && x.y/*b*/; + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert to optional chain expression", + actionName: "Convert to optional chain expression", + actionDescription: "Convert && chain to optional chain expression", + newContent: +`let a = { b: 0 }; +let x = { y: 0 }; +a && a.b && x?.y;` +}); \ No newline at end of file From 3dac9c6c2b4e8ccf31608cc53acd5a870a4326c9 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 18:57:59 -0700 Subject: [PATCH 08/46] update tests --- .../refactorConvertToOptionalChainExpression7.ts | 4 ++-- .../refactorConvertToOptionalChainExpression8.ts | 4 ++-- .../refactorConvertToOptionalChainExpression9.ts | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts index 0c59737bb2c25..ee4a0d303afd2 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts @@ -2,7 +2,7 @@ ////let a = { b: 0 }; ////let x = { y: 0 }; -/////*a*/a && a.b/*b*/ && x && x.y; +/////*a*/a && a.b()/*b*/ && x && x.y(); goTo.select("a", "b"); edit.applyRefactor({ @@ -12,5 +12,5 @@ edit.applyRefactor({ newContent: `let a = { b: 0 }; let x = { y: 0 }; -a?.b && x && x.y;` +a?.b() && x && x.y();` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts index 3d45c0c4104ac..b97f810295db9 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts @@ -2,7 +2,7 @@ ////let a = { b: 0 }; ////let x = { y: 0 }; -////a && a.b && /*a*/x && x.y/*b*/; +////a && a.b() && /*a*/x && x.y()/*b*/; goTo.select("a", "b"); edit.applyRefactor({ @@ -12,5 +12,5 @@ edit.applyRefactor({ newContent: `let a = { b: 0 }; let x = { y: 0 }; -a && a.b && x?.y;` +a && a.b() && x?.y();` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts new file mode 100644 index 0000000000000..710620be29129 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +a?.b?.c;` +}); \ No newline at end of file From b40303769e2327de6a48168058596de506bb9226 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 19:23:06 -0700 Subject: [PATCH 09/46] consider explicit requests for empty spans --- .../convertToOptionalChainExpression.ts | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 813031a076aaa..517008728467a 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -6,8 +6,8 @@ namespace ts.refactor.convertToOptionalChainExpression { registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - const { file, startPosition, endPosition, program } = context; - const info = getExpressionToConvert(file, startPosition, endPosition, program); + const { file, startPosition, endPosition, program, triggerReason } = context; + const info = getInfo(file, startPosition, endPosition, program, triggerReason === "invoked"); if (!info) return emptyArray; return [{ name: refactorName, @@ -21,7 +21,7 @@ namespace ts.refactor.convertToOptionalChainExpression { function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { const { file, startPosition, endPosition, program } = context; - const info = getExpressionToConvert(file, startPosition, endPosition, program); + const info = getInfo(file, startPosition, endPosition, program); if (!info) return undefined; const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, t, info, actionName)); return { edits, renameFilename: undefined, renameLocation: undefined }; @@ -33,17 +33,25 @@ namespace ts.refactor.convertToOptionalChainExpression { containingExpression: BinaryExpression } - function getExpressionToConvert(file: SourceFile, startPosition: number, endPosition: number | undefined, program: Program): Info | undefined { + function getInfo(file: SourceFile, startPosition: number, endPosition: number | undefined, program: Program, considerEmptySpans = true): Info | undefined { + if (startPosition === endPosition && !considerEmptySpans) return undefined; + const forEmptySpan = startPosition === endPosition && considerEmptySpans; const startToken = getTokenAtPosition(file, startPosition); const endToken = endPosition ? findTokenOnLeftOfPosition(file, endPosition) : startToken; - const startBinary = findAncestor(startToken, (node) => { return isBinaryExpression(node); }); - const endBinary = endToken !== startToken ? findAncestor(endToken, (node) => { return isBinaryExpression(node); }) : startBinary; - - if (!startBinary || !endBinary || !isBinaryExpression(startBinary) || !isBinaryExpression(endBinary)) return undefined; - const parent = getParentNodeInSpan(startBinary, file, createTextSpanFromBounds(startBinary.pos, endBinary.end)); + const startBinary = findAncestor(startToken, (node): node is BinaryExpression => { return isBinaryExpression(node); }); + const endBinary = endToken !== startToken ? findAncestor(endToken, (node): node is BinaryExpression => { return isBinaryExpression(node); }) : startBinary; + // a request for an empty span should simply check if the largest containing binary expression is valid + const parentForEmptySpan = forEmptySpan ? findAncestor(startBinary, (node) => { return node.parent && isBinaryExpression(node) && !isBinaryExpression(node.parent); }): undefined; + const parent = forEmptySpan + ? parentForEmptySpan + : startBinary && endBinary + ? getParentNodeInSpan(startBinary, file, createTextSpanFromBounds(startBinary.pos, endBinary.end)) : undefined; if (!parent || !isBinaryExpression(parent)) return undefined; + const firstBinaryForEmpty = forEmptySpan ? findAncestor(parent.getFirstToken(), (node): node is BinaryExpression => { return isBinaryExpression(node); }): undefined; + const startNode = forEmptySpan ? firstBinaryForEmpty : startBinary; + if (!startNode) return undefined; const checker = program.getTypeChecker(); const fullPropertyAccess = getFullPropertyAccessChain(parent, checker); @@ -52,7 +60,7 @@ namespace ts.refactor.convertToOptionalChainExpression { // ensure that each sequential operand in range matches the longest acceess chain let checkNode = parent.left; let firstOccurence: PropertyAccessExpression | Identifier = fullPropertyAccess; - while (isBinaryExpression(checkNode) && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= startBinary.right.pos) { + while (isBinaryExpression(checkNode) && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= startNode.right.pos) { if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { return undefined; } @@ -60,7 +68,7 @@ namespace ts.refactor.convertToOptionalChainExpression { checkNode = checkNode.left; } // check final identifier - if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= startBinary.pos) { + if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= startNode.pos) { firstOccurence = checkNode; } return firstOccurence ? { fullPropertyAccess, firstOccurence, containingExpression:parent } : undefined; @@ -90,15 +98,15 @@ namespace ts.refactor.convertToOptionalChainExpression { ? node.right : undefined; } - function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { - const { fullPropertyAccess, firstOccurence } = info; - changes.replaceNodeRange(sourceFile, firstOccurence, fullPropertyAccess, convertPropertyAccessToOptionalChain(fullPropertyAccess)); - } - function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { if (toConvert.expression.kind === SyntaxKind.PropertyAccessExpression) { return createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), createToken(SyntaxKind.QuestionDotToken), toConvert.name); } return createPropertyAccessChain(toConvert.expression, createToken(SyntaxKind.QuestionDotToken), toConvert.name); } + + function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { + const { fullPropertyAccess, firstOccurence } = info; + changes.replaceNodeRange(sourceFile, firstOccurence, fullPropertyAccess, convertPropertyAccessToOptionalChain(fullPropertyAccess)); + } } \ No newline at end of file From 1ae550040835f650fe6f61bb6c1a96cfbd97a428 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 19:23:59 -0700 Subject: [PATCH 10/46] update fourslash to use trigger reason --- src/harness/fourslashImpl.ts | 4 ++-- src/harness/fourslashInterfaceImpl.ts | 1 + tests/cases/fourslash/fourslash.ts | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 3d3347c3532ba..e09984aefdbdb 100644 --- a/src/harness/fourslashImpl.ts +++ b/src/harness/fourslashImpl.ts @@ -3253,9 +3253,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)}`); diff --git a/src/harness/fourslashInterfaceImpl.ts b/src/harness/fourslashInterfaceImpl.ts index 07eea0c906655..23060c0df1745 100644 --- a/src/harness/fourslashInterfaceImpl.ts +++ b/src/harness/fourslashInterfaceImpl.ts @@ -1483,6 +1483,7 @@ namespace FourSlashInterface { actionName: string; actionDescription: string; newContent: NewFileContent; + triggerReason?: ts.RefactorTriggerReason; } export type ExpectedCompletionEntry = string | ExpectedCompletionEntryObject; diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index ce3eefcb4ac84..1cb4d6f1150b5 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -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; From bf28673a28a89c68e954e23dea90f63e02bbb37b Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 19:25:21 -0700 Subject: [PATCH 11/46] add tests cases for trigger reason --- ...tToOptionalChainExpressionForTriggerReason1.ts | 9 +++++++++ ...tToOptionalChainExpressionForTriggerReason2.ts | 15 +++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts new file mode 100644 index 0000000000000..4cbaab28834d7 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts @@ -0,0 +1,9 @@ +/// + +////let a = { b: { c: 0 } }; +////a && a.b && /*a*//*b*/a.b.c; + +// Only offer refactor for empty span if explicity requested +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); +verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts new file mode 100644 index 0000000000000..9341f0d37892c --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts @@ -0,0 +1,15 @@ +/// + +////let a = { b: { c: 0 } }; +////a && a.b && /*a*//*b*/a.b.c; + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert to optional chain expression", + actionName: "Convert to optional chain expression", + actionDescription: "Convert && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +a?.b?.c;`, + triggerReason: "invoked" +}); \ No newline at end of file From fa3f9c8531130b884d4a0c1347aed8d12d6bc3c9 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 21:04:33 -0700 Subject: [PATCH 12/46] fix errors --- src/compiler/checker.ts | 6 +++--- src/services/refactors/convertToOptionalChainExpression.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 2dec7f13a805c..843b39aa15a7c 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -674,9 +674,9 @@ namespace ts { getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, isDeclarationVisible, - containsMatchingReference: (node, target) =>{ - node = getParseTreeNode(node); - target = getParseTreeNode(target); + containsMatchingReference: (nodeIn, targetIn) => { + const node = getParseTreeNode(nodeIn); + const target = getParseTreeNode(targetIn); return !!node && !!target && containsMatchingReference(node, target); }, }; diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 517008728467a..2b62c3b0b90f2 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -100,9 +100,9 @@ namespace ts.refactor.convertToOptionalChainExpression { function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { if (toConvert.expression.kind === SyntaxKind.PropertyAccessExpression) { - return createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), createToken(SyntaxKind.QuestionDotToken), toConvert.name); + return factory.createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); } - return createPropertyAccessChain(toConvert.expression, createToken(SyntaxKind.QuestionDotToken), toConvert.name); + return factory.createPropertyAccessChain(toConvert.expression, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); } function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { From 8794154d1c9060806a78fc56d3d2d06dcbc7ebfe Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 17 Jun 2020 21:08:53 -0700 Subject: [PATCH 13/46] remove type assertion --- src/services/refactors/convertToOptionalChainExpression.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 2b62c3b0b90f2..6daa530981781 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -99,10 +99,10 @@ namespace ts.refactor.convertToOptionalChainExpression { } function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { - if (toConvert.expression.kind === SyntaxKind.PropertyAccessExpression) { - return factory.createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + if (isPropertyAccessExpression(toConvert.expression)) { + return factory.createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); } - return factory.createPropertyAccessChain(toConvert.expression, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + return factory.createPropertyAccessChain(toConvert.expression, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); } function doChange(sourceFile: SourceFile, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { From 3d8fed18d7202f03df877df8c9fd6a0c7089c550 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 18 Jun 2020 11:01:55 -0700 Subject: [PATCH 14/46] fix non ampersand chains --- .../convertToOptionalChainExpression.ts | 4 +-- ...actorConvertToOptionalChainExpression10.ts | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 6daa530981781..e8d5e27c53abc 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -47,7 +47,7 @@ namespace ts.refactor.convertToOptionalChainExpression { ? parentForEmptySpan : startBinary && endBinary ? getParentNodeInSpan(startBinary, file, createTextSpanFromBounds(startBinary.pos, endBinary.end)) : undefined; - if (!parent || !isBinaryExpression(parent)) return undefined; + if (!parent || !isBinaryExpression(parent) || (parent !== endBinary && parent.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken)) return undefined; const firstBinaryForEmpty = forEmptySpan ? findAncestor(parent.getFirstToken(), (node): node is BinaryExpression => { return isBinaryExpression(node); }): undefined; const startNode = forEmptySpan ? firstBinaryForEmpty : startBinary; @@ -94,7 +94,7 @@ namespace ts.refactor.convertToOptionalChainExpression { function getFullPropertyAccessChain(node: BinaryExpression, _checker: TypeChecker): PropertyAccessExpression | undefined { return isBinaryExpression(node.right) || isCallExpression(node.right) - ? getRightHandSidePropertyAccess(node.right) : isPropertyAccessExpression(node.right) + ? getRightHandSidePropertyAccess(node.right) : node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && isPropertyAccessExpression(node.right) ? node.right : undefined; } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts new file mode 100644 index 0000000000000..8e355fa5a4374 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts @@ -0,0 +1,28 @@ +/// + +////let a = { b: { c: { d: 0 } } }; +/////*1a*/a || a.b && a.b.c && a.b.c.d/*1b*/; +/////*2a*/a && a.b || a.b.c && a.b.c.d/*2b*/; +/////*3a*/a && a.b && a.b.c || a.b.c.d/*3b*/; +/////*4a*/a ?? a.b && a.b.c && a.b.c.d/*4b*/; +/////*5a*/a && a.b ?? a.b.c || a.b.c.d/*5b*/; +/////*6a*/a && a.b && a.b.c ?? a.b.c.d/*6b*/; + +// Only offer refactor for && chains. +goTo.select("1a", "1b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("2a", "2b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("3a", "3b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("4a", "4b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("5a", "5b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("6a", "6b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); From b5c833c295886714675ca349e2e7f4541f9e73bc Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 18 Jun 2020 16:24:55 -0700 Subject: [PATCH 15/46] clean up some logic --- .../convertToOptionalChainExpression.ts | 58 +++++++++---------- ...factorConvertToOptionalChainExpression1.ts | 2 +- ...actorConvertToOptionalChainExpression10.ts | 29 ++-------- ...factorConvertToOptionalChainExpression2.ts | 2 +- ...factorConvertToOptionalChainExpression3.ts | 2 +- ...factorConvertToOptionalChainExpression4.ts | 2 +- ...factorConvertToOptionalChainExpression6.ts | 2 +- ...factorConvertToOptionalChainExpression8.ts | 10 ++-- ...factorConvertToOptionalChainExpression9.ts | 38 ++++++++---- 9 files changed, 66 insertions(+), 79 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index e8d5e27c53abc..ec42253f05c3b 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -6,8 +6,7 @@ namespace ts.refactor.convertToOptionalChainExpression { registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { - const { file, startPosition, endPosition, program, triggerReason } = context; - const info = getInfo(file, startPosition, endPosition, program, triggerReason === "invoked"); + const info = getInfo(context, context.triggerReason === "invoked"); if (!info) return emptyArray; return [{ name: refactorName, @@ -20,8 +19,7 @@ namespace ts.refactor.convertToOptionalChainExpression { } function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { - const { file, startPosition, endPosition, program } = context; - const info = getInfo(file, startPosition, endPosition, program); + const info = getInfo(context); if (!info) return undefined; const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, t, info, actionName)); return { edits, renameFilename: undefined, renameLocation: undefined }; @@ -33,34 +31,32 @@ namespace ts.refactor.convertToOptionalChainExpression { containingExpression: BinaryExpression } - function getInfo(file: SourceFile, startPosition: number, endPosition: number | undefined, program: Program, considerEmptySpans = true): Info | undefined { - if (startPosition === endPosition && !considerEmptySpans) return undefined; - const forEmptySpan = startPosition === endPosition && considerEmptySpans; - const startToken = getTokenAtPosition(file, startPosition); - const endToken = endPosition ? findTokenOnLeftOfPosition(file, endPosition) : startToken; - const startBinary = findAncestor(startToken, (node): node is BinaryExpression => { return isBinaryExpression(node); }); - const endBinary = endToken !== startToken ? findAncestor(endToken, (node): node is BinaryExpression => { return isBinaryExpression(node); }) : startBinary; - - // a request for an empty span should simply check if the largest containing binary expression is valid - const parentForEmptySpan = forEmptySpan ? findAncestor(startBinary, (node) => { return node.parent && isBinaryExpression(node) && !isBinaryExpression(node.parent); }): undefined; - const parent = forEmptySpan - ? parentForEmptySpan - : startBinary && endBinary - ? getParentNodeInSpan(startBinary, file, createTextSpanFromBounds(startBinary.pos, endBinary.end)) : undefined; - if (!parent || !isBinaryExpression(parent) || (parent !== endBinary && parent.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken)) return undefined; - - const firstBinaryForEmpty = forEmptySpan ? findAncestor(parent.getFirstToken(), (node): node is BinaryExpression => { return isBinaryExpression(node); }): undefined; - const startNode = forEmptySpan ? firstBinaryForEmpty : startBinary; - if (!startNode) return undefined; - const checker = program.getTypeChecker(); + 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; + + const startToken = getTokenAtPosition(file, span.start); + + const parent = forEmptySpan ? findAncestor(startToken, (node) => { return isExpressionStatement(node) && isBinaryExpression(node.expression); }) : getParentNodeInSpan(startToken, file, span); + if (!parent) return undefined; - const fullPropertyAccess = getFullPropertyAccessChain(parent, checker); + const binaryExpression = isExpressionStatement(parent) && isBinaryExpression(parent.expression) ? parent.expression : isBinaryExpression(parent) ? parent : undefined; + if (!binaryExpression) return undefined; + + const start = forEmptySpan ? binaryExpression.pos : startToken.pos; + + const checker = program.getTypeChecker(); + const fullPropertyAccess = getFullPropertyAccessChain(binaryExpression); if (!fullPropertyAccess) return undefined; + if (binaryExpression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && binaryExpression.operatorToken.pos <= fullPropertyAccess.pos) return undefined; // ensure that each sequential operand in range matches the longest acceess chain - let checkNode = parent.left; + let checkNode = binaryExpression.left; let firstOccurence: PropertyAccessExpression | Identifier = fullPropertyAccess; - while (isBinaryExpression(checkNode) && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= startNode.right.pos) { + while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= start) { if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { return undefined; } @@ -68,10 +64,10 @@ namespace ts.refactor.convertToOptionalChainExpression { checkNode = checkNode.left; } // check final identifier - if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= startNode.pos) { + if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= start) { firstOccurence = checkNode; } - return firstOccurence ? { fullPropertyAccess, firstOccurence, containingExpression:parent } : undefined; + return firstOccurence ? { fullPropertyAccess, firstOccurence, containingExpression:binaryExpression } : undefined; } function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { @@ -92,9 +88,9 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } - function getFullPropertyAccessChain(node: BinaryExpression, _checker: TypeChecker): PropertyAccessExpression | undefined { + function getFullPropertyAccessChain(node: BinaryExpression): PropertyAccessExpression | undefined { return isBinaryExpression(node.right) || isCallExpression(node.right) - ? getRightHandSidePropertyAccess(node.right) : node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && isPropertyAccessExpression(node.right) + ? getRightHandSidePropertyAccess(node.right) : node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && isPropertyAccessExpression(node.right) && !isOptionalChain(node.right) ? node.right : undefined; } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts index 4308adee80ccd..8e8a6aaa9518e 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts @@ -1,7 +1,7 @@ /// ////let a = { b: { c: 0 } }; -/////*a*/a && a.b && a.b.c/*b*/; +/////*a*/a && a.b && a.b.c;/*b*/ goTo.select("a", "b"); edit.applyRefactor({ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts index 8e355fa5a4374..a74717ebce8c8 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts @@ -1,28 +1,7 @@ /// -////let a = { b: { c: { d: 0 } } }; -/////*1a*/a || a.b && a.b.c && a.b.c.d/*1b*/; -/////*2a*/a && a.b || a.b.c && a.b.c.d/*2b*/; -/////*3a*/a && a.b && a.b.c || a.b.c.d/*3b*/; -/////*4a*/a ?? a.b && a.b.c && a.b.c.d/*4b*/; -/////*5a*/a && a.b ?? a.b.c || a.b.c.d/*5b*/; -/////*6a*/a && a.b && a.b.c ?? a.b.c.d/*6b*/; +////let a = { b: { c: 0 } }; +/////*a*/a && a.b && a?.b?.c;/*b*/ -// Only offer refactor for && chains. -goTo.select("1a", "1b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); - -goTo.select("2a", "2b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); - -goTo.select("3a", "3b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); - -goTo.select("4a", "4b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); - -goTo.select("5a", "5b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); - -goTo.select("6a", "6b"); -verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts index ecc4a7b550c14..1fa392e7c919a 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts @@ -1,7 +1,7 @@ /// ////let a = { b: { c: () => { } } }; -/////*a*/a && a.b && a.b.c()/*b*/; +/////*a*/a && a.b && a.b.c();/*b*/ goTo.select("a", "b"); edit.applyRefactor({ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts index beb40c26d52fe..82e5a484bf243 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts @@ -1,7 +1,7 @@ /// ////let a = { b: { c: 0 } }; -/////*a*/a && a.b && a.b.c === 1/*b*/; +/////*a*/a && a.b && a.b.c === 1;/*b*/ goTo.select("a", "b"); edit.applyRefactor({ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts index a9c9e6cb3f05f..75bb9ecd27a3e 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts @@ -1,7 +1,7 @@ /// ////let a = { b: { c: () => { } } }; -/////*a*/a && a.b && a.b.c() === 1/*b*/; +/////*a*/a && a.b && a.b.c() === 1;/*b*/ goTo.select("a", "b"); edit.applyRefactor({ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts index 452bb72183ada..c7f4dfdba7d95 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts @@ -6,7 +6,7 @@ //// } ////} ////declare let foo: Foo | undefined; -/////*a*/foo && foo.bar && foo.bar.baz/*b*/; +/////*a*/foo && foo.bar && foo.bar.baz;/*b*/ goTo.select("a", "b"); edit.applyRefactor({ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts index b97f810295db9..431c65cac0ba5 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts @@ -1,8 +1,7 @@ /// -////let a = { b: 0 }; -////let x = { y: 0 }; -////a && a.b() && /*a*/x && x.y()/*b*/; +////let a = { b: { c: 0 } }; +/////*a*/a.b && a.b.c;/*b*/ goTo.select("a", "b"); edit.applyRefactor({ @@ -10,7 +9,6 @@ edit.applyRefactor({ actionName: "Convert to optional chain expression", actionDescription: "Convert && chain to optional chain expression", newContent: -`let a = { b: 0 }; -let x = { y: 0 }; -a && a.b() && x?.y();` +`let a = { b: { c: 0 } }; +a?.b?.c;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts index 710620be29129..5dffd5cc7e108 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts @@ -1,14 +1,28 @@ /// -////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 && chain to optional chain expression", - newContent: -`let a = { b: { c: 0 } }; -a?.b?.c;` -}); \ No newline at end of file +////let a = { b: { c: { d: 0 } } }; +/////*1a*/a || a.b && a.b.c && a.b.c.d;/*1b*/ +/////*2a*/a && a.b || a.b.c && a.b.c.d;/*2b*/ +/////*3a*/a && a.b && a.b.c || a.b.c.d;/*3b*/ +/////*4a*/a ?? a.b && a.b.c && a.b.c.d;/*4b*/ +/////*5a*/a && a.b ?? a.b.c || a.b.c.d;/*5b*/ +/////*6a*/a && a.b && a.b.c ?? a.b.c.d;/*6b*/ + +// Only offer refactor for && chains. +goTo.select("1a", "1b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("2a", "2b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("3a", "3b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("4a", "4b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("5a", "5b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +goTo.select("6a", "6b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); From 796b2bff77980f5d89e826b8aca9ddc96e82b13e Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Sat, 20 Jun 2020 14:25:05 -0700 Subject: [PATCH 16/46] add ternary case --- .../convertToOptionalChainExpression.ts | 80 ++++++++++++------- ...actorConvertToOptionalChainExpression11.ts | 14 ++++ ...actorConvertToOptionalChainExpression12.ts | 14 ++++ ...factorConvertToOptionalChainExpression8.ts | 2 +- ...ptionalChainExpressionForTriggerReason3.ts | 9 +++ 5 files changed, 87 insertions(+), 32 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index ec42253f05c3b..23185244e887e 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -21,14 +21,14 @@ namespace ts.refactor.convertToOptionalChainExpression { 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, t, info, actionName)); + 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, - firstOccurence: Node, - containingExpression: BinaryExpression + firstOccurrence: Node, + expression: BinaryExpression | ConditionalExpression } function getInfo(context: RefactorContext, considerEmptySpans = true): Info | undefined { @@ -40,34 +40,46 @@ namespace ts.refactor.convertToOptionalChainExpression { const startToken = getTokenAtPosition(file, span.start); - const parent = forEmptySpan ? findAncestor(startToken, (node) => { return isExpressionStatement(node) && isBinaryExpression(node.expression); }) : getParentNodeInSpan(startToken, file, span); - if (!parent) return undefined; + const containingNode = forEmptySpan ? findAncestor(startToken, (node) => { return isExpressionStatement(node) && (isBinaryExpression(node.expression) || isConditionalExpression(node.expression)); }) : getParentNodeInSpan(startToken, file, span); + const expression = containingNode && isExpressionStatement(containingNode) ? containingNode.expression : containingNode; + if (!expression) return undefined; - const binaryExpression = isExpressionStatement(parent) && isBinaryExpression(parent.expression) ? parent.expression : isBinaryExpression(parent) ? parent : undefined; - if (!binaryExpression) return undefined; + const checker = program.getTypeChecker(); - const start = forEmptySpan ? binaryExpression.pos : startToken.pos; - const checker = program.getTypeChecker(); - const fullPropertyAccess = getFullPropertyAccessChain(binaryExpression); - if (!fullPropertyAccess) return undefined; - if (binaryExpression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && binaryExpression.operatorToken.pos <= fullPropertyAccess.pos) return undefined; - - // ensure that each sequential operand in range matches the longest acceess chain - let checkNode = binaryExpression.left; - let firstOccurence: PropertyAccessExpression | Identifier = fullPropertyAccess; - while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right)) && checkNode.right.pos >= start) { - if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { - return undefined; + if (isBinaryExpression(expression)) { + 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) { + if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { + 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; } - firstOccurence = checkNode.right; - checkNode = checkNode.left; + return firstOccurrence ? { fullPropertyAccess, firstOccurrence, expression } : undefined; } - // check final identifier - if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= start) { - firstOccurence = checkNode; + + if (isConditionalExpression(expression)) { + 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 firstOccurence ? { fullPropertyAccess, firstOccurence, containingExpression:binaryExpression } : undefined; + return undefined; } function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { @@ -94,15 +106,21 @@ namespace ts.refactor.convertToOptionalChainExpression { ? node.right : undefined; } - function convertPropertyAccessToOptionalChain(toConvert: PropertyAccessExpression): PropertyAccessExpression { - if (isPropertyAccessExpression(toConvert.expression)) { - return factory.createPropertyAccessChain(convertPropertyAccessToOptionalChain(toConvert.expression), factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + 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, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { - const { fullPropertyAccess, firstOccurence } = info; - changes.replaceNodeRange(sourceFile, firstOccurence, fullPropertyAccess, convertPropertyAccessToOptionalChain(fullPropertyAccess)); + 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)); + } } } \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts new file mode 100644 index 0000000000000..c46a6b4027a58 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts @@ -0,0 +1,14 @@ +/// + +////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 && chain to optional chain expression", + newContent: +`let a = { b: 0 }; +a?.b ?? "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts new file mode 100644 index 0000000000000..68c3a8b6a1f02 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts @@ -0,0 +1,14 @@ +/// + +////let a = { b: { c: 0 } }; +/////*a*/a.b ? a.b.c : "whenFalse";/*b*/ + +goTo.select("a", "b"); +edit.applyRefactor({ + refactorName: "Convert to optional chain expression", + actionName: "Convert to optional chain expression", + actionDescription: "Convert && chain to optional chain expression", + newContent: +`let a = { b: { c: 0 } }; +a.b?.c ?? "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts index 431c65cac0ba5..89bf7c60fa69b 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts @@ -10,5 +10,5 @@ edit.applyRefactor({ actionDescription: "Convert && chain to optional chain expression", newContent: `let a = { b: { c: 0 } }; -a?.b?.c;` +a.b?.c;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts new file mode 100644 index 0000000000000..949df00ecf659 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts @@ -0,0 +1,9 @@ +/// + +////let a = { b: { c: 0 } }; +////a.b ? /*a*//*b*/a.b.c : "whenFalse"; + +// Only offer refactor for empty span if explicity requested +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); +verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); From 37004b7877e85255afc45ef2293bec2187b70526 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Sat, 20 Jun 2020 14:42:44 -0700 Subject: [PATCH 17/46] add diagnostic message --- src/compiler/diagnosticMessages.json | 4 ++++ src/services/refactors/convertToOptionalChainExpression.ts | 6 +++--- .../fourslash/refactorConvertToOptionalChainExpression1.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression11.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression12.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression2.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression3.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression4.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression5.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression6.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression7.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression8.ts | 2 +- ...ctorConvertToOptionalChainExpressionForTriggerReason2.ts | 2 +- 13 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index e863de12537d6..de084f317ca4f 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -5737,6 +5737,10 @@ "category": "Message", "code": 95125 }, + "Convert to optional chain expression": { + "category": "Message", + "code": 95126 + }, "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": { "category": "Error", diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 23185244e887e..8a19ed9ff9e79 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -1,7 +1,7 @@ /* @internal */ namespace ts.refactor.convertToOptionalChainExpression { const refactorName = "Convert to optional chain expression"; - const convertToOptionalChainExpression = "Convert && chain to optional chain expression"; + const convertToOptionalChainExpressionMessage = getLocaleSpecificMessage(Diagnostics.Convert_to_optional_chain_expression); registerRefactor(refactorName, { getAvailableActions, getEditsForAction }); @@ -10,10 +10,10 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!info) return emptyArray; return [{ name: refactorName, - description: convertToOptionalChainExpression, + description: convertToOptionalChainExpressionMessage, actions: [{ name: refactorName, - description: convertToOptionalChainExpression + description: convertToOptionalChainExpressionMessage }] }]; } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts index 8e8a6aaa9518e..400c84adb43e8 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; a?.b?.c;` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts index c46a6b4027a58..7c73a2d2ee2e4 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: 0 }; a?.b ?? "whenFalse";` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts index 68c3a8b6a1f02..85e6fec971c63 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; a.b?.c ?? "whenFalse";` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts index 1fa392e7c919a..9385a0268b7cf 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: () => { } } }; a?.b?.c();` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts index 82e5a484bf243..2e61bfea4da45 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; a?.b?.c === 1;` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts index 75bb9ecd27a3e..d9397ab11f2d8 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: () => { } } }; a?.b?.c() === 1;` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts index bcfddd8cc19cd..03562de6639c7 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; if(a?.b?.c){};` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts index c7f4dfdba7d95..1d777daa8c04c 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts @@ -12,7 +12,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `interface Foo { bar?:{ diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts index ee4a0d303afd2..a136c63709d68 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts @@ -8,7 +8,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: 0 }; let x = { y: 0 }; diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts index 89bf7c60fa69b..29b7710dcd39f 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; a.b?.c;` diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts index 9341f0d37892c..e13f9676438c6 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts @@ -7,7 +7,7 @@ goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", actionName: "Convert to optional chain expression", - actionDescription: "Convert && chain to optional chain expression", + actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; a?.b?.c;`, From afb0e44a31c901d144f9ab495da6dbaed28cf584 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 22 Jun 2020 13:54:31 -0700 Subject: [PATCH 18/46] add nullish check for ternary expressions --- .../refactors/convertToOptionalChainExpression.ts | 6 +++++- .../refactorConvertToOptionalChainExpression13.ts | 15 +++++++++++++++ .../refactorConvertToOptionalChainExpression14.ts | 15 +++++++++++++++ .../refactorConvertToOptionalChainExpression15.ts | 15 +++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 8a19ed9ff9e79..e1112ed4b835b 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -74,8 +74,12 @@ namespace ts.refactor.convertToOptionalChainExpression { if (isConditionalExpression(expression)) { const whenTrue = expression.whenTrue; const condition = expression.condition; - if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) { + // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor + const type = checker.getTypeAtLocation(whenTrue.name); + if (checker.isNullableType(type) || type.flags & TypeFlags.Any) { + return undefined; + } return { fullPropertyAccess: whenTrue, firstOccurrence: condition, expression }; } } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts new file mode 100644 index 0000000000000..995fd55d15717 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts @@ -0,0 +1,15 @@ +/// + +// @strict: true + +////interface Foo { +//// bar?:{ +//// baz: string; +//// } +////} +////declare let foo: Foo; +/////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ + +// Offer the refactor for ternary expressions if type of baz is not any, null, unknown, or undefined +goTo.select("a", "b"); +verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts new file mode 100644 index 0000000000000..3845f588d90b9 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts @@ -0,0 +1,15 @@ +/// + +// @strict: true + +////interface Foo { +//// bar?:{ +//// baz?: any; +//// } +////} +////declare let foo: Foo; +/////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ + +// do not offer a refactor for ternary expression if type of baz is any +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts new file mode 100644 index 0000000000000..eef8c58c84397 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts @@ -0,0 +1,15 @@ +/// + +// @strict: true + +////interface Foo { +//// bar?:{ +//// baz?: string | null; +//// } +////} +////declare let foo: Foo; +/////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ + +// do not offer a refactor for ternary expression if type of baz is nullish +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); From 65ca81e6e373373b1fc5da9527e900d451cc2a07 Mon Sep 17 00:00:00 2001 From: Jesse Trinity <42591254+jessetrinity@users.noreply.github.com> Date: Mon, 22 Jun 2020 14:42:18 -0700 Subject: [PATCH 19/46] Update src/services/refactors/convertToOptionalChainExpression.ts Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- src/services/refactors/convertToOptionalChainExpression.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index e1112ed4b835b..690738fced892 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -35,8 +35,8 @@ namespace ts.refactor.convertToOptionalChainExpression { const { file, program } = context; const span = getRefactorContextSpan(context); - if (span.length === 0 && !considerEmptySpans) return undefined; - const forEmptySpan = span.length === 0 && considerEmptySpans; + const forEmptySpan = span.length === 0; + if (forEmptySpan && !considerEmptySpans) return undefined; const startToken = getTokenAtPosition(file, span.start); @@ -127,4 +127,4 @@ namespace ts.refactor.convertToOptionalChainExpression { changes.replaceNode(sourceFile, expression, factory.createBinaryExpression(convertPropertyAccessToOptionalChain(checker, fullPropertyAccess, until), factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse)); } } -} \ No newline at end of file +} From d9c34ff5d978ab24582933408ca12701124e20a7 Mon Sep 17 00:00:00 2001 From: Jesse Trinity <42591254+jessetrinity@users.noreply.github.com> Date: Mon, 22 Jun 2020 14:42:57 -0700 Subject: [PATCH 20/46] Update src/services/refactors/convertToOptionalChainExpression.ts Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- src/services/refactors/convertToOptionalChainExpression.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 690738fced892..0f41e86d0d1ce 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -40,7 +40,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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); + const containingNode = forEmptySpan ? findAncestor(startToken, node => isExpressionStatement(node) && (isBinaryExpression(node.expression) || isConditionalExpression(node.expression))) : getParentNodeInSpan(startToken, file, span); const expression = containingNode && isExpressionStatement(containingNode) ? containingNode.expression : containingNode; if (!expression) return undefined; From c2b9924eb490ae8a22f9ce8265dafb1dbe262fa7 Mon Sep 17 00:00:00 2001 From: Jesse Trinity <42591254+jessetrinity@users.noreply.github.com> Date: Mon, 22 Jun 2020 14:43:15 -0700 Subject: [PATCH 21/46] Update tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- ...refactorConvertToOptionalChainExpressionForTriggerReason3.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts index 949df00ecf659..958c5999e7e24 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts @@ -3,7 +3,7 @@ ////let a = { b: { c: 0 } }; ////a.b ? /*a*//*b*/a.b.c : "whenFalse"; -// Only offer refactor for empty span if explicity requested +// Only offer refactor for empty span if explicitly requested goTo.select("a", "b"); verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); From 1d51dab4b11bce83cf7a2bd63a22f8f771c1ae23 Mon Sep 17 00:00:00 2001 From: Jesse Trinity <42591254+jessetrinity@users.noreply.github.com> Date: Mon, 22 Jun 2020 15:02:32 -0700 Subject: [PATCH 22/46] Update tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- ...refactorConvertToOptionalChainExpressionForTriggerReason1.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts index 4cbaab28834d7..f13a196a4afd7 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts @@ -3,7 +3,7 @@ ////let a = { b: { c: 0 } }; ////a && a.b && /*a*//*b*/a.b.c; -// Only offer refactor for empty span if explicity requested +// Only offer refactor for empty span if explicitly requested goTo.select("a", "b"); verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); From adbd586f65dd11e1b5dd9e30f68b5d33e88f6baa Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 22 Jun 2020 15:23:26 -0700 Subject: [PATCH 23/46] reformat and remove unused checks --- .../convertToOptionalChainExpression.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 0f41e86d0d1ce..a6eb5a6dd6489 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -48,8 +48,6 @@ namespace ts.refactor.convertToOptionalChainExpression { if (isBinaryExpression(expression)) { - 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; @@ -57,7 +55,8 @@ namespace ts.refactor.convertToOptionalChainExpression { // 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) { + while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && + (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { return undefined; } @@ -65,7 +64,7 @@ namespace ts.refactor.convertToOptionalChainExpression { checkNode = checkNode.left; } // check final identifier - if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode) && checkNode.pos >= start) { + if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode)) { firstOccurrence = checkNode; } return firstOccurrence ? { fullPropertyAccess, firstOccurrence, expression } : undefined; @@ -74,7 +73,8 @@ namespace ts.refactor.convertToOptionalChainExpression { if (isConditionalExpression(expression)) { const whenTrue = expression.whenTrue; const condition = expression.condition; - if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) { + if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && + isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) { // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor const type = checker.getTypeAtLocation(whenTrue.name); if (checker.isNullableType(type) || type.flags & TypeFlags.Any) { @@ -105,9 +105,9 @@ namespace ts.refactor.convertToOptionalChainExpression { } function getFullPropertyAccessChain(node: BinaryExpression): PropertyAccessExpression | undefined { - return isBinaryExpression(node.right) || isCallExpression(node.right) - ? getRightHandSidePropertyAccess(node.right) : node.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && isPropertyAccessExpression(node.right) && !isOptionalChain(node.right) - ? node.right : undefined; + return isBinaryExpression(node.right) || isCallExpression(node.right) ? getRightHandSidePropertyAccess(node.right) + : isPropertyAccessExpression(node.right) && !isOptionalChain(node.right) ? node.right + : undefined; } function convertPropertyAccessToOptionalChain(checker: TypeChecker, toConvert: PropertyAccessExpression, until: Identifier | PrivateIdentifier): PropertyAccessExpression { From fb6b8313e861032869fd352aac453e7d2fc80546 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 08:41:39 -0700 Subject: [PATCH 24/46] allow any for ternary refactor --- src/services/refactors/convertToOptionalChainExpression.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index a6eb5a6dd6489..7d16bc39cb88e 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -46,7 +46,6 @@ namespace ts.refactor.convertToOptionalChainExpression { const checker = program.getTypeChecker(); - if (isBinaryExpression(expression)) { const fullPropertyAccess = getFullPropertyAccessChain(expression); if (!fullPropertyAccess) return undefined; @@ -77,10 +76,7 @@ namespace ts.refactor.convertToOptionalChainExpression { isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) { // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor const type = checker.getTypeAtLocation(whenTrue.name); - if (checker.isNullableType(type) || type.flags & TypeFlags.Any) { - return undefined; - } - return { fullPropertyAccess: whenTrue, firstOccurrence: condition, expression }; + return !checker.isNullableType(type) ? { fullPropertyAccess: whenTrue, firstOccurrence: condition, expression } : undefined; } } return undefined; From 8184ecfe0a32aad201508ebc5efb64f87c01931e Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 09:02:14 -0700 Subject: [PATCH 25/46] add tests --- ...refactorConvertToOptionalChainExpression14.ts | 5 +++-- ...refactorConvertToOptionalChainExpression16.ts | 16 ++++++++++++++++ ...refactorConvertToOptionalChainExpression17.ts | 16 ++++++++++++++++ ...refactorConvertToOptionalChainExpression18.ts | 14 ++++++++++++++ ...refactorConvertToOptionalChainExpression19.ts | 8 ++++++++ ...refactorConvertToOptionalChainExpression20.ts | 8 ++++++++ ...refactorConvertToOptionalChainExpression21.ts | 8 ++++++++ ...refactorConvertToOptionalChainExpression22.ts | 9 +++++++++ ...refactorConvertToOptionalChainExpression23.ts | 9 +++++++++ 9 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts index 3845f588d90b9..7ed6b5513fd5a 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts @@ -10,6 +10,7 @@ ////declare let foo: Foo; /////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ -// do not offer a refactor for ternary expression if type of baz is any +// It is reasonable to offer a refacor when baz is of type any since using any in strict mode +// produces an error and those with strict mode off aren't getting null checks anyway. goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); +verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts new file mode 100644 index 0000000000000..229863ef24b74 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts @@ -0,0 +1,16 @@ +/// + +////let a = { b: { c: 0 } }; +////let foo; +/////*a*/foo && 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 } }; +let foo; +foo && a?.b?.c;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts new file mode 100644 index 0000000000000..95fb3237befeb --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts @@ -0,0 +1,16 @@ +/// + +////let a = { b: { c: 0 } }; +////let foo; +/////*a*/a && a.b && a.b.c/*b*/ && foo; + +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 } }; +let foo; +a?.b?.c && foo;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts new file mode 100644 index 0000000000000..5ddefb61d9c08 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts @@ -0,0 +1,14 @@ +/// + +////let a = { b: { c: 0 } }; +/////*a*/a && a.b/*b*/ ? a.b.c : "whenFalse"; + +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 ? a.b.c : "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts new file mode 100644 index 0000000000000..94d8a9979f781 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts @@ -0,0 +1,8 @@ +/// + +////let a = { b: { c: 0 } }; +/////*a*/a && a.b ? a.b.c : "whenFalse";/*b*/ + +// We do not combine condition and whenTrue to an optional chain but should in the future. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts new file mode 100644 index 0000000000000..0ce3a175da246 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts @@ -0,0 +1,8 @@ +/// + +////let a = { b: () => { return { c: 0 } } } +/////*a*/a && a.b && a.b().c/*b*/; + +// We do not currently offer a refactor for this case but may want to in the future. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts new file mode 100644 index 0000000000000..225f5c90651c6 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts @@ -0,0 +1,8 @@ +/// + +////let a = { b: () => { return () => { } } } +/////*a*/a && a.b()()/*b*/; + +// We do not currently offer a refactor for this case but may want to in the future. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts new file mode 100644 index 0000000000000..4879612b27971 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts @@ -0,0 +1,9 @@ +/// + +////let a = { b: 0 }; +////let x = { b: 0 }; +/////*a*/a && x && a.b && x.y;/*b*/ + +// We don't currently offer a refactor for this case but should add it in the future. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts new file mode 100644 index 0000000000000..9e07f1ca6d2fe --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts @@ -0,0 +1,9 @@ +/// + +////let a = { b: 0 }; +////let x = { b: 0 }; +/////*a*/a && a.b && x && x.y;/*b*/ + +// We don't currently offer a refactor for this case but should add it in the future. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file From 5ac29a002c66ca0c70396974e494ab8f5a17aa55 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 12:33:43 -0700 Subject: [PATCH 26/46] add tests --- ...actorConvertToOptionalChainExpression24.ts | 18 ++++++++++++ ...actorConvertToOptionalChainExpression25.ts | 18 ++++++++++++ ...actorConvertToOptionalChainExpression26.ts | 28 +++++++++++++++++++ ...actorConvertToOptionalChainExpression27.ts | 14 ++++++++++ ...actorConvertToOptionalChainExpression28.ts | 14 ++++++++++ ...actorConvertToOptionalChainExpression29.ts | 19 +++++++++++++ ...ptionalChainExpressionForTriggerReason1.ts | 13 +++++++-- ...ptionalChainExpressionForTriggerReason2.ts | 7 +++-- ...ptionalChainExpressionForTriggerReason3.ts | 19 +++++++++++-- ...ptionalChainExpressionForTriggerReason4.ts | 22 +++++++++++++++ ...ptionalChainExpressionForTriggerReason5.ts | 18 ++++++++++++ ...ptionalChainExpressionForTriggerReason6.ts | 18 ++++++++++++ 12 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts new file mode 100644 index 0000000000000..78b39d484fdfd --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////function f(){ +//// return /*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 } }; +function f(){ + return a?.b?.c; +}` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts new file mode 100644 index 0000000000000..3849741eb9e97 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////function f(){ +//// return /*a*/a.b ? a.b.c : "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: { c: 0 } }; +function f(){ + return a.b?.c ?? "whenFalse"; +}` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts new file mode 100644 index 0000000000000..54a4c0d90a287 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts @@ -0,0 +1,28 @@ +/// + +////let a = { b: { c: 0 } }; +////function f()1{ +//// /*1a*/return a && a.b && a.b.c;/*1b*/ +////} +////function f()2{ +//// return /*2a*/a && a.b && a.b.c;/*2b*/ +////} +////function f()3{ +//// return /*3a*/a && a.b && a.b.c/*3b*/; +////} +////function f()4{ +//// return /*4a*/a.b ? a.b.c : "whenFalse";/*4b*/ +////} + +// valid spans for return statement +goTo.select("1a", "1b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("2a", "2b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("3a", "3b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("4a", "4b"); +verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts new file mode 100644 index 0000000000000..205c3f893347e --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts @@ -0,0 +1,14 @@ +/// + +////let a = { b: { c: 0 } }; +////let x = /*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 } }; +let x = a?.b?.c;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts new file mode 100644 index 0000000000000..97ff17e680ee5 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts @@ -0,0 +1,14 @@ +/// + +////let a = { b: { c: 0 } }; +////let x = /*a*/a.b ? a.b.c : "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: { c: 0 } }; +let x = a.b?.c ?? "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts new file mode 100644 index 0000000000000..67eab8346ed58 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts @@ -0,0 +1,19 @@ +/// + +////let a = { b: { c: 0 } }; +/////*1a*/let x1 = a && a.b && a.b.c;/*1b*/ +////let x2 = /*2a*/a && a.b && a.b.c;/*2b*/ +////let x3 = /*3a*/a && a.b && a.b.c/*3b*/; +////let x4 = /*4a*/a.b ? a.b.c : "whenFalse"/*4b*/; + +goTo.select("1a", "1b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("2a", "2b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("3a", "3b"); +verify.refactorAvailable("Convert to optional chain expression"); + +goTo.select("4a", "4b"); +verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts index f13a196a4afd7..ad68d59052080 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts @@ -3,7 +3,16 @@ ////let a = { b: { c: 0 } }; ////a && a.b && /*a*//*b*/a.b.c; -// Only offer refactor for empty span if explicitly requested +// verify that the refactor is offered for empty spans in expression statements. goTo.select("a", "b"); verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); -verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); + +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;`, + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts index e13f9676438c6..caf1ccab89a12 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts @@ -1,15 +1,18 @@ /// ////let a = { b: { c: 0 } }; -////a && a.b && /*a*//*b*/a.b.c; +////a.b ? /*a*//*b*/a.b.c : "whenFalse"; +// verify that the refactor is offered for empty spans in expression statements. goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + 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;`, +a.b?.c ?? "whenFalse";`, triggerReason: "invoked" }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts index 958c5999e7e24..fe4f85484dfff 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts @@ -1,9 +1,22 @@ /// ////let a = { b: { c: 0 } }; -////a.b ? /*a*//*b*/a.b.c : "whenFalse"; +////function f(){ +//// return a && a.b && /*a*//*b*/a.b.c; +////} -// Only offer refactor for empty span if explicitly requested +// verify that the refactor is offered for empty spans in return statements. goTo.select("a", "b"); verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); -verify.refactorAvailableForTriggerReason("invoked", "Convert to optional chain expression"); + +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 } }; +function f(){ + return a?.b?.c; +}`, + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts new file mode 100644 index 0000000000000..494cb91986d68 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts @@ -0,0 +1,22 @@ +/// + +////let a = { b: { c: 0 } }; +////function f(){ +//// return a.b ? /*a*//*b*/a.b.c : "whenFalse"; +////} + +// verify that the refactor is offered for empty spans in return statements. +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +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 } }; +function f(){ + return a.b?.c ?? "whenFalse"; +}`, + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts new file mode 100644 index 0000000000000..b8233a55c4d89 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////let x = a && a.b && /*a*//*b*/a.b.c; + +// verify that the refactor is offered for empty spans in variable statements. +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +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 } }; +let x = a?.b?.c;`, + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts new file mode 100644 index 0000000000000..75445c055d59f --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////let x = a.b ? /*a*//*b*/a.b.c : "whenFalse"; + +// verify that the refactor is offered for empty spans in variable statements. +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +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 } }; +let x = a.b?.c ?? "whenFalse";`, + triggerReason: "invoked" +}); \ No newline at end of file From a0708be3fcb4a5ed68b7e207c6052245897d1729 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 12:36:35 -0700 Subject: [PATCH 27/46] check return and variable statements --- src/compiler/utilities.ts | 2 +- .../convertToOptionalChainExpression.ts | 42 +++++++++++++++++-- ...ptionalChainExpressionForTriggerReason5.ts | 10 +++-- ...ptionalChainExpressionForTriggerReason6.ts | 4 +- ...ptionalChainExpressionForTriggerReason7.ts | 18 ++++++++ ...ptionalChainExpressionForTriggerReason8.ts | 18 ++++++++ 6 files changed, 85 insertions(+), 9 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index b147e79348733..38e2eb9509e68 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -2460,7 +2460,7 @@ namespace ts { } } - function getSingleVariableOfVariableStatement(node: Node): VariableDeclaration | undefined { + export function getSingleVariableOfVariableStatement(node: Node): VariableDeclaration | undefined { return isVariableStatement(node) ? firstOrUndefined(node.declarationList.declarations) : undefined; } diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 7d16bc39cb88e..6d4e08d2e71ac 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -31,6 +31,22 @@ namespace ts.refactor.convertToOptionalChainExpression { expression: BinaryExpression | ConditionalExpression } + type ValidExpressionOrStatement = ValidExpression | ValidStatement; + type ValidExpression = BinaryExpression | ConditionalExpression; + 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); @@ -40,8 +56,8 @@ namespace ts.refactor.convertToOptionalChainExpression { const startToken = getTokenAtPosition(file, span.start); - const containingNode = forEmptySpan ? findAncestor(startToken, node => isExpressionStatement(node) && (isBinaryExpression(node.expression) || isConditionalExpression(node.expression))) : getParentNodeInSpan(startToken, file, span); - const expression = containingNode && isExpressionStatement(containingNode) ? containingNode.expression : containingNode; + const parent = forEmptySpan ? getParentForEmptySpanStartToken(startToken) : getParentNodeInSpan(startToken, file, span); + const expression = parent && isValidExpressionOrStatement(parent) ? getExpression(parent) : undefined; if (!expression) return undefined; const checker = program.getTypeChecker(); @@ -51,7 +67,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 + // ensure that each sequential operand in range matches the longest access chain let checkNode = expression.left; let firstOccurrence: PropertyAccessExpression | Identifier = fullPropertyAccess; while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && @@ -82,6 +98,26 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } + function getParentForEmptySpanStartToken(start: Node): ValidExpressionOrStatement | undefined { + return findAncestor(start, (node): node is ValidExpressionOrStatement => { + return node.parent && + (isValidExpression(node) && !isValidExpression(node.parent) || + isValidStatement(node) && !isValidStatement(node.parent)); + }); + } + + 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; + } + function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { if (isCallExpression(node) && isPropertyAccessExpression(node.expression)) { // a && |a.b|(); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts index b8233a55c4d89..3f7b6bf10fd53 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts @@ -1,9 +1,11 @@ /// ////let a = { b: { c: 0 } }; -////let x = a && a.b && /*a*//*b*/a.b.c; +////function f(){ +//// ret/*a*//*b*/urn a.b ? a.b.c : "whenFalse"; +////} -// verify that the refactor is offered for empty spans in variable statements. +// verify that the refactor is offered for empty spans in return statements. goTo.select("a", "b"); verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); @@ -13,6 +15,8 @@ edit.applyRefactor({ actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; -let x = a?.b?.c;`, +function f(){ + return a.b?.c ?? "whenFalse"; +}`, triggerReason: "invoked" }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts index 75445c055d59f..b8233a55c4d89 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts @@ -1,7 +1,7 @@ /// ////let a = { b: { c: 0 } }; -////let x = a.b ? /*a*//*b*/a.b.c : "whenFalse"; +////let x = a && a.b && /*a*//*b*/a.b.c; // verify that the refactor is offered for empty spans in variable statements. goTo.select("a", "b"); @@ -13,6 +13,6 @@ edit.applyRefactor({ actionDescription: "Convert to optional chain expression", newContent: `let a = { b: { c: 0 } }; -let x = a.b?.c ?? "whenFalse";`, +let x = a?.b?.c;`, triggerReason: "invoked" }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts new file mode 100644 index 0000000000000..75445c055d59f --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////let x = a.b ? /*a*//*b*/a.b.c : "whenFalse"; + +// verify that the refactor is offered for empty spans in variable statements. +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +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 } }; +let x = a.b?.c ?? "whenFalse";`, + triggerReason: "invoked" +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts new file mode 100644 index 0000000000000..6f6bd981979ea --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts @@ -0,0 +1,18 @@ +/// + +////let a = { b: { c: 0 } }; +////let/*a*//*b*/ x = a.b ? a.b.c : "whenFalse"; + +// verify that the refactor is offered for empty spans in variable statements. +goTo.select("a", "b"); +verify.not.refactorAvailableForTriggerReason("implicit", "Convert to optional chain expression"); + +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 } }; +let x = a.b?.c ?? "whenFalse";`, + triggerReason: "invoked" +}); \ No newline at end of file From 6810cea9facd825bbe7c2ed4babcdb69637b0eb8 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 13:43:50 -0700 Subject: [PATCH 28/46] use isMatchingReference instead of containsMatchingReference --- src/compiler/checker.ts | 4 +- src/compiler/types.ts | 2 +- .../convertToOptionalChainExpression.ts | 50 +++++++++++-------- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 45e405e29b988..5a434193b3609 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -674,10 +674,10 @@ namespace ts { getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, isDeclarationVisible, - containsMatchingReference: (nodeIn, targetIn) => { + isMatchingReference: (nodeIn, targetIn) => { const node = getParseTreeNode(nodeIn); const target = getParseTreeNode(targetIn); - return !!node && !!target && containsMatchingReference(node, target); + return !!node && !!target && isMatchingReference(node, target); }, }; diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 9519f7425fde0..7bf22da85d417 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4120,7 +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 */ isMatchingReference(node: Node, target: Node): boolean; } /* @internal */ diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 6d4e08d2e71ac..68a914152ef8a 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -26,7 +26,7 @@ namespace ts.refactor.convertToOptionalChainExpression { } interface Info { - fullPropertyAccess: PropertyAccessExpression, + lastPropertyAccessChain: PropertyAccessExpression, firstOccurrence: Node, expression: BinaryExpression | ConditionalExpression } @@ -63,36 +63,40 @@ namespace ts.refactor.convertToOptionalChainExpression { const checker = program.getTypeChecker(); if (isBinaryExpression(expression)) { - const fullPropertyAccess = getFullPropertyAccessChain(expression); - if (!fullPropertyAccess) return undefined; - if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= fullPropertyAccess.pos) return undefined; + const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); + if (!lastPropertyAccessChain) return undefined; + if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= lastPropertyAccessChain.pos) return undefined; // ensure that each sequential operand in range matches the longest access chain let checkNode = expression.left; - let firstOccurrence: PropertyAccessExpression | Identifier = fullPropertyAccess; + let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { - if (!checker.containsMatchingReference(fullPropertyAccess, checkNode.right)) { + const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; + if (!checker.isMatchingReference(previousReference, checkNode.right)) { return undefined; } firstOccurrence = checkNode.right; checkNode = checkNode.left; } - // check final identifier - if ((isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) && checker.containsMatchingReference(fullPropertyAccess, checkNode)) { - firstOccurrence = checkNode; + // check final LHS node + if (isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) { + const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; + if (isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { + firstOccurrence = checkNode; + } } - return firstOccurrence ? { fullPropertyAccess, firstOccurrence, expression } : undefined; + return firstOccurrence ? { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } if (isConditionalExpression(expression)) { const whenTrue = expression.whenTrue; const condition = expression.condition; if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && - isPropertyAccessExpression(whenTrue) && checker.containsMatchingReference(whenTrue, condition)) { + isPropertyAccessExpression(whenTrue) && checker.isMatchingReference(whenTrue.expression, condition)) { // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor const type = checker.getTypeAtLocation(whenTrue.name); - return !checker.isNullableType(type) ? { fullPropertyAccess: whenTrue, firstOccurrence: condition, expression } : undefined; + return !checker.isNullableType(type) ? { lastPropertyAccessChain: whenTrue, firstOccurrence: condition, expression } : undefined; } } return undefined; @@ -119,26 +123,28 @@ namespace ts.refactor.convertToOptionalChainExpression { } function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { + // && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a + // binary expression with an operator if higher precedence. We may want to add recursion for more complex cases like a && a.b()()() == 1; if (isCallExpression(node) && isPropertyAccessExpression(node.expression)) { - // a && |a.b|(); + // a && a.b(); return node.expression; } else if (isBinaryExpression(node)) { if (isPropertyAccessExpression(node.left)) { - // a && |a.b| == 1; + // a && a.b == 1; return node.left; } else if (isCallExpression(node.left) && isPropertyAccessExpression(node.left.expression)) { - // a && |a.b|() == 1; + // a && a.b() == 1; return node.left.expression; } } return undefined; } - function getFullPropertyAccessChain(node: BinaryExpression): PropertyAccessExpression | undefined { - return isBinaryExpression(node.right) || isCallExpression(node.right) ? getRightHandSidePropertyAccess(node.right) - : isPropertyAccessExpression(node.right) && !isOptionalChain(node.right) ? node.right + function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { + return isBinaryExpression(node) || isCallExpression(node) ? getRightHandSidePropertyAccess(node) + : isPropertyAccessExpression(node) && !isOptionalChain(node) ? node : undefined; } @@ -150,13 +156,13 @@ namespace ts.refactor.convertToOptionalChainExpression { } 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; + const { lastPropertyAccessChain, firstOccurrence, expression } = info; + const until = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.name : isIdentifier(firstOccurrence) ? firstOccurrence : lastPropertyAccessChain.name; if (isBinaryExpression(expression)) { - changes.replaceNodeRange(sourceFile, firstOccurrence, fullPropertyAccess, convertPropertyAccessToOptionalChain(checker, fullPropertyAccess, until)); + changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertPropertyAccessToOptionalChain(checker, lastPropertyAccessChain, until)); } else if (isConditionalExpression(expression)) { - changes.replaceNode(sourceFile, expression, factory.createBinaryExpression(convertPropertyAccessToOptionalChain(checker, fullPropertyAccess, until), factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse)); + changes.replaceNode(sourceFile, expression, factory.createBinaryExpression(convertPropertyAccessToOptionalChain(checker, lastPropertyAccessChain, until), factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse)); } } } From 5634a4c3e4c488491fae13674ea76c921a95a473 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 14:41:46 -0700 Subject: [PATCH 29/46] allow partial selections --- .../convertToOptionalChainExpression.ts | 51 +++++++++++-------- ...actorConvertToOptionalChainExpression30.ts | 15 ++++++ ...actorConvertToOptionalChainExpression31.ts | 15 ++++++ ...actorConvertToOptionalChainExpression32.ts | 15 ++++++ ...ptionalChainExpressionForTriggerReason9.ts | 16 ++++++ 5 files changed, 90 insertions(+), 22 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 68a914152ef8a..6ee93f262eb24 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -54,9 +54,12 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 = getTokenAtPosition(file, span.start + span.length); + const adjustedSpan = createTextSpanFromNode(startToken, file, endToken); - const parent = forEmptySpan ? getParentForEmptySpanStartToken(startToken) : getParentNodeInSpan(startToken, file, span); + const parent = forEmptySpan ? getParentForEmptySpanStartToken(startToken) : getParentNodeInSpan(startToken, file, adjustedSpan); const expression = parent && isValidExpressionOrStatement(parent) ? getExpression(parent) : undefined; if (!expression) return undefined; @@ -65,27 +68,7 @@ namespace ts.refactor.convertToOptionalChainExpression { if (isBinaryExpression(expression)) { const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); if (!lastPropertyAccessChain) return undefined; - if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= lastPropertyAccessChain.pos) return undefined; - - // ensure that each sequential operand in range matches the longest access chain - let checkNode = expression.left; - let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; - while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && - (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { - const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; - if (!checker.isMatchingReference(previousReference, checkNode.right)) { - return undefined; - } - firstOccurrence = checkNode.right; - checkNode = checkNode.left; - } - // check final LHS node - if (isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) { - const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; - if (isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { - firstOccurrence = checkNode; - } - } + const firstOccurrence = getFirstOccurrence(expression, lastPropertyAccessChain, checker); return firstOccurrence ? { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } @@ -102,6 +85,30 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } + function getFirstOccurrence(expression: BinaryExpression, lastPropertyAccessChain: PropertyAccessExpression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= lastPropertyAccessChain.pos) return undefined; + // ensure that each sequential operand in range matches the longest access chain + let checkNode = expression.left; + let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; + while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && + (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { + const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; + if (!checker.isMatchingReference(previousReference, checkNode.right)) { + return undefined; + } + firstOccurrence = checkNode.right; + checkNode = checkNode.left; + } + // check final LHS node + if (isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) { + const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; + if (isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { + firstOccurrence = checkNode; + } + } + return firstOccurrence; + } + function getParentForEmptySpanStartToken(start: Node): ValidExpressionOrStatement | undefined { return findAncestor(start, (node): node is ValidExpressionOrStatement => { return node.parent && diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts new file mode 100644 index 0000000000000..1495cee31cbff --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts @@ -0,0 +1,15 @@ +/// + +////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;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts new file mode 100644 index 0000000000000..e394527764325 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts @@ -0,0 +1,15 @@ +/// + +////let foo = { bar: { baz: 0 } }; +////f/*a*/oo.bar ? foo.bar.baz : "when/*b*/False"; + +// 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 ?? "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts new file mode 100644 index 0000000000000..964f0089d73ee --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts @@ -0,0 +1,15 @@ +/// + +////let foo = { bar: { baz: 0 } }; +////f(/*a*/foo && foo.bar && foo.bar.baz/*b*/); + +// allow for call arguments +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 } }; +f(foo?.bar?.baz);` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts new file mode 100644 index 0000000000000..5b0a9cb22ed18 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts @@ -0,0 +1,16 @@ +/// + +////let foo = { bar: { baz: 0 } }; +////f(foo && foo.ba/*a*//*b*/r && foo.bar.baz); + +// allow for call arguments +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 } }; +f(foo?.bar?.baz);`, + triggerReason: "invoked" +}); \ No newline at end of file From e6e54cbd300b7ce81e19f6457c8cc081dab56cc0 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 23 Jun 2020 19:08:33 -0700 Subject: [PATCH 30/46] fine tune selection ranges --- .../convertToOptionalChainExpression.ts | 36 ++++++++++++------- ...actorConvertToOptionalChainExpression16.ts | 7 ++-- ...actorConvertToOptionalChainExpression17.ts | 7 ++-- ...actorConvertToOptionalChainExpression23.ts | 16 ++++++--- 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 6ee93f262eb24..adff19741ac36 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -56,10 +56,10 @@ namespace ts.refactor.convertToOptionalChainExpression { // 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 = getTokenAtPosition(file, span.start + span.length); - const adjustedSpan = createTextSpanFromNode(startToken, file, endToken); + 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 ? getParentForEmptySpanStartToken(startToken) : getParentNodeInSpan(startToken, file, adjustedSpan); + const parent = forEmptySpan ? getParentNodeForEmptySpan(startToken) : getParentNodeContainingSpan(startToken, adjustedSpan); const expression = parent && isValidExpressionOrStatement(parent) ? getExpression(parent) : undefined; if (!expression) return undefined; @@ -86,7 +86,7 @@ namespace ts.refactor.convertToOptionalChainExpression { } function getFirstOccurrence(expression: BinaryExpression, lastPropertyAccessChain: PropertyAccessExpression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { - if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken && expression.operatorToken.pos <= lastPropertyAccessChain.pos) return undefined; + if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; // ensure that each sequential operand in range matches the longest access chain let checkNode = expression.left; let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; @@ -94,7 +94,7 @@ namespace ts.refactor.convertToOptionalChainExpression { (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; if (!checker.isMatchingReference(previousReference, checkNode.right)) { - return undefined; + break; } firstOccurrence = checkNode.right; checkNode = checkNode.left; @@ -106,15 +106,27 @@ namespace ts.refactor.convertToOptionalChainExpression { firstOccurrence = checkNode; } } - return firstOccurrence; + return firstOccurrence !== lastPropertyAccessChain ? firstOccurrence : undefined; } - function getParentForEmptySpanStartToken(start: Node): ValidExpressionOrStatement | undefined { - return findAncestor(start, (node): node is ValidExpressionOrStatement => { - return node.parent && - (isValidExpression(node) && !isValidExpression(node.parent) || - isValidStatement(node) && !isValidStatement(node.parent)); - }); + function getParentNodeContainingSpan(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; + } + + function getParentNodeForEmptySpan(node: Node): ValidExpressionOrStatement | undefined { + while (node.parent) { + if (isValidExpressionOrStatement(node) && !isValidExpressionOrStatement(node.parent)) { + return node; + } + node = node.parent; + } + return undefined; } function getExpression(node: ValidExpressionOrStatement): ValidExpression | undefined { diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts index 229863ef24b74..074a5bcf51230 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts @@ -2,8 +2,10 @@ ////let a = { b: { c: 0 } }; ////let foo; -/////*a*/foo && a && a.b && a.b.c;/*b*/ +////let bar; +////foo && bar && /*a*/a && a.b && a.b.c;/*b*/ +// verify that stop at prefix sequence. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", @@ -12,5 +14,6 @@ edit.applyRefactor({ newContent: `let a = { b: { c: 0 } }; let foo; -foo && a?.b?.c;` +let bar; +foo && bar && a?.b?.c;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts index 95fb3237befeb..3a07b13f740c4 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts @@ -2,8 +2,10 @@ ////let a = { b: { c: 0 } }; ////let foo; -/////*a*/a && a.b && a.b.c/*b*/ && foo; +////let bar; +/////*a*/a && a.b && a.b.c/*b*/ && foo && bar; +// verify that we stop at suffix sequence. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", @@ -12,5 +14,6 @@ edit.applyRefactor({ newContent: `let a = { b: { c: 0 } }; let foo; -a?.b?.c && foo;` +let bar; +a?.b?.c && foo && bar;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts index 9e07f1ca6d2fe..ae6234eeb86c1 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts @@ -1,9 +1,17 @@ /// ////let a = { b: 0 }; -////let x = { b: 0 }; -/////*a*/a && a.b && x && x.y;/*b*/ +////let x = { y: 0 }; +////a && a.b && /*a*/x && x.y;/*b*/ -// We don't currently offer a refactor for this case but should add it in the future. +// Verify that we stop at suffix sequence that is otherwise valid for conversion. goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file +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 && a.b && x?.y;` +}); \ No newline at end of file From 77f47e7f74ad4067af0c24a780d8c94f6782e6d5 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 24 Jun 2020 14:06:17 -0700 Subject: [PATCH 31/46] recurse for call expressions --- src/compiler/checker.ts | 5 ++ src/compiler/types.ts | 1 + .../convertToOptionalChainExpression.ts | 80 +++++++++++-------- ...actorConvertToOptionalChainExpression20.ts | 10 ++- ...actorConvertToOptionalChainExpression21.ts | 14 +++- ...actorConvertToOptionalChainExpression33.ts | 34 ++++++++ 6 files changed, 105 insertions(+), 39 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 5a434193b3609..704d25a752e8f 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -679,6 +679,11 @@ namespace ts { const target = getParseTreeNode(targetIn); return !!node && !!target && isMatchingReference(node, target); }, + isOrContainsMatchingReference: (nodeIn, targetIn) => { + const node = getParseTreeNode(nodeIn); + const target = getParseTreeNode(targetIn); + return !!node && !!target && isOrContainsMatchingReference(node, target); + }, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 7bf22da85d417..fe377f174f3f9 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4121,6 +4121,7 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; /* @internal */ isMatchingReference(node: Node, target: Node): boolean; + /* @internal */ isOrContainsMatchingReference(node: Node, target: Node): boolean; } /* @internal */ diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index adff19741ac36..3ed770c871bbd 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -71,8 +71,7 @@ namespace ts.refactor.convertToOptionalChainExpression { const firstOccurrence = getFirstOccurrence(expression, lastPropertyAccessChain, checker); return firstOccurrence ? { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } - - if (isConditionalExpression(expression)) { + else if (isConditionalExpression(expression)) { const whenTrue = expression.whenTrue; const condition = expression.condition; if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && @@ -90,19 +89,20 @@ namespace ts.refactor.convertToOptionalChainExpression { // ensure that each sequential operand in range matches the longest access chain let checkNode = expression.left; let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; - while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && - (isPropertyAccessExpression(checkNode.right) || isIdentifier(checkNode.right))) { - const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; - if (!checker.isMatchingReference(previousReference, checkNode.right)) { + while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { + const toCheck = isIdentifier(checkNode.right) ? checkNode.right : getRightHandSidePropertyAccess(checkNode.right); + // The right hand side may be a (recursive) call expression, so we need to call getRightHandSidePropertyAccess + const previousReference = isPropertyAccessExpression(firstOccurrence) ? getRightHandSidePropertyAccess(firstOccurrence.expression) : firstOccurrence; + if (!previousReference || !toCheck || !checker.isOrContainsMatchingReference(previousReference, toCheck)) { break; } - firstOccurrence = checkNode.right; + firstOccurrence = toCheck; checkNode = checkNode.left; } // check final LHS node if (isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) { - const previousReference = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.expression : firstOccurrence; - if (isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { + const previousReference = isIdentifier(firstOccurrence) ? firstOccurrence : getRightHandSidePropertyAccess(firstOccurrence.expression); + if (previousReference && isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { firstOccurrence = checkNode; } } @@ -141,47 +141,61 @@ namespace ts.refactor.convertToOptionalChainExpression { return node.expression && isValidExpression(node.expression) ? node.expression : undefined; } - function getRightHandSidePropertyAccess(node: BinaryExpression | CallExpression): PropertyAccessExpression | undefined { + function getRightHandSidePropertyAccess(node: Expression): PropertyAccessExpression | Identifier | undefined { // && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a - // binary expression with an operator if higher precedence. We may want to add recursion for more complex cases like a && a.b()()() == 1; - if (isCallExpression(node) && isPropertyAccessExpression(node.expression)) { + // binary expression with an operator if higher precedence. + if (isPropertyAccessExpression(node) || isIdentifier(node)) { + return node; + } + else if (isCallExpression(node)) { + // cases // a && a.b(); - return node.expression; + // a && a.b()(); + return getRightHandSidePropertyAccess(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)) { - // a && a.b() == 1; - return node.left.expression; - } + // cases + // a && a.b == 1; + // a && a.b() == 1; + return getRightHandSidePropertyAccess(node.left); } return undefined; } function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { - return isBinaryExpression(node) || isCallExpression(node) ? getRightHandSidePropertyAccess(node) - : isPropertyAccessExpression(node) && !isOptionalChain(node) ? node - : undefined; + if (isBinaryExpression(node) || isCallExpression(node)) { + const rhs = getRightHandSidePropertyAccess(node); + return rhs && isPropertyAccessExpression(rhs) ? rhs : undefined; + } + else if (isPropertyAccessExpression(node) && !isOptionalChain(node)) { + return node; + } + return 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); + function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: Identifier | PrivateIdentifier): Expression { + if (isCallExpression(toConvert)) { + const chain = convertToOptionalChain(checker, toConvert.expression, until); + return factory.createCallExpression(chain, toConvert.typeArguments, toConvert.arguments); } - return factory.createPropertyAccessChain(toConvert.expression, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + else if (isPropertyAccessExpression(toConvert) && checker.getSymbolAtLocation(toConvert.name) !== checker.getSymbolAtLocation(until)) { + const chain = convertToOptionalChain(checker, toConvert.expression, until); + return factory.createPropertyAccessChain(chain, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + } + return toConvert; } function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { const { lastPropertyAccessChain, firstOccurrence, expression } = info; const until = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.name : isIdentifier(firstOccurrence) ? firstOccurrence : lastPropertyAccessChain.name; - if (isBinaryExpression(expression)) { - changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertPropertyAccessToOptionalChain(checker, lastPropertyAccessChain, until)); - } - else if (isConditionalExpression(expression)) { - changes.replaceNode(sourceFile, expression, factory.createBinaryExpression(convertPropertyAccessToOptionalChain(checker, lastPropertyAccessChain, until), factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse)); + const convertedChain = convertToOptionalChain(checker, lastPropertyAccessChain, until); + if (convertedChain && isPropertyAccessExpression(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)); + } } } } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts index 0ce3a175da246..51e3743e028fa 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts @@ -3,6 +3,12 @@ ////let a = { b: () => { return { c: 0 } } } /////*a*/a && a.b && a.b().c/*b*/; -// We do not currently offer a refactor for this case but may want to in the future. goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file +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;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts index 225f5c90651c6..9593ffa67ed71 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts @@ -1,8 +1,14 @@ /// -////let a = { b: () => { return () => { } } } -/////*a*/a && a.b()()/*b*/; +////let a = { b: () => { return () => { c: 0 } } } +/////*a*/a && a.b && a.b()().c/*b*/; -// We do not currently offer a refactor for this case but may want to in the future. goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file +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;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts new file mode 100644 index 0000000000000..5a6ebef27fc52 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts @@ -0,0 +1,34 @@ +/// + +////let a = { b: ()=> { +//// return { +//// c: ()=> { +//// return { +//// d: 0 +//// } +//// } +//// } +////}}; +/////*1a*//*2a*/a && a.b && a.b().c/*1b*/ && a.b().c().d;/*2b*/ + +// We should stop at the first call for b since it may not be a pure function. +goTo.select("2a", "2b"); +verify.not.refactorAvailable("Convert to optional chain expression"); + +goTo.select("1a", "1b"); +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: ()=> { + return { + d: 0 + } + } + } +}}; +a?.b()?.c && a.b().c().d;` +}); \ No newline at end of file From 1ba5fd0778219daeb2a29a6d12f9ff8a1fcd26ac Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 24 Jun 2020 14:12:00 -0700 Subject: [PATCH 32/46] fix spellings --- src/services/refactors/convertToOptionalChainExpression.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression13.ts | 2 +- .../fourslash/refactorConvertToOptionalChainExpression14.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 3ed770c871bbd..891ba7c477cd9 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -143,7 +143,7 @@ namespace ts.refactor.convertToOptionalChainExpression { function getRightHandSidePropertyAccess(node: Expression): PropertyAccessExpression | Identifier | undefined { // && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a - // binary expression with an operator if higher precedence. + // binary expression with an operator of higher precedence. if (isPropertyAccessExpression(node) || isIdentifier(node)) { return node; } diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts index 995fd55d15717..65f49e1ce459d 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts @@ -10,6 +10,6 @@ ////declare let foo: Foo; /////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ -// Offer the refactor for ternary expressions if type of baz is not any, null, unknown, or undefined +// Offer the refactor for ternary expressions if type of baz is not null, unknown, or undefined goTo.select("a", "b"); verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts index 7ed6b5513fd5a..9cddab3bfa036 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts @@ -10,7 +10,7 @@ ////declare let foo: Foo; /////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ -// It is reasonable to offer a refacor when baz is of type any since using any in strict mode +// It is reasonable to offer a refactor when baz is of type any since using any in strict mode // produces an error and those with strict mode off aren't getting null checks anyway. goTo.select("a", "b"); verify.refactorAvailable("Convert to optional chain expression"); From 6095e9501fa0c093a8b92cf6a846c5b284a831f8 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 25 Jun 2020 13:57:33 -0700 Subject: [PATCH 33/46] add recursive cases --- .../convertToOptionalChainExpression.ts | 85 +++++++++++-------- ...actorConvertToOptionalChainExpression19.ts | 10 ++- ...actorConvertToOptionalChainExpression34.ts | 15 ++++ 3 files changed, 74 insertions(+), 36 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 891ba7c477cd9..f885d0e01551d 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -27,7 +27,7 @@ namespace ts.refactor.convertToOptionalChainExpression { interface Info { lastPropertyAccessChain: PropertyAccessExpression, - firstOccurrence: Node, + firstOccurrence: PropertyAccessExpression | Identifier, expression: BinaryExpression | ConditionalExpression } @@ -64,49 +64,66 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!expression) return undefined; const checker = program.getTypeChecker(); + return getExpressionInfo(expression, checker); + } - if (isBinaryExpression(expression)) { - const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); - if (!lastPropertyAccessChain) return undefined; - const firstOccurrence = getFirstOccurrence(expression, lastPropertyAccessChain, checker); - return firstOccurrence ? { lastPropertyAccessChain, firstOccurrence, expression } : undefined; - } - else if (isConditionalExpression(expression)) { - const whenTrue = expression.whenTrue; - const condition = expression.condition; - if ((isIdentifier(condition) || isPropertyAccessExpression(condition)) && - isPropertyAccessExpression(whenTrue) && checker.isMatchingReference(whenTrue.expression, condition)) { - // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor - const type = checker.getTypeAtLocation(whenTrue.name); - return !checker.isNullableType(type) ? { lastPropertyAccessChain: whenTrue, firstOccurrence: condition, expression } : undefined; - } + function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker) { + const whenTrue = expression.whenTrue; + const condition = expression.condition; + if((isIdentifier(condition) || isPropertyAccessExpression(condition)) && + isPropertyAccessExpression(whenTrue) && checker.isMatchingReference(whenTrue.expression, condition)) { + // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor + const type = checker.getTypeAtLocation(whenTrue.name); + return !checker.isNullableType(type) ? { lastPropertyAccessChain: whenTrue, firstOccurrence: condition, expression } : undefined; } return undefined; } - function getFirstOccurrence(expression: BinaryExpression, lastPropertyAccessChain: PropertyAccessExpression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { - if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; - // ensure that each sequential operand in range matches the longest access chain - let checkNode = expression.left; - let firstOccurrence: PropertyAccessExpression | Identifier = lastPropertyAccessChain; - while (isBinaryExpression(checkNode) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const toCheck = isIdentifier(checkNode.right) ? checkNode.right : getRightHandSidePropertyAccess(checkNode.right); - // The right hand side may be a (recursive) call expression, so we need to call getRightHandSidePropertyAccess - const previousReference = isPropertyAccessExpression(firstOccurrence) ? getRightHandSidePropertyAccess(firstOccurrence.expression) : firstOccurrence; - if (!previousReference || !toCheck || !checker.isOrContainsMatchingReference(previousReference, toCheck)) { + function getExpressionInfo(expression: ValidExpression, checker: TypeChecker): Info | undefined { + // todo need to do the nullcheck for conditional here too + if (isConditionalExpression(expression) && !isBinaryExpression(expression.condition)) { + return getConditionalInfo(expression, checker); + } + if (isBinaryExpression(expression) && expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) { + return undefined; + } + const lastPropertyAccessChain = isConditionalExpression(expression) ? + getLastPropertyAccessChain(expression.whenTrue) : getLastPropertyAccessChain(expression.right); + if (!lastPropertyAccessChain) return undefined; + + let checkNode = isConditionalExpression(expression) ? expression.condition : expression.left; + let firstOccurrence: Expression = lastPropertyAccessChain; + let toCheck: Expression = lastPropertyAccessChain; + while (isBinaryExpression(checkNode) && toCheck && (isPropertyAccessExpression(toCheck) || isCallExpression(toCheck)) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { + const match = checkMatch(checkNode.right, toCheck.expression, checker); + if (!match) { break; } - firstOccurrence = toCheck; + toCheck = match; + firstOccurrence = checkNode.right; checkNode = checkNode.left; } - // check final LHS node - if (isIdentifier(checkNode) || isPropertyAccessExpression(checkNode)) { - const previousReference = isIdentifier(firstOccurrence) ? firstOccurrence : getRightHandSidePropertyAccess(firstOccurrence.expression); - if (previousReference && isPropertyAccessExpression(firstOccurrence) && checker.isMatchingReference(previousReference, checkNode)) { - firstOccurrence = checkNode; - } + const lastMatch = isCallExpression(toCheck) || isPropertyAccessExpression(toCheck) ? checkMatch(checkNode, toCheck.expression, checker) : undefined; + if (lastMatch) { + firstOccurrence = checkNode; } - return firstOccurrence !== lastPropertyAccessChain ? firstOccurrence : undefined; + return firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? + { lastPropertyAccessChain, firstOccurrence, expression } : undefined; + } + + function checkMatch(expression: Expression, target: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + if (isCallExpression(target)) { + // Recurse through the call expressions to match a.b to a.b()(). + return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; + } + else if (isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) { + // we shouldn't offer a refactor for a.b && a.b().c && a.b.c().d so we need to check that the entire access chain matches. + return checker.isOrContainsMatchingReference(expression, target) ? expression : undefined; + } + else if (isIdentifier(expression) && isIdentifier(target)) { + return checker.isMatchingReference(expression, target) ? expression : undefined; + } + return undefined; } function getParentNodeContainingSpan(node: Node, span: TextSpan): ValidExpressionOrStatement | undefined { diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts index 94d8a9979f781..d885b544cc1e3 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts @@ -3,6 +3,12 @@ ////let a = { b: { c: 0 } }; /////*a*/a && a.b ? a.b.c : "whenFalse";/*b*/ -// We do not combine condition and whenTrue to an optional chain but should in the future. goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); \ No newline at end of file +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";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts new file mode 100644 index 0000000000000..24a072f572ceb --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts @@ -0,0 +1,15 @@ +/// + +////let a = { +//// b: () => { +//// return { +//// c: () => { +//// return { d: 0 }; +//// } +//// }; +//// } +////} +/////*a*/a && a.b() && a.b.c;/*b*/ + +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression"); From 89c8c9d98ba8441b077541467d983c82bcf28ba1 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 25 Jun 2020 20:43:07 -0700 Subject: [PATCH 34/46] remove isOrContainsMatchingReference --- src/compiler/checker.ts | 5 ----- src/compiler/types.ts | 1 - .../refactors/convertToOptionalChainExpression.ts | 8 +++----- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 704d25a752e8f..5a434193b3609 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -679,11 +679,6 @@ namespace ts { const target = getParseTreeNode(targetIn); return !!node && !!target && isMatchingReference(node, target); }, - isOrContainsMatchingReference: (nodeIn, targetIn) => { - const node = getParseTreeNode(nodeIn); - const target = getParseTreeNode(targetIn); - return !!node && !!target && isOrContainsMatchingReference(node, target); - }, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index fe377f174f3f9..7bf22da85d417 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4121,7 +4121,6 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; /* @internal */ isMatchingReference(node: Node, target: Node): boolean; - /* @internal */ isOrContainsMatchingReference(node: Node, target: Node): boolean; } /* @internal */ diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index f885d0e01551d..c020d26d9f63e 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -67,7 +67,7 @@ namespace ts.refactor.convertToOptionalChainExpression { return getExpressionInfo(expression, checker); } - function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker) { + function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { const whenTrue = expression.whenTrue; const condition = expression.condition; if((isIdentifier(condition) || isPropertyAccessExpression(condition)) && @@ -116,11 +116,9 @@ namespace ts.refactor.convertToOptionalChainExpression { // Recurse through the call expressions to match a.b to a.b()(). return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; } - else if (isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) { + else if ((isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) || + (isIdentifier(expression) && isIdentifier(target))) { // we shouldn't offer a refactor for a.b && a.b().c && a.b.c().d so we need to check that the entire access chain matches. - return checker.isOrContainsMatchingReference(expression, target) ? expression : undefined; - } - else if (isIdentifier(expression) && isIdentifier(target)) { return checker.isMatchingReference(expression, target) ? expression : undefined; } return undefined; From b20cd9528dc84fa97dcf03d5ecf84f7bb2af65ff Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 25 Jun 2020 21:04:38 -0700 Subject: [PATCH 35/46] cleanup --- .../convertToOptionalChainExpression.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index c020d26d9f63e..f9759dc87563c 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -94,7 +94,8 @@ namespace ts.refactor.convertToOptionalChainExpression { let checkNode = isConditionalExpression(expression) ? expression.condition : expression.left; let firstOccurrence: Expression = lastPropertyAccessChain; let toCheck: Expression = lastPropertyAccessChain; - while (isBinaryExpression(checkNode) && toCheck && (isPropertyAccessExpression(toCheck) || isCallExpression(toCheck)) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { + while (isBinaryExpression(checkNode) && toCheck && (isPropertyAccessExpression(toCheck) || + isCallExpression(toCheck)) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { const match = checkMatch(checkNode.right, toCheck.expression, checker); if (!match) { break; @@ -111,7 +112,7 @@ namespace ts.refactor.convertToOptionalChainExpression { { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } - function checkMatch(expression: Expression, target: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + function checkMatch(expression: Expression | Identifier | PrivateIdentifier, target: Expression | Identifier | PrivateIdentifier, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { if (isCallExpression(target)) { // Recurse through the call expressions to match a.b to a.b()(). return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; @@ -188,12 +189,12 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } - function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: Identifier | PrivateIdentifier): Expression { + function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: PropertyAccessExpression | Identifier | PrivateIdentifier): Expression { if (isCallExpression(toConvert)) { const chain = convertToOptionalChain(checker, toConvert.expression, until); return factory.createCallExpression(chain, toConvert.typeArguments, toConvert.arguments); } - else if (isPropertyAccessExpression(toConvert) && checker.getSymbolAtLocation(toConvert.name) !== checker.getSymbolAtLocation(until)) { + else if (isPropertyAccessExpression(toConvert) && !checkMatch(toConvert, until, checker)) { const chain = convertToOptionalChain(checker, toConvert.expression, until); return factory.createPropertyAccessChain(chain, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); } @@ -202,14 +203,15 @@ namespace ts.refactor.convertToOptionalChainExpression { function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { const { lastPropertyAccessChain, firstOccurrence, expression } = info; - const until = isPropertyAccessExpression(firstOccurrence) ? firstOccurrence.name : isIdentifier(firstOccurrence) ? firstOccurrence : lastPropertyAccessChain.name; - const convertedChain = convertToOptionalChain(checker, lastPropertyAccessChain, until); + const convertedChain = convertToOptionalChain(checker, lastPropertyAccessChain, firstOccurrence); if (convertedChain && isPropertyAccessExpression(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)); + changes.replaceNode(sourceFile, expression, + factory.createBinaryExpression(convertedChain, factory.createToken(SyntaxKind.QuestionQuestionToken), expression.whenFalse) + ); } } } From 15176b265ba17db5082653f42b3701d4a0cc4478 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 29 Jun 2020 14:02:43 -0700 Subject: [PATCH 36/46] more refactoring --- .../convertToOptionalChainExpression.ts | 121 +++++++++++------- ...actorConvertToOptionalChainExpression35.ts | 26 ++++ ...actorConvertToOptionalChainExpression36.ts | 15 +++ ...actorConvertToOptionalChainExpression37.ts | 7 + 4 files changed, 120 insertions(+), 49 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index f9759dc87563c..e89ba9533fb8f 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -28,11 +28,19 @@ namespace ts.refactor.convertToOptionalChainExpression { interface Info { lastPropertyAccessChain: PropertyAccessExpression, firstOccurrence: PropertyAccessExpression | Identifier, - expression: BinaryExpression | ConditionalExpression + 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 { @@ -59,60 +67,61 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 ? getParentNodeForEmptySpan(startToken) : getParentNodeContainingSpan(startToken, adjustedSpan); + 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 getExpressionInfo(expression, checker); + return isConditionalExpression(expression) ? getConditionalInfo(expression, checker) : getBinaryInfo(expression, checker); } function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { - const whenTrue = expression.whenTrue; const condition = expression.condition; - if((isIdentifier(condition) || isPropertyAccessExpression(condition)) && - isPropertyAccessExpression(whenTrue) && checker.isMatchingReference(whenTrue.expression, condition)) { - // The ternary expression and nullish coalescing would result in different return values if c is nullish so do not offer a refactor - const type = checker.getTypeAtLocation(whenTrue.name); - return !checker.isNullableType(type) ? { lastPropertyAccessChain: whenTrue, firstOccurrence: condition, expression } : undefined; + const lastPropertyAccessChain = getLastPropertyAccessChain(expression.whenTrue); + if (lastPropertyAccessChain && !checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) { + if (isBinaryExpression(condition)) { + const firstOccurrence = getFirstOccurrenceInBinary(condition, lastPropertyAccessChain.expression, checker); + return firstOccurrence && firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? + { lastPropertyAccessChain, firstOccurrence, expression } : undefined; + } + else if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) && checkMatch(condition, lastPropertyAccessChain.expression, checker)) { + return { firstOccurrence: condition, lastPropertyAccessChain, expression }; + } } return undefined; } - function getExpressionInfo(expression: ValidExpression, checker: TypeChecker): Info | undefined { - // todo need to do the nullcheck for conditional here too - if (isConditionalExpression(expression) && !isBinaryExpression(expression.condition)) { - return getConditionalInfo(expression, checker); - } - if (isBinaryExpression(expression) && expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) { - return undefined; - } - const lastPropertyAccessChain = isConditionalExpression(expression) ? - getLastPropertyAccessChain(expression.whenTrue) : getLastPropertyAccessChain(expression.right); + function getBinaryInfo(expression: BinaryExpression, checker: TypeChecker): Info | undefined { + if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; + const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); if (!lastPropertyAccessChain) return undefined; - let checkNode = isConditionalExpression(expression) ? expression.condition : expression.left; - let firstOccurrence: Expression = lastPropertyAccessChain; - let toCheck: Expression = lastPropertyAccessChain; - while (isBinaryExpression(checkNode) && toCheck && (isPropertyAccessExpression(toCheck) || - isCallExpression(toCheck)) && checkNode.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const match = checkMatch(checkNode.right, toCheck.expression, checker); - if (!match) { - break; - } - toCheck = match; - firstOccurrence = checkNode.right; - checkNode = checkNode.left; + const firstOccurrence = getFirstOccurrenceInBinary(expression.left, lastPropertyAccessChain.expression, checker); + return firstOccurrence && firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? + { lastPropertyAccessChain, firstOccurrence, expression } : undefined; + + } + + /** + * Gets the earliest expression or identifier in an && chain that matches the target access chain. + * e.g. a && a.b is searched with target a.b and returns a. + */ + function getFirstOccurrenceInBinary(expression: Expression, target: Expression | Identifier, checker: TypeChecker): Expression | Identifier | undefined { + if (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { + const match = getFirstOccurrenceInBinary(expression.right, target, checker); + if (!match) return undefined; + return isPropertyAccessExpression(match) ? getFirstOccurrenceInBinary(expression.left, match.expression, checker) ?? expression.right : expression.right; } - const lastMatch = isCallExpression(toCheck) || isPropertyAccessExpression(toCheck) ? checkMatch(checkNode, toCheck.expression, checker) : undefined; - if (lastMatch) { - firstOccurrence = checkNode; + else { + const match = checkMatch(expression, target, checker); + return match ? expression : undefined; } - return firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? - { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } - function checkMatch(expression: Expression | Identifier | PrivateIdentifier, target: Expression | Identifier | PrivateIdentifier, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + /** + * Checks that expression matches the final identifier in a property access expression or call expression. + */ + function checkMatch(expression: Expression | Identifier, target: Expression | Identifier, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { if (isCallExpression(target)) { // Recurse through the call expressions to match a.b to a.b()(). return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; @@ -120,12 +129,16 @@ namespace ts.refactor.convertToOptionalChainExpression { else if ((isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) || (isIdentifier(expression) && isIdentifier(target))) { // we shouldn't offer a refactor for a.b && a.b().c && a.b.c().d so we need to check that the entire access chain matches. - return checker.isMatchingReference(expression, target) ? expression : undefined; + const symbol = checker.getSymbolAtLocation(expression); + return symbol && !checker.isUnknownSymbol(symbol) && checker.isMatchingReference(expression, target) ? target : undefined; } return undefined; } - function getParentNodeContainingSpan(node: Node, span: TextSpan): ValidExpressionOrStatement | undefined { + /** + * 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; @@ -135,7 +148,10 @@ namespace ts.refactor.convertToOptionalChainExpression { return undefined; } - function getParentNodeForEmptySpan(node: Node): ValidExpressionOrStatement | 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; @@ -145,6 +161,9 @@ namespace ts.refactor.convertToOptionalChainExpression { 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; @@ -157,39 +176,43 @@ namespace ts.refactor.convertToOptionalChainExpression { return node.expression && isValidExpression(node.expression) ? node.expression : undefined; } + /** + * && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a + * binary expression with an operator of higher precedence. + */ function getRightHandSidePropertyAccess(node: Expression): PropertyAccessExpression | Identifier | undefined { - // && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a - // binary expression with an operator of higher precedence. if (isPropertyAccessExpression(node) || isIdentifier(node)) { return node; } else if (isCallExpression(node)) { - // cases - // a && a.b(); // a && a.b()(); return getRightHandSidePropertyAccess(node.expression); } else if (isBinaryExpression(node)) { - // cases - // a && a.b == 1; - // a && a.b() == 1; + // a && a.b == 1 is valid but a && 1 === a.b is not return getRightHandSidePropertyAccess(node.left); } return undefined; } + /** + * Gets the right-most property access expression + */ function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { if (isBinaryExpression(node) || isCallExpression(node)) { const rhs = getRightHandSidePropertyAccess(node); return rhs && isPropertyAccessExpression(rhs) ? rhs : undefined; } - else if (isPropertyAccessExpression(node) && !isOptionalChain(node)) { + else if (isPropertyAccessExpression(node) && !isOptionalChain(node) /* don't include already converted chains */) { return node; } return undefined; } - function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: PropertyAccessExpression | Identifier | PrivateIdentifier): Expression { + /** + * Converts a property access chain to an optional property access chain. + */ + function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: PropertyAccessExpression | Identifier): Expression { if (isCallExpression(toConvert)) { const chain = convertToOptionalChain(checker, toConvert.expression, until); return factory.createCallExpression(chain, toConvert.typeArguments, toConvert.arguments); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts new file mode 100644 index 0000000000000..953f44a51bfc4 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts @@ -0,0 +1,26 @@ +/// + +// @strict: true + +////interface Foo { +//// bar:{ +//// baz: string; +//// } +////} +////declare let foo: Foo; +/////*a*/foo && foo.bar ? foo.bar.baz : "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: +`interface Foo { + bar:{ + baz: string; + } +} +declare let foo: Foo; +foo?.bar?.baz ?? "whenFalse";` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts new file mode 100644 index 0000000000000..e1c068e281003 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts @@ -0,0 +1,15 @@ +/// + +// @strict: true + +////interface Foo { +//// bar:{ +//// baz: string | null; +//// } +////} +////declare let foo: Foo; +/////*a*/foo && foo.bar ? foo.bar.baz : "whenFalse";/*b*/ + +// Do not offer refactor when true condition can be null. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression") \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts new file mode 100644 index 0000000000000..472b8a22b3d75 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts @@ -0,0 +1,7 @@ +/// + +/////*a*/foo && foo.bar;/*b*/ + +// Do not offer refactor for unknown symbols. +goTo.select("a", "b"); +verify.not.refactorAvailable("Convert to optional chain expression") \ No newline at end of file From 2de5e283e2ab274369d3cccf94f3c76e20b2d323 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 29 Jun 2020 14:50:30 -0700 Subject: [PATCH 37/46] cleanup --- .../convertToOptionalChainExpression.ts | 64 +++++++------------ 1 file changed, 22 insertions(+), 42 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index e89ba9533fb8f..7866922447418 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -78,17 +78,15 @@ namespace ts.refactor.convertToOptionalChainExpression { function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { const condition = expression.condition; const lastPropertyAccessChain = getLastPropertyAccessChain(expression.whenTrue); - if (lastPropertyAccessChain && !checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) { - if (isBinaryExpression(condition)) { - const firstOccurrence = getFirstOccurrenceInBinary(condition, lastPropertyAccessChain.expression, checker); - return firstOccurrence && firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? - { lastPropertyAccessChain, firstOccurrence, expression } : undefined; - } - else if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) && checkMatch(condition, lastPropertyAccessChain.expression, checker)) { - return { firstOccurrence: condition, lastPropertyAccessChain, expression }; - } - } - return undefined; + if (!lastPropertyAccessChain || checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) return undefined; + + const firstOccurrence = (isPropertyAccessExpression(condition) || isIdentifier(condition)) + && checkMatch(condition, lastPropertyAccessChain.expression, checker) ? + condition : isBinaryExpression(condition) ? + getFirstOccurrenceInExpression(condition, lastPropertyAccessChain.expression, checker): undefined; + + return firstOccurrence && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? + { lastPropertyAccessChain, firstOccurrence, expression } : undefined; } function getBinaryInfo(expression: BinaryExpression, checker: TypeChecker): Info | undefined { @@ -96,21 +94,21 @@ namespace ts.refactor.convertToOptionalChainExpression { const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); if (!lastPropertyAccessChain) return undefined; - const firstOccurrence = getFirstOccurrenceInBinary(expression.left, lastPropertyAccessChain.expression, checker); - return firstOccurrence && firstOccurrence !== lastPropertyAccessChain && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? + const firstOccurrence = getFirstOccurrenceInExpression(expression.left, lastPropertyAccessChain.expression, checker); + return firstOccurrence && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? { lastPropertyAccessChain, firstOccurrence, expression } : undefined; - } /** * Gets the earliest expression or identifier in an && chain that matches the target access chain. * e.g. a && a.b is searched with target a.b and returns a. */ - function getFirstOccurrenceInBinary(expression: Expression, target: Expression | Identifier, checker: TypeChecker): Expression | Identifier | undefined { + function getFirstOccurrenceInExpression(expression: Expression, target: Expression, checker: TypeChecker): Expression | Identifier | undefined { if (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const match = getFirstOccurrenceInBinary(expression.right, target, checker); + const match = getFirstOccurrenceInExpression(expression.right, target, checker); if (!match) return undefined; - return isPropertyAccessExpression(match) ? getFirstOccurrenceInBinary(expression.left, match.expression, checker) ?? expression.right : expression.right; + return isPropertyAccessExpression(match) ? + getFirstOccurrenceInExpression(expression.left, match.expression, checker) ?? expression.right : expression.right; } else { const match = checkMatch(expression, target, checker); @@ -121,14 +119,13 @@ namespace ts.refactor.convertToOptionalChainExpression { /** * Checks that expression matches the final identifier in a property access expression or call expression. */ - function checkMatch(expression: Expression | Identifier, target: Expression | Identifier, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + function checkMatch(expression: Expression, target: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { if (isCallExpression(target)) { // Recurse through the call expressions to match a.b to a.b()(). return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; } else if ((isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) || (isIdentifier(expression) && isIdentifier(target))) { - // we shouldn't offer a refactor for a.b && a.b().c && a.b.c().d so we need to check that the entire access chain matches. const symbol = checker.getSymbolAtLocation(expression); return symbol && !checker.isUnknownSymbol(symbol) && checker.isMatchingReference(expression, target) ? target : undefined; } @@ -176,34 +173,17 @@ namespace ts.refactor.convertToOptionalChainExpression { return node.expression && isValidExpression(node.expression) ? node.expression : undefined; } - /** - * && binary expressions are left-heavy so for most cases we just need to check if the last property access chain is on the RHS of a - * binary expression with an operator of higher precedence. - */ - function getRightHandSidePropertyAccess(node: Expression): PropertyAccessExpression | Identifier | undefined { - if (isPropertyAccessExpression(node) || isIdentifier(node)) { - return node; - } - else if (isCallExpression(node)) { - // a && a.b()(); - return getRightHandSidePropertyAccess(node.expression); - } - else if (isBinaryExpression(node)) { - // a && a.b == 1 is valid but a && 1 === a.b is not - return getRightHandSidePropertyAccess(node.left); - } - return undefined; - } - /** * Gets the right-most property access expression */ function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { - if (isBinaryExpression(node) || isCallExpression(node)) { - const rhs = getRightHandSidePropertyAccess(node); - return rhs && isPropertyAccessExpression(rhs) ? rhs : undefined; + if (isBinaryExpression(node)) { + return getLastPropertyAccessChain(node.left); + } + else if (isCallExpression(node)) { + return getLastPropertyAccessChain(node.expression); } - else if (isPropertyAccessExpression(node) && !isOptionalChain(node) /* don't include already converted chains */) { + else if (isPropertyAccessExpression(node) && !isOptionalChain(node)) { return node; } return undefined; From 4126a46749de13fc3884a7d67ee0d64e972d25f6 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 29 Jun 2020 15:18:49 -0700 Subject: [PATCH 38/46] rename tests --- .../refactorConvertToOptionalChainExpression12.ts | 14 -------------- .../refactorConvertToOptionalChainExpression19.ts | 14 -------------- .../refactorConvertToOptionalChainExpression28.ts | 14 -------------- ...alChainExpression_AccessCallCallReturnValue.ts} | 0 ...tionalChainExpression_AccessCallReturnValue.ts} | 0 ...tToOptionalChainExpression_BinaryExpression.ts} | 0 ...ChainExpression_BinaryExpressionPartialSpan.ts} | 0 ...nalChainExpression_BinaryWithCallExpression.ts} | 0 ...nalChainExpression_CallExpressionComparison.ts} | 0 ...oOptionalChainExpression_ComparisonOperator.ts} | 0 ...ToOptionalChainExpression_ConditionalForAny.ts} | 0 ...hainExpression_ConditionalInitialIdentifier.ts} | 0 ...ptionalChainExpression_ConditionalNoNullish.ts} | 0 ...ionalChainExpression_ConditionalPartialSPan.ts} | 0 ...tionalChainExpression_ConditionalStrictMode.ts} | 0 ...nExpression_ConditionalWithBinaryCondition1.ts} | 0 ...nExpression_ConditionalWithBinaryCondition2.ts} | 0 ...ion_ConditionalWithBinaryConditionNoNullish.ts} | 0 ...alChainExpression_EmptySpanBinaryExpression.ts} | 0 ...inExpression_EmptySpanBinaryReturnStatement.ts} | 0 ...tionalChainExpression_EmptySpanCallArgument.ts} | 0 ...ptionalChainExpression_EmptySpanConditional.ts} | 0 ...xpression_EmptySpanConditionalReturnKeyword.ts} | 0 ...ression_EmptySpanConditionalReturnStatement.ts} | 0 ...OptionalChainExpression_EmptySpanVarKeyword.ts} | 0 ...Expression_EmptySpanVariableStatementBinary.ts} | 0 ...ssion_EmptySpanVariableStatementConditional.ts} | 0 ...ainExpression_ExpressionStatementValidSpans.ts} | 0 ...ertToOptionalChainExpression_InFunctionCall.ts} | 0 ...vertToOptionalChainExpression_InIfStatement.ts} | 0 ...OptionalChainExpression_NoInitialIdentifier.ts} | 0 ...ertToOptionalChainExpression_NoPreviousCall.ts} | 0 ...vertToOptionalChainExpression_NoRepeatCalls.ts} | 0 ...OptionalChainExpression_NotForOptionalChain.ts} | 0 ...ptionalChainExpression_NotForOtherOperators.ts} | 0 ...nalChainExpression_NotForOutOfOrderSequence.ts} | 0 ...vertToOptionalChainExpression_NotForUnknown.ts} | 0 ...ToOptionalChainExpression_OptionalInterface.ts} | 0 ...tionalChainExpression_ReturnStatementBinary.ts} | 0 ...lChainExpression_ReturnStatementConditional.ts} | 0 ...alChainExpression_ReturnStatementValidSpans.ts} | 0 ...ptionalChainExpression_SemicolonNotSelected.ts} | 0 ...pression_SubexpressionWithConvertiblePrefix.ts} | 0 ...pression_SubexpressionWithConvertibleSuffix.ts} | 0 ...onalChainExpression_SubexpressionWithPrefix.ts} | 0 ...onalChainExpression_SubexpressionWithSuffix.ts} | 0 46 files changed, 42 deletions(-) delete mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts delete mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts delete mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression21.ts => refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression20.ts => refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression1.ts => refactorConvertToOptionalChainExpression_BinaryExpression.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression30.ts => refactorConvertToOptionalChainExpression_BinaryExpressionPartialSpan.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression2.ts => refactorConvertToOptionalChainExpression_BinaryWithCallExpression.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression4.ts => refactorConvertToOptionalChainExpression_CallExpressionComparison.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression3.ts => refactorConvertToOptionalChainExpression_ComparisonOperator.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression14.ts => refactorConvertToOptionalChainExpression_ConditionalForAny.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression11.ts => refactorConvertToOptionalChainExpression_ConditionalInitialIdentifier.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression15.ts => refactorConvertToOptionalChainExpression_ConditionalNoNullish.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression31.ts => refactorConvertToOptionalChainExpression_ConditionalPartialSPan.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression13.ts => refactorConvertToOptionalChainExpression_ConditionalStrictMode.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression18.ts => refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition1.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression35.ts => refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition2.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression36.ts => refactorConvertToOptionalChainExpression_ConditionalWithBinaryConditionNoNullish.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason1.ts => refactorConvertToOptionalChainExpression_EmptySpanBinaryExpression.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason3.ts => refactorConvertToOptionalChainExpression_EmptySpanBinaryReturnStatement.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason9.ts => refactorConvertToOptionalChainExpression_EmptySpanCallArgument.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason2.ts => refactorConvertToOptionalChainExpression_EmptySpanConditional.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason5.ts => refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnKeyword.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason4.ts => refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnStatement.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason8.ts => refactorConvertToOptionalChainExpression_EmptySpanVarKeyword.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason6.ts => refactorConvertToOptionalChainExpression_EmptySpanVariableStatementBinary.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpressionForTriggerReason7.ts => refactorConvertToOptionalChainExpression_EmptySpanVariableStatementConditional.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression29.ts => refactorConvertToOptionalChainExpression_ExpressionStatementValidSpans.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression32.ts => refactorConvertToOptionalChainExpression_InFunctionCall.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression5.ts => refactorConvertToOptionalChainExpression_InIfStatement.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression8.ts => refactorConvertToOptionalChainExpression_NoInitialIdentifier.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression34.ts => refactorConvertToOptionalChainExpression_NoPreviousCall.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression33.ts => refactorConvertToOptionalChainExpression_NoRepeatCalls.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression10.ts => refactorConvertToOptionalChainExpression_NotForOptionalChain.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression9.ts => refactorConvertToOptionalChainExpression_NotForOtherOperators.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression22.ts => refactorConvertToOptionalChainExpression_NotForOutOfOrderSequence.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression37.ts => refactorConvertToOptionalChainExpression_NotForUnknown.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression6.ts => refactorConvertToOptionalChainExpression_OptionalInterface.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression24.ts => refactorConvertToOptionalChainExpression_ReturnStatementBinary.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression25.ts => refactorConvertToOptionalChainExpression_ReturnStatementConditional.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression26.ts => refactorConvertToOptionalChainExpression_ReturnStatementValidSpans.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression27.ts => refactorConvertToOptionalChainExpression_SemicolonNotSelected.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression23.ts => refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression7.ts => refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression16.ts => refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts} (100%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression17.ts => refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts} (100%) diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts deleted file mode 100644 index 85e6fec971c63..0000000000000 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression12.ts +++ /dev/null @@ -1,14 +0,0 @@ -/// - -////let a = { b: { c: 0 } }; -/////*a*/a.b ? a.b.c : "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: { c: 0 } }; -a.b?.c ?? "whenFalse";` -}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts deleted file mode 100644 index d885b544cc1e3..0000000000000 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression19.ts +++ /dev/null @@ -1,14 +0,0 @@ -/// - -////let a = { b: { c: 0 } }; -/////*a*/a && a.b ? a.b.c : "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: { c: 0 } }; -a?.b?.c ?? "whenFalse";` -}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts deleted file mode 100644 index 97ff17e680ee5..0000000000000 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression28.ts +++ /dev/null @@ -1,14 +0,0 @@ -/// - -////let a = { b: { c: 0 } }; -////let x = /*a*/a.b ? a.b.c : "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: { c: 0 } }; -let x = a.b?.c ?? "whenFalse";` -}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression21.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression20.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryExpression.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression1.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryExpression.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryExpressionPartialSpan.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression30.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryExpressionPartialSpan.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryWithCallExpression.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression2.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_BinaryWithCallExpression.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_CallExpressionComparison.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression4.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_CallExpressionComparison.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ComparisonOperator.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression3.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ComparisonOperator.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression14.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalInitialIdentifier.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression11.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalInitialIdentifier.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalNoNullish.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression15.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalNoNullish.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalPartialSPan.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression31.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalPartialSPan.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalStrictMode.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression13.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalStrictMode.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition1.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression18.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition1.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition2.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression35.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryCondition2.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryConditionNoNullish.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression36.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalWithBinaryConditionNoNullish.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanBinaryExpression.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason1.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanBinaryExpression.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanBinaryReturnStatement.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason3.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanBinaryReturnStatement.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanCallArgument.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason9.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanCallArgument.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditional.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason2.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditional.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnKeyword.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason5.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnKeyword.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnStatement.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason4.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanConditionalReturnStatement.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVarKeyword.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason8.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVarKeyword.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVariableStatementBinary.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason6.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVariableStatementBinary.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVariableStatementConditional.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpressionForTriggerReason7.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_EmptySpanVariableStatementConditional.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ExpressionStatementValidSpans.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression29.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ExpressionStatementValidSpans.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_InFunctionCall.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression32.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_InFunctionCall.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_InIfStatement.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression5.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_InIfStatement.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoInitialIdentifier.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression8.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoInitialIdentifier.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoPreviousCall.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression34.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoPreviousCall.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression33.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOptionalChain.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression10.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOptionalChain.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOtherOperators.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression9.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOtherOperators.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOutOfOrderSequence.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression22.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForOutOfOrderSequence.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression37.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_OptionalInterface.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression6.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_OptionalInterface.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementBinary.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression24.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementBinary.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementConditional.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression25.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementConditional.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementValidSpans.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression26.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_ReturnStatementValidSpans.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SemicolonNotSelected.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression27.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SemicolonNotSelected.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression23.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression7.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression16.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts similarity index 100% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression17.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts From 8f65c0240795ed7ec04ff6ac3b69cb29cf44cc00 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Wed, 1 Jul 2020 12:40:10 -0700 Subject: [PATCH 39/46] address PR comments --- src/compiler/checker.ts | 5 + src/compiler/types.ts | 1 + .../convertToOptionalChainExpression.ts | 103 ++++++++++-------- ...ainExpression_AccessCallCallReturnValue.ts | 2 +- ...alChainExpression_AccessCallReturnValue.ts | 2 +- ...ToOptionalChainExpression_NoRepeatCalls.ts | 2 +- ...ToOptionalChainExpression_NotForUnknown.ts | 9 +- ...tToOptionalChainExpression_SparseAccess.ts | 15 +++ ...ainExpression_SubexpressionWithPrefix1.ts} | 2 +- ...ainExpression_SubexpressionWithSuffix1.ts} | 2 +- ...ainExpression_SubexpressionWithSuffix2.ts} | 1 + ...inExpression_SubexpressionsWithPrefix2.ts} | 2 +- 12 files changed, 95 insertions(+), 51 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression_SparseAccess.ts rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts => refactorConvertToOptionalChainExpression_SubexpressionWithPrefix1.ts} (86%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts => refactorConvertToOptionalChainExpression_SubexpressionWithSuffix1.ts} (86%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts => refactorConvertToOptionalChainExpression_SubexpressionWithSuffix2.ts} (83%) rename tests/cases/fourslash/{refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts => refactorConvertToOptionalChainExpression_SubexpressionsWithPrefix2.ts} (80%) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 5a434193b3609..704d25a752e8f 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -679,6 +679,11 @@ namespace ts { const target = getParseTreeNode(targetIn); return !!node && !!target && isMatchingReference(node, target); }, + isOrContainsMatchingReference: (nodeIn, targetIn) => { + const node = getParseTreeNode(nodeIn); + const target = getParseTreeNode(targetIn); + return !!node && !!target && isOrContainsMatchingReference(node, target); + }, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 7bf22da85d417..fe377f174f3f9 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4121,6 +4121,7 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; /* @internal */ isMatchingReference(node: Node, target: Node): boolean; + /* @internal */ isOrContainsMatchingReference(node: Node, target: Node): boolean; } /* @internal */ diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 7866922447418..b7f1ff96e2bb3 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -27,7 +27,7 @@ namespace ts.refactor.convertToOptionalChainExpression { interface Info { lastPropertyAccessChain: PropertyAccessExpression, - firstOccurrence: PropertyAccessExpression | Identifier, + occurrences: (PropertyAccessExpression | Identifier)[], expression: ValidExpression } @@ -78,62 +78,70 @@ namespace ts.refactor.convertToOptionalChainExpression { function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { const condition = expression.condition; const lastPropertyAccessChain = getLastPropertyAccessChain(expression.whenTrue); - if (!lastPropertyAccessChain || checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) return undefined; - const firstOccurrence = (isPropertyAccessExpression(condition) || isIdentifier(condition)) - && checkMatch(condition, lastPropertyAccessChain.expression, checker) ? - condition : isBinaryExpression(condition) ? - getFirstOccurrenceInExpression(condition, lastPropertyAccessChain.expression, checker): undefined; + if (!lastPropertyAccessChain || checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) return undefined; + const first = lastPropertyAccessChain?.getFirstToken(); + if (!first || !checker.getSymbolAtLocation(first)) return undefined; - return firstOccurrence && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? - { lastPropertyAccessChain, firstOccurrence, expression } : undefined; + if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) + && checkMatch(condition, lastPropertyAccessChain.expression, checker)) { + return { lastPropertyAccessChain, occurrences:[condition], expression }; + } + else if (isBinaryExpression(condition)) { + const occurrences = getOccurrencesInExpression(lastPropertyAccessChain.expression, condition, checker); + return occurrences ? { lastPropertyAccessChain, occurrences, expression } : undefined; + } } function getBinaryInfo(expression: BinaryExpression, checker: TypeChecker): Info | undefined { if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); + if (!lastPropertyAccessChain) return undefined; + const first = lastPropertyAccessChain?.getFirstToken(); + if (!first || !checker.getSymbolAtLocation(first)) return undefined; - const firstOccurrence = getFirstOccurrenceInExpression(expression.left, lastPropertyAccessChain.expression, checker); - return firstOccurrence && (isPropertyAccessExpression(firstOccurrence) || isIdentifier(firstOccurrence)) ? - { lastPropertyAccessChain, firstOccurrence, expression } : undefined; + const occurrences = getOccurrencesInExpression(lastPropertyAccessChain.expression, expression.left, checker); + return occurrences ? { lastPropertyAccessChain, occurrences, expression } : undefined; } /** - * Gets the earliest expression or identifier in an && chain that matches the target access chain. - * e.g. a && a.b is searched with target a.b and returns a. + * Gets a list of property accesses that appear in matchTo and occur in sequence in expression. */ - function getFirstOccurrenceInExpression(expression: Expression, target: Expression, checker: TypeChecker): Expression | Identifier | undefined { - if (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const match = getFirstOccurrenceInExpression(expression.right, target, checker); - if (!match) return undefined; - return isPropertyAccessExpression(match) ? - getFirstOccurrenceInExpression(expression.left, match.expression, checker) ?? expression.right : expression.right; + function getOccurrencesInExpression(matchTo: Expression, expression: Expression, checker: TypeChecker): (PropertyAccessExpression | Identifier)[] | undefined { + const occurrences: (PropertyAccessExpression | Identifier)[] = []; + while (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { + const match = checkMatch(matchTo, expression.right, checker); + if (!match) { + break; + } + occurrences.push(match); + matchTo = match; + expression = expression.left; } - else { - const match = checkMatch(expression, target, checker); - return match ? expression : undefined; + const finalMatch = checkMatch(matchTo, expression, checker); + if (finalMatch) { + occurrences.push(finalMatch); } + return occurrences.length > 0 ? occurrences: undefined; } /** - * Checks that expression matches the final identifier in a property access expression or call expression. + * Checks that expression is a subchain of matchTo. */ - function checkMatch(expression: Expression, target: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { - if (isCallExpression(target)) { + function checkMatch(matchTo: Expression, expression: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + if (isCallExpression(matchTo)) { // Recurse through the call expressions to match a.b to a.b()(). - return !isCallExpression(expression) ? checkMatch(expression, target.expression, checker) : undefined; + return !isCallExpression(expression) ? checkMatch(matchTo.expression, expression, checker) : undefined; } - else if ((isPropertyAccessExpression(expression) && isPropertyAccessExpression(target)) || - (isIdentifier(expression) && isIdentifier(target))) { - const symbol = checker.getSymbolAtLocation(expression); - return symbol && !checker.isUnknownSymbol(symbol) && checker.isMatchingReference(expression, target) ? target : undefined; + else if ((isPropertyAccessExpression(expression) || isIdentifier(expression)) && (isPropertyAccessExpression(matchTo) || isIdentifier(matchTo))) { + return checker.isOrContainsMatchingReference(matchTo, expression) ? expression : undefined; } return undefined; } /** - * Find the least ancestor of the input node that is a valid type for extraction and contains the input span + * 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) { @@ -174,7 +182,7 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Gets the right-most property access expression + * Gets the right-most property access expression. */ function getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { if (isBinaryExpression(node)) { @@ -190,23 +198,32 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Converts a property access chain to an optional property access chain. + * Creates an access chain from toConvert with '?.' accesses at expressions appearing in occurrences. */ - function convertToOptionalChain(checker: TypeChecker, toConvert: Expression, until: PropertyAccessExpression | Identifier): Expression { - if (isCallExpression(toConvert)) { - const chain = convertToOptionalChain(checker, toConvert.expression, until); - return factory.createCallExpression(chain, toConvert.typeArguments, toConvert.arguments); - } - else if (isPropertyAccessExpression(toConvert) && !checkMatch(toConvert, until, checker)) { - const chain = convertToOptionalChain(checker, toConvert.expression, until); - return factory.createPropertyAccessChain(chain, factory.createToken(SyntaxKind.QuestionDotToken), toConvert.name); + 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 && checker.isMatchingReference(lastOccurrence, toConvert.expression); + 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 { lastPropertyAccessChain, firstOccurrence, expression } = info; - const convertedChain = convertToOptionalChain(checker, lastPropertyAccessChain, firstOccurrence); + const { lastPropertyAccessChain, occurrences, expression } = info; + const firstOccurrence = occurrences[occurrences.length - 1]; + const convertedChain = convertOccurrences(checker, lastPropertyAccessChain, occurrences); if (convertedChain && isPropertyAccessExpression(convertedChain)) { if (isBinaryExpression(expression)) { changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertedChain); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts index 9593ffa67ed71..3336a71ed8360 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallCallReturnValue.ts @@ -10,5 +10,5 @@ edit.applyRefactor({ actionDescription: "Convert to optional chain expression", newContent: `let a = { b: () => { return () => { c: 0 } } } -a?.b()()?.c;` +a?.b?.()().c;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts index 51e3743e028fa..f4f2a6768aa04 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessCallReturnValue.ts @@ -10,5 +10,5 @@ edit.applyRefactor({ actionDescription: "Convert to optional chain expression", newContent: `let a = { b: () => { return { c: 0 } } } -a?.b()?.c;` +a?.b?.().c;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts index 5a6ebef27fc52..99f34efe1bbd1 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NoRepeatCalls.ts @@ -30,5 +30,5 @@ edit.applyRefactor({ } } }}; -a?.b()?.c && a.b().c().d;` +a?.b?.().c && a.b().c().d;` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts index 472b8a22b3d75..13b24a264edf3 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts @@ -1,7 +1,12 @@ /// /////*a*/foo && foo.bar;/*b*/ +////let baz; +/////*c*/baz && baz.biz;/*d*/ -// Do not offer refactor for unknown symbols. +// Require at minimum that the first symbol in chain exists. TODO: can we relax this requirement for non-strict JS users? goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression") \ No newline at end of file +verify.not.refactorAvailable("Convert to optional chain expression"); + +goTo.select("c", "d"); +verify.refactorAvailable("Convert to optional chain expression") \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SparseAccess.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SparseAccess.ts new file mode 100644 index 0000000000000..13c0fac40cd3a --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SparseAccess.ts @@ -0,0 +1,15 @@ +/// + +////let a = { b: { c: { d: { e: {f: 0} } } } }; +/////*a*/a.b && a.b.c.d && a.b.c.d.e.f;/*b*/ + +// Only convert the accesses for which existence is checked. +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: { d: { e: {f: 0} } } } }; +a.b?.c.d?.e.f;` +}); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix1.ts similarity index 86% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix1.ts index 074a5bcf51230..ed6061a8e6726 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithPrefix1.ts @@ -5,7 +5,7 @@ ////let bar; ////foo && bar && /*a*/a && a.b && a.b.c;/*b*/ -// verify that stop at prefix sequence. +// verify that we stop at an invalid prefix sequence. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix1.ts similarity index 86% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix1.ts index 3a07b13f740c4..ab48eb8c85dec 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix1.ts @@ -5,7 +5,7 @@ ////let bar; /////*a*/a && a.b && a.b.c/*b*/ && foo && bar; -// verify that we stop at suffix sequence. +// verify that we stop at an invalid suffix sequence. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix2.ts similarity index 83% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix2.ts index a136c63709d68..ec038ae76c67f 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertibleSuffix.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithSuffix2.ts @@ -4,6 +4,7 @@ ////let x = { y: 0 }; /////*a*/a && a.b()/*b*/ && x && x.y(); +// verify that we stop at a suffix sequence which is otherwise valid. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionsWithPrefix2.ts similarity index 80% rename from tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts rename to tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionsWithPrefix2.ts index ae6234eeb86c1..15b3dbb8e70dc 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionWithConvertiblePrefix.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_SubexpressionsWithPrefix2.ts @@ -4,7 +4,7 @@ ////let x = { y: 0 }; ////a && a.b && /*a*/x && x.y;/*b*/ -// Verify that we stop at suffix sequence that is otherwise valid for conversion. +// Verify that we stop at a prefix sequence that is otherwise valid. goTo.select("a", "b"); edit.applyRefactor({ refactorName: "Convert to optional chain expression", From 8aed315c0b7f6344edc5825daba81ad29c65b098 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Thu, 2 Jul 2020 14:51:22 -0700 Subject: [PATCH 40/46] check match syntactically --- src/compiler/checker.ts | 10 ---- src/compiler/types.ts | 2 - .../convertToOptionalChainExpression.ts | 58 ++++++++++++++----- ...tionalChainExpression_ConditionalForAny.ts | 2 +- ...ToOptionalChainExpression_NotForUnknown.ts | 12 ---- ...ToOptionalChainExpression_UnknownSymbol.ts | 6 ++ 6 files changed, 51 insertions(+), 39 deletions(-) delete mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression_UnknownSymbol.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index fdf1d25e0c4e2..e22ab8affea01 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -675,16 +675,6 @@ namespace ts { getLocalTypeParametersOfClassOrInterfaceOrTypeAlias, isDeclarationVisible, - isMatchingReference: (nodeIn, targetIn) => { - const node = getParseTreeNode(nodeIn); - const target = getParseTreeNode(targetIn); - return !!node && !!target && isMatchingReference(node, target); - }, - isOrContainsMatchingReference: (nodeIn, targetIn) => { - const node = getParseTreeNode(nodeIn); - const target = getParseTreeNode(targetIn); - return !!node && !!target && isOrContainsMatchingReference(node, target); - }, }; function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index fe178446f537b..ce1162933e4e5 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4121,8 +4121,6 @@ namespace ts { /* @internal */ getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol: Symbol): readonly TypeParameter[] | undefined; /* @internal */ isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean; - /* @internal */ isMatchingReference(node: Node, target: Node): boolean; - /* @internal */ isOrContainsMatchingReference(node: Node, target: Node): boolean; } /* @internal */ diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index b7f1ff96e2bb3..babf1c6f6f33f 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -80,11 +80,9 @@ namespace ts.refactor.convertToOptionalChainExpression { const lastPropertyAccessChain = getLastPropertyAccessChain(expression.whenTrue); if (!lastPropertyAccessChain || checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) return undefined; - const first = lastPropertyAccessChain?.getFirstToken(); - if (!first || !checker.getSymbolAtLocation(first)) return undefined; if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) - && checkMatch(condition, lastPropertyAccessChain.expression, checker)) { + && getMatchingSubexpression(condition, lastPropertyAccessChain.expression, checker)) { return { lastPropertyAccessChain, occurrences:[condition], expression }; } else if (isBinaryExpression(condition)) { @@ -98,8 +96,6 @@ namespace ts.refactor.convertToOptionalChainExpression { const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); if (!lastPropertyAccessChain) return undefined; - const first = lastPropertyAccessChain?.getFirstToken(); - if (!first || !checker.getSymbolAtLocation(first)) return undefined; const occurrences = getOccurrencesInExpression(lastPropertyAccessChain.expression, expression.left, checker); return occurrences ? { lastPropertyAccessChain, occurrences, expression } : undefined; @@ -111,7 +107,7 @@ namespace ts.refactor.convertToOptionalChainExpression { function getOccurrencesInExpression(matchTo: Expression, expression: Expression, checker: TypeChecker): (PropertyAccessExpression | Identifier)[] | undefined { const occurrences: (PropertyAccessExpression | Identifier)[] = []; while (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const match = checkMatch(matchTo, expression.right, checker); + const match = getMatchingSubexpression(matchTo, expression.right, checker); if (!match) { break; } @@ -119,7 +115,7 @@ namespace ts.refactor.convertToOptionalChainExpression { matchTo = match; expression = expression.left; } - const finalMatch = checkMatch(matchTo, expression, checker); + const finalMatch = getMatchingSubexpression(matchTo, expression, checker); if (finalMatch) { occurrences.push(finalMatch); } @@ -127,19 +123,47 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Checks that expression is a subchain of matchTo. + * Checks that expression is a syntactic subexpression of matchTo. */ - function checkMatch(matchTo: Expression, expression: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + function getMatchingSubexpression(matchTo: Expression, expression: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { if (isCallExpression(matchTo)) { - // Recurse through the call expressions to match a.b to a.b()(). - return !isCallExpression(expression) ? checkMatch(matchTo.expression, expression, checker) : undefined; + return !isCallExpression(expression) ? getMatchingSubexpression(matchTo.expression, expression, checker) : undefined; } else if ((isPropertyAccessExpression(expression) || isIdentifier(expression)) && (isPropertyAccessExpression(matchTo) || isIdentifier(matchTo))) { - return checker.isOrContainsMatchingReference(matchTo, expression) ? expression : undefined; + return containsSyntacticSubchain(matchTo, expression, checker) ? expression : undefined; } return undefined; } + /** + * Returns true if target is a syntactic subchain of source. + */ + function containsSyntacticSubchain(source: Node, target: Node, checker: TypeChecker): boolean { + while (isPropertyAccessExpression(source) && !isSyntacticMatch(source, target)) { + source = source.expression; + } + while (isPropertyAccessExpression(source) && isPropertyAccessExpression(target)) { + if (!isSyntacticMatch(source, target)) return false; + source = source.expression; + target = target.expression; + } + const fullMatch = isSyntacticMatch(source, target); + if (fullMatch && !isInJSFile(source)) { + Debug.assert(checker.getSymbolAtLocation(source) === checker.getSymbolAtLocation(target)); + } + return fullMatch; + } + + function isSyntacticMatch(source: Node, target: Node): boolean { + if (isIdentifier(source) && isIdentifier(target)) { + return source.getText() === target.getText(); + } + else if (isPropertyAccessExpression(source) && isPropertyAccessExpression(target)) { + return source.name.getText() === target.name.getText(); + } + return false; + } + /** * Find the least ancestor of the input node that is a valid type for extraction and contains the input span. */ @@ -182,12 +206,18 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Gets the right-most property access expression. + * Gets a property access expression which may be nested inside of a call expression or binary expression. The final + * expression in an && chain will occur as the right child of the parent binary expression, unless it is a call expression + * or is followed by a different binary operator. + * @param node the right child of a binary expression or a call expression. */ function getLastPropertyAccessChain(node: Expression): 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)) { return getLastPropertyAccessChain(node.left); } + // foo && |foo.bar()()| - if the right child is a call expression, simply search its expression. else if (isCallExpression(node)) { return getLastPropertyAccessChain(node.expression); } @@ -204,7 +234,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 && checker.isMatchingReference(lastOccurrence, toConvert.expression); + const isOccurrence = lastOccurrence && isSyntacticMatch(lastOccurrence, toConvert.expression); if (isOccurrence) occurrences.pop(); if (isCallExpression(toConvert)) { return isOccurrence ? diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts index 9cddab3bfa036..cecca1a295e40 100644 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_ConditionalForAny.ts @@ -10,7 +10,7 @@ ////declare let foo: Foo; /////*a*/foo.bar ? foo.bar.baz : "whenFalse";/*b*/ -// It is reasonable to offer a refactor when baz is of type any since using any in strict mode +// It is reasonable to offer a refactor when baz is of type any since implicit any in strict mode // produces an error and those with strict mode off aren't getting null checks anyway. goTo.select("a", "b"); verify.refactorAvailable("Convert to optional chain expression"); diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts deleted file mode 100644 index 13b24a264edf3..0000000000000 --- a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_NotForUnknown.ts +++ /dev/null @@ -1,12 +0,0 @@ -/// - -/////*a*/foo && foo.bar;/*b*/ -////let baz; -/////*c*/baz && baz.biz;/*d*/ - -// Require at minimum that the first symbol in chain exists. TODO: can we relax this requirement for non-strict JS users? -goTo.select("a", "b"); -verify.not.refactorAvailable("Convert to optional chain expression"); - -goTo.select("c", "d"); -verify.refactorAvailable("Convert to optional chain expression") \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_UnknownSymbol.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_UnknownSymbol.ts new file mode 100644 index 0000000000000..ea4b6d6c1683b --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_UnknownSymbol.ts @@ -0,0 +1,6 @@ +/// + +/////*a*/foo && foo.bar;/*b*/ + +goTo.select("a", "b"); +verify.refactorAvailable("Convert to optional chain expression") \ No newline at end of file From 602075f1cc3012618cdfb511a19fd58454ea29a2 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 7 Jul 2020 09:57:48 -0700 Subject: [PATCH 41/46] handle another call expression case --- .../convertToOptionalChainExpression.ts | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index babf1c6f6f33f..68bf7dfeef3f1 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -26,7 +26,7 @@ namespace ts.refactor.convertToOptionalChainExpression { } interface Info { - lastPropertyAccessChain: PropertyAccessExpression, + finalExpression: PropertyAccessExpression | CallExpression, occurrences: (PropertyAccessExpression | Identifier)[], expression: ValidExpression } @@ -77,28 +77,28 @@ namespace ts.refactor.convertToOptionalChainExpression { function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { const condition = expression.condition; - const lastPropertyAccessChain = getLastPropertyAccessChain(expression.whenTrue); + const finalExpression = getFinalExpressionInChain(expression.whenTrue); - if (!lastPropertyAccessChain || checker.isNullableType(checker.getTypeAtLocation(lastPropertyAccessChain))) return undefined; + if (!finalExpression || checker.isNullableType(checker.getTypeAtLocation(finalExpression))) return undefined; if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) - && getMatchingSubexpression(condition, lastPropertyAccessChain.expression, checker)) { - return { lastPropertyAccessChain, occurrences:[condition], expression }; + && getMatchingSubexpression(condition, finalExpression.expression, checker)) { + return { finalExpression, occurrences:[condition], expression }; } else if (isBinaryExpression(condition)) { - const occurrences = getOccurrencesInExpression(lastPropertyAccessChain.expression, condition, checker); - return occurrences ? { lastPropertyAccessChain, occurrences, expression } : undefined; + const occurrences = getOccurrencesInExpression(finalExpression.expression, condition, checker); + return occurrences ? { finalExpression, occurrences, expression } : undefined; } } function getBinaryInfo(expression: BinaryExpression, checker: TypeChecker): Info | undefined { if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; - const lastPropertyAccessChain = getLastPropertyAccessChain(expression.right); + const finalExpression = getFinalExpressionInChain(expression.right); - if (!lastPropertyAccessChain) return undefined; + if (!finalExpression) return undefined; - const occurrences = getOccurrencesInExpression(lastPropertyAccessChain.expression, expression.left, checker); - return occurrences ? { lastPropertyAccessChain, occurrences, expression } : undefined; + const occurrences = getOccurrencesInExpression(finalExpression.expression, expression.left, checker); + return occurrences ? { finalExpression, occurrences, expression } : undefined; } /** @@ -139,7 +139,7 @@ namespace ts.refactor.convertToOptionalChainExpression { * Returns true if target is a syntactic subchain of source. */ function containsSyntacticSubchain(source: Node, target: Node, checker: TypeChecker): boolean { - while (isPropertyAccessExpression(source) && !isSyntacticMatch(source, target)) { + while ((isPropertyAccessExpression(source) || isCallExpression(source)) && !isSyntacticMatch(source, target)) { source = source.expression; } while (isPropertyAccessExpression(source) && isPropertyAccessExpression(target)) { @@ -206,22 +206,19 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Gets a property access expression which may be nested inside of a call expression or binary expression. The final - * expression in an && chain will occur as the right child of the parent binary expression, unless it is a call expression - * or is followed by a different binary operator. + * 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 getLastPropertyAccessChain(node: Expression): PropertyAccessExpression | undefined { + 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)) { - return getLastPropertyAccessChain(node.left); + return getFinalExpressionInChain(node.left); } - // foo && |foo.bar()()| - if the right child is a call expression, simply search its expression. - else if (isCallExpression(node)) { - return getLastPropertyAccessChain(node.expression); - } - else if (isPropertyAccessExpression(node) && !isOptionalChain(node)) { + // foo && |foo.bar()()| - nested calls are treated like further accesses. + else if ((isPropertyAccessExpression(node) || isCallExpression(node)) && !isOptionalChain(node)) { return node; } return undefined; @@ -251,10 +248,10 @@ namespace ts.refactor.convertToOptionalChainExpression { } function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { - const { lastPropertyAccessChain, occurrences, expression } = info; + const { finalExpression: lastPropertyAccessChain, occurrences, expression } = info; const firstOccurrence = occurrences[occurrences.length - 1]; const convertedChain = convertOccurrences(checker, lastPropertyAccessChain, occurrences); - if (convertedChain && isPropertyAccessExpression(convertedChain)) { + if (convertedChain && (isPropertyAccessExpression(convertedChain) || isCallExpression(convertedChain))) { if (isBinaryExpression(expression)) { changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertedChain); } From a4cc0607d289a3e1533193a476b7aeedfd1f5c7f Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 7 Jul 2020 10:16:44 -0700 Subject: [PATCH 42/46] some renames --- .../convertToOptionalChainExpression.ts | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 68bf7dfeef3f1..0ff5e1503266d 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -82,7 +82,7 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!finalExpression || checker.isNullableType(checker.getTypeAtLocation(finalExpression))) return undefined; if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) - && getMatchingSubexpression(condition, finalExpression.expression, checker)) { + && getMatchingStart(condition, finalExpression.expression, checker)) { return { finalExpression, occurrences:[condition], expression }; } else if (isBinaryExpression(condition)) { @@ -107,7 +107,7 @@ namespace ts.refactor.convertToOptionalChainExpression { function getOccurrencesInExpression(matchTo: Expression, expression: Expression, checker: TypeChecker): (PropertyAccessExpression | Identifier)[] | undefined { const occurrences: (PropertyAccessExpression | Identifier)[] = []; while (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) { - const match = getMatchingSubexpression(matchTo, expression.right, checker); + const match = getMatchingStart(matchTo, expression.right, checker); if (!match) { break; } @@ -115,7 +115,7 @@ namespace ts.refactor.convertToOptionalChainExpression { matchTo = match; expression = expression.left; } - const finalMatch = getMatchingSubexpression(matchTo, expression, checker); + const finalMatch = getMatchingStart(matchTo, expression, checker); if (finalMatch) { occurrences.push(finalMatch); } @@ -123,38 +123,41 @@ namespace ts.refactor.convertToOptionalChainExpression { } /** - * Checks that expression is a syntactic subexpression of matchTo. + * Returns subchain if chain begins with subchain syntactically. */ - function getMatchingSubexpression(matchTo: Expression, expression: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { - if (isCallExpression(matchTo)) { - return !isCallExpression(expression) ? getMatchingSubexpression(matchTo.expression, expression, checker) : undefined; + function getMatchingStart(chain: Expression, subchain: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { + if (isCallExpression(chain)) { + return !isCallExpression(subchain) ? getMatchingStart(chain.expression, subchain, checker) : undefined; } - else if ((isPropertyAccessExpression(expression) || isIdentifier(expression)) && (isPropertyAccessExpression(matchTo) || isIdentifier(matchTo))) { - return containsSyntacticSubchain(matchTo, expression, checker) ? expression : undefined; + else if ((isPropertyAccessExpression(subchain) || isIdentifier(subchain)) && (isPropertyAccessExpression(chain) || isIdentifier(chain))) { + return chainStartsWith(chain, subchain, checker) ? subchain : undefined; } return undefined; } /** - * Returns true if target is a syntactic subchain of source. + * Returns true if chain begins with subchain syntactically. */ - function containsSyntacticSubchain(source: Node, target: Node, checker: TypeChecker): boolean { - while ((isPropertyAccessExpression(source) || isCallExpression(source)) && !isSyntacticMatch(source, target)) { - source = source.expression; + function chainStartsWith(chain: Node, subchain: Node, checker: TypeChecker): boolean { + while ((isPropertyAccessExpression(chain) || isCallExpression(chain)) && !finalIdentifierMatches(chain, subchain)) { + chain = chain.expression; } - while (isPropertyAccessExpression(source) && isPropertyAccessExpression(target)) { - if (!isSyntacticMatch(source, target)) return false; - source = source.expression; - target = target.expression; + while (isPropertyAccessExpression(chain) && isPropertyAccessExpression(subchain)) { + if (!finalIdentifierMatches(chain, subchain)) return false; + chain = chain.expression; + subchain = subchain.expression; } - const fullMatch = isSyntacticMatch(source, target); - if (fullMatch && !isInJSFile(source)) { - Debug.assert(checker.getSymbolAtLocation(source) === checker.getSymbolAtLocation(target)); + const fullMatch = finalIdentifierMatches(chain, subchain); + if (fullMatch && !isInJSFile(chain)) { + Debug.assert(checker.getSymbolAtLocation(chain) === checker.getSymbolAtLocation(subchain)); } return fullMatch; } - function isSyntacticMatch(source: Node, target: Node): boolean { + /** + * Returns true if the identifier or final identifier in two access chains matches syntactically. + */ + function finalIdentifierMatches(source: Node, target: Node): boolean { if (isIdentifier(source) && isIdentifier(target)) { return source.getText() === target.getText(); } @@ -231,7 +234,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 && isSyntacticMatch(lastOccurrence, toConvert.expression); + const isOccurrence = lastOccurrence && finalIdentifierMatches(lastOccurrence, toConvert.expression); if (isOccurrence) occurrences.pop(); if (isCallExpression(toConvert)) { return isOccurrence ? From e89ae0822b718752bfce863b92c18acaac7f8604 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 7 Jul 2020 13:45:15 -0700 Subject: [PATCH 43/46] inline some checks --- .../convertToOptionalChainExpression.ts | 59 +++++++------------ 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 0ff5e1503266d..110704305bd79 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -72,7 +72,7 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!expression) return undefined; const checker = program.getTypeChecker(); - return isConditionalExpression(expression) ? getConditionalInfo(expression, checker) : getBinaryInfo(expression, checker); + return isConditionalExpression(expression) ? getConditionalInfo(expression, checker) : getBinaryInfo(expression); } function getConditionalInfo(expression: ConditionalExpression, checker: TypeChecker): Info | undefined { @@ -82,32 +82,32 @@ namespace ts.refactor.convertToOptionalChainExpression { if (!finalExpression || checker.isNullableType(checker.getTypeAtLocation(finalExpression))) return undefined; if ((isPropertyAccessExpression(condition) || isIdentifier(condition)) - && getMatchingStart(condition, finalExpression.expression, checker)) { + && getMatchingStart(condition, finalExpression.expression)) { return { finalExpression, occurrences:[condition], expression }; } else if (isBinaryExpression(condition)) { - const occurrences = getOccurrencesInExpression(finalExpression.expression, condition, checker); + const occurrences = getOccurrencesInExpression(finalExpression.expression, condition); return occurrences ? { finalExpression, occurrences, expression } : undefined; } } - function getBinaryInfo(expression: BinaryExpression, checker: TypeChecker): Info | 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, checker); + 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, checker: TypeChecker): (PropertyAccessExpression | Identifier)[] | undefined { + 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, checker); + const match = getMatchingStart(matchTo, expression.right); if (!match) { break; } @@ -115,7 +115,7 @@ namespace ts.refactor.convertToOptionalChainExpression { matchTo = match; expression = expression.left; } - const finalMatch = getMatchingStart(matchTo, expression, checker); + const finalMatch = getMatchingStart(matchTo, expression); if (finalMatch) { occurrences.push(finalMatch); } @@ -125,46 +125,29 @@ namespace ts.refactor.convertToOptionalChainExpression { /** * Returns subchain if chain begins with subchain syntactically. */ - function getMatchingStart(chain: Expression, subchain: Expression, checker: TypeChecker): PropertyAccessExpression | Identifier | undefined { - if (isCallExpression(chain)) { - return !isCallExpression(subchain) ? getMatchingStart(chain.expression, subchain, checker) : undefined; - } - else if ((isPropertyAccessExpression(subchain) || isIdentifier(subchain)) && (isPropertyAccessExpression(chain) || isIdentifier(chain))) { - return chainStartsWith(chain, subchain, checker) ? subchain : undefined; - } - return undefined; + function getMatchingStart(chain: Expression, subchain: Expression): PropertyAccessExpression | Identifier | undefined { + if (!isIdentifier(subchain) && !isPropertyAccessExpression(subchain)) return undefined; + return chainStartsWith(chain, subchain) ? subchain : undefined; } /** * Returns true if chain begins with subchain syntactically. */ - function chainStartsWith(chain: Node, subchain: Node, checker: TypeChecker): boolean { - while ((isPropertyAccessExpression(chain) || isCallExpression(chain)) && !finalIdentifierMatches(chain, subchain)) { + 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 (!finalIdentifierMatches(chain, subchain)) return false; + if (chain.name.getText() !== subchain.name.getText()) return false; chain = chain.expression; subchain = subchain.expression; } - const fullMatch = finalIdentifierMatches(chain, subchain); - if (fullMatch && !isInJSFile(chain)) { - Debug.assert(checker.getSymbolAtLocation(chain) === checker.getSymbolAtLocation(subchain)); - } - return fullMatch; - } - - /** - * Returns true if the identifier or final identifier in two access chains matches syntactically. - */ - function finalIdentifierMatches(source: Node, target: Node): boolean { - if (isIdentifier(source) && isIdentifier(target)) { - return source.getText() === target.getText(); - } - else if (isPropertyAccessExpression(source) && isPropertyAccessExpression(target)) { - return source.name.getText() === target.name.getText(); - } - return false; + // check if we have reached a final identifier. + return isIdentifier(chain) && isIdentifier(subchain) && chain.getText() === subchain.getText(); } /** @@ -234,7 +217,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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 && finalIdentifierMatches(lastOccurrence, toConvert.expression); + const isOccurrence = lastOccurrence?.getText() === toConvert.expression.getText(); if (isOccurrence) occurrences.pop(); if (isCallExpression(toConvert)) { return isOccurrence ? From 2cdb5e1f882597f96a8b3d78b8ff93472df053b6 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Tue, 7 Jul 2020 14:12:03 -0700 Subject: [PATCH 44/46] add test --- ...onvertToOptionalChainExpression_AccessThenCall.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessThenCall.ts diff --git a/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessThenCall.ts b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessThenCall.ts new file mode 100644 index 0000000000000..3e6f799604784 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToOptionalChainExpression_AccessThenCall.ts @@ -0,0 +1,12 @@ +/// + +/////*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?.();` +}); \ No newline at end of file From fd64b14c95b7562b1ef362c8090cb58c69e1c6b8 Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 13 Jul 2020 12:19:42 -0700 Subject: [PATCH 45/46] address comments --- .../refactors/convertToOptionalChainExpression.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 110704305bd79..94b3d04582518 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -107,7 +107,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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); + const match = getMatchingStart(skipParentheses(matchTo), skipParentheses(expression.right)); if (!match) { break; } @@ -126,8 +126,8 @@ namespace ts.refactor.convertToOptionalChainExpression { * 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; + return (isIdentifier(subchain) || isPropertyAccessExpression(subchain)) && + chainStartsWith(chain, subchain) ? subchain : undefined; } /** @@ -200,6 +200,7 @@ namespace ts.refactor.convertToOptionalChainExpression { 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); } @@ -234,12 +235,12 @@ namespace ts.refactor.convertToOptionalChainExpression { } function doChange(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, info: Info, _actionName: string): void { - const { finalExpression: lastPropertyAccessChain, occurrences, expression } = info; + const { finalExpression, occurrences, expression } = info; const firstOccurrence = occurrences[occurrences.length - 1]; - const convertedChain = convertOccurrences(checker, lastPropertyAccessChain, occurrences); + const convertedChain = convertOccurrences(checker, finalExpression, occurrences); if (convertedChain && (isPropertyAccessExpression(convertedChain) || isCallExpression(convertedChain))) { if (isBinaryExpression(expression)) { - changes.replaceNodeRange(sourceFile, firstOccurrence, lastPropertyAccessChain, convertedChain); + changes.replaceNodeRange(sourceFile, firstOccurrence, finalExpression, convertedChain); } else if (isConditionalExpression(expression)) { changes.replaceNode(sourceFile, expression, From 01854bbc1a3c0ba6f56da8e5774b5b15b7c0b7ff Mon Sep 17 00:00:00 2001 From: Jesse Trinity Date: Mon, 13 Jul 2020 14:22:15 -0700 Subject: [PATCH 46/46] add refactorNotAvailableReason --- src/compiler/diagnosticMessages.json | 12 +++ .../convertToOptionalChainExpression.ts | 74 +++++++++++++------ 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 5f1e894dda32a..9e07924dce005 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -5831,6 +5831,18 @@ "category": "Message", "code": 95139 }, + "Could not find convertible access expression": { + "category": "Message", + "code": 95140 + }, + "Could not find matching access expressions": { + "category": "Message", + "code": 95141 + }, + "Can only convert logical AND access chains": { + "category": "Message", + "code": 95142 + }, "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": { "category": "Error", diff --git a/src/services/refactors/convertToOptionalChainExpression.ts b/src/services/refactors/convertToOptionalChainExpression.ts index 94b3d04582518..0333506b1e3f2 100644 --- a/src/services/refactors/convertToOptionalChainExpression.ts +++ b/src/services/refactors/convertToOptionalChainExpression.ts @@ -8,28 +8,54 @@ namespace ts.refactor.convertToOptionalChainExpression { function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] { const info = getInfo(context, context.triggerReason === "invoked"); if (!info) return emptyArray; - return [{ - name: refactorName, - description: convertToOptionalChainExpressionMessage, - actions: [{ + + if (!info.error) { + return [{ + name: refactorName, + description: convertToOptionalChainExpressionMessage, + actions: [{ + name: refactorName, + description: convertToOptionalChainExpressionMessage + }] + }]; + } + + if (context.preferences.provideRefactorNotApplicableReason) { + return [{ name: refactorName, - description: convertToOptionalChainExpressionMessage - }] - }]; + 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) return undefined; - const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program.getTypeChecker(), t, info, actionName)); + 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 - } + expression: ValidExpression, + }; type ValidExpressionOrStatement = ValidExpression | ValidStatement; @@ -55,7 +81,7 @@ namespace ts.refactor.convertToOptionalChainExpression { return isValidExpression(node) || isValidStatement(node); } - function getInfo(context: RefactorContext, considerEmptySpans = true): Info | undefined { + function getInfo(context: RefactorContext, considerEmptySpans = true): InfoOrError | undefined { const { file, program } = context; const span = getRefactorContextSpan(context); @@ -69,36 +95,42 @@ namespace ts.refactor.convertToOptionalChainExpression { const parent = forEmptySpan ? getValidParentNodeOfEmptySpan(startToken) : getValidParentNodeContainingSpan(startToken, adjustedSpan); const expression = parent && isValidExpressionOrStatement(parent) ? getExpression(parent) : undefined; - if (!expression) return 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): Info | undefined { + 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 undefined; + 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 { finalExpression, occurrences:[condition], expression }; + return { info: { finalExpression, occurrences: [condition], expression } }; } else if (isBinaryExpression(condition)) { const occurrences = getOccurrencesInExpression(finalExpression.expression, condition); - return occurrences ? { finalExpression, occurrences, expression } : undefined; + return occurrences ? { info: { finalExpression, occurrences, expression } } : + { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_matching_access_expressions) }; } } - function getBinaryInfo(expression: BinaryExpression): Info | undefined { - if (expression.operatorToken.kind !== SyntaxKind.AmpersandAmpersandToken) return undefined; + 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 undefined; + if (!finalExpression) return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_convertible_access_expression) }; const occurrences = getOccurrencesInExpression(finalExpression.expression, expression.left); - return occurrences ? { finalExpression, occurrences, expression } : undefined; + return occurrences ? { info: { finalExpression, occurrences, expression } } : + { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_matching_access_expressions) }; } /**