[Bug]: plugin-transform-block-scoping constant checks treats deconstruction incorrectly

[overlookmotel]

💻

• Would you like to work on a fix?

How are you using Babel?

@babel/cli

Input code

const c = 0;
let x = 1, y = 2;
try {
  [x, c, y] = [11, 0, 22];
} catch (e) {
  console.log( { x, y } );
}

REPL

Configuration file name

No response

Configuration

No response

Current and expected behavior

[x, c, y] = [11, 0, 22] is transformed to [11, 0, 22], _readOnlyError("c").

Original logs {x: 11, y: 2} whereas transpiled code logs {x: 1, y: 2}. It should assign to x before throwing, but not assign to c or y.

Environment

Babel REPL

Possible solution

Best solution I can see is:

// Assignments to `c` and `y` have been removed
[x] = [11, 0, 22], _readOnlyError("c");

If there's a nested object/array on the receiving end of the assignment, all elements before the const-violating identifier should be retained, and all after it removed - at all levels of the nested object/array.

i.e.: if c is a const:

Input: [ p, q, { r: [ ...s ], t: [ u, c, v ], w, ...x }, y, ...z ] = arr;
Output: [ p, q, { r: [ ...s ], t: [ u ] } ] = arr, _readOnlyError("c");

(note c, v, w, x, y and z have all been removed)

NB c may also have a default e.g. { c = 1 } = {}

Additional context

#13248 fixed some other cases where const violations were transformed incorrectly, but didn't fix this one.

[babel-bot]

Hey @overlookmotel! 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."

[overlookmotel]

Actually, solution is a bit more complex due to getters. e.g.:

let getterIsCalled = false;
const obj = {
  get x() {
    getterIsCalled = true;
  }
};

try {
  const c = 1;
  ( { x: c } = obj );
} catch (e) {
  console.log( getterIsCalled );
}

This logs "true".

So, to make sure getter is called before throwing, transformation would need to be:

var c = 1;
var _c;
( { x: _c } = obj ), _readOnlyError("c");

[overlookmotel]

Same bug with imports which are assigned to in object/array deconstruction in plugin-transform-modules-commonjs. e.g.: this is not transformed correctly:

import c from 'foo';
let x = 0;
[ x, c ] = [ 1, 2 ];

[overlookmotel]

There is one rather ugly solution which would be easier to implement.

Input:

const c = 1;
let x = 0, y = 0;
( { x, c, y } = { x: 1, c: 2, y: 3 } );

Output:

function _readOnlyErrorObject(name) {
  return {
    set a(v) {
       _readOnlyError(name);
    }
  };
}

var c = 1;
var x = 0, y = 0;
( { x, c: _readOnlyErrorObject("c").a, y } = { x: 1, c: 2, y: 3 } );

This does behave correctly - the error only gets thrown when the setter is triggered, so x is assigned to, y is not, and any getter on the c property of right-hand object is called before throwing.

If you'd be willing to accept such an ugly solution, I'd be willing to make a PR.

[nicolo-ribaudo]

Would reusing the existing helper work?

var c = 1;
var x = 0, y = 0;
[ x, _readOnlyError("c")._, y ] = [ 1, 2, 3 ];

._ is just needed to have a syntactically valid code, it's never actually evaluated.

[overlookmotel]

@nicolo-ribaudo No. I tried that. Problem is that _readOnlyError("c") gets evaluated before the property getter on object on right-hand side gets called. So the error is thrown prematurely. Using a setter on the object being assigned to is the only way around it I could find.

I mean:

const obj = {
  get c() {
    console.log('getter called');
  }
};

const c = 0;
let x;
( {x, c} = obj );

The original logs "getter called" but if _readOnlyError("c")._ is substituted for c, it doesn't.

[nicolo-ribaudo]

Oh ok. Using a new helper looks good to me then (I'm working on almost the same thing but for private methods, and I didn't notice that readOnlyError()._ doesn't work).

[overlookmotel]

Well it doesn't work in Node anyway.

Stripping out assignments which will never be made is a cleaner solution in terms of producing more minimal output:

Input:

const c = 0;
let x, y;
[ x, c, y ] = [ 1, 2, 3 ];

Output:

var c = 0;
var x, y;
var _c;
[ x, _c ] = [ 1, 2, 3 ], _readOnlyError("c");

This also works perfectly. It's just that it'd be a pain to implement handling when the object/array on the left is nested - [ x, [ { y, c }, ...z ], q, v ] etc.

[nicolo-ribaudo]

I prefer the new runtime helper: even if it generates more code at runtime, this is an edge case and I don't think it's worth the implementation complexity.

[overlookmotel]

OK great. I'll get onto that at some point. I'm afraid it may be a long time before I do though - really busy with work at the moment.

[overlookmotel]

By the way, I was intending to amend plugin-transform-modules-commonjs to reuse the _readOnlyError helper. Currently it produces this function to throw an error inline every time.

However, I couldn't figure out how to access state.addHelper() deep within https://github.com/babel/babel/blob/main/packages/babel-helper-module-transforms/src/rewrite-live-references.ts Any ideas?

[nicolo-ribaudo]

It's a change that we should do, but the answer is not nice 😛

You need to pass state/file as a parameter to @babel/helper-module-transforms, but also keep a fallback for when state is not passed (otherwise it would be a breaking change)

[overlookmotel]

Uck. I generally consider that the documentation is the public API contract. In this case, it is... minimal.

https://babeljs.io/docs/en/babel-helper-module-transforms

[nicolo-ribaudo]

It doesn't matter, every older version of @babel/plugin-transform-modules-commonjs would break if we add a new required parameter.