Double declarations - #2279

I take it this is the fix? Is there no way to ensure a strong separation of the binding phase? How is this timing issue occurring in the first place?

Unfortunately at least at the moment, I do not know if a separation will be possible at all. Here is an outline of the issue:

The issue

Before the actual tree-shaking, we pass through three phases:

• the initialization or construction phase in which rollup's AST nodes are constructed, the scopes are created and the declaration nodes register themselves on the scope and create the corresponding local variables.
• the link phase where the inter-module links are established
• the bind phase during which the remaining identifiers are associated with the declarations and all reassignments are collected.

The bind phase relies on all links being in place and the variables having their proper initializers set. This is the reason why before this PR, double declarations were normalized during construction by setting the "init" to "unknown".

However as this means we are losing track of the initializers, it is important we mark all their paths as "reassigned" so that their usages are never dropped in case they are mutated somewhere.

Reassigning paths, however, is not always possible during initialization in case of imported variables which was the reason for this issue.

Why do we need to track the "bound" state of variables?

When using functions, it is easy to access a variable the definition of which is further down in the file. As the binding phase just works from top to bottom, we can therefore not rely on the declarations of the variable being consolidated before any of the functions that can be used during bind access them (those being reassignPath, getReturnExpressionWhenCalledAtPath and getLiteralValueAtPath, all of which can pass through several variables to do their work).

A separate phase between link and bind would also not solve the issue entirely as it is still possible that the necessary reassignments would reach a variable that is not bound yet.

Another approach could be to not consolidate declarations at all. But this would make all of LocalVariable more complicated and definitely slow down tree-shaking while I think the optimization benefit would be small due to the fact that double declarations are rather rare. I'll think about how this can be made a little less convoluted.

I love how this project is committed to being rebelliously European :)

I hate to be difficult, but surely we shouldn't need to track all initializers here, since initializer precedence can always apply (#2278)? Or do we specifically need to apply reassignPath(UNKNOWN_PATH) to each initializer? Can you perhaps help clarify why tracking this information on the variable is not enough?

Alternatively could this.init be replaced to always reference this.initialisers[0]? I just hate to see the redundancy.

Without TDZ/flow analysis, we do not know which definition applies where. Thus the approach is to make the variable "unknown". As we can no longer track initializer references in such a situation, all initializers need the reassignPath(UNKNOWN_PATH).
I could however simplify this to get rid of the redundancy.

I'd prefer to fix root cause if we can... this feels a bit like trying to plug a leak.

Can we not apply initializer.reassignPath in addDeclaration? Or is this the whole issue with the .bound checks?

Yes, this was actually the initial approach that was the root cause of #2275. addDeclaration needs to be called during the initialization phase so that after that we can rely on all variables being known. reassignPath, however, cannot be called before the inter-module bindings have been established which can only happen after that phase.