Handle some cases of duplicate export bindings

[joeljeske]

WIP for #3566

[guybedford]

This is an incredible start, thank you for diving into these details @joeljeske.

We will get through these cases I'm sure, we just need to be careful about them.

[guybedford]

Here are some other edge cases I can think of:

1. Destructuring defaults themselves setting live bindings

export var p = 5;
export var q = 10;
({ p = q = 20 } = {});

2. Bindings updated in for expressions:

export var p = 5;
for (p of [1,2,3]);

This one is probably entirely unnecessary and I don't even think it's possible to support actually. Maybe just ensuring it outputs valid JS is enough, even if the live bindings don't work.

@joeljeske are there any other cases you think are still missing here? Or refactorings you think still need to be considered?

Thanks again for the great work.

[joeljeske]

@guybedford Thanks, yea those are good cases to consider. Now that I better understand the expected output, I don't think it'll be too bad to update the AST to render those scenarios. Just a matter of getting some time for it.

I'll ping you back here when I'm ready for a more in-depth review.

Thanks for the guidance and help

[guybedford]

I've pushed up some updates to those edge cases here - this gets the output correct for default destructuring assignment expressions, while ensuring that we use the IIFE expression as much as possible.

Note that the expression IIFE form is better than the statement form as it works inline in expressions - I did some tests with destructuring and can confirm that the binding updates are interleaved with the default expressions, and the semantics implemented here will match that if you happen to call a function in another module that expects the partial binding updates to be available already!

I've left out the ForStatement assignment cases as that just seems rather silly TBH. I've also included a test for function export assignment (yes this is actually possible!)

@lukastaegert all of the rendering cases seem to be worked out here now, but there is still an information flow problem with exportNames. The chunking tests are all currently broken due to the fact that exportNames is picking up both the original module export name and the chunk module export name.

It seems like we need to separate exportNames into:

• The actual rendered binding name
• The list of exports of the chunk that reflect this binding

If you have any suggestions on how better to pipe this information please let me know - I'm leaving some comments on the appropriate code paths about this.

[guybedford]

@lukastaegert the comments mostly cover the information flow issues left here. Would be great to get these ironed out if you have a moment to share some suggestions. Failing tests are the many chunking-form cases.

Update - to summarize the issue: The implementation in this PR relies on variable.safeExportName always representing the singular exported binding in a non-public chunk. But the failing chunk test cases demonstrate chunks where safeExportName is not set to represent the chunk boundary - so the public exportNames get exported which do not represent the chunk interface. exportNames also seems to include a mix of the public and private export names.

[lukastaegert]

@guybedford @joeljeske If you search the code, you'll see that Variable.safeExportName is only queried but never actually set. You can just remove this property from Variable, doing so on master and removing the usages in VariableDeclaration.ts and systemJsRendering.ts did not break any tests because the property is always null. Must be a relic from a different time.

[lukastaegert]

See my comment, I think getting rid of safeExportName and always adding the exportNames property to all variables will already make the code much clearer, then I will take a deeper look at the rest.

If exportNames are never mutated, you could even use a shared constant, i.e. NO_EXPORTS with an empty array as the initial value for exportNames. This mini-optimization would save a little memory as the runtime can use the same array instance everywhere and make it clearer what an empty array means. But just an idea.

[guybedford]

@lukastaegert I've updated this PR to remove safeExportName and renamed exportName to exportNames.

The form tests are still passing, but as before the chunking tests are not because exportNames is not a correct representation of the expected chunk export names.

Note that exportNames meaning very much splits on the chunk / entry differences, which doesn't currently seem to be reflected in the logic.

Further suggestions / help welcome.

[joeljeske]

Thanks for the work on this, I appreciate it.

I’m a bit confused, how did exportName use to work? Didn’t it already have to keep track of the proper name to use, depending on use in a chunk or entry?

[lukastaegert]

The problem was not that exportNames was used wrongly or had the wrong meaning, but that it was not reset between rendering different chunks or outputs, resulting in the test failures. I pushed a fix that works by getting rid of the property altogether as export names are not variable-specific only but also chunk specific. Thus the export names are now only stored in a Map on each chunk and looked up when needed, fixing the issues. This will also make further refactorings easier as it gets rid of some state in the individual variables.

[guybedford]

Thanks so much @lukastaegert for taking the time to dig into fixing the approach here. Very glad the shared names are finally refactored out entirely. That was a vestige of the pre-code-splitting days!

I've gone through and polished up the outputs some more here, paying more attention to whitespace as well.

@joeljeske if you feel like throwing some more tests at this PR please do, otherwise I think we are close to being ready to merge here.

[guybedford]

The git hooks really weren't happy with running on my WSL environment so note the commit hooks still need running here.

[codecov]

Codecov Report

Merging #3575 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3575      +/-   ##
==========================================
+ Coverage   96.39%   96.48%   +0.09%     
==========================================
  Files         182      182              
  Lines        6212     6233      +21     
  Branches     1819     1830      +11     
==========================================
+ Hits         5988     6014      +26     
+ Misses        111      108       -3     
+ Partials      113      111       -2     

Impacted Files	Coverage Δ
src/ast/nodes/ImportExpression.ts	100.00% <ø> (ø)
src/ast/variables/Variable.ts	100.00% <ø> (ø)
src/Chunk.ts	100.00% <100.00%> (+0.20%)	⬆️
src/ast/nodes/ArrayPattern.ts	88.23% <100.00%> (ø)
src/ast/nodes/AssignmentExpression.ts	100.00% <100.00%> (+3.70%)	⬆️
src/ast/nodes/AssignmentPattern.ts	100.00% <100.00%> (ø)
src/ast/nodes/ClassDeclaration.ts	100.00% <100.00%> (ø)
src/ast/nodes/ExportDefaultDeclaration.ts	100.00% <100.00%> (ø)
src/ast/nodes/Identifier.ts	100.00% <100.00%> (ø)
src/ast/nodes/ObjectPattern.ts	86.66% <100.00%> (ø)
... and 11 more

---

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 e2618c8...f2e4f1d. Read the comment docs.

[lukastaegert]

I removed some afaik impossible cases to improve coverage. What I would like to see before this is done:

• Add tests for the few missing bits as indicated by coverage:
  • AssignmentExpression in compact mode
  • Quite a few cases around UpdateExpression
• Clarify what the return value of exports({foo: v, bar: v}) is

[lukastaegert]

@guybedford It would be really nice if you pulled before force pushing or used force-with-lease and not overwrite my commits. I will just force-push again over your changes to restore this.

[lukastaegert]

https://blog.developer.atlassian.com/force-with-lease/

[guybedford]

@lukastaegert I've gone through and made some further careful refinements here:

• Only using the return value function form
• Including a compact AssignmentExpression case
• Fixing issues with the destructuring output, and adding another sugar case here
• Including more tests for UpdateExpression

[guybedford]

@lukastaegert I did merge in your changes, but made further significant refactorings as well. Can you point me to something from the diff I missed?

[guybedford]

It seems the force push added zero new commits anyway, so I think we are all good here.

[guybedford]

Coverage looking good! Please double check a final review when you can and thanks again for all the help here all, a real collaborative effort.

[lukastaegert]

Sorry, maybe I also read the commit history wrongly, I was missing my "improve coverage" commits. But it appears everything is fine now, thanks!

[lukastaegert]

I think this is good to go mostly. I two places I find the code-style micro-optimizations a little over-the-top and making it really hard to read and maintain the code, so I would hope maybe we can go without those?

[guybedford]

Good point - I've put a bit more work into the whitespace handling by refactoring it with a scanWs function. I think this will be a good general approach for replacements going forward.

The newline handling was mostly to keep the pure comment test cases as before, but instead of special casing this, they are now handled by the same system. Just check that the test cases for that seem ok to you in the latest commit.

[lukastaegert]

Thanks. I slightly changed the name of the scan function to make clearer what it does and align better with other naming. Otherwise I would see this fit for release. Any more comments @joeljeske @guybedford ?

[lukastaegert]

Ah, screw that, I want to do a release because I have little time left today and want this in master soon. We can always iterate in patch releases here.

[joeljeske]

Thanks so much for working this out! It was educational for me to understand this project better. Thanks!

[lukastaegert]

Always happy to have new interested contributors here! If you ever feel like fixing something again, just take a shot 😜