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

Fixed a bug with interpreter peeling introduced in #1510 #1578

Merged

Conversation

andreabergia
Copy link
Contributor

I found a bug with the interpreter peeling introduced in #1510.

The code has an if (fun instanceof LambdaFunction) that does not work for LambdaConstructor. These are a subclass of LambdaFunction, so the instanceof check passes, but they do not have the target from the base class filled, so we get an error.

Added also an unit test.

+ "}\n"
+ "testLambdaFunction().name";
Object value = cx.evaluateString(scope, source, "test", 1, null);
assertEquals("Andrea", value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, its about the interpreter, but i think we should run as many tests as possible with all optimization levels.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, not much of a risk to use one of the methods to "runWithAllOptimizationLevels".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also some assert... methods in the Utils class, maybe you can also use one of these to make this simpler
eg. assertWithAllOptimizationLevels

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@gbrail
Copy link
Collaborator

gbrail commented Aug 23, 2024

Thanks for catching this -- I ran in to the very same issue when trying to "lambdaize" some other classes. Could you please address the comments on running the test at many levels, in case this "white box test" ends up being important in the future?

@andreabergia
Copy link
Contributor Author

I've changed the code to use Utils.runWithAllOptimizationLevels as requested, but I'm not convinced it's better:

  • we already have tests in LambdaFunctionTest that cover the cases for compiled mode;
  • this is really an extension of InterpreterFunctionPeelingTest - hence the similar class name
  • now the class name (InterpreterFunctionPeelingNonRegressionTest) doesn't make a lot of sense and I'm hard pressed coming up with something better. 🙂

@andreabergia
Copy link
Contributor Author

Actually I ended up modifying the pre-existing LambdaFunctionTest so that it runs in all modes. That reproduces the bug, and it is a lot better than adding a new test.

@gbrail
Copy link
Collaborator

gbrail commented Aug 23, 2024

Great solution. Thanks!

@gbrail gbrail merged commit 8c4b663 into mozilla:master Aug 23, 2024
3 checks passed
@andreabergia andreabergia deleted the interpreter-peeling-lambda-constructor branch November 28, 2024 08:54
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.

3 participants