From 202755bafc1ec195e5f5e7121622fc86ed95e93d Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 26 Apr 2023 14:38:50 -0400 Subject: [PATCH 01/23] Support long and double for arithmetic operator functions --- .../Evaluation/Expander_Tests.cs | 39 ++-- src/Build/Evaluation/Expander.cs | 175 ++++++++++++++++-- 2 files changed, 189 insertions(+), 25 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index b55b0dec344..6b9bd50e7d9 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -3394,12 +3394,6 @@ public void PropertyFunctionStaticMethodIntrinsicMaths() result = expander.ExpandIntoStringLeaveEscaped(@"$([MSBuild]::Modulo(2345.5, 43))", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); Assert.Equal((2345.5 % 43).ToString(), result); - - // test for overflow wrapping - result = expander.ExpandIntoStringLeaveEscaped(@"$([MSBuild]::Add(9223372036854775807, 20))", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); - - double expectedResult = 9223372036854775807D + 20D; - Assert.Equal(expectedResult.ToString(), result); } /// @@ -3692,14 +3686,16 @@ public void Medley() new string[] {@"$([System.Text.RegularExpressions.Regex]::Match($(Input), `EXPORT\s+(.+)`).Groups[1].Value)","a"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(3))", "0"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(3))", "0"}, - new string[] {"$([MSBuild]::Add(1,2).CompareTo(3.0))", "0"}, + new string[] {"$([MSBuild]::Add(1,2.0).CompareTo(3.0))", "0"}, + new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.0))", "0"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo('3'))", "0"}, - new string[] {"$([MSBuild]::Add(1,2).CompareTo(3.1))", "-1"}, + new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.1))", "-1"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(2))", "1"}, new string[] {"$([MSBuild]::Add(1,2).Equals(3))", "True"}, - new string[] {"$([MSBuild]::Add(1,2).Equals(3.0))", "True"}, + new string[] {"$([MSBuild]::Add(1,2.0).Equals(3.0))", "True"}, + new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).Equals(3.0))", "True"}, new string[] {"$([MSBuild]::Add(1,2).Equals('3'))", "True"}, - new string[] {"$([MSBuild]::Add(1,2).Equals(3.1))", "False"}, + new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).Equals(3.1))", "False"}, new string[] {"$(a.Insert(0,'%28'))", "%28no"}, new string[] {"$(a.Insert(0,'\"'))", "\"no"}, new string[] {"$(a.Insert(0,'(('))", "%28%28no"}, @@ -3858,7 +3854,11 @@ public void Medley() "$((((", "$($())", "$", - "()" + "()", + "$([MSBuild]::Add(1,2).CompareTo(3.0))", // Add() returns a long + "$([MSBuild]::Add(1,2).CompareTo(3.1))", + "$([MSBuild]::Add(1,2).Equals(3.0))", + "$([MSBuild]::Add(1,2).Equals(3.1))" }; #if !RUNTIME_TYPE_NETCORE @@ -4179,6 +4179,9 @@ public void PropertyFunctionMathMin() public void PropertyFunctionMSBuildAdd() { TestPropertyFunction("$([MSBuild]::Add($(X), 5))", "X", "7", "12"); + TestPropertyFunction("$([MSBuild]::Add($(X), 0.5))", "X", "7", "7.5"); + // Overflow wrapping + TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", long.MaxValue.ToString(), "-9223372036854775808"); } [Fact] @@ -4191,12 +4194,16 @@ public void PropertyFunctionMSBuildAddComplex() public void PropertyFunctionMSBuildSubtract() { TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000))", "X", "20100042", "42"); + TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000.0))", "X", "20100042", "42"); + // Overflow wrapping + TestPropertyFunction("$([MSBuild]::Subtract($(X), 9223372036854775806))", "X", long.MaxValue.ToString(), "1"); } [Fact] public void PropertyFunctionMSBuildMultiply() { TestPropertyFunction("$([MSBuild]::Multiply($(X), 8800))", "X", "2", "17600"); + TestPropertyFunction("$([MSBuild]::Multiply($(X), .5))", "X", "2", "1"); } [Fact] @@ -4208,7 +4215,15 @@ public void PropertyFunctionMSBuildMultiplyComplex() [Fact] public void PropertyFunctionMSBuildDivide() { - TestPropertyFunction("$([MSBuild]::Divide($(X), 10000))", "X", "65536", (6.5536).ToString()); + TestPropertyFunction("$([MSBuild]::Divide($(X), 10000))", "X", "65536", "6"); + TestPropertyFunction("$([MSBuild]::Divide($(X), 10000.0))", "X", "65536", "6.5536"); + } + + [Fact] + public void PropertyFunctionMSBuildModulo() + { + TestPropertyFunction("$([MSBuild]::Modulo($(X), 3))", "X", "10", "1"); + TestPropertyFunction("$([MSBuild]::Modulo($(X), 3.0))", "X", "10", "1"); } [Fact] diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 459217b3389..2ba63af71aa 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -78,7 +78,7 @@ internal enum ExpanderOptions /// When an error occurs expanding a property, just leave it unexpanded. /// /// - /// This should only be used in cases where property evaluation isn't critcal, such as when attempting to log a + /// This should only be used in cases where property evaluation isn't critical, such as when attempting to log a /// message with a best effort expansion of a string, or when discovering partial information during lazy evaluation. /// LeavePropertiesUnexpandedOnError = 0x20, @@ -285,7 +285,7 @@ private void FlushFirstValueIfNeeded() /// /// The CultureInfo from the invariant culture. Used to avoid allocations for - /// perfoming IndexOf etc. + /// performing IndexOf etc. /// private static CompareInfo s_invariantCompareInfo = CultureInfo.InvariantCulture.CompareInfo; @@ -3530,7 +3530,7 @@ internal object Execute(object objectInstance, IPropertyProvider properties, functionResult = _receiverType.InvokeMember(_methodMethodName, _bindingFlags, Type.DefaultBinder, objectInstance, args, CultureInfo.InvariantCulture); } // If we're invoking a method, then there are deeper attempts that can be made to invoke the method. - // If not, we were asked to get a property or field but found that we cannot locate it. No further argument coersion is possible, so throw. + // If not, we were asked to get a property or field but found that we cannot locate it. No further argument coercion is possible, so throw. catch (MissingMethodException ex) when ((_bindingFlags & BindingFlags.InvokeMethod) == BindingFlags.InvokeMethod) { // The standard binder failed, so do our best to coerce types into the arguments for the function @@ -3934,41 +3934,36 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Add), StringComparison.OrdinalIgnoreCase)) { - if (TryGetArgs(args, out double arg0, out double arg1)) + if (TryExecuteAdd(args, out returnVal)) { - returnVal = IntrinsicFunctions.Add(arg0, arg1); return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Subtract), StringComparison.OrdinalIgnoreCase)) { - if (TryGetArgs(args, out double arg0, out double arg1)) + if (TryExecuteSubtract(args, out returnVal)) { - returnVal = IntrinsicFunctions.Subtract(arg0, arg1); return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Multiply), StringComparison.OrdinalIgnoreCase)) { - if (TryGetArgs(args, out double arg0, out double arg1)) + if (TryExecuteMultiply(args, out returnVal)) { - returnVal = IntrinsicFunctions.Multiply(arg0, arg1); return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Divide), StringComparison.OrdinalIgnoreCase)) { - if (TryGetArgs(args, out double arg0, out double arg1)) + if (TryExecuteDivide(args, out returnVal)) { - returnVal = IntrinsicFunctions.Divide(arg0, arg1); return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Modulo), StringComparison.OrdinalIgnoreCase)) { - if (TryGetArgs(args, out double arg0, out double arg1)) + if (TryExecuteModulo(args, out returnVal)) { - returnVal = IntrinsicFunctions.Modulo(arg0, arg1); return true; } } @@ -4534,6 +4529,24 @@ private static bool TryConvertToInt(object value, out int arg0) return false; } + private static bool TryConvertToLong(object value, out long arg0) + { + switch (value) + { + case double d: + arg0 = Convert.ToInt64(d); + return arg0 == d; + case long i: + arg0 = i; + return true; + case string s when long.TryParse(s, out arg0): + return true; + } + + arg0 = 0; + return false; + } + private static bool TryConvertToDouble(object value, out double arg) { if (value is double unboxed) @@ -4677,6 +4690,142 @@ private static bool TryGetArgs(object[] args, out string arg0, out int arg1) return false; } + private static bool IsFloatingPointRepresentation(object value) + { + return value is double || + (value is string str && str.Contains(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator)); + } + + private static bool TryExecuteAdd(object[] args, out object resultValue) + { + resultValue = null; + + if (args.Length != 2) + { + return false; + } + + if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + { + if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + { + resultValue = IntrinsicFunctions.Add(arg0, arg1); + return true; + } + } + else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + { + resultValue = IntrinsicFunctions.Add(arg0, arg1); + return true; + } + + return false; + } + + private static bool TryExecuteSubtract(object[] args, out object resultValue) + { + resultValue = null; + + if (args.Length != 2) + { + return false; + } + + if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + { + if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + { + resultValue = IntrinsicFunctions.Subtract(arg0, arg1); + return true; + } + } + else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + { + resultValue = IntrinsicFunctions.Subtract(arg0, arg1); + return true; + } + + return false; + } + + private static bool TryExecuteMultiply(object[] args, out object resultValue) + { + resultValue = null; + + if (args.Length != 2) + { + return false; + } + + if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + { + if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + { + resultValue = IntrinsicFunctions.Multiply(arg0, arg1); + return true; + } + } + else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + { + resultValue = IntrinsicFunctions.Multiply(arg0, arg1); + return true; + } + + return false; + } + + private static bool TryExecuteDivide(object[] args, out object resultValue) + { + resultValue = null; + + if (args.Length != 2) + { + return false; + } + + if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + { + if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + { + resultValue = IntrinsicFunctions.Divide(arg0, arg1); + return true; + } + } + else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + { + resultValue = IntrinsicFunctions.Divide(arg0, arg1); + return true; + } + + return false; + } + + private static bool TryExecuteModulo(object[] args, out object resultValue) + { + resultValue = null; + + if (args.Length != 2) + { + return false; + } + + if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + { + if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + { + resultValue = IntrinsicFunctions.Modulo(arg0, arg1); + return true; + } + } + else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + { + resultValue = IntrinsicFunctions.Modulo(arg0, arg1); + return true; + } + + return false; + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void LogFunctionCall(string fileName, object objectInstance, object[] args) { From d01d30e5b64dea0791623e434f39d9256e47dc5b Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 26 Apr 2023 18:49:30 -0400 Subject: [PATCH 02/23] added argument that exceeds the size of long --- src/Build.UnitTests/Evaluation/Expander_Tests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 6b9bd50e7d9..08bf51b8e64 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -4180,8 +4180,14 @@ public void PropertyFunctionMSBuildAdd() { TestPropertyFunction("$([MSBuild]::Add($(X), 5))", "X", "7", "12"); TestPropertyFunction("$([MSBuild]::Add($(X), 0.5))", "X", "7", "7.5"); + // Overflow wrapping TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", long.MaxValue.ToString(), "-9223372036854775808"); + + // Argument exceeds size of long + double value = long.MaxValue + 1.0; + double expected = value + 1.0; + TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", value.ToString(), expected.ToString()); } [Fact] @@ -4195,7 +4201,6 @@ public void PropertyFunctionMSBuildSubtract() { TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000))", "X", "20100042", "42"); TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000.0))", "X", "20100042", "42"); - // Overflow wrapping TestPropertyFunction("$([MSBuild]::Subtract($(X), 9223372036854775806))", "X", long.MaxValue.ToString(), "1"); } From eb2d53f55be1fd9eec3c18abc9d6ab89b76cc627 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Thu, 27 Apr 2023 22:19:16 -0400 Subject: [PATCH 03/23] use a const char for the decimal separator --- src/Build/Evaluation/Expander.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 2ba63af71aa..e564c3ac963 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4692,8 +4692,9 @@ private static bool TryGetArgs(object[] args, out string arg0, out int arg1) private static bool IsFloatingPointRepresentation(object value) { - return value is double || - (value is string str && str.Contains(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator)); + const char numberDecimalSeparator = '.'; + + return value is double || (value is string str && str.Contains(numberDecimalSeparator)); } private static bool TryExecuteAdd(object[] args, out object resultValue) From 68b71d69247fdb48868169c314316336b9d910a4 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Fri, 28 Apr 2023 11:50:50 -0400 Subject: [PATCH 04/23] Support comparison when the lhs is an integer --- src/Build.UnitTests/Evaluation/Expander_Tests.cs | 12 +++++++----- src/Build/Evaluation/Expander.cs | 12 ++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 08bf51b8e64..db0df20ddff 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -3686,15 +3686,21 @@ public void Medley() new string[] {@"$([System.Text.RegularExpressions.Regex]::Match($(Input), `EXPORT\s+(.+)`).Groups[1].Value)","a"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(3))", "0"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(3))", "0"}, + new string[] {"$([MSBuild]::Add(1,2).CompareTo(3.0))", "0"}, new string[] {"$([MSBuild]::Add(1,2.0).CompareTo(3.0))", "0"}, new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.0))", "0"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo('3'))", "0"}, + new string[] {"$([MSBuild]::Add(1,2).CompareTo(3.1))", "-1"}, + new string[] {"$([MSBuild]::Add(1,2.0).CompareTo(3.1))", "-1"}, new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).CompareTo(3.1))", "-1"}, new string[] {"$([MSBuild]::Add(1,2).CompareTo(2))", "1"}, new string[] {"$([MSBuild]::Add(1,2).Equals(3))", "True"}, + new string[] {"$([MSBuild]::Add(1,2).Equals(3.0))", "True"}, new string[] {"$([MSBuild]::Add(1,2.0).Equals(3.0))", "True"}, new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).Equals(3.0))", "True"}, new string[] {"$([MSBuild]::Add(1,2).Equals('3'))", "True"}, + new string[] {"$([MSBuild]::Add(1,2).Equals(3.1))", "False"}, + new string[] {"$([MSBuild]::Add(1,2.0).Equals(3.1))", "False"}, new string[] {"$([System.Convert]::ToDouble($([MSBuild]::Add(1,2))).Equals(3.1))", "False"}, new string[] {"$(a.Insert(0,'%28'))", "%28no"}, new string[] {"$(a.Insert(0,'\"'))", "\"no"}, @@ -3854,11 +3860,7 @@ public void Medley() "$((((", "$($())", "$", - "()", - "$([MSBuild]::Add(1,2).CompareTo(3.0))", // Add() returns a long - "$([MSBuild]::Add(1,2).CompareTo(3.1))", - "$([MSBuild]::Add(1,2).Equals(3.0))", - "$([MSBuild]::Add(1,2).Equals(3.1))" + "()" }; #if !RUNTIME_TYPE_NETCORE diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index e564c3ac963..a4a13b4f6f8 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1338,7 +1338,7 @@ private static class PropertyExpander if (function != null) { // We will have either extracted the actual property name - // or realised that there is none (static function), and have recorded a null + // or realized that there is none (static function), and have recorded a null propertyName = function.Receiver; } else @@ -3463,6 +3463,13 @@ internal object Execute(object objectInstance, IPropertyProvider properties, // that it matches the left hand side ready for the default binder’s method invoke. if (objectInstance != null && args.Length == 1 && (String.Equals("Equals", _methodMethodName, StringComparison.OrdinalIgnoreCase) || String.Equals("CompareTo", _methodMethodName, StringComparison.OrdinalIgnoreCase))) { + // Support comparison when the lhs is an integer + if (IsFloatingPointRepresentation(args[0]) && !IsFloatingPointRepresentation(objectInstance)) + { + objectInstance = Convert.ChangeType(objectInstance, typeof(double), CultureInfo.InvariantCulture); + _receiverType = objectInstance.GetType(); + } + // change the type of the final unescaped string into the destination args[0] = Convert.ChangeType(args[0], objectInstance.GetType(), CultureInfo.InvariantCulture); } @@ -3470,14 +3477,11 @@ internal object Execute(object objectInstance, IPropertyProvider properties, if (_receiverType == typeof(IntrinsicFunctions)) { // Special case a few methods that take extra parameters that can't be passed in by the user - // - if (_methodMethodName.Equals("GetPathOfFileAbove") && args.Length == 1) { // Append the IElementLocation as a parameter to GetPathOfFileAbove if the user only // specified the file name. This is syntactic sugar so they don't have to always // include $(MSBuildThisFileDirectory) as a parameter. - // string startingDirectory = String.IsNullOrWhiteSpace(elementLocation.File) ? String.Empty : Path.GetDirectoryName(elementLocation.File); args = new[] From a5375f3de7692c97cab4ab6f10a491c588204553 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Fri, 5 May 2023 21:41:26 -0400 Subject: [PATCH 05/23] Order members by TypeCode; add tests and ChangeWave --- .../Evaluation/Expander_Tests.cs | 72 ++++- .../IntrinsicFunctionOverload_Tests.cs | 268 ++++++++++++++++++ src/Build/Evaluation/Expander.cs | 51 ++-- src/Framework/ChangeWaves.cs | 3 +- 4 files changed, 364 insertions(+), 30 deletions(-) create mode 100644 src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index db0df20ddff..69392976c03 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -4178,15 +4178,29 @@ public void PropertyFunctionMathMin() } [Fact] - public void PropertyFunctionMSBuildAdd() + public void PropertyFunctionMSBuildAddIntegerLiteral() { TestPropertyFunction("$([MSBuild]::Add($(X), 5))", "X", "7", "12"); + } + + [Fact] + public void PropertyFunctionMSBuildAddRealLiteral() + { TestPropertyFunction("$([MSBuild]::Add($(X), 0.5))", "X", "7", "7.5"); + } - // Overflow wrapping - TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", long.MaxValue.ToString(), "-9223372036854775808"); + [Fact] + public void PropertyFunctionMSBuildAddIntegerOverflow() + { + // Overflow wrapping - result exceeds size of long + string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "-9223372036854775808" : (long.MaxValue + 1.0).ToString(); + TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", long.MaxValue.ToString(), expected); + } - // Argument exceeds size of long + [Fact] + public void PropertyFunctionMSBuildAddRealArgument() + { + // string argument is an integer that exceeds the size of long. double value = long.MaxValue + 1.0; double expected = value + 1.0; TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", value.ToString(), expected.ToString()); @@ -4199,18 +4213,43 @@ public void PropertyFunctionMSBuildAddComplex() } [Fact] - public void PropertyFunctionMSBuildSubtract() + public void PropertyFunctionMSBuildSubtractIntegerLiteral() { TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000))", "X", "20100042", "42"); + } + + [Fact] + public void PropertyFunctionMSBuildSubtractRealLiteral() + { TestPropertyFunction("$([MSBuild]::Subtract($(X), 20100000.0))", "X", "20100042", "42"); - TestPropertyFunction("$([MSBuild]::Subtract($(X), 9223372036854775806))", "X", long.MaxValue.ToString(), "1"); } [Fact] - public void PropertyFunctionMSBuildMultiply() + public void PropertyFunctionMSBuildSubtractIntegerMaxValue() + { + // If the double overload is used, there will be a rounding error. + string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "1" : "0"; + TestPropertyFunction("$([MSBuild]::Subtract($(X), 9223372036854775806))", "X", long.MaxValue.ToString(), expected); + } + + [Fact] + public void PropertyFunctionMSBuildMultiplyIntegerLiteral() { TestPropertyFunction("$([MSBuild]::Multiply($(X), 8800))", "X", "2", "17600"); - TestPropertyFunction("$([MSBuild]::Multiply($(X), .5))", "X", "2", "1"); + } + + [Fact] + public void PropertyFunctionMSBuildMultiplyRealLiteral() + { + TestPropertyFunction("$([MSBuild]::Multiply($(X), 1.5))", "X", "2", "3"); + } + + [Fact] + public void PropertyFunctionMSBuildMultiplyIntegerOverflow() + { + // Overflow - result exceeds size of long + string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "-2" : (long.MaxValue * 2.0).ToString(); + TestPropertyFunction("$([MSBuild]::Multiply($(X), 2))", "X", long.MaxValue.ToString(), expected); } [Fact] @@ -4220,16 +4259,27 @@ public void PropertyFunctionMSBuildMultiplyComplex() } [Fact] - public void PropertyFunctionMSBuildDivide() + public void PropertyFunctionMSBuildDivideIntegerLiteral() + { + string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "6" : "6.5536"; + TestPropertyFunction("$([MSBuild]::Divide($(X), 10000))", "X", "65536", expected); + } + + [Fact] + public void PropertyFunctionMSBuildDivideRealLiteral() { - TestPropertyFunction("$([MSBuild]::Divide($(X), 10000))", "X", "65536", "6"); TestPropertyFunction("$([MSBuild]::Divide($(X), 10000.0))", "X", "65536", "6.5536"); } [Fact] - public void PropertyFunctionMSBuildModulo() + public void PropertyFunctionMSBuildModuloIntegerLiteral() { TestPropertyFunction("$([MSBuild]::Modulo($(X), 3))", "X", "10", "1"); + } + + [Fact] + public void PropertyFunctionMSBuildModuloRealLiteral() + { TestPropertyFunction("$([MSBuild]::Modulo($(X), 3.0))", "X", "10", "1"); } diff --git a/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs new file mode 100644 index 00000000000..33fc175f4d6 --- /dev/null +++ b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs @@ -0,0 +1,268 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using System.Xml; + +using Microsoft.Build.Evaluation; +using Microsoft.Build.Framework; +using Microsoft.Build.Shared; +using Microsoft.Build.UnitTests; + +using Shouldly; + +using Xunit; + +namespace Microsoft.Build.Engine.UnitTests.Evaluation +{ + public class IntrinsicFunctionOverload_Tests + { + private Version ChangeWaveForOverloading = ChangeWaves.Wave17_8; + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MSBuildAddInteger(bool enableIntrinsicFunctionOverloads) + { + const string projectContent = @" + + + $([MSBuild]::Add($([System.Int64]::MaxValue), 1)) + + "; + + string expected = enableIntrinsicFunctionOverloads ? unchecked(long.MaxValue + 1).ToString() : (long.MaxValue + 1.0).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + if (!enableIntrinsicFunctionOverloads) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + } + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildAddReal() + { + const string projectContent = @" + + + $([MSBuild]::Add(1.0, 2.0)) + + "; + + string expected = 3.0.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MSBuildSubtractInteger(bool enableIntrinsicFunctionOverloads) + { + const string projectContent = @" + + + $([MSBuild]::Subtract($([System.Int64]::MaxValue), 9223372036854775806)) + + "; + + string expected = enableIntrinsicFunctionOverloads ? 1.ToString() : 0.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + if (!enableIntrinsicFunctionOverloads) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + } + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildSubtractReal() + { + const string projectContent = @" + + + $([MSBuild]::Subtract(2.0, 1.0)) + + "; + + string expected = 1.0.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MSBuildMultiplyInteger(bool enableIntrinsicFunctionOverloads) + { + const string projectContent = @" + + + $([MSBuild]::Multiply($([System.Int64]::MaxValue), 2)) + + "; + + string expected = enableIntrinsicFunctionOverloads ? unchecked(long.MaxValue * 2).ToString() : (long.MaxValue * 2.0).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + if (!enableIntrinsicFunctionOverloads) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + } + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildMultiplyReal() + { + const string projectContent = @" + + + $([MSBuild]::Multiply(2.0, 1.0)) + + "; + + string expected = 2.0.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MSBuildDivideInteger(bool enableIntrinsicFunctionOverloads) + { + const string projectContent = @" + + + $([MSBuild]::Divide(10, 3)) + + "; + + string expected = enableIntrinsicFunctionOverloads ? (10 / 3).ToString() : (10.0 / 3.0).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + if (!enableIntrinsicFunctionOverloads) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + } + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildDivideReal() + { + const string projectContent = @" + + + $([MSBuild]::Divide(1, 0.5)) + + "; + + string expected = 2.0.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MSBuildModuloInteger(bool enableIntrinsicFunctionOverloads) + { + const string projectContent = @" + + + $([MSBuild]::Modulo(10, 3)) + + "; + + string expected = 1.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + if (!enableIntrinsicFunctionOverloads) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); + BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); + } + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildModuloReal() + { + const string projectContent = @" + + + $([MSBuild]::Modulo(11.0, 2.5)) + + "; + + string expected = 1.ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + } +} diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index a4a13b4f6f8..3be0b4ac397 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5337,13 +5337,20 @@ private static bool IsInstanceMethodAvailable(string methodName) return methodName != "GetType"; } + private static TypeCode SelectTypeOfFirstParameter(MethodBase method) + { + ParameterInfo[] parameters = method.GetParameters(); + return parameters.Length > 0 + ? Type.GetTypeCode(parameters[0].ParameterType) + : TypeCode.Empty; + } + /// /// Construct and instance of objectType based on the constructor or method arguments provided. /// Arguments must never be null. /// private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object objectInstance /* null unless instance method */, object[] args, bool isConstructor) { - // First let's try for a method where all arguments are strings.. Type[] types = new Type[_arguments.Length]; for (int n = 0; n < _arguments.Length; n++) @@ -5365,15 +5372,26 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o // search for a method with the right number of arguments if (memberInfo == null) { - MethodBase[] members; // Gather all methods that may match + IEnumerable members; if (isConstructor) { members = _receiverType.GetConstructors(bindingFlags); } else { - members = _receiverType.GetMethods(bindingFlags); + members = _receiverType.GetMethods(bindingFlags).Where(m => string.Equals(m.Name, _methodMethodName, StringComparison.OrdinalIgnoreCase)); + + if (_receiverType == typeof(IntrinsicFunctions)) + { + // Order by the TypeCode of the first parameter. + // When change wave is enabled, order long before double. + // Otherwise preserve prior behavior of double before long. + IComparer comparer = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) + ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) + : Comparer.Create((key0, key1) => key1.CompareTo(key0)); + members = members.OrderBy(SelectTypeOfFirstParameter, comparer); + } } foreach (MethodBase member in members) @@ -5383,22 +5401,19 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o // Simple match on name and number of params, we will be case insensitive if (parameters.Length == _arguments.Length) { - if (isConstructor || String.Equals(member.Name, _methodMethodName, StringComparison.OrdinalIgnoreCase)) + // Try to find a method with the right name, number of arguments and + // compatible argument types + // we have a match on the name and argument number + // now let's try to coerce the arguments we have + // into the arguments on the matching method + object[] coercedArguments = CoerceArguments(args, parameters); + + if (coercedArguments != null) { - // Try to find a method with the right name, number of arguments and - // compatible argument types - // we have a match on the name and argument number - // now let's try to coerce the arguments we have - // into the arguments on the matching method - object[] coercedArguments = CoerceArguments(args, parameters); - - if (coercedArguments != null) - { - // We have a complete match - memberInfo = member; - args = coercedArguments; - break; - } + // We have a complete match + memberInfo = member; + args = coercedArguments; + break; } } } diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index 2af7392c4d2..e3d6a13187c 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -27,7 +27,8 @@ internal class ChangeWaves internal static readonly Version Wave17_2 = new Version(17, 2); internal static readonly Version Wave17_4 = new Version(17, 4); internal static readonly Version Wave17_6 = new Version(17, 6); - internal static readonly Version[] AllWaves = { Wave17_2, Wave17_4, Wave17_6 }; + internal static readonly Version Wave17_8 = new Version(17, 8); + internal static readonly Version[] AllWaves = { Wave17_2, Wave17_4, Wave17_6, Wave17_8 }; /// /// Special value indicating that all features behind all Change Waves should be enabled. From 16670c7e87a335e77940ab6c969ed715f2af5f58 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Fri, 5 May 2023 22:06:21 -0400 Subject: [PATCH 06/23] Add change wave to fast path --- src/Build/Evaluation/Expander.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 3be0b4ac397..b5108284b17 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4710,7 +4710,7 @@ private static bool TryExecuteAdd(object[] args, out object resultValue) return false; } - if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4736,7 +4736,7 @@ private static bool TryExecuteSubtract(object[] args, out object resultValue) return false; } - if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4762,7 +4762,7 @@ private static bool TryExecuteMultiply(object[] args, out object resultValue) return false; } - if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4788,7 +4788,7 @@ private static bool TryExecuteDivide(object[] args, out object resultValue) return false; } - if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4814,7 +4814,7 @@ private static bool TryExecuteModulo(object[] args, out object resultValue) return false; } - if (IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -5345,6 +5345,8 @@ private static TypeCode SelectTypeOfFirstParameter(MethodBase method) : TypeCode.Empty; } + private static bool EnableIntrinsicFunctionOverloads() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); + /// /// Construct and instance of objectType based on the constructor or method arguments provided. /// Arguments must never be null. @@ -5387,7 +5389,7 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o // Order by the TypeCode of the first parameter. // When change wave is enabled, order long before double. // Otherwise preserve prior behavior of double before long. - IComparer comparer = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) + IComparer comparer = EnableIntrinsicFunctionOverloads() ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) : Comparer.Create((key0, key1) => key1.CompareTo(key0)); members = members.OrderBy(SelectTypeOfFirstParameter, comparer); From 7de86f041c696c91d6e717bb861fd87e5f4326bf Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Fri, 5 May 2023 22:43:37 -0400 Subject: [PATCH 07/23] add property in Expander_Tests; rename in other classes --- .../Evaluation/Expander_Tests.cs | 10 ++++--- .../IntrinsicFunctionOverload_Tests.cs | 28 +++++++++---------- src/Build/Evaluation/Expander.cs | 14 +++++----- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 69392976c03..43340ba2959 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -37,6 +37,8 @@ public class Expander_Tests private string _dateToParse = new DateTime(2010, 12, 25).ToString(CultureInfo.CurrentCulture); private static readonly string s_rootPathPrefix = NativeMethodsShared.IsWindows ? "C:\\" : Path.VolumeSeparatorChar.ToString(); + private static bool IsIntrinsicFunctionOverloadsEnabled => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); + [Fact] public void ExpandAllIntoTaskItems0() { @@ -4193,7 +4195,7 @@ public void PropertyFunctionMSBuildAddRealLiteral() public void PropertyFunctionMSBuildAddIntegerOverflow() { // Overflow wrapping - result exceeds size of long - string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "-9223372036854775808" : (long.MaxValue + 1.0).ToString(); + string expected = IsIntrinsicFunctionOverloadsEnabled ? "-9223372036854775808" : (long.MaxValue + 1.0).ToString(); TestPropertyFunction("$([MSBuild]::Add($(X), 1))", "X", long.MaxValue.ToString(), expected); } @@ -4228,7 +4230,7 @@ public void PropertyFunctionMSBuildSubtractRealLiteral() public void PropertyFunctionMSBuildSubtractIntegerMaxValue() { // If the double overload is used, there will be a rounding error. - string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "1" : "0"; + string expected = IsIntrinsicFunctionOverloadsEnabled ? "1" : "0"; TestPropertyFunction("$([MSBuild]::Subtract($(X), 9223372036854775806))", "X", long.MaxValue.ToString(), expected); } @@ -4248,7 +4250,7 @@ public void PropertyFunctionMSBuildMultiplyRealLiteral() public void PropertyFunctionMSBuildMultiplyIntegerOverflow() { // Overflow - result exceeds size of long - string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "-2" : (long.MaxValue * 2.0).ToString(); + string expected = IsIntrinsicFunctionOverloadsEnabled ? "-2" : (long.MaxValue * 2.0).ToString(); TestPropertyFunction("$([MSBuild]::Multiply($(X), 2))", "X", long.MaxValue.ToString(), expected); } @@ -4261,7 +4263,7 @@ public void PropertyFunctionMSBuildMultiplyComplex() [Fact] public void PropertyFunctionMSBuildDivideIntegerLiteral() { - string expected = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8) ? "6" : "6.5536"; + string expected = IsIntrinsicFunctionOverloadsEnabled ? "6" : "6.5536"; TestPropertyFunction("$([MSBuild]::Divide($(X), 10000))", "X", "65536", expected); } diff --git a/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs index 33fc175f4d6..0256a871a56 100644 --- a/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs +++ b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs @@ -23,7 +23,7 @@ public class IntrinsicFunctionOverload_Tests [Theory] [InlineData(true)] [InlineData(false)] - public void MSBuildAddInteger(bool enableIntrinsicFunctionOverloads) + public void MSBuildAddInteger(bool isIntrinsicFunctionOverloadsEnabled) { const string projectContent = @" @@ -32,12 +32,12 @@ public void MSBuildAddInteger(bool enableIntrinsicFunctionOverloads) "; - string expected = enableIntrinsicFunctionOverloads ? unchecked(long.MaxValue + 1).ToString() : (long.MaxValue + 1.0).ToString(); + string expected = isIntrinsicFunctionOverloadsEnabled ? unchecked(long.MaxValue + 1).ToString() : (long.MaxValue + 1.0).ToString(); using TestEnvironment env = TestEnvironment.Create(); ChangeWaves.ResetStateForTests(); - if (!enableIntrinsicFunctionOverloads) + if (!isIntrinsicFunctionOverloadsEnabled) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); @@ -72,7 +72,7 @@ public void MSBuildAddReal() [Theory] [InlineData(true)] [InlineData(false)] - public void MSBuildSubtractInteger(bool enableIntrinsicFunctionOverloads) + public void MSBuildSubtractInteger(bool isIntrinsicFunctionOverloadsEnabled) { const string projectContent = @" @@ -81,12 +81,12 @@ public void MSBuildSubtractInteger(bool enableIntrinsicFunctionOverloads) "; - string expected = enableIntrinsicFunctionOverloads ? 1.ToString() : 0.ToString(); + string expected = isIntrinsicFunctionOverloadsEnabled ? 1.ToString() : 0.ToString(); using TestEnvironment env = TestEnvironment.Create(); ChangeWaves.ResetStateForTests(); - if (!enableIntrinsicFunctionOverloads) + if (!isIntrinsicFunctionOverloadsEnabled) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); @@ -121,7 +121,7 @@ public void MSBuildSubtractReal() [Theory] [InlineData(true)] [InlineData(false)] - public void MSBuildMultiplyInteger(bool enableIntrinsicFunctionOverloads) + public void MSBuildMultiplyInteger(bool isIntrinsicFunctionOverloadsEnabled) { const string projectContent = @" @@ -130,12 +130,12 @@ public void MSBuildMultiplyInteger(bool enableIntrinsicFunctionOverloads) "; - string expected = enableIntrinsicFunctionOverloads ? unchecked(long.MaxValue * 2).ToString() : (long.MaxValue * 2.0).ToString(); + string expected = isIntrinsicFunctionOverloadsEnabled ? unchecked(long.MaxValue * 2).ToString() : (long.MaxValue * 2.0).ToString(); using TestEnvironment env = TestEnvironment.Create(); ChangeWaves.ResetStateForTests(); - if (!enableIntrinsicFunctionOverloads) + if (!isIntrinsicFunctionOverloadsEnabled) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); @@ -170,7 +170,7 @@ public void MSBuildMultiplyReal() [Theory] [InlineData(true)] [InlineData(false)] - public void MSBuildDivideInteger(bool enableIntrinsicFunctionOverloads) + public void MSBuildDivideInteger(bool isIntrinsicFunctionOverloadsEnabled) { const string projectContent = @" @@ -179,12 +179,12 @@ public void MSBuildDivideInteger(bool enableIntrinsicFunctionOverloads) "; - string expected = enableIntrinsicFunctionOverloads ? (10 / 3).ToString() : (10.0 / 3.0).ToString(); + string expected = isIntrinsicFunctionOverloadsEnabled ? (10 / 3).ToString() : (10.0 / 3.0).ToString(); using TestEnvironment env = TestEnvironment.Create(); ChangeWaves.ResetStateForTests(); - if (!enableIntrinsicFunctionOverloads) + if (!isIntrinsicFunctionOverloadsEnabled) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); @@ -219,7 +219,7 @@ public void MSBuildDivideReal() [Theory] [InlineData(true)] [InlineData(false)] - public void MSBuildModuloInteger(bool enableIntrinsicFunctionOverloads) + public void MSBuildModuloInteger(bool isIntrinsicFunctionOverloadsEnabled) { const string projectContent = @" @@ -233,7 +233,7 @@ public void MSBuildModuloInteger(bool enableIntrinsicFunctionOverloads) using TestEnvironment env = TestEnvironment.Create(); ChangeWaves.ResetStateForTests(); - if (!enableIntrinsicFunctionOverloads) + if (!isIntrinsicFunctionOverloadsEnabled) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaveForOverloading.ToString()); BuildEnvironmentHelper.ResetInstance_ForUnitTestsOnly(); diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index b5108284b17..b85cee18bb2 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4710,7 +4710,7 @@ private static bool TryExecuteAdd(object[] args, out object resultValue) return false; } - if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4736,7 +4736,7 @@ private static bool TryExecuteSubtract(object[] args, out object resultValue) return false; } - if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4762,7 +4762,7 @@ private static bool TryExecuteMultiply(object[] args, out object resultValue) return false; } - if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4788,7 +4788,7 @@ private static bool TryExecuteDivide(object[] args, out object resultValue) return false; } - if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -4814,7 +4814,7 @@ private static bool TryExecuteModulo(object[] args, out object resultValue) return false; } - if (!EnableIntrinsicFunctionOverloads() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) { if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) { @@ -5345,7 +5345,7 @@ private static TypeCode SelectTypeOfFirstParameter(MethodBase method) : TypeCode.Empty; } - private static bool EnableIntrinsicFunctionOverloads() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); + private static bool IsIntrinsicFunctionOverloadsEnabled() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); /// /// Construct and instance of objectType based on the constructor or method arguments provided. @@ -5389,7 +5389,7 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o // Order by the TypeCode of the first parameter. // When change wave is enabled, order long before double. // Otherwise preserve prior behavior of double before long. - IComparer comparer = EnableIntrinsicFunctionOverloads() + IComparer comparer = IsIntrinsicFunctionOverloadsEnabled() ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) : Comparer.Create((key0, key1) => key1.CompareTo(key0)); members = members.OrderBy(SelectTypeOfFirstParameter, comparer); From 49b0afdf8e501acdce6e2d829a0fba3060f687c7 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 10 May 2023 19:48:35 -0400 Subject: [PATCH 08/23] reduce code repetition; try double if long fails in the same method --- src/Build/Evaluation/Expander.cs | 125 +++---------------------------- 1 file changed, 11 insertions(+), 114 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index b85cee18bb2..fa6c71b023a 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -3938,35 +3938,35 @@ private bool TryExecuteWellKnownFunction(out object returnVal, object objectInst } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Add), StringComparison.OrdinalIgnoreCase)) { - if (TryExecuteAdd(args, out returnVal)) + if (TryExecuteArithmeticOverload(args, IntrinsicFunctions.Add, IntrinsicFunctions.Add, out returnVal)) { return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Subtract), StringComparison.OrdinalIgnoreCase)) { - if (TryExecuteSubtract(args, out returnVal)) + if (TryExecuteArithmeticOverload(args, IntrinsicFunctions.Subtract, IntrinsicFunctions.Subtract, out returnVal)) { return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Multiply), StringComparison.OrdinalIgnoreCase)) { - if (TryExecuteMultiply(args, out returnVal)) + if (TryExecuteArithmeticOverload(args, IntrinsicFunctions.Multiply, IntrinsicFunctions.Multiply, out returnVal)) { return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Divide), StringComparison.OrdinalIgnoreCase)) { - if (TryExecuteDivide(args, out returnVal)) + if (TryExecuteArithmeticOverload(args, IntrinsicFunctions.Divide, IntrinsicFunctions.Divide, out returnVal)) { return true; } } else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Modulo), StringComparison.OrdinalIgnoreCase)) { - if (TryExecuteModulo(args, out returnVal)) + if (TryExecuteArithmeticOverload(args, IntrinsicFunctions.Modulo, IntrinsicFunctions.Modulo, out returnVal)) { return true; } @@ -4701,7 +4701,7 @@ private static bool IsFloatingPointRepresentation(object value) return value is double || (value is string str && str.Contains(numberDecimalSeparator)); } - private static bool TryExecuteAdd(object[] args, out object resultValue) + private static bool TryExecuteArithmeticOverload(object[] args, Func integerOperation, Func realOperation, out object resultValue) { resultValue = null; @@ -4710,121 +4710,18 @@ private static bool TryExecuteAdd(object[] args, out object resultValue) return false; } - if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) + if (IsIntrinsicFunctionOverloadsEnabled() && !IsFloatingPointRepresentation(args[0]) && !IsFloatingPointRepresentation(args[1])) { - if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) + if (TryConvertToLong(args[0], out long argLong0) && TryConvertToLong(args[1], out long argLong1)) { - resultValue = IntrinsicFunctions.Add(arg0, arg1); + resultValue = integerOperation(argLong0, argLong1); return true; } } - else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) - { - resultValue = IntrinsicFunctions.Add(arg0, arg1); - return true; - } - - return false; - } - - private static bool TryExecuteSubtract(object[] args, out object resultValue) - { - resultValue = null; - - if (args.Length != 2) - { - return false; - } - - if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) - { - if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) - { - resultValue = IntrinsicFunctions.Subtract(arg0, arg1); - return true; - } - } - else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) - { - resultValue = IntrinsicFunctions.Subtract(arg0, arg1); - return true; - } - - return false; - } - - private static bool TryExecuteMultiply(object[] args, out object resultValue) - { - resultValue = null; - - if (args.Length != 2) - { - return false; - } - if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) - { - if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) - { - resultValue = IntrinsicFunctions.Multiply(arg0, arg1); - return true; - } - } - else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) - { - resultValue = IntrinsicFunctions.Multiply(arg0, arg1); - return true; - } - - return false; - } - - private static bool TryExecuteDivide(object[] args, out object resultValue) - { - resultValue = null; - - if (args.Length != 2) - { - return false; - } - - if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) - { - if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) - { - resultValue = IntrinsicFunctions.Divide(arg0, arg1); - return true; - } - } - else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) - { - resultValue = IntrinsicFunctions.Divide(arg0, arg1); - return true; - } - - return false; - } - - private static bool TryExecuteModulo(object[] args, out object resultValue) - { - resultValue = null; - - if (args.Length != 2) - { - return false; - } - - if (!IsIntrinsicFunctionOverloadsEnabled() || IsFloatingPointRepresentation(args[0]) || IsFloatingPointRepresentation(args[1])) - { - if (TryConvertToDouble(args[0], out double arg0) && TryConvertToDouble(args[1], out double arg1)) - { - resultValue = IntrinsicFunctions.Modulo(arg0, arg1); - return true; - } - } - else if (TryConvertToLong(args[0], out long arg0) && TryConvertToLong(args[1], out long arg1)) + if (TryConvertToDouble(args[0], out double argDouble0) && TryConvertToDouble(args[1], out double argDouble1)) { - resultValue = IntrinsicFunctions.Modulo(arg0, arg1); + resultValue = realOperation(argDouble0, argDouble1); return true; } From 792e61a9899a50a1f88254d52b5e597385663da7 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Thu, 11 May 2023 14:08:06 -0400 Subject: [PATCH 09/23] modify TryConvertToLong to check long min/max values before converting double to long; add tests for values that exceed long min/max --- .../IntrinsicFunctionOverload_Tests.cs | 210 ++++++++++++++++++ src/Build/Evaluation/Expander.cs | 9 +- 2 files changed, 217 insertions(+), 2 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs index 0256a871a56..aa43ed04e22 100644 --- a/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs +++ b/src/Build.UnitTests/Evaluation/IntrinsicFunctionOverload_Tests.cs @@ -48,6 +48,48 @@ public void MSBuildAddInteger(bool isIntrinsicFunctionOverloadsEnabled) actualProperty.EvaluatedValue.ShouldBe(expected); } + [Fact] + public void MSBuildAddIntegerGreaterThanMax() + { + const string projectContent = @" + + + $([MSBuild]::Add(9223372036854775808, 1)) + + "; + + string expected = ((long.MaxValue +1D) + 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildAddIntegerLessThanMin() + { + const string projectContent = @" + + + $([MSBuild]::Add(-9223372036854775809, 1)) + + "; + + string expected = ((long.MinValue - 1D) + 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + [Fact] public void MSBuildAddReal() { @@ -97,6 +139,48 @@ public void MSBuildSubtractInteger(bool isIntrinsicFunctionOverloadsEnabled) actualProperty.EvaluatedValue.ShouldBe(expected); } + [Fact] + public void MSBuildSubtractIntegerGreaterThanMax() + { + const string projectContent = @" + + + $([MSBuild]::Subtract(9223372036854775808, 1)) + + "; + + string expected = ((long.MaxValue + 1D) - 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildSubtractIntegerLessThanMin() + { + const string projectContent = @" + + + $([MSBuild]::Subtract(-9223372036854775809, 1)) + + "; + + string expected = ((long.MinValue - 1D) - 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + [Fact] public void MSBuildSubtractReal() { @@ -146,6 +230,48 @@ public void MSBuildMultiplyInteger(bool isIntrinsicFunctionOverloadsEnabled) actualProperty.EvaluatedValue.ShouldBe(expected); } + [Fact] + public void MSBuildMultiplyIntegerGreaterThanMax() + { + const string projectContent = @" + + + $([MSBuild]::Multiply(9223372036854775808, 1)) + + "; + + string expected = ((long.MaxValue + 1D) * 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildMultiplyIntegerLessThanMin() + { + const string projectContent = @" + + + $([MSBuild]::Multiply(-9223372036854775809, 1)) + + "; + + string expected = ((long.MinValue - 1D) * 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + [Fact] public void MSBuildMultiplyReal() { @@ -195,6 +321,48 @@ public void MSBuildDivideInteger(bool isIntrinsicFunctionOverloadsEnabled) actualProperty.EvaluatedValue.ShouldBe(expected); } + [Fact] + public void MSBuildDivideIntegerGreaterThanMax() + { + const string projectContent = @" + + + $([MSBuild]::Divide(9223372036854775808, 1)) + + "; + + string expected = ((long.MaxValue + 1D) / 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildDivideIntegerLessThanMin() + { + const string projectContent = @" + + + $([MSBuild]::Divide(-9223372036854775809, 1)) + + "; + + string expected = ((long.MinValue - 1D) / 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + [Fact] public void MSBuildDivideReal() { @@ -244,6 +412,48 @@ public void MSBuildModuloInteger(bool isIntrinsicFunctionOverloadsEnabled) actualProperty.EvaluatedValue.ShouldBe(expected); } + [Fact] + public void MSBuildModuloIntegerGreaterThanMax() + { + const string projectContent = @" + + + $([MSBuild]::Modulo(9223372036854775808, 1)) + + "; + + string expected = ((long.MaxValue + 1D) % 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + + [Fact] + public void MSBuildModuloIntegerLessThanMin() + { + const string projectContent = @" + + + $([MSBuild]::Modulo(-9223372036854775809, 1)) + + "; + + string expected = ((long.MinValue - 1D) % 1).ToString(); + + using TestEnvironment env = TestEnvironment.Create(); + + ChangeWaves.ResetStateForTests(); + + var project = new Project(XmlReader.Create(new StringReader(projectContent.Cleanup()))); + ProjectProperty? actualProperty = project.GetProperty("Actual"); + actualProperty.EvaluatedValue.ShouldBe(expected); + } + [Fact] public void MSBuildModuloReal() { diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index fa6c71b023a..3a49f199838 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4538,8 +4538,13 @@ private static bool TryConvertToLong(object value, out long arg0) switch (value) { case double d: - arg0 = Convert.ToInt64(d); - return arg0 == d; + if (d >= long.MinValue && d <= long.MaxValue) + { + arg0 = Convert.ToInt64(d); + return arg0 == d; + } + + break; case long i: arg0 = i; return true; From 333f24273f2bb7fd37f27bab6e65e068d3c98233 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Thu, 11 May 2023 14:10:08 -0400 Subject: [PATCH 10/23] modify TryConvertToInt to check int min/max before converting to double --- src/Build/Evaluation/Expander.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 3a49f199838..5175e27811c 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4520,8 +4520,13 @@ private static bool TryConvertToInt(object value, out int arg0) switch (value) { case double d: - arg0 = Convert.ToInt32(d); - return arg0 == d; + if (d >= int.MinValue && d <= int.MaxValue) + { + arg0 = Convert.ToInt32(d); + return arg0 == d; + } + + break; case int i: arg0 = i; return true; From 6369e80b073f43324ddc78944b8ae9b7f3a23475 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Sat, 20 May 2023 18:30:18 -0400 Subject: [PATCH 11/23] change access to TryConvertToInt, TryConvertToLong, TryConvertToDouble; add unit tests; change functions based on unit test findings --- .../Evaluation/ExpanderFunction_Tests.cs | 239 ++++++++++++++++++ src/Build/Evaluation/Expander.cs | 72 ++++-- 2 files changed, 286 insertions(+), 25 deletions(-) create mode 100644 src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs diff --git a/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs new file mode 100644 index 00000000000..bdae9b44695 --- /dev/null +++ b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs @@ -0,0 +1,239 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using Microsoft.Build.Evaluation; + +using Shouldly; + +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.Build.Engine.UnitTests.Evaluation +{ + public class ExpanderFunction_Tests + { + private readonly ITestOutputHelper _output; + + public ExpanderFunction_Tests(ITestOutputHelper output) => _output = output; + + /* Tests for TryConvertToInt */ + + [Fact] + public void TryConvertToIntGivenNull() + { + Expander.Function.TryConvertToInt(null, out int actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToIntGivenDouble() + { + const double value = 10.0; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToIntGivenLong() + { + const long value = 10; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToIntGivenInt() + { + const int value = 10; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToIntGivenString() + { + const string value = "10"; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToIntGivenDoubleWithIntMinValue() + { + const int expected = int.MinValue; + const double value = expected; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(expected); + } + + [Fact] + public void TryConvertToIntGivenDoubleWithIntMaxValue() + { + const int expected = int.MaxValue; + const double value = expected; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeTrue(); + actual.ShouldBe(expected); + } + + [Fact] + public void TryConvertToIntGivenDoubleWithLessThanIntMinValue() + { + const double value = int.MinValue - 1.0; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToIntGivenDoubleWithGreaterThanIntMaxValue() + { + const double value = int.MaxValue + 1.0; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToIntGivenLongWithGreaterThanIntMaxValue() + { + const long value = int.MaxValue + 1L; + Expander.Function.TryConvertToInt(value, out int actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + /* Tests for TryConvertToLong */ + + [Fact] + public void TryConvertToLongGivenNull() + { + Expander.Function.TryConvertToLong(null, out long actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToLongGivenDouble() + { + const double value = 10.0; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToLongGivenLong() + { + const long value = 10; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToLongGivenInt() + { + const int value = 10; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToLongGivenString() + { + const string value = "10"; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(10); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithLongMinValue() + { + const long expected = long.MinValue; + const double value = expected; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(expected); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithLongMaxValueShouldNotThrow() + { + // An OverflowException should not be thrown from TryConvertToLong(). + // Convert.ToInt64(double) has a defect and will throw an OverflowException + // for values >= (long.MaxValue - 511) and <= long.MaxValue. + _ = Should.NotThrow(() => Expander.Function.TryConvertToLong((double)long.MaxValue, out _)); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithLongMaxValue() + { + // Because of loss of precision, long.MaxValue will not 'round trip' from long to double to long. + Expander.Function.TryConvertToLong((double)long.MaxValue, out long actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithVeryLargeLongValue() + { + // Because of loss of precision, veryLargeLong will not 'round trip' but within TryConvertToLong + // the double to long conversion will pass the tolerance test. Return will be true and veryLargeLong != expected. + const long veryLargeLong = long.MaxValue - 512; + const double value = veryLargeLong; + const long expected = 9223372036854774784L; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeTrue(); + actual.ShouldBe(expected); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithLessThanLongMinValue() + { + const double value = -92233720368547758081D; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToLongGivenDoubleWithGreaterThanLongMaxValue() + { + const double value = (double)long.MaxValue + long.MaxValue; + Expander.Function.TryConvertToLong(value, out long actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + /* Tests for TryConvertToDouble */ + + [Fact] + public void TryConvertToDoubleGivenNull() + { + Expander.Function.TryConvertToDouble(null, out double actual).ShouldBeFalse(); + actual.ShouldBe(0); + } + + [Fact] + public void TryConvertToDoubleGivenDouble() + { + const double value = 10.0; + Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); + actual.ShouldBe(10.0); + } + + [Fact] + public void TryConvertToDoubleGivenLong() + { + const long value = 10; + Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); + actual.ShouldBe(10.0); + } + + [Fact] + public void TryConvertToDoubleGivenInt() + { + const int value = 10; + Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); + actual.ShouldBe(10.0); + } + + [Fact] + public void TryConvertToDoubleGivenString() + { + const string value = "10"; + Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); + actual.ShouldBe(10.0); + } + } +} diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 5175e27811c..5ec00c1d588 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -3162,7 +3162,7 @@ private struct FunctionBuilder /// It is also responsible for executing the function. /// /// Type of the properties used to expand the expression. - private class Function + internal class Function where T : class, IProperty { /// @@ -4515,66 +4515,88 @@ private static bool TryConvertToVersion(object value, out Version arg0) return true; } - private static bool TryConvertToInt(object value, out int arg0) + internal static bool TryConvertToInt(object value, out int arg) { switch (value) { case double d: if (d >= int.MinValue && d <= int.MaxValue) { - arg0 = Convert.ToInt32(d); - return arg0 == d; + arg = Convert.ToInt32(d); + if (Math.Abs(arg - d) == 0) + { + return true; + } + } + + break; + case long l: + if (l >= int.MinValue && l <= int.MaxValue) + { + arg = Convert.ToInt32(l); + return true; } break; case int i: - arg0 = i; + arg = i; return true; - case string s when int.TryParse(s, out arg0): + case string s when int.TryParse(s, out arg): return true; } - arg0 = 0; + arg = 0; return false; } - private static bool TryConvertToLong(object value, out long arg0) + internal static bool TryConvertToLong(object value, out long arg) { switch (value) { case double d: if (d >= long.MinValue && d <= long.MaxValue) { - arg0 = Convert.ToInt64(d); - return arg0 == d; + arg = (long)d; + if (Math.Abs(arg - d) == 0) + { + return true; + } } break; - case long i: - arg0 = i; + case long l: + arg = l; + return true; + case int i: + arg = i; return true; - case string s when long.TryParse(s, out arg0): + case string s when long.TryParse(s, out arg): return true; } - arg0 = 0; + arg = 0; return false; } - private static bool TryConvertToDouble(object value, out double arg) + internal static bool TryConvertToDouble(object value, out double arg) { - if (value is double unboxed) - { - arg = unboxed; - return true; - } - else if (value is string str && double.TryParse(str, out arg)) + switch (value) { - return true; + case double unboxed: + arg = unboxed; + return true; + case long l: + arg = l; + return true; + case int i: + arg = i; + return true; + case string str when double.TryParse(str, out arg): + return true; + default: + arg = 0; + return false; } - - arg = 0; - return false; } private static bool TryGetArg(object[] args, out string arg0) From e1e705f6cb23c8019f4f24ad5f926619bee3beca Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Sat, 20 May 2023 19:13:32 -0400 Subject: [PATCH 12/23] chnages to rely on TryParse --- src/Build/Evaluation/Expander.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 5ec00c1d588..44a7dcbadad 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4728,9 +4728,7 @@ private static bool TryGetArgs(object[] args, out string arg0, out int arg1) private static bool IsFloatingPointRepresentation(object value) { - const char numberDecimalSeparator = '.'; - - return value is double || (value is string str && str.Contains(numberDecimalSeparator)); + return value is double || (value is string str && double.TryParse(str, out double _)); } private static bool TryExecuteArithmeticOverload(object[] args, Func integerOperation, Func realOperation, out object resultValue) @@ -4742,7 +4740,7 @@ private static bool TryExecuteArithmeticOverload(object[] args, Func Date: Sun, 21 May 2023 10:49:41 -0400 Subject: [PATCH 13/23] modify test TryConvertToLongGivenDoubleWithLongMaxValue for Apple Silicon --- .../Evaluation/ExpanderFunction_Tests.cs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs index bdae9b44695..f8089b7337c 100644 --- a/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs @@ -1,7 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; +using System.Runtime.InteropServices; using Microsoft.Build.Evaluation; using Shouldly; @@ -162,9 +162,20 @@ public void TryConvertToLongGivenDoubleWithLongMaxValueShouldNotThrow() [Fact] public void TryConvertToLongGivenDoubleWithLongMaxValue() { - // Because of loss of precision, long.MaxValue will not 'round trip' from long to double to long. - Expander.Function.TryConvertToLong((double)long.MaxValue, out long actual).ShouldBeFalse(); - actual.ShouldBe(0); + const long longMaxValue = long.MaxValue; + bool result = Expander.Function.TryConvertToLong((double)longMaxValue, out long actual); + if (RuntimeInformation.OSArchitecture != Architecture.Arm64) + { + // Because of loss of precision, long.MaxValue will not 'round trip' from long to double to long. + result.ShouldBeFalse(); + actual.ShouldBe(0); + } + else + { + // Testing on macOS 12 on Apple Silicon M1 Pro produces different result. + result.ShouldBeTrue(); + actual.ShouldBe(longMaxValue); + } } [Fact] From 158e6e64b7d99ebfb0330a42f634039daa7ca328 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Mon, 22 May 2023 11:59:51 -0400 Subject: [PATCH 14/23] cache comparer for arithmetic overloads --- src/Build/Evaluation/Expander.cs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 44a7dcbadad..549747e5989 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4740,7 +4740,7 @@ private static bool TryExecuteArithmeticOverload(object[] args, Func ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); - /// /// Construct and instance of objectType based on the constructor or method arguments provided. /// Arguments must never be null. @@ -5313,13 +5311,7 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o if (_receiverType == typeof(IntrinsicFunctions)) { - // Order by the TypeCode of the first parameter. - // When change wave is enabled, order long before double. - // Otherwise preserve prior behavior of double before long. - IComparer comparer = IsIntrinsicFunctionOverloadsEnabled() - ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) - : Comparer.Create((key0, key1) => key1.CompareTo(key0)); - members = members.OrderBy(SelectTypeOfFirstParameter, comparer); + members = members.OrderBy(SelectTypeOfFirstParameter, IntrinsicFunctionOverload.IntrinsicFunctionOverloadComparer); } } @@ -5417,4 +5409,23 @@ internal string CurrentlyEvaluatingPropertyElementName set; } } + + internal static class IntrinsicFunctionOverload + { + private static IComparer s_intrinsicFunctionOverloadComparer; + + // Order by the TypeCode of the first parameter. + // When change wave is enabled, order long before double. + // Otherwise preserve prior behavior of double before long. + // For reuse, the comparer is cached in a non-generic type. + internal static IComparer IntrinsicFunctionOverloadComparer => + s_intrinsicFunctionOverloadComparer ??= IsIntrinsicFunctionOverloadsEnabled() + ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) + : Comparer.Create((key0, key1) => key1.CompareTo(key0)); + + /* When the change wave is retired, the expression body should be changed to + s_intrinsicFunctionOverloadComparer ??= Comparer.Create((key0, key1) => key0.CompareTo(key1)); . */ + + internal static bool IsIntrinsicFunctionOverloadsEnabled() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); + } } From f2cb002fb1c282af42ed1cf45458a8284ca457e3 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Mon, 22 May 2023 15:46:20 -0400 Subject: [PATCH 15/23] change to use Type.FindMembers and Array.Sort --- src/Build/Evaluation/Expander.cs | 55 ++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 549747e5989..15d8bfb6150 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5264,14 +5264,6 @@ private static bool IsInstanceMethodAvailable(string methodName) return methodName != "GetType"; } - private static TypeCode SelectTypeOfFirstParameter(MethodBase method) - { - ParameterInfo[] parameters = method.GetParameters(); - return parameters.Length > 0 - ? Type.GetTypeCode(parameters[0].ParameterType) - : TypeCode.Empty; - } - /// /// Construct and instance of objectType based on the constructor or method arguments provided. /// Arguments must never be null. @@ -5305,14 +5297,19 @@ private object LateBindExecute(Exception ex, BindingFlags bindingFlags, object o { members = _receiverType.GetConstructors(bindingFlags); } + else if (_receiverType == typeof(IntrinsicFunctions) && IntrinsicFunctionOverload.IsKnownOverloadMethodName(_methodMethodName)) + { + MemberInfo[] foundMembers = _receiverType.FindMembers( + MemberTypes.Method, + bindingFlags, + (info, criteria) => string.Equals(info.Name, (string)criteria, StringComparison.OrdinalIgnoreCase), + _methodMethodName); + Array.Sort(foundMembers, IntrinsicFunctionOverload.IntrinsicFunctionOverloadMethodComparer); + members = foundMembers.Cast(); + } else { members = _receiverType.GetMethods(bindingFlags).Where(m => string.Equals(m.Name, _methodMethodName, StringComparison.OrdinalIgnoreCase)); - - if (_receiverType == typeof(IntrinsicFunctions)) - { - members = members.OrderBy(SelectTypeOfFirstParameter, IntrinsicFunctionOverload.IntrinsicFunctionOverloadComparer); - } } foreach (MethodBase member in members) @@ -5412,20 +5409,38 @@ internal string CurrentlyEvaluatingPropertyElementName internal static class IntrinsicFunctionOverload { - private static IComparer s_intrinsicFunctionOverloadComparer; + private static readonly string[] s_knownOverloadName = { "Add", "Subtract", "Multiply", "Divide", "Modulo", }; // Order by the TypeCode of the first parameter. // When change wave is enabled, order long before double. // Otherwise preserve prior behavior of double before long. // For reuse, the comparer is cached in a non-generic type. - internal static IComparer IntrinsicFunctionOverloadComparer => - s_intrinsicFunctionOverloadComparer ??= IsIntrinsicFunctionOverloadsEnabled() - ? Comparer.Create((key0, key1) => key0.CompareTo(key1)) - : Comparer.Create((key0, key1) => key1.CompareTo(key0)); + // Both comparer instances can be cached to support change wave testing. + private static IComparer s_comparerLongBeforeDouble; + private static IComparer s_comparerDoubleBeforeLong; + + internal static IComparer IntrinsicFunctionOverloadMethodComparer => IsIntrinsicFunctionOverloadsEnabled() ? LongBeforeDoubleComparer : DoubleBeforeLongComparer; - /* When the change wave is retired, the expression body should be changed to - s_intrinsicFunctionOverloadComparer ??= Comparer.Create((key0, key1) => key0.CompareTo(key1)); . */ + private static IComparer LongBeforeDoubleComparer => s_comparerLongBeforeDouble ??= Comparer.Create((key0, key1) => SelectTypeOfFirstParameter(key0).CompareTo(SelectTypeOfFirstParameter(key1))); + + private static IComparer DoubleBeforeLongComparer => s_comparerDoubleBeforeLong ??= Comparer.Create((key0, key1) => SelectTypeOfFirstParameter(key1).CompareTo(SelectTypeOfFirstParameter(key0))); internal static bool IsIntrinsicFunctionOverloadsEnabled() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); + + internal static bool IsKnownOverloadMethodName(string methodName) => s_knownOverloadName.Any(name => string.Equals(name, methodName, StringComparison.OrdinalIgnoreCase)); + + private static TypeCode SelectTypeOfFirstParameter(MemberInfo member) + { + MethodBase method = member as MethodBase; + if (method == null) + { + return TypeCode.Empty; + } + + ParameterInfo[] parameters = method.GetParameters(); + return parameters.Length > 0 + ? Type.GetTypeCode(parameters[0].ParameterType) + : TypeCode.Empty; + } } } From 824825bb77ed537ac3fc6766a1d8f7753fced30b Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Mon, 22 May 2023 22:52:38 -0400 Subject: [PATCH 16/23] add comment --- src/Build/Evaluation/Expander.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 15d8bfb6150..dc103bc5ef1 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5425,6 +5425,7 @@ internal static class IntrinsicFunctionOverload private static IComparer DoubleBeforeLongComparer => s_comparerDoubleBeforeLong ??= Comparer.Create((key0, key1) => SelectTypeOfFirstParameter(key1).CompareTo(SelectTypeOfFirstParameter(key0))); + // The arithmetic overload feature uses this method to test for the change wave. internal static bool IsIntrinsicFunctionOverloadsEnabled() => ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_8); internal static bool IsKnownOverloadMethodName(string methodName) => s_knownOverloadName.Any(name => string.Equals(name, methodName, StringComparison.OrdinalIgnoreCase)); From f64fa81d4e5c636fe333d3a3e116c8c3936a4a79 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Thu, 25 May 2023 19:26:25 -0400 Subject: [PATCH 17/23] update TryParse to use invariant culture --- src/Build/Evaluation/Expander.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index dc103bc5ef1..3cce3b4184e 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4541,7 +4541,7 @@ internal static bool TryConvertToInt(object value, out int arg) case int i: arg = i; return true; - case string s when int.TryParse(s, out arg): + case string s when int.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture.NumberFormat, out arg): return true; } @@ -4570,7 +4570,7 @@ internal static bool TryConvertToLong(object value, out long arg) case int i: arg = i; return true; - case string s when long.TryParse(s, out arg): + case string s when long.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture.NumberFormat, out arg): return true; } @@ -4591,7 +4591,7 @@ internal static bool TryConvertToDouble(object value, out double arg) case int i: arg = i; return true; - case string str when double.TryParse(str, out arg): + case string str when double.TryParse(str, NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out arg): return true; default: arg = 0; @@ -4728,7 +4728,7 @@ private static bool TryGetArgs(object[] args, out string arg0, out int arg1) private static bool IsFloatingPointRepresentation(object value) { - return value is double || (value is string str && double.TryParse(str, out double _)); + return value is double || (value is string str && double.TryParse(str, NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out double _)); } private static bool TryExecuteArithmeticOverload(object[] args, Func integerOperation, Func realOperation, out object resultValue) From 23396810b918727f230a54f6ec3199ea8d349f90 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Mon, 29 May 2023 12:24:23 -0400 Subject: [PATCH 18/23] change to accept thousands separator and add test for different locale --- .../Evaluation/ExpanderFunction_Tests.cs | 29 +++++++++++++++++++ src/Build/Evaluation/Expander.cs | 4 +-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs index f8089b7337c..43c261e5676 100644 --- a/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ExpanderFunction_Tests.cs @@ -1,7 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Globalization; using System.Runtime.InteropServices; +using System.Threading; + using Microsoft.Build.Evaluation; using Shouldly; @@ -246,5 +249,31 @@ public void TryConvertToDoubleGivenString() Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); actual.ShouldBe(10.0); } + + [Fact] + public void TryConvertToDoubleGivenStringAndLocale() + { + const string value = "1,2"; + + Thread currentThread = Thread.CurrentThread; + CultureInfo originalCulture = currentThread.CurrentCulture; + + try + { + // English South Africa locale uses ',' as decimal separator. + // The invariant culture should be used and "1,2" should be 12.0 not 1.2. + var cultureEnglishSouthAfrica = CultureInfo.CreateSpecificCulture("en-ZA"); + currentThread.CurrentCulture = cultureEnglishSouthAfrica; + Expander.Function.TryConvertToDouble(value, out double actual).ShouldBeTrue(); + actual.ShouldBe(12.0); + } + finally + { + // Restore CultureInfo. + currentThread.CurrentCulture = originalCulture; + CultureInfo.CurrentCulture = originalCulture; + CultureInfo.DefaultThreadCurrentCulture = originalCulture; + } + } } } diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 3cce3b4184e..d43a58009f4 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4591,7 +4591,7 @@ internal static bool TryConvertToDouble(object value, out double arg) case int i: arg = i; return true; - case string str when double.TryParse(str, NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out arg): + case string str when double.TryParse(str, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out arg): return true; default: arg = 0; @@ -4728,7 +4728,7 @@ private static bool TryGetArgs(object[] args, out string arg0, out int arg1) private static bool IsFloatingPointRepresentation(object value) { - return value is double || (value is string str && double.TryParse(str, NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out double _)); + return value is double || (value is string str && double.TryParse(str, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat, out double _)); } private static bool TryExecuteArithmeticOverload(object[] args, Func integerOperation, Func realOperation, out object resultValue) From 3d66331766b8ad4dd78821672b71e43c8327161a Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 7 Jun 2023 09:48:56 -0400 Subject: [PATCH 19/23] use InvariantCulture with double.TryParse --- .../BackEnd/Components/Scheduler/Scheduler.cs | 118 +++++++++--------- src/Shared/Tracing.cs | 7 +- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/src/Build/BackEnd/Components/Scheduler/Scheduler.cs b/src/Build/BackEnd/Components/Scheduler/Scheduler.cs index bedccfe03cd..56dcda004ed 100644 --- a/src/Build/BackEnd/Components/Scheduler/Scheduler.cs +++ b/src/Build/BackEnd/Components/Scheduler/Scheduler.cs @@ -318,7 +318,7 @@ public IEnumerable ReportRequestBlocked(int nodeId, BuildReque else if ((blocker.BlockingRequestId == blocker.BlockedRequestId) && blocker.BlockingRequestId != BuildRequest.InvalidGlobalRequestId) { ErrorUtilities.VerifyThrow(string.IsNullOrEmpty(blocker.BlockingTarget), "Blocking target should be null because this is not a request blocking on a target"); - // We are blocked waiting for a transfer of results. + // We are blocked waiting for a transfer of results. HandleRequestBlockedOnResultsTransfer(parentRequest, responses); } else if (blocker.BlockingRequestId != BuildRequest.InvalidGlobalRequestId) @@ -349,7 +349,7 @@ public IEnumerable ReportRequestBlocked(int nodeId, BuildReque responses.Add(ScheduleResponse.CreateCircularDependencyResponse(nodeId, parentRequest.BuildRequest, ex.Request)); } - // Now see if we can schedule requests somewhere since we + // Now see if we can schedule requests somewhere since we // a) have a new request; and // b) have a node which is now waiting and not doing anything. ScheduleUnassignedRequests(responses); @@ -368,7 +368,7 @@ public IEnumerable ReportResult(int nodeId, BuildResult result if (result.NodeRequestId == BuildRequest.ResultsTransferNodeRequestId) { - // We are transferring results. The node to which they should be sent has already been recorded by the + // We are transferring results. The node to which they should be sent has already been recorded by the // HandleRequestBlockedOnResultsTransfer method in the configuration. BuildRequestConfiguration config = _configCache[result.ConfigurationId]; ScheduleResponse response = ScheduleResponse.CreateReportResultResponse(config.ResultsNodeId, result); @@ -380,7 +380,7 @@ public IEnumerable ReportResult(int nodeId, BuildResult result SchedulableRequest request = _schedulingData.GetExecutingRequest(result.GlobalRequestId); request.Complete(result); - // Report results to our parent, or report submission complete as necessary. + // Report results to our parent, or report submission complete as necessary. if (request.Parent != null) { // responses.Add(new ScheduleResponse(request.Parent.AssignedNode, new BuildRequestUnblocker(request.Parent.BuildRequest.GlobalRequestId, result))); @@ -388,10 +388,10 @@ public IEnumerable ReportResult(int nodeId, BuildResult result // When adding the result to the cache we merge the result with what ever is already in the cache this may cause // the result to have more target outputs in it than was was requested. To fix this we can ask the cache itself for the result we just added. - // When results are returned from the cache we filter them based on the targets we requested. This causes our result to only + // When results are returned from the cache we filter them based on the targets we requested. This causes our result to only // include the targets we requested rather than the merged result. - // Note: In this case we do not need to log that we got the results from the cache because we are only using the cache + // Note: In this case we do not need to log that we got the results from the cache because we are only using the cache // for filtering the targets for the result instead rather than using the cache as the location where this result came from. ScheduleResponse response = TrySatisfyRequestFromCache(request.Parent.AssignedNode, request.BuildRequest, skippedResultsDoNotCauseCacheMiss: _componentHost.BuildParameters.SkippedResultsDoNotCauseCacheMiss()); @@ -429,13 +429,13 @@ public IEnumerable ReportResult(int nodeId, BuildResult result int parentNode = (unscheduledRequest.Parent == null) ? InvalidNodeId : unscheduledRequest.Parent.AssignedNode; // There are other requests which we can satisfy based on this result, lets pull the result out of the cache - // and satisfy those requests. Normally a skipped result would lead to the cache refusing to satisfy the - // request, because the correct response in that case would be to attempt to rebuild the target in case there + // and satisfy those requests. Normally a skipped result would lead to the cache refusing to satisfy the + // request, because the correct response in that case would be to attempt to rebuild the target in case there // are state changes that would cause it to now excute. At this point, however, we already know that the parent - // request has completed, and we already know that this request has the same global request ID, which means that - // its configuration and set of targets are identical -- from MSBuild's perspective, it's the same. So since - // we're not going to attempt to re-execute it, if there are skipped targets in the result, that's fine. We just - // need to know what the target results are so that we can log them. + // request has completed, and we already know that this request has the same global request ID, which means that + // its configuration and set of targets are identical -- from MSBuild's perspective, it's the same. So since + // we're not going to attempt to re-execute it, if there are skipped targets in the result, that's fine. We just + // need to know what the target results are so that we can log them. ScheduleResponse response = TrySatisfyRequestFromCache(parentNode, unscheduledRequest.BuildRequest, skippedResultsDoNotCauseCacheMiss: true); // If we have a response we need to tell the loggers that we satisified that request from the cache. @@ -445,8 +445,8 @@ public IEnumerable ReportResult(int nodeId, BuildResult result } else { - // Response may be null if the result was never added to the cache. This can happen if the result has - // an exception in it. If that is the case, we should report the result directly so that the + // Response may be null if the result was never added to the cache. This can happen if the result has + // an exception in it. If that is the case, we should report the result directly so that the // build manager knows that it needs to shut down logging manually. response = GetResponseForResult(parentNode, unscheduledRequest.BuildRequest, newResult.Clone()); } @@ -507,7 +507,7 @@ public void ReportBuildAborted(int nodeId) { _schedulingData.EventTime = DateTime.UtcNow; - // Get the list of build requests currently assigned to the node and report aborted results for them. + // Get the list of build requests currently assigned to the node and report aborted results for them. TraceScheduler("Build aborted by node {0}", nodeId); foreach (SchedulableRequest request in _schedulingData.GetScheduledRequestsByNode(nodeId)) @@ -727,15 +727,15 @@ private void ScheduleUnassignedRequests(List responses) } else if (_schedulingData.BlockedRequestsCount != 0) { - // It is legitimate to have blocked requests with none executing if none of the requests can - // be serviced by any currently existing node, or if they are blocked by requests, none of - // which can be serviced by any currently existing node. However, in that case, we had better - // be requesting the creation of a node that can service them. + // It is legitimate to have blocked requests with none executing if none of the requests can + // be serviced by any currently existing node, or if they are blocked by requests, none of + // which can be serviced by any currently existing node. However, in that case, we had better + // be requesting the creation of a node that can service them. // - // Note: This is O(# nodes * closure of requests blocking current set of blocked requests), - // but all three numbers should usually be fairly small and, more importantly, this situation - // should occur at most once per build, since it requires a situation where all blocked requests - // are blocked on the creation of a node that can service them. + // Note: This is O(# nodes * closure of requests blocking current set of blocked requests), + // but all three numbers should usually be fairly small and, more importantly, this situation + // should occur at most once per build, since it requires a situation where all blocked requests + // are blocked on the creation of a node that can service them. foreach (SchedulableRequest request in _schedulingData.BlockedRequests) { if (RequestOrAnyItIsBlockedByCanBeServiced(request)) @@ -843,7 +843,7 @@ private bool GetSchedulingPlanAndAlgorithm() if (!String.IsNullOrEmpty(customScheduler)) { - // Assign to the delegate + // Assign to the delegate if (customScheduler.Equals("WithPlanByMostImmediateReferences", StringComparison.OrdinalIgnoreCase) && _schedulingPlan.IsPlanValid) { _customRequestSchedulingAlgorithm = AssignUnscheduledRequestsWithPlanByMostImmediateReferences; @@ -886,7 +886,7 @@ private bool GetSchedulingPlanAndAlgorithm() string multiplier = Environment.GetEnvironmentVariable("MSBUILDCUSTOMSCHEDULERFORSQLCONFIGURATIONLIMITMULTIPLIER"); double convertedMultiplier = 0; - if (!Double.TryParse(multiplier, out convertedMultiplier) || convertedMultiplier < 1) + if (!Double.TryParse(multiplier, NumberStyles.AllowDecimalPoint | NumberStyles.AllowLeadingSign, CultureInfo.InvariantCulture.NumberFormat, out convertedMultiplier) || convertedMultiplier < 1) { _customSchedulerForSQLConfigurationLimitMultiplier = DefaultCustomSchedulerForSQLConfigurationLimitMultiplier; } @@ -1142,7 +1142,7 @@ private void AssignUnscheduledRequestsWithMaxWaitingRequests(List configurationCountsByNode = new Dictionary(_availableNodes.Count); - // The configuration count limit will be the average configuration count * X (to allow for some wiggle room) where + // The configuration count limit will be the average configuration count * X (to allow for some wiggle room) where // the default value of X is 1.1 (+ 10%) int configurationCountLimit = 0; @@ -1315,8 +1315,8 @@ private void AssignUnscheduledRequestsUsingCustomSchedulerForSQL(List @@ -1409,7 +1409,7 @@ private bool AtSchedulingLimit() return false; } - // We're at our limit of schedulable requests if: + // We're at our limit of schedulable requests if: // (1) MaxNodeCount requests are currently executing if (_schedulingData.ExecutingRequestsCount >= _componentHost.BuildParameters.MaxNodeCount) { @@ -1471,9 +1471,9 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab { int assignedNodeForConfiguration = _schedulingData.GetAssignedNodeForRequestConfiguration(request.BuildRequest.ConfigurationId); - // Although this request has not been scheduled, this configuration may previously have been - // scheduled to an existing node. If so, we shouldn't count it in our checks for new node - // creation, because it'll only eventually get assigned to its existing node anyway. + // Although this request has not been scheduled, this configuration may previously have been + // scheduled to an existing node. If so, we shouldn't count it in our checks for new node + // creation, because it'll only eventually get assigned to its existing node anyway. if (assignedNodeForConfiguration != Scheduler.InvalidNodeId) { continue; @@ -1486,9 +1486,9 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab case NodeAffinity.InProc: inProcNodesToCreate++; - // If we've previously seen "Any"-affinitized requests, now that there are some - // genuine inproc requests, they get to play with the inproc node first, so - // push the "Any" requests to the out-of-proc nodes. + // If we've previously seen "Any"-affinitized requests, now that there are some + // genuine inproc requests, they get to play with the inproc node first, so + // push the "Any" requests to the out-of-proc nodes. if (requestsWithAnyAffinityOnInProcNodes > 0) { requestsWithAnyAffinityOnInProcNodes--; @@ -1525,7 +1525,7 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab break; } - // We've already hit the limit of the number of nodes we'll be allowed to create, so just quit counting now. + // We've already hit the limit of the number of nodes we'll be allowed to create, so just quit counting now. if (inProcNodesToCreate >= availableNodesWithInProcAffinity && outOfProcNodesToCreate >= availableNodesWithOutOfProcAffinity) { break; @@ -1535,7 +1535,7 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab // If we think we want to create inproc nodes if (inProcNodesToCreate > 0) { - // In-proc node determination is simple: we want as many as are available. + // In-proc node determination is simple: we want as many as are available. inProcNodesToCreate = Math.Min(availableNodesWithInProcAffinity, inProcNodesToCreate); // If we still want to create one, go ahead @@ -1545,8 +1545,8 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab TraceScheduler("Requesting creation of new node satisfying affinity {0}", NodeAffinity.InProc); responses.Add(ScheduleResponse.CreateNewNodeResponse(NodeAffinity.InProc, 1)); - // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler - // to do more scheduling, so the other request will be dealt with soon enough. + // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler + // to do more scheduling, so the other request will be dealt with soon enough. return true; } } @@ -1554,17 +1554,17 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab // If we think we want to create out-of-proc nodes if (outOfProcNodesToCreate > 0) { - // Out-of-proc node determination is a bit more complicated. If we have N out-of-proc requests, we want to - // fill up to N out-of-proc nodes. However, if we have N "any" requests, we must assume that at least some of them - // will be fulfilled by the inproc node, in which case we only want to launch up to N-1 out-of-proc nodes, for a - // total of N nodes overall -- the scheduler will only schedule to N nodes at a time, so launching any more than that - // is ultimately pointless. + // Out-of-proc node determination is a bit more complicated. If we have N out-of-proc requests, we want to + // fill up to N out-of-proc nodes. However, if we have N "any" requests, we must assume that at least some of them + // will be fulfilled by the inproc node, in which case we only want to launch up to N-1 out-of-proc nodes, for a + // total of N nodes overall -- the scheduler will only schedule to N nodes at a time, so launching any more than that + // is ultimately pointless. int maxCreatableOutOfProcNodes = availableNodesWithOutOfProcAffinity; if (requestsWithOutOfProcAffinity < availableNodesWithOutOfProcAffinity) { - // We don't have enough explicitly out-of-proc requests to justify creating every technically allowed - // out-of-proc node, so our max is actually one less than the absolute max for the reasons explained above. + // We don't have enough explicitly out-of-proc requests to justify creating every technically allowed + // out-of-proc node, so our max is actually one less than the absolute max for the reasons explained above. maxCreatableOutOfProcNodes--; } @@ -1577,12 +1577,12 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab responses.Add(ScheduleResponse.CreateNewNodeResponse(NodeAffinity.OutOfProc, outOfProcNodesToCreate)); } - // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler - // to do more scheduling, so the other request will be dealt with soon enough. + // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler + // to do more scheduling, so the other request will be dealt with soon enough. return true; } - // If we haven't returned before now, we haven't asked that any new nodes be created. + // If we haven't returned before now, we haven't asked that any new nodes be created. return false; } @@ -1647,14 +1647,14 @@ private void HandleRequestBlockedOnResultsTransfer(SchedulableRequest parentRequ // we will update the storage location in the configuration. This is doing a bit of a run around the scheduler - we don't // create a new formal request, so we treat the blocked request as if it is still executing - this prevents any other requests // from getting onto that node and also means we don't have to do additional work to get the scheduler to understand the bizarre - // case of sending a request for results from a project's own configuration (which it believes reside on the very node which + // case of sending a request for results from a project's own configuration (which it believes reside on the very node which // is actually requesting the results in the first place.) BuildRequestConfiguration configuration = _configCache[parentRequest.BuildRequest.ConfigurationId]; responses.Add(ScheduleResponse.CreateScheduleResponse(configuration.ResultsNodeId, newRequest, false)); TraceScheduler("Created request {0} (node request {1}) for transfer of configuration {2}'s results from node {3} to node {4}", newRequest.GlobalRequestId, newRequest.NodeRequestId, configuration.ConfigurationId, configuration.ResultsNodeId, parentRequest.AssignedNode); - // The configuration's results will now be homed at the new location (once they have come back from the + // The configuration's results will now be homed at the new location (once they have come back from the // original node.) configuration.ResultsNodeId = parentRequest.AssignedNode; } @@ -1856,7 +1856,7 @@ private void ResolveRequestFromCacheAndResumeIfPossible(SchedulableRequest reque responses.Add(response); } - // Is the node we are reporting to idle? If so, does reporting this result allow it to proceed with work? + // Is the node we are reporting to idle? If so, does reporting this result allow it to proceed with work? if (!_schedulingData.IsNodeWorking(response.NodeId)) { ResumeReadyRequestIfAny(response.NodeId, responses); @@ -2090,7 +2090,7 @@ internal void RecordResultToCurrentCacheIfConfigNotInOverrideCache(BuildResult r /// private ScheduleResponse GetResponseForResult(int parentRequestNode, BuildRequest requestWhichGeneratedResult, BuildResult result) { - // We have results, return them to the originating node, or if it is a root request, mark the submission complete. + // We have results, return them to the originating node, or if it is a root request, mark the submission complete. if (requestWhichGeneratedResult.IsRootRequest) { // return new ScheduleResponse(result); @@ -2209,9 +2209,9 @@ private bool RequestOrAnyItIsBlockedByCanBeServiced(SchedulableRequest request) } } - // if none of the requests we are blocked by can be serviced, it doesn't matter - // whether we can be serviced or not -- the reason we're blocked is because none - // of the requests we are blocked by can be serviced. + // if none of the requests we are blocked by can be serviced, it doesn't matter + // whether we can be serviced or not -- the reason we're blocked is because none + // of the requests we are blocked by can be serviced. return false; } else diff --git a/src/Shared/Tracing.cs b/src/Shared/Tracing.cs index 71b52e4d565..51013cb06f2 100644 --- a/src/Shared/Tracing.cs +++ b/src/Shared/Tracing.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Globalization; #if DEBUG using System.Reflection; #endif @@ -18,7 +19,7 @@ namespace Microsoft.Build.Internal /// internal static class Tracing { - // Disabling warning about unused fields -- this is effectively a + // Disabling warning about unused fields -- this is effectively a // debug-only class, so these fields cause a build break in RET #pragma warning disable 649 /// @@ -43,7 +44,7 @@ internal static class Tracing /// /// Short name of the current assembly - to distinguish statics when this type is shared into different assemblies - /// + /// private static string s_currentAssemblyName; #pragma warning restore 649 @@ -58,7 +59,7 @@ static Tracing() string val = Environment.GetEnvironmentVariable("MSBUILDTRACEINTERVAL"); double seconds; - if (!String.IsNullOrEmpty(val) && System.Double.TryParse(val, out seconds)) + if (!String.IsNullOrEmpty(val) && System.Double.TryParse(val, NumberStyles.AllowDecimalPoint | NumberStyles.AllowLeadingSign, CultureInfo.InvariantCulture.NumberFormat, out seconds)) { s_interval = TimeSpan.FromSeconds(seconds); } From 22f8684303abadef7f69b21f9a3c12325a0356b9 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 7 Jun 2023 11:11:37 -0400 Subject: [PATCH 20/23] add comments --- src/Build/Evaluation/Expander.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index d43a58009f4..c40482fe4bd 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -4515,6 +4515,9 @@ private static bool TryConvertToVersion(object value, out Version arg0) return true; } + /// + /// Try to convert value to int. + /// internal static bool TryConvertToInt(object value, out int arg) { switch (value) @@ -4549,6 +4552,9 @@ internal static bool TryConvertToInt(object value, out int arg) return false; } + /// + /// Try to convert value to long. + /// internal static bool TryConvertToLong(object value, out long arg) { switch (value) @@ -4578,6 +4584,9 @@ internal static bool TryConvertToLong(object value, out long arg) return false; } + /// + /// Try to convert value to double. + /// internal static bool TryConvertToDouble(object value, out double arg) { switch (value) From e1c7115e08de1c1c77dbc59030f37ce86473a537 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 7 Jun 2023 15:30:55 -0400 Subject: [PATCH 21/23] fix IDE0005 error that is not reported in local builds --- src/Shared/Tracing.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Shared/Tracing.cs b/src/Shared/Tracing.cs index 51013cb06f2..f1549cae456 100644 --- a/src/Shared/Tracing.cs +++ b/src/Shared/Tracing.cs @@ -5,8 +5,8 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Globalization; #if DEBUG +using System.Globalization; using System.Reflection; #endif From 4f21730ae383e052851a69b2816423b6d68befce Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Wed, 7 Jun 2023 21:11:15 -0400 Subject: [PATCH 22/23] revert unintended whitespace formatting --- .../BackEnd/Components/Scheduler/Scheduler.cs | 116 +++++++++--------- src/Shared/Tracing.cs | 4 +- 2 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/Build/BackEnd/Components/Scheduler/Scheduler.cs b/src/Build/BackEnd/Components/Scheduler/Scheduler.cs index 56dcda004ed..53bf46ec2f3 100644 --- a/src/Build/BackEnd/Components/Scheduler/Scheduler.cs +++ b/src/Build/BackEnd/Components/Scheduler/Scheduler.cs @@ -318,7 +318,7 @@ public IEnumerable ReportRequestBlocked(int nodeId, BuildReque else if ((blocker.BlockingRequestId == blocker.BlockedRequestId) && blocker.BlockingRequestId != BuildRequest.InvalidGlobalRequestId) { ErrorUtilities.VerifyThrow(string.IsNullOrEmpty(blocker.BlockingTarget), "Blocking target should be null because this is not a request blocking on a target"); - // We are blocked waiting for a transfer of results. + // We are blocked waiting for a transfer of results. HandleRequestBlockedOnResultsTransfer(parentRequest, responses); } else if (blocker.BlockingRequestId != BuildRequest.InvalidGlobalRequestId) @@ -349,7 +349,7 @@ public IEnumerable ReportRequestBlocked(int nodeId, BuildReque responses.Add(ScheduleResponse.CreateCircularDependencyResponse(nodeId, parentRequest.BuildRequest, ex.Request)); } - // Now see if we can schedule requests somewhere since we + // Now see if we can schedule requests somewhere since we // a) have a new request; and // b) have a node which is now waiting and not doing anything. ScheduleUnassignedRequests(responses); @@ -368,7 +368,7 @@ public IEnumerable ReportResult(int nodeId, BuildResult result if (result.NodeRequestId == BuildRequest.ResultsTransferNodeRequestId) { - // We are transferring results. The node to which they should be sent has already been recorded by the + // We are transferring results. The node to which they should be sent has already been recorded by the // HandleRequestBlockedOnResultsTransfer method in the configuration. BuildRequestConfiguration config = _configCache[result.ConfigurationId]; ScheduleResponse response = ScheduleResponse.CreateReportResultResponse(config.ResultsNodeId, result); @@ -380,7 +380,7 @@ public IEnumerable ReportResult(int nodeId, BuildResult result SchedulableRequest request = _schedulingData.GetExecutingRequest(result.GlobalRequestId); request.Complete(result); - // Report results to our parent, or report submission complete as necessary. + // Report results to our parent, or report submission complete as necessary. if (request.Parent != null) { // responses.Add(new ScheduleResponse(request.Parent.AssignedNode, new BuildRequestUnblocker(request.Parent.BuildRequest.GlobalRequestId, result))); @@ -388,10 +388,10 @@ public IEnumerable ReportResult(int nodeId, BuildResult result // When adding the result to the cache we merge the result with what ever is already in the cache this may cause // the result to have more target outputs in it than was was requested. To fix this we can ask the cache itself for the result we just added. - // When results are returned from the cache we filter them based on the targets we requested. This causes our result to only + // When results are returned from the cache we filter them based on the targets we requested. This causes our result to only // include the targets we requested rather than the merged result. - // Note: In this case we do not need to log that we got the results from the cache because we are only using the cache + // Note: In this case we do not need to log that we got the results from the cache because we are only using the cache // for filtering the targets for the result instead rather than using the cache as the location where this result came from. ScheduleResponse response = TrySatisfyRequestFromCache(request.Parent.AssignedNode, request.BuildRequest, skippedResultsDoNotCauseCacheMiss: _componentHost.BuildParameters.SkippedResultsDoNotCauseCacheMiss()); @@ -429,13 +429,13 @@ public IEnumerable ReportResult(int nodeId, BuildResult result int parentNode = (unscheduledRequest.Parent == null) ? InvalidNodeId : unscheduledRequest.Parent.AssignedNode; // There are other requests which we can satisfy based on this result, lets pull the result out of the cache - // and satisfy those requests. Normally a skipped result would lead to the cache refusing to satisfy the - // request, because the correct response in that case would be to attempt to rebuild the target in case there + // and satisfy those requests. Normally a skipped result would lead to the cache refusing to satisfy the + // request, because the correct response in that case would be to attempt to rebuild the target in case there // are state changes that would cause it to now excute. At this point, however, we already know that the parent - // request has completed, and we already know that this request has the same global request ID, which means that - // its configuration and set of targets are identical -- from MSBuild's perspective, it's the same. So since - // we're not going to attempt to re-execute it, if there are skipped targets in the result, that's fine. We just - // need to know what the target results are so that we can log them. + // request has completed, and we already know that this request has the same global request ID, which means that + // its configuration and set of targets are identical -- from MSBuild's perspective, it's the same. So since + // we're not going to attempt to re-execute it, if there are skipped targets in the result, that's fine. We just + // need to know what the target results are so that we can log them. ScheduleResponse response = TrySatisfyRequestFromCache(parentNode, unscheduledRequest.BuildRequest, skippedResultsDoNotCauseCacheMiss: true); // If we have a response we need to tell the loggers that we satisified that request from the cache. @@ -445,8 +445,8 @@ public IEnumerable ReportResult(int nodeId, BuildResult result } else { - // Response may be null if the result was never added to the cache. This can happen if the result has - // an exception in it. If that is the case, we should report the result directly so that the + // Response may be null if the result was never added to the cache. This can happen if the result has + // an exception in it. If that is the case, we should report the result directly so that the // build manager knows that it needs to shut down logging manually. response = GetResponseForResult(parentNode, unscheduledRequest.BuildRequest, newResult.Clone()); } @@ -507,7 +507,7 @@ public void ReportBuildAborted(int nodeId) { _schedulingData.EventTime = DateTime.UtcNow; - // Get the list of build requests currently assigned to the node and report aborted results for them. + // Get the list of build requests currently assigned to the node and report aborted results for them. TraceScheduler("Build aborted by node {0}", nodeId); foreach (SchedulableRequest request in _schedulingData.GetScheduledRequestsByNode(nodeId)) @@ -727,15 +727,15 @@ private void ScheduleUnassignedRequests(List responses) } else if (_schedulingData.BlockedRequestsCount != 0) { - // It is legitimate to have blocked requests with none executing if none of the requests can - // be serviced by any currently existing node, or if they are blocked by requests, none of - // which can be serviced by any currently existing node. However, in that case, we had better - // be requesting the creation of a node that can service them. + // It is legitimate to have blocked requests with none executing if none of the requests can + // be serviced by any currently existing node, or if they are blocked by requests, none of + // which can be serviced by any currently existing node. However, in that case, we had better + // be requesting the creation of a node that can service them. // - // Note: This is O(# nodes * closure of requests blocking current set of blocked requests), - // but all three numbers should usually be fairly small and, more importantly, this situation - // should occur at most once per build, since it requires a situation where all blocked requests - // are blocked on the creation of a node that can service them. + // Note: This is O(# nodes * closure of requests blocking current set of blocked requests), + // but all three numbers should usually be fairly small and, more importantly, this situation + // should occur at most once per build, since it requires a situation where all blocked requests + // are blocked on the creation of a node that can service them. foreach (SchedulableRequest request in _schedulingData.BlockedRequests) { if (RequestOrAnyItIsBlockedByCanBeServiced(request)) @@ -843,7 +843,7 @@ private bool GetSchedulingPlanAndAlgorithm() if (!String.IsNullOrEmpty(customScheduler)) { - // Assign to the delegate + // Assign to the delegate if (customScheduler.Equals("WithPlanByMostImmediateReferences", StringComparison.OrdinalIgnoreCase) && _schedulingPlan.IsPlanValid) { _customRequestSchedulingAlgorithm = AssignUnscheduledRequestsWithPlanByMostImmediateReferences; @@ -1142,7 +1142,7 @@ private void AssignUnscheduledRequestsWithMaxWaitingRequests(List configurationCountsByNode = new Dictionary(_availableNodes.Count); - // The configuration count limit will be the average configuration count * X (to allow for some wiggle room) where + // The configuration count limit will be the average configuration count * X (to allow for some wiggle room) where // the default value of X is 1.1 (+ 10%) int configurationCountLimit = 0; @@ -1315,8 +1315,8 @@ private void AssignUnscheduledRequestsUsingCustomSchedulerForSQL(List @@ -1409,7 +1409,7 @@ private bool AtSchedulingLimit() return false; } - // We're at our limit of schedulable requests if: + // We're at our limit of schedulable requests if: // (1) MaxNodeCount requests are currently executing if (_schedulingData.ExecutingRequestsCount >= _componentHost.BuildParameters.MaxNodeCount) { @@ -1471,9 +1471,9 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab { int assignedNodeForConfiguration = _schedulingData.GetAssignedNodeForRequestConfiguration(request.BuildRequest.ConfigurationId); - // Although this request has not been scheduled, this configuration may previously have been - // scheduled to an existing node. If so, we shouldn't count it in our checks for new node - // creation, because it'll only eventually get assigned to its existing node anyway. + // Although this request has not been scheduled, this configuration may previously have been + // scheduled to an existing node. If so, we shouldn't count it in our checks for new node + // creation, because it'll only eventually get assigned to its existing node anyway. if (assignedNodeForConfiguration != Scheduler.InvalidNodeId) { continue; @@ -1486,9 +1486,9 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab case NodeAffinity.InProc: inProcNodesToCreate++; - // If we've previously seen "Any"-affinitized requests, now that there are some - // genuine inproc requests, they get to play with the inproc node first, so - // push the "Any" requests to the out-of-proc nodes. + // If we've previously seen "Any"-affinitized requests, now that there are some + // genuine inproc requests, they get to play with the inproc node first, so + // push the "Any" requests to the out-of-proc nodes. if (requestsWithAnyAffinityOnInProcNodes > 0) { requestsWithAnyAffinityOnInProcNodes--; @@ -1525,7 +1525,7 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab break; } - // We've already hit the limit of the number of nodes we'll be allowed to create, so just quit counting now. + // We've already hit the limit of the number of nodes we'll be allowed to create, so just quit counting now. if (inProcNodesToCreate >= availableNodesWithInProcAffinity && outOfProcNodesToCreate >= availableNodesWithOutOfProcAffinity) { break; @@ -1535,7 +1535,7 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab // If we think we want to create inproc nodes if (inProcNodesToCreate > 0) { - // In-proc node determination is simple: we want as many as are available. + // In-proc node determination is simple: we want as many as are available. inProcNodesToCreate = Math.Min(availableNodesWithInProcAffinity, inProcNodesToCreate); // If we still want to create one, go ahead @@ -1545,8 +1545,8 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab TraceScheduler("Requesting creation of new node satisfying affinity {0}", NodeAffinity.InProc); responses.Add(ScheduleResponse.CreateNewNodeResponse(NodeAffinity.InProc, 1)); - // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler - // to do more scheduling, so the other request will be dealt with soon enough. + // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler + // to do more scheduling, so the other request will be dealt with soon enough. return true; } } @@ -1554,17 +1554,17 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab // If we think we want to create out-of-proc nodes if (outOfProcNodesToCreate > 0) { - // Out-of-proc node determination is a bit more complicated. If we have N out-of-proc requests, we want to - // fill up to N out-of-proc nodes. However, if we have N "any" requests, we must assume that at least some of them - // will be fulfilled by the inproc node, in which case we only want to launch up to N-1 out-of-proc nodes, for a - // total of N nodes overall -- the scheduler will only schedule to N nodes at a time, so launching any more than that - // is ultimately pointless. + // Out-of-proc node determination is a bit more complicated. If we have N out-of-proc requests, we want to + // fill up to N out-of-proc nodes. However, if we have N "any" requests, we must assume that at least some of them + // will be fulfilled by the inproc node, in which case we only want to launch up to N-1 out-of-proc nodes, for a + // total of N nodes overall -- the scheduler will only schedule to N nodes at a time, so launching any more than that + // is ultimately pointless. int maxCreatableOutOfProcNodes = availableNodesWithOutOfProcAffinity; if (requestsWithOutOfProcAffinity < availableNodesWithOutOfProcAffinity) { - // We don't have enough explicitly out-of-proc requests to justify creating every technically allowed - // out-of-proc node, so our max is actually one less than the absolute max for the reasons explained above. + // We don't have enough explicitly out-of-proc requests to justify creating every technically allowed + // out-of-proc node, so our max is actually one less than the absolute max for the reasons explained above. maxCreatableOutOfProcNodes--; } @@ -1577,12 +1577,12 @@ private bool CreateNewNodeIfPossible(List responses, IEnumerab responses.Add(ScheduleResponse.CreateNewNodeResponse(NodeAffinity.OutOfProc, outOfProcNodesToCreate)); } - // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler - // to do more scheduling, so the other request will be dealt with soon enough. + // We only want to submit one node creation request at a time -- as part of node creation we recursively re-request the scheduler + // to do more scheduling, so the other request will be dealt with soon enough. return true; } - // If we haven't returned before now, we haven't asked that any new nodes be created. + // If we haven't returned before now, we haven't asked that any new nodes be created. return false; } @@ -1647,14 +1647,14 @@ private void HandleRequestBlockedOnResultsTransfer(SchedulableRequest parentRequ // we will update the storage location in the configuration. This is doing a bit of a run around the scheduler - we don't // create a new formal request, so we treat the blocked request as if it is still executing - this prevents any other requests // from getting onto that node and also means we don't have to do additional work to get the scheduler to understand the bizarre - // case of sending a request for results from a project's own configuration (which it believes reside on the very node which + // case of sending a request for results from a project's own configuration (which it believes reside on the very node which // is actually requesting the results in the first place.) BuildRequestConfiguration configuration = _configCache[parentRequest.BuildRequest.ConfigurationId]; responses.Add(ScheduleResponse.CreateScheduleResponse(configuration.ResultsNodeId, newRequest, false)); TraceScheduler("Created request {0} (node request {1}) for transfer of configuration {2}'s results from node {3} to node {4}", newRequest.GlobalRequestId, newRequest.NodeRequestId, configuration.ConfigurationId, configuration.ResultsNodeId, parentRequest.AssignedNode); - // The configuration's results will now be homed at the new location (once they have come back from the + // The configuration's results will now be homed at the new location (once they have come back from the // original node.) configuration.ResultsNodeId = parentRequest.AssignedNode; } @@ -1856,7 +1856,7 @@ private void ResolveRequestFromCacheAndResumeIfPossible(SchedulableRequest reque responses.Add(response); } - // Is the node we are reporting to idle? If so, does reporting this result allow it to proceed with work? + // Is the node we are reporting to idle? If so, does reporting this result allow it to proceed with work? if (!_schedulingData.IsNodeWorking(response.NodeId)) { ResumeReadyRequestIfAny(response.NodeId, responses); @@ -2090,7 +2090,7 @@ internal void RecordResultToCurrentCacheIfConfigNotInOverrideCache(BuildResult r /// private ScheduleResponse GetResponseForResult(int parentRequestNode, BuildRequest requestWhichGeneratedResult, BuildResult result) { - // We have results, return them to the originating node, or if it is a root request, mark the submission complete. + // We have results, return them to the originating node, or if it is a root request, mark the submission complete. if (requestWhichGeneratedResult.IsRootRequest) { // return new ScheduleResponse(result); @@ -2209,9 +2209,9 @@ private bool RequestOrAnyItIsBlockedByCanBeServiced(SchedulableRequest request) } } - // if none of the requests we are blocked by can be serviced, it doesn't matter - // whether we can be serviced or not -- the reason we're blocked is because none - // of the requests we are blocked by can be serviced. + // if none of the requests we are blocked by can be serviced, it doesn't matter + // whether we can be serviced or not -- the reason we're blocked is because none + // of the requests we are blocked by can be serviced. return false; } else diff --git a/src/Shared/Tracing.cs b/src/Shared/Tracing.cs index f1549cae456..d26f7127305 100644 --- a/src/Shared/Tracing.cs +++ b/src/Shared/Tracing.cs @@ -19,7 +19,7 @@ namespace Microsoft.Build.Internal /// internal static class Tracing { - // Disabling warning about unused fields -- this is effectively a + // Disabling warning about unused fields -- this is effectively a // debug-only class, so these fields cause a build break in RET #pragma warning disable 649 /// @@ -44,7 +44,7 @@ internal static class Tracing /// /// Short name of the current assembly - to distinguish statics when this type is shared into different assemblies - /// + /// private static string s_currentAssemblyName; #pragma warning restore 649 From 19b50397c0a1f95dd524d3c102bb72ab860ee9c9 Mon Sep 17 00:00:00 2001 From: Jonathan Dodds Date: Tue, 27 Jun 2023 20:25:43 -0400 Subject: [PATCH 23/23] fix nullable errors after merge --- src/Build/Evaluation/Expander.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index df493de45c1..075090e0f9a 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -5463,8 +5463,8 @@ internal static class IntrinsicFunctionOverload // Otherwise preserve prior behavior of double before long. // For reuse, the comparer is cached in a non-generic type. // Both comparer instances can be cached to support change wave testing. - private static IComparer s_comparerLongBeforeDouble; - private static IComparer s_comparerDoubleBeforeLong; + private static IComparer? s_comparerLongBeforeDouble; + private static IComparer? s_comparerDoubleBeforeLong; internal static IComparer IntrinsicFunctionOverloadMethodComparer => IsIntrinsicFunctionOverloadsEnabled() ? LongBeforeDoubleComparer : DoubleBeforeLongComparer; @@ -5479,7 +5479,7 @@ internal static class IntrinsicFunctionOverload private static TypeCode SelectTypeOfFirstParameter(MemberInfo member) { - MethodBase method = member as MethodBase; + MethodBase? method = member as MethodBase; if (method == null) { return TypeCode.Empty;