Fix TimeSpan parsing (#21968)

* Fix TimeSpan parsing

* Temporary disabling the failed CI tests

diff --git a/src/System.Private.CoreLib/shared/System/Globalization/TimeSpanParse.cs b/src/System.Private.CoreLib/shared/System/Globalization/TimeSpanParse.cs
index 1bf8174..a9172e2 100644 (file)
--- a/src/System.Private.CoreLib/shared/System/Globalization/TimeSpanParse.cs
+++ b/src/System.Private.CoreLib/shared/System/Globalization/TimeSpanParse.cs
@@ -8,10 +8,10 @@
 //
 //  Standard Format:
 //  -=-=-=-=-=-=-=-
-//  "c":  Constant format.  [-][d'.']hh':'mm':'ss['.'fffffff]  
+//  "c":  Constant format.  [-][d'.']hh':'mm':'ss['.'fffffff]
 //  Not culture sensitive.  Default format (and null/empty format string) map to this format.
 //
-//  "g":  General format, short:  [-][d':']h':'mm':'ss'.'FFFFFFF  
+//  "g":  General format, short:  [-][d':']h':'mm':'ss'.'FFFFFFF
 //  Only print what's needed.  Localized (if you want Invariant, pass in Invariant).
 //  The fractional seconds separator is localized, equal to the culture's DecimalSeparator.
 //
@@ -43,8 +43,8 @@
 // - For multi-letter formats "TryParseByFormat" is called
 // - TryParseByFormat uses helper methods (ParseExactLiteral, ParseExactDigits, etc)
 //   which drive the underlying TimeSpanTokenizer.  However, unlike standard formatting which
-//   operates on whole-tokens, ParseExact operates at the character-level.  As such, 
-//   TimeSpanTokenizer.NextChar and TimeSpanTokenizer.BackOne() are called directly. 
+//   operates on whole-tokens, ParseExact operates at the character-level.  As such,
+//   TimeSpanTokenizer.NextChar and TimeSpanTokenizer.BackOne() are called directly.
 //
 ////////////////////////////////////////////////////////////////////////////
 
@@ -104,19 +104,50 @@ namespace System.Globalization
                 _sep = separator;
             }
 
-            public bool IsInvalidFraction()
+            public bool NormalizeAndValidateFraction()
             {
                 Debug.Assert(_ttt == TTT.Num);
                 Debug.Assert(_num > -1);
 
-                if (_num > MaxFraction || _zeroes > MaxFractionDigits)
+                if (_num == 0)
                     return true;
 
-                if (_num == 0 || _zeroes == 0)
+                if (_zeroes == 0 && _num > MaxFraction)
                     return false;
 
-                // num > 0 && zeroes > 0 && num <= maxValue && zeroes <= maxPrecision
-                return _num >= MaxFraction / Pow10(_zeroes - 1);
+                int totalDigitsCount = ((int) Math.Floor(Math.Log10(_num))) + 1 + _zeroes;
+
+                if (totalDigitsCount == MaxFractionDigits)
+                {
+                    // Already normalized. no more action needed
+                    // .9999999  normalize to 9,999,999 ticks
+                    // .0000001  normalize to 1 ticks
+                    return true;
+                }
+
+                if (totalDigitsCount < MaxFractionDigits)
+                {
+                    // normalize the fraction to the 7-digits
+                    // .999999  normalize to 9,999,990 ticks
+                    // .99999   normalize to 9,999,900 ticks
+                    // .000001  normalize to 10 ticks
+                    // .1       normalize to 1,000,000 ticks
+
+                    _num *= (int) Pow10(MaxFractionDigits - totalDigitsCount);
+                    return true;
+                }
+
+                // totalDigitsCount is greater then MaxFractionDigits, we'll need to do the rounding to 7-digits length
+                // .00000001    normalized to 0 ticks
+                // .00000005    normalized to 1 ticks
+                // .09999999    normalize to 1,000,000 ticks
+                // .099999999   normalize to 1,000,000 ticks
+
+                Debug.Assert(_zeroes > 0); // Already validated that in the condition _zeroes == 0 && _num > MaxFraction
+                _num = (int) Math.Round((double)_num / Pow10(totalDigitsCount - MaxFractionDigits), MidpointRounding.AwayFromZero);
+                Debug.Assert(_num < MaxFraction);
+
+                return true;
             }
         }
 
@@ -184,7 +215,7 @@ namespace System.Globalization
                         }
 
                         num = num * 10 + digit;
-                        if ((num & 0xF0000000) != 0)
+                        if ((num & 0xF0000000) != 0) // Max limit we can support 268435455 which is FFFFFFF
                         {
                             return new TimeSpanToken(TTT.NumOverflow);
                         }
@@ -557,7 +588,7 @@ namespace System.Globalization
                 hours._num > MaxHours ||
                 minutes._num > MaxMinutes ||
                 seconds._num > MaxSeconds ||
-                fraction.IsInvalidFraction())
+                !fraction.NormalizeAndValidateFraction())
             {
                 result = 0;
                 return false;
@@ -570,31 +601,7 @@ namespace System.Globalization
                 return false;
             }
 
-            // Normalize the fraction component
-            //
-            // string representation => (zeroes,num) => resultant fraction ticks
-            // ---------------------    ------------    ------------------------
-            // ".9999999"            => (0,9999999)  => 9,999,999 ticks (same as constant maxFraction)
-            // ".1"                  => (0,1)        => 1,000,000 ticks
-            // ".01"                 => (1,1)        =>   100,000 ticks
-            // ".001"                => (2,1)        =>    10,000 ticks
-            long f = fraction._num;
-            if (f != 0)
-            {
-                long lowerLimit = InternalGlobalizationHelper.TicksPerTenthSecond;
-                if (fraction._zeroes > 0)
-                {
-                    long divisor = Pow10(fraction._zeroes);
-                    lowerLimit = lowerLimit / divisor;
-                }
-
-                while (f < lowerLimit)
-                {
-                    f *= 10;
-                }
-            }
-
-            result = ticks * TimeSpan.TicksPerMillisecond + f;
+            result = ticks * TimeSpan.TicksPerMillisecond + fraction._num;
             if (positive && result < 0)
             {
                 result = 0;
@@ -1338,7 +1345,7 @@ namespace System.Globalization
 
                     case '%':
                         // Optional format character.
-                        // For example, format string "%d" will print day 
+                        // For example, format string "%d" will print day
                         // Most of the cases, "%" can be ignored.
                         nextFormatChar = DateTimeFormat.ParseNextChar(format, i);
 
@@ -1455,7 +1462,7 @@ namespace System.Globalization
 
         /// <summary>
         /// Parses the "c" (constant) format.  This code is 100% identical to the non-globalized v1.0-v3.5 TimeSpan.Parse() routine
-        /// and exists for performance/appcompat with legacy callers who cannot move onto the globalized Parse overloads. 
+        /// and exists for performance/appcompat with legacy callers who cannot move onto the globalized Parse overloads.
         /// </summary>
         private static bool TryParseTimeSpanConstant(ReadOnlySpan<char> input, ref TimeSpanResult result) =>
             new StringParser().TryParse(input, ref result);
@@ -1628,7 +1635,7 @@ namespace System.Globalization
                 if (_ch == ':')
                 {
                     NextChar();
-                    
+
                     // allow seconds with the leading zero
                     if (_ch != '.')
                     {

diff --git a/tests/CoreFX/CoreFX.issues.json b/tests/CoreFX/CoreFX.issues.json
index 3699ee5..25970b3 100644 (file)
--- a/tests/CoreFX/CoreFX.issues.json
+++ b/tests/CoreFX/CoreFX.issues.json
@@ -1001,6 +1001,22 @@
                 {
                     "name": "System.Tests.ArrayTests.Copy",
                     "reason": "Needs updates for XUnit 2.4"
+                },
+                {
+                    "name": "System.Tests.TimeSpanTests.Parse_Invalid",
+                    "reason": "Temporary disabling till merging the PR https://github.com/dotnet/corefx/pull/34561"
+                },
+                {
+                    "name": "System.Tests.TimeSpanTests.Parse_Span",
+                    "reason": "Temporary disabling till merging the PR https://github.com/dotnet/corefx/pull/34561"
+                },
+                {
+                    "name": "System.Tests.TimeSpanTests.Parse_Span_Invalid",
+                    "reason": "Temporary disabling till merging the PR https://github.com/dotnet/corefx/pull/34561"
+                },
+                {
+                    "name": "System.Tests.TimeSpanTests.Parse",
+                    "reason": "Temporary disabling till merging the PR https://github.com/dotnet/corefx/pull/34561"
                 }
             ]
         }