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

[API Proposal]: Expose GlobalizationMode.Invariant (and potentially others) publicly #81429

Open
gurustron opened this issue Jan 31, 2023 · 29 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Globalization
Milestone

Comments

@gurustron
Copy link

gurustron commented Jan 31, 2023

Background and motivation

Expose members of GlobalizationMode publicly. There is request for that:

API Proposal

namespace System.Globalization
{
    public static partial class GlobalizationMode
    {
        public static bool IsInvariant => ...;
    }
}

API Usage

var isGlobalizationInvarinat = GlobalizationMode.IsInvariant;

Alternative Designs

No response

Risks

No response

@gurustron gurustron added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 31, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 31, 2023
@ghost
Copy link

ghost commented Jan 31, 2023

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Expose members of GlobalizationMode publicly. There is request for that:

API Proposal

namespace System.Globalization
{
    public static partial class GlobalizationMode
    {
        public static bool IsInvariant => ...;
    }
}

API Usage

var isGlobalizationInvarinat = GlobalizationMode.IsInvariant;

Alternative Designs

No response

Risks

No response

Author: gurustron
Assignees: -
Labels:

api-suggestion, area-System.Globalization

Milestone: -

@gurustron gurustron changed the title [API Proposal]: Expose GlobalizationMode.Invariant (and potentially) publicly [API Proposal]: Expose GlobalizationMode.Invariant (and potentially others) publicly Jan 31, 2023
@tarekgh
Copy link
Member

tarekgh commented Jan 31, 2023

Why is reading the settings not good enough for that? I don't think this is going to change at all in the future.

       // https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs,15
       // https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs,13

        public static bool IsInvariantMode()
        {
            if (!AppContext.TryGetSwitch("System.Globalization.Invariant", out bool ret))
            {
                string? switchValue = Environment.GetEnvironmentVariable("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT");
                ret = switchValue != null ? switchValue.Equals("True", StringComparison.OrdinalIgnoreCase) || switchValue.Equals("1")) : false;
            }

            return ret;
        }

@tarekgh tarekgh added this to the Future milestone Jan 31, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 31, 2023
@tarekgh tarekgh added untriaged New issue has not been triaged by the area owner needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 31, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 31, 2023
@ghost
Copy link

ghost commented Jan 31, 2023

This issue has been marked needs-author-action and may be missing some important information.

@gurustron
Copy link
Author

gurustron commented Jan 31, 2023

@tarekgh because it requires some understanding of internal workings of the feature and leads to some inventive code even in Microsoft products as you see in the linked Microsoft.Data.SqlClient sources. And it is small but still bunch of code instead of one small concise static property access.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 31, 2023
@jeroen-mostert
Copy link

jeroen-mostert commented Jan 31, 2023

One problem is that even if this property was added today, existing libraries like Microsoft.Data.SqlClient still couldn't use it because they have to support earlier versions, so they'd basically be stuck copying this code anyway -- or relying on a separate package to roll up this code, since presumably it's backwards compatible at runtime. But that package could be written today without this proposal.

By the time libraries would be free to only use this property, secure that it's part of the core runtime, we're several years down the road, limiting its usefulness. That's not to say it's an absolute argument against, of course.

@tarekgh tarekgh removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jan 31, 2023
@tarekgh
Copy link
Member

tarekgh commented Jan 31, 2023

don't take this as push back on the proposal idea.

because it requires some understanding of internal workings of the feature

The switches are public and documented so it is not internal. But I got a related point which makes reading the config switches are not reliable enough. I'll mention that later in this reply.

And it is small but still bunch of code instead of one small concise static property access.

I do not see this compelling argument to expose API for especially the workaround is not big.

Here are some thoughts:

  • @jeroen-mostert has a good point regarding the apps/libraries need to work with the down-level versions of the .NET. They will need to include the workaround anyway.
  • I mentioned above the idea of reading the config switches. Unfortunately, this will not be reliable enough because in native compilation scenarios (AOT), it is possible the invariant mode value will get embedded inside the app and not get affected by the switches nor the environment variables. If we are going to have a workaround, it will need to be different.
  • It will be good if we understand how the libraries/apps are going to use the new API if we exposed it? The SQL issue suggests detecting the mode and still fails. more like showing more clearer failing reason.
  • The Invariant mode works in two ways, the default one is not allowing creating any culture except the Invariant culture. The other way is turning off PredefinedCulturesOnly which make the Invariant mode create any culture, but all created culture will be using the Invariant culture data. Considering that, the proposed API needs to be adjusted if we need to communicate which way is enabled in the Invariant mode.

If need to have temporary workaround for now, I suggest you do something like the following:

        public enum InvariantMode
        {
            None,
            PredefinedCultureOnly,
            AllCultures
        }

        public static InvariantMode Invariant { get; } = GetInvariantMode();

        private static InvariantMode GetInvariantMode()
        {
            try
            {
                return CultureInfo.GetCultureInfo("en-US").NumberFormat.CurrencySymbol == "¤" ? InvariantMode.AllCultures : InvariantMode.None;
            }
            catch (CultureNotFoundException)
            {
                return InvariantMode.PredefinedCultureOnly;
            }
        }

If you care only if Invariant mode is on or off, you may simplify that as

        public static bool Invariant { get; } = GetInvariantMode();

        private static bool GetInvariantMode()
        {
            try { return CultureInfo.GetCultureInfo("en-US").NumberFormat.CurrencySymbol == "¤" ; }
            catch (CultureNotFoundException) { return true;}
        }

@jkotas
Copy link
Member

jkotas commented Jan 31, 2023

The switches are public and documented so it is not internal.

The switch is not reliable way to detect invariant mode in the presence of trimming. Trimming can hardcode the app to "always invariant mode" or "never invariant mode". The switch is ignored in that case.

@jeroen-mostert
Copy link

jeroen-mostert commented Jan 31, 2023

This code may throw an exception when nothing's wrong, just to detect behavior. Such exceptions cause noise and slowdowns when debugging (even if it happens just once at startup, since it still requires developer attention every time). This is not a dealbreaker, but still annoying.

Worse, like the code applied in Microsoft.Data.SqlClient, it relies on globalization-invariant mode behaving a particular way just to detect it, in a way that's not guaranteed except by how the implementation happens to behave now (with little guarantee that it will continue to do so, since it's not clear who's taking dependencies). Microsoft.Data.SqlClient checks if the EnglishName of the culture includes Invariant, this code checks if the CurrencySymbol is ¤. This code is actually less reliable, in that a Windows end user can change their en-US regional settings to exactly that symbol. This isn't hypothetical either -- my machine uses exactly this setup, and not specifically to break this code. :) This check returns true on my machine even outside globalization-invariant mode. Of course some other property can be tested, but you need to be really sure it will only hold true for globalization-invariant mode, and not just accidentally but because the implementation requires it. That's a hard ask.

@tarekgh
Copy link
Member

tarekgh commented Jan 31, 2023

this code checks if the CurrencySymbol is ¤. This code is actually less reliable, in that a Windows end user can change their en-US

Using CultureInfo.GetCultureInfo("en-US") will protect against this case. I'll modify the code I included before.

@tarekgh
Copy link
Member

tarekgh commented Jan 31, 2023

The switch is not reliable way to detect invariant mode in the presence of trimming. Trimming can hardcode the app to "always invariant mode" or "never invariant mode". The switch is ignored in that case.

That is what I was trying to say in the second bullet in my reply.

@KGuetter
Copy link

KGuetter commented Feb 1, 2023

catch (CultureNotFoundException) { return false;}

Shouldn't this be return true?

@tarekgh
Copy link
Member

tarekgh commented Feb 1, 2023

Shouldn't this be return true?

You are right. I fixed it in the code.

@jeroen-mostert
Copy link

Is there any possibility that the en-US culture is not available in a situation where globalization-invariant mode is not enabled, either in .NET Framework (which doesn't have GIM, of course) or .NET Core, for example, because it was excluded or disabled somehow?

@tarekgh
Copy link
Member

tarekgh commented Feb 2, 2023

Is there any possibility that the en-US culture is not available in a situation where globalization-invariant mode is not enabled, either in .NET Framework (which doesn't have GIM, of course) or .NET Core, for example, because it was excluded or disabled somehow?

Currently en-US exist all the time and never get disabled.

@jkotas
Copy link
Member

jkotas commented Feb 2, 2023

Currently en-US exist all the time and never get disabled.

Is that valid statement even for configurations with custom ICU builds? IIRC, custom ICU builds can be trimmed down in number of ways.

@tarekgh
Copy link
Member

tarekgh commented Feb 2, 2023

Is that valid statement even for configurations with custom ICU builds? IIRC, custom ICU builds can be trimmed down in number of ways.

Are you referring to the buildings for WASM and mobile apps? en-US exist there (at least for resource fallback lookups).

CC @akoeplinger @ilonatommy if they add more details as needed.

@gurustron
Copy link
Author

gurustron commented Feb 2, 2023

@tarekgh Here is another example were this API can be useful - Humanizr/Humanizer#1213 instead of unnecessary exception management.

@tarekgh
Copy link
Member

tarekgh commented Feb 2, 2023

@gurustron I am not pushing back having API for that :-) what I have provided before just a workaround to use till we have the API. That is why I am keeping the issue open for tracking.

@gurustron
Copy link
Author

@tarekgh great, thank you!

@ilonatommy
Copy link
Member

Is that valid statement even for configurations with custom ICU builds? IIRC, custom ICU builds can be trimmed down in number of ways.

Are you referring to the buildings for WASM and mobile apps? en-US exist there (at least for resource fallback lookups).

CC @akoeplinger @ilonatommy if they add more details as needed.

That is true but actually we are thinking also about introducing a "real" custom ICU option for WASM where loading ICU bundles that contain a custom set of locales for specific app (nothing prevents from creating such ICU without en-US) will be possible. It is motivated by the fact that some customers require locales that are not present in the default ICU for WASM and we don't want to increase the size of the default ICU bundle.
In that situation check for InvariantMode relying on presence of "en-US" will fail for sure.

@jeroen-mostert
Copy link

jeroen-mostert commented Feb 3, 2023

I guess it ultimately comes down to why a library is trying to detect GIM and what happens if it doesn't. For example, for something like Microsoft.Data.SqlClient, it would probably have no problem to have the detection contingent on en-US, because it needs the client-side cultures to support the server-side collations. In theory you could trim locales corresponding to collations you actually use on the server (and nothing else), but M.D.S would need more code to support that scenario cleanly (if it's deemed worth it at all), so for now it just wants to bail out with a clear message if GIM is in use, as the failure is otherwise much more cryptic and not easily tied to GIM.

For a library like Humanizer, the problem specifically only occurred because it used en-US as a default, and there's a pull to make it fall back to CultureInfo.InvariantCulture if that's not available. In this case the only real need for detecting GIM is to avoid an unnecessary exception, which is valuable but minor. Arguably the proper fix here would be to make an API available for trying to get a culture -- the CultureNotFoundException is an example of what has now, with more variance in whether cultures are available or not, become a vexing exception (it used to be worse pre-.NET 4, as it was an ArgumentException, which is of course not something you should normally catch -- the first sign that something was wrong).

CultureInfo.TryGetCultureInfo (and overloads) is worth considering. The constructor of CultureInfo would still throw; the alternative there if you need a customizable culture is to check if it's available first with TryGetCultureInfo before instantiating. Libraries that need a specific culture (whether en-US for a default or anything else) would then be able to support this without having to handle exceptions (the Humanizer example would become new DefaultFormatter(CultureInfo.TryGetCultureInfo("en-US", out var usEnglish) ? usEnglish : CultureInfo.InvariantCulture), while libraries like M.D.S which don't want to put in the effort to support mix and match locales (for now) could use bool invariantMode = !CultureInfo.TryGetCultureInfo("en-US", out var usEnglish) || usEnglish.NumberFormat.CurrencySymbol == "¤" (assuming checking the CurrencySymbol is the most convenient/stable way of doing it), although realistically it would not start using this because of the downlevel requirement.

Bottom line is that detecting GIM as a flag should be unusual because what clients ought to be doing is checking for support for the cultures they need -- but they currently can't do this without handling exceptions or by inefficiently enumerating CultureInfo.GetCultures(...) (and then they still have to pick a flag).

@akoeplinger
Copy link
Member

In my opinion given all the edge cases that were already listed pushing the checking code onto the end user doesn't seem viable to me. I like the TryGetCultureInfo proposal on first glance.

@KGuetter
Copy link

KGuetter commented Feb 3, 2023

In our case it is that we want to skip a rather expensive process (involving loading translations) that is definetely not useful with GIM.

@jeroen-mostert
Copy link

jeroen-mostert commented Feb 3, 2023

@KGuetter: but which translations? If the user has enabled a customized set of locales, would it be beneficial to only load those? In other words, might you be better off by checking what's in CultureInfo.GetCultures vs. available translations (by name or LCID or whatever's appropriate), rather than the broad switch for GIM? In the special case of this returning only a single culture/language (which in the case of GIM is the invariant culture) translations can be disabled entirely, if this speeds up anything beyond only arranging for the needed ones.

@KGuetter
Copy link

KGuetter commented Feb 3, 2023

@jeroen-mostert What you suggest might indeed also be a workaround: Check if CultureInfo.GetCultures(CultureTypes.NeutralCultures) contains only the invariant culture.

@jeroen-mostert
Copy link

For the scenario where you may proficiently use the cultures for limiting translations anyway, yes. Not necessarily universally, because I imagine getting all the cultures (even if just to check there's only one) might have considerable overhead in other scenarios (I haven't actually benchmarked it). It'll almost certainly be slower than just checking if there's no en-US or a property of the invariant culture (though maybe not once exception handling gets thrown in. :P

@tarekgh
Copy link
Member

tarekgh commented Feb 3, 2023

@ilonatommy

In that situation check for InvariantMode relying on presence of "en-US" will fail for sure.

If you are going to support that, you need to think about the resource fallback handling. Currently all .NET libraries have [assembly: NeutralResourcesLanguage("en-US")] which make en-US is a required existing culture (except in Invariant mode as we are restricting everything anyway).

@akoeplinger

I like the TryGetCultureInfo proposal on first glance.

This comes everywhile and I agree it is worth considering this API too.

@jeroen-mostert

I guess it ultimately comes down to why a library is trying to detect GIM and what happens if it doesn't.

I fully agree with that, that is what I tried to point at in the third bullet of my reply #81429 (comment).

but they currently can't do this without handling exceptions or by inefficiently enumerating CultureInfo.GetCultures(...) (and then they still have to pick a flag).

Enumeration is not enough here either because there are aliased cultures which are not returned by the enumeration but still valid. But, in general I agree checking for GIM should be unusual and needed only in specific situations.

In general, I can see some libraries will not work properly (or will not work at all) in GIM, but this is part of the GIM contract that apps who enable GIM understand they will get some restrictions.

@jeroen-mostert
Copy link

jeroen-mostert commented Feb 3, 2023

@tarekgh:

this is part of the GIM contract that apps who enable GIM understand they will get some restrictions

The problem there is twofold:

  • End users may not be clear they have actually enabled GIM (they "just picked alpine for my Docker image because it's small!")
  • If a library fails due to GIM, this may be somewhere in the bowels where a "culture not found" error or its fallout are not easily traced back to GIM in a causal or functional sense (as in the example of Microsoft.Data.SqlClient needing cultures to support collations -- most users don't even think of collations).

For the scenarios that this proposal was inspired by, it's not a simple case of "well you asked for it, you got it, what did you expect?"

@tarekgh
Copy link
Member

tarekgh commented Feb 3, 2023

Thinking a little more, I think the best solution here is like what @jeroen-mostert mentioned before with a little modification. We can expose CultureInfo.TryGetCultureInfo(string cultureName, bool predefinedCulturesOnly, out CultureInfo culture). note predefinedCulturesOnly flag.
if passing predefinedCulturesOnly=true will always make CultureInfo.TryGetCultureInfo return false (except with Invariant culture) in GIM regardless of predefined-cultures environmental settings.

Doing that will give full flexibility to the libraries and not need to check any culture property like currency symbol. Also, this API can be super useful even for non-GIM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Globalization
Projects
None yet
Development

No branches or pull requests

7 participants