Tree shaking fixes - #2315

As far as I can tell this is for the scenario:

function x () {
}
x.asdf = 'asdf';

in which case surely the return expressions of the function don't need to be reassigned? If you're after x().asdf = 'asdf' then that would only apply to the call expression surely?

In the given example, path[0] would be 'asdf'. This scenario corresponds to something like

function x () {}
x[thisIsPrototype]= {the: 'prototype'};

as well as

function x () {}
globalFunction(x);

Why the latter? In this situation, we lose track of x. For simplicity, this is tracked as reassigning [UNKNOWN_KEY] (as this goes hand in hand in most situations). I added comments at the corresponding places of FunctionNode and ArrowFunctionExpression explaining this as well as further tests as commenting out these lines did not turn any test red.

It's worth noting here that spread can call the iterator protocol which may result in side effects.

For example this test case - https://rollupjs.org/repl?version=0.62.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMnZhciUyMG9iaiUyMCUzRCUyMCU3QiU1Q24lNUN0eCUzQSUyMGZhbHNlJTVDbiU3RCUzQiU1Q24lNUNudmFyJTIwcCUyMCUzRCUyMCU3QiU1Q24lNUN0KiU1QlN5bWJvbC5pdGVyYXRvciU1RCUyMCgpJTIwJTdCJTVDbiU1Q3QlNUN0eWllbGQlMjBvYmolM0IlNUNuJTVDdCU3RCU1Q24lN0QlM0IlNUNuJTVDbiU1Qi4uLnAlNUQlNUIwJTVELnglMjAlM0QlMjB0cnVlJTNCJTVDbiU1Q25pZiUyMChvYmoueCklMjAlN0IlNUNuJTVDdGNvbnNvbGUubG9nKCdpdCUyMGlzJTIwdHJ1ZScpJTNCJTVDbiU3RCUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJjanMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

seems to fail the deoptimization process on this build.

It seems like the above issue may be that yield isn't handled at all.

Here's another version of that using a custom iterator protocol, which also fails:

https://rollupjs.org/repl?version=0.62.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMnZhciUyMG9iaiUyMCUzRCUyMCU3QiU1Q24lNUN0eCUzQSUyMGZhbHNlJTVDbiU3RCUzQiU1Q24lNUNudmFyJTIwcCUyMCUzRCUyMCU3QiU1Q24lNUN0JTVCU3ltYm9sLml0ZXJhdG9yJTVEJTIwKCklMjAlN0IlNUNuJTVDdCU1Q3R2YXIlMjBmaXJzdCUyMCUzRCUyMHRydWUlM0IlNUNuJTVDdCU1Q3RyZXR1cm4lMjAlN0IlNUNuJTVDdCU1Q3QlNUN0bmV4dCUyMCgpJTIwJTdCJTVDbiU1Q3QlNUN0JTVDdCU1Q3RpZiUyMChmaXJzdCklMjAlN0IlNUNuJTVDdCU1Q3QlNUN0JTVDdCU1Q3RmaXJzdCUyMCUzRCUyMGZhbHNlJTNCJTVDbiU1Q3QlNUN0JTVDdCU1Q3QlMjAlMjByZXR1cm4lMjAlN0IlNUNuJTVDdCU1Q3QlNUN0JTVDdCU1Q3QlMjAlMjBkb25lJTNBJTIwZmFsc2UlMkMlNUNuJTVDdCU1Q3QlNUN0JTVDdCU1Q3QlMjAlMjB2YWx1ZSUzQSUyMG9iaiU1Q24lNUN0JTVDdCU1Q3QlNUN0JTIwJTIwJTdEJTNCJTVDbiU1Q3QlNUN0JTVDdCU1Q3QlN0QlNUNuJTVDdCU1Q3QlNUN0JTVDdHJldHVybiUyMCU3QiUyMGRvbmUlM0ElMjB0cnVlJTJDJTIwdmFsdWUlM0ElMjBudWxsJTIwJTdEJTNCJTVDbiU1Q3QlNUN0JTVDdCU3RCU1Q24lNUN0JTVDdCU3RCUzQiU1Q24lNUN0JTdEJTVDbiU3RCUzQiU1Q24lNUNuJTVCLi4ucCU1RCU1QjAlNUQueCUyMCUzRCUyMHRydWUlM0IlNUNuJTVDbmlmJTIwKG9iai54KSUyMCU3QiU1Q24lNUN0Y29uc29sZS5sb2coJ2l0JTIwaXMlMjB0cnVlJyklM0IlNUNuJTdEJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmNqcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

(These are edge cases I know, but good to get them ironed out!)

Definitely, thanks a lot! Will see what solutions I can find.

The second example was already handle explicitly by the new spread element reassignment logic. Added a comment here + added handling for yield expressions.

Did you mean A reassignment of UNKNOWN_PATH here, since UNKNOWN_PATH === [UNKNOWN_KEY]?

Thanks, that makes sense.

So the meaning of reassignPath is more like:

a) The path is being assigned somewhere we are no longer tracking
b) The path is being provided as an argument to a function we are no longer tracking

Is that all the cases?

It does seem somewhat circumstantial to the current analysis that the name here is reassignPath, as if we did happen to track some basic value propagation, then it would surely need another name.

Perhaps a clearer name other than reassignPath might help make this clear? Like taintPath, exposePath, deoptimizePath or spillPath or something?

A clear comment / documentation providing a comprehensive list of the above could help supplement the understanding here, I may well take this sort of work on at some point as an exercise in my own understanding, may well be beneficial both to increase my own contributions here as well as engaging other core analysis maintainers.

There are more situations where a reassignment happens. In all situations where the path is NOT UNKNOWN_PATH, there is an actual assignment taking place. More situations where we reassign UNKNOWN_PATH:

• there is more than one return expression
• a previously optimized conditional is deoptimized
• something is an export of the bundle
• something is an array element
• a variable has more than one declaration
• in any assignment, not only is the EMPTY_PATH of the left side reassigned, we also reassign UNKNOWN_PATH of the right side as we are no longer tracking mutations here (this one is REALLY important)
• this also goes for assignment patterns
• something is yielded (this one is new)

But you are right, this double meaning of reassignPath(UNKNOWN_PATH) should warrant a new name. I will go for deoptimizePath and probably rename the existing deoptimize methods to deoptimizeCache to better distinguish between the two.

In cases where reassignPath really does mean reassignPath could we retain the meaning, or is that a big refactor? I just think it seems like there should be a semantic distinction between reassignPath which refers to reassigning a binding (which might be possible to track in future), and deoptimizePath?

Nice changing to deoptimizePath though that seems much clearer to me!

Or do you think that reassignPath for actual reassignments is always about deoptimization, and that if we were to have better tracking here, we'd do it another way?

Also deoptimizeCache sounds great, nice to be clear on what exactly is the deoptimization!

The hope is that ultimately with SSA, all remaining reassignments will be "real" deoptimizations where an entity is out of sight of the algorithm. But also in its current form, deoptimize is probably a better name because we never reassign to anything but mark it as evil and unreliable ;)

This conditional isn't necessary? It isn't made in other places for the UNKNOWN style call noops, so just wondering why it's used here.

Just to make it a little cheaper and save us possibly a call but yes, this is a little over-optimization.

In theory, could we be adding duplicates to this? Should we not use a Set() implementation?

(that might also handle the UNKNOWN_EXPRESSION case more simply, if wanted anywhere)

I initially had a Set here but as a matter of fact, this should not be possible as each return expression is only ever retrieved once. The same goes for literal values in other places. I also checked this in some places by throwing an error when adding a duplicate and there were none I could find.

Not sure if this is valid or not, but would it be possible to use this as a recursion tracker itself?

I did not check but I would not assume it to work as well as the ImmutableEntityPathTracker in that it would deoptimize more literal values than necessary e.g. in situations where one origin needs more than one lookup to get its value. At least these were the reason why the immutable tracker was used here.

Much nicer.

It does seem a little unfortunate that the following:

var obj = {
  x: 'x',
  y: 'y'
};
var test = false;

function asdf () {
  return obj;
}

if (asdf().x === 'x')
  console.log('yes');

if (obj.x === 'x')
  console.log('true');

function updateTest () {
  test = true;
}

loses all the analysis benfits if the return statement is just changed to test ? obj : obj.

I might give it a try to improve a little here

No, it seems with the return expression handling, fixing this would be a little more involved. This will probably need to wait for SSA.

Perhaps cache the property here -

let propertyMapProperty = this.propertyMap[key];

good idea

+1 to having internal assertions... will be useful to catch more bugs.

This is a big step. Intermediate expressions!

Could function returns and generator yields not be treated as MultiExpression of all their return values?

The problem with function returns turned out to be a problem of convergence order for some code-bases (say bundling the TypeScript compiler for instance). If there are many functions with many returns, then with only a few steps, a single hasEffects check could suddenly trigger a million secondary checks as all returnExpressions are checked. This should definitely be revisited, though. One might consider e.g. limiting the number of tracked return expressions to two or similar things.

Definitely, setting a limit on this would be a good starting approach!