Prefer locally defined exports and reexports over external namespaces

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
Resolves #4049

Description

This resolves the scenario in #4049 where you have two competing namespace reexports in a file where the first one reexports the namespace of an external module. When looking for explicit reexports, the logic will first check all namespaces for exports that are not resolved via the external namespace until falling back to that one.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#gh-4049-namespace-reexport-issue

or load it into the REPL:
https://rollupjs.org/repl/?pr=4064

[codecov]

Codecov Report

Merging #4064 (f2cda99) into master (7927f3d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4064      +/-   ##
==========================================
+ Coverage   97.43%   97.49%   +0.05%     
==========================================
  Files         192      193       +1     
  Lines        6794     6821      +27     
  Branches     1995     2006      +11     
==========================================
+ Hits         6620     6650      +30     
  Misses         84       84              
+ Partials       90       87       -3     

Impacted Files	Coverage Δ
src/finalisers/shared/warnOnBuiltins.ts	100.00% <ø> (ø)
cli/run/batchWarnings.ts	99.19% <100.00%> (+0.74%)	⬆️
src/ExternalModule.ts	100.00% <100.00%> (+3.77%)	⬆️
src/Module.ts	100.00% <100.00%> (ø)
src/ast/nodes/MemberExpression.ts	99.16% <100.00%> (-0.02%)	⬇️
src/ast/variables/NamespaceVariable.ts	100.00% <100.00%> (ø)
src/utils/error.ts	100.00% <100.00%> (ø)
src/utils/printStringList.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7927f3d...f2cda99. Read the comment docs.

[guybedford]

This seems great. I wish we had had ambiguous errors for the namespace cases. I guess these could be refined over time possibly, but should likely be avoided anyway.

[lukastaegert]

I also refined how conflicts that we definitely know are handled. See in this example how ns.foo is replaced (correctly) with undefined and ns does not contain foo: https://rollupjs.org/repl/?pr=4064&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG5zJTIwZnJvbSUyMCcuJTJGcmVleHBvcnQnJTNCJTVDbmNvbnNvbGUubG9nKG5zKSUzQiU1Q25jb25zb2xlLmxvZyhucy5mb28pJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIycmVleHBvcnQuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwKiUyMGZyb20lMjAnLiUyRmZpcnN0JyUzQiU1Q25leHBvcnQlMjAqJTIwZnJvbSUyMCcuJTJGc2Vjb25kJyUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJmaXJzdC5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBjb25zdCUyMGZvbyUyMCUzRCUyMDElM0IlNUNuZXhwb3J0JTIwY29uc3QlMjBiYXIlMjAlM0QlMjAxJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnNlY29uZC5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBjb25zdCUyMGZvbyUyMCUzRCUyMDIlM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

[lukastaegert]

As a consequence, using a non-namespace import will throw now (just as it would throw in a real engine): https://rollupjs.org/repl/?pr=4064&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QmZvbyU3RCUyMGZyb20lMjAnLiUyRnJlZXhwb3J0JyUzQiU1Q25jb25zb2xlLmxvZyhmb28pJTNCJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnJlZXhwb3J0LmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJy4lMkZmaXJzdCclM0IlNUNuZXhwb3J0JTIwKiUyMGZyb20lMjAnLiUyRnNlY29uZCclM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyZmlyc3QuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwY29uc3QlMjBmb28lMjAlM0QlMjAxJTNCJTVDbmV4cG9ydCUyMGNvbnN0JTIwYmFyJTIwJTNEJTIwMSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJzZWNvbmQuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwY29uc3QlMjBmb28lMjAlM0QlMjAyJTNCJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=
(Note that you also get the conflicting namespace warning first, the REPL just does not display it, so that you can place the error in context)

[lukastaegert]

And last, I added a completely new warning when a named import cannot unambiguously be resolved via more than one external namespace: https://rollupjs.org/repl/?pr=4064&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QmZvbyU3RCUyMGZyb20lMjAnLiUyRnJlZXhwb3J0JyUzQiU1Q25jb25zb2xlLmxvZyhmb28pJTNCJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMnJlZXhwb3J0LmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJ2ZpcnN0JyUzQiU1Q25leHBvcnQlMjAqJTIwZnJvbSUyMCdzZWNvbmQnJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

[guybedford]

Wow, that almost seems like parity and the warning is definitely very useful. Strictly speaking un-errored ambiguous properties are still on the namespace just as undefined values. I've posted a spec PR to explicitly define this in tc39/ecma262#2401.

[guybedford]

Strictly speaking un-errored ambiguous properties are still on the namespace just as undefined values. I've posted a spec PR to explicitly define this in tc39/ecma262#2401.

Turns out they aren't enumerable - should have checked the case more clearly here. So this PR does make RollupJS behaviour fully spec compatible down to the externalization ambiguity that we now have warnings for as well!