Nullable: NumberFormatInfo, CultureInfo

[krwq]

No description provided.

[stephentoub]

This is an interesting case, @jaredpar. In general nullability analysis explicitly ignores threading, e.g. a property that's been validated to be non-null will suppress the compiler warning:

if (obj.SomeProp != null) obj.SomeProp.SomeMethod(); // no warning

even though the second call to SomeProp could return null (either due to some other thread changing things, or just more simply the property not being particularly well-behaved).

This feels like a similar case, but at the same time, if you're using CompareExchange, there be dragons. It seems like the net result here will be that lots of cases involving CompareExchange, in particular for lazy initialization, will also require using the null-forgiveness operator.

[jaredpar]

This was a deliberate design trade off the nullable feature made. The explicit assumption was made that properties values will be non-null or null across various invocations. Had a healthy debate about this but eventually came down on this side.

[stephentoub]

Right, I understand the decision about properties. My point (which I didn't make particularly well) is that this case is similar: the line right above it is initializing the field, and then we read from the field. Following the same logic as was used with properties, you could see an argument being made for the compiler treating it as initialized here as well, assuming GetUserDefaultUICulture is typed to return non-null.

[jaredpar]

Ah I see. How are you annotating CompareExachange first parameter here: nullable or nonnullableL

[stephentoub]

How are you annotating CompareExachange first parameter here

How are you allowing generics to be annotated? :-)
https://source.dot.net/#System.Private.CoreLib/src/System/Threading/Interlocked.cs,105

Regardless, you're absolutely able to pass null for either/both the second or third arguments.

[stephentoub]

We're not doing any argument validation on the input name. Seems like we should be. If you pass in null, this method will end up throwing NullReferenceException instead of ArgumentNullException.

@safern, @danmosemsft, can we keep a log somewhere of issues found from doing this annotation effort? Ideally, categorized. e.g. methods that don't expect null and fail with the wrong exception type if you pass null, methods that should be handling null but aren't correctly, etc.? I think the results of that will be helpful as we look back on this effort and figure out where to take it next.

[krwq]

also - presumably - should we be adding checks as we annote it or leave it and assume in the future more people will opt-into this feature and it just won't compile for them? (no personal preference either way but since we chose compile errors I think it makes sense not to do any checks)

[safern]

@safern, @danmosemsft, can we keep a log somewhere of issues found from doing this annotation effort? Ideally, categorized. e.g. methods that don't expect null and fail with the wrong exception type if you pass null, methods that should be handling null but aren't correctly, etc.? I think the results of that will be helpful as we look back on this effort and figure out where to take it next.

Would an issue work fine?

[krwq]

Created one. https://github.com/dotnet/corefx/issues/36384

[krwq]

let's just reference it without any notes on the PRs unfortunately we lose info about API this way - we need to edit the issue

[stephentoub]

GitHub won't let me comment on it, but I think a bunch of the virtuals above are wrong, in that there's nothing preventing a derived type from returning null, e.g. this compiles fine and prints out true:

using System;
using System.Globalization;

    class Program
    {
        static void Main() => Console.WriteLine(new MyCultureInfo().DisplayName == null);
    }

class MyCultureInfo : CultureInfo
{
    public MyCultureInfo() : base("en-US") { }
    public override string DisplayName => null;
}

@MadsTorgersen, @terrajobst, how should we be thinking about this? If the annotations are meant to reflect what's possible, then most of these should return nullable. If the annotations are meant to reflect what's intended, then most of these should return non-nullable, and derived types that violate that would need to be fixed.

cc: @danmosemsft

[krwq]

I've not fixed this yet until we decide on guidelines - I personally lean toward what makes sense vs what is possible - so i.e. would not expect CompareInfo and other similar to be null so IMO should be non-nullable but technically you could do what you described above.

[MadsTorgersen]

I feel strongly that we should annotate intent. That's what the feature is for. It puts the onus on misbehaving implementations, not innocent users.