Skip to content

Test export * as x from and import * as x; export { x } binding equivalence #4606

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 26, 2025
Merged

Test export * as x from and import * as x; export { x } binding equivalence #4606

merged 2 commits into from
Nov 26, 2025

Conversation

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Oct 28, 2025 •
edited
Loading

This PR adds tests for tc39/ecma262#3715.

The first commit just moves the tests related to binding ambiguity to a single folder, so that it's easier to tell what is already tested.

@nicolo-ribaudo nicolo-ribaudo force-pushed the ambiguous-binding branch 7 times, most recently from 0ae13c9 to ae7fc83 Compare October 28, 2025 10:39
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review October 28, 2025 10:41
@nicolo-ribaudo nicolo-ribaudo requested a review from a team as a code owner October 28, 2025 10:41
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 28, 2025

SM/JSC pass all the tests, while V8/engine262/XS fail test/language/module-code/ambiguous-export-bindings/namespace-unambiguous-if-export-start-as-from.js

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 28, 2025

Marking as draft because I plan to present something related to this in plenary.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft October 28, 2025 10:45
@ptomato ptomato added the awaiting consensus This needs committee consensus before it can be eligible to be merged. label Oct 29, 2025
@guybedford
Copy link

guybedford commented Oct 30, 2025 •
edited
Loading

Hmm, what is the value in allowing export * as foo to not be an ambiguous error? Is it not just an oversight that this doesn't throw as an ambiguous case? It doesn't seem to match the expectations for other unambiguous cases to me.

@guybedford guybedford mentioned this pull request Nov 6, 2025
Open
@bakkot
Copy link
Member

bakkot commented Nov 24, 2025 •
edited
Loading

I believe the behavior here has consensus (looks like it's asserting the post-tc39/ecma262#3715 behavior). @nicolo-ribaudo did you want to do any further updates or is this ready to review?

@ptomato ptomato added has consensus This has committee consensus and removed awaiting consensus This needs committee consensus before it can be eligible to be merged. labels Nov 24, 2025
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Nov 25, 2025

The PR is testing the old behavior

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review November 25, 2025 09:09
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Nov 25, 2025

Updated ✔️

@nicolo-ribaudo nicolo-ribaudo changed the title Add test for difference between export * as x from and import * as x; export { x } Test export * as x from and import * as x; export { x } binding equivalence Nov 25, 2025
nicolo-ribaudo
nicolo-ribaudo commented Nov 25, 2025
View reviewed changes
...xport-bindings/namespace-unambiguous-if-export-star-as-from-and-import-star-as-and-export.js Outdated
export * from "./namespace-export-star-as-from-1_FIXTURE.js";
export * from "./namespace-import-star-as-and-export-1_FIXTURE.js";

import { foo } from './namespace-ambiguous-if-import-star-as-and-export.js';
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Nov 25, 2025

Choose a reason for hiding this comment

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

Before the spec change, this would be an error.

.../module-code/ambiguous-export-bindings/namespace-unambiguous-if-import-star-as-and-export.js Outdated
export * from "./namespace-import-star-as-and-export-1_FIXTURE.js";
export * from "./namespace-import-star-as-and-export-2_FIXTURE.js";

import { foo } from './namespace-ambiguous-if-import-star-as-and-export.js';
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Nov 25, 2025

Choose a reason for hiding this comment

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

Before the spec change, this would be an error.

@nicolo-ribaudo nicolo-ribaudo force-pushed the ambiguous-binding branch 2 times, most recently from 6cdfc0e to b47cae6 Compare November 25, 2025 15:20
bakkot
bakkot approved these changes Nov 25, 2025
View reviewed changes
Copy link
Member

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

Oh, I overlooked the negative bits earlier, oops.

Anyway, this looks good to me other than the descriptions I commented on. Specifically, it is testing

  • that import { foo } from 'bar'; export { foo } does not conflict with export { foo } from 'bar' (in import-and-export-propagates-binding.js) - not technically part of the changes in the PR, but good to test
  • that two import * as foo from 'bar'; export { foo } do not conflict with each other (in namespace-unambiguous-if-import-star-as-and-export.js)
  • that two export * as foo from 'bar' do not conflict with each other (in the namespace-unambiguous-if-export-star-as-from.js test)
  • that export * as foo from 'bar' does not conflict with import * as foo from 'bar'; export { foo } (in namespace-unambiguous-if-export-star-as-from-and-import-star-as-and-export.js)

Ms2ger
Ms2ger approved these changes Nov 26, 2025
View reviewed changes
Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Thanks

@Ms2ger Ms2ger enabled auto-merge (rebase) November 26, 2025 10:30
@Ms2ger Ms2ger merged commit 0718086 into tc39:main Nov 26, 2025
12 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the ambiguous-binding branch November 26, 2025 14:22
lando-prod-mozilla bot pushed a commit to mozilla-firefox/firefox that referenced this pull request Nov 26, 2025
@nicolo-ribaudo @jonco3
Bug 2002191 - Align indirect and direct module namespace re-exports r…
61644fd 
…=jonco

This patch implements tc39/ecma262#3715, to make

  import * as ns from "mod";
  export { ns };

behave the same as

  export * as ns from "mod";

Tested by tc39/test262#4606

Differential Revision: https://phabricator.services.mozilla.com/D274013
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Nov 26, 2025
Open
nicolo-ribaudo added a commit to nicolo-ribaudo/WebKit that referenced this pull request Nov 27, 2025
@nicolo-ribaudo
Align import * as ns; export { ns } with export * as ns
28d10fd 
https://bugs.webkit.org/show_bug.cgi?id=303141

Reviewed by NOBODY (OOPS!).

This patch changes JSC's handling of `import * as ns; export { ns }`
to align with tc39/ecma262#3715, which reached
consensus in the Nov 2025 TC39 meeting.

This will be tested by tc39/test262#4606, for
now I manually verified that the relevant tests in that PR are failing
before this patch and passing after it.

* Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:
(JSC::ModuleAnalyzer::exportVariable):
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 27, 2025
@marco-c
Bug 2002191 - Align indirect and direct module namespace re-exports r…
effd9e9 
…=jonco

This patch implements tc39/ecma262#3715, to make

  import * as ns from "mod";
  export { ns };

behave the same as

  export * as ns from "mod";

Tested by tc39/test262#4606

Differential Revision: https://phabricator.services.mozilla.com/D274013

UltraBlame original commit: d14a908552e6d040c7538351430b3152a4a36887
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 27, 2025
@marco-c
Bug 2002191 - Align indirect and direct module namespace re-exports r…
136985b 
…=jonco

This patch implements tc39/ecma262#3715, to make

  import * as ns from "mod";
  export { ns };

behave the same as

  export * as ns from "mod";

Tested by tc39/test262#4606

Differential Revision: https://phabricator.services.mozilla.com/D274013

UltraBlame original commit: d14a908552e6d040c7538351430b3152a4a36887
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 27, 2025
@marco-c
Bug 2002191 - Align indirect and direct module namespace re-exports r…
d859cc0 
…=jonco

This patch implements tc39/ecma262#3715, to make

  import * as ns from "mod";
  export { ns };

behave the same as

  export * as ns from "mod";

Tested by tc39/test262#4606

Differential Revision: https://phabricator.services.mozilla.com/D274013

UltraBlame original commit: d14a908552e6d040c7538351430b3152a4a36887
nicolo-ribaudo added a commit to nicolo-ribaudo/WebKit that referenced this pull request Nov 28, 2025
@nicolo-ribaudo
Align import * as ns; export { ns } with export * as ns
d202890 
https://bugs.webkit.org/show_bug.cgi?id=303141

Reviewed by NOBODY (OOPS!).

This patch changes JSC's handling of `import * as ns; export { ns }`
to align with tc39/ecma262#3715, which reached
consensus in the Nov 2025 TC39 meeting.

This will be tested by tc39/test262#4606, for
now I manually verified that the relevant tests in that PR are failing
before this patch and passing after it.

* Source/JavaScriptCore/parser/ModuleAnalyzer.cpp:
(JSC::ModuleAnalyzer::exportVariable):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ms2ger Ms2ger Ms2ger approved these changes

@bakkot bakkot bakkot approved these changes

Assignees

No one assigned

Labels

has consensus This has committee consensus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolo-ribaudo @guybedford @bakkot @Ms2ger @ptomato