[Proposal]: NotNullWhen on return values

[TobiasKnauss]

NotNullWhen on return values

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: Not Started

Summary

The proposed usage of the existing compiler attribute NotNullWhen allows declaring a nullable return value as not-null when a boolean parameter has the specified value.

Motivation

In the following example, the returned XmlNode may be null if the given XPath is not found. If the boolean parameter i_throw is true, the function will never return null, but throw an exception instead. There is no way to tell this to the compiler.

public XmlNode? SelectNode (string? i_xPath, bool i_throw = true)
{
  var xmlNode = ...;
  return xmlNode == null && i_throw
           ? throw new Exception ()
           : xmlNode;
}

With the proposed attribute usage, the compiler would know the behaviour of the function and it would not mark the variable in the screenshot as "may be null".

[return: NotNullWhen(nameof(i_throw), true)]
public XmlNode? SelectNode (string? i_xPath, bool i_throw = true)

Detailed design

I am not an expert on C# language, so I don't know how the existing implementation must be enhanced or modified, if at all.
Someone else may

Drawbacks

Maybe the benefit is not worth the effort.
But I don't see any language-related issues.

Alternatives

The alternative, which I already use, is creating an overload of the function with return value XmlNode instead of XmlNode?:

public XmlNode SelectNode (string? i_xPath)
{
  var xmlNode = SelectNode (i_xPath, true);
  return xmlNode!;
}

public XmlNode? SelectNode (string? i_xPath, bool i_throw)
{
  ...
}

I would like to avoid this extra work.

Unresolved questions

Design meetings

[RikkiGibson]

Roslyn internally tends to use a name like GetRequiredThing when the throw-on-not-found behavior is desired.

[jnm2]

Type.GetType and Assembly.GetType are more examples I've noticed would play better with NRTs with a facility like this.

[TobiasKnauss]

I did not know that .NET FCL methods actuality exist, which use this boolean parameter.

[olivier-spinelli]

Many thanks for this proposal. I'm waiting for this too.

FYI, some people consider that this boolean is a code smell and should be avoided (by using 2 different methods). My humble opinion is that even if it could be done, this inevitably leads to shouldThrow ? CallThatThrows() : CallThatDoesntThrow() fragments at places where the behavior HAS to be dynamically chosen (from configuration, from usage context, etc.).

I understand the "code smell" but for low-level API, the boolean helps a lot (at least for me).

[TobiasKnauss]

@olivier-spinelli I am pretty sure that the throwOnError parameter is not the only use case for this attribute, it's just the only one that we're discussing so far.

[olivier-spinelli]

Totally agree :)

[dubiousconst282]

For what is worth, this can be accomplished today with DoesNotReturnIf, and I don't think there are edge-cases with it, despite it sounding a bit misleading.

public Thing? GetThing(string name, [DoesNotReturnIf(true)] bool throwIfNotFound = true) { ... }

[TobiasKnauss]

@dubiousconst282 I think you are misunderstanding something: The function will of course return if it executed successfully, even if the boolean parameter is true. This parameter only has effect in the case where the function failed to execute: It specifies whether the function will return then or not.
Adding the DoesNotReturnIf to the boolean parameter would mean, then the function never returned if this parameter was true, which is wrong.

[dubiousconst282]

That's what I meant with it sounding misleading.

In practice, the DoesNotReturn attribute only seems to affect the nullable reachability analysis, so adding it to a function effectively means that it will never return null if the argument has the specified value. This behavior is unlikely to change based on #2555 (comment) and #3826 (comment).

[TobiasKnauss]

Yes, it is true that it will never return null if the parameter is true and the attribute exists. This would indeed meet the target.
But it also means that it never returns at all, which might impact the nullability analysis and other analysis inside the calling function in a negative way...? Therefore I am not sure whether it meets the target completely or only partially and with negative side effects.

[RikkiGibson]

The main concern I'd have is, it might make the compiler assume that expressions after the call are in an unreachable state where everything is assumed to be non-null.

[dubiousconst282]

Did a quick test, and indeed using DoesNotReturnIf causes the issues mentioned. The analysis will stop at the call and every other value will be considered to be non-null:

void M() {
    string? a = null;
    string? b = B(true);

    a.Contains('1'); //no warning for `a`
}
string? B([DoesNotReturnIf(true)] bool r) => null;

[TobiasKnauss]

@dubiousconst282

The analysis will stop at the call and every other value will be considered to be non-null

I rather think that the analysis will stop at the call and nothing else will be considered at all. There simply will be no further checks inside this method, hence no warnings, because DoesNotReturnIf tells the analysis that the rest of the method is unreachable.

I appreciate your idea, but as we found out, it looks like it does not produce the desired result. I think we still need this proposal.

[hughesjs]

Even just simple:

[return: NotNullIfTrue(paramName)]
[return: NotNullIfFalse(paramName)] 

Would be a huge improvement and probably easier to implement than the more general [NotNullIfTrue(expr)]

[Thomas-Shephard]

This would be amazingly helpful as it would allow better support for methods where you either throw an exception if the item is not found or return null based on a parameter.

I think it would make sense to me for the attribute to be used like the DoesNotReturnIfAttribute i.e. NotNullIf on a parameter so that could be used like this.

internal static string? GetAttribute(this XmlReader reader, string attributeName, [NotNullIf(true)] bool throwIfNotFound = true) {
    string? attributeValue = reader.GetAttribute(attributeName);

    if (throwIfNotFound && attributeValue is null)
        throw new Exception($"Attribute '{attributeName}' not found");

    return attributeValue;
}

[CaptaiNiveau]

I've often thought that something like this exists, went to documentation only to remember that it doesn't.
Would love to see this added :)