Responding to LDM feedback

proposals/null-arg-checking.md

@@ -1,4 +1,4 @@
-# Simplified Null Argument Checknig
+# Simplified Null Argument Checking
 
 ## Summary
 This proposal provides a simplified syntax for validating method arguments are not `null` and 
@@ -12,6 +12,8 @@ gave us the desire to explore a minimal syntax for argument `null` validation in
 anticipated to pair often with NRT the proposal is independent of it. 
 
 ## Detailed Design 
+
+### Null validation
 The bang operator, `!`, can be positioned after any identifier in a parameter list and this will 
 cause the C# compilet to emit standard `null` checking code for that parameter. For example:
 
@@ -48,43 +50,167 @@ void G<T>(T arg!) {
 }
 ```
 
-In the case of a constructor that uses `this` or `base` to chain to another constructor the 
-`null` validation will occur after such a call. 
+In the case of a constructor the `null` validation will occur before any other code in the constructor. That includes: 
+
+- Chaining to other constructors with `this` or `base` 
+- Field initializers which implicitly occur in the constructor
 
 ``` csharp
 class C {
-    C(string name!) :this(name) {
+    string field = GetString();
+    C(string name!): this(name) {
         ...
     }
 }
 ```
 
-Will be translated into:
+Will be roughly translated into the following:
 
 ``` csharp
 class C {
-    C(string name!) :this(name) {
+    C(string name)
         if (name is null) {
             throw new ArgumentNullException(nameof(name));
         }
+        field = GetString();
+        :this(name);
         ...
-    }
 }
 ```
 
+Note: this is not legal C# code but instead just an approximation of what the implementation does. 
+
 The `!` operator can only be used for parameter lists which have an associated method body. This
 means it cannot be used in an `abstract` method, `interface`, `delegate` or `partial` method 
 definition.
 
+### Extending is null
+The types for which the expression `is null` is valid will be extended to include unconstrained type parameters. This 
+will allow it to fill the intent of checking for `null` on all types which a `null` check is valid. That is types 
+which are not definitely known to be value types. Type parameters which are constrained to `struct` cannot be 
+used with this syntax.
+
+``` csharp
+void NullCheck<T1, T2>(T1 p1, T2 p2) where T2 : struct {
+    if (p1 is null) {
+        ...
+    }
+
+    // Error 
+    if (p2 is null) { 
+        ...
+    }
+}
+```
+
+The behavior of `is null` on a type parameter will be the same as `== null` today. In the cases where the type parameter
+is instantiated as a value type the code will be evaluated as `false`. For cases where it is a reference type the 
+code will do a proper `is null` check.
+
+### Intersection with Nullable Reference Types
+Any parameter which has a `!` operator applied to it's name will start with the nullable state being not `null`. This is
+true even if the type of the parameter itself is potentially `null`. That can occur with an explicitly nullable type, 
+such as say `string?`, or with an unconstrained type parameter. 
+
+When a `!` operator on parameters is combined with an explicitly nullable type on the parameter then a warning will
+be issued by the compiler:
+
+``` csharp
+void WarnCase<T>(
+    string? name!, // Warning: combining explicit null checking with a nullable type
+    T value1 // Okay
+)
+```
+
 ## Open Issuess
-- There is no way to check the `value` argument of a property setter for `null` using this syntax. 
+None
 
 ## Considerations
--  In the case a constructor chains to another constructor with `this` or `base` the `null` 
-validation could occur before the chaining call. This is a stronger guarantee the developer could 
-desire. However C# code cannot be authored that way today and it seems unlikely that it's a 
-significant issue. Leaving the check after the chaining call means existing code can be ported
-to the new pattern without fear of compat concerns.
+
+### Constructors
+The code generation for constructors means there is a small, but observable, behavior change when moving from standard
+`null` validation today and the `null` validation parameter syntax (`!`). The `null` check in standard validation 
+occurs after both field initializers and any `base` or `this` calls. This means a developer can't necessarily migrate
+100% of their `null` validation to the new syntax. Constructors at least require some inspection.
+
+After discussion though it was decided that this is very unlikely to cause any significant adoption issues. It's more
+logical that the `null` check run before any logic in the constructor does. Can revisit if significant compat issues
+are discovered.
+
+### Warning when mixing ? and !
+There was a lengthy discussion on whether or not a warning should be issued when the `!` syntax is applied to a
+parameter which is explicitly typed to a nullable type. On the surface it seems like a non-sensical declaration by 
+the developer but there are cases where type hierarchies could force developers into such a situation. 
+
+Consider the following class hierarchy across a series of assemlbies (assuming all are compiled with `null` checking
+enabled):
+
+``` csharp
+// Assembly1
+abstract class C1 {
+    protected abstract void M(object o); 
+}
+
+// Assembly2
+abstract class C2 : C1 {
+
+}
+
+// Assembly3
+abstract class C3 : C2 { 
+    protected override void M(object o!) {
+        ...
+    }
+}
+```
+
+Here the author of `C3` decided to add `null` validation to the parameter `o`. This is completely in line with how the
+feature is intended to be used.
+
+Now imagine at a later date the author of Assembly2 decides to add the following override:
+
+``` csharp
+// Assembly2
+abstract class C2 : C1 {
+   protected override void M(object? o) { 
+       ...
+   }
+}
+```
+
+This is allowed by nullable reference types as it's legal to make the contract more flexible for input positions. The 
+NRT feature in general allows for reasonable co/contravariance on parameter / return nullability. However the language
+does the co/contravariance checking based on the most specific override, not the original declaration. This means the
+author of Assembly3 will get a warning about the type of `o` not matching and will need to change the signature to the
+following to eliminate it: 
+
+``` csharp
+// Assembly3
+abstract class C3 : C2 { 
+    protected override void M(object? o!) {
+        ...
+    }
+}
+```
+
+At this point the author of Assembly3 has a few choices:
+
+- They can accept / suppress the warning about `object?` and `object` mismatch.
+- They can accept / suppress the warning about `object?` and `!` mismatch.
+- They can just remove the `null` validation check (delete `!` and do explicit checking)
+
+This is a real scenario but for now the idea is to move forward with the warning. If it turns out the warning happens
+more frequently than we anticipate then we can remove it later (the reverse is not true).
+
+### Implicit property setter arguments
+The `value` argument of a parameter is implicit and does not appear in any parameter list. That means it cannot be a 
+target of this feature. The property setter syntax could be extended to include a parameter list to allow the `!` 
+operator to be applied. But that cuts against the idea of this feature making `null` validation simpler. As such the 
+implicit `value` argument just won't work with this feature.
 
 ## Future Considerations
-None
+None
+
+- iterators
+- ctors
+- lambda parameters