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

Do not expect specific error type in import-attributes/allow-nlt-before-with.js #3987

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 11, 2024

Hosts are free to throw whatever error they want during module loading: there is no guarantee that the error thrown due to the import with type: "foo" is a SyntaxError (it could also be, for example, an Error caused by a network failure). Technically this is also true when loading just JS, but not specifying type reduces variance and in practice all test262 runners will not fail when loading JS modules that exist :)

@nicolo-ribaudo nicolo-ribaudo requested a review from a team as a code owner January 11, 2024 15:37
@ptomato
Copy link
Contributor

ptomato commented Jan 13, 2024

Is my understanding here correct? Thinking of HTML hosts as an example, type is a supported import attribute, so the SyntaxError will not be thrown in InnerModuleLoading step 2.d.i.1. Instead a TypeError will be thrown in onSingleFetchComplete step 2, failing the test.
After this change, the TypeError in onSingleFetchComplete step 2 is not thrown, and instead the line import "./ensure-linking-error_FIXTURE.js"; throws the SyntaxError.

However, how do we distinguish between the SyntaxError from import "./ensure-linking-error_FIXTURE.js"; and the SyntaxError that would be thrown if the import declaration were instead parsed as an import declaration + with-statement, which would be a SyntaxError in module code?

@nicolo-ribaudo
Copy link
Member Author

Is my understanding here correct? Thinking of HTML hosts as an example, type is a supported import attribute, so the SyntaxError will not be thrown in InnerModuleLoading step 2.d.i.1. Instead a TypeError will be thrown in onSingleFetchComplete step 2, failing the test.
After this change, the TypeError in onSingleFetchComplete step 2 is not thrown, and instead the line import "./ensure-linking-error_FIXTURE.js"; throws the SyntaxError.

This is all correct. Before this PR, hosts might throw an error due to the import attributes (either because they don't support type, or `

Before this PR, there where three possible cases:

  • the host does not support the type attribute, so InnerModuleLoading throws a SyntaxError
  • the host doesn't know how to handle that specific type, so it throws some error in HostLoadImporterModule
  • loading works, so we ensure a SyntaxError error in ensure-linking-error_FIXTURE.js

This PR removes the first two cases, ensuring a consistent error.

If a host doesn't parse this properly, it would throw in the parse phase rather than in the resolution phase.

…re-with.js

Hosts are free to throw whatever error they want during module loading: there is no guarantee that the error thrown due to the import with `type: "foo"` is a SyntaxError.
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.
I was going to ask, why don't we remove the ensure-linking-error_FIXTURE.js import, but then we don't actually have a way to say "this test is only for the parsing phase" other than causing an intentional error in the resolution phase. So, fine with me.

@ptomato ptomato enabled auto-merge (rebase) January 15, 2024 23:16
@ptomato ptomato merged commit bebce4a into main Jan 15, 2024
9 checks passed
@ptomato ptomato disabled auto-merge January 15, 2024 23:16
@ptomato ptomato deleted the nicolo-ribaudo-patch-1 branch January 15, 2024 23:16
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.

2 participants