1797736 - eager evaluation fails when inner function uses variable from outside its scope

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Given

function test() {
    const level = 0;
    return Object.keys({a: 1}).map(() => level)
}

the eager evaluation of test() is bailing, even though it's not stateful

changing test to

function test() {
    return Object.keys({a: 1}).map(() => {
        const level = 0;
        return level;
    })
}

the eager evaluation of test() does return the expected value ([0]);

Comment 1

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

arai, would you know what's happening here?

Comment 2

[Tooru Fujisawa [:arai]]

the first case bails because of JSOp::InitAliasedLexical for const level = 0;.

https://searchfox.org/mozilla-central/rev/73ff5f1ac45202c4621a1bcee4b35d20027e71ad/js/src/debugger/Script.cpp#1394-1395,1415,1426

static bool BytecodeIsEffectful(JSOp op) {
  switch (op) {
...
    case JSOp::InitAliasedLexical:
...
      return true;

if a variable is closed over (referred from inner functions), it's marked as "aliased",
and a modification, including initialization, for aliased variables is currently considered effectful,
even if the variable doesn't escape the scope of the eager evaluation.

for the second case, level variable isn't aliased, and JSOp::InitLexical is used instead, which isn't marked effectful.

https://searchfox.org/mozilla-central/rev/73ff5f1ac45202c4621a1bcee4b35d20027e71ad/js/src/debugger/Script.cpp#1394-1395,1494,1636

static bool BytecodeIsEffectful(JSOp op) {
  switch (op) {
...
    case JSOp::InitLexical:
...
      return false;

Then, we might be able to mark JSOp::InitAliasedLexical also non-effectful, unless there's some case that an aliased variable can be initialized beyond the "local" scope.
I'll look into the details.

Comment 3

[Tooru Fujisawa [:arai]]

So far, there's a case that JSOp::InitAliasedLexical can be used across function

  class A extends class {} {
    constructor() {
      var fun = async() => {
        super();  // <= InitAliasedLexical ".this" (hops = 1, slot = 2)
      };
      fun();
    }
  }
  new A();

I'll check if such case can be interacted with eager evaluation context.

If eager evaluation cannot mess it up, we could mark JSOp::InitAliasedLexical as non-effectful.
otherwise, I think we could mark JSOp::InitAliasedLexical with hops == 0 as non-effectful.

Comment 4

[Tooru Fujisawa [:arai]]

given that JSOp::InitAliasedLexical is used for multiple purposes, especially for new features such as private fields,
it's better not treating hops != 0 case non-effectful, to avoid accidental side-effect.

I'll make hops == 0 cases non-effectful.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1797736 - Part 1: Mark JSOp::InitAliasedLexical with hops == 0 as non-effectful. r?tcampbell!

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1797736 - Part 2: Remove obsolete comment about async function and microtask checkpoints. r?tcampbell!

Eager evaluation already supports async function, and supporting it there isn't
necessarily related to microtask checkpoints.

Depends on D161017

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1797736 - Part 3: Add testcase for eager evaluation with InitAliasedLexical with hops == 0. r?nchevobbe!

Depends on D161018

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1355104f54da
Part 1: Mark JSOp::InitAliasedLexical with hops == 0 as non-effectful. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/40cefa0a6d1b
Part 2: Remove obsolete comment about async function and microtask checkpoints. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/b4b49813278c
Part 3: Add testcase for eager evaluation with InitAliasedLexical with hops == 0. r=nchevobbe

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1355104f54da
https://hg.mozilla.org/mozilla-central/rev/40cefa0a6d1b
https://hg.mozilla.org/mozilla-central/rev/b4b49813278c