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

Fix double evaluation when Operator Overloading is enabled #945

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

n1xx1
Copy link
Contributor

@n1xx1 n1xx1 commented Aug 3, 2021

When you have operator overloading enabled the operands of almost all binary and unary expression are evaluated multiple times. This PR should fix it but it's marked as draft because it was suggested to add some tests to test against it.

The problem is easily verifiable like this:

var engine = new Engine(cfg => { cfg.AllowOperatorOverloading(); });
engine.SetValue("Log", (Action<string>) (s => Console.Out.WriteLine(s)));
engine.Execute("const test = () => { Log('Example'); return 1; }; test() + 1; ");

This should only print a single Example line, but instead prints two.

@n1xx1 n1xx1 changed the title Fix double evaluation when Operator Overloading is enabled. Fix double evaluation when Operator Overloading is enabled Aug 3, 2021
@lahma
Copy link
Collaborator

lahma commented Aug 3, 2021

Change seems logical, it would be great if you can add test case along the other operator overloading tests.

@@ -53,6 +53,7 @@ public override JsValue GetValue()

protected override object EvaluateInternal()
{
var v = _argument.GetValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might have broken the test suite as it seems that this could have behavioral change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found some issues with the UnaryExpression fixes, I will add another commit to this soon that should also avoid this behavioral change.

@@ -165,7 +166,8 @@ protected override object EvaluateInternal()
}
}

var v = _engine.GetValue(value, true);
// TODO: double evaluation problem
Copy link
Contributor Author

@n1xx1 n1xx1 Aug 3, 2021

Choose a reason for hiding this comment

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

This is a problem that needs more work to fix. GetValue is overloaded by different expressions, but we need to first check if it's an undefined reference so that we can return "undefined" without an error. At the moment this should pass all the tests, but it isn't the correct behaviour as you can see with this example:

const fn = () => Log('test');
typeof fn();

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what's the correct behavior, this is what node gives:

Welcome to Node.js v14.16.1.
Type ".help" for more information.
> const fn = () => Log('test');
undefined
> typeof fn();
Uncaught ReferenceError: Log is not defined
    at fn (REPL1:1:18)

And Jint follows the logic:

jint> const fn = () => Log('test');
undefined
jint> typeof fn();
Jint.Runtime.JavaScriptException: Log is not defined
   at fn () repl:1:1
   at repl:1:8

Copy link
Contributor Author

@n1xx1 n1xx1 Aug 3, 2021

Choose a reason for hiding this comment

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

Yes, of course, Log is not defined 😅, you have to use the corresponding print function.

With node:

> const fn = () => { console.log('test'); }
undefined
> typeof fn();
test
'undefined'
> 

With jint:

jint> const fn = () => { print('test'); }
undefined
jint> typeof fn();
test
test
"undefined"
jint> 

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK thanks, can you open a separate issue so that can be looked into, unless you want to solve all in this PR 😉

@n1xx1 n1xx1 marked this pull request as ready for review August 3, 2021 15:49
@lahma lahma merged commit 20dcf69 into sebastienros:main Aug 3, 2021
@lahma
Copy link
Collaborator

lahma commented Aug 3, 2021

Thanks for going through the process and helping out, always appreciated!

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