Completion issue with reused variable name in different scope

[frabbit]

class Test  {
  public static function main () {
    var x = [1,2,3];
    var r = x.map(function (x) return x+1); // here we use x again in different scope
    r.| // completion positon
  }
}

the compiler provides only Dynamic for completion at this position. If the function parameter name x is renamed to an unused variable like y everything works as expected.

[Simn]

This works with -D no-copt. I really wish this was the default completion setting. :/

[Simn]

I don't know what to do with it because I don't quite understand what the completion optimization is supposed to do, other than introducing issues like this. Assigning to Nicolas.

[ncannasse]

The completion optimization is meant to get very quick result even if the completion is at the bottom of a very big function. I'll make sure to fix the bug

[Simn]

I wonder how big such a function has to be before this optimization starts becoming significant.

[frabbit]

but every line can possibly call a lot of other stuff in other classes that needs to be typed too, i guess that's the reason.

[Simn]

That might be the case, but I would still prefer a default setting which is robust and allow to opt into the optimized version. There are some really basic cases which are broken by completion optimization such as this:

class Main  {
    public static function main () {
        var a = [];
        a.push("1");
        a.pop().|
    }
}

[ncannasse]

@Simn I thought you fixed this one by having a two passes check. First we do the optimized completion, then the type contains a monomorph we will type the whole again.
Optimizing completion really makes a big difference. In some cases (this var or typed local var) you even don't have anything to type, it's really useful so I prefer to have it by default and fix the reported bugs.

[Simn]

Now that you mention it I remember implementing the two passes. The issue here is that the type is reported as Null<Unknown<0>> which is not matched correctly. I'll follow it.

[Simn]

Incidentally it looks like we can "fix" the issue here by also retrying on TDynamic. Do you think that makes sense?

[ncannasse]

I think it's Dynamic because the variable is not found, and because there's an issue with the optimizer, I prefer that we fix the later.

[Simn]

The typed display expression is (Local $tmp_2:4 : Dynamic), so it is found. It just has the wrong type for some reason.