Adjusted execution for circular stacks

[guybedford]

I've included a sample spec diff here to alter execution order of Source Text Module Records to ensure an invariant for Abstract Module Records in a circular reference scenario.

What is Specified

In the following case:

a.mjs

import './b.mjs';
export * from './x.mjs';
console.log('a exec');

b.mjs

import * as a from './a.mjs';
console.log('b exec');
console.log(a);

x.mjs

export var p = 5;
console.log('x exec');

If you run this today executing a.mjs you get the output:

b exec
[Module] { p: undefined }
x exec
a exec

The proposal here is to change the above execution order in the spec to rather look like:

x exec
b exec
[Module] { p: 5 }
a exec

That is, when there is a circular reference, to execute modules that are not part of the cycle first, before executing the cycle module.

The specification diff provided here achieves that.

In the process, it does seem like it would make circular execution more useful in that non-circular dependencies are defined.

Why is this necessary

If you imagine that x.mjs is a dynamic module record, then we have inspected its export list (the module log above), before the module x has finished executing. This is a problem for dynamic modules as we change their export list after execution, so that a partial namespace must not be observable.

Why can't a partial namespace be observable?

It could certainly be an alternative to just accept that you could observe as a user partially-defined dynamic module exports, as this is quite an extreme edge case, but in the name of a spec actually being well defined it seems better to fix the problem.

How does the fix work?

We already track cycles uniquely in the spec using the DFSAncestorIndex. A unique DFSAncestorIndex corresponds to a unique sub-cycle of the graph. A graph without any cycles will have a single DFSAncestorIndex always matching the DFSIndex.

The DFSAncestorIndex is there to track that marking module records as "evaluated" is done on the correct execution subgraphs. This marking exactly happens in the right order we need, so moving execution to happen just like this same marking algorithm on the subset of the execution stack provides the fix.

//cc @bmeck @caridy

[ljharb]

What does this mean for the execution order in a graph when a cyclic dependency gets added?

[guybedford]

@ljharb it's as if the module in the cycle were imported last, even if it was imported first in terms of the order of import statements - that's the only execution ordering difference.

[ljharb]

I’m more wondering if that’s too surprising a change - it can be very hard to know when you’ve added a circular dep.

[caridy]

Yes, that will never fly IMO. I have the same question, why is that the case? In our discussions, we never talked about changing the order. Can you provide more details? Why is export * from './x.mjs'; that important here?

[guybedford]

@caridy what aspect of the ordering change do you think is untenable? The reason is that if x.mjs is a dynamic module and not a source text module, then because of the circular reference execution we get the export names of x exported through that export * indirection before x is itself executed. As a result it is possible to observe the partially defined export list state, which will include only what has been imported, but not everything that will be defined on execution. This may be acceptable. Or we could consider disabling export * for dynamic modules. This execution approach stops it from being possible to inspect the partial state.

…

On Fri, 20 Jul 2018 at 20:26, Caridy Patiño ***@***.***> wrote: Yes, that will never fly IMO. I have the same question, why is that the case? In our discussions, we never talked about changing the order. Can you provide more details? Why is export * from './x.mjs'; that important here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkiyqGG4dXXYRLEwnttLe1Jb0P113ctks5uIiDegaJpZM4VYR-I> .

[guybedford]

An alternative approach here might be to specifically move the creation of module namespaces to dynamic modules to only be marked as "valid" after execution, and treat early execution access to these as a TDZ reference error. The behaviour for normal module records would behave the same so we could do this with a flag.

Will aim to get a new PR up with the approach to iterate on further.

[guybedford]

Abandoned in favour of #4 which has been merged here.