Initial roll out of !!

[stephentoub]

C# 11 has implemented the parameter null checking feature !!, which let's us replace:

void M(object arg)
{
    if (arg is null)
    {
        throw new ArgumentNullException(nameof(arg));
    }
    ...
}

with:

void M(object arg!!)
{
    ...
}

This PR provides an initial pass through dotnet/runtime replacing manual ANE throwing with !! where possible, and ArgumentNullException.ThrowIfNull where !! isn't possible but the method call is.

This was partially done with the new analyzer/fixer in VS, but there are some issues still being worked out with it, so I manually reviewed and fixed up issues in the changes and then also did a lot of greping and manual changes. I'm 100% certain there are a non-trivial number of additional opportunities, which we can follow-up with subsequently, but I wanted to get this initial round in, as it's quite a large change.

For the most part I avoided any behavioral changes, e.g. in cases where using !! would have changed exception order, I avoided doing so in almost all cases (the only cases I went ahead with it were similar to those in #64357, where two parameters were being validated with multiple argument exceptions and this just changes the order of those argument exceptions in some multi-argument-error cases.

I still need to fix up some warnings/errors/test failures here and there.

cc: @RikkiGibson, @jaredpar

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

C# 11 has implemented the parameter null checking feature !!, which let's us replace:

void M(object arg)
{
    if (arg is null)
    {
        throw new ArgumentNullException(nameof(arg));
    }
    ...
}

with:

void M(object arg!!)
{
    ...
}

This PR provides an initial pass through dotnet/runtime replacing manual ANE throwing with !! where possible, and ArgumentNullException.ThrowIfNull where !! isn't possible but the method call is.

This was partially done with the new analyzer/fixer in VS, but there are some issues still being worked out with it, so I manually reviewed and fixed up issues in the changes and then also did a lot of greping and manual changes. I'm 100% certain there are a non-trivial number of additional opportunities, which we can follow-up with subsequently, but I wanted to get this initial round in, as it's quite a large change.

For the most part I avoided any behavioral changes, e.g. in cases where using !! would have changed exception order, I avoided doing so in almost all cases (the only cases I went ahead with it were similar to those in #64357, where two parameters were being validated with multiple argument exceptions and this just changes the order of those argument exceptions in some multi-argument-error cases.

I still need to fix up some warnings/errors/test failures here and there.

cc: @RikkiGibson, @jaredpar

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-Meta
Milestone:	-

[jeffhandley]

I reviewed 10% of the files, including some from the top, some from the middle, and some from the bottom. I observed several different patterns, saw a couple that took me a second, but then I understood them, and I saw nothing amiss.

[bartonjs]

Since I don't know if we're going for doc-it or revert-it, I marked all of the exception ordering changes I saw (and, for good measure, one that didn't (oops)).

Checked

• src/libraries/src/Common/System/{IO,Security}/...
• src/libraries/System.Formats.Asn1/...
• src/libraries/System.IO.Hashing/...
• src/libraries/System.Security*/...
• src/libraries/System.Text*/...

Aside from the one method on CryptoConfig, LGTM.

[stephentoub]

@LunarWhisper, @sstregg, @alhimik45, I'm curious about the confused emoji you selected. Is that about the !! feature itself? About dotnet/runtime using it? About something else? Thanks for any feedback.

[LunarWhisper]

@LunarWhisper, @sstregg, @alhimik45, I'm curious about the confused emoji you selected. Is that about the !! feature itself? About dotnet/runtime using it? About something else? Thanks for any feedback.

Hi! Yes, my reaction refers to the feature itself.
I have three concerns:

1. It seems to me that this reduces the readability of the code and increases the entry threshold. Instead of self-documenting attributes like [NotNull], we introduce additional language tokens, the purpose of which can only be understood from the documentation. It's like managed pointers ^, it's like the first step towards getting closer to the C++ syntax, which has raised a lot of questions about the difference between const char* and char const *. !! matter - we must pay attention to them. But at the same time, these are only two characters - we will have to focus on each argument, looking for them. The difference from the [ThrowIfNull] attribute is only in length. So we slightly help those who will edit a method that uses NotNull arguments, but complicate life for those who use this method.
2. It seems to me that instead of modifying the compiler, we delegate its task to the programmer. Thanks to non-nullable reference types, the compiler knows exactly which arguments can be null and which can't. If we don't want to validate arguments in the code, then let's not and let the compiler provide Fail Fast! for us.
3. I think this is a step in the wrong direction. Besides to checking for null, we have other checks for: empty string, empty collection, undefined Enum element, out of range, null element in collection, etc. All these checks will remain in the body of method. As a result, the probability of error will increase, since now both the one who edits the method and the one who uses it need to look for checks in two places. And the solutions for the cases described above cannot be written so simply as !! It seems to me that it would be more correct to concentrate on the ability to generate code based on attributes and increase the flexibility of the attributes, which would allow us to get AoP for all cases.

[alhimik45]

I'm curious about the confused emoji you selected. Is that about the !! feature itself? About dotnet/runtime using it? About something else? Thanks for any feedback.

I also was confused by the feature itself as I saw it first time. And it's strange that having the course to the explicitly nullable types devs are bothered by adding null-checking syntax crutch for one special case. It would make sense if !! was added like in Kotlin as shorthand for ?? throw new ArgumentNullException(nameof(val));. But current syntax looks very specific to be in the language.

[stephentoub]

Thanks for the feedback.

It would make sense if !! was added like in Kotlin as shorthand for ?? throw new ArgumentNullException(nameof(val))

That is what it is. Writing:

void M(object arg!!)
{
}

is the same functionally as:

void M(object arg)
{
    _ = arg ?? throw new ArgumentNullException(nameof(arg));
}

which is the same functionally as:

void M(object arg)
{
    if (arg is null) throw new ArgumentNullException(nameof(arg));
}

The code generated by the C# compiler for !! is more efficient than that, but it's functionally the same. Though it does enable some uses that you can't write today without much more code. The null checks are injected at the very beginning of the method, even before the method delegates to base(...) or this(...) invocations, which means previously if you had to write something like:

public C(List<T> list) : base(list is not null ? list.Count : 0)
{
    if (list is null) throw new ArgumentNullException(nameof(list));
    ...
}

now you don't need the second guard, because the validation happens before the base invocation:

public C(List<T> list!!) : base(list.Count)
{
    ...
}

Similarly, consider an async iterator like:

public static IAsyncEnumerator<T> WhereAsync(this IAsyncEnumerator<T> source, Func<T, ValueTask<bool>> predicate)
{
    if (source is null) throw new ArgumentNullException(nameof(source));
    if (predicate is null) throw new ArgumentNullException(nameof(predicate));
    return WhereAsyncCore(source, predicate);

    static async IAsyncEnumerator<T> WhereAsyncCore(IAsyncEnumerator<T> source, Func<T, ValueTask<bool>> predicate)
    {
        await foreach (T item in source)
            if (await predicate(item))
                yield return item;
    }
}

This employs an outer and an inner method, the outer of which isn't async while the inner is, in order to enable the exceptions to propagate synchronously at the time of the WhereAsync call, rather than only manifesting on the first MoveNextAsync invocation and being stored inside of the returned ValueTask<bool>. Now with !!, we can write it like:

public static async IAsyncEnumerator<T> WhereAsync(this IAsyncEnumerator<T> source!!, Func<T, ValueTask<bool>> predicate!!)
{
    await foreach (T item in source)
        if (await predicate(item))
            yield return item;
}

with exactly the same effect, because the !! injects the argument validation into the actual WhereAsync method rather than into the iterator.

Note that !! is currently limited to only apply to method arguments, as that's the vast majority use case (as is exemplified by the changes made in this PR), but the C# language team has talked about and is open to extending it in the future should it prove warranted, e.g. to properties, to locals, even to arbitrary expressions.

It seems to me that this reduces the readability of the code and increases the entry threshold

Obviously smart people are able to disagree on what helps and hurts a situation. FWIW, I think it improves the readability and maintainability of the code. This PR, for example, is removing ~17,000 lines of boilerplate from the repo, taking code that previously looked like:

        public static Task<int> Choose<T1, T2, T3>(
            ISourceBlock<T1> source1, Action<T1> action1,
            ISourceBlock<T2> source2, Action<T2> action2,
            ISourceBlock<T3> source3, Action<T3> action3,
            DataflowBlockOptions dataflowBlockOptions)
        {
            // Validate arguments
            if (source1 == null) throw new ArgumentNullException(nameof(source1));
            if (action1 == null) throw new ArgumentNullException(nameof(action1));
            if (source2 == null) throw new ArgumentNullException(nameof(source2));
            if (action2 == null) throw new ArgumentNullException(nameof(action2));
            if (source3 == null) throw new ArgumentNullException(nameof(source3));
            if (action3 == null) throw new ArgumentNullException(nameof(action3));
            if (dataflowBlockOptions == null) throw new ArgumentNullException(nameof(dataflowBlockOptions));

and now enabling it to look like:

        public static Task<int> Choose<T1, T2, T3>(
            ISourceBlock<T1> source1!!, Action<T1> action1!!,
            ISourceBlock<T2> source2!!, Action<T2> action2!!,
            ISourceBlock<T3> source3!!, Action<T3> action3!!,
            DataflowBlockOptions dataflowBlockOptions!!)
        {

That's a random example I pulled from this diff. To me, I can much more easily see the intention behind the validation being performed, that all the arguments that are supposed to be null checked are being null checked, and that they're being null checked correctly, e.g. the right argument is being thrown with the right argument name. And on top of that, the use of !! results in smaller, more efficient code gen than what was there before.

f we don't want to validate arguments in the code, then let's not and let the compiler provide Fail Fast! for us.

If nullable reference types were a thing 20 years ago in C# 1.0 and .NET Framework 1.0, it's quite possible we would have ended up with a solution like you suggest. But nullable reference types didn't come along until just a couple of years ago, are still not widely used, aren't 100% perfect in their validation and representation of reality, and are optional. The vast majority of code consuming the core .NET libraries, for example, is not using nullable reference types (but uses the same core library binaries), but even if that weren't the case, suddenly changing all code that did throw new ArgumentNullException to code that did Environment.FailFast would be a monumental and untenable breaking change (real world code catches Exception, ArgumentException, etc.), such that we couldn't use the feature at all. Basing it off of the existing nullable reference type annotations (or lack thereof) would also introduce further breaking changes... just because something is annotated to say "please don't pass nulls here" doesn't mean the method throws for null... in a non-trivial number of situations, for example, a base virtual will declare a parameter as non-nullable but either it or an override allows nulls (either explicitly or just ignoring the argument entirely), in which case injecting a fail fast when null was passed would be breaking. On top of all that, it would add a significant performance penalty. Null validation is typically only performed at certain entrypoints, after which the developer stops adding null checks (sometimes using debug asserts instead, sometimes just trusting in the non-nullness that was previously validated). If the compiler were to instead add null checks and fail fast calls for every non-nullable reference type parameter on every method, that would be adding a heck of a lot of IL and a heck of a lot of additional checks to be performed.

It seems to me that it would be more correct to concentrate on the ability to generate code based on attributes and increase the flexibility of the attributes, which would allow us to get AoP for all cases.

Nothing prevents that from happening in addition in the future. Such contract-based schemes were discussed in depth in multiple C# language design team meetings.

In any event, I appreciate your taking the time to share your feedback. As this PR is about employing a C# language feature that's already been approved and is already in the compiler, the feedback doesn't really impact the PR. But if after my responses you still have strong feelings about this not being the right shape of the feature, I'd encourage you to open an issue in https://github.com/dotnet/csharplang.

Thanks.

[TanvirArjel]

@Vake93 It has already been answered in here

[Runaurufu]

I see this solution as weird and confusing way to properly implement non-nullability...
hear me out here..
If I have int type which I want to be on some occasions null I make it as Nullable<int>... or just int?
Why on earth I cannot have same in opposite way?
Why I cannot make string bar into string! bar instead of string bar!!?

That is inconsistent and uclear way to solve pretty much the same problem which was already solved in completely different way (decorating type and not parameter name).
Another thing which @LunarWhisper already mentioned - this solution mean that you want to handle null values in special way which will not be available for other values (like empty list).. but yet what you want at the end (and examples you posted support this) is limiting type itself to be non nullable one.

That will also be for sure a lot of confusion for anyone new coming to C# :/

[Vake93]

@Vake93 It has already been answered in here

My question is about where is this exception thrown from. Is it thrown from inside the method?
Because once this feature is rolled out and being used we'll start getting error logs on this we should be able to easily identify the location of the code from the stack trace. If the stack trace ends up being like that of async methods with a lot of stack frames from System.Runtime.CompilerServices debugging these is gonna be a painful experience.

[javiercampos]

Please stop adding weird symbols for nullability reasons. Everything is already confusing enough with using the same nullability symbols for value and reference types (int?/string?) with completely different meaning and behaviors.

The feature itself is fine, but it could be easily added with something like:

void M([ThrowIfNull] string a) {
}

Which is explicit, and readable... because the next thing you know, we'll be reading the language when loud as questioning and shouting stuff (string?[]? M(int? a!!) anyone?).

This is all making the language completely unreadable

(And yes, I understand [ThrowIfNull] could clash with custom attributes, was just an example of something to make it readable... it could be <ThrowIfNull> or something else, but the trend of adding symbols to everything will never stop if we follow this route)

You could even use notnull which is already a C# keyword (and used for constraints), something like:

void M(notnull string a) {}

[Runaurufu]

Also that approach:
void M([ThrowIfNull] string a) { }
lets us go further this avenue and make more checks like:
void M([ThrowIfEmpty] string a) { }
void M([ThrowIfEmpty] int[] a) { }
and so on...

[ryanbnl]

We're turning C# into Javascript 👎

Is there a reason why this cannot be a compile-time option? You almost always want these guard clauses to be generated, however it can break things in order code bases.

So make a compile-time flag which will auto-generate the guard clause for these:

void M(string a)

but not for this:

void M(string? a)

As it's a flag it can also be turned off completely.

[worldpwn]

I think it will be better to pick the same approach as with ?.

We have:

  <PropertyGroup>
    ...
    <Nullable>enable</Nullable>
   ...
  </PropertyGroup>

It should be:

  <PropertyGroup>
    ...
    <Nullable>throw</Nullable>
   ...
  </PropertyGroup>

This will lead to the same behavior as described but without !!. All arguments will automatically throw in this case except the ones that are with the nullable sign ?.

[Hermholtz]

string?[]? M(int? a!!)

Yeah and waiting for this disaster to spill to generic types as well :)
string?[]? M<T?!!, in out _?>(int? a!!, ?T? b?!**^)

I think I'd just choose Perl for readability :)

[iancooper]

I don't think this has good usability. We should not be adding language statements to variable names. The name ought not to indicate anything about the type, the type should.

[jods4]

@stephentoub Your Choose example with 7 arguments and 14 ! in signature made me wonder...
As you explained, the policy is to always validate arguments on public facing apis.

Wouldn't it be better to decorate the method itself?

Instead of:

        public static Task<int> Choose<T1, T2, T3>(
            ISourceBlock<T1> source1!!, Action<T1> action1!!,
            ISourceBlock<T2> source2!!, Action<T2> action2!!,
            ISourceBlock<T3> source3!!, Action<T3> action3!!,
            DataflowBlockOptions dataflowBlockOptions!!)
        {

Maybe?

        [ValidateArguments]
        public static Task<int> Choose<T1, T2, T3>(
            ISourceBlock<T1> source1, Action<T1> action1,
            ISourceBlock<T2> source2, Action<T2> action2,
            ISourceBlock<T3> source3, Action<T3> action3,
            DataflowBlockOptions dataflowBlockOptions)
        {

Codegen would be the same, of course.

I'm probably too late, though?

[JoeStead]

I don't think this has good usability. We should not be adding language statements to variable names. The name ought not to indicate anything about the type, the type should.

Agreed, we have ? to indicate nullability of a type, it makes more sense to add !! to the type to indicate it will throw if it's null. Adding it to the name is odd and a bit inconsistent, no?

[dustinmoris]

I think a lot of the recent and upcoming C# language features get one fundamental thing wrong (in my personal opinion).

They optimise for writing code, not for reading code. I think it should be the opposite.

@stephentoub has shown a great number of examples throughout this conversation how !! substantically improves writing certain fragments of code, but in all those cases I found that it was much harder to read and grok that code afterwards.

Writing a few extra lines of if statements is not that hard and especially in .NET it's a minuscule problem given that most IDEs already write it for me at the tip of a few key strokes followed by a double TAB stroke.

I would encourage developers at Microsoft to ask themselves how open they actually are to community feedback like this. I don't mean to suggest anything bad, but personally I often get the feeling that by the time a feature like this gets revealed for feedback a lot of investment (time and emotion) has already gone into developing a (more or less polished) prototype of it at which point people already have a personal investment in defending their decisions and have a bias towards downplaying good counter arguments as "minority noise".

[olivier-spinelli]

What I'd like would be a simple new compilation flag on a project per project basis: <Nullable>enableStrong</Nullable> or <Nullable>enable!!</Nullable> (or whatever) that will consider any parameter T as being a "T!!" and nicely handles the throw for me.
I have plenty of projects that are ready for this....
No impacts on the signatures of my methods, no surprises for the caller... What's wrong with this ?

[Nihlus]

This is such an odd way of implementing an arguably very useful feature. Why add yet another syntax variant that doesn't jive with existing syntax? Why not allow developers to globally enable it for a project that's already fully annotated with nullability? Why two exclamation marks and not one?

Please listen to the community on this one and don't just blindly push it through. It's not the feature that's being questioned, it's the way it's implemented.

[olivier-spinelli]

@Nihlus What about the <Nullable>enable!!</Nullable>?

[SteveDunn]

Since the advent of NRT,s it's been like a green light to use more nulls. C# devs seem particularly averse to Maybe or Option. Maybe (ahem), if these were added to the language, they'd get better adoption and then we wouldn't need so much extra syntax.

I also have another comment re. the actual exception type: in a library, like .NET itself, the exception is as meaningul as it can be. However, in client apps and libraries, where things are much more specific, then ArgumentNullException doesn't really convey anything meaningful in that domain (for instance, you wouldn't sanely catch an ArgumentNullException). For instance, a better exception when processing Customers and encountering a null Customer, would be a CustomerProcessingFailedException.
Over the years, I've had devs quote guidance from 'Framework Design Guidelines' (which is a superb book, don't get me wrong), which makes little sense in non generic Frameworks, and I feel that this exception is one of those things that only make sense in a generic library/framework.

[asbjornu]

I don't really understand the design choice here. Nullable types already establish Type? as a convention where ? is used to annotate the nullability of a type. That would lead to the natural conclusion that Type! would be the design of, in effect, the opposite of nullable types.

But somehow you concluded that the annotation should go on the argument name and not on the type as well as requiring two exclamation marks instead of one? I think this reads so much better than the proposed implementation:

void M(object! arg)
{
    ...
}

Another option would be to expand on NRT to make the compiler even more strict, adding null-checks to all reference types automatically, avoiding the need for syntactic sugar altogether.

[SteveDunn]

I would like a combination of:

1. A project-wide setting (<Nullable>autoThrow</Nullable>) - that auth-throws on null parameters
2. A formal way in C# to validate parameters, e.g. ProcessCustomer(int customerId) requires customerId > 0

I wrote about validation, invariants, and a modern-day replacement of Code Contracts, here

... and for validation, a language proposal for invariant records

[nathan130200]

I agree will be more simple adding in csproj <Nullable>thrown</Nullable> but at same time !! symbol self-explain feature. Isn't hard to understand.

I hope that !! begin optional, because C# will begin transform as trash kotlin lang that enfores everything with nullable checks with !! even variables that never should be null.

[Runaurufu]

I agree will be more simple adding in csproj <Nullable>thrown</Nullable> but at same time !! symbol self-explain feature. Isn't hard to understand

I would prefer !! in code over config in project... but simply not as a postfix for parameter name. Make it a postfix for type in same way as ? is and we will have clear view of what is going on.

[worldpwn]

I agree will be more simple adding in csproj <Nullable>thrown</Nullable> but at same time !! symbol self-explain feature. Isn't hard to understand

I would prefer !! in code over config in project... but simply not as a postfix for parameter name. Make it a postfix for type in same way as ? is and we will have clear view of what is going on.

For example, I use the nullable feature. And in my projects, I have nullable only from DB/DataInput all others are considered not null because I make null checks after DB/DatatInput. So in the case of !! I have to add them everywhere. Imagine how code will look like 🤪

[javiercampos]

I agree will be more simple adding in csproj <Nullable>thrown</Nullable> but at same time !! symbol self-explain feature. Isn't hard to understand

I would prefer !! in code over config in project... but simply not as a postfix for parameter name. Make it a postfix for type in same way as ? is and we will have clear view of what is going on.

Except they involve different dynamics (also, different for value and reference types). A string? is a reference types that the programmer knows "can be null", and a string (with NRT enabled) is a string that "should not be null but can be null". So you start adding symbols and symbols in the type.

Also, would this work for Nullable<T> types? Because otherwise, int!! doesn't make sense because int can't be null, and then you need to go int?!! which looks extremely weird, but then you could say that, "in the case of value types, having !! also implicitly makes it Nullable<T>, which complicates its reading, but then not on reference types, where string!! and string?!! would mean two different things.

This is all too complicated, again, I'd vote for notnull type name, which doesn't introduce any additional keywords or symbols (notnull is already a C# keyword), it's easy to read, and gives a perfectly reasonable meaning, and would work for any type that "allows null" (with either NRT enabled or not)

[80O]

I would rather see nullable being applied to existing source code and have nullable enabled be the default. Adding !! feels like a hack to not resolve the problem of having no precondition checks in your signature.

If I enable nullable in my projects, and something can be null, its ?, otherwise it will never be null. If someone uses my API and tries to add in an null object, it will crash somewhere in the method. Then the caller should've checked for null, its not the callees' responsibility to null check any parameter input when <Nullable>enable</Nullable> and its signature is clear about the preconditions.

[rcollina]

Prefix notnull instead of !! is much better.
More keystrokes, granted, but it’s readable, can be searched online, and it’s better than turning C# into symbol soup in the long run.

[Runaurufu]

Except they involve different dynamics (also, different for value and reference types). A string? is a reference types that the programmer knows "can be null", and a string (with NRT enabled) is a string that "should not be null but can be null". So you start adding symbols and symbols in the type.

And we are back in time when NRT and string? were introduced... for starters NRT is not enforcing anything on how code works (because it only hints you) while nullables int? does. Using same symbol ? for two different things (documenting intend vs. type change) was bad back then and is still eyesore.

Also, would this work for Nullable<T> types? Because otherwise, int!! doesn't make sense because int can't be null, and then you need to go int?!! which looks extremely weird, but then you could say that, "in the case of value types, having !! also implicitly makes it Nullable<T>, which complicates its reading, but then not on reference types, where string!! and string?!! would mean two different things.

This is same case as for nullable ref types.. originally things like string? were not allowed...
So using public struct Nullable<T> where T : struct as base we can introduce public struct NotNullable<T> where T : class
And make string!! or string! shorthand for NotNullable<string> it in same was as int? is for Nullable<int>.

As for constructs like string!? or int?! ... that would translate into: Nullable<NotNullable<string>> and NotNullable<Nullable<int>> but I would personally forbid such constructs... allow either ? or ! but not both.

[stephentoub]

Everyone, thank you for the discussion, but this is not the place for it. This PR is simply using a now-existing C# feature, it is not designing nor implementing that feature in the language or compiler (if the feedback were on how the feature was employed in dotnet/runtime, that would be appropriate here, but that's not what this discussion has become). The C# language design team doesn't regularly pay attention to this repo, nor to specific PRs in this repo, and this is not where the community-at-large expects to find and discuss language features. Feedback on a language feature on a PR in this repo will largely go unseen by the relevant folks. If you would like to provide feedback on C# language design, the right place is https://github.com/dotnet/csharplang. If you feel strongly that !! is the wrong design and/or could be improved, as I requested earlier please open an issue in https://github.com/dotnet/csharplang/issues to discuss it there, where language discussions are had and language decisions are made. Thank you.

[Hermholtz]

Yeah while I agree, these discussions tend to surface the world anyway, like when Microsoft decided to remove watch from dotnet cli. Then the damage is out of control already.

[dustinmoris]

Done:
dotnet/csharplang#5735

[stephentoub]

Thank you, @dustinmoris.