Skip to content
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

Only use phase: parse for errors that happens while parsing the test entry point #4124

Open
nicolo-ribaudo opened this issue Jun 29, 2024 · 2 comments

Comments

@nicolo-ribaudo
Copy link
Member

https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#negative

For a file with no dependencies, it's obvious that parse means that the error is thrown when calling ParseModule on its source code.

On the other hand, what happens when there are multiple files involved is less clear. Resolution of module dependencies is not defined in ECMA-262, but in HTML. Before the layering changes from 2022, there was a HostResolveImportedModule host hook that was responsible for loading/resolution of modules. There is also a ResolveExport AO, that might map to this "test262" resolution phase.

When running a test, there four possible phases:

  1. module = ParseModule(): parse the entry module
  2. module.LoadRequestedModules(): resolve, load and parse its dependencies
  3. module.Link(): resolve the various exported and imported bindings
  4. module.Evaluate(): executes the module and its dependencies

Before the 2022 layering changes, 1 and 2 where mixed together from an ECMA-262 point of view.

Looking at current tests, it seems like:

  • (1) is covered by phase: parse
  • (3) is covered by phase: resolution
  • (4) phase: evaluation

There is some confusion about (2):

Using phase: parse for tests thrown during (2) prevents most JavaScript parsers from successfully using test262 as a test suite for parsing tests, since those projects would just call ParseModule( test source ) and would not proceed to (2)/(3)/(4). On the other hand, using phase: resolution for it seems wrong, since module resolution happens outside of ecma262 and thus we are probably testing binding resolution.

I propose that we clarify the following:

  1. module = ParseModule() throws at phase: parse
  2. module.LoadRequestedModules() doesn't have a corresponding phase, and we will make sure to write no tests that throw at this point.
  3. module.Link() throws at phase: resolution
  4. module.Evaluate() throws at phase: evaluation
@ptomato
Copy link
Contributor

ptomato commented Jul 4, 2024

I think we should discuss this in the next maintainers meeting.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 14, 2024

Proposal:

  1. Clarify the documentation
  2. Use phase: resolution for test/language/import/import-assertions/json-invalid.js since we couldn't figure a use case to distinguish between your steps 2 and 3
  3. Rewrite test/language/module-code/source-phase-import/import-source-binding-name.js as a fully passing test case by replacing '<do not resolve>' by a real fixture

@nicolo-ribaudo does this make sense to you?

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

No branches or pull requests

3 participants