[Bug]: Inconsistent floating point behaviour changed between msbuild 16 and 17

[oskarb]

Issue Description

Running in Swedish regional settings, certain expressions that involve dividing and rounding values from msbuild properties return different results in msbuild 17 compared to msbuild 16. I suspect it's related to #8710 from 17.8.

Steps to Reproduce

In summary, I take TotalSeconds (double) from a TimeSpan and put that in an msbuild property. Depending on how I then combined dividing and rounding in separate or combined expressions, the output is different.

The following msbuild file show the differences:

<?xml version="1.0" encoding="utf-8"?>
<Project
 xmlns="http://schemas.microsoft.com/developer/msbuild/2003"
         ToolsVersion="Current"
         DefaultTargets="Build">

    <Target Name="Build">
    
        <PropertyGroup>
            <TotalSeconds>$([System.DateTime]::UtcNow.Subtract($([System.DateTime]::UtcNow.Date)).TotalSeconds)</TotalSeconds>
    
            <Divided>$([MSBuild]::Divide($(TotalSeconds), 2))</Divided>
            <ThenRounded>$([System.Math]::Round($(Divided)))</ThenRounded>
    
            <CombinedDivideAndRound>$([System.Math]::Round($([MSBuild]::Divide($(TotalSeconds), 2))))</CombinedDivideAndRound>
    
            <AllInOne>$([System.Math]::Round($([MSBuild]::Divide($([System.DateTime]::UtcNow.Subtract($([System.DateTime]::UtcNow.Date)).TotalSeconds), 2))))</AllInOne>
        </PropertyGroup>
  
         <!-- This will output something like 31161,3224946. -->
        <Message Text="TotalSeconds: $(TotalSeconds)" Importance="High"/>
        
        <!-- Sending the msbuild (string?) property $(TotalSeconds) to the msbuild Divide function.
             Seems msbuild 16 will understand the decimal comma, while msbuild 17 will ignore it (thousand sep?) -->
        <!-- MSBUILD 16 output: 15580,66112473 -->
        <!-- MSBUILD 17 output: 1558066112473 -->
        <Message Text="Divided: $(Divided)" Importance="High"/>
        
        <!-- Sending the msbuild (string?) property $(Divided) to the Math Round function.
             In this case also msbuild 16 will ignore the decimal comma that we got in $(Divided)
             on that version. -->
        <!-- MSBUILD 16 output: 1558066112473 -->
        <!-- MSBUILD 17 output: 1558066112473 -->
        <Message Text="ThenRounded: $(ThenRounded)" Importance="High"/>
        
        <!-- Sending the output from divide directly to round in a single expression. -->
        <!-- MSBUILD 16 output: 15581 -->
        <!-- MSBUILD 17 output: 1558066112473 -->
        <Message Text="CombinedDivideAndRound: $(CombinedDivideAndRound)" Importance="High"/>
        
        <!-- Everything in a single expression. Correct output on both versions. But the expression is long and difficult to read... -->
        <!-- MSBUILD 16 output: 15581 -->
        <!-- MSBUILD 17 output: 15581 -->
        <Message Text="AllInOne: $(AllInOne)" Importance="High"/>
    </Target>

</Project>

Expected Behavior

I don't want to claim this is really the expected output, but it is the actual output on msbuild 16.11:

> "C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\MSBuild\Current\Bin\amd64\MSBuild.exe" dividetest.msbuild
  TotalSeconds: 34655,5998993
  Divided: 17327,79994965
  ThenRounded: 1732779994965
  CombinedDivideAndRound: 17328
  AllInOne: 17328

Actual Behavior

This is the actual output from msbuild 17.8:

"C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Current\Bin\amd64\MSBuild.exe" dividetest.msbuild
  TotalSeconds: 34738,1944274
  Divided: 173690972137
  ThenRounded: 173690972137
  CombinedDivideAndRound: 173690972137
  AllInOne: 17369

Analysis

It seems that on v16, msbuild will honour the decimal comma when it's present in a string msbuild property passed to Divide, but not when passed to Round. For Round I assume it is instead perceived as a thousands separator.

In v17, it is perceived as a thousands separator also for the Divide method.

When the different methods are combined into a single expression it seems to work and produce the correct result - I suppose in this case it never round-trips as a string msbuild property. But the expression gets long and difficult to read.

I suspect this is related to the application of InvariantCulture for double TryParse as part of #8710.

I do hesitate to claim this is a bug... Perhaps it is as intended, but it does raise an interesting dilemma:

When a floating point value is put in a string msbuild property, a decimal comma will be used. When later trying to use this property as an argument to something that takes a double, the decimal comma will be silently treated as a thousands separator instead. It is inconsistent. And to make matters worse, I suppose the same syntax will work fine in regions using a decimal point, but then produce incorrect results if run by someone in e.g. Swedish regional format settings.

If double parsing will insist on requiring decimal point, should msbuild not take care to use that also when converting doubles to strings?

So far I haven't found any docs talking about this subject in terms of "best practice" or similar.

Versions & Configurations

No response

[KirillOsenkov]

@jrdodds @tannergooding

[jrdodds]

The change in PR #8710 was for BUG #8798

[MichalPavlik]

@rainersigwald, @baronfel What would be the best strategy here? From engineering point of view, I would prefer to have everything invariant, but it could be a break change for some customers :| Other option is to revert the change and be dependent on current active culture.

[oskarb]

As for this excerpt from my analysis, regarding the inconsistency:

When a floating point value is put in a string msbuild property, a decimal comma will be used. When later trying to use this property as an argument to something that takes a double, the decimal comma will be silently treated as a thousands separator instead. It is inconsistent. And to make matters worse, I suppose the same syntax will work fine in regions using a decimal point, but then produce incorrect results if run by someone in e.g. Swedish regional format settings.

If double parsing will insist on requiring decimal point, should msbuild not take care to use that also when converting doubles to strings?

I now think that this inconsistency is what #9874 was about. It was resolved in 17.11.

Back in May when we started getting 17.8 I summarised and worked around the issue like this:

<!--Calculation of AllieraBuildSeconds. Seconds since midnight divided by two. -->
  <PropertyGroup>
    <!-- On msbuild 16 it worked to have separate properties like this, even with Swedish formatting
         putting a decimal comma in TotalSeconds. On msbuild 17 (17.8) it will instead parse that back
         as a thousands separator, producing a much too large value. Putting it all in a single expression works.
         See msbuild issues #8710 and #10105. Possibly #9874 fixes it in 17.11 (changewave 17.12) by always using a decimal point? -->
    <!-- <TotalSeconds>$([System.DateTime]::UtcNow.Subtract($([System.DateTime]::UtcNow.Date)).TotalSeconds)</TotalSeconds> -->
    <!-- <AllieraBuildSeconds>$([System.Math]::Round($([MSBuild]::Divide($(TotalSeconds), 2))))</AllieraBuildSeconds> -->
    <AllieraBuildSeconds>$([System.Math]::Round($([MSBuild]::Divide($([System.DateTime]::UtcNow.Subtract($([System.DateTime]::UtcNow.Date)).TotalSeconds), 2))))</AllieraBuildSeconds>
  </PropertyGroup>

And now, when some developers have moved to 17.11 we got a similar error in a different, but related code block, that I handled and described in this way just last week:

 <!--Calculation of AllieraBuildDays. Days passed since the introduction of the current versioning schema. -->
  <PropertyGroup>
    <!-- The following two lines fail from msbuils 17.11 likely due to msbuild change #9874. Before this change,
         TotalDays would be coerced to string using decimal *comma* in Swedish locale, and ToDouble() would parse
         that as expected using the same locale. Starting in 17.11 (changewave 17.12), msbuild will always use 
         a decimal point when converting to string, which .Net ToDouble() will not accept. We could fix for 17.11 by
         just removing the ToDouble(), but then on 17.10 and earlier there will still be a decimal comma and
         msbuild built-in coercion back to double will interpret it as a thousands separator. -->
    <!-- <TotalDays>$([System.DateTime]::UtcNow.Subtract($([System.DateTime]::Parse("2020-01-01"))).TotalDays)</TotalDays> -->
    <!-- <AllieraBuildDays>$([System.Math]::Floor($([System.Convert]::ToDouble($(TotalDays)))))</AllieraBuildDays> -->
    <AllieraBuildDays>$([System.Math]::Floor($([System.DateTime]::UtcNow.Subtract($([System.DateTime]::Parse("2020-01-01"))).TotalDays)))</AllieraBuildDays>
  </PropertyGroup>

So at the moment I'm thinking that

• From 17.11 msbuild seems consistent in that string conversion both from and to double/decimal is consistent - always using invariant culture in both cases, if I understand the various issues/PR:s correctly.
• However, for some cases it can be tricky to write an expression that works and produces the same result on both v16, v17.8 and v17.11. In my two cases I resolved it my making it one very long expression, which presumably simply skips the string conversion to avoid the problems on older versions.

The very long expressions are a bit difficult to read but I suppose they could be simplified after enough time has passed. I suppose conditionals and separate expressions could be an option.

As for the specific behaviour I wanted in my code - maybe there are other ways to do it but it was what I came up with in reasonable time.

So unless you see some wider problem here I suspect this can be closed.

[MichalPavlik]

I see, thanks for the info. You could try to use overloads for number parsing and Convert.ToString to define culture you want.

For example:

<Message Text="$([System.Double]::Parse('1234,56', $([System.Globalization.CultureInfo]::GetCultureInfo('cs-CZ'))))" Importance="High" />
<Message Text="$([System.Double]::Parse('1234,56', $([System.Globalization.CultureInfo]::GetCultureInfo('en-US'))))" Importance="High" />

outputs

1234.56
123456

so they are parsed differently.

I'm closing this issue, but feel free to reopen it if you need additional support.