Skip to content

Ensure all evaluated modules have a [[CycleRoot]] #3583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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~.

Verified

This commit was signed with the committer’s verified signature.
nicolo-ribaudo Nicolò Ribaudo
nicolo-ribaudo added a commit to nicolo-ribaudo/engine262 that referenced this pull request May 2, 2025

Verified

This commit was signed with the committer’s verified signature.
nicolo-ribaudo Nicolò Ribaudo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
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
1 participant