Indirect calls for imported functions (#44624)

* Indirect calls for imported functions

* Fix unit tests

Author: rbuckton

src/compiler/emitter.ts

@@ -2473,7 +2473,17 @@ namespace ts {
         }
 
         function emitCallExpression(node: CallExpression) {
+            const indirectCall = getEmitFlags(node) & EmitFlags.IndirectCall;
+            if (indirectCall) {
+                writePunctuation("(");
+                writeLiteral("0");
+                writePunctuation(",");
+                writeSpace();
+            }
             emitExpression(node.expression, parenthesizer.parenthesizeLeftSideOfAccess);
+            if (indirectCall) {
+                writePunctuation(")");
+            }
             emit(node.questionDotToken);
             emitTypeArguments(node, node.typeArguments);
             emitExpressionList(node, node.arguments, ListFormat.CallExpressionArguments, parenthesizer.parenthesizeExpressionForDisallowedComma);
@@ -2488,7 +2498,17 @@ namespace ts {
         }
 
         function emitTaggedTemplateExpression(node: TaggedTemplateExpression) {
+            const indirectCall = getEmitFlags(node) & EmitFlags.IndirectCall;
+            if (indirectCall) {
+                writePunctuation("(");
+                writeLiteral("0");
+                writePunctuation(",");
+                writeSpace();
+            }
             emitExpression(node.tag, parenthesizer.parenthesizeLeftSideOfAccess);
+            if (indirectCall) {
+                writePunctuation(")");
+            }
             emitTypeArguments(node, node.typeArguments);
             writeSpace();
             emitExpression(node.template);

src/compiler/transformers/module/module.ts

@@ -33,6 +33,8 @@ namespace ts {
         const previousOnEmitNode = context.onEmitNode;
         context.onSubstituteNode = onSubstituteNode;
         context.onEmitNode = onEmitNode;
+        context.enableSubstitution(SyntaxKind.CallExpression); // Substitute calls to imported/exported symbols to avoid incorrect `this`.
+        context.enableSubstitution(SyntaxKind.TaggedTemplateExpression); // Substitute calls to imported/exported symbols to avoid incorrect `this`.
         context.enableSubstitution(SyntaxKind.Identifier); // Substitutes expression identifiers with imported/exported symbols.
         context.enableSubstitution(SyntaxKind.BinaryExpression); // Substitutes assignments to exported symbols.
         context.enableSubstitution(SyntaxKind.PrefixUnaryExpression); // Substitutes updates to exported symbols.
@@ -1741,6 +1743,10 @@ namespace ts {
             switch (node.kind) {
                 case SyntaxKind.Identifier:
                     return substituteExpressionIdentifier(node as Identifier);
+                case SyntaxKind.CallExpression:
+                    return substituteCallExpression(node as CallExpression);
+                case SyntaxKind.TaggedTemplateExpression:
+                    return substituteTaggedTemplateExpression(node as TaggedTemplateExpression);
                 case SyntaxKind.BinaryExpression:
                     return substituteBinaryExpression(node as BinaryExpression);
                 case SyntaxKind.PostfixUnaryExpression:
@@ -1751,6 +1757,43 @@ namespace ts {
             return node;
         }
 
+        function substituteCallExpression(node: CallExpression) {
+            if (isIdentifier(node.expression)) {
+                const expression = substituteExpressionIdentifier(node.expression);
+                noSubstitution[getNodeId(expression)] = true;
+                if (!isIdentifier(expression)) {
+                    return addEmitFlags(
+                        factory.updateCallExpression(node,
+                            expression,
+                            /*typeArguments*/ undefined,
+                            node.arguments
+                        ),
+                        EmitFlags.IndirectCall
+                    );
+
+                }
+            }
+            return node;
+        }
+
+        function substituteTaggedTemplateExpression(node: TaggedTemplateExpression) {
+            if (isIdentifier(node.tag)) {
+                const tag = substituteExpressionIdentifier(node.tag);
+                noSubstitution[getNodeId(tag)] = true;
+                if (!isIdentifier(tag)) {
+                    return addEmitFlags(
+                        factory.updateTaggedTemplateExpression(node,
+                            tag,
+                            /*typeArguments*/ undefined,
+                            node.template
+                        ),
+                        EmitFlags.IndirectCall
+                    );
+                }
+            }
+            return node;
+        }
+
         /**
          * Substitution for an Identifier expression that may contain an imported or exported
          * symbol.

src/compiler/types.ts

@@ -6732,6 +6732,7 @@ namespace ts {
         /*@internal*/ NeverApplyImportHelper = 1 << 26, // Indicates the node should never be wrapped with an import star helper (because, for example, it imports tslib itself)
         /*@internal*/ IgnoreSourceNewlines = 1 << 27,   // Overrides `printerOptions.preserveSourceNewlines` to print this node (and all descendants) with default whitespace.
         /*@internal*/ Immutable = 1 << 28,      // Indicates a node is a singleton intended to be reused in multiple locations. Any attempt to make further changes to the node will result in an error.
+        /*@internal*/ IndirectCall = 1 << 29,   // Emit CallExpression as an indirect call: `(0, f)()`
     }
 
     export interface EmitHelperBase {

src/testRunner/tsconfig.json

@@ -93,6 +93,7 @@
         "unittests/evaluation/asyncGenerator.ts",
         "unittests/evaluation/awaiter.ts",
         "unittests/evaluation/destructuring.ts",
+        "unittests/evaluation/externalModules.ts",
         "unittests/evaluation/forAwaitOf.ts",
         "unittests/evaluation/forOf.ts",
         "unittests/evaluation/optionalCall.ts",

src/testRunner/unittests/evaluation/externalModules.ts

@@ -0,0 +1,84 @@
+describe("unittests:: evaluation:: externalModules", () => {
+    // https://github.com/microsoft/TypeScript/issues/35420
+    it("Correct 'this' in function exported from external module", async () => {
+        const result = evaluator.evaluateTypeScript({
+            files: {
+                "/.src/output.ts": `
+                    export const output: any[] = [];
+                `,
+                "/.src/other.ts": `
+                    import { output } from "./output";
+                    export function f(this: any, expected) {
+                        output.push(this === expected);
+                    }
+
+                    // 0
+                    f(undefined);
+                `,
+                "/.src/main.ts": `
+                    export { output } from "./output";
+                    import { output } from "./output";
+                    import { f } from "./other";
+                    import * as other from "./other";
+
+                    // 1
+                    f(undefined);
+
+                    // 2
+                    const obj = {};
+                    f.call(obj, obj);
+
+                    // 3
+                    other.f(other);
+                `
+            },
+            rootFiles: ["/.src/main.ts"],
+            main: "/.src/main.ts"
+        });
+        assert.equal(result.output[0], true); // `f(undefined)` inside module. Existing behavior is correct.
+        assert.equal(result.output[1], true); // `f(undefined)` from import. New behavior to match first case.
+        assert.equal(result.output[2], true); // `f.call(obj, obj)`. Behavior of `.call` (or `.apply`, etc.) should not be affected.
+        assert.equal(result.output[3], true); // `other.f(other)`. `this` is still namespace because it is left of `.`.
+    });
+
+    it("Correct 'this' in function expression exported from external module", async () => {
+        const result = evaluator.evaluateTypeScript({
+            files: {
+                "/.src/output.ts": `
+                    export const output: any[] = [];
+                `,
+                "/.src/other.ts": `
+                    import { output } from "./output";
+                    export const f = function(this: any, expected) {
+                        output.push(this === expected);
+                    }
+
+                    // 0
+                    f(undefined);
+                `,
+                "/.src/main.ts": `
+                    export { output } from "./output";
+                    import { output } from "./output";
+                    import { f } from "./other";
+                    import * as other from "./other";
+
+                    // 1
+                    f(undefined);
+
+                    // 2
+                    const obj = {};
+                    f.call(obj, obj);
+
+                    // 3
+                    other.f(other);
+                `
+            },
+            rootFiles: ["/.src/main.ts"],
+            main: "/.src/main.ts"
+        });
+        assert.equal(result.output[0], true); // `f(undefined)` inside module. Existing behavior is incorrect.
+        assert.equal(result.output[1], true); // `f(undefined)` from import. New behavior to match first case.
+        assert.equal(result.output[2], true); // `f.call(obj, obj)`. Behavior of `.call` (or `.apply`, etc.) should not be affected.
+        assert.equal(result.output[3], true); // `other.f(other)`. `this` is still namespace because it is left of `.`.
+    });
+});

src/testRunner/unittests/tsserver/projectReferenceCompileOnSave.ts

@@ -55,8 +55,8 @@ exports.fn3 = fn3;`;
                 content: `"use strict";
 exports.__esModule = true;${appendJsText === changeJs ? "\nexports.fn3 = void 0;" : ""}
 var fns_1 = require("../decls/fns");
-fns_1.fn1();
-fns_1.fn2();
+(0, fns_1.fn1)();
+(0, fns_1.fn2)();
 ${appendJs}`
             }];
         }

tests/baselines/reference/allowJscheckJsTypeParameterNoCrash.js

@@ -30,7 +30,7 @@ exports.__esModule = true;
 exports.a = void 0;
 var func_1 = require("./func");
 // hover on vextend
-exports.a = func_1.vextend({
+exports.a = (0, func_1.vextend)({
     watch: {
         data1: function (val) {
             this.data2 = 1;

tests/baselines/reference/allowSyntheticDefaultImportsCanPaintCrossModuleDeclaration.js

@@ -23,7 +23,7 @@ exports.__esModule = true;
 exports.__esModule = true;
 exports.A = void 0;
 var file1_1 = require("./file1");
-exports.A = file1_1.styled();
+exports.A = (0, file1_1.styled)();
 
 
 //// [color.d.ts]

tests/baselines/reference/ambientDeclarationsPatterns.js

@@ -37,9 +37,9 @@ foo(fileText);
 exports.__esModule = true;
 ///<reference path="declarations.d.ts" />
 var foobarbaz_1 = require("foobarbaz");
-foobarbaz_1.foo(foobarbaz_1.baz);
+(0, foobarbaz_1.foo)(foobarbaz_1.baz);
 var foosball_1 = require("foosball");
-foobarbaz_1.foo(foosball_1.foos);
+(0, foobarbaz_1.foo)(foosball_1.foos);
 // Works with relative file name
 var file_text_1 = require("./file!text");
-foobarbaz_1.foo(file_text_1["default"]);
+(0, foobarbaz_1.foo)(file_text_1["default"]);

tests/baselines/reference/ambientShorthand.js

@@ -20,4 +20,4 @@ exports.__esModule = true;
 var jquery_1 = require("jquery");
 var baz = require("fs");
 var boom = require("jquery");
-jquery_1["default"](jquery_1.bar, baz, boom);
+(0, jquery_1["default"])(jquery_1.bar, baz, boom);