[Bug]: Tagged template with strict private field/method tag has incorrect receiver

[jridgewell]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

While scanning the recent esbuild commits, I noticed evanw/esbuild@f66b586. Essentially, Tagged Template Literals are a special form of function invocation, and we need to preserve the this receiver when invoking:

REPL

class Foo {
  #tag() {
    return this;
  }

  constructor() {
    const receiver = this.#tag`tagged template`;
    console.assert(receiver === this);
  }
}
new Foo();

Configuration file name

babel.config.json

Configuration

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "shippedProposals": true,
        "targets": {
          "chrome": "75"
        }
      }
    ]
  ]
}

Current and expected behavior

Currently, the this.#tag is transformed into _classPrivateMethodGet(this, _tag, _tag2). The return value from _classPrivateMethodGet (which is _tag2, the transformed #tag method) is then invoked via Tagged Template:

const receiver = _classPrivateMethodGet(this, _tag, _tag2)`tagged template`;

In this case, the receiver is undefined, which fails the assertion.

console.assert(receiver === this); // Failure, receiver is undefined

Environment

Babel v7.14.1

Possible solution

When we're transforming a private access in the tag of a Tagged Template Literal, we need to bind the value to the correct receiver:

- const receiver = _classPrivateMethodGet(this, _tag, _tag2)`tagged template`;
+ const receiver = _classPrivateMethodGet(this, _tag, _tag2).bind(this)`tagged template`;

Where this is done is a bit confusing. Transforming is done via transformPrivateNamesUsage, which calls an abstract memberExpressionToFunctions transformer. memberExpressionToFunctions will call several "handler" functions, which are defined in privateNameHandlerSpec.

Currently, the this.#tag`template literal` is invoking the get handler. This fails to preserve the this receiver once transformed. Luckily, there's a similar handler called boundGet which will preserve the receiver.

We actually just need to fix the memberExpressionToFunctions implementation to detect when the get is happening inside a Tagged Template's tag. In this case, we need to call boundGet instead of get. Specifically this line:

babel/packages/babel-helper-member-expression-to-functions/src/index.ts

Lines 465 to 466 in b1f57e5

	// MEMBER -> _get(MEMBER)
	member.replaceWith(this.get(member));

We'll also need

Additional context

This also needs to be applied when transforming a private field, not just a private method:

class Foo {
  #tag() {
    return this;
  }
  
  #tag2 = this.#tag;

  constructor() {
    const receiver = this.#tag`tagged template`;
    console.assert(receiver === this);

    const receiver2 = this.#tag2`tagged template`;
    console.assert(receiver2 === this);
  }
}
new Foo();

[nicolo-ribaudo]

If it is the first time that you contribute to Babel, follow these steps: (you need to have make and yarn available on your machine)

1. Write a comment there to let other possible contributors know that you are working on this bug.
2. Fork the repo
3. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
4. Run yarn && make bootstrap
5. Wait ⏳
6. Run make watch (or make build whenever you change a file)
7. Add a test (only input.js; output.js will be automatically generated. You can also add an exec.js test)
8. Update the code!
9. yarn jest private-methods to run the tests
   • If some test outputs don't match but the new results are correct, you can delete the bad output.js files and run the tests again
   • If you prefer, you can run OVERWRITE=true yarn jest [name-of-the-package-to-test] and they will be automatically updated.
10. If it is working, run make test to run all the tests
11. Run git push and open a PR!

[sag1v]

Can i take this? 🙂

[jridgewell]

@sag1v: Sure

[sag1v]

@nicolas-marien / @jridgewell
I'm kind of confused about how do i test it and where is the test file should be

[jridgewell]

There are a few needed , which will live in the following directories:

Class fields:

• packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template
• packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template-static

Private method:

• packages/babel-plugin-proposal-private-methods/test/fixtures/private-method/tagged-template
• packages/babel-plugin-proposal-private-methods/test/fixtures/private-static-method/tagged-template

Private accessor:

• packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/tagged-template
• packages/babel-plugin-proposal-private-methods/test/fixtures/accessors-static/tagged-template

You can look at the other fixtures in these directories to see an example of the input.js and output.js.

[sag1v]

EDIT

Please Ignore, i was using a wrong nodeJS version

---

@jridgewell Thanks!
Though it seems i can't run the tests, they fail due to lack of configurations?
I haven't made any new tests yet, i just wanted to see that it runs as expected but i get an error when running:

yarn jest private-methods

The error is:

 FAIL  packages/babel-plugin-proposal-private-methods/test/index.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/sagivbengiat/cloned_projects/babel/packages/babel-helper-transform-fixture-test-runner/lib/index.js:357
                ((_task$options = task.options).plugins ?? (_task$options.plugins = [])).push(["external-helpers", {
                                                         ^

    SyntaxError: Unexpected token '?'

       6 | exports.default = _default;
       7 |
    >  8 | var _helperTransformFixtureTestRunner = require("@babel/helper-transform-fixture-test-runner");
         |                                         ^
       9 |
      10 | var _path = require("path");
      11 |

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1327:14)
      at Object.<anonymous> (packages/babel-helper-plugin-test-runner/lib/index.js:8:41)

I was following these steps, without steps 7 and 8 of creating new tests and updating any code.

[nicolo-ribaudo]

Is it possible that you changed your Node.js version after running make bootstrap/make build/make watch? In dev we compile targeting your current Node.js version.

[sag1v]

Is it possible that you changed your Node.js version after running make bootstrap/make build/make watch? In dev we compile targeting your current Node.js version.

Yep, i'm using nvm and i guess a new terminal session picked a lower version.

[sag1v]

@nicolo-ribaudo / @jridgewell
I got 2 major issues :)

1. I wanted to push the code to my fork so you can see my progress and diff and make it is easy for us to talk about my changes (still a WIP) but the pre-commit hook is failing:

Oops! Something went wrong! :(
ESLint: 7.5.0
/Users/.../babel/packages/babel-core/lib/config/index.js:33
return (config == null ? void 0 : config.options) ?? null;
                                                   ^
SyntaxError: Unexpected token '?'

when i run make fix it changes 50+ options.json files..

2. I think i got the fix itself covered but i'm really confused regarding the tests.

I reckon the fix is something like the following:

// packages/babel-helper-member-expression-to-functions/src/index.ts
-465 // MEMBER   ->   _get(MEMBER)
-466 member.replaceWith(this.get(member));
+465 if (t.isTaggedTemplateExpression(parent)) {
+466   // MEMBER   ->   _get(MEMBER).bind(this)
+467   member.replaceWith(this.boundGet(member));
+468 } else {
+469   // MEMBER   ->   _get(MEMBER)
+470   member.replaceWith(this.get(member));
+471 }

And it does pass the exec tests i added (currently only 2 exec and 2 inputs):

// packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template/exec.js
class Foo {
    #tag() {
      return this;
    }
  
    #tag2 = this.#tag;

    constructor() {
      const receiver = this.#tag`tagged template`;
      expect(receiver === this).toBe(true);
  
      const receiver2 = this.#tag2`tagged template`;
      expect(receiver2 === this).toBe(true);
    }
}
  new Foo();

// packages/babel-plugin-proposal-class-properties/test/fixtures/private/tagged-template/input.js

class Foo {
    #tag() {
      return this;
    }
    
    #tag2 = this.#tag;
  }
  new Foo();

And

// packages/babel-plugin-proposal-private-methods/test/fixtures/private-method/tagged-template/exec.js

class Foo {
    #tag() {
      return this;
    }
  
    constructor() {
      const receiver = this.#tag`tagged template`;
      expect(receiver === this).toBe(true);
    }
  }
  new Foo();

//packages/babel-plugin-proposal-private-methods/test/fixtures/private-method/tagged-template/input.js

class Foo {
  #tag() {
    return this;
  }
}
new Foo();

I have doubts regarding these tests, i'm not sure this is the way to write them.
Another thing, i'm not sure i know how to write the tests for all the static fixtures.

[nicolo-ribaudo]

For (1), we should probably update the CONTRIBUTING.md guide but I suggest running make fix-js rather than make fix. Also, you can skip the pre-commit hook by running git commit --no-verify instead of git commit.

For (2), your input.js tests should have a tagged template, since it's what we are fixing. You can write a static test like this:

class Foo {
  static #tag() {
    return this;
  }

  static getReceiver() {
    return this.#tag``;
  }
}

expect(Foo.getReceiver()).toBe(Foo);

Also please yes, open a draft PR so we can discuss there 🙏

[sag1v]

@nicolo-ribaudo Thanks, i will try that make fix-js script and open a draft PR.

I'm a bit confused then, if tests inside input.js contains an expect statement, so what's the difference than an exec.js test.

I guess for this PR i only need input.js files?

[sag1v]

This is the draft
#13395

[nicolo-ribaudo]

The difference is that exec.js tests are executed, while input.js tests are compared against their output.js file.

[sag1v]

The difference is that exec.js tests are executed, while input.js tests are compared against their output.js file.

Ok so expect statement are useless inside input.js right?

[nicolo-ribaudo]

Yup, right