[Bug]: Omitted spread properties are incorrectly hoisted when using computed properties with template strings

[tidychips]

💻

• Would you like to work on a fix?

How are you using Babel?

babel-loader (webpack)

Input code

const example = (obj) => {
  const foo = 'foo';
  const { [`prefix_${foo}`]: _, ...rest } = obj;
};

REPL Link

Configuration file name

No response

Configuration

The copied config is a bit over complicated to reproduce the issue. The repl link above has a minimal config that highlights the issue better.

{
  "assumptions": {},
  "compact": false,
  "sourceMaps": true,
  "sourceType": "unambiguous",
  "cwd": "/Users/tidychips/repos/repo",
  "filename": "/Users/tidychips/repos/repo/path/to/file",
  "sourceFileName": "/Users/tidychips/repos/repo/path/to/file",
  "targets": {},
  "cloneInputAst": true,
  "babelrc": false,
  "configFile": false,
  "browserslistConfigFile": false,
  "passPerPreset": false,
  "envName": "development",
  "root": "/Users/tidychips/repos/repo",
  "rootMode": "root",
  "plugins": [
    [
      "[Function: function _default(_ref) {\n  var t = _ref.types;\n   ... ]",
      {
        "pure": false,
        "minify": false,
        "transpileTemplateLiterals": false
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "corejs": false,
        "helpers": true,
        "regenerator": true,
        "useESModules": false
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "legacy": true
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "loose": true
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "loose": true
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "loose": true
      }
    ],
    "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
    "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
    "[Function: function _default() {\n  let changed = [];\n  /**\n   ... ]"
  ],
  "presets": [
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "modules": "commonjs",
        "targets": {
          "chrome": "73",
          "edge": "17",
          "firefox": "68",
          "ie": "11",
          "ios": "11",
          "safari": "11.1"
        },
        "useBuiltIns": "entry",
        "corejs": 3,
        "exclude": [
          "transform-typeof-symbol"
        ],
        "debug": false
      }
    ],
    [
      "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
      {
        "development": true,
        "useBuiltIns": true
      }
    ],
    "[Function: (api, options, dirname) => {\n    var _clonedApi2;\n ... ]",
    "/Users/tidychips/repos/repo/node_modules/@m/babel-preset-companyname/src/index.js"
  ]
}

Current and expected behavior

Current Behavior: The change made in #13384 will incorrectly hoist omitted template string properties to the top level scope of the file when they contain interpolations using locally scoped variables. This causes the file to throw a ReferenceError when imported because the variable is not defined in the top level scope.

Expected Behavior: Do not hoist omitted keys when they depend on locally scoped variables

Environment

  System:
    OS: macOS 10.15.7
  Binaries:
    Node: 12.22.1 - ~/.nvm/versions/node/v12.22.1/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.12 - ~/.nvm/versions/node/v12.22.1/bin/npm
  Monorepos:
    Yarn Workspaces: 1.22.10
    Lerna: 4.0.0
  npmPackages:
    jest: 26.6.3 => 26.6.3 
    lerna: 4.0.0 => 4.0.0 

Possible solution

No response

Additional context

No response

[babel-bot]

Hey @tidychips! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

[nicolo-ribaudo]

The problem is that in the else-if branch of

babel/packages/babel-plugin-transform-destructuring/src/index.js

Lines 226 to 243 in 4eda1c8

	if (!allLiteral) {
	keyExpression = t.callExpression(
	t.memberExpression(keyExpression, t.identifier("map")),
	[this.addHelper("toPropertyKey")],
	);
	} else if (!t.isProgram(this.scope.block)) {
	// Hoist definition of excluded keys, so that it's not created each time.
	const program = this.scope.path.findParent(path => path.isProgram());
	const id = this.scope.generateUidIdentifier("excluded");
	program.scope.push({
	id,
	init: keyExpression,
	kind: "const",
	});
	keyExpression = t.cloneNode(id);
	}

we hoist the computed keys if they are all literals.

As this issue shows, it is a wrong approach: we should only do that if we only have literals that do not reference any other variable. The easiest way to do so is probably to add a new variable allPureLiterals, and set it to false not only when we find a generic computed key, but also when we find a key that is a template literal with > 0 child expressions.

It also needs to be fixed at

babel/packages/babel-plugin-proposal-object-rest-spread/src/index.js

Line 182 in 4eda1c8

if (!t.isProgram(path.scope.block)) {

.

[nicolo-ribaudo]

@tidychips As a workaround, you can manually extract the computed key:

const k = `prefix_${foo}`;
const { [k]: _, ...rest } = obj;

Or you can downgrade @babel/plugin-transform-destructuring to 7.13.17 and @babel/plugin-proposal-object-rest-spread to 7.14.2

[tony-go]

✋ I'll be happy to give a try ^^

[jridgewell]

Ugh, I hate that we called `foo` and `foo${bar}` "literals". They're a template expressions. TC39 actually rejected expanding string literals to include expression-less templates (eg, const o = { `foo`: true } isn't valid).

@tony-go: There are some related code locations at:

• babel/packages/babel-plugin-transform-destructuring/src/index.js

  Lines 207 to 208 in 4eda1c8

  	} else if (t.isTemplateLiteral(prop.key)) {
  	keys.push(t.cloneNode(prop.key));

• babel/packages/babel-plugin-proposal-object-rest-spread/src/index.js

  Lines 103 to 104 in 4eda1c8

  	} else if (t.isTemplateLiteral(prop.key)) {
  	keys.push(t.cloneNode(prop.key));

Be careful that you do not use isPure helper when checking the template, we since templates which reference constant values are "pure" but still unhoistable. Eg, https://astexplorer.net/#/gist/5f2607e8caf62da7d72bb1044c8f102b/0c0bdb994cc263ee0c9e2dcb54bc2cab3d39229b. Instead, we should just deopt if the template has any expressions.

[tony-go]

Hi Guys 👋

Thanks a lot for your insights.

@jridgewell Can you give me more details about:

Instead, we should just deopt if the template has any expressions.

I'm not familiar with all terms (deopt) yet ^^

[jridgewell]

Deopt means de-optimize. We currently have an optimization that omitted keys can be hoisted if they're allLiteral, but we need to add a de-optimization if we find that one of the "literal" keys is actually a template with an expression (these aren't really "literal").

So, essentially:

if (allLiteral && !hasTemplateExpression) {
  // optimize by hoisting
} else {
  // Do nothing, ie, deopt
}

[tony-go]

Hi guys 👋

Just to keep you informed that I'm currently working on the PR #13419

An it's seems more complicated than we expected.

Hopefully @JLHwung helps me a lot ^^

Note: If you consider that i'm too slow, feel free to reassign it. 🤟

[nicolo-ribaudo]

@lala7573 Do you want to work in this? Ideally we can release a fix in the next patch release.

[lala7573]

@nicolo-ribaudo
I just fixed not to generate hoisted. As jridgewell mentioned, I add a new flag named hasTemplateExpression.
I'm thinking about another corner case... If it is ok, I'm going to make a new PR.

main...lala7573:fix-13430

[nicolo-ribaudo]

If there are only literals (either template or not) we don't need to call babelHelpers.toPropertyKey.

[lala7573]

@nicolo-ribaudo
Oh now I understand... I fix it.
main...lala7573:fix-13430

[nicolo-ribaudo]

Please open a PR, it's easier to review and comment!

[jridgewell]

This still needs to be fixed in object-rest-spread:

babel/packages/babel-plugin-proposal-object-rest-spread/src/index.js

Lines 103 to 104 in 4eda1c8

	} else if (t.isTemplateLiteral(prop.key)) {
	keys.push(t.cloneNode(prop.key));

REPL