correctly determine scope of AST_This

[alexlamsl]

fixes #2107

Technically speaking this fixes a sub-optimal case and re-masks an underlying bug.

[kzc]

re-masks an underlying bug.

Ouch! :-)

[kzc]

Just to clarify, the bug you're referring to - is it the function deep clone which does not create new definitions for local variables such that collapse_vars' ref count assumption fails?

[alexlamsl]

I don't think it has anything to do with definitions - it's down to this:

                 c = 1 + c
                 /       \
                /         \
               /           \
              /             \
             /               \
            /                 \
           /                   \
c += 1, c = 1 + c     c += 1, c = 1 + c
   (original)             (cloned)
                             ||
                        collapse_vars
                             ||
                       c = 1 + (c += 1)

Now because of the shared AST_Node, the original expression is inadventently altered to become c += 1, c = 1 + (c += 1)

With this PR that now corrupted instance gets discarded and never made it to the final AST_Toplevel, so we are good. But the corruption itself might manifest in other corner cases.

Also note that the cloned copy is not the one done within inline as that is a deep-clone. The sharing is due to some shallow cloning elsewhere in the codebase.

[kzc]

I don't think it has anything to do with definitions

        var c = 0;
        !function() {
            c++;
        }(c++ + new function() {
            this.a = 0;
            var a = (c = c + 1) + (c = 1 + c);
            return c++ + a;
        }());
        console.log(c);

Each function (clone or original) is pointing to the same definition for var a. Once one of the functions' var a (either in the original or clone) is collapsed by collapse_vars its definition is set to null and the other function clone is screwed. Or at least that's how collapse_vars used to work. But you've changed it significantly since I last worked on it.

I think you agree with me that any function with variable definitions should not be cloned?

[alexlamsl]

... collapse_vars ...

Ah, I think I misunderstood what you meant by "definition" - I was referring to SymbolDef, i.e. stuff from AST_SymbolRef.definition(), whereas you are talking about AST_VarDef. My apologies.

I think you agree with me that any function with variable definitions should not be cloned?

Yes, that's currently how it should work.

[kzc]

Ah, I think I misunderstood what you meant by "definition" - I was referring to SymbolDef, i.e. stuff from AST_SymbolRef.definition(), whereas you are talking about AST_VarDef. My apologies.

Please pardon the vague terminology I used. Actually, I was also referring to SymbolDef and AST_Symbol.thedef. If a function AST were to be cloned, regardless of being shallow or deep, the clone of the AST_SymbolVars and AST_SymbolRefs would be incorrectly sharing the same underlying SymbolDefs (a.k.a. thedef) would they not?

I think you agree with me that any function with variable definitions should not be cloned?

Yes, that's currently how it should work.

Then it's all good. The problematic situation is avoided altogether.

[alexlamsl]

If a function AST were to be cloned, regardless of being shallow or deep, the clone of the AST_SymbolVar s and AST_SymbolRef s would be incorrectly sharing the same underlying SymbolDef s (a.k.a. thedef ) would they not?

That is correct - .thedef remains identical. No new instances of SymbolDef is created in the process of .clone()