Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Champion: Simplified parameter null validation code #2145

Closed
5 tasks done
jaredpar opened this issue Jan 15, 2019 · 773 comments
Closed
5 tasks done

Champion: Simplified parameter null validation code #2145

jaredpar opened this issue Jan 15, 2019 · 773 comments

Comments

@jaredpar
Copy link
Member

jaredpar commented Jan 15, 2019

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (rejected)
  • Spec'ed

Specification: https://github.com/dotnet/csharplang/blob/main/proposals/param-nullchecking.md

In short though this allows for standard null validation on parameters to be simplified using a small annotation on parameters:

// Before
void Insert(string s) {
  if (s is null)
    throw new ArgumentNullException(nameof(s));

  ...
}

// After
void Insert(string s!!) {
  ...
}

LDM history:

@Thaina
Copy link

Thaina commented Jan 15, 2019

Shouldn't this be string! ?

And also doesn't C# will always warning on nullable reference type from now on?

@jaredpar
Copy link
Member Author

@Thaina

Shouldn't this be string! ?

No. This feature is for runtime value checking only. It does not affect the type system in any way. Hence the syntax annotation is on the value identifier, not the type.

And also doesn't C# will always warning on nullable reference type from now on?

Nullable reference type checking is an opt-in feature. Also it is designed to warn on potential null references in the code base but it doesn't affect the execution semantics of it.

@qrli
Copy link

qrli commented Jan 15, 2019

Maybe over simple, though 😆
And it looks like a declaration rather than implementation

@DavidArno
Copy link

Great to see this proposal has resurfaced, and has been championed. I think it an excellent idea but thought it had been lost amongst all the other work going on around NRTs. Thanks, @jaredpar. 👍

@Joe4evr
Copy link
Contributor

Joe4evr commented Jan 15, 2019

Shouldn't this be string! ?

No. This feature is for runtime value checking only. It does not affect the type system in any way. Hence the syntax annotation is on the value identifier, not the type.

You say that, but the original idea does put it on the type.

@GeirGrusom
Copy link

It's the dammit operator placed on the argument. It makes sense that it is applied to the value rather than the type.

@alrz
Copy link
Member

alrz commented Jan 15, 2019

It makes sense that it is applied to the value rather than the type.

that logic works only for parameters not property setters (that's why we'd need to invent syntax like set!). to me, this is more of a very specific code generation problem (what if I just want to Assert?), IMO the functionality should be a part of method contracts, or alternatively, some special attribute (which could be extended to other validation logics as well).

@Thaina
Copy link

Thaina commented Jan 15, 2019

@Joe4evr If I understand it right (from the jaredpar's comment after mine) this is actually newer feature unrelated to nullable reference type. It just a syntactic sugar to throw exception while nullable reference type is compile time warning

@Logerfo
Copy link
Contributor

Logerfo commented Jan 15, 2019

is this feature supposed to ship with 8.0 as well?

@DavidArno
Copy link

@Logerfo, my money is on "unlikely, but not impossible". Seems a candidate for 8.1+ to me. But I could be wrong...

@bbarry
Copy link
Contributor

bbarry commented Jan 15, 2019

In isolation I think this is great but I'd much rather see a more comprehensive contract annotation system with a potential way to expand it for pattern matching validations and potentially even arbitrary code validation as well as return value validations, even if those advancements don't come as part of a first version of this.

In short I worry that this:

ReadOnlySpan<char> Foo(string s!, int n) 
{
    ...
}

means something like this will never happen:

ReadOnlySpan<char> Foo(string s, int n)
    requires s != null // perhaps: requires s! 
    requires s.Length > 0
    requires n >= 0
    requires n < s.Length
    ensures value.Length > 0 
{
    ...
}

@Joe4evr
Copy link
Contributor

Joe4evr commented Jan 15, 2019

If I understand it right (from the jaredpar's comment after mine) this is actually newer feature unrelated to nullable reference type.

You're not wrong in that this feature could be achieved without NRTs, but the initial idea of having the compiler generate null checks for parameters (which is an extremely common scenario) came up during discussions of the NRT design. (Or rather, that was the moment the LDT would take it seriously. Theoretically, we could've had a syntax for generated null-checks ages ago.)

And while it's true that the check is about the value and not the type, applying the ! to the type 1) makes it more symmetrical, and 2) would probably make a lot of edge cases moot. (Granted, the reverse index syntax shows how much they care about symmetry. ¯\_(ツ)_/¯)

@jaredpar jaredpar self-assigned this Jan 15, 2019
@jaredpar jaredpar added this to the 8.0 candidate milestone Jan 15, 2019
@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Jan 15, 2019

I spoke with @tannergooding a little regarding this. He suggested that it might be more sensible for the compiler to emit a call to a helper method to perform the throw instead. I agree with this, but for somewhat different reasons.

In my idealized scenario, the null check would actually be emitted by the JIT / codegen, not the C# compiler. The reason for this is that the JIT can generally handle this more optimally since it's already responsible for setting up things like exception dispatch. Consider the following method, whose prologue is quite common throughout the framework.

public void DoSomethingWithArrays(byte[] buffer, int offset, int count)
{
  if (buffer is null) { /* throw */ }
  if ((uint)offset > (uint)buffer.Length || (uint)count > (uint)(buffer.Length - offset)) { /* throw */ }

  /* actual method logic goes here */
}

With the proposed syntax the method declaration DoSomethingWithArrays(byte[] buffer!, int offset, int count) would allow the developer to elide the first line, and the C# compiler would emit essentially the same thing.

What I would love to see is the JIT elide the check entirely. Instead, optimistically and immediately attempt to dereference buffer.Length without performing a null check:

mov eax, dword ptr [rcx + 8h] ; rcx := buffer, move buffer.Length into eax

If rcx (buffer) is null, this will immediately trigger an interrupt just like any other null ref would have. The runtime would need to perform some sleight of hand to deduce that this should become an ArgumentNullException instead of a standard NullReferenceException. The end result is that the original method is faster in the happy path of no exception being thrown - less code gen to emit, smaller method sizes, fewer branches - at the expense of making exception dispatch a little more complicated. Generally this seems like a good tradeoff to me.

Coming back to the start of this comment, the reason this matters from a compiler perspective is that we should plan for a future JIT which might have these optimizations. There are a handful of ways we could do this: put a special attribute on the arguments, call a helper method to perform the null check, or smuggle some other metadata that allows the JIT to recognize and remove the compiler-emitted checks. I don't really have a vested interest in which particular mechanism is used, but I wanted to make the compiler team aware of the implications of the feature design.

@HaloFour
Copy link
Contributor

@GrabYourPitchforks

And what if this method doesn't immediately attempt to use the non-nullable argument and instead does some body of business logic which definitely should not be happening if the argument is null? That seems like a lot of tricky optimization that would only apply to very specific codepaths.

@GrabYourPitchforks
Copy link
Member

@HaloFour In most cases I've seen the null check is only meant to avoid a null ref exception on the line immediately following, which generally is a candidate for this optimization. This should serve the majority of developers well by improving the efficiency of their code. Any other more complex code would still go down the standard "perform an explicit null check" path.

@HaloFour
Copy link
Contributor

@GrabYourPitchforks

So the specific optimization would only apply to a method accepting a single reference argument that shouldn't be null where the code immediately attempts a deference? Is that really so common? And if it is, wouldn't it be simple enough for the JIT to recognize and optimize without requiring a new special handshake from the compiler?

I'm not making the argument against making things faster for a common case, especially if it's common in the BCL itself, this just seems oddly specific. Even in your use case above, what happens if the C# code validates that offset is >= 0 before accessing buffer.Length, does that defeat this potential optimization?

@HaloFour
Copy link
Contributor

@GrabYourPitchforks

Rereading your original comment it does sound like you're suggesting that this is something that the JIT could do, aided by the compiler emitting a call to a helper method to throw the exception instead of throwing it directly. If that's the case, sounds good to me. :)

@jaredpar
Copy link
Member Author

@GrabYourPitchforks

Coming back to the start of this comment, the reason this matters from a compiler perspective is that we should plan for a future JIT which might have these optimizations

In this case I think we are fairly safe to change the implementation of the throw at a later date. Consider if the runtime introduced a helper Runtime.ParameterNullCheck<T>(T arg). A newer version of the compiler could bind to that if it existed and fall back to a generated helper if it didn't. There isn't much of a compat concern here as there is no user code involved.

@Korporal
Copy link

I think C# is getting unwieldy, newer features are leading to more and more idiosyncratic syntax and non intuitive behavior, consider the assymetric Index concept and the odd way ref and var behave. The language is becoming more and more inelegant each year. Its time for a new language with a richer grammar, these C based grammars are crippled.

@HaloFour
Copy link
Contributor

@Korporal

What does any of that rant have do with this proposal?

@Korporal
Copy link

@Korporal

What does any of that rant have do with this proposal?

@HaloFour - I'm voicing an opinion on the contrived use of "!" which usually means "not" but will now have another meaning. This is being done because the options are limited due to the inflexible grammar.

I then go on to suggest that perhaps a new language with a better more flexible grammar is the way to go rather than performing syntactic acrobatics, for example a grammar that has no reserved words would perhaps offer much greater options for adding new features over time.

@HaloFour
Copy link
Contributor

@Korporal

Funny, your rant doesn't mention that at all.

Syntax often has multiple meanings in many different languages. Many languages overload the definition of !, such as Rust and Swift.

I then go on to suggest that perhaps a new language

You're free to start a new language if you want, or use any of the other many, many languages in the ecosystem, .NET or otherwise. But I'd suggest that design discussions for languages that are not C# aren't appropriate for a repo specific for the design of the C# language, nor are you likely to recruit many like-minded individuals.

@CyrusNajmabadi
Copy link
Member

I then go on to suggest that perhaps a new language with a better more flexible grammar is the way to go rather than performing syntactic acrobatics,

Can you please take that discussion elsewhere? It is specifically out of scope and unnecessary in this specific issue. You are free to do any of the following as it suits you:

  1. take it to gitter.im. That's a good place to wax philosophical about what you want the language to be.
  2. blog about it on any platform of your choice.
  3. create a honey-pot issue for that discussion. while i would prefer it not be in csharplang at all, this would at least localize the dicsussion to a single space instead of leaking out and getting mixed up into real issues that are being worked on.

@MrJul
Copy link

MrJul commented Jan 21, 2019

I find it weird that this feature is being championed at a time where another feature makes it almost unnecessary.

I believe that a few years from now, when nullable reference types and C# 8 are completely adopted by the BCL and most popular libraries, this type of check will be redundant: why would you want to check for a null argument, when you already know you're being passed a non-null one (unless you're an existing public library method already documented to throw ArgumentNullException)?

I know about the implementation details of null reference types, and that everyone is still "free" to pass null to non-null arguments. In practice, once everything is correctly annotated, that won't really happen.

I don't have any crystal ball, but I already saw a similar effect in a large codebase. Everything in this code base (and I mean every field, property, parameter and return type of every method) is annotated with ReSharper's [NotNull] and [CanBeNull] attributes. (I can't wait to convert them to C# 8 nullable/non-nullable references!) At first, any public method had the classic if (arg == null) throw new ArgumentNullException() for every argument, and private/internal methods had debug assertions.

We never encountered those. With everything annotated and compile-time checked we didn't see any NullReferenceException, ArgumentNullException or null assertions fired in years in first party code. Believe in the tools! :) We started to gradually remove all those checks, since they sometimes take more lines that the method itself, plus they have a (very small) runtime cost for what is basically dead code.

Please, at least consider using an attribute like [NullCheck] instead, keeping the simple suffix syntax available in the future for other purposes. This also seems the kind of feature which won't be needed with proper code generators / AOP.

@qrli
Copy link

qrli commented Jan 22, 2019

Reference #2153
Especially, to make the two features to work as one: #2153 (comment)

@yaakov-h
Copy link
Member

With everything annotated and compile-time checked we didn't see any NullReferenceException, ArgumentNullException or null assertions fired in years in first party code.

My team has seen the same effect from hundreds of thousands of unit tests, and from Code Contracts.

However, we often have developers try and use code in new and exciting ways, like jumping into an assembly from PowerShell or C# Interactive or similar. Plus, a lot of code such as DI ends up going into Reflection and similar.

As such, the approach my team is taking is:

  • Add/keep null checks at the surface of the assembly
  • Remove them for all internal and private calls
  • (Pedantic mode:) Keep for constructors, particularly with DI.

Given the above, I personally do still see a use for this, even with a history of correctness proofs.

@fschmied
Copy link

fschmied commented Jan 26, 2019

On my team, we are currently using Fody NullGuard to automatically generate argument null checks in conjunction with the Implicit Nullability ReSharper add-in, which implements static checking for non-nullable reference types. The nice thing is that with this combination (and a little bit of configuration), NullGuard already knows which method parameters need argument null checks and which ones don't. Switching to the feature of this proposal would require us to explicitly annotate the parameters again with no gain, which feels redundant and cumbersome to me.

You might argue that being explicit is better in this case, as you enable a side effect, but if you're embracing non-nullable reference types, the side effect is only the second line of defense (for Reflection calls, type checker insufficiency, etc) and, in my opinion, doesn't deserve syntax of its own.

About the proposed syntax, I feel it's too special-purpose and closed for future extension, as it can't be extended to more general contact checking.

And finally about the position of the exclamation mark: the proposal argues that it can't be put on the type because it's not part of the type in the type system. But it really could be part of the type system quite easily by making T! denote a runtime-checked, i.e., guaranteed, hard non-nullable type (as opposed to an ordinary non-nullable type, which isn't guaranteed to be non-null). For hard non-nullable types, the compiler and JIT could implement stricter rules, e.g., emit errors instead of warning or elide null-checks as an optimization when passing a guaranteed non-null value to a T! parameter.

So, T? would be a nullable reference type, T a soft non-nullable reference type, T! a hard non-nullable reference type. Very symmetric and, IMO, more useful than a non-extensible shorthand for a very specific element of contract annotations. Even if you don't plan to implement different behaviors in the compiler at first, the syntax would allow for evolution into this direction in the future.

@TahirAhmadov
Copy link

@TahirAhmadov

I think the misunderstanding here is this notion that async means "no exceptions will be thrown", which is certainly not correct. I would expect any features specific to validation, such as contracts, to be enforced immediately and for the exception to be thrown synchronously to the caller, not to bounce into the machinery of the coroutine.

Sure, I'm not saying that shouldn't be allowed. But I would like it to be an option, not a mandate, with !! and future contract work.

@HaloFour
Copy link
Contributor

HaloFour commented Mar 28, 2022

@TahirAhmadov

Sure, I'm not saying that shouldn't be allowed. But I would like it to be an option, not a mandate, with !! and future contract work.

You have the option of not using the language feature. It's normal for language features to optimize towards guidelines and best practices.

@binki
Copy link

binki commented Mar 28, 2022

Would there be a way to ban usage of this feature? Would it get a code style flag so that usage can be set to error? If someone is using a static contract analysis framework, they’d want to be able to disallow this feature from being used in their project.

@CyrusNajmabadi
Copy link
Member

That's a contrived example which is different because of a type conversion nuance.

It's not contrived. People write this code, and the language has very different meanings here. It's fine to have a general intuition on how these things work, but then using that intuition to make strong claims that certain things definitely work the same way is not correct.

@333fred
Copy link
Member

333fred commented Mar 28, 2022

Would there be a way to ban usage of this feature? Would it get a code style flag so that usage can be set to error? If someone is using a static contract analysis framework, they’d want to be able to disallow this feature from being used in their project.

I don't think we'll add a code style flag for it (though I could be wrong), but it would be fairly trivial to write your own syntax analyzer to forbid it.

@CyrusNajmabadi
Copy link
Member

I don't think we'll add a code style flag for it (though I could be wrong), but it would be fairly trivial to write your own syntax analyzer to forbid it.

I don't think we have any intent on banning this feature. That's really not something we do (var being a very narrow exception). As Fred mentioned, this would be trivial for you to write your analyzer for.

@Eli-Black-Work
Copy link

Would there be a way to ban usage of this feature? Would it get a code style flag so that usage can be set to error? If someone is using a static contract analysis framework, they’d want to be able to disallow this feature from being used in their project.

I don't think we'll add a code style flag for it (though I could be wrong), but it would be fairly trivial to write your own syntax analyzer to forbid it.

Personally, I'd love it if the compiler showed an info/warning/error when it sees code like this:

public void Example(User user)
{
   // Should show info "Can use !! instead"
   if(user == null)
      throw new ArgumentNullException(nameof(user));
}

That way, I could bump the message up from "info" to "warning" or "error", to enforce style guidelines on new projects 🙂

@stephentoub
Copy link
Member

Personally, I'd love it if the compiler showed an info/warning/error when it sees code like this

That analyzer already exists in VS, along with a fixer to automatically make the change.

@Eli-Black-Work
Copy link

Oh, great! 🙂

@naamunds
Copy link
Member

naamunds commented Apr 1, 2022

To confirm, will this be supported on record types as well?

public record Person(string Name!!)

Would that result in an ArgumentNullException being thrown if Name is null in the constructor?

@HaloFour
Copy link
Contributor

HaloFour commented Apr 1, 2022

@naamunds

To confirm, will this be supported on record types as well?

Yes: SharpLab

@quinmars
Copy link

quinmars commented Apr 1, 2022

Interestingly, this will not throw:

var person = new Person("John")
{
    Name = null
};

record Person(string Name!!);

SharpLab

@HaloFour
Copy link
Contributor

HaloFour commented Apr 1, 2022

@quinmars

Interestingly, this will not throw:

Also under discussion: #5823

@TahirAhmadov
Copy link

Just confirming, this isn't supported for property setters and event accessors, right?

@D0ctorWh0
Copy link

For this code shouldn 't the compiler show an error on Work(null); as this will definitely cause an exception at runtime?

void DoWork()
{
    Work(null);
}

void Work(Test tst!!)
{
    tst.MyProperty = 0;
}

@stephentoub
Copy link
Member

For this code shouldn 't the compiler show an error on Work(null);

It issues a warning:

warning CS8625: Cannot convert null literal to non-nullable reference type.

SharpLab

@kimsey0
Copy link

kimsey0 commented Apr 19, 2022

This feature has been removed from C# 11: https://devblogs.microsoft.com/dotnet/csharp-11-preview-updates/#remove-parameter-null-checking-from-c-11

@En3Tho
Copy link

En3Tho commented Apr 19, 2022

Poll please. I don't want noise makers which are like 1% in any community forcing language team to cancel features actual majority needs.

@Thaina
Copy link

Thaina commented Apr 19, 2022

Poll please. I don't want noise makers which are like 1% in any community forcing language team to cancel features actual majority needs.

I don't think they was just cancel it, it just being removed from C#11 because it not ready yet. It might come back later, but not now

@AraHaan
Copy link
Member

AraHaan commented Apr 19, 2022

Honestly the only form of param null checking I can think of are:

  1. the current form
  2. using % at the end instead.
  3. using [NotNull] attribute which then implicitly gets executed (with compiler inserted code to do so on it's parameters) that then calls the ThrowIfNull() function for us (but expose the attribute as a keyword where the compiler can transform it to the attribute and insert the code to "run the attribute's code inside").

@AraHaan
Copy link
Member

AraHaan commented Apr 19, 2022

@jaredpar why not keep this open for further discussion into possibly having this refined for future C# versions instead of closing it so that way more ideas for refinement can flow in?

Also just thought of this option as well:

public class Example
{
    public void Something(string s1 not null, bool b1 not null);
}

Where the existing keywords could be used by the compiler to insert an ThrowIfNull() call for each param marked with not null from pattern matching (but reused for this feature).

@CyrusNajmabadi
Copy link
Member

This is an issue, which tracked an actual championed proposa/implementation. For a discussion, there should be an actual github discussion for it (which could link to this for example).

@kimsey0
Copy link

kimsey0 commented Apr 19, 2022

For context, here are the minutes from the two Language Design meetings that lead to the feature being removed:
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-06.md#parameter-null-checking
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-13.md#parameter-null-checking

@CyrusNajmabadi
Copy link
Member

@AraHaan we considered that syntax but didn't like it. See the notes in: https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-13.md#parameter-null-checking

@sungaila
Copy link

sungaila commented Apr 19, 2022

https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-13.md#parameter-null-checking

void M(string s) where s is not null;
This looks like a promising start for a full contracts feature, but not for a feature only doing null checking of parameters.

I love that idea! Microsoft Research had these .NET code contracts for testing 14 years ago. Having pre-/postconditions, invariants and parameter null checking with first class language and compiler support would be awesome.

This would be one of the biggest and best C# releases yet!

@AraHaan
Copy link
Member

AraHaan commented Apr 19, 2022

@AraHaan we considered that syntax but didn't like it. See the notes in: https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-13.md#parameter-null-checking

While the LDM members might not like it, the community might. Same for using contracts to check for nulls. I think having all 3 of them as an option but as an .editorconfig option to select their personal preference on what they want to be used for param null checking.

There are devs who would prefer these for null checking:

  • !! (the current syntax)
  • not null (reuse these pattern matching keywords so devs new to C# can get the general idea that the function will throw if they pass in null (the !! for dummies as some people say).
  • where s1, b1 is not null; for using contracts to check multiple parameters at one time without having to mark each one with !! but do the same thing.
    • perfect for functions that take in 8 ~ 16 parameters where adding !! can be exhausting depending on how many of such functions they have in their code.
    • also perfect for having minimal line changes in their code while also being able to use the new feature to enforce parameters are not null, but have the compiler generate the throw for them as well for each parameter in their contract (minimizes whitespace changes).

@CyrusNajmabadi
Copy link
Member

While the LDM members might not like it, the community might. Same for using contracts to check for nulls. I think having all 3 of them as an option but as an .editorconfig option to select their personal preference on what they want to be used for param null checking.

We will not do that. We're not going to add multiple redundant syntaxes because there are lots of different opinions on what people personally prefer for each of these areas.

@TahirAhmadov
Copy link

Is there an issue/discussion and/or a champion for !! in expressions?

@HaloFour
Copy link
Contributor

@TahirAhmadov

Is there an issue/discussion and/or a champion for !! in expressions?

Discussion, yes: #6034
Champion, no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests