dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

DateTime parsing. #9565

Closed tarekgh closed 4 years ago

tarekgh commented 6 years ago

From @wstaelens on January 12, 2018 15:32

Not sure if this is normal or not (tested in a solution with .NET 4.6.1 and in Visual Studio 2017 C# Interactive window):

> var a = DateTime.UtcNow.ToString("ffffHHMMyytssddmm");
> DateTime ts;
> bool test = DateTime.TryParseExact(a, "ffffHHMMyytssddmm", null, System.Globalization.DateTimeStyles.None, out ts);
Index was outside the bounds of the array.
  + System.DateTimeParse.MatchAbbreviatedTimeMark(ref System.__DTString, System.Globalization.DateTimeFormatInfo, ref System.DateTimeParse.TM)
  + System.DateTimeParse.ParseByFormat(ref System.__DTString, ref System.__DTString, ref System.ParsingInfo, System.Globalization.DateTimeFormatInfo, ref System.DateTimeResult)
  + System.DateTimeParse.DoStrictParse(string, string, System.Globalization.DateTimeStyles, System.Globalization.DateTimeFormatInfo, ref System.DateTimeResult)
  + System.DateTimeParse.TryParseExact(string, string, System.Globalization.DateTimeFormatInfo, System.Globalization.DateTimeStyles, out System.DateTime)
> 

Copied from original issue: Microsoft/dotnet#600

tarekgh commented 6 years ago

I cannot repro this issue. could you please try to print the value of CultureInfo.CurrentCulture to know what is the culture used in this operation? if you can share the project repro this issue that would be good too.

tarekgh commented 6 years ago

From @wstaelens on January 15, 2018 7:36

Microsoft (R) Roslyn C# Compiler version 2.6.0.62329
Loading context from 'CSharpInteractive.rsp'.
Type "#help" for more information.
> var debug = System.Globalization.CultureInfo.CurrentCulture;
> debug
[nl-BE]
> 

I have an asp.net mvc project on .net framework 4.6.1. I tested this in Visual Studio Pro 2017 version 15.5.3 in the C# Interactive window.

(I can't share the project)

tarekgh commented 6 years ago

From @svick on January 16, 2018 19:44

I can reproduce the issue on .Net 4.7.1 and .Net Core 2.0. Looking at the source code of MatchAbbreviatedTimeMark, this will happen for any culture where the AM or PM designator is an empty string (which is the case for nl-BE).

A simplified repro is:

DateTime.TryParseExact(" ", "%t", new CultureInfo("nl-BE"), DateTimeStyles.None, out _);

I think the right solution would be to ensure that all cultures have non-empty AM/PM designators, especially since it means a reasonable format like hh:mm:ss t is ambiguous (and thus does not round-trip).

Maybe English AM/PM designators (i.e. "AM" and "PM") are reasonable defaults for such cases?

tarekgh commented 6 years ago

@svick is right. I am contacting Windows team to know why nl-BE has empty strings AM/PM designators. meanwhile the workaround will be, either use a different culture or overwrite the AM/PM designators on that culture.

            CultureInfo.CurrentCulture = new CultureInfo("nl-BE");
            CultureInfo.CurrentCulture.DateTimeFormat.AMDesignator = " ";
            CultureInfo.CurrentCulture.DateTimeFormat.PMDesignator = " ";
tarekgh commented 6 years ago

@krwq could you please track fixing this issue? the problem here is the following 2 lines

https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Globalization/DateTimeParse.cs#L3610 https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Globalization/DateTimeParse.cs#L3615

as we are accessing the AMDesignator and PMDesignator strings while these are empty strings. I contacted Windows team regarding that and they mentioned that by design some cultures don't have such designators.

Please track fixing that in 2.1 and possibly 4.7.3. let me know if you want any help.

tarekgh commented 6 years ago

CC @AlexGhiondea

krwq commented 6 years ago

@tarekgh will do, thank you for investigation

tarekgh commented 6 years ago

@krwq are you tracking fixing the issue for the full framework too?

krwq commented 6 years ago

@tarekgh I marked this issue as netfx-port-consider - do we also need internal bugs?

tarekgh commented 6 years ago

yes please open TFS bug and apply the fix to the full framework enlistment.