1708812 - Adding private fields to an object is side-effectful and should be marked as such, Redux

Status: NEW

(DevTools :: Console, defect, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

This bug is a clone of Bug 1651420, which was fixed, and then broken (silently, due to an error in the activation of the xpcshell-test that was written for Bug 1651420) by 1653567.

That test case, with some more investigation also seems to be in error.

---

Adding a private field to an object is side-effectful and should block instant-evaluation.

Assuming you have already executed this in the console:

class Base {
  constructor(o) { return o; } 
}; 

class A extends Base { 
   #x 
} 
var obj = {}; 

Then typing new A(obj) should not be evaluated, as it would mutate obj. Then subsequently, if you hit enter, it would issue an error as the object would already have the field, which is a TypeError with private Fields.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

I tweaked the test case a bit:

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

"use strict";

/*
 * Check that private field initialization is marked as effectful.
 */

add_task(
  threadFrontTest(async ({ threadFront, targetFront, debuggee }) => {
    const consoleFront = await targetFront.getFront("console");
    const initial_response = await consoleFront.evaluateJSAsync(`
      // This is a silly trick that allows stamping a field into
      // arbitrary objects.
      class Base { constructor(o) { return o; }};
      class A extends Base {
        #x = 10;
      };
      var obj = {};
    `);
    // If an exception occurred, private fields are not yet enabled. Abort
    // the rest of this test.
    if (initial_response.hasException) {
      return;
    }

    // Try eager evaluation, ensure we don't have any result.
    let eagerResult = await consoleFront.evaluateJSAsync("new A(obj)", {
      eager: true,
    });

    console.log(eagerResult.result.type)
    Assert.equal(
      eagerResult.result.type,
      undefined,
      "shouldn't have actually produced a result"
    );

    // Do it again, make sure we didn't stamp obj.
    eagerResult = await consoleFront.evaluateJSAsync("new A(obj)", {
      eager: true,
    });

    console.log(eagerResult.result.type)

    Assert.equal(
      eagerResult.result.type,
      undefined,
      "shouldn't have actually produced a result"
    );

    const timeoutResult = await consoleFront.evaluateJSAsync("f(); 1", { eager: false });
    console.log(timeoutResult.result)
    Assert.equal(timeoutResult.result, 1, "normal eval, no throw.");
  })
);

So the first eager evaluation reports result.type == undefined; the second execution has result.type == "undefined";

Comment 2

[ljharb]

It seems to me that adding private fields to an object is not changing a property of the object, in the same way as adding an object as a key in a WeakMap is not changing a property of the object (unless you're talking about FF internals that I'm not familiar with)

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Stamping private fields twice throws, so it's definitely effectful:

class Base { constructor(o) { return o; } };
class A extends Base {
    #x = 10;
};
var obj = {};

new A(obj);
print("once")
new A(obj) // Throws
print("twice")

So, given that, we cannot eagerly evaluate new A(obj) in devtools, or else, contrary to user expectation, when they hit enter, they'll get an exception thrown.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Error messages for reference:

matthew@xtower:~/unified$ ./mach run --enable-private-fields /tmp/t.js
 0:00.62 /home/matthew/unified/obj-opt-shell-x86_64-pc-linux-gnu/dist/bin/js --enable-private-fields /tmp/t.js
once
/tmp/t.js:3:5 TypeError: Initializing an object twice is an error with private fields
Stack:
  @/tmp/t.js:3:5
  A@/tmp/t.js:2:1
  @/tmp/t.js:9:1
matthew@xtower:~/unified$ v8 /tmp/t.js 
once
/tmp/t.js:3: TypeError: Identifier '#x' has already been declared
    #x = 10;
         ^
TypeError: Identifier '#x' has already been declared
    at Object.<instance_members_initializer> (/tmp/t.js:3:10)
    at new A (/tmp/t.js:2:1)
    at /tmp/t.js:9:1

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Side note: This eager evaluation bug applies mostly to a peculiar construct in JS; modifying the checks to make sure this is blocked for eager evaluation might end up blocking more functionality than we'd like, and so it would have to be approached cautiously. I'd seriously consider leaving this as low priority