-
Notifications
You must be signed in to change notification settings - Fork 464
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
Avoid duplicate binding import-source-binding-name.js #4121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, thanks!
test/language/module-code/source-phase-import/import-source-binding-name-2.js
Show resolved
Hide resolved
test/language/module-code/source-phase-import/import-source-binding-name.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
@nicolo-ribaudo I thought the whole point of these tests was that the |
It's |
@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
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 #4193 to track this issue. |
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. |
The |
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 babel/babel#16596