Support nullable reference feature better with attribute
iron9light opened this issue · 14 comments
I just saw this dotnet/standard#1359 .
I'm not sure if this lib should do similar things.
Unfortunately I don't see a way how these attributes can help with Requires for null detection. :(
@terrajobst Should there be an attribute that I can decorate my Requires.NotNull methods with so that the C# compiler knows an argument is not null after it returns? I can't find one among the various new attributes.
I've pushed some changes and I'm just trying to figure out how to validate that it's effective.
Should there be an attribute that I can decorate my Requires.NotNull methods with so that the C# compiler knows an argument is not null after it returns?
That's NotNull. See #33.
Since this lib started targeting netstandard2.1, I think it's time to use c# 8.0 and enable Nullable feature now.
@iron9light yes, I'm toying with that already.
@cartermp Is NotNull the right pattern here?
We settled on NotNull here as it seemed to produce the behavior in the compiler that we wanted. I hope that was the right call.
Interesting! The annotated methods don't use ref parameters and when calling something like Requires.NotNull, developers won't be using its return value. But we still want to ensure the T is non-null after the method is called.
I agree with the decision to go with NotNull here even though this scenario isn't quite about dealing with a result value. Perhaps I should adjust the blog post to have an example like this showing that you don't need to apply this only for resultant values.
I agree with the decision to go with
NotNullhere even though this scenario isn't quite about dealing with a result value. Perhaps I should adjust the blog post to have an example like this showing that you don't need to apply this only for resultant values.
That would be good. Null checkers seem reasonable common.
They would, though I think it's also worth considering that a sub-goal of NRT could be to eliminate the need for null-checker APIs over time, since these sorts of pre- and postconditions would be handled more at the type system over time. But certainly from the perspective of keeping existing code happy, these are valuable.
That is to say, I'll have to think about the example a bit. I'd like to encourage people to stick with using their type system itself rather than throw additional machinery atop APIs that they may not need in the future. Perhaps this blog post isn't the appropriate avenue for that, though, and docs are a better place.
I think it's also worth considering that a sub-goal of NRT could be to eliminate the need for null-checker APIs over time
If it's deeply embedded in the type system such that it's impossible for my method to be called with a null value, that's fine. But until then, I still see a lot of value in guarding against null values "at the door" so that we don't throw NRE at arbitrary places later.
I'd like to encourage people to stick with using their type system itself rather than throw additional machinery atop APIs that they may not need in the future.
We need to be careful to not "over-sell" this feature though. People might think that because they've added #nullable enable and not used a ? suffix on a ref type that they're "safe". They are not. Not a single one of their existing external callers (and many of their new ones) will even get a warning when passing null into their API after they make this change, and there is no enforcement anywhere. People who found it useful to add explicit null checks in their methods should absolutely continue to do so, at least in their public/protected entrypoints. I think removing explicit checks from private members is fine since you know your codebase is already using nullable:enabled everywhere. But for a public library, null checks are a must. This should be called out. And properly supported, of course.
If the point of NRT is to eliminate NREs being thrown, omitting the guidance that folks should continue having their runtime checks in these public entrypoints is a way to achieve the opposite, IMO.
I agree with @AArnott. Also, keep in mind that nullable reference types don't generate throw ArgumentNullException(...) code -- developers are still expected to write this manually which makes me believe that extracting that to helper methods would still make sense.
But until then, I still see a lot of value in guarding against null values "at the door" so that we don't throw NRE at arbitrary places later.
I think that's kind of the point. Over time (and by that I mean ~5+ years from now), the flow of data through a program where NRTs were used should obviate the use of a null-checker API for the vast majority of users. I don't think we'll ever get to where they'll never be used, but it's certainly a goal to get to the point where very few people would have to reach for such an API because their compiler isn't smart enough. Otherwise, if someone's having to reach for a null-checker API all the time in the future, is this feature really helping them?
If the point of NRT is to eliminate NREs being thrown, omitting the guidance that folks should continue having their runtime checks in these public entrypoints is a way to achieve the opposite, IMO.
I agree with this. The goal isn't for library authors to avoid practices they've used before, and we've continued with null checks in CoreFX for exactly this reason. Though one thing worth considering in the future is if we introduce a way to give a warning when referencing a component that assumes you're nullable-aware. In other words, if a library author would rather not guard against nulls and require that their callers abide by their compiler, they don't have tools to at least let people know they're not operating in the right context with this component. I could imagine a scenario like that in the future, but today the status quo hasn't materially changes for library authors aside from having a few tools to satisfy the compiler with.