Nullable: System.Object

[safern]

This one is a simple one.

Edit

[stephentoub]

Should ToString return string?? This gets back to the question of how we want to handle base virtuals. I guarantee you that there are ToString overrides that return null (I'm pretty sure we even have a few in coreclr/corefx), even though design guidelines say you shouldn't do that.
cc: @terrajobst, @MadsTorgersen

[safern]

For other types we haven’t annotated the return type of ToString as nullable. However this depend on what we want to do. I guess if the design guidelines say you shouldn’t return null wouldn’t this be a good reason to leave it as non-nullable?

[stephentoub]

I guess if the design guidelines say you shouldn’t return null wouldn’t this be a good reason to leave it as non-nullable?

Except then if something did return null, a caller that does ToString().Length won't get a warning and will null ref.

[stephentoub]

For other types we haven’t annotated the return type of ToString as nullable.

@jaredpar can correct me if I'm wrong, but I believe we allow for overrides to be more constrained than a base, e.g. an override could be string with the base being string?.

[safern]

I'll wait for @jaredpar to confirm, if overrides can be more contrained, then it makes sense to make Object.ToString() return string?

[jaredpar]

@gafter

[tannergooding]

I would have expected that, if not matching was allowed, then narrowing (going from nullable to non-nullable) works, without a warning and widening (going from non-nullable to nullable) doesn't work (because virtuals and that means the base could be "lying").

• This is, of course, assuming both types in the hierarchy have opted-into nullable, since things all go out the window otherwise

[jaredpar]

The intent is to allow for parameters to allow widening on overrides, and narrowing on returns. There is some work in the compiler left to implement this completely but I thought that was in the interface case only.

[tannergooding]

For reference, here is sharplab showing both cases using last night's master bits:
https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEAMACFBGA3AWACgiBiAOwFcAbKiYKuDOMuhoolAZiwCYMBhDAG8iGMVm4A3AJYIYFCFSzY0AfgwBZABQBKYaPGGUAdgyUaBQoYC+BsXYkYZchUpyYAcrv1XD4kxgARIGWNkS2xIRcvBgAIhggAj5G3AD2kogI0gAmjO6a3iK+flimwaHiEYYO0emZOXkq6l56RSViAeZUFWIR1kA=

[jcouv]

@tannergooding We've not implemented the narrowing/widening rules in overrides yet.
Tracked by dotnet/roslyn#23268

[stephentoub]

One comment/question, otherwise LGTM.

[safern]

Merging without marking ToString return type to be nullable per @terrajobst comment which makes sense:

Hmm. Do believe returning null is common or desirable? If we believe it's not I think we should mark it non-nullable.

[stephentoub]

Hmm. Do believe returning null is common or desirable? If we believe it's not I think we should mark it non-nullable.

I disagree. If something consistently returns null, you're going to catch it immediately in tests. If something only does sometimes, this feature helps alert you to your mistake when you dereference it. If we make Object.ToString non-nullable despite it easily returning null, I do not understand the benefit of this feature. We'll actually be making consuming code more likely to null ref, because developers will see it's non-null and won't guard.

[safern]

If we make Object.ToString non-nullable despite it easily returning null, I do not understand the benefit of this feature. We'll actually be making consuming code more likely to null ref, because developers will see it's non-null and won't guard.

Your concern is for APIs that haven't yet been annotated and could potentially return null? Because when we annotated all of the ToString overrides, if Object.ToString() is not-nullable, then the overrides shouldn't be able to return null, right?

[stephentoub]

Your concern is for APIs that haven't yet been annotated

No, it doesn't matter if they're annotated. A developer using the base class only sees the base class annotation. The base class says "this can't be null", and yet it can be, easily.

[krwq]

@stephentoub you can also workaround any non-nullable by setting it to null by reflection or native code - I'd rather treat it as hint and what the expectation should be - if someone messes it up on purpose that's rather unexpected and perhaps good that we get some kind of unexpected error like NRE or something else

[stephentoub]

This isn't about unsafe code or private reflection or trying to mess things up on purpose. ToString has existed for two decades. Implementations return null. Period.

[krwq]

@terrajobst thoughts? I'm personally slightly toward this being non-nullable but not very strongly (I cannot see a good reason to return null from this API but from practical point of view this might (possibly will?) fall over)

[tannergooding]

I agree that it doesn't make sense for ToString() to return null and maybe if we were designing this from day 1, we could say that. The trouble is, we are retrofitting nullable on 20 years worth of API design much of it where null was considered for parameters; but not necessarily for return types (or virtuals or other places that matters).

I think the best scenario is for us to say this (the return of ToString()) can be nullable and, from the comments above from @jaredpar, @gafter, it sounds like narrowing the constraint on an override should be fine (maybe we just don't have the correct compiler bits yet) for users who want/can restrict this more.

I don't think we should be in the business of breaking existing code that "just works" for people. Instead, we are only providing the metadata so that consumers can opt-into turning their code that would 100% fail at runtime into code that fails at compile time.

[terrajobst]

Hmm. Do believe returning null is common or desirable? If we believe it's not I think we should mark it non-nullable.

I disagree. If something consistently returns null, you're going to catch it immediately in tests. If something only does sometimes, this feature helps alert you to your mistake when you dereference it. If we make Object.ToString non-nullable despite it easily returning null, I do not understand the benefit of this feature. We'll actually be making consuming code more likely to null ref, because developers will see it's non-null and won't guard.

Wouldn't a person overriding ToString() now get a warning when returning null? So the feature would push more ecosystem libraries and code to stop returning null as virtually no code is guarding against ToString() returning null. I think that's much better than making everyone pay for the mistake of a few by guarding all their calls now.

I think the best scenario is for us to say this (the return of ToString()) can be nullable

I disagree because there are many many call sites where the consuming code will be calling ToString() through a reference typed as object. Making all these people guard their calls because someone might have returned null is the wrong direction IMHO. Rather, I want the folks returning null to get a warning. For starters, because that's much fewer people but also because that's the code that is wrong in the first place.

I'm all for pragmatic choices but we also we need to consider the impact here: if we create too many false positives the feature also loses value.

[stephentoub]

Wouldn't a person overriding ToString() now get a warning when returning null?

Only if they opt-in to enabling nullability warnings. There's potentially multiple parties involved here:

• Someone consuming object.ToString
• Someone else producing an object that overrides ToString

The former can have warnings enabled and the latter not. Which with the feature being opt-in seems pretty likely.

So the feature would push more ecosystem libraries and code to stop returning null as virtually no code is guarding against ToString() returning null

For new libraries, I agree. For existing libraries, I'm not sure the outcome would be to change the implementation to stop returning null; I would not be surprised if the majority of devs that get that warning would have a first reaction of just using ! to silence it, since the code technically isn't doing anything wrong.

I think that's much better than making everyone pay for the mistake of a few by guarding all their calls now.

I walk up to an arbitrary object and call its ToString. That can return null. But if it self-describes itself as never returning null, then developers won't guard access to it even if they previously may have... after all, the API is telling them it'll never return null. Seems to me it'll actually result in more NullReferenceExceptions in the ecosystem rather than fewer.

[terrajobst]

Taking your argument to its logical conclusion it would mean that all virtual methods in the framework that return reference types must be annotated as nullable then. I assert that this isn't useful.

[stephentoub]

. I assert that this isn't useful.

Neither is the feature if it outright lies and simply can't be trusted. I honestly don't know what the benefit of the feature is if a method like ToString that can and does return null says that it can't.

[tannergooding]

it would mean that all virtual methods in the framework that return reference types must be annotated as nullable then

I think this largely depends.

We likely have a number of methods where the contract of the API is indicative that the return type must not be null.

We also likely have a number of APIs where the API is clear that the return type can be null.

Unfortunately, I think we also have a category of APIs where the ideal would be non-null, but the documentation is ambiguous. object.ToString is one of those cases.

Perhaps the ideal scenario would be to collect these up and do a special API review to determine their nullability contract publicly and see if anyone has any strong objections to various things. It probably shouldn't be too hard to look at a number of popular open source projects and determine if any of them return null for a given ToString override (either via IL reflection or a source code analyzer).

In any case, we need to try and ensure nullability is being considered in API reviews going forward and maybe we should look at requiring API proposals to include this information moving forward.

[dsyme]

@dsyme how will leaving ToString as nullable impact F# in terms of eg %O inside of string formatters?

It wouldn't affect %O or %A or string but would affect f (x.ToString()) where f accepts string - you'd get a warning there.

[poke]

as virtually no code is guarding against ToString() returning null

This is true. I don’t think I have ever seen code that checked the return value of ToString() for null. If some implementations do return null, then that is likely to be caught only in tests, at which point you may end up with an explicit check for that particular special case.

This feature works in the same way really: It does not give you ultimate safety, that just doesn’t work (for reasons shown by @tannergooding above for example). But it will give you a sane idea on how things should behave, so you can work with that.

If ToString() were to officially return a nullable string, I don’t think I would see change in behavior when working with ToString(). People likely won’t start to check for null because of that. It’s a lot more likely that they get annoyed by all those warnings and either force it to non-nullable explicitly instead, or turning off the feature altogether (which would be very unfortunate).

While I understand that this should reflect the reality, the reality is also that ToString() is consumed a lot more than it is written, so moving away from the common expectation (that it does not return null) will create a lot of noise to consumers that will make this feature in general more difficult to accept.

[Lexcess]

@stephentoub that's fair, I had misremembered that null handling was only for the string[] with params. Still we certainly have other places or wrappers for Format() where I would expect us to be non-nullable even if the BCL isn't. Certainly the main point remains, that you really don't see guard code around ToString (or at least it is pretty rare).

[mcetkovsky]

Docs clearly state ToString should not return null:
Your ToString() override should not return Empty or a null string.

And yet the example in the docs is capable of returning Empty:
public override string ToString()
{
string retVal = string.Empty;
foreach (T item in this) {
if (string.IsNullOrEmpty(retVal))
retVal += item.ToString();
else
retVal += string.Format(", {0}", item);
}
return retVal;
}
Even the docs can't follow its own guidance. And, to be clear, it is guidance, not a mandate. Guidance isn't adhered to all over the place, sometimes accidentally, sometimes on purpose; doesn't mean it's always a bug.

I do not feel the "should not return Empty" should be as forbidden as null as there are many common cases where an empty string is the best return value, for example string.Empty.ToString() or in some cases also for (new string[] {}).ToString().

[cartermp]

@isaacabraham We will have non-nullability assertions through a function.

@stephentoub We should not consider the use of ! to be anything other than the feature failing someone. A metric of success is how little one needs to use it.

I don't have a say in CoreFX but given that docs and conventional use of ToString() assume non-nullable aside from a super edge case, I expect we'll be walking this one back when people start using the feature.

[stephentoub]

We should not consider the use of ! to be anything other than the feature failing someone.

If that's the bar, the feature is going to be failing lots of people.

[jbtule]

I'm talking about our framework code, not arbitrary code in the wild. If we have code where we say we'll never return X and then we return X, that's a bug to be fixed like any other; it has been, and it will continue to be. We can't make such statements about virtuals where we don't control the whole hierarchy.

I stand by what I said. It will not be 100% even in your ms framework code non-virtual methods, unless you are sure that no code path returns a reference that could be publicly provided, because then it's legit possible for something in your framework annotated nonnull to return a null, since not every in the wild framework, that your opted-in-consumer is going to use will be opted-in, and that wild framework could be interacting with your framework classes.

The expectation just won't be 100%, but 99.8% isn't bad, has worked for F#.

[stephentoub]

@jbtule, my point is that when we own all of the code involved, we can have a goal of 100%, and anything short of that is a bug. If we don't own all the code involved, we can't have a goal of 100%.

[nguerrera]

I went to sleep firmly in the camp that feels object.ToString() should not return nullable, but this morning I see things with more nuance.

I am still certain that we should not make all existing overridable returns nullable. IMHO, that can only lead to low feature adoption and code that is overly upset! or fearful?. But I will say that a major difference between object and other abstractions is that object is literally anything, and that does make a case for calling through it with caution. At the other extreme are methods that are technically overridable by arbitrary implementation, but are almost never anything but code we control. For example, Type.FullName. And there's a lot of stuff in between.

Looking at the call sites to object.ToString() in the framework is interesting:
https://source.dot.net/#System.Private.CoreLib/shared/System/Object.cs,ff31a6bf27c58f89,references

1. A lot of the usage will just proceed on to Concat, Format, WriteLine and co where string? is fine.
2. The BCL is inconsistent about how it handles object.ToString() returning null.

Here is String.Concat being fully defensive:

coreclr/src/System.Private.CoreLib/shared/System/String.Manipulation.cs

Line 111 in dc0d072

string toString = value?.ToString() ?? string.Empty; // We need to handle both the cases when value or value.ToString() is null

And here is TypeConverter treating null object as converting to empty string, but otherwise letting a null from object.ToString() out.

https://github.com/dotnet/corefx/blob/c141b8c7aed44817baad190d35d0f29408eb3f7a/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeConverter.cs#L121-L133

[jbtule]

@stephentoub My point is owning 100% of the code in something with enough public exposed properties as corefx, is not a guarantee to owning 100% of the references. And while it is easier to think about breaking of the non nullable contract by third parties with virtual methods, it's not crazy to think that non-virtual methods contract could be inadvertanly broken by some third party dependency to the surprise of the end consumer (think dependency injection, autofac). So then corefx virtuals and non-virtuals aren't so different in respect to the nullablity contract and it really should be about intent.

[stephentoub]

is not a guarantee to owning 100% of the references

It doesn't matter if we "own" 100% of the references. If we're handed an input that has any chance of being null and we might return it out of something we've said is never null, then we better be validating that reference... if we're not, that's a bug.

[jbtule]

It doesn't matter if we "own" 100% of the references. If we're handed an input that has any chance of being null and we might return it out of something we've said is never null, then we better be validating that reference... if we're not, that's a bug.

🤔 , you may have convinced me that corefx specifically, needs all virtuals and ( it probably just a given on interfaces) to return nullable refs.

[tannergooding]

we might return it out of something we've said is never null, then we better be validating that reference... if we're not, that's a bug.

If this is the case and we say the bug is on our signature, then all public/protected virtuals must have nullable returns and we live in a very sad world where the feature loses an enormous amount of value.

[isaacabraham]

@stephentoub

If that's the bar, the feature is going to be failing lots of people.

Well... what's the point of this feature if ! if going to be used everywhere? Trust will rapidly evaporate and people will end up null checking anyway. Or getting null refs at runtime and blaming the language. Worse yet, that runs the risk of a real split in C#, with some people using it and some not.

@cartermp

We will have non-nullability assertions through a function.

So at runtime then.

[stephentoub]

If this is the case and we say the bug is on our signature

I'm not sure how you jumped to the "is on our signature" part. If we believe that the right answer is for the return type to be non-nullable, then the bug isn't in the signature, but rather the implementation, and we should have been validating the input and either throwing or producing some non-null result.

I fully expect that we will have some signature bugs in this first go-around. But hopefully with the number of eyes on all of this, the number of those will be minimal, and the majority of bugs that might still exist would actually be in implementation.

then all public/protected virtuals must have nullable returns

I think that should be the default position we take, that the return type of abstract/virtual methods where we don't control the complete hierarchy should be nullable. I agree with @nguerrera that in cases where the likelihood of someone other than us deriving from that type is rare, we could reconsider, e.g. with System.Type. But everyone derives from object.

what's the point of this feature if ! if going to be used everywhere? Trust will rapidly evaporate and people will end up null checking anyway.

What's the point of the feature if an API says it can never return null and then does? "Trust will rapidly evaporate and people will end up null checking anyway."

[tannergooding]

I'm not sure how you jumped to the "is on our signature" part.

If we say something is non-nullable and an overrider returns null; then the bug is either in the overrider (and they should fix it) or it is in how we declared the nullablility in our signature (and we should fix it).

Perhaps an ideal solution here would be for:

• We say return types for public/protected abstract and virtual signatures are nullable by default
  • Anyone can override and "lie"
  • Not everyone will be implementing nullable
• We get narrowing the signature on override to not give a warning and for sealed signatures (structs, sealed methods, or sealed classes) we can narrow the signature to be non-nullable
  • This allows us to give "better" semantics for the more explicit implementations where we can guarantee that it won't return null
  • It leaves it open for individual implementors to declare their APIs should never return null if they derive from some other derived type
  • This already works today; as does widening. But both give a compiler warning. It seems like narrowing (going from nullable to non-nullable) the signature would be fine to not give a warning; but widening (going from non-nullable to nullable) still should give a warning

[stephentoub]

If we say something is non-nullable and an overrider returns null; then the bug is either in the overrider (and they should fix it) or it is in how we declared the nullablility in our signature (and we should fix it).

Which is exactly why we need to be super cautious about making such statements in the first place, and the default should be as I outlined.

We get narrowing the signature on override to not give a warning

My understanding is it already shouldn't be warning to return T from an override when the base returns T?. If it does today, I believe that's a compiler bug. @jaredpar can correct me if I'm wrong.

[roji]

@stephentoub is it worth listing which types exactly are currently returning nulls from ToString() so as to better inform this conversation? Or do you have some prominent/problematic already in mind?

Another option is of course to fix those implementations (or some of them) to not return null. That would be a breaking change but it may be worth exploring.

Finally, as I wrote above, unless I'm mistaken there's nothing stopping us from annotating nullable strings for types which return null while keeping it non-nullable for the rest (including object) - this only implies suppressing a warning for those problematic types.

[stephentoub]

is it worth listing which types exactly are currently returning nulls

Obviously I can't do that for the vast majority of types in the world. For coreclr/corefx specifically I've not audited the hundreds of ToString overrides. On a code reviewer earlier in the week for something unrelated I did happen to notice one, though:
https://github.com/dotnet/corefx/blob/864bc8a12dfcd561f79820641f60cb96d6c81512/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs#L210
I'd be very surprised if there aren't more.

there's nothing stopping us from annotating nullable strings for types which return null while keeping it non-nullable for the rest (including object)

You can do the opposite: have the base return string? and have derived types return string. But if the base returns string, and derived types are typed to return string?, any accesses through the base type (which is a very common use case) are going to get the wrong answer.

[safern]

My understanding is it already shouldn't be warning to return T from an override when the base returns T?. If it does today, I believe that's a compiler bug. @jaredpar can correct me if I'm wrong.

dotnet/roslyn#23268

[jbtule]

So is DiagnosticListener.Name intended to be nullable or not? The default value is "DiagnosticListener.DefaultListener", so maybe not?

If not and it needs a runtime check on the constructor, having ToString() be non-nullable would've alerted this bug. 🤔
https://github.com/dotnet/corefx/blob/8b3446f4e8ad1a7b19c7298e20396629c6e0092e/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs#L134

[stephentoub]

So is DiagnosticListener.Name intended to be nullable or not?

No idea. But it can be.

To be clear, for new APIs introduced after the nullability feature exists, I think it's ok to use the feature as a way to document new guarantees a virtual method intends, and then it's definitively a bug if an implementation violates that. But we missed the boat there for existing stuff.

With object being the type everyone derives from, and it having a virtual ToString that lots and lots and lots of types override, and with tons of such implementations already existing, and with an inability for us to say "this is an implementation bug we can fix" (since we control only a very small portion), and with it never having been explicitly documented "this must never return null" (only guidance that's violated all over the place), I just don't see how we can make ToString return string. Maybe I'm blowing it out of proportion, but to me it throws into question everything this feature exists for.

Just quickly browsing around some of the repos I happen to already have cloned locally, here's an example from Roslyn:
https://github.com/dotnet/roslyn/blob/51af99e253c21e268518feb6a1fd4e123240fd70/src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayPart.cs#L60
and from NuGet:
https://github.com/NuGet/NuGet.Client/blob/3803820961f4d61c06d07b179dab1d0439ec0d91/src/NuGet.Clients/NuGet.PackageManagement.UI/Models/PackageDependencyMetadata.cs#L35
and from ASP.NET Core:
https://github.com/aspnet/AspNetCore/blob/f6130e8430d16f614c9ad7e5363e465804c2dd34/src/Identity/Extensions.Stores/src/IdentityRole.cs#L83
and so on...

This is not rare.

[roji]

@stephentoub

I'm far from an expert on the examples you're citing, but it seems like some are cases where ToString() isn't checking for null simply because it simply can't be. In the Roslyn SymbolDisplayPart example, for instance, text is checked in the constructor and it doesn't seem to be possible for _text to actually contain null. Even in cases where there's no check (e.g. DiagnosticSource.Name), it may be that the class can't or isn't meant to function with a null value. It may even be that these cases can be "fixed" by introducing the missing runtime checks that the value isn't null when accepting it.

[stephentoub]

In the Roslyn SymbolDisplayPart example, for instance, text is checked in the constructor and it doesn't seem to be possible for _text to actually contain null

It's a struct. default(SymbolDisplayPart).ToString() will return null.

[jbtule]

These examples are all things that look like they aren't intended to null, so it kind of suggest, it would be general code quality for ToString to be non nullable.

[stephentoub]

it would be general code quality for ToString to be non nullable.

If we were starting today, sure, we could make it non-nullable, and then it would help overrides to get it right. But we're not. There a bazillion implementations already in the wild. And consumers see that same annotation.

[nguerrera]

I filed dotnet/roslyn#34604 to make sure that the completion of override ToString defaults to non-nullable return to encourage new overrides to not return null.

[terrajobst]

I think we've collected enough feedback here. Well publish a more carefully written document with our current thinking as a Markdown file in the CoreFx repo. We'll tune this document as we find more interesting APIs as part of us annotating the code base and, of course, based on customer feedback consuming said annotations.

To keep things organized, I'm going to lock this thread.