Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tracking function name for void feature at the compiler #478

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vi3tL0u1s
Copy link
Contributor

Hi, I found an error in the void compilation as follows.

When void is suffixed to a function expression, and that function is called again later, the compiler will not retain information about the void function. As a result, it will create a call to a new named function as follows:

void test() {
   console.log('1');
}
test();

will be compiled incorrectly into:

function f0() {
    console.log('1');
}
void f0();
test();

I fixed this by adding the function name to the AST structure, enabling the compiler to track it. Please see the corrected compilation output below:

function f0() {
    console.log('1');
}
void f0();
f0();

I also identified a semantic error with void in the FuzzIL and JavaScript lifters as follows:

v8 <- BeginPlainFunction -> 
    v9 <- LoadString '1'
    v10 <- LoadNamedVariable 'console'
    v11 <- CallMethod v10, 'log', [v9]
EndPlainFunction
v12 <- CallFunction v8, []
v13 <- Void_ v12

function f8() {
    console.log("1");
}
void f8();
f8();

Here, we add void to f8(), then execute the function f8. The string 1 is printed, which is semantically incorrect since f8() is a void function and should return undefined. I think the void keyword should be lifted together with the function definition to be semantically correct somehow. This semantic error has not been addressed in this PR.

@vi3tL0u1s
Copy link
Contributor Author

To clarify the semantic error with void, I think void should be lifted with the function expression according to the MDN documentation.

@saelo
Copy link
Collaborator

saelo commented Dec 10, 2024

Thanks for raising this! I'm not entirely sure I understand the issue. What is the AST that our parser generates for:

void function test() {
   console.log('1');
}
test();

? The way we handle void in our Compiler looks correct to me. I guess the issue is that we generate code like this:

function f0() {
   console.log('1');
}
void f0;
test();

(i.e. we generate a void expression for the variable holding the function, not for the function expression)? I think this is almost correct since in both cases the call to test() will (correctly) fail as there is no variable with that name. However, a sample that we do actually miscompile is this here:

void function f0() {
   console.log('1');
}
f0();

Because we'll turn it into

function f0() {
    console.log("1");
}
void f0;
f0();

And now it suddenly works. I guess the problem here is to some degree that our IL cannot distinguish between function expressions (what comes after the void) and function statements. We could probably fix this by making the inlining in the JavaScriptLifter more powerful so it can inline the function into the use, and then we'd get the exact same code back. I think this would be the "correct" fix here. It'd probably be a bit of work and probably not super high priority since void shouldn't be a super common feature.

@vi3tL0u1s
Copy link
Contributor Author

Hi @saelo, thanks so much for your review. Given the input below:

void function test() {
  console.log('1');
}
test();

The AST is as follows, where I added the function name. This fixed the error because previously there was no name for the function expression in the AST. That’s why when calling test(), the incorrect function was called.

{
  "statements": [
    {
      "expressionStatement": {
        "expression": {
          "unaryExpression": {
            "operator": "void",
            "argument": {
              "functionExpression": {
                "type": "PLAIN",
                "body": [
                  {
                    "expressionStatement": {
                      "expression": {
                        "callExpression": {
                          "callee": {
                            "memberExpression": {
                              "object": {
                                "identifier": {
                                  "name": "console"
                                }
                              },
                              "name": "log",
                              "isOptional": false
                            }
                          },
                          "arguments": [
                            {
                              "stringLiteral": {
                                "value": "1"
                              }
                            }
                          ],
                          "isOptional": false
                        }
                      }
                    }
                  }
                ],
               "name": "test" // <-- The added function name
              }
            }
          }
        }
      }
    },
    {
      "expressionStatement": {
        "expression": {
          "callExpression": {
            "callee": {
              "identifier": {
                "name": "test"
              }
            },
            "isOptional": false
          }
        }
      }
    }
  ]
}

@vi3tL0u1s
Copy link
Contributor Author

I also agree with the fix for the JavaScript lifter for void. If it is not a priority, I believe this semantic error does not occur often. At least with this compiler fix, void now calls the correct function name and does not cause the function to disappear from the program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants