Test plan for "Parameter null-checking"

[jcouv]

This is a place to collect test ideas.

Proposal: dotnet/csharplang#2145

Specification

• Specification checked in to csharplang link
• Confirm codegen (use helper method from BCL, and/or fallback to generating if/throw)

Infrastructure

• LangVersion check
• IOperation/CFG nodes

Parsing

• string s !!= null parsed as string s!! = null

Generics

• void M<T>(T value!) { } is OK
• void M<T>(T value!) where T : struct { } is an error
• void M<T>(T value!) where T : unmanaged { } is an error
• void M<T>(T value!) where T : notnull { } is OK
• void M<T>(T value!) where T : class { } is OK
• void M<T>(T value!) where T : SomeStruct { } is an error
• void M<T>(T value!) where T : SomeClass { } is OK

Iterators

Null checking should always happen in initial call, not lowered methods

• Top-level iterator method returning IEnumerable
• Top-level iterator method returning IEnumerator
• Local function returning IEnumerable
• Local function return IEnumerator

Constructors

Null checking should be the first thing in a constructor

• Constructor with field initializers
• Constructor with this call
• Constructor with base call
• Record and record struct primary constructors

Nested functions

• Null-checked lambda parameter
• Null-checked local function parameter
• Null-checked lambda parameter inside field initializer
• allowed in anonymous method syntax delegate (object o!!) { }
• allowed in lambda parameter without explicit type (with and without parentheses) x!! => x, (x, y!!) => y
• allowed in lambda discard parameter _!! => { }

Warnings and errors

• Warn on null-checked parameter with default null value
• Warn on string? x! (null-check doesn't make sense on those)
• Error on int x! parameters
• Error on out string x! (null-check doesn't make sense on out parameter)
• Error on null-check on declarations without bodies (interface members, abstract, extern, declaration portion of a partial method)

Misc.

• uses reference equality (user-defined conversions are ignored)
• no boxing in check for S? s!! for struct type S
• check argument in getter and setter of indexer

Misc (from discussion in LDM)

• Null-checked params parameter (allowed, checks the array, not the elements)
• test void M<T>(T t!) where T : notnull { ... } (ok)
• should we use helper type from BCL to throw exception?
• Null-checked pointers and function pointers (works as expected)
• error on __arglist!!

From test plan review

• confirm CFG design with API review (delaying resolution till after feature merge) Parameter null-checking public API #58307
• LDM: allow on lambda parameter without type or parentheses? resolution: Allow.
• LDM: allow on lambda discard parameter? resolution: Disallow. (delaying till after feature merge) Disallow null checking discard parameters #58952
• async iterator methods
• LDM: allow on ref or in parameters? resolution: Allow. Allow 'ref' and 'in' parameters to be null-checked #58938
• check for unconstrained T does not box when T is a nullable value type (include a ThrowIfNull<T>(T value, string parameterName) helper method?) (delaying till after feature merge)
  • We don't box, but we also don't call the 'Throw' helper, which could improve inlining behavior of methods with null checked Nullable<T> parameters.
• disallow in delegate declaration
• override and interface implementation with/without !! is allowed
• SymbolDisplay is not affected
• IDE: classification, formatting, SyntaxNormalizer, quick info, typing, completion

Productivity

• Verify formatting in IDE
• verify that IDE doesn't gray out null-checked parameter which is otherwise unused
• need an analyzer/fixer to convert codebase (delaying till after feature merge) Parameter null-checking analyzer and fixer #58182
• Update F1 service a la Handle more cases in F1 help service #60156

[jcouv]

I added a bunch of test scenarios from today's LDM decision.

[333fred]

Need to consider (_!!, _!!) => {} behavior.

[jcouv]

From discussion with BCL and runtime folks, we've landed on:

• Roslyn injects an internal ThrowIfNull(object argument, string argumentName) helper into every assembly that uses the feature, similar to how it injects helpers into assemblies for other purposes, like switching on strings (SharpLab) or use of nullability annotations (SharpLab)
• We are free to liberally use !! throughout dotnet/runtime to replace places we currently do if (arg == null) throw new ArgumentNullException(nameof(arg)); for argument checking.
• We can choose to also introduce a public ThrowIfNull helper for devs to use directly, but it’s unrelated to !!.

[hez2010]

I think this shouldn't emit a null reference return warning: https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQAcoKgW1QOzgBsiBjACwlIGsBLfAcwBoYQE58AfAYkJKmBEIAAgj4BQgLAAoAAIAmAIwyZsxQAZhAUXzJEEAHIB7GAeJEAFGvUB+YcgCEDgJQyA3jOFfhsgOz2AbhkAXxU5RQBOCx09JGNTcwsAIkAeDcAQfaTnZwCgA

[Youssef1313]

@hez2010 It looks like there are few attributes that should be generated by the compiler but are not currently.:

 [CompilerGenerated]
 internal sealed class <PrivateImplementationDetails>
 {
+    [DoesNotReturn]
     internal static void Throw(string paramName)
     {
         throw new ArgumentNullException(paramName);
     }

-    internal static void ThrowIfNull(object argument, string paramName)
+    internal static void ThrowIfNull([NotNull] object? argument, string paramName)
     {
         if (argument == null)
         {
             Throw(paramName);
         }
     }
 }

This may help the flow analysis to work properly. But @RikkiGibson can confirm whether I'm correct.

[RikkiGibson]

If the functions were explicitly called they would need the attributes you've indicated, @Youssef1313. However, the Throw/ThrowIfNull functions don't exist in the "initial" bound tree used by nullable analysis. They're only produced during lowering. I think we would want to solve this by ensuring the "initial state" of a parameter with !! is properly updated to not-null.

Thanks for catching this issue @hez2010!

[Youssef1313]

@jaredpar Should this be closed?