Enable nullable annotations in basic types in Scripting

[BCSharp]

Activates creation of polyfills NotNull and MemberNotNullWhen.

[slozier]

Thanks!

[slozier]

I was looking at this since it would technically be a change in the public API, but I think it's fine. The whole PAL concept is such a mess, whether we use it or not seems arbitrary (at least in IronPython). Looking at the null vs empty usage in IronPython I think we probably have a few bugs...

[BCSharp]

I had looked into this, before I decided on the publicly visible nullabiltiy of the method's signature, and it has nothing to do with the messiness of the PAL usage. I am going to write down my findings here, so that we have some record for our future selves and others that may be wondering what happened and why. For an executive summary, aka bottom line aka tl;dr, look to the bottom of this post. If you keep reading, grab a coffee first…

This is PAL, which purpose it to provide the platform functionality in a platform-independent way. Maybe in the past (Silverlight, Unity) it had some important value, but now it practically routes everything through .NET (which is the true PAL now), possibly changing the signatures, like in the case of GetEnvironmentVariables(). It changes the return type from an IDictionary to a more specific and convenient to use Dictionary<string, string> (I would make it an interface, but it is another story; it's public now). The conversion converts from key/value types object/object? to string/string, so there is downcasting involved, as well as nullability change of the value type. So although interface IDictionary can technically, by the virtue of its typing, hold references to any object, reading the documentation of GetEnvironmentVariables() we learn that all keys and values are strings. So the downcasting is safe. As for nullability, the keys in IDictionary must not be null, so the new method signature is correct on that point. The question is the nullability of value. In practice, on all currently supported OSes by the DLR (Windows, macOS, Linux), the OS calls will never return a null value from the environment, by virtue of processing of the environment block. The worst case is an empty string, when a variable exists but is not set to anything. This is still not null. So the signature is correct.

However, the method is virtual, so one can claim it is "breaking" the API. My position is that it is not breaking the API, because we are going from nullability-agnostic to nullability-enabled. The API never made any nullability claims of the signature. As long as a subclass remains nullability-agnostic, it keeps working, even if it returns dictionaries with null values, which, by the way, is wrong, but this rule was not enforced. If the subclass becomes nullability-aware, well, then it has to conform to the nullability-aware PAL API.

The next question is: is it even possible for some sort of GetEnvironmentVariables() in some environment to have null values? It appears that yes, possibly, when reading from a filesystem provider for Windows registry (e.g. Environment.GetEnvironmentVariables(EnvironmentVariableTarget.User) and keys of type REG_NONE). It uses RegistryKey.GetValue(name, ""), where the second argument is the default value in case the key is declared but the value is missing. Note that it is an empty string, and not null; this is important because it shows the intent: values are never null.

The next step in the process is to convert that received registry value to a string, and the code uses Object.ToString(), which has an unfortunate signature of returning string?. My personal opinion is that this is a mistake, and there was a heated discussion online between the .NET developers themselves, plus public opinion, about making the correct choice, when the nullable attributes were introduced. I am not going to summarize here how it went (wrong), but the status quo is that the return value is annotated as nullable (for the sake of API compatibilty, since it is virtual, just as our GetEnvironmentVariables, so the same arguments and counter-arguments), and at the same time the documentation recommends (requires) ToString to never return null. Which is a missed opportunity for the sake of API stability, because that was exactly the point of the presence/absence of ?. So in the end, we have a function call that may return null, but the docs say it should not, though we never get proper compiler support to enforce this rule. In practice, for the existing types of the registry, this will never happen (Microsoft adheres to its own rules here), but null coalescing to an empty string in PAL enforces this in a non-exception-throwing manner, in some weird or future scenarios.

Bottom Line

1. The intention from PAL is to (among other things) provide a convenient interface change from weakly typed IDictionary to strongly typed IDictionary<string,string>.
2. This dictionary should not contain null values, but if a subclass does it anyway, it is still OK (null-forgiving operator, nullability-agnostic). The nullability annotation on string does not introduce a different type.