Intrinsic arithmetic function overloads

[jrdodds]

Fixes #8698

Context

The MSBuild property functions for Add(), Subtract(), Multiply(), Divide(), and Modulo() have overloads for long and double. The 'fast path' resolution for the functions supports the double versions only. Both overloads should be supported.

Design

The changes are focused on the 'fast path' for intrinsic functions. To determine which overload to call, the arguments are checked to see if they can be converted to a long or, for a string, parsed to a long. If the arguments can be long, the long overload is called. Otherwise, the double overload is called.

From MSBuild XML, invocation of the double overload can be ensured by using a real literal, e.g. $([MSBuild]::Add(1, 0.0)) invokes the double overload.

With the long overload an overflow result that wraps is kept, e.g. -9223372036854775808 is the result of the following function call.

$([MSBuild]::Add($([System.Int64]::MaxValue), 1))

Preserve Existing Behavior for Equals and CompareTo

The following two code examples currently work because Add() always returns a double but there is an issue when the long override is invoked.

$([MSBuild]::Add(1,2).CompareTo(3.0))

$([MSBuild]::Add(1,2).Equals(3.0))

The Microsoft.Build.Evaluation.Expander<P, I>.Function<T>.Execute() method has an existing special case for handling Equals and CompareTo where the type of the argument to Equals or CompareTo is converted to match the type of the 'lhs' of the comparison. 3.0 can't be converted to a long. To preserve the existing behavior, the argument is tested and if it is a real literal the lhs of the comparison is converted to a double.

In the MSBuild XML the double to double comparison can be explicit by using a real literal or by converting to double as in the following examples.

$([MSBuild]::Add(1,2.0).CompareTo(3.0))

$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.0))

Changes Made

Implementation

Modified the Microsoft.Build.Evaluation.Expander<P, I>.Function<T> class:

• Added TryConvertToLong().
• Modified TryConvertToInt() and TryConvertToDouble().
• Added IsFloatingPointRepresentation().
• Added a set of TryExecute*() methods, e.g. TryExecuteAdd().
• Modified TryExecuteWellKnownFunction() to call the appropriate TryExecute*() method.
• Modified Execute() to support existing behavior for comparisons with Equals and CompareTo.
• Modified LateBindExecute() to support selecting the correct overload to call.
• Added static class IntrinsicFunctionOverload to provide cached comparer for sorting candidate overloads.

Unit Tests

Modified the Microsoft.Build.UnitTests.Evaluation.Expander_Tests class:

• Removed a test from PropertyFunctionStaticMethodIntrinsicMaths() that was commented "test for overflow wrapping" but was testing for a double result. Overflow wrapping is covered in PropertyFunctionMSBuildAdd().
• Modified Medley() to add some tests. The change to Medley() are related to the comparison behavior with Equals and CompareTo.
• Modified PropertyFunctionMSBuildAdd() to test double and long including integer overflow wrapping.
• Modified PropertyFunctionMSBuildSubtract(), PropertyFunctionMSBuildMultiply(), PropertyFunctionMSBuildDivide() to test long and double.
• Added PropertyFunctionMSBuildModulo().

Added the ExpanderFunction_Tests unit test class.

Added the IntrinsicFunctionOverload_Tests unit test class.

PR #8853

The changes in PR 8853 to use the InvariantCulture with double.TryParse have been included in this PR as well. If this PR is accepted and merged first, then issue #8798 and PR #8853 can also be closed based on this PR. But PR #8853 is smaller and can be accepted ahead of this PR.

Testing

Tested on Windows 11 and macOS 12.

Added new unit tests, extended existing tests, and ran all unit tests. The existing unit tests revealed the Equals / CompareTo comparison behavior.

Ran a test MSBuild XML project file. See below.

Notes

Negates PR #8649.

Test Project File

<!-- IntrinsicFunctionsOverload.proj -->
<Project>
  <Target Name="Testing" DependsOnTargets="Add;BadSubtract;Subtract;Multiply;Divide;Modulo;MedleyExamples" />

  <Target Name="Add">
    <!-- long -->
    <Message Text="$([MSBuild]::Add(1, 0)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Add(1, 1)) should be 2" Importance="High" />
    <Message Text="$([MSBuild]::Add(1, 2)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Add(-1, 2)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Add($([System.Int32]::MaxValue), 1)) should be 2147483648" Importance="High" />
    <Message Text="$([MSBuild]::Add(2147483648, 1)) should be 2147483649" Importance="High" />
    <Message Text="$([MSBuild]::Add($([System.Int64]::MaxValue), 1)) should be -9223372036854775808" Importance="High" />
    <!-- double -->
    <Message Text="$([MSBuild]::Add(1, 0.0)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Add(1, 0.5)) should be 1.5" Importance="High" />
    <Message Text="$([MSBuild]::Add(1, .5)) should be 1.5" Importance="High" />
    <Message Text="$([MSBuild]::Add(-1.0, 2)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Add(1.0, 2.0)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Add($([System.Int32]::MaxValue), 1.0)) should be 2147483648" Importance="High" />
    <!-- Int64 MaxValue is 9223372036854775807 -->
    <Message Text="$([MSBuild]::Add(9223372036854775808, 1)) should be 9.223372036854776E+18" Importance="High" />
  </Target>

  <Target Name="Test">
    <Message Text="$([MSBuild]::Add($([System.Int64]::MaxValue), 1)) should be -9223372036854775808" Importance="High" />
    <Message Text="$([MSBuild]::Add(9223372036854775808, 1)) should be 9.223372036854776E+18" Importance="High" />
    <Message Text="$([MSBuild]::Multiply($([System.Int64]::MaxValue), 2)) should be -2" Importance="High" />
    <Message Text="$([MSBuild]::Divide(65536, 10000)) should be 6" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(9223372036854775807, 9223372036854775806)) should be 1" Importance="High" />
  </Target>

  <!-- From Issue #8698 -->
  <Target Name="BadSubtract">
    <Message Text="$([MSBuild]::Subtract(9223372036854775807, 9223372036854775806)) should be 1" Importance="High" />
  </Target>

  <Target Name="Subtract">
    <Message Text="$([MSBuild]::Subtract(1, 0)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(1, 1)) should be 0" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(2, 1)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(1, 0.0)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(1, 0.5)) should be 0.5" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(2, -1.0)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Subtract(1.0, 2.0)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Subtract($([System.Int64]::MaxValue), 9223372036854775806)) should be 1" Importance="High" />
  </Target>

  <Target Name="Multiply">
    <Message Text="$([MSBuild]::Multiply(1, 0)) should be 0" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(1, 1)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(2, 1)) should be 2" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(1, 0.0)) should be 0" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(1, 0.5)) should be 0.5" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(2, -1.0)) should be -2" Importance="High" />
    <Message Text="$([MSBuild]::Multiply(1.0, 2.0)) should be 2" Importance="High" />
    <Message Text="$([MSBuild]::Multiply($([System.Int64]::MaxValue), 1)) should be 9223372036854775807" Importance="High" />
    <Message Text="$([MSBuild]::Multiply($([System.Int64]::MaxValue), 2)) should be -2" Importance="High" />
  </Target>

  <Target Name="Divide">
    <Message Text="$([MSBuild]::Divide(1, 1)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Divide(9, 3)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Divide(10, 3)) should be 3" Importance="High" />
    <Message Text="$([MSBuild]::Divide(65536, 10000)) should be 6" Importance="High" />
    <Message Text="$([MSBuild]::Divide(1, 0.5)) should be 2" Importance="High" />
    <Message Text="$([MSBuild]::Divide(10.0, 3)) should be 3.33..." Importance="High" />
  </Target>

  <Target Name="Modulo">
    <Message Text="$([MSBuild]::Modulo(1, 1)) should be 0" Importance="High" />
    <Message Text="$([MSBuild]::Modulo(9, 3)) should be 0" Importance="High" />
    <Message Text="$([MSBuild]::Modulo(9, 2)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Modulo(9, 2.0)) should be 1" Importance="High" />
    <Message Text="$([MSBuild]::Modulo(10.0, 3)) should be 1" Importance="High" />
  </Target>

  <!-- Long versus Double -->
  <Target Name="MedleyFail">
    <Message Text="$([MSBuild]::Add(1,2).CompareTo(3.0))" Importance="High" />
  </Target>

  <!-- Examples from the 'Medley' test -->
  <Target Name="MedleyExamples">
    <Message Text="$([MSBuild]::Add(1,2))" Importance="High" />
    <Message Text="$([MSBuild]::Add(1,2.0))" Importance="High" />
    <Message Text="$([MSBuild]::Add(1,2).CompareTo(3))" Importance="High" />
    <Message Text="$([MSBuild]::Add(1,2).CompareTo('3'))" Importance="High" />
    <Message Text="$([MSBuild]::Add(1,2).CompareTo(3.0))" Importance="High" />
    <Message Text="$([MSBuild]::Add(1,2.0).CompareTo(3.0))" Importance="High" />
    <Message Text="$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.0))" Importance="High" />
  </Target>
</Project>

[Forgind]

Two things:

Before we merge this, it'd be nice to validate that the fast path code now aligns properly with what would happen without the fast path. To do that, can you try ripping it all out and making sure all the tests pass as expected? (We'd want the fast path back before merging, of course.)

Second, although long division truncating values is what we've advertised in our documentation, it isn't what we're currently doing, and someone may have taken a dependency on 10/3 being 3.33... Can you add a ChangeWave for division?

I haven't looked too closely at your code yet, but the tests you posted in your comment look great to me. Thanks for working on this!

[jrdodds]

When testing with the fast path disabled, the LateBindExecute method stops searching when it finds the first method that could support the argument types and it finds the double versions of the arithmetic functions first.

Example:

$([MSBuild]::Add($([System.Int64]::MaxValue), 1))

produces a result of 9.22337203685478E+18 instead of the expected -9223372036854775808.

I'll make a change to LateBindExecute to choose methods with types that match over types that can be coerced.

I'll make the ChangeWave cover integer versus real for addition, subtraction, multiplication, and division.

(I also need to add tests for multiplication overflow.)

[Forgind]

When testing with the fast path disabled, the LateBindExecute method stops searching when it finds the first method that could support the argument types and it finds the double versions of the arithmetic functions first.

This was a bit surprising to me, but I can see why it would do that. Rather than any overly complicated logic, can we just reorder the Add methods, for instance, such that it tries to coerce the arguments to integers, then longs, then doubles? I'm trying to minimize the impact of the change while still getting the desired outcome.

[jrdodds]

Yes, the methods in the IntrinsicFunctions class could be re-ordered. I wasn't keen on doing that. Having functionality depend on the order of the methods in the source code seems brittle. It would need to be well commented or else it could be a very mysterious 'gotcha'. But more importantly there is a non-trivial behavior change that probably should be in a change wave. For the change wave mechanism, there needs to be a way to switch at runtime between the two behaviors.

[jrdodds]

The changes are using change wave 17.8 as a placeholder pending discussion.

[JanKrivanek]

This mostly looks good to me. Thanks for the improvements iterations on this!

I'd only like to see the handling of attempts of large doubles to long conversions and explicit unit test(s) for integer literals outside long bounds. Then I'm ready to sign off

[JanKrivanek]

Thank you for the contribution and iterations on it!

I think it's good to go now (we'll just need to properly docuemnt the behavior after merging)

[jrdodds]

@JanKrivanek Was there feedback on the decimal separator?

[JanKrivanek]

@JanKrivanek Jan Krivanek FTE Was there feedback on the decimal separator?

Not yet. I suppose we'll discuss this on Monday
Per our informal SOP the PR will need to be signed off by one another team member anyways

[ladipro]

Looks good overall. I've Added a few comments inline.

[jrdodds]

New commits to address #8798 within the scope of this work - i.e. code touched or changed by this PR now uses a different overload of double.Parse:

double.TryParse(str, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out arg)

This may not close #8798. The issue could be in other places.

[ladipro]

Apologies for the delay, the changes look great. I've added one question inline. Also, when you get a chance, can you please update the description - I believe that some parts got out of sync with the change.

[jrdodds]

The description has been updated.