Roslyn: DateTime.Parse(string s) should have a warning

[ghost]

According to docs at: https://docs.microsoft.com/ar-sa/dotnet/api/system.datetime.parse?view=netframework-4.7.1#System_DateTime_Parse_System_String_ :

This overload attempts to parse s by using the formatting conventions of the current culture. The current culture is indicated by the CultureInfo.CurrentCulture property.

This means:

var D = DateTime.Parse("1/15/2018");

is equivilant to:

var D = DateTime.Parse("1/15/2018", CultureInfo.CurrentCulture);

So, that code will have different results on different PCs!.. On Windows with Arabic culture, that code will cause an exception, because this culture uses the format: "dd/MM/yyyy" and there is no 15th month!

This is a bad error-prone design! This overload should work with the Invariant Culture ("en"), and if anyone wants to change that, uses the second param. in the other overloads.

I suggest that the IDE should warn us about this trap, and offer to replace any new DateTime.Parse(s) and it’s alike methods with DateTime.Parse(s, new CultureInfo("en")).
Note: the Convert.ToDateTime calls DateTime.Parse(s)!

[Neme12]

ehm... did you know that pretty much everything uses current culture by default including things like int.Parse and even "hello " + 0.1? Should we add a warning here? This is just something you need to be aware of either way no matter which one (current or invariant) you happen to need right now.

This is what I get on my machine:

[Neme12]

You can definitely write a custom analyzer that would require explicitly passing a culture to any sort of formatting functions. I'm sure many people would find that useful (although it could get pretty annoying especially with string interpolation & concatenation)

[ghost]

@neme12
I think it's a bad choice to make parsing depend on CurrentCulture, becuase it will make troubles when the app runs on different PCs with different cultures! The logical thing is to use an invariant culture in code, and use globalization and localization to output the result to the UI. I already write a code in a foreign language, so what benifit I gain from useing my local culture to format dates and numbers in code? I have to use the longest second oveload of the Parse method to avoid problems involved with the first overload.
Thanks.

[Neme12]

I think it's a bad choice to make parsing depend on CurrentCulture, becuase it will make troubles when the app runs on different PCs with different cultures!

Ok? That's an interesting discussion to have but this can't be changed regardless.

I already write a code in a foreign language, so what benifit I gain from useing my local culture to format dates and numbers in code?

I really don't know what you're saying here. So that users can see localized dates and numbers?

[Neme12]

The logical thing is to use an invariant culture in code, and use globalization and localization to output the result to the UI

I don't see the logic here. If you use invariant culture in code to create a string and then output that result to the UI, wouldn't it still be created with the invariant culture?

[ghost]

@Neme12
This problem can be solved at the IL by translating the Parse(s) to Parse(s, c) where c is the exact culture used. This will preserve existing codes, let us use our cultures, and make the code run the same wayy on all PCs whatever their CurrentCutlure is. Is this possible?

[HaloFour]

@MohammadHamdyGhanem

There is no problem here. These APIs were all explicitly designed to work with the current culture because when parsing input from an end user that is exactly the behavior that is expected.

[sharwell]

I believe this issue is outside the scope of this repository. It likely falls into one of the following:

1. dotnet/coreclr or dotnet/corefx for a request to mark the member(s) [Obsolete]
2. dotnet/roslyn-analyzers for a request to add an analyzer that reports diagnostics for uses of the method

[CyrusNajmabadi]

@Neme12
This problem can be solved at the IL by translating the Parse(s) to Parse(s, c) where c is the exact culture used. This will preserve existing codes,

No it won't. As you said, it would change the IL to something different. So that would actually break people who are using these APIs as they are currently documented.

let us use our cultures, and make the code run the same wayy on all PCs whatever their CurrentCutlure is. Is this possible?

Yes. You are free to create these sorts of adhoc tools for your own purpose. But, as mentioned in other bugs you have opened, suggestion along the lines of "break users" will face a large uphill battle.

--

As I recommended in your other issue, i think your time would be better spent writing analyzers for the sorts of problems you care about instead of opening up issues asking for MS to make breaking changes every time you run into something. I say that just because hte latter has almost no chance of succeeding, whereas the former would allow you to actually generate real solutions to the problems that you feel you are facing.

[ghost]

@CyrusNajmabadi
I prefer to criticize the design regardless you will fix the issue or not. Being aware of our openions can affect your new designs. Besides things don't last forever, and MS is famous of breakthroughs and abandoned technologies. Some day you may find most of programmers depend on customized nugets instead of basic .Net classes, just to avoid some annoying unexpected behaviors.
So, please be patient with me. I can make my code work by any means, but I like to give you feeadbacks.
Thanks.

[ghost]

@CyrusNajmabadi

it would change the IL to something different. So that would actually break people who are using these APIs as they are currently documented

The only break that will happen, is preventing an exception on other PCs that have different cultures than the programmer's.
If a German programmer uses DateTime.Parse(s) on his PC, it will parse his strings in German format. If I run his app on my PC, it will try to parse the same string in Arabic format and it will crash! If IL changed the code of the Parse(s) to Pasre(s, new CultureInfo("de"), the app will run in germany as expected, and also will run on my PC without crashing! So, this will break nothing. It will fix.

[HaloFour]

@MohammadHamdyGhanem

The machine of the developer who happened to compile the code should be entirely irrelevant. If a German programmer ships an application and someone on an Arabic computer tries to enter a date, it should accept and parse that date in Arabic format.

If you, as the developer, want to explicitly parse a String into a Date using a single predictable format then it if your job, as the developer, to specify that culture.

[ghost]

@HaloFour

it should accept and parse that date in Arabic format.

Parse(s) depends on CultureInfo.CurrentCulture which depends on the operating system language and regional settings.
Parse("1/15/2018") should work on your machine but will fail on mine!
This makes this overload buggy and the second overload is the practical solution.

[Neme12]

This makes this overload buggy

No it doesn't. That's the expect behavior, not a bug.

[HaloFour]

@MohammadHamdyGhanem

Parse(s) depends on CultureInfo.CurrentCulture which depends on the operating system language and regional settings.

Yes, this is exactly what it's supposed to be doing so that it can parse dates in the format of whoever is running the application. That's why it was designed the way it was designed. That's not a bug, regardless of how much you insist it must be. The behavior of those members is never going to be changed, nor will the compiler ever emit a warning when invoking them.

[sharwell]

If IL changed the code of the Parse(s) to Pasre(s, new CultureInfo("de"), the app will run in germany as expected, and also will run on my PC without crashing!

This type of change would need to be proposed and accepted in dotnet/csharplang before it could be implemented here in the compiler.

[sharwell]

@MohammadHamdyGhanem GitHub is certainly a great place to bring up issues that led to struggles during software development with C#. However, this repository is not the correct repository for the specific issue(s) you've raised in this thread. As such, no matter how much discussion occurs in this issue, there is no way we could reach any sort of a decision because the owners of the impacted features work in other repositories. I've provided links to other repositories that you can use to file specific proposals if you wish, and if any proposal is accepted in one of those locations it will become actionable.

If you are instead interested in building an analyzer (and possibly code fix) that users could install if they want some customized behavior for handling culture-dependent methods, separately from this project and run into trouble working with the Roslyn APIs, we can help point you to answers.

[CyrusNajmabadi]

If IL changed the code of the Parse(s) to Pasre(s, new CultureInfo("de"), the app will run in germany as expected, and also will run on my PC without crashing

But it will not be working as the API is doc'ed to behave. That is a break. If i code against the documented behavior of the API, but then that changes at runtime, that is a break. For example, i use culture-sensitive operations precisely so that they adopt the culture of the end user when running. That's the point of them and there are many cases where that is completely appropriate and desirable. Changing that behavior will break my code.

[ghost]

@CyrusNajmabadi

so that they adopt the culture of the end user when running

This is true if you read input from user. I mean the case where the programmer imputs the date string in code. I didn't face this problem becuase I use the ## date Literals in VB.NET and they use the invarient culture, but C# forces the programmer to use the Parse method just to write dates in code, so this code will make trouble on other PCs. This is ths case I want to fix. If the Date is hard coded, then IL can fix this case. I suggeste to add ## in C#, but no one is excited for it!

[CyrusNajmabadi]

This is ths case I want to fix. If the Date is hard coded, then IL can fix this case.

This will change the meaning of existing code, where the user may absolutely have wanted that behavior.

As both I and sam have mentioned:

If you are instead interested in building an analyzer (and possibly code fix) that users could install if they want some customized behavior for handling culture-dependent methods, separately from this project and run into trouble working with the Roslyn APIs, we can help point you to answers.

This would solve the problem for you, and would allow you to provide that solution to other developers who want a similar sort of check for their code. As i've mentioned, changing existing behavior is unlikely to happen.

That said, if you want to continue pushing on this request, i would suggest placing it in the correct repo. Roslyn has nothing to do with IL rewriting. We simply take your code and compile it as per how you wrote it. Similarly, the method you asked to be called is beahving as it is spec'ed.

If you want the method to change it's behavior, please ask the owner of that method. If you want the IL to be rewritten, please find a more appropriate repo where that would be applicable.

[HaloFour]

@MohammadHamdyGhanem

but C# forces the programmer to use the Parse method just to write dates in code,

No it doesn't. Use the constructors instead, which are more efficient anyway (no string parsing, no culture garbage).

var dt = new DateTime(2018, 1, 15);

[CyrusNajmabadi]

I use the ## date Literals in VB.NET and they use the invarient culture,

Yes, they are behaving as per their spec. As are the DateTime.Parse methods and the C# compiler. All of htese are unlikely to change as it could absolutely break many users. However, there are many open options for how out of band solutions could be provided (including by you) to address this for customers who would be ok with the break. Many people have gone this route, and it's 100% supported and encouraged.

I think your efforts would be better spent using the existing and strongly encouraged options toward solving these problems you're having, instead of adamant insistence on paths that feel nearly certain to make no headway.

[CyrusNajmabadi]

Note: as i mentioned in your other issue, i would be happy to help out here. If you want to discuss things more on https://gitter.im/dotnet/roslyn, that would be great.

[ghost]

@HaloFour
The constructor is not readable and serchable as writing the whole date!

[CyrusNajmabadi]

Seems completely readable to me :) The year, month and day are all nicely explicitly specified. it just uses comma, instead of some other culture sensitive separator. It is now culture insensitive, which seems to solve the problem you're having :)

[ghost]

The problem is that not all programmers are aware of that Parse(s) uses thier own culture, so they use it!
See how much questions about that in stackoverflow:
https://www.google.com.eg/search?q=DateTime.Parse+causes+an+exception+site:stackoverflow.com&sa=X&ved=0ahUKEwikzp_5zvTaAhXCZFAKHQftDUgQrQIIMygEMAA&biw=853&bih=437&dpr=1.5
specially these two:
https://stackoverflow.com/questions/19487356/weird-datetime-parsing-behaviour
https://stackoverflow.com/questions/42717939/datetime-parseexact-different-behaviour-on-other-server
This is why I say it needs a warning.
A good design whouldn't make such confusion!
So, should I repost this issue to 2.dotnet/roslyn-analyzers?

[CyrusNajmabadi]

This is why I say it needs a warning.

Providing warnings can be done with analyzers. That was one of hte suggested routes i recommended. I would be happy to help you with creating such an analyzer if you want. Feel free to join over at https://gitter.im/dotnet/csharplang and i can walk you through it.

[CyrusNajmabadi]

@MohammadHamdyGhanem here's a piece of code you could start with:

```c#

[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public class DateTimeLiteralAnalyzer : DiagnosticAnalyzer
{
    private static readonly DiagnosticDescriptor s_descriptor = new DiagnosticDescriptor(
        "MHG001",
        "Do not pass literals to DateTime.Parse without explicitly providing a culture",
        "Do not pass literals to DateTime.Parse without explicitly providing a culture",
        "Globalization",
        DiagnosticSeverity.Warning,
        isEnabledByDefault: true);

    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } =
        ImmutableArray.Create(s_descriptor);

    public override void Initialize(AnalysisContext context)
    {
        context.RegisterCompilationStartAction(ctx =>
        {
            var type = ctx.Compilation.GetTypeByMetadataName(typeof(DateTime).FullName);
            if (type != null)
            {
                ctx.RegisterOperationAction(
                    opCtx => AnalyzeOperation(opCtx, type), OperationKind.Invocation);
            }
        });
    }

    private void AnalyzeOperation(OperationAnalysisContext opCtx, INamedTypeSymbol type)
    {
        var invocationOp = (IInvocationOperation)opCtx.Operation;
        if (invocationOp.TargetMethod != null && 
            type.Equals(invocationOp.TargetMethod.ContainingType) &&
            invocationOp.Arguments.Length == 1 &&
            invocationOp.Arguments[0].Kind == OperationKind.Literal &&
            invocationOp.TargetMethod.Name == nameof(DateTime.Parse))
        {
            opCtx.ReportDiagnostic(Diagnostic.Create(
                s_descriptor,
                invocationOp.Arguments[0].Syntax.GetLocation()));
        }
    }
}

```

it took me a couple of minutes to write. Certainly far less time to write it than has been spent arguing here about this topic.

Feel free to take it and expand on it for all the cases you feel are important! :)

[sharwell]

What about installing FXCop Analyzers and enabling CA1304?

CA1304: Specify CultureInfo

https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.NetCore.Analyzers/Core/Runtime/SpecifyCultureInfo.cs

[ghost]

it took me a couple of minutes to write. Certainly far less time to write it than has been spent arguing here about this topic.

Thanks, but your 2 minutes equals many hours I need to spend reading about analyzers. I started reading already, but this will take me to an area I don't fit in. Currently, I can give you more feedback, because I test many small code samples covering dozens of classes. I think it will be effictive and productive if everyone works in his layer.

By the way, I benfit from our arguing, and add new notes and warinings about these issues and how to avoid them for Arabic developers, many of them based on your replies. I applied @HaloFour advice, and replaced DateTime.Parse(s) with new DateTime() in nearly all code samples, with a hint pased on @CyrusNajmabadi argument about when using String.Parse(s) is siutable.
Thanks a lot.

[CyrusNajmabadi]

Thanks, but your 2 minutes equals many hours I need to spend reading about analyzers.

Investing this time into learning pays long term dividends.

[ghost]

@CyrusNajmabadi
The code sems easy, but there is a catch: what if the programmer put the date string in a variable, or pass it as a parameter, or get it from a resource?
We can warn about Parse(s) whether it is a leteral or not, and use the message:

"DateTime.Parse(string) uses the local culture of the machine that it runs on"

and the Detailed message:

"DateTime.Parse(string) is suitable to parse a datetime provided by the user with his local culture, but if you use it to parse yuor own dates formated in your local culture, it will succeed on your machine but can cause an exception on other machines that have different cultures than yours, so in this case you should use the DateTime.Parse(string, IFormatProvider) instead and explicitly provid a culture."

[CyrusNajmabadi]

@MohammadHamdyGhanem As i said: Feel free to take it and expand on it for all the cases you feel are important! :)

This was a starting point. You can do whatever you want with this. The important thing is that it's in your control and can be done right now. Instead of hoping that teams will end up breaking many customers in the future (which is unlikely to ever happen), you can get a solutoin that can be perfectly fit to your needs today.

[CyrusNajmabadi]

The code sems easy, but there is a catch: what if the programmer put the date string in a variable

Only only checked literals, and not anything else because you said precisely:

This is true if you read input from user. I mean the case where the programmer imputs the date string in code

You already limited the case to where the input string was provided directly and not read in from the user and placed in a variable.

[CyrusNajmabadi]

In any event, i think this issue has run it's course. If you want to discuss this anymore, i would recommend coming over to gitter. It is a far better place to actually converse about this, instead of using roslyn issues to discuss the best ways for you to implement an analyzer for this case. If you want to converse about tihs topic anymore, please continue here: https://gitter.im/dotnet/roslyn

[ghost]

@sharwell
Thanks, this helps.