search

tc39 / test262

Official ECMAScript Conformance Test Suite
Other
2.3k stars 456 forks source link

Avoid duplicate binding import-source-binding-name.js #4121

Closed nicolo-ribaudo closed 2 months ago

nicolo-ribaudo commented 2 months ago

The current test contained a parser error due to the duplicate binding. This splits the test in two files, so that we actually successfully parse the file.

cc @legendecas

Ref https://github.com/babel/babel/pull/16596

woess commented 2 weeks ago

@nicolo-ribaudo I thought the whole point of these tests was that the ImportBinding may be source and from, so why do they expect a SyntaxError then? Isn't this a bug?

nicolo-ribaudo commented 2 weeks ago

It's ensure-linking-error_FIXTURE.js that causes the syntax error. The key bit here is that the error happens in the "resolution" (i.e. linking) phase, and not in the parse phase.

woess commented 2 weeks ago

@nicolo-ribaudo My bad, I missed that part; thanks for the clarification.

Although I still think there's a problem with these tests since AFAIK, the correct evaluation order would be to try to load '<do not resolve>' first before linking ensure-linking-error_FIXTURE.js, so you wouldn't even get to the syntax error. This is also the behavior I see from node, btw:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '<do not resolve>' imported from …/language/module-code/source-phase-import/import-source-binding-name.js

So I think the tests should be changed to point to an existing file, so that the resolution phase will throw the expected SyntaxError.

edit I've opened https://github.com/tc39/test262/issues/4193 to track this issue.

legendecas commented 2 weeks ago

Yes, I found the trick didn't work in my V8 CL as well: https://chromium-review.googlesource.com/c/v8/v8/+/5783137. The failure to read the file happens before the linking failure, which leads to a host defined error, instead of SyntaxError.

legendecas commented 2 weeks ago

The <do not resolve> notation at https://github.com/tc39/test262/pull/3980#discussion_r1646096110 was not documented in INTERPRETING.md, only <module source> was documented. I will add a note in the doc.