From a78ebed3da195f700cc8e2552e6ff31060121e9d Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Thu, 14 Mar 2024 13:41:39 +0100 Subject: [PATCH 1/8] Use invariant culture for the number parsing --- documentation/wiki/ChangeWaves.md | 1 + .../Evaluation/Expander_Tests.cs | 78 +++++++++++++++++++ src/Build/Evaluation/Expander.cs | 15 +++- 3 files changed, 93 insertions(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index b7508160ef6..036a6bf4f7c 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -34,6 +34,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Load NuGet.Frameworks into secondary AppDomain (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9446) - [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655) - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722) +- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types](https://github.com/dotnet/msbuild/pull/) ### 17.8 diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index d7744692f25..6ae516d1d8c 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -9,6 +9,7 @@ using System.Runtime.InteropServices; using System.Runtime.Versioning; using System.Text; +using System.Threading; using System.Xml; using Microsoft.Build.BackEnd; using Microsoft.Build.Collections; @@ -4859,5 +4860,82 @@ public void ExpandItemVectorFunctions_Exists_Directories() squiggleItems.Select(i => i.EvaluatedInclude).ShouldBe(new[] { alphaBetaPath, alphaDeltaPath }, Case.Insensitive); } } + + [Fact] + public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData() + { + var currentThread = Thread.CurrentThread; + var originalCulture = currentThread.CurrentCulture; + var originalUICulture = currentThread.CurrentUICulture; + + try + { + var svSECultureInfo = new CultureInfo("sv-SE"); + using (var env = TestEnvironment.Create()) + { + CultureInfo.CurrentCulture = svSECultureInfo; + CultureInfo.CurrentUICulture = svSECultureInfo; + var root = env.CreateFolder(); + + var projectFile = env.CreateFile(root, ".proj", + @" + + + <_value>$([MSBuild]::Subtract(0, 1)) + <_otherValue Condition=""'$(_value)' >= -1"">test-value + + +"); + ProjectInstance projectInstance = new ProjectInstance(projectFile.Path); + projectInstance.GetPropertyValue("_value").ShouldBe("-1"); + projectInstance.GetPropertyValue("_otherValue").ShouldBe("test-value"); + } + } + finally + { + currentThread.CurrentCulture = originalCulture; + currentThread.CurrentUICulture = originalUICulture; + } + } + + [Fact] + public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData_RespectingChangeWave() + { + var currentThread = Thread.CurrentThread; + var originalCulture = currentThread.CurrentCulture; + var originalUICulture = currentThread.CurrentUICulture; + + try + { + var svSECultureInfo = new CultureInfo("sv-SE"); + using (var env = TestEnvironment.Create()) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_10.ToString()); + CultureInfo.CurrentCulture = svSECultureInfo; + CultureInfo.CurrentUICulture = svSECultureInfo; + var root = env.CreateFolder(); + + var projectFile = env.CreateFile(root, ".proj", + @" + + + <_value>$([MSBuild]::Subtract(0, 1)) + <_otherValue Condition=""'$(_value)' >= -1"">test-value + + +"); + var exception = Should.Throw(() => + { + new ProjectInstance(projectFile.Path); + }); + exception.BaseMessage.ShouldContain("A numeric comparison was attempted on \"$(_value)\""); + } + } + finally + { + currentThread.CurrentCulture = originalCulture; + currentThread.CurrentUICulture = originalUICulture; + } + } } } diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index b98f902e994..38288c46e34 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1478,12 +1478,25 @@ internal static string ConvertToString(object valueToConvert) else { // The fall back is always to just convert to a string directly. - convertedString = valueToConvert.ToString(); + // Issue: https://github.com/dotnet/msbuild/issues/9757 + if (IsNumberType(valueToConvert) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) + { + convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture.NumberFormat); + } + else + { + convertedString = valueToConvert.ToString(); + } } return convertedString; } + private static bool IsNumberType(object obj) + { + return obj is double || obj is long || obj is int || obj is byte; + } + /// /// Look up a simple property reference by the name of the property, e.g. "Foo" when expanding $(Foo). /// From 170e3279627d708ff06ce91fbcba972f5da64bd7 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 15 Mar 2024 16:27:21 +0100 Subject: [PATCH 2/8] Update the PR number in docs --- documentation/wiki/ChangeWaves.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 036a6bf4f7c..0019fa24a79 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -34,7 +34,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Load NuGet.Frameworks into secondary AppDomain (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9446) - [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655) - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722) -- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types](https://github.com/dotnet/msbuild/pull/) +- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types](https://github.com/dotnet/msbuild/pull/9874) ### 17.8 From 55f0aa86a2600859963b8bca76508ccd7d0046a5 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 18 Mar 2024 12:59:57 +0100 Subject: [PATCH 3/8] Respect the ICU availability in the tests --- .../Evaluation/Expander_Tests.cs | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Expander_Tests.cs b/src/Build.UnitTests/Evaluation/Expander_Tests.cs index 6ae516d1d8c..1e4e9b38918 100644 --- a/src/Build.UnitTests/Evaluation/Expander_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Expander_Tests.cs @@ -4873,8 +4873,8 @@ public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData() var svSECultureInfo = new CultureInfo("sv-SE"); using (var env = TestEnvironment.Create()) { - CultureInfo.CurrentCulture = svSECultureInfo; - CultureInfo.CurrentUICulture = svSECultureInfo; + currentThread.CurrentCulture = svSECultureInfo; + currentThread.CurrentUICulture = svSECultureInfo; var root = env.CreateFolder(); var projectFile = env.CreateFile(root, ".proj", @@ -4901,6 +4901,12 @@ public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData() [Fact] public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData_RespectingChangeWave() { + // Note: Skipping the test since it is not a valid scenario when ICU mode is not used. + if (!ICUModeAvailable()) + { + return; + } + var currentThread = Thread.CurrentThread; var originalCulture = currentThread.CurrentCulture; var originalUICulture = currentThread.CurrentUICulture; @@ -4911,8 +4917,8 @@ public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData_Respect using (var env = TestEnvironment.Create()) { env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_10.ToString()); - CultureInfo.CurrentCulture = svSECultureInfo; - CultureInfo.CurrentUICulture = svSECultureInfo; + currentThread.CurrentCulture = svSECultureInfo; + currentThread.CurrentUICulture = svSECultureInfo; var root = env.CreateFolder(); var projectFile = env.CreateFile(root, ".proj", @@ -4937,5 +4943,17 @@ public void ExpandItem_ConvertToStringUsingInvariantCultureForNumberData_Respect currentThread.CurrentUICulture = originalUICulture; } } + + /// + /// Determines if ICU mode is enabled. + /// Copied from: https://learn.microsoft.com/en-us/dotnet/core/extensions/globalization-icu#determine-if-your-app-is-using-icu + /// + private static bool ICUModeAvailable() + { + SortVersion sortVersion = CultureInfo.InvariantCulture.CompareInfo.Version; + byte[] bytes = sortVersion.SortId.ToByteArray(); + int version = bytes[3] << 24 | bytes[2] << 16 | bytes[1] << 8 | bytes[0]; + return version != 0 && version == sortVersion.FullVersion; + } } } From 2acd8858d049c0efd77d849a62ca05df72545648 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 18 Mar 2024 14:10:36 +0100 Subject: [PATCH 4/8] InvariantCulture for all types --- src/Build/Evaluation/Expander.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 38288c46e34..94a6157f2df 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1479,9 +1479,9 @@ internal static string ConvertToString(object valueToConvert) { // The fall back is always to just convert to a string directly. // Issue: https://github.com/dotnet/msbuild/issues/9757 - if (IsNumberType(valueToConvert) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) { - convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture.NumberFormat); + convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture); } else { @@ -1492,11 +1492,6 @@ internal static string ConvertToString(object valueToConvert) return convertedString; } - private static bool IsNumberType(object obj) - { - return obj is double || obj is long || obj is int || obj is byte; - } - /// /// Look up a simple property reference by the name of the property, e.g. "Foo" when expanding $(Foo). /// From 4517fcac0cd8b7956128153ebf683b7351131cf1 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 18 Mar 2024 17:56:15 +0100 Subject: [PATCH 5/8] Return back the numeric type comparison --- src/Build/Evaluation/Expander.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 94a6157f2df..6f8cb0c865d 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1479,7 +1479,7 @@ internal static string ConvertToString(object valueToConvert) { // The fall back is always to just convert to a string directly. // Issue: https://github.com/dotnet/msbuild/issues/9757 - if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) + if (IsNumberType(valueToConvert) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) { convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture); } @@ -1492,6 +1492,11 @@ internal static string ConvertToString(object valueToConvert) return convertedString; } + private static bool IsNumberType(object obj) + { + return obj is double || obj is long || obj is int; + } + /// /// Look up a simple property reference by the name of the property, e.g. "Foo" when expanding $(Foo). /// From c95376d4ea83d6dcfed269afcfc729ca7d141558 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 18 Mar 2024 17:59:54 +0100 Subject: [PATCH 6/8] Update the wiki including the types which are converted via InvariantCulture --- documentation/wiki/ChangeWaves.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index ccf559595cf..f5cdf156d82 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -34,7 +34,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655) - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722) - [Keep the encoding of standard output & error consistent with the console code page for ToolTask](https://github.com/dotnet/msbuild/pull/9539) -- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types](https://github.com/dotnet/msbuild/pull/9874) +- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types (double, long, int)](https://github.com/dotnet/msbuild/pull/9874) ### 17.8 From cafdb98146c8aacfe288269670882d7e0765b7bf Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 18 Mar 2024 18:00:13 +0100 Subject: [PATCH 7/8] Use CultureInfo.InvariantCulture.NumberFormat --- src/Build/Evaluation/Expander.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 6f8cb0c865d..f8b815bb93e 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1481,7 +1481,7 @@ internal static string ConvertToString(object valueToConvert) // Issue: https://github.com/dotnet/msbuild/issues/9757 if (IsNumberType(valueToConvert) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) { - convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture); + convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture.NumberFormat); } else { From dd6374d81349fc5d98ecca0055f128a5598b6ad4 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 25 Mar 2024 11:30:12 +0100 Subject: [PATCH 8/8] ConvertToString using invariant culture for all types --- documentation/wiki/ChangeWaves.md | 2 +- src/Build/Evaluation/Expander.cs | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index b245210d89e..8282f5e8ed2 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -35,7 +35,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722) - [Introduce [MSBuild]::StableStringHash overloads](https://github.com/dotnet/msbuild/issues/9519) - [Keep the encoding of standard output & error consistent with the console code page for ToolTask](https://github.com/dotnet/msbuild/pull/9539) -- [Convert.ToString during a property evaluation uses the InvariantCulture for numeric types (double, long, int)](https://github.com/dotnet/msbuild/pull/9874) +- [Convert.ToString during a property evaluation uses the InvariantCulture for all types](https://github.com/dotnet/msbuild/pull/9874) ### 17.8 diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 5328f1f15ff..11bfe678360 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1479,9 +1479,9 @@ internal static string ConvertToString(object valueToConvert) { // The fall back is always to just convert to a string directly. // Issue: https://github.com/dotnet/msbuild/issues/9757 - if (IsNumberType(valueToConvert) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) + if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)) { - convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture.NumberFormat); + convertedString = Convert.ToString(valueToConvert, CultureInfo.InvariantCulture); } else { @@ -1492,11 +1492,6 @@ internal static string ConvertToString(object valueToConvert) return convertedString; } - private static bool IsNumberType(object obj) - { - return obj is double || obj is long || obj is int; - } - /// /// Look up a simple property reference by the name of the property, e.g. "Foo" when expanding $(Foo). ///