[Bug]: Numeric comparison uses current culture

[akoeplinger]

Issue Description

We hit this in dotnet/runtime#98550 where a numeric comparison fails in the fi_FI culture because it uses U+2212 : MINUS SIGN instead of - for negative numbers.

I'm not sure whether this is intentional or a bug.

Steps to Reproduce

<Project>

  <PropertyGroup>
    <_value>$([MSBuild]::Subtract(0, 1))</_value>
    <_otherValue Condition="'$(_value)' &gt; -1">true</_otherValue>
  </PropertyGroup>

  <Target Name="Build" />
</Project>

Expected Behavior

No error.

Actual Behavior

$ LANG=fi_FI dotnet msbuild culture.proj
MSBuild version 17.10.0-preview-24101-01+07fd5d51f for .NET
  culture failed with errors (0,0s)
    /Users/alexander/dev/test/culture.proj(5,18): error MSB4086: A numeric comparison was attempted on "$(_value)" that evaluates to "−1" instead of a number, in condition "'$(_value)' > -1". [/Users/alexander/dev/test/culture.proj]

Note that the "−1" in the error message uses U+2212 : MINUS SIGN

Analysis

No response

Versions & Configurations

Fails with both 8.0.100 and 9.0.100 P1 dotnet SDKs.

[akoeplinger]

Ah looks like this was reported in #5502 too but I'm not sure we should've closed the issue back then.

[danmoseley]

Yeah I thought it looked familiar. But IIRC I didn't see any numeric parse that wasn't culture invariant. I remember fixing an unrelated regex that wasn't, though.

[danmoseley]

It was #7499 and some are still missing invariant but not relevant to this.

[rainersigwald]

From @danmoseley in dotnet/runtime#98550 (comment)

@rainersigwald

It looks like <_indexOfPeriod>$(_originalTargetOS.IndexOf('.'))</_indexOfPeriod> causes the int->string conversion to happen with current culture:

this seems like something MSBuild should consider fixing -- by whatever trick inside the binder. Anything culture-specific by default in a build seems like a bug to me. wdyt?

This looks like the problem, as opposed to the string->int conversion that's actually reporting the problem (but appears to be culture neutral or it would understand the localized U+2212.

I agree on principle that everything should be culture-neutral. As usual, I'm worried about regressions--but that feels like something we could try behind a changewave.

[f-alizada]

As @rainersigwald mentioned the problem in different approaches to converting string -> int and int -> string
The code to reproduce the issue:

public static void CheckConvertToString()
{
    Thread.CurrentThread.CurrentCulture = new CultureInfo("fi_FI");

    long minusOne = -1;
    object minusOneObj = minusOne;
    string minusOneString = minusOneObj.ToString();

    Console.WriteLine((int)minusOneString.ToString()[0]); // 8722


    // This will not be parsed. This scenario happens in MSBuild
    if (Double.TryParse(minusOneString, NumberStyles.AllowDecimalPoint | NumberStyles.AllowLeadingSign, CultureInfo.InvariantCulture.NumberFormat, out var val))
    {
        Console.WriteLine($"parsed neutral = {val}");
    }
    else
    {
        Console.WriteLine("Not parsed neutral");
    }

    // this will be parsed. 
    if (Double.TryParse(minusOneString, out var val2))
    {
        Console.WriteLine($"parsed  Non neutral = {val2}");
    }
    else
    {
        Console.WriteLine("Not parsed Non neutral");
    }
}

The issue described earlier reproducible on unix following the steps. However I was not able to reproduce the issue on windows yet (except the simple code).
The fix would be in

msbuild/src/Build/Evaluation/Expander.cs

Line 1417 in 0326fd7

internal static string ConvertToString(object valueToConvert)

to convert Culture neutral in case the object is a number.
Temp fix tested and verified it is working.

[f-alizada]

Update to the previous comment: In case there Thread.CurrentThread.CurrentCulture = new CultureInfo("fi_FI"); is not set and the CultureInfo is set from the Windows settings there is no errors.