Roslyn: OverrideCompletion isn't aware of nullability

[jcouv]

1. The completion list doesn't show nullability annotations (?) in the signature
2. The generated code also lacks nullability annotations in the signature

Update: from discussion with C# and corefx folks on 3/29/2019, we think that override completion should always change the return type to be non-nullable. For example, if you make an override for object.ToString(), the completion should be override string ToString() ....

Further Update: read the whole conversation below.

FYI @dpoeschl

Relates to https://github.com/dotnet/roslyn/issues/23268 (Covariant return types with respect to nullable reference types)

[jasonmalinowski]

Update: from discussion with C# and corefx folks on 3/29/2019, we think that override completion should always change the return type to be non-nullable. For example, if you make an override for object.ToString(), the completion should be override string ToString() ....

@jcouv: I disagree the answer is _always_ -- what happens for an API that returning null is a completely normal thing? Override completion _should_ spit the return type as nullable, because otherwise it's really confusing: not only does the user have to change it back, it might confuse them implementing the override because they might think they're not allowed to return null.

I think people were a bit to excited to extrapolate the ToString() case to everything! If the "the base may return null, but overrides shouldn't be", I suspect that's something we'll want to have extra annotations of some kind for and the IDE can respect that.

[jcouv]

I agree there is a trade-off.

Some alternatives:

1. special-case object.ToString, Exception.Message and a few others
2. use an attribute to mark methods that need such special-casing

[jasonmalinowski]

Didn't object.ToString() ultimately end up as non-nullable anyways?

I'm entirely OK with special casing, but I suspect the attribute (or XML doc comment...) is possibly the right way to go.

[jcouv]

We ended up with string? object.ToString() because many implementations do return null already and most users will be using overrides anyways (which will naturally declare a string when turning the feature on). I expect Immo will share notes.

[CyrusNajmabadi]

We ended up with string? object.ToString() because many implementations do return null already

I'd love to know numbers on what constitutes "many" :) Note that i would be interested in both absolute values as well as percentages. i.e. if there are 100 that do that, but it's 0.00001% of all types that would be interested to know :)

[jasonmalinowski]

@jcouv The PR that went in went in the other direction -- is there some other conversation that I'm missing here? And if the design for whatever is happening is predicated on the IDE working around that decision, then we need to be involved in that. :smile:

[CyrusNajmabadi]

I think people were a bit to excited to extrapolate the ToString() case to everything! If the "the base may return null, but overrides shouldn't be", I suspect that's something we'll want to have extra annotations of some kind for and the IDE can respect that.

Would love info on this. To me, even if some small amount of ToString methods returned null, my bias would be on the vast majority not returning null. As such, i would expect that i would be able to treat object.ToString as giving me back non-null so i would not have to always shut the compiler up for the common case.

In other words, while this signature woudl not be truth, having it be technically correct for the vastly rare case seems like it would be too painful for regular use.

[jcouv]

There was a conversation following the PR at noon today with Santiago, Tanner, Stephen, Immo, Mads, Jared, Neal and myself. We didn't know it might involve an IDE component. Sorry about that (we can certainly refine).

seems like it would be too painful for regular use.

Most people will not be bothered when consuming ToString, because you rarely use object.ToString() but often use the override on your own types.
The cases where that overload is used probably don't involve dereference (for example, you will likely not substring into something you don't know the format for), and the cases with dereference will probably benefit from the warning.
A potentially annoying use case is doing T.ToString() where T is constrained to one of your internal interfaces (so you know the types that could implement it are safe).

[jcouv]

Tagging @terrajobst who owns publishing some notes.

[jasonmalinowski]

We didn't know it might involve an IDE component. Sorry about that (we can certainly refine).

It happens -- but if the decision ultimately lands on the assumption of the IDE working a certain way, we'll need to check it. I suspect if we see this pattern happening regularly then we'll need to figure out some annotation mechanism for the IDE to respond to.

But add me onto the list that thinks this isn't the right way to do this. I realize there are offenders...but those are bugs. They should just fix those. I saw @stephentoub flagging some cases in Roslyn where we might have gotten it wrong -- let's make sure we fix those!

[terrajobst]

Let's separate the discussion on ToString() from this issue here (I'm not a fan of the decision for ToString() but I still believe there is value in this feature as described here by @jcouv).

The rationale for not copying null modifiers from the base is that we want developers to think about whether returning & accepting null makes sense for them.

Tagging @terrajobst who owns publishing some notes.

Notes will be sent later -- I'm waiting for what Stephen wrote up.

[CyrusNajmabadi]

The rationale for not copying null modifiers from the base is that we want developers to think about whether returning & accepting null makes sense for them.

How deep does that go. If the base says: I return IList<string?>? does the override say: You should return IList<string>?

[jcouv]

How deep does that go?

Only top-level. Although we've not implemented variance on parameter and return, only the top-level nullability could vary in the general case.

[CyrusNajmabadi]

The rationale for not copying null modifiers from the base is that we want developers to think about whether returning & accepting null makes sense for them.

I'm actually much more worried about that. We just spit out string and not string?. They don't realize and they ship, and then they either need to change something later, which is now a break, or someone who derives from them themselves wants to return string?.

They can always safely move from string? to string if they want that. But they can't do the opposite. So just stripping this, without the user realizing it at all, may put them into a bad place that they don't realize.

[jasonmalinowski]

The rationale for not copying null modifiers from the base is that we want developers to think about whether returning & accepting null makes sense for them.

I'd have a few concerns with blindly doing it everywhere:

1. If I'm doing an override of a method, it spitting the nullable return means it's a clear hint to me that a null return is at least part of the contract from the base type, and that a null return is something that is being considered by the caller. It tells me when overrriding this method I don't have to go to contortions to avoid returning null.
2. If somebody didn't think about it, and then shipped with the non-null return in a library, other developers are now depending on that. They can't later realize their mistake and go to the nullable return.
3. Doing this magic is indistingushable from an IDE bug to somebody who doesn't specifically know this works like this.

I think the feature you all really want is a code fix that detects that you have a method with a nullable return, but all of the "return" statements are returning non-nullable things. In that case, we should suggest to drop the nullable marker. And that'd work for whether it's an override or not. We could even make that a warning/error so you force developers to think about it or have to disable it with a documentation of why they're allowing for null returns.

[CyrusNajmabadi]

only the top-level nullability could vary in the general case.

Really? IEnumerable<string> is not an IEnumerable<string?>? That's super weird....

[jasonmalinowski]

I see @CyrusNajmabadi's comment is also the same as my concern 2... :smile:

[CyrusNajmabadi]

Oh look, I'm irrelevant now. I'll just let jason speak for my concerns :D

[jcouv]

Enumerable<string> is an IEnumerable<string?> (because of out).
That's what I meant by "not the general case" ;-)

Updated: copy/paste error fixed

[CyrusNajmabadi]

I think the feature you all really want is a code fix that detects that you have a method with a nullable return, but all of the "return" statements are returning non-nullable things. In that case, we should suggest to drop the nullable marker. And that'd work for whether it's an override or not. We could even make that a warning/error so you force developers to think about it or have to disable it with a documentation of why they're allowing for null returns.

I really lik this idea, and i think it's a better "IDE" way of thinking about this problem space. It delicately balances the concerns around blindly changing types, while ensuring users have information presented to them to make informed choices.

Let's not overly fixate on "override completion". INstead, let's talk about our general concerns about the user and their code, and see what we think the right experience is.

[terrajobst]

While I don't mind the lightbulb feature for suggesting removing the question mark, I think this feature wouldn't address the class of concerns we have for ToString(), namely, where we cannot annotate a virtual method to be non-null for practical reasons while we simultaneously don't want encourage new overrides to return null.

So if we don't want to omit top level null markers for all methods, could we do something else, such as:

1. Special cases the affected API by the IDE?
2. Adding a custom attribute for the APIs this is desired for that the IDE could honor?

[jasonmalinowski]

I'd prefer 2, because:

1. Any time you or _any other library author_ runs into another case like this you don't have to bug us.
2. The attribute also means we can do other smarts too. For example, somebody could then write a analyzer to have the compiler warn if somebody manually the override back to returning a nullable return. I could imagine somebody trying to null-annotate their code, run into this case, but since they were an offender returning null from ToString they'd leave it as nullable, and disable the warning while filing a bug to clean it up, or analyze whether it's a breaking change for them to do so. Having an analyzer would mean they could discover if they're now "clean".

[terrajobst]

Totally agree. Just wanted to bring the (potentially) cheaper option to the table too :-)

[jasonmalinowski]

Sure, we can special case it for just object.ToString(), but I'll be mighty suspicious when the second API request comes in. :smile:

[jasonmalinowski]

Although from the IDE's perspective, checking for an attribute on the method is very cheap to do; I suspect the hardest part would be bikeshedding the attribute name. :smile:

[terrajobst]

Looks pretty straight-forward to me:

[DoThingInTheIdeWeCouldntAgreeOnInAMeeting(Case = 47)] 😉

Seriously though, I think it wouldn't too bad. We'd invent some attribute that we can put in compiler services (or wherever we end up putting the other null-related attributes, such as the ones for the Try pattern). Something like [OmitNullableAnnotationsOnOverride].

[jasonmalinowski]

I'm not just bikeshedding because I joked about it but this doesn't apply to just overrides: consider us doing "implement interface" where we spit method names from an interface. There's also cases too like:

object o = ...;
MethodThatDoesNotExist(o.ToString())

If I invoke our "generate method" refactoring on that method, I'm guessing you want the IDE to treat that as non-null, despite the annotation?

[jasonmalinowski]

Actually, I guess you maybe the whole point here is you don't know that and want to propagate it. Ugh. 😦

[terrajobst]

Hmm, fair enough. This will take some time to flesh out. Either way, I'm not too scared though -- we agreed on tuple naming. I consider us unstoppable now 😄

[jasonmalinowski]

Well, and even if we decide for the ToString() case to call it non-null, we'll still probably want to figure out this mechanism anyways. All good fun!

[jasonmalinowski]

Current state of this feature with all our fixes so far:

1. The completion list item still doesn't show the ?s (although the description popup does, incidentally enough.)
2. We now spit the ?s everywhere, but we are dropping top-level nullability on the return. Although that's ironically the behavior some are asking for in some cases, I suspect this is right now happening by chance and not by a feature.

@terrajobst Now that we're past the annotation of corelib, did we discover other places where we also wanted override completion to do things other than directly copy it like ToString()?