fuzzilli icon indicating copy to clipboard operation
fuzzilli copied to clipboard

feature/loop without var decl

Open TobiasWienand opened this issue 1 year ago • 3 comments

Without loss of generality, we will only talk about for-of loops in the following, even though everything applies to for-in loops as well. The problem (Expected variable declaration as init part of a for-of loop, found Identifier) addressed with this PR was the most common reason why compilation of JS files in test/mjsunit failed, excluding the yet to be supported different parameter types and currently pending PRs.

Description of the problem

Previously we could compile this

case 1

for (var c of ["bar"]){
}
console.log(c); // bar

but not this

case 2

var c = "foo";
for ( c of ["bar"]){
}
console.log(c); // bar

or even this

case 3

for ( c of ["bar"]){
}
console.log(c); // bar
console.log(this.c); // bar 

Solution in a nutshell

This PR only addresses case 2 so far. To support this new feature, in a nutshell, we first verify if the identifier points to a declared variable (so we know we are not in case 3) and if so, we give the variable corresponding to the identifier as input into BeginForInLoop and do not create a new inner output for the iterator. During lifting to FuzzIL and JS we check the number of inputs to know if the iterator is the second input (as in case 2) or the inner output (as in case 1). Because iterating over an object with the iterator inherently changes the iterators value, we need to mark this FuzzIL instruction as having the "reassigning" property. Otherwise inlining would happen because the lifter thinks that the iterator still has the previous value, e.g. 'foo' in case 2

Some questions and next steps

  1. The current design choice is to give BeginForInLoop a parameter called usesPredeclaredIterator to distinguish case 1 from case 2. However, there is also the option of heaving unique IL instructions for each case. I like the current solution more but if you like the other option better I can change it.

  2. case 3 is not supported. I want to change that by utilizing LoadNamedVariable and StoreNamedVariable

  3. Writing tests. Do you have suggestions where to implement it`? Otherwise I would write the obligatory CompilerTest and now, since we also changed FuzzIL itself, also tests in FuzzilliTests

TobiasWienand avatar Nov 20 '24 13:11 TobiasWienand

I wanted to implement case 3 with commit 89d2d77a9e3cb6660863c8f053f458e9f52f6da2

I noticed that there are still some persisting compilation errors. See this before and after:

before

c = "old";
for (c of ["new"]) {
}
if (c == "new" && this.c === "new")
    console.log("Test 5 successful") // success with v8

after

c = "old";
const v2 = ["new"];
let v3 = c; // should not have become a let
for (v3 of v2) {
}
if ((v3 == "new") && (this.c === "new")) {
    console.log("Test 5 successful");  // failure because this.c is "old" 
}

I assume this error stems from the JavascriptLifter in the assign method because there we create the line "let v3 = c;"

This will be fixed soon

TobiasWienand avatar Dec 07 '24 13:12 TobiasWienand

This is ready for the final review now.

TobiasWienand avatar Dec 17 '24 05:12 TobiasWienand

tldr;

  1. Merged the createNamedVariable changes
  2. Loops that create a variable now behave similarly to createNamedVariable
  3. The ugly logic with forceInlining is removed
  4. Some minor bug fixes in Compiler.swift are included that are not just about for-in loops and for-of loops but in general about mapping the identifier name after creating a new global variable

TobiasWienand avatar Jan 23 '25 20:01 TobiasWienand