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

Plus, minus and times crossing 32 to 64 bit boundary #488

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

Plus, minus and times crossing 32 to 64 bit boundary #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf7d141
Handle integer numbers beyond the range of 32 bit integer but still w…
daviburg May 9, 2022
ce4cdac
Formatting nit.
daviburg May 9, 2022
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
82 changes: 82 additions & 0 deletions src/DotLiquid.Tests/StandardFilterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1205,6 +1205,31 @@ public void TestPlusStringV21()
TestPlus(context);
}

[Test]
public void TestPlusNumericBoundary()
{
TestPlusNumericBoundaryByContext(_contextV20);
TestPlusNumericBoundaryByContext(_contextV21);
Helper.AssertTemplateResult(expected: "2147483648", template: "{{ '2147483647' | plus: 1 }}", syntax: _contextV21.SyntaxCompatibilityLevel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we comment that this tests a string which is supported in Syntax 2.1? Should we also test a string as a local variable?

}

private void TestPlusNumericBoundaryByContext(Context context)
{
// NOTE(David Burg): Tests promotion from 32-bit to 64-bit integer.
Helper.AssertTemplateResult(
expected: "2147483648",
template: "{{ big | plus: 1 }}",
localVariables: Hash.FromAnonymousObject(new { big = int.MaxValue }),
syntax: context.SyntaxCompatibilityLevel);

// NOTE(David Burg): We don't have support for BigInteger as this time.
Helper.AssertTemplateResult(
Copy link
Member Author

Choose a reason for hiding this comment

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

todo: also add tests demonstrating that byte max, short max work already, but double range limit cannot be extended beyond current overflow.

expected: "Liquid error: Arithmetic operation resulted in an overflow.",
template: "{{ big | plus: 1 }}",
localVariables: Hash.FromAnonymousObject(new { big = long.MaxValue }),
syntax: context.SyntaxCompatibilityLevel);
}

[Test]
public void TestMinus()
{
Expand Down Expand Up @@ -1241,6 +1266,27 @@ public void TestMinusStringV21()
TestMinus(context);
}

[Test]
public void TestMinusNumericBoundary()
{
// NOTE(David Burg): Tests promotion from 32-bit to 64-bit integer.
Helper.AssertTemplateResult(
expected: "-2147483649",
template: "{{ big | minus: 1 }}",
localVariables: Hash.FromAnonymousObject(new { big = int.MinValue }));

// NOTE(David Burg): We don't have support for BigInteger as this time.
Helper.AssertTemplateResult(
expected: "Liquid error: Arithmetic operation resulted in an overflow.",
template: "{{ big | minus: 1 }}",
localVariables: Hash.FromAnonymousObject(new { big = long.MinValue }));

Helper.AssertTemplateResult(
expected: "-2147483649",
template: "{{ '-2147483648' | minus: 1 }}",
syntax: _contextV21.SyntaxCompatibilityLevel);
}

[Test]
public void TestPlusCombinedWithMinus()
{
Expand Down Expand Up @@ -1346,6 +1392,42 @@ private void TestTimes(Context context)
Assert.AreEqual(7556.3, StandardFilters.Times(context: context, input: 7.5563m, operand: 1000));
}


[Test]
public void TestTimesNumericBoundary()
{
TestTimesNumericBoundaryByContext(_contextV20);
TestTimesNumericBoundaryByContext(_contextV21);
Helper.AssertTemplateResult(expected: "4294967294", template: "{{ '2147483647' | times: 2 }}", syntax: _contextV21.SyntaxCompatibilityLevel);
}

private void TestTimesNumericBoundaryByContext(Context context)
{
// NOTE(David Burg): Tests promotion from 32-bit to 64-bit integer.
Helper.AssertTemplateResult(
expected: "4294967294",
template: "{{ big | times: 2 }}",
localVariables: Hash.FromAnonymousObject(new { big = int.MaxValue }),
syntax: context.SyntaxCompatibilityLevel);
Helper.AssertTemplateResult(
expected: "-4294967296",
template: "{{ big | times: 2 }}",
localVariables: Hash.FromAnonymousObject(new { big = int.MinValue }),
syntax: context.SyntaxCompatibilityLevel);

// NOTE(David Burg): We don't have support for BigInteger as this time.
Helper.AssertTemplateResult(
expected: "Liquid error: Arithmetic operation resulted in an overflow.",
template: "{{ big | times: 2 }}",
localVariables: Hash.FromAnonymousObject(new { big = long.MaxValue }),
syntax: context.SyntaxCompatibilityLevel);
Helper.AssertTemplateResult(
expected: "Liquid error: Arithmetic operation resulted in an overflow.",
template: "{{ big | times: 2 }}",
localVariables: Hash.FromAnonymousObject(new { big = long.MinValue }),
syntax: context.SyntaxCompatibilityLevel);
}

[Test]
public void TestTimesStringV20()
{
Expand Down
43 changes: 32 additions & 11 deletions src/DotLiquid/StandardFilters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,12 +751,19 @@ public static object Last(IEnumerable array)
/// <param name="operand">Number to be added to input</param>
public static object Plus(Context context, object input, object operand)
{
if (context.SyntaxCompatibilityLevel >= SyntaxCompatibility.DotLiquid21)
return DoMathsOperation(context, input, operand, Expression.AddChecked);
try
{
if (context.SyntaxCompatibilityLevel >= SyntaxCompatibility.DotLiquid21)
return DoMathsOperation(context, input, operand, Expression.AddChecked);

return input is string
? string.Concat(input, operand)
: DoMathsOperation(context, input, operand, Expression.AddChecked);
return input is string
? string.Concat(input, operand)
: DoMathsOperation(context, input, operand, Expression.AddChecked);
}
catch (OverflowException) when (input is int)
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't uint also have a boundary? and smaller integer values, like short? Additionally, Ruby implementation supports adding 1 to decimal max without losing precision.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried bytes and short, they worked fine without modification, I can add a few tests to demonstrate it. I didn't try uint.

Copy link
Member Author

Choose a reason for hiding this comment

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

BigInteger is out of scope for a bug fix, that support would be a feature in itself.

{
return DoMathsOperation(context, Convert.ToInt64(input), operand, Expression.AddChecked);
}
}

/// <summary>
Expand All @@ -767,7 +774,14 @@ public static object Plus(Context context, object input, object operand)
/// <param name="operand">Number to be subtracted from input</param>
public static object Minus(Context context, object input, object operand)
{
return DoMathsOperation(context, input, operand, Expression.SubtractChecked);
try
{
return DoMathsOperation(context, input, operand, Expression.SubtractChecked);
}
catch (OverflowException) when (input is int)
{
return DoMathsOperation(context, Convert.ToInt64(input), operand, Expression.SubtractChecked);
}
}

/// <summary>
Expand All @@ -778,12 +792,19 @@ public static object Minus(Context context, object input, object operand)
/// <param name="operand">Number to multiple input by</param>
public static object Times(Context context, object input, object operand)
{
if (context.SyntaxCompatibilityLevel >= SyntaxCompatibility.DotLiquid21)
return DoMathsOperation(context, input, operand, Expression.MultiplyChecked);
try
{
if (context.SyntaxCompatibilityLevel >= SyntaxCompatibility.DotLiquid21)
return DoMathsOperation(context, input, operand, Expression.MultiplyChecked);

return input is string && (operand is int || operand is long)
? Enumerable.Repeat((string)input, Convert.ToInt32(operand))
: DoMathsOperation(context, input, operand, Expression.MultiplyChecked);
return input is string && (operand is int || operand is long)
? Enumerable.Repeat((string)input, Convert.ToInt32(operand))
: DoMathsOperation(context, input, operand, Expression.MultiplyChecked);
}
catch (OverflowException) when (input is int)
{
return DoMathsOperation(context, Convert.ToInt64(input), operand, Expression.MultiplyChecked);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what about divide by fraction? e.g. 2147483647 / 0.5 might result in an overflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should convert to a floating point hence range is different.


/// <summary>
Expand Down