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.

Sign up for GitHub

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

Jump to bottom

Fix double evaluation when Operator Overloading is enabled #945

Merged
merged 3 commits into from
Aug 3, 2021
Merged

Fix double evaluation when Operator Overloading is enabled #945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3db9399
Fix double evaluation when Operator Overloading is enabled.
n1xx1 Aug 3, 2021
0d05db7
Small behaviour fixes to JintUnaryExpression
n1xx1 Aug 3, 2021
91bc1a7
Added test for the new feature. Fixed failing ones and reimplemented …
n1xx1 Aug 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion Jint.Tests/Runtime/OperatorOverloadingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,5 +315,29 @@ public void OperatorOverloading_ShouldWorkOnDerivedClasses()
");
}

[Fact]
public void OperatorOverloading_ShouldEvaluateOnlyOnce()
{
RunTest(@"
var c;
var resolve = v => { c++; return v; };

c = 0;
var n1 = resolve(1) + 2;
equal(n1, 3);
equal(c, 1);

c = 0;
var n2 = resolve(2) + resolve(3);
equal(n2, 5);
equal(c, 2);

c = 0;
var n3 = -resolve(1);
equal(n3, -1);
equal(c, 1);
");
}

}
}
}
83 changes: 42 additions & 41 deletions Jint/Runtime/Interpreter/Expressions/JintBinaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ private JintBinaryExpression(Engine engine, BinaryExpression expression) : base(
_right = Build(_engine, expression.Right);
}

protected bool TryOperatorOverloading(string clrName, out object result)
protected bool TryOperatorOverloading(JsValue left, JsValue right, string clrName, out object result)
{
return TryOperatorOverloading(_engine, _left.GetValue(), _right.GetValue(), clrName, out result);
return TryOperatorOverloading(_engine, left, right, clrName, out result);
}

internal static bool TryOperatorOverloading(Engine engine, JsValue leftValue, JsValue rightValue, string clrName, out object result)
Expand Down Expand Up @@ -267,14 +267,14 @@ public LessBinaryExpression(Engine engine, BinaryExpression expression) : base(e

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_LessThan", out var opResult))
&& TryOperatorOverloading(left, right, "op_LessThan", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();
var value = Compare(left, right);

return value._type == InternalTypes.Undefined
Expand All @@ -291,14 +291,15 @@ public GreaterBinaryExpression(Engine engine, BinaryExpression expression) : bas

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_GreaterThan", out var opResult))
&& TryOperatorOverloading(left, right, "op_GreaterThan", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();
var value = Compare(right, left, false);

return value._type == InternalTypes.Undefined
Expand All @@ -315,15 +316,15 @@ public PlusBinaryExpression(Engine engine, BinaryExpression expression) : base(e

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_Addition", out var opResult))
&& TryOperatorOverloading(left, right, "op_Addition", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

if (AreIntegerOperands(left, right))
{
return JsNumber.Create(left.AsInteger() + right.AsInteger());
Expand All @@ -344,15 +345,15 @@ public MinusBinaryExpression(Engine engine, BinaryExpression expression) : base(

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_Subtraction", out var opResult))
&& TryOperatorOverloading(left, right, "op_Subtraction", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

return AreIntegerOperands(left, right)
? JsNumber.Create(left.AsInteger() - right.AsInteger())
: JsNumber.Create(TypeConverter.ToNumber(left) - TypeConverter.ToNumber(right));
Expand All @@ -367,15 +368,15 @@ public TimesBinaryExpression(Engine engine, BinaryExpression expression) : base(

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_Multiply", out var opResult))
&& TryOperatorOverloading(left, right, "op_Multiply", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

if (AreIntegerOperands(left, right))
{
return JsNumber.Create((long) left.AsInteger() * right.AsInteger());
Expand All @@ -398,15 +399,15 @@ public DivideBinaryExpression(Engine engine, BinaryExpression expression) : base

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_Division", out var opResult))
&& TryOperatorOverloading(left, right, "op_Division", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

return Divide(left, right);
}
}
Expand All @@ -422,15 +423,15 @@ public EqualBinaryExpression(Engine engine, BinaryExpression expression, bool in

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading(_invert ? "op_Inequality" : "op_Equality", out var opResult))
&& TryOperatorOverloading(left, right, _invert ? "op_Inequality" : "op_Equality", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

return Equal(left, right) == !_invert
? JsBoolean.True
: JsBoolean.False;
Expand All @@ -448,15 +449,15 @@ public CompareBinaryExpression(Engine engine, BinaryExpression expression, bool

protected override object EvaluateInternal()
{
var leftValue = _left.GetValue();
var rightValue = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading(_leftFirst ? "op_GreaterThanOrEqual" : "op_LessThanOrEqual", out var opResult))
&& TryOperatorOverloading(leftValue, rightValue, _leftFirst ? "op_GreaterThanOrEqual" : "op_LessThanOrEqual", out var opResult))
{
return opResult;
}

var leftValue = _left.GetValue();
var rightValue = _right.GetValue();

var left = _leftFirst ? leftValue : rightValue;
var right = _leftFirst ? rightValue : leftValue;

Expand Down Expand Up @@ -525,15 +526,15 @@ public ModuloBinaryExpression(Engine engine, BinaryExpression expression) : base

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading("op_Modulus", out var opResult))
&& TryOperatorOverloading(left, right, "op_Modulus", out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

if (AreIntegerOperands(left, right))
{
var leftInteger = left.AsInteger();
Expand Down Expand Up @@ -588,15 +589,15 @@ public BitwiseBinaryExpression(Engine engine, BinaryExpression expression) : bas

protected override object EvaluateInternal()
{
var left = _left.GetValue();
var right = _right.GetValue();

if (_engine.Options._IsOperatorOverloadingAllowed
&& TryOperatorOverloading(OperatorClrName, out var opResult))
&& TryOperatorOverloading(left, right, OperatorClrName, out var opResult))
{
return opResult;
}

var left = _left.GetValue();
var right = _right.GetValue();

if (AreIntegerOperands(left, right))
{
int leftValue = left.AsInteger();
Expand Down
79 changes: 42 additions & 37 deletions Jint/Runtime/Interpreter/Expressions/JintUnaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,50 +53,54 @@ public override JsValue GetValue()

protected override object EvaluateInternal()
{
if (_engine.Options._IsOperatorOverloadingAllowed)
{
string operatorClrName = null;
switch (_operator)
{
case UnaryOperator.Plus:
operatorClrName = "op_UnaryPlus";
break;
case UnaryOperator.Minus:
operatorClrName = "op_UnaryNegation";
break;
case UnaryOperator.BitwiseNot:
operatorClrName = "op_OnesComplement";
break;
case UnaryOperator.LogicalNot:
operatorClrName = "op_LogicalNot";
break;
default:
break;
}

if (operatorClrName != null &&
TryOperatorOverloading(_engine, _argument.GetValue(), operatorClrName, out var result))
{
return result;
}
}

switch (_operator)
{
case UnaryOperator.Plus:
var plusValue = _argument.GetValue();
return plusValue.IsInteger() && plusValue.AsInteger() != 0
? plusValue
: JsNumber.Create(TypeConverter.ToNumber(plusValue));
{
var v = _argument.GetValue();
if (_engine.Options._IsOperatorOverloadingAllowed &&
TryOperatorOverloading(_engine, v, "op_UnaryPlus", out var result))
{
return result;
}

return v.IsInteger() && v.AsInteger() != 0
? v
: JsNumber.Create(TypeConverter.ToNumber(v));
}
case UnaryOperator.Minus:
return EvaluateMinus(_argument.GetValue());
{
var v = _argument.GetValue();
if (_engine.Options._IsOperatorOverloadingAllowed &&
TryOperatorOverloading(_engine, v, "op_UnaryNegation", out var result))
{
return result;
}

return EvaluateMinus(v);
}
case UnaryOperator.BitwiseNot:
return JsNumber.Create(~TypeConverter.ToInt32(_argument.GetValue()));
{
var v = _argument.GetValue();
if (_engine.Options._IsOperatorOverloadingAllowed &&
TryOperatorOverloading(_engine, v, "op_OnesComplement", out var result))
{
return result;
}

return JsNumber.Create(~TypeConverter.ToInt32(v));
}
case UnaryOperator.LogicalNot:
return !TypeConverter.ToBoolean(_argument.GetValue()) ? JsBoolean.True : JsBoolean.False;
{
var v = _argument.GetValue();
if (_engine.Options._IsOperatorOverloadingAllowed &&
TryOperatorOverloading(_engine, v, "op_LogicalNot", out var result))
{
return result;
}

return !TypeConverter.ToBoolean(v) ? JsBoolean.True : JsBoolean.False;
}

case UnaryOperator.Delete:
var r = _argument.Evaluate() as Reference;
Expand Down Expand Up @@ -150,6 +154,7 @@ protected override object EvaluateInternal()
return Undefined.Instance;

case UnaryOperator.TypeOf:
{
var value = _argument.Evaluate();
r = value as Reference;
if (r != null)
Expand All @@ -161,8 +166,8 @@ protected override object EvaluateInternal()
}
}

// TODO: double evaluation problem
Copy link
Contributor Author

@n1xx1 n1xx1 Aug 3, 2021 •
edited
Loading

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 •
edited
Loading

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 😉

var v = _argument.GetValue();

if (v.IsUndefined())
{
return JsString.UndefinedString;
Expand All @@ -187,7 +192,7 @@ protected override object EvaluateInternal()
}

return JsString.ObjectString;

}
default:
ExceptionHelper.ThrowArgumentException();
return null;
Expand Down