Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in CultureInfo.Clone() causes .Calendar and .DateTimeFormat.Calendar to diverge #40953

Closed
mklement0 opened this issue Sep 9, 2019 · 19 comments · Fixed by dotnet/coreclr#26721

Comments

@mklement0
Copy link
Collaborator

commented Sep 9, 2019

Courtesy of excellent sleuthing by @lpatalas:

Accessing a CultureInfo instance's .DateTimeFormat property before it is cloned with .Clone() unexpectedly causes the clone's .Calendar and .DateTimeFormat.Calendar properties to reference different objects; from PowerShell/PowerShell#10438 (comment):

If I would have to guess it's caused by this line: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L1015. The _calendar field is null so it's not cloned until it's accessed for the first time. After that it's set and each subsequent Clone() call will create new Calendar instance.

Code to reproduce:

using System;
using System.Globalization;

public static class Program
{
    public static void Main()
    {
      var orig = CultureInfo.InvariantCulture;
      
      for (var i=0; i<2; ++i) {

        var clone = (CultureInfo)orig.Clone();
        clone.Calendar.TwoDigitYearMax = 2020;

        Console.WriteLine($"Clone {i}: Do .Calendar and .DateTimeFormat.Calendar referrence the same object? {object.ReferenceEquals(clone.Calendar, clone.DateTimeFormat.Calendar)}");
        Console.WriteLine($"Clone {i}: .Calendar.TwoDigitYearMax vs. .DateTimeFormat.Calendar.TwoDigitYearMax: {clone.Calendar.TwoDigitYearMax} vs. {clone.DateTimeFormat.Calendar.TwoDigitYearMax}");

        // Trigger the bug: after this property access,
        // cloning `orig` again makes the clones' .Calendar and .
        // DateTimeFormat.Calendar references *differ*.
        var dummy = orig.DateTimeFormat;
      }

    }
}

Expected:

Clone 0: Do .Calendar and .DateTimeFormat.Calendar referrence the same object? True
Clone 0: .Calendar.TwoDigitYearMax vs. .DateTimeFormat.Calendar.TwoDigitYearMax: 2020 vs. 2020
Clone 1: Do .Calendar and .DateTimeFormat.Calendar referrence the same object? True
Clone 1: .Calendar.TwoDigitYearMax vs. .DateTimeFormat.Calendar.TwoDigitYearMax: 2020 vs. 2020

Actual (the 2nd iteration exhibits the bug):

Clone 0: Do .Calendar and .DateTimeFormat.Calendar referrence the same object? True
Clone 0: .Calendar.TwoDigitYearMax vs. .DateTimeFormat.Calendar.TwoDigitYearMax: 2020 vs. 2020
Clone 1: Do .Calendar and .DateTimeFormat.Calendar referrence the same object? False
Clone 1: .Calendar.TwoDigitYearMax vs. .DateTimeFormat.Calendar.TwoDigitYearMax: 2020 vs. 2029
@lpatalas

This comment has been minimized.

Copy link

commented Sep 9, 2019

Actually accessing CultureInfo.DateTimeFormat alone is enough to trigger behaviour described above.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2019

Thanks, @lpatalas - I've corrected the initial post accordingly.

@scalablecory

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Thanks for the great repro!

@tarekgh tarekgh added this to the 5.0 milestone Sep 9, 2019

@tarekgh tarekgh added the bug label Sep 9, 2019

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Actually this is by design. here is some comments why:

  • Cloning depends on the object state. If you cloned the same object twice while changing the state of this object between the 2 cloning operations, then expect you get a different cloned objects.
  • When requesting CultureInfo.DateTimeFormat for the first time, it is actually changing the cultureInfo object state as it creates a new DateTimFormatInfo object using the CultureInfo.Calendar object.
  • CultureInfo.Clanedar and CultureInfo.DateTimeFormat.Calendar is not necessary to match all the time. If you tweak one it doesn't have to affect the other. And it depends if you tweak CultureInfo.Calendar before of after CultureInfo.DateTimeFormat get created. As I mentioned first time you request CultureInfo.DateTimeFormat, will create the DateTimeFormatInfo using CultureInfo.Calendar.
  • During the CultureInfo cloning we don't create a new objects for the properties which has null values for the following reason:
    • It is not good to change the original object state while we are just cloning it.
    • The most important reason is the perf. To avoid creating more objects when cloning while not necessary these objects are going to be used.

I agree this maybe a little bit not clear that when requesting CultureInfo.DateTimeFormat is changing the CultureInfo. We can clarify that in the docs if needed.

@tarekgh tarekgh closed this Sep 10, 2019

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

while changing the state of this object between the 2 cloning operations

There is no intentional change of state here - just an implementation detail (lazy initialization) that should never affect the behavior.

In fact, in PowerShell you don't get a choice: merely referencing [cultureinfo]::InvariantCulture] retrieves the .DateTimeFormat property behind the scenes and therefore invariably surfaces the problem.

CultureInfo.Clanedar and CultureInfo.DateTimeFormat.Calendar is not necessary to match all the time.

What is their intended relationship and when does it ever make sense for them to point to different objects? It seems to me that .Calendar, as a read-only property, is only ever meant to reflect the same value as the read-write .DateTimeFormat.Calendar property; from the documentation of the .Calendar property:

Your application changes the calendar used by the current CultureInfo by setting the Calendar property of DateTimeFormat

If the current behavior is by design, I suggest revisiting that design, given its highly obscure and hard-to-predict-by-the-user nature.
Please reopen this issue.

@GSPP

This comment has been minimized.

Copy link

commented Sep 11, 2019

I think the core of the issue is that the getter of CultureInfo.DateTimeFormat changes state in ways that are observable. It's not just lazy initialization or caching. It's actually a side-effect in a property getter.

@danmosemsft danmosemsft reopened this Sep 11, 2019

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Reopening to continue discussion above.

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

There is no intentional change of state here - just an implementation detail (lazy initialization) that should never affect the behavior.

Not really, when accessing the DateTimeFormat we'll create it using the CultureInfo.Calendar which is not just lazy initialization. You actually changing the state according what you have customized in Calendar.

It seems to me that .Calendar, as a read-only property, is only ever meant to reflect the same value as the read-write .DateTimeFormat.Calendar property

Not true, CultureInfo.Calendar represent the default calendar of the culture. DateTimeFormat.Calendar can be set to any other optional calendar for this culture. for example, ja-JP can have Gregorian as the default and can customize the DateTimeInfo.Calendar to be Japanese calendar. If both are same, then wouldn't make sense exposing both in the first place.

If the current behavior is by design, I suggest revisiting that design, given its highly obscure and hard-to-predict-by-the-user nature.

Could you elaborate on the scenario you are referring to? and what exactly you are suggesting to do? I would say in my experience I didn't see many people cloning the culture. Also, as pointed here, currently there is easy workaround.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

Not true, CultureInfo.Calendar represent the default calendar of the culture.

No, as the following PowerShell snippet demonstrates:

PS> ($c = [cultureinfo] 'fr-FR').DateTimeFormat.Calendar.TwoDigitYearMax = 2020; $c.Calendar.TwoDigitYearMax
2020 # $c.Calendar points to the modified $c.DateTimeFormat.Calendar instance.

That is, after modifying the (on-demand created) DateTimeFormat.Calendar instance, .Calendar points to that. It is only initially that the default calendar is referenced.

In fact, it works the other way around too: modifying a property of .Calendar implicitly populates .DateTimeFormat.Calendar too and again, both point to the same instance:

PS> ($c = [cultureinfo] 'ru-RU').Calendar.TwoDigitYearMax = 2020; $c.DateTimeFormat.Calendar.TwoDigitYearMax
2020 # $c.Calendar and $c.DateTimeFormat.Calendar are again the same instance.

If both are same, then wouldn't make sense exposing both in the first place.

I can't speak to the original design intent, but providing a shortcut is one possible motivation.

The fact is that DE FACTO they point to the same calendar instance - except when the bug surfaces.
The fact that .DateTimeFormat.Calendar doesn't initially exist shouldn't be the user's concern.

If the following invariant holds for a CultureInfo singleton or one obtained via .GetCultureInfo(), it should hold for clones too:

PS> $c = [cultureinfo] 'fr-FR'; [object]::ReferenceEquals($c.Calendar, $c.DateTimeFormat.Calendar)
True  # true whether or not you've modified $c.DateTimeFormat.Calendar

what exactly you are suggesting to do?

Fully hide the implementation details and ensure that .Calendar and .DateTimeFormat.Calendar (if the latter is accessed) always reference the same calendar instance.

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

As I mentioned, when creating DateTimeInfo from CultureInfo we use CultureInfo.Calendar as initial calendar in DateTimeInfo. in other word, both calendars are pointing to the same object. changing one will change the other. If you customize DateTimeFormatInfo.Calendar, you are disconnecting the 2 at that time.

do the following:

CultureInfo ci = new CultureInfo("ja-JP");
ci.DateTimeInfo.Calendar = new JapaneseCalendar();

Now you have totally 2 different calendars in CultureInfo and DateTimeInfo.

Fully hide the implementation details and ensure that .Calendar and .DateTimeFormat.Calendar (if the latter is accessed) always reference the same calendar instance.

This is not Correct as I pointed these are 2 calendars for different purposes and we cannot unify both all the time. CultureInfo.Calendar is always pointing to the default calendar of the culture. DateTimeInfo.Calendar is pointing to the calendar used for formatting and parsing.

What is the scenario you need cloning before accessing DateTimeFormat property and after customizing CultureInfo.Calendar properties?

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

The ci.DateTimeInfo.Calendar = new JapaneseCalendar(); scenario is technically distinct and unrelated to this bug report - we are not talking about intentionally making the two calendars diverge.

in other word, both calendars are pointing to the same object. changing one will change the other.

As highlighted in my previous comment, that's the condition that the clone violates - situationally in general, always in PowerShell - and that is what needs fixing.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

As for an example where the bug actively makes user code malfunction, due to PowerShell's unbeknownst-to-the-user access of .DateTimeFormat behind the scenes:

PS> ($c = [CultureInfo]::InvariantCulture.Clone()).Calendar.TwoDigitYearMax = 2020; [datetime]::ParseExact('21', 'yy', $c).Year  
2021 # !! WRONG: should be 1921

The reason this fails is that the .ParseExact() is based on .DateTimeFormat.Calendar, not .Calendar.

Obviously, that's not a problem if both properties point to the same instance, but - due to the bug - they don't - and, as you can see, the code didn't even have to touch DateTimeFormat.Calendar (that happens behind the scenes).

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Customizing CultureInfo.Calendar and expected to always reflect on formatting and parsing is wrong assumption in general. Formatting and Parsing always use DateTimeFormat.Calendar.

CultureInfo.Calendar is NOT for formatting or parsing. It is just telling what is the default calendar of the culture. You are mixing 2 things in one which is incorrect. We never promised customizing CultureInfo.Calendar will reflect on the formatting/parsing. And even will be wrong to do so.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

It is just telling what is the default calendar of the culture.

As amply demonstrated above, that is not true, even though it may have been the original intent.

If it were true, then changes to .DateTimeFormat.Calendar should never be reflected in .Calendar - yet they are, in almost all cases - except the corner cases that constitutes the bug at hand.

Yes, you could argue that's how it should be: let .Calendar only ever reflect the - unmodified - default calendar (in which case its properties would ideally be read-only too), and customize the calendar solely via .DateTimeFormat.Calendar (either by property modification or by assigning a new instance) - but that ship has sailed.


At the end of the day, without having looked at the source code, I suspect the problem will go away if @GSPP's implied recommendation is heeded:

Don't create a new, distinct calendar instance on getting the .DateTimeFormat property.

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Don't create a new, distinct calendar instance on getting the .DateTimeFormat property.

I am not sure I follow here. do you mean always create a distinct calendar when creating DateTimeFormat? Currently we don't create new and distinct calendar when requesting DateTimeFormat.
The only concern with that is we'll create extra object every time you create DateTimeFormat which is not really worth it to pay that cost for this corner case scenario. Also, this will not help any way for your scenario which the users want to format/parse datetime.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 11, 2019

Sorry - I shouldn't have suggested an implementation for the fix, since I'm not familiar enough with the source code, so let me bring it back to the desired behavior:

  • (Original design intent aside) currently, it's reasonable to rely on .Calendar and .DateTimeFormat.Calendar referencing the same instance by default, and that modifying the properties of .Calendar is reflected in .DateTimeFormat.Calendar and vice versa.

  • This invariant is violated - for no good reason - when .Clone() comes into play, if .DateTimeFormat happens to have been accessed previously.

This makes the behavior of .Clone() generally hard to predict, but especially so in PowerShell, where the user code itself never even needs to reference .DateTimeFormat to provoke the symptom.

Also, this will not help any way for your scenario which the users want to format/parse datetime.

If the invariant described above is made to hold in all scenarios, it's fine to always modify parsing-relevant properties via .Calendar too (assuming no new instance was deliberately assigned to .DateTimeFormat.Calendar) - and the PowerShell code shown above then won't malfunction.

I'll leave it to you and others to find a technical solution that won't hurt performance.

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@mklement0 thanks. I'll think about it and will try to find if there is a way we can do it without allocating more objects.

@mklement0

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 16, 2019

Thank you for the quick turnaround, @tarekgh.

@tarekgh

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Thanks @mklement0 for reporting and discussing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.