Added: LDM Notes for May 15, 2019

[jcouv]

https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md

Agenda:
Refining nullability signatures in APIs

[bbarry]

On those attributes could we have description strings that appear in the warnings?

[jcouv]

@bbarry Taking an example: If you pass a null to a parameter with [DisallowNull] attribute, the compiler will warn as today on caller (sharplab). Maybe the warning message could be refined to mention the attribute in such scenario.
Is that what you meant?

[bbarry]

I was thinking that it might be useful for [DisallowNull] to have a message similar to how [Obsolete] works, but thinking about it more that may be excessive. For example if you ship an api and then later decide that a precondition shouldn't accept null you could add a [DisallowNull("passing null here is not allowed because it would later trigger a null reference when calling Baz")].

The current warning seems pretty sufficient though.

Reading further along I love the flow attributes. A [DoesNotReturnWhenNull(nameof(parameter))] is useful for parameter validations, We use R#'s ContractAnnotationAttribute pretty heavily in our codebase to identify halting conditions and have found it quite nice, but basically all of ours could be expressed as a DoesNotReturnWhenNullAttribute

https://www.jetbrains.com/help/resharper/Reference__Code_Annotation_Attributes.html#ContractAnnotationAttribute

[DavidArno]

I'm confused by:

public class Array 
{ 
    // Result can be default(T) if no match is found 
    [return: MaybeNull] 
    public static T Find<T>(T[] array, Predicate<T> match); 
} 

What is the purpose of that attribute? Why not write it as:

public class Array 
{ 
    public static T? Find<T>(T[] array, Predicate<T> match); 
} 

Is there a difference between using [return: MaybeNull] with a T return type and just using T? for the return type?

[Joe4evr]

@DavidArno This is the whole "unconstrained T" problem. T? has to be interpreted differently between reference types and value types and it's a bit of a mess to think about when you can only have one generic method.

[DavidArno]

@Joe4evr,

Ah, OK. That's worrying then as the use of the attribute looks like an attempt to paper over the cracks rather than fix "the whole "unconstrained T" problem". Hopefully it's just a stop-gap solution therefore.

[CyrusNajmabadi]

Ah, OK. That's worrying then as the use of the attribute looks like an attempt to paper over the cracks rather than fix "the whole "unconstrained T" problem". Hopefully it's just a stop-gap solution therefore.

I thnk that's an accurate way to describe things. I imagine unconstrained T won't be solved for 8.0. But it's certainly a very obvious and impactful wart that i think the LDM would want to address in the future.

[YairHalberstadt]

Will the compiler try to enforce these attributes internally to the method, or only externally to consumers of the API?

For example, will this warn:

[return: NotNull] object? M() => null;

?

[jcouv]

Per the notes, the priority is to enforce the contract on consumers. But if possible we'd like to enforce the contract on implementers too (this needs more investigation, as it may be tricky in some cases).

[qrli]

Having used R# for years, the [AllowNull] vs [MayBeNull], and [DisallowNull] vs [NotNull] looks duplicated to me. Is the pre-condition vs post-condition difference really worth separation?

The notes also lists two ways to annotate, which achieve the same. This looks like a proof of the redundancy.

The only case where the difference is necessary, I can think of, is if the annotation is to be put on properties than accessors. Then the pre-condition vs post-condition can tell whether it applies to getter or setter.

[YairHalberstadt]

I am concerned by the points @qrli mentioned as well as a couple of others:

1. In the common case, where you want the getter and the setter to match, and both allow/disallow null, you need to apply two attributes. The current design is favouring the rare case over the common.

2. With fields it is extremely rare for there to be a difference between the in and the out direction. So either we force all fields to pointlessly have two attributes, or we make them inconsistent with properties.

Perhaps we could replace the current 4 attributes with the following 2:

public AllowNullAttribute : Attribute
{
   public bool In { get; } = true;
   public bool Out { get; } = true;
}

public NotNullAttribute : Attribute
{
   public bool In { get; } = true;
   public bool Out { get; } = true;
}

That way we get the common behaviour by default, but it is possible to override it.

[jcouv]

Aside from properties, the difference between pre- and post-condition is also applicable to ref parameters.

Perhaps we could replace the current 4 attributes with [AllowNull] and [NotNull] with optional In or Out flags

I find that interesting. I'll circulate for more thoughts.

The notes also lists two ways to annotate, which achieve the same. This looks like a proof of the redundancy.

I think the redundancy there was different. You could constrain a T to class and then annotate it with [MaybeNull] (let's say, on a return value) or you could constrain it to class? but then annotate it with [NotNull]. Both ways achieve the same result for this simple scenario.

[alrz]

What are the additional generic constraints added for NRT so far? it seems like object has been changed to notnull?

[YairHalberstadt]

@jcouv

Perhaps we could replace the current 4 attributes with [AllowNull] and [NotNull] with optional In or Out flags

I find that interesting. I'll circulate for more thoughts.

I'm interested if there was any feedback on that?

Thanks

[jcouv]

I'm interested if there was any feedback on that?

Thanks for the ping. I raised the suggestion with LDM, which ended up leaning towards the current design. The current design allows better alignment between simple attributes and conditional attributes. Also, given the stage in the release cycle there is some bias towards what we have (the bar is higher to motivate a change).