Runtime: Bug in CultureInfo.Clone() causes .Calendar and .DateTimeFormat.Calendar to diverge

[mklement0]

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 https://github.com/PowerShell/PowerShell/issues/10438#issuecomment-529628273:

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]

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

[mklement0]

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

[scalablecory]

Thanks for the great repro!

[tarekgh]

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.

[mklement0]

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]

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]

Reopening to continue discussion above.

[tarekgh]

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]

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]

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:

C# 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]

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]

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]

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]

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]

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]

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]

@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]

Thank you for the quick turnaround, @tarekgh.

[tarekgh]

Thanks @mklement0 for reporting and discussing this issue.