Short term field keyword nullable analysis

[RikkiGibson]

Part of #75244

I found that the dotnet/csharplang#8425 did not really include enough details to attack the attribute problem.

Basically, we have a precedent with auto properties using nullable attributes, that the field is subject to those same attributes in the same way. For example, [AllowNull] string Prop { get; set; } = null; is valid, even though the = null is assigning to the backing field rather than the auto-prop which claims to accept null.

However, our goal with the field keyword nullability proposal was to allow customizing the field's nullability independently of the property. Therefore, I have arrived at the following principle:

• For classic auto properties, i.e. field-backed properties which don't use the field keyword, field: targeted nullability attributes are ignored, and the attributes from the property are used for the field instead. Essentially, the field is not a distinct thing from the property for the purposes of nullable analysis.
  • The goal here is avoid any behavior or diagnostic changes for existing auto-property scenarios in the short term. Warn when auto-property setter is more tolerant than getter #50244 suggests making changes to existing scenarios to improve safety.
• For properties using field keyword, the field is a distinct thing. field: targeted nullability attributes are used on the field, and nullability attributes on the property are not applied to the field.
  • Therefore, [AllowNull] string Prop { get => field; set => field = value } = null; results in a warning--since the initializer is assigning to the field, and the AllowNull doesn't apply to the field.
  • However, we still view the property as a proxy to the field in a constructor. This is accomplished by sharing a slot between the property and the backing field in constructors. In other words, within a constructor, whatever flow state the property has, we assume the field also has that same state, and any state changes to either one are reflected in both. This means the right thing happens whether field/property assignment is used, or if (Prop is null) throw ...; is used, to ensure that the property is in the correct state before exiting the constructor.
  • We use the property's nullability attributes to decide what values can be assigned to the property, when the language defines that the property is the thing being assigned--such as when a property with a setter is assigned in a constructor. Similarly, the field's nullability attributes decide what values can be assigned to the field when the field is the thing being assigned, such as in a property initializer.

The short term solution reflects the need for user to specify whether the field is nullable or not independently of the property. When we introduce a solution which infers the field's nullability across members, we expect the need for the user to specify will disappear in most cases.

[RikkiGibson]

Test failure appears to be just the following

[xUnit.net 00:00:47.12]     Microsoft.CodeAnalysis.CSharp.UnitTests.DiagnosticAnalyzerTests.TestDescriptorForConfigurableCompilerDiagnostics [FAIL]
  Failed Microsoft.CodeAnalysis.CSharp.UnitTests.DiagnosticAnalyzerTests.TestDescriptorForConfigurableCompilerDiagnostics [7 ms]
  Error Message:
   Add resource string named 'WRN_UninitializedNonNullableBackingField_Title' for Title of 'CS9264' to 'CSharpResources'
Expected: True
Actual:   False
  Stack Trace:
     at Microsoft.CodeAnalysis.CSharp.UnitTests.DiagnosticAnalyzerTests.TestDescriptorForConfigurableCompilerDiagnostics() in /_/src/Compilers/CSharp/Test/Emit3/Diagnostics/DiagnosticAnalyzerTests.cs:line 733

   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

I will try to address tomorrow.

[cston]

constructor.IsStatic == symbol.IsStatic

Are we testing this case? That is, are we testing use of static property in an instance constructor?

[RikkiGibson]

This check is only really going to be relevant in a scenario like the following:

1. static property is used in instance constructor, and
2. somehow, backing field of the same static property is also used in instance constructor. I think this is not possible.

I am adding some tests to poke at that area, but, even if the tests don't show the necessity of the check, I think I will feel more comfortable having it there.

[333fred]

Hmm. Existing problem, but you can't exactly mark Prop as required...

[cston]

Nullability_07

Minor: It seems like Nullability_08 covers this test. Similar comment for _07_Getter_only.

[RikkiGibson]

The goal of this separation was to test the constructor exit checks separately from testing field flow state being reflected when reading the property.

[cston]

// 1

Minor: Consider moving // 1 to setter.

[cston]

Nullability_20_ManualGetter_AutoSetter_NullTest

Is this distinct from the previous test? (What are we testing with the addition of if (Prop is null) throw null!?)

[RikkiGibson]

The goal is to demonstrate that a null-test+throw on the property puts a non-nullable field into the correct state. Adding a version without initializer to demonstrate that more clearly.