Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field nullability revisions #9184

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Field nullability revisions #9184

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9714b35
Simplify
RikkiGibson Feb 27, 2025
44fd11f
Updates
RikkiGibson Feb 27, 2025
9420558
design for suppressions
RikkiGibson Mar 4, 2025
aa4fbfa
Revision to make field always nullable
RikkiGibson Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions proposals/field-keyword.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,18 @@ See [Glossary](#glossary) for definitions of new terms.
The *backing field* has the same type as the property. However, its nullable annotation may differ from the property. To determine this nullable annotation, we introduce the concept of *null-resilience*. *Null-resilience* intuitively means that the property's `get` accessor preserves null-safety even when the field contains the `default` value for its type.

A *field-backed property* is determined to be *null-resilient* or not by performing a special nullable analysis of its `get` accessor.
- For the purposes of this analysis, `field` is temporarily assumed to have *annotated* nullability, e.g. `string?`. This causes `field` to have *maybe-null* or *maybe-default* initial state in the `get` accessor, depending on its type.
- Then, if nullable analysis of the getter yields no nullable warnings, the property is *null-resilient*. Otherwise, it is not *null-resilient*.
- Two separate nullable analysis passes are performed: one where the field is assumed to have *annotated* nullability, and one where the field is assumed to have *not-annotated* nullability. The nullable diagnostics resulting from each analysis are recorded.
- If there is a nullable diagnostic in the pass where the field was *annotated*, which was not present in the pass where the field is *not-annotated*, then the property is not null-resilient. Otherwise, it is null-resilient.
- The implementation can optimize by first performing the pass where the field was *annotated*. If this results in no diagnostics at all, then the property is null-resilient and the *not-annotated* pass can be skipped.
- 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.
Copy link
Contributor

@jnm2 jnm2 Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?? "";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@RikkiGibson RikkiGibson Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#### Constructor analysis

Expand Down