101 lines
3.6 KiB
Markdown
101 lines
3.6 KiB
Markdown
|
|
# C# Language Design Meeting for September 4, 2019
|
|
|
|
## Agenda
|
|
|
|
1. `[AllowNull]` on properties
|
|
|
|
## Discussion
|
|
|
|
[https://github.com/dotnet/roslyn/issues/37313](https://github.com/dotnet/roslyn/issues/37313)
|
|
|
|
### AllowNull and flow analysis
|
|
|
|
Some questions have come up concerning `AllowNull` and property analysis, like in the following example:
|
|
|
|
```C#
|
|
class Program
|
|
{
|
|
private string _f = string.Empty;
|
|
|
|
[AllowNull]
|
|
public string P
|
|
{
|
|
get => _f;
|
|
set => _f = value ?? string.Empty;
|
|
}
|
|
|
|
static void Main(string[] args)
|
|
{
|
|
var p = new Program();
|
|
p.P = null; // No warning, as expected
|
|
Console.Write(p.P.Length); // unexpected warning
|
|
}
|
|
}
|
|
```
|
|
|
|
The tricky part here is that we are tracking flow state for properties, meaning if `null` is stored in a
|
|
property, we treat that property as having a null state. Although there is an attribute, the flow state
|
|
of the variable wins, meaning that we think there is a `null` inside `P`, even though the stated type
|
|
is `string`. Similarly, if you invert the attribute and use `[NotNull]` on a `string?`, the flow state
|
|
will win again.
|
|
|
|
There a couple ideas to address the problem, including
|
|
|
|
1. Don't flow-track non-nullable fields/properties.
|
|
2. Have `[AllowNull]` suppress both the warning, and suppress setting the flow state to `null`
|
|
3. Have the `[NotNull]` attribute suppress the flow state (which it currently doesn't), and require
|
|
the property be written with a nullable type (`string?`) and have `[NotNull]` on the getter.
|
|
4. Stop tracking when any nullability attributes are present on properties.
|
|
|
|
|
|
2 and 3 are somewhat related and we could do both, in theory. The main problem is that it greatly
|
|
complicates the user's understanding of when flow tracking is enabled, and there are also potentially
|
|
downstream affects for type inference if we allow the rule for (2) to apply to parameters.
|
|
|
|
For (1) it seems plausible, since we would still have a warning on assigning a null to a non-nullable
|
|
member. This would remove the flow-based subsequent warnings, but the user would still be warned at
|
|
the point where the problem happens. However, it doesn't seem to solve the problem for generics, e.g.
|
|
|
|
```C#
|
|
class C<T> where T : new()
|
|
{
|
|
private T _f = new T();
|
|
|
|
[AllowNull]
|
|
public T P
|
|
{
|
|
get => _f ?? throw new Exception();
|
|
set
|
|
{
|
|
if (value is null)
|
|
{
|
|
throw new Exception();
|
|
}
|
|
return _f;
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
Here the generic property `P` cannot be marked nullable because it is unconstrained.
|
|
|
|
However, 2 & 3 also have a problem around `MaybeNull`. If a property is annotated `MaybeNull`, then
|
|
the attribute would override the state, meaning that a null check is useless. If you check for null
|
|
it would not matter, because when you read the property again, the attribute would override the state
|
|
and the result would still be maybe null.
|
|
|
|
An idea to address this is a combination of (4) and (3), where we have special attribute behavior
|
|
for properties, and in that case `NotNull` has precedence over nullable state, but nullable state
|
|
has precedence over `MaybeNull`. In addition, `AllowNull` modifies the state transition to use
|
|
the declared state if the input is null, while it otherwise uses the non-null state.
|
|
|
|
**Conclusion**
|
|
|
|
The proposal starting point is:
|
|
|
|
NotNull wins over tracked state, which wins over MaybeNull.
|
|
AllowNull transforms an incoming maybe null state to the declared state.
|
|
|
|
There's an action item to go investigate how this will play into the rest nullability, and an open
|
|
question of whether to treat fields like properties, or like local variables. |