From d3d088fac50335c7bb7daa4f8e6578ba7f4ea9c2 Mon Sep 17 00:00:00 2001 From: Oleksandr T Date: Mon, 29 Nov 2021 23:22:53 +0200 Subject: [PATCH] fix(44639): Transpilation of optional chaining combined with type casting results in function call losing its context (#44666) * fix(44639): Fix emit for optional chain with type assertions * Move ASI-blocking parens out of ts transform * Add missing comments from PartiallyEmittedExpression+minor cleanup * Avoid comment duplication on copied receiver Co-authored-by: Ron Buckton --- src/compiler/emitter.ts | 61 ++++++++++++++++--- src/compiler/transformers/es2020.ts | 14 +++-- src/compiler/transformers/ts.ts | 9 ++- ...ChainingInTypeAssertions(target=es2015).js | 24 ++++++++ ...ingInTypeAssertions(target=es2015).symbols | 32 ++++++++++ ...iningInTypeAssertions(target=es2015).types | 45 ++++++++++++++ ...ChainingInTypeAssertions(target=esnext).js | 23 +++++++ ...ingInTypeAssertions(target=esnext).symbols | 32 ++++++++++ ...iningInTypeAssertions(target=esnext).types | 45 ++++++++++++++ .../optionalChainingInTypeAssertions.ts | 13 ++++ 10 files changed, 280 insertions(+), 18 deletions(-) create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).js create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).symbols create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).types create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).js create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).symbols create mode 100644 tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).types create mode 100644 tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts index 5a2fc96f58b..0df29c10ad6 100644 --- a/src/compiler/emitter.ts +++ b/src/compiler/emitter.ts @@ -2766,7 +2766,7 @@ namespace ts { function emitYieldExpression(node: YieldExpression) { emitTokenWithComment(SyntaxKind.YieldKeyword, node.pos, writeKeyword, node); emit(node.asteriskToken); - emitExpressionWithLeadingSpace(node.expression, parenthesizer.parenthesizeExpressionForDisallowedComma); + emitExpressionWithLeadingSpace(node.expression && parenthesizeExpressionForNoAsi(node.expression), parenthesizeExpressionForNoAsiAndDisallowedComma); } function emitSpreadElement(node: SpreadElement) { @@ -2990,9 +2990,49 @@ namespace ts { return pos; } + function commentWillEmitNewLine(node: CommentRange) { + return node.kind === SyntaxKind.SingleLineCommentTrivia || !!node.hasTrailingNewLine; + } + + function willEmitLeadingNewLine(node: Expression): boolean { + if (!currentSourceFile) return false; + if (some(getLeadingCommentRanges(currentSourceFile.text, node.pos), commentWillEmitNewLine)) return true; + if (some(getSyntheticLeadingComments(node), commentWillEmitNewLine)) return true; + if (isPartiallyEmittedExpression(node)) { + if (node.pos !== node.expression.pos) { + if (some(getTrailingCommentRanges(currentSourceFile.text, node.expression.pos), commentWillEmitNewLine)) return true; + } + return willEmitLeadingNewLine(node.expression); + } + return false; + } + + /** + * Wraps an expression in parens if we would emit a leading comment that would introduce a line separator + * between the node and its parent. + */ + function parenthesizeExpressionForNoAsi(node: Expression) { + if (!commentsDisabled && isPartiallyEmittedExpression(node) && willEmitLeadingNewLine(node)) { + const parseNode = getParseTreeNode(node); + if (parseNode && isParenthesizedExpression(parseNode)) { + // If the original node was a parenthesized expression, restore it to preserve comment and source map emit + const parens = factory.createParenthesizedExpression(node.expression); + setOriginalNode(parens, node); + setTextRange(parens, parseNode); + return parens; + } + return factory.createParenthesizedExpression(node); + } + return node; + } + + function parenthesizeExpressionForNoAsiAndDisallowedComma(node: Expression) { + return parenthesizeExpressionForNoAsi(parenthesizer.parenthesizeExpressionForDisallowedComma(node)); + } + function emitReturnStatement(node: ReturnStatement) { emitTokenWithComment(SyntaxKind.ReturnKeyword, node.pos, writeKeyword, /*contextNode*/ node); - emitExpressionWithLeadingSpace(node.expression); + emitExpressionWithLeadingSpace(node.expression && parenthesizeExpressionForNoAsi(node.expression), parenthesizeExpressionForNoAsi); writeTrailingSemicolon(); } @@ -3024,7 +3064,7 @@ namespace ts { function emitThrowStatement(node: ThrowStatement) { emitTokenWithComment(SyntaxKind.ThrowKeyword, node.pos, writeKeyword, node); - emitExpressionWithLeadingSpace(node.expression); + emitExpressionWithLeadingSpace(parenthesizeExpressionForNoAsi(node.expression), parenthesizeExpressionForNoAsi); writeTrailingSemicolon(); } @@ -3974,7 +4014,14 @@ namespace ts { // Transformation nodes function emitPartiallyEmittedExpression(node: PartiallyEmittedExpression) { + const emitFlags = getEmitFlags(node); + if (!(emitFlags & EmitFlags.NoLeadingComments) && node.pos !== node.expression.pos) { + emitTrailingCommentsOfPosition(node.expression.pos); + } emitExpression(node.expression); + if (!(emitFlags & EmitFlags.NoTrailingComments) && node.end !== node.expression.end) { + emitLeadingCommentsOfPosition(node.expression.end); + } } function emitCommaList(node: CommaListExpression) { @@ -4362,10 +4409,8 @@ namespace ts { // Emit this child. previousSourceFileTextKind = recordBundleFileInternalSectionStart(child); if (shouldEmitInterveningComments) { - if (emitTrailingCommentsOfPosition) { - const commentRange = getCommentRange(child); - emitTrailingCommentsOfPosition(commentRange.pos); - } + const commentRange = getCommentRange(child); + emitTrailingCommentsOfPosition(commentRange.pos); } else { shouldEmitInterveningComments = mayEmitInterveningComments; @@ -4743,7 +4788,7 @@ namespace ts { function writeLineSeparatorsAndIndentBefore(node: Node, parent: Node): boolean { const leadingNewlines = preserveSourceNewlines && getLeadingLineTerminatorCount(parent, [node], ListFormat.None); if (leadingNewlines) { - writeLinesAndIndent(leadingNewlines, /*writeLinesIfNotIndenting*/ false); + writeLinesAndIndent(leadingNewlines, /*writeSpaceIfNotIndenting*/ false); } return !!leadingNewlines; } diff --git a/src/compiler/transformers/es2020.ts b/src/compiler/transformers/es2020.ts index 5e2311a2d27..328cc2376d6 100644 --- a/src/compiler/transformers/es2020.ts +++ b/src/compiler/transformers/es2020.ts @@ -122,11 +122,11 @@ namespace ts { function visitOptionalExpression(node: OptionalChain, captureThisArg: boolean, isDelete: boolean): Expression { const { expression, chain } = flattenChain(node); - const left = visitNonOptionalExpression(expression, isCallChain(chain[0]), /*isDelete*/ false); - const leftThisArg = isSyntheticReference(left) ? left.thisArg : undefined; - let leftExpression = isSyntheticReference(left) ? left.expression : left; - let capturedLeft: Expression = leftExpression; - if (!isSimpleCopiableExpression(leftExpression)) { + const left = visitNonOptionalExpression(skipPartiallyEmittedExpressions(expression), isCallChain(chain[0]), /*isDelete*/ false); + let leftThisArg = isSyntheticReference(left) ? left.thisArg : undefined; + let capturedLeft = isSyntheticReference(left) ? left.expression : left; + let leftExpression = factory.restoreOuterExpressions(expression, capturedLeft, OuterExpressionKinds.PartiallyEmittedExpressions); + if (!isSimpleCopiableExpression(capturedLeft)) { capturedLeft = factory.createTempVariable(hoistVariableDeclaration); leftExpression = factory.createAssignment(capturedLeft, leftExpression); } @@ -152,6 +152,10 @@ namespace ts { break; case SyntaxKind.CallExpression: if (i === 0 && leftThisArg) { + if (!isGeneratedIdentifier(leftThisArg)) { + leftThisArg = factory.cloneNode(leftThisArg); + addEmitFlags(leftThisArg, EmitFlags.NoComments); + } rightExpression = factory.createFunctionCallCall( rightExpression, leftThisArg.kind === SyntaxKind.SuperKeyword ? factory.createThis() : leftThisArg, diff --git a/src/compiler/transformers/ts.ts b/src/compiler/transformers/ts.ts index 8561db1c633..c6c76f70dfb 100644 --- a/src/compiler/transformers/ts.ts +++ b/src/compiler/transformers/ts.ts @@ -2237,11 +2237,10 @@ namespace ts { // we can safely elide the parentheses here, as a new synthetic // ParenthesizedExpression will be inserted if we remove parentheses too // aggressively. - // HOWEVER - if there are leading comments on the expression itself, to handle ASI - // correctly for return and throw, we must keep the parenthesis - if (length(getLeadingCommentRangesOfNode(expression, currentSourceFile))) { - return factory.updateParenthesizedExpression(node, expression); - } + // + // If there are leading comments on the expression itself, the emitter will handle ASI + // for return, throw, and yield by re-introducing parenthesis during emit on an as-need + // basis. return factory.createPartiallyEmittedExpression(expression, node); } diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).js b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).js new file mode 100644 index 00000000000..1c3ccbc1d61 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).js @@ -0,0 +1,24 @@ +//// [optionalChainingInTypeAssertions.ts] +class Foo { + m() {} +} + +const foo = new Foo(); + +(foo.m as any)?.(); +(foo.m)?.(); + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); + + +//// [optionalChainingInTypeAssertions.js] +var _a, _b, _c, _d; +class Foo { + m() { } +} +const foo = new Foo(); +(_a = foo.m) === null || _a === void 0 ? void 0 : _a.call(foo); +(_b = foo.m) === null || _b === void 0 ? void 0 : _b.call(foo); +/*a1*/ (_c = /*a2*/ foo.m /*a3*/ /*a4*/) === null || _c === void 0 ? void 0 : _c.call(foo); +/*b1*/ (_d = /*b2*/ foo.m /*b3*/ /*b4*/) === null || _d === void 0 ? void 0 : _d.call(foo); diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).symbols b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).symbols new file mode 100644 index 00000000000..3c84c092176 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).symbols @@ -0,0 +1,32 @@ +=== tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts === +class Foo { +>Foo : Symbol(Foo, Decl(optionalChainingInTypeAssertions.ts, 0, 0)) + + m() {} +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +} + +const foo = new Foo(); +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>Foo : Symbol(Foo, Decl(optionalChainingInTypeAssertions.ts, 0, 0)) + +(foo.m as any)?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +(foo.m)?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).types b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).types new file mode 100644 index 00000000000..2df0f69dbc5 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=es2015).types @@ -0,0 +1,45 @@ +=== tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts === +class Foo { +>Foo : Foo + + m() {} +>m : () => void +} + +const foo = new Foo(); +>foo : Foo +>new Foo() : Foo +>Foo : typeof Foo + +(foo.m as any)?.(); +>(foo.m as any)?.() : any +>(foo.m as any) : any +>foo.m as any : any +>foo.m : () => void +>foo : Foo +>m : () => void + +(foo.m)?.(); +>(foo.m)?.() : any +>(foo.m) : any +>foo.m : any +>foo.m : () => void +>foo : Foo +>m : () => void + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +>(/*a2*/foo.m as any/*a3*/)/*a4*/?.() : any +>(/*a2*/foo.m as any/*a3*/) : any +>foo.m as any : any +>foo.m : () => void +>foo : Foo +>m : () => void + +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); +>(/*b2*/foo.m/*b3*/)/*b4*/?.() : any +>(/*b2*/foo.m/*b3*/) : any +>foo.m : any +>foo.m : () => void +>foo : Foo +>m : () => void + diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).js b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).js new file mode 100644 index 00000000000..454b3962c57 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).js @@ -0,0 +1,23 @@ +//// [optionalChainingInTypeAssertions.ts] +class Foo { + m() {} +} + +const foo = new Foo(); + +(foo.m as any)?.(); +(foo.m)?.(); + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); + + +//// [optionalChainingInTypeAssertions.js] +class Foo { + m() { } +} +const foo = new Foo(); +foo.m?.(); +foo.m?.(); +/*a1*/ /*a2*/ foo.m /*a3*/ /*a4*/?.(); +/*b1*/ /*b2*/ foo.m /*b3*/ /*b4*/?.(); diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).symbols b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).symbols new file mode 100644 index 00000000000..3c84c092176 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).symbols @@ -0,0 +1,32 @@ +=== tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts === +class Foo { +>Foo : Symbol(Foo, Decl(optionalChainingInTypeAssertions.ts, 0, 0)) + + m() {} +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +} + +const foo = new Foo(); +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>Foo : Symbol(Foo, Decl(optionalChainingInTypeAssertions.ts, 0, 0)) + +(foo.m as any)?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +(foo.m)?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); +>foo.m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) +>foo : Symbol(foo, Decl(optionalChainingInTypeAssertions.ts, 4, 5)) +>m : Symbol(Foo.m, Decl(optionalChainingInTypeAssertions.ts, 0, 11)) + diff --git a/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).types b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).types new file mode 100644 index 00000000000..2df0f69dbc5 --- /dev/null +++ b/tests/baselines/reference/optionalChainingInTypeAssertions(target=esnext).types @@ -0,0 +1,45 @@ +=== tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts === +class Foo { +>Foo : Foo + + m() {} +>m : () => void +} + +const foo = new Foo(); +>foo : Foo +>new Foo() : Foo +>Foo : typeof Foo + +(foo.m as any)?.(); +>(foo.m as any)?.() : any +>(foo.m as any) : any +>foo.m as any : any +>foo.m : () => void +>foo : Foo +>m : () => void + +(foo.m)?.(); +>(foo.m)?.() : any +>(foo.m) : any +>foo.m : any +>foo.m : () => void +>foo : Foo +>m : () => void + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +>(/*a2*/foo.m as any/*a3*/)/*a4*/?.() : any +>(/*a2*/foo.m as any/*a3*/) : any +>foo.m as any : any +>foo.m : () => void +>foo : Foo +>m : () => void + +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.(); +>(/*b2*/foo.m/*b3*/)/*b4*/?.() : any +>(/*b2*/foo.m/*b3*/) : any +>foo.m : any +>foo.m : () => void +>foo : Foo +>m : () => void + diff --git a/tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts b/tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts new file mode 100644 index 00000000000..3140c2d6ca5 --- /dev/null +++ b/tests/cases/conformance/expressions/optionalChaining/optionalChainingInTypeAssertions.ts @@ -0,0 +1,13 @@ +// @target: es2015, esnext + +class Foo { + m() {} +} + +const foo = new Foo(); + +(foo.m as any)?.(); +(foo.m)?.(); + +/*a1*/(/*a2*/foo.m as any/*a3*/)/*a4*/?.(); +/*b1*/(/*b2*/foo.m/*b3*/)/*b4*/?.();