thenables in object literals erroneously dropped with accompanying async/await/Promise

[kzc]

• Rollup Version: rollup v2.50.3
• Operating System (or Browser): n/a
• Node Version (if applicable): v16.1.0

This is an obscure corner of the ECMAScript specification - thenables are similar to Promises:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve#resolving_thenables_and_throwing_errors

Repro

2.50.3 REPL

$ cat then.js
const effects = [], effect = x => effects.push(x);
Promise.resolve({
    then() {
        effect(1);
    }
});
(async function() {
    return {
        then() {
            effect(2);
        }
    };
})();
(async function() {
    await ({
        then: function() {
            effect(3);
        }
    });
})();
(async function() {
    await ({
        get then() {
            effect(4);
            return () => effect(5);
        }
    });
})();
(async function() {
    await ({
        then() {
            effect(6);
        }
    });
})();
(async function() {
    await await ({
        then(resolve) {
            resolve({
                then() {
                    effect(7);
                }
            });
        }
    });
})();
async function action(x) {
    return x;
}
action({}); // no side effects - may be dropped
var promise = action(8);
action({
    then(success, fail) {
        success(effect(9));
    }
});
promise.then(x => effect(x));
action({
    then(resolve) {
        resolve({
            then() {
                console.log(effects.join(" "));
            }
        });
    }
});

Expected Behavior

$ cat then.js | node
4 1 2 3 5 6 9 8 7

Actual Behavior

$ cat then.js | rollup --silent | node
$ 

$ cat then.js | rollup --silent
const effects = [], effect = x => effects.push(x);
async function action(x) {
    return x;
}
var promise = action(8);
promise.then(x => effect(x));

Edit 2: test case was further refined.

[lukastaegert]

That is unfortunately a known issue, but good to track here nonetheless.

[kzc]

Sorry, I had missed #2915. This issue can be closed as a dup if you like.

I think the only scenarios of concern are anything that could create a Promise like async return, await and of course Promise itself and its methods. Couldn't the recently introduced object mutation tracker handle something like this? If a then property is seen that happens to be of type function, always assume it induces a side effect in the aforementioned cases?

[lukastaegert]

I actually closed the other one as your tests are much more comprehensive.

[lukastaegert]

If a then property is seen that happens to be of type function, always assume it induces a side effect in the aforementioned cases?

That is actually tricky i.e. we cannot easily "scan" an object for properties from another node. A proper fix would likely mean introducing a new kind of effect (e.g. "hasEffectWhenAwaited"), at which point we might likely want to rework the effect system to work more like deoptimizations with an "event" parameter ("hasEffectWhenIterated" would be something else which would be nice to have so that we can eventually remove for-of loops). Will need to think about this.

[kzc]

I see.

I wonder if a quick hack could be implemented to pessimize the common uses of then properties generally - for both object literals and when explicitly set. It might not affect much code as then method calls at least always appear to be retained now - even if side effect free:

$ rollup -v
rollup v2.50.3

$ echo 'Promise.resolve(12); Promise.resolve(34).then(()=>{});' | rollup --silent
Promise.resolve(34).then(()=>{});

The test case in the top post was further refined.

[kzc]

As for the example in #2915:

var count = 0;
var thenable = Object.defineProperty({}, 'then', { get: function () { count += 1; } });
Promise.resolve(thenable);
assert.equal(count, 1);

if Promise.resolve has an argument of unknown composition such as the result of Object.defineProperty() it ought to assume the worst - that it might contain a then property and retain the call.

[lukastaegert]

Fix at #4115