HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.2k stars 658 forks source link

local static inside switch is not static enough #11800

Closed Antriel closed 4 weeks ago

Antriel commented 1 month ago
function main() {
    static var a = 0; // Works.
    for (i in 0...3) {
        switch i {
            case n if (n < 2):
                trace(++a);
                static var b = 0; // Not static.
                trace(++b); // Always `1`.
            case _:
        }
    }
}
Simn commented 4 weeks ago

Turns out our variable duplication doesn't copy the flags, argh...

Interestingly, this particular issue runs into a separate issue when we fix that:

 ERROR  source/Main.hx:9: characters 16-17

 9 |     static var b = 0; // Not static.
   |                ^
   | The expanded name of this local (main_b) conflicts with another static field

      | Conflicting field was found here

It took me a second to understand that the issue is the for (i in 0...3) because that unrolls the loop and thus duplicates the code, so I think we should disable loop unrolling when there are static variables in the body.

tobil4sk commented 4 weeks ago

I think we should disable loop unrolling when there are static variables in the body.

Would there be any issue with simply hoisting the variable declaration out of the loop? It's confusing when a feature silently disables an optimisation like this.

Simn commented 4 weeks ago

Hoisting changes the scope of a local variable, but that should be fine here because it still can only be accessed in its original scope as far as typing is concerned. Unrolling also happens before our local variable renaming step, so this should indeed be possible.