Field nullability revisions #9184
Conversation
| - If the property does not have a get accessor, it is (vacuously) null-resilient. | ||
| - If the get accessor is auto-implemented, the property is not null-resilient. | ||
|
|
||
| The nullability of the backing field is determined as follows: | ||
| - If the field has nullability attributes such as `[field: MaybeNull]`, `AllowNull`, `NotNull`, or `DisallowNull`, then the field's nullable annotation is the same as the property's nullable annotation. | ||
| - This is because when the user starts applying nullability attributes to the field, we no longer want to infer anything, we just want the nullability to be *what the user said*. | ||
| - If the containing property has ***oblivious*** or ***annotated*** nullability, then the backing field has the same nullability as the property. | ||
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`) or has the `[NotNull]` attribute, and the property is ***null-resilient***, then the backing field has ***annotated*** nullability. | ||
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`) or has the `[NotNull]` attribute, and the property is ***not null-resilient***, then the backing field has ***not-annotated*** nullability. |
There was a problem hiding this comment.
I guess the only difference this change makes is whether this becomes a constructor warning requiring initialization instead of a "possible null reference return" warning on get?
[NotNull]
public string? Prop
{
get;
set => field = value ?? "";
}There was a problem hiding this comment.
Yeah I think so. And yes I decided that this isn’t a scenario where we need to do inference. Also, in the case of unconstrained T, inferring not nullable field may not be sufficient for things to work satisfactorily here. You’re already expressing things in a “wonky” way, go ahead and call it [AllowNull] string instead, or, attribute the field so that it works how you want.
proposals/field-keyword.md
Outdated
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`) or has the `[NotNull]` attribute, and the property is ***null-resilient***, then the backing field has ***annotated*** nullability. | ||
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`) or has the `[NotNull]` attribute, and the property is ***not null-resilient***, then the backing field has ***not-annotated*** nullability. | ||
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`), and the property is ***null-resilient***, then the backing field has ***annotated*** nullability. | ||
| - If the containing property has *not-annotated* nullability (e.g. `string` or `T`), and the property is ***not null-resilient***, then the backing field has ***not-annotated*** nullability. |
There was a problem hiding this comment.
Consider an alternative approach where we say that the backing field is always annotated if we get into this section. This doesn't change our ability to decide if the get works with an unannotated return type, it just changes how we emit the field. I'm curious what specific things break, from a user and roslyn API perspective, if the field is always annotated?
There was a problem hiding this comment.
I've been turning this idea over in my mind quite a bit and I think this is actually going to simplify implementation and improve clarity.
it just changes how we emit the field
This seems fine. If we say a backing field of reference type is always nullable, when field keyword is used, it would affect some reflection behaviors, but I would be surprised if it was anything that breaking. EF, for example, does not look at the backing field's nullability, only the property's nullability.
I'm curious what specific things break, from a user and roslyn API perspective, if the field is always annotated?
Basically it means that for field in string Prop { get => field; }, Quick Info shows string? Prop.field, instead of string Prop.field. I actually think this is a good thing. Currently the implementation always shows string Prop.field, even when we inferred a nullable annotation for it.
The current design was based on the idea that if we just infer the nullable annotation, then constructor analysis will just do the right thing, in terms of knowing whether to enforce initialization of the property. If field is string?, then no need for enforcement. If it is string!, then need to enforce.
My stretch goal was to make public API show the inferred annotation. But if we do that, I'm concerned about "Schrodinger's null", where assigning maybe-null to the field actually changes its type from string to string?. This seems confusing, as the point of saying its type is string is to convey to the user that they are not allowed to put null in it.
If we go with "field is always annotated", then we need to adjust the constructor analysis design so that it uses the "null resilient" concept for these backing fields instead. "Null-resilient" means no need for enforcement. "Not null-resilient" means need to enforce. But that's not a big deal.
There was a problem hiding this comment.
Also, the current design is to give the field the same nullability as the property, when any nullability attributes are used on the field. If we said the field is "always annotated when reference type", then we would probably want to do that even when attributes are used on the field.
This may break some existing cases where only a precondition or only postcondition attribute is used. Cases where both are specified (e.g. [DisallowNull, NotNull] won't change. I do not think such a break is problematic.
| - If the property does not have a get accessor, it is (vacuously) null-resilient. | ||
| - If the get accessor is auto-implemented, the property is not null-resilient. | ||
|
|
||
| Null-forgiving (`!`) operators, directives like `#nullable disable` and `#pragma disable warning`, and conventional project-level settings like `<NoWarn>`, are all respected when deciding if a nullable analysis has diagnostics. [DiagnosticSuppressors](https://github.com/dotnet/roslyn/blob/a91d7700db4a8b5da626d272d371477c6975f10e/docs/analyzers/DiagnosticSuppressorDesign.md) are not respected when deciding if a nullable analysis has diagnostics. |
| - If the associated property's nullable annotation is *annotated* or *oblivious*, then the initial flow state is the same as the initial flow state of the associated property. | ||
| - If the associated property's nullable annotation is *not-annotated*, then: | ||
| - If the property is *null-resilient*, the initial flow state is the *uninitialized* flow state (e.g. maybe-null, or maybe-default for unconstrained `T`). | ||
| - If the property is not *null-resilient*, the initial flow state is the *initialized* flow state (e.g. not-null, or maybe-null for unconstrained `T`). | ||
|
|
||
| #### Constructor analysis | ||
|
|
||
| Currently, an auto property is treated very similarly to an ordinary field in [nullable constructor analysis](nullable-constructor-analysis.md). We extend this treatment to *field-backed properties*, by treating every *field-backed property* as a proxy to its backing field. |
There was a problem hiding this comment.
Consider using a fully-qualified link: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/nullable-constructor-analysis.md
|
|
||
| #### Constructor analysis | ||
|
|
||
| Currently, an auto property is treated very similarly to an ordinary field in [nullable constructor analysis](nullable-constructor-analysis.md). We extend this treatment to *field-backed properties*, by treating every *field-backed property* as a proxy to its backing field. | ||
|
|
||
| We update the following spec language from the previous [proposed approach](nullable-constructor-analysis.md#proposed-approach) to accomplish this: | ||
| We update the following spec language from the previous [proposed approach](csharp-9.0/nullable-constructor-analysis.md#proposed-approach) to accomplish this (new language in **bold**): |
| Instead of inferring an initial flow state for the `field`, we could infer its nullable annotation instead; this seems to make some pieces related to constructor analysis fall into place a little more easily. However, we found that this had some undesirable costs: | ||
| 1) "Schrodinger's null". It was felt to be confusing that a backing field displayed in the IDE like `string Prop.field`, could suddenly change to `string? Prop.field`, because the user assigned null to it. Its type being `string` implies that null can't be assigned to it. | ||
| 2) Exposing an inferred nullable annotation through the symbol APIs, internally and externally, was found to be more difficult from an engineering perspective, compared to simply setting an initial flow state. There's a risk that accessing the `FieldSymbol.TypeWithAnnotations`, `IFieldSymbol.Type`, or similar APIs, could result in the binding and flow analysis pulling on those same APIs, in simple or mutual recursion, overflowing the stack at runtime and crashing the host process. | ||
| - We also considered spec'ing and using this inferred annotation anyways, without exposing it in the usual symbol APIs, which seems again to undermine clarity, as a property displaying as `string Prop.field` can again have null written to it, have maybe-null initial state, etc. |
From simply trying the feature out I decided that 2-pass is needed. Also simplified some language relating to
[NotNull]on the property which didn't seem to be necessary in practice.