Skip to content

Conversation

nicolo-ribaudo
Copy link
Member

This PR fixes #3582 by defaulting [[CycleRoot]] to the module itself in case of error.

In case the module is effectively on a cycle, this would be lying, but in practice it doesn't matter: when a module has an error, the error is immediately propagated to all its ancestors, so the real cycle root has the same error state.

I originally tried to set [[CycleRoot]] properly, which requires more complexity. This code that sets the states of the modules in stack in case of error, needs to:

  • first, iterate the stack reversed, propagating setting [[DFSAncestorIndex]] of each module to min([[DFSAncestorIndex]] of module, [[DFSAncestorIndex]] of its child)
  • then, when iterating in order, it needs to set [[CycleRoot]] to the module itself unless the [[DFSAncestorIndex]] is the same as the previous module, in which case it copies the [[CycleRoot]] from it.

While this complex approach is more correct, I discarded it because it's still not 100% correct. In this graph, assuming that E throws, we will have not traversed F at that point so we don't know that D should actually have [[CycleRoot]] set to C.

In https://github.com/nicolo-ribaudo/ecma262/tree/cycleroot-fix-ugly I have an alternative fix if you prefer, that instead of ensuring that [[CycleRoot]] is always set it acknowledges that in this specific case it will be ~empty~.

@bakkot bakkot added the editor call to be discussed in the next editor call label Sep 22, 2025
@syg
Copy link
Contributor

syg commented Sep 29, 2025

Editors' call: We prefer the alternative fix at 09d32a3 for clarity, since it immediately answers the question of what [[CycleRoot]] is used for for the error case.

@guybedford Can we ask a favor for you to review the alternative fix being equivalent to this one?

@guybedford
Copy link

@syg sure I'd be happy to do a careful review on this. Will aim to follow-up soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor call to be discussed in the next editor call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[[CycleRoot]] is expected on all evaluated modules, but it's not set on modules that throw synchronously
4 participants