Remove unreachable code in InnerModuleEvaluation

[nicolo-ribaudo]

I think the contents of the If are unreachable, so it can be removed.

My reasoning is that:

• If requiredModule(cycle root).[[EvaluationError]] is not ~empty~, then requiredModule(cycle root).[[Status]] must be ~evaluated~
• If requiredModule(cycle root) is evaluated, then also requiredModule(not the root) must be evaluated, because it's a descendant of it (since the cycle-breaking logic puts the cycle root as the root node of the flattened tree)
• If requiredModule(not the root) is evaluated and requiredModule(not the root).[[EvaluationError]] is not ~empty~, InnerModuleEvaluation(requiredModule(not the root)) would have returned that throw completion, and thus we would have returned early due to the ? before the InnerModuleEvaluation call.

[nicolo-ribaudo]

I just checked in engine262, and indeed replacing the If with an assertion keeps al test262 tests as passing.

EDIT: I'm trying to understand if the "Set requriedModule to requiredModule.[[CycleRoot]] step" is useful/correct.

EDIT 2: Yes, that step is necessary, and there is no test262 coverage for it. I'll open a test262 PR.

[guybedford]

If requiredModule(cycle root) is evaluated, then also requiredModule(not the root) must be evaluated

I think it is actually possible for this not to be the case. And the reason for this is that since the very first iteration of this Tarjan algorithm in the spec, error states don't get transitioned across the whole strongly connected component of the cycle due to the early exit that applies here. In fact I think it's an early limitation that that is not the case as it leads to this kind of in-between state behaviour with extra checks needed.

So I believe what is written today is correct though and necessary because of this, short of a refactoring of error states to transition in alignment across a whole strongly connected component.

Here is a counter example:

A -> B
B -> C, D
C -> B
D -> B
E -> D

Where all of the above are synchronous - this case isn't about async graphs but sync ones.

Consider that the error module is C.

Then consider executing A first. In the above there is one strongly connected component - B, C, D with cycle root B if we execute A first. Then which will result in the states:

A: Evaluated, Error
B (cycle root): Evaluated, Error
C: Evaluated, Error
D: ** Unexecuted **
E: Unexecuted

Then if I import B, I will InnerModuleEvaluate(D), which will not be errored, but its cycle root B will be in an errored state, thereby exercising this execution path.

This is just on a very quick review though so please do let me know if I'm missing anything further though.

[nicolo-ribaudo]

@guybedford There is probably some typo somewhere with your example, where you wrote B but probably meant E (you have B twice in the list of modules).

Anyway, assuming that there is no typo, the first time you execute A, the algorithms that run are:

• InnerModuleEvaluation(A)
  • InnerModuleEvaluation(B)
    • InnerModuleEvaluation(C)
      • InnerModuleEvaluation(B)
        • It's already evaluating, early return at step 3
      • Error! -> stop traversing the graph, from step 13 of InnerModuleEvaluation
    • -> propagate the error, return early from step 11.b
  • -> propagate the error, return early from step 11.b
• In the error path of Evaluate(), set A/B/C to evaluated.

So there is no [[CycleRoot]] set, because we always return from InnerModuleEvaluation before getting to step 16.

When then you import again one of B or C (either directly, or through an other entrypoint such as E->D->B), InnerModuleEvaluation(B/C) returns early at step 2.b, thus without ever looking at their dependencies and thus they are never considered again to be in a cycle.

[guybedford]

you wrote B but probably meant E

Thanks I've corrected the first example last listing item for E.

Your response is certainly correct though - because the [[CycleRoot]] is only set with the sync evaluation completion tarjan transition, partial error state cases are never possible when there is a [[CycleRoot]]. I'd add that to the list of constraints which make your assertion true, as it's not an obvious one either at first glance.

The only cases then would be async cases, but I think there might still be a possibility of this happening in the async case as well.

Consider:

A -> B
B -> C, D
C [async] -> B
D -> B
E -> D

Where C is now async and has an async error.

In this case, after the error happens async we have graph states:

A: evaluated, error
B: evaluated, error
C: evaluated, error
D: evaluated, error unset

In this example, D was not in the async parent modules set of C, therefore D does not have an evaluation error set.

When I later import E, it will not see an evaluation error on D, and therefore we need the cycle root check that D does on B to verify there is no evaluation errors. This is since in async error cases evaluation errors only propogate upwards to the cycle root and not out to the entire strongly connected component.

So while you have convinced me we don't need this check in the sync case I'm pretty sure we do still need it in the async case.

[nicolo-ribaudo]

You are right. I didn't realize I was implicitly assuming that if a member of a SCC has an [[EvaluationError]] then the whole SCC has the same [[EvaluationError]] (which is guaranteed for sync cases due the cleanup logic in Evaluation(), but not for async cases).

It is a property that I would like to restore also for the async case, but I'm not sure there is a simple way to do it.

I will add your example as a test262 test case, since we had no coverage for this line.

[nicolo-ribaudo]

@guybedford Wdyt about making the [[Status]]: ~async-evaluated~, [[Status]]: ~evaluated~ and [[EvaluationError]] shared state across a whole SCC, so that a whole SCC transitions at the same time from EVALUATING to EVALUATING-ASYNC, and then all at the same time from EVALUATING-ASYNC to EVALUATED? Similarly to how it happens for the sync case.

It would reduce the possible combinations of states that modules can have in a graph. I would like to get to a point where these assertions are all true, all the time (except for the couple steps where you first change one module and then immediately after the other module):

Given two modules A, B such that B is a transitive dependency of A (i.e. there are modules M₀=A, ..., M_n=B such that M_{i + 1} "∈" M_i.[[RequiredModules]] for all i):

1. if B is linking, A is linking
2. if A is linked, B is linked
3. if B is evaluating, A is evaluating
4. if A is evaluated, B is not evaluating
5. if A is evaluated and B is evaluated, if B.[[EvaluationError]] is not empty then A.[[EvaluationError]] is not empty
6. if B is evaluating-async, A is evaluating or evaluating-async
7. if B is evaluated and B.[[EvaluationError]] is not empty, then A is not evaluating-async

In other words:

• that the ancestors of a module are never "ahead" of the module itself, according to the linking<linked<evaluating<evaluating-async<evaluated order.
• evaluation errors immediately propagate to all ancestors

Currently (1)/(2)/(3) are true, and (4)/(5) are only true for sync graphs.