Emitting Debug.Assert(e != null) via null-supression operator

[gafter]

@alrz commented on Tue Jan 15 2019

When we're passing a e! where e is not the literal null, we expect it to be not null at that point but the flow analysis couldn't infer that, so I think we should emit Assert(e != null) to catch an unexpected null early, rather than silently passing it through until a thought-to-be-safe dereference occurs.

---

@gafter commented on Wed Jan 16 2019

This is a request to change the planned language specification for the nullable feature.

[arekbal]

Why are these features implemented through runtime checks instead of static flow analysis that wouldn't allow invocation of such attributed methods without prove that these arguments are not null. I would understand that Reflection API would need such runtime checks... apart from that, I don't understand...

Could you guys elaborate a little why you picked that route (emitting runtime checks) instead of the one I described?

[alrz]

I would understand that Reflection API would need such runtime checks... apart from that, I don't understand...

It's about overall program state, which is not local, the compiler should go long ways to prove nullability of such values, and in a lot of cases, it can be impossible. But sometimes the programmer could know that in a certain point a value cannot (or more precisely, shouldn't) be null, that's where you use the null-suppression operator.

Since this is the programmer expectation after all, the value could still be null in practice, that's why I proposed to add an assertion to catch an unexpected null early. Without it, a null value could silently passed where a null is not assumed to be reachable normally and you get a NRE somewhere down the call stack which doesn't show the actual source of bug.

[Joe4evr]

Why do you want Debug.Assert over if (e is null) throw new ArgumentNullException(nameof(e));?

[alrz]

I'd prefer another helper completely like e! => AssertNotNull(e) but if anything, I think it should be a conditional assertion because these would be "internal" bugs rather than incorrect "client" code.

Usually, public APIs throw to inform user that an argument is not valid, but for non-public methods we only assert, because those cases should never happen in production and the assertion failure is only useful to catch bugs in tests (that I think is one of the reasons we run tests in both debug and release configurations). and we don't want the code-size and runtime overhead of these checks in the final bits.

[YairHalberstadt]

If you specifically wanted null to be able to pass through, you would have to do

value is null ? null! : value

Which should elide the assert.

Whilst this is ugly, I'm guessing that it would be uncommon enough that it's probably worth the trade-off.

[YairHalberstadt]

I don't like the language knowing about the difference between debug and release builds.

I think it would be nicer if there was a compiler switch to enable a debug assert, a release assert, or no assertion.

[alrz]

I don't like the language knowing about the difference between debug and release builds.

It doesn't. the helper we emit will have a ConditionalAttribute which binds it to some configuration. that particular setting is completely outside of the language itself.

[mikernet]

What about doing this at the start of methods with non-null parameters as well?

[alrz]

See https://github.com/dotnet/csharplang/pull/2144#issuecomment-455895834 - I'm not sure if the team is considering it or if it's possible to do it in a later version.

For suppression operators, per @jcouv's https://github.com/dotnet/roslyn/issues/31732#issuecomment-458347316

The current behavior is that ! is a suppression, not an assertion.

So I think if this ever made it, it could also affect nullable analysis in some cases.

[mikernet]

@alrz Hmmm, I see. I think a compiler flag to optionally add debug assertions anytime ! is used would be an awesome feature. Pretty sure most people would use it.

[gafter]

The semantics of the annotations introduced through the nullable reference types feature are intended to have no impact on the runtime semantics of the program. This requests that they do, but we've discussed the design approach at length and we are very comfortable where we are.