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 1 commit
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
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
19 changes: 8 additions & 11 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,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.

if (_engine.Options._IsOperatorOverloadingAllowed)
{
string operatorClrName = null;
Expand All @@ -75,7 +76,7 @@ protected override object EvaluateInternal()
}

if (operatorClrName != null &&
TryOperatorOverloading(_engine, _argument.GetValue(), operatorClrName, out var result))
TryOperatorOverloading(_engine, v, operatorClrName, out var result))
{
return result;
}
Expand All @@ -84,19 +85,18 @@ protected override object EvaluateInternal()
switch (_operator)
{
case UnaryOperator.Plus:
var plusValue = _argument.GetValue();
return plusValue.IsInteger() && plusValue.AsInteger() != 0
? plusValue
: JsNumber.Create(TypeConverter.ToNumber(plusValue));
return v.IsInteger() && v.AsInteger() != 0
? v
: JsNumber.Create(TypeConverter.ToNumber(v));

case UnaryOperator.Minus:
return EvaluateMinus(_argument.GetValue());
return EvaluateMinus(v);

case UnaryOperator.BitwiseNot:
return JsNumber.Create(~TypeConverter.ToInt32(_argument.GetValue()));
return JsNumber.Create(~TypeConverter.ToInt32(v));

case UnaryOperator.LogicalNot:
return !TypeConverter.ToBoolean(_argument.GetValue()) ? JsBoolean.True : JsBoolean.False;
return !TypeConverter.ToBoolean(v) ? JsBoolean.True : JsBoolean.False;

case UnaryOperator.Delete:
var r = _argument.Evaluate() as Reference;
Expand Down Expand Up @@ -146,7 +146,6 @@ protected override object EvaluateInternal()
return bindings.DeleteBinding(property.ToString()) ? JsBoolean.True : JsBoolean.False;

case UnaryOperator.Void:
_argument.GetValue();
return Undefined.Instance;

case UnaryOperator.TypeOf:
Expand All @@ -161,8 +160,6 @@ protected override object EvaluateInternal()
}
}

var v = _argument.GetValue();

if (v.IsUndefined())
{
return JsString.UndefinedString;
Expand Down