dotnet / csharplang

The official repo for the design of the C# programming language
11.61k stars 1.03k forks source link

[Proposal]: Improved Definite Assignment Analysis #4465

Open RikkiGibson opened 3 years ago

RikkiGibson commented 3 years ago

Improved Definite Assignment Analysis

Summary

Definite assignment analysis as specified has a few gaps which have caused users inconvenience. In particular, scenarios involving comparison to boolean constants, conditional-access, and null coalescing.

Related discussions and issues

csharplang discussion of this proposal: #4240

Probably a dozen or so user reports can be found via this or similar queries (i.e. search for "definite assignment" instead of "CS0165", or search in csharplang). https://github.com/dotnet/roslyn/issues?q=is%3Aclosed+is%3Aissue+label%3A%22Resolution-By+Design%22+cs0165

I have included related issues in the scenarios below to give a sense of the relative impact of each scenario.

Scenarios

As a point of reference, let's start with a well-known "happy case" that does work in definite assignment and in nullable.

#nullable enable

C c = new C();
if (c != null && c.M(out object obj0))
{
    obj0.ToString(); // ok
}

public class C
{
    public bool M(out object obj)
    {
        obj = new object();
        return true;
    }
}

Comparison to bool constant

if ((c != null && c.M(out object obj2)) is true) { obj2.ToString(); // undesired error }


### Comparison between a conditional access and a constant value

- https://github.com/dotnet/roslyn/issues/33559
- https://github.com/dotnet/csharplang/discussions/4214
- https://github.com/dotnet/csharplang/issues/3659
- https://github.com/dotnet/csharplang/issues/3485
- https://github.com/dotnet/csharplang/issues/3659

This scenario is probably the biggest one. We do support this in nullable but not in definite assignment.

```cs
if (c?.M(out object obj3) == true)
{
    obj3.ToString(); // undesired error
}

Conditional access coalesced to a bool constant

This scenario is very similar to the previous one. This is also supported in nullable but not in definite assignment.

if (c?.M(out object obj4) ?? false)
{
    obj4.ToString(); // undesired error
}

Conditional expressions where one arm is a bool constant

It's worth pointing out that we already have special behavior for when the condition expression is constant (i.e. true ? a : b). We just unconditionally visit the arm indicated by the constant condition and ignore the other arm.

Also note that we haven't handled this scenario in nullable.

if (c != null ? c.M(out object obj4) : false)
{
    obj4.ToString(); // undesired error
}

Specification

The specification has moved to https://github.com/dotnet/csharplang/blob/master/proposals/improved-definite-assignment.md

Drawbacks

It feels odd to have the analysis "reach down" and have special recognition of conditional accesses, when typically flow analysis state is supposed to propagate upward. We are concerned about how a solution like this could intersect painfully with possible future language features that do null checks.

Alternatives

Two alternatives to this proposal:

  1. Introduce "state when null" and "state when not null" to the language and compiler. This has been judged to be too much effort for the scenarios we are trying to solve, but that we could potentially implement the above proposal and then move to a "state when null/not null" model later on without breaking people.
  2. Do nothing.

Unresolved questions

Design meetings

https://github.com/dotnet/csharplang/discussions/4243

RikkiGibson commented 3 years ago

The feature is now implemented and merged into the compiler.

jnm2 commented 11 months ago

Could this be extended to work in switch expressions, a natural evolution from conditional expressions?

// Already works
public string ValidationMessage => validationResult?.IsError(out var errorMessage) is true ? errorMessage : null

// But this does not
public string ValidationMessage => validationResult?.IsError(out var errorMessage) switch
{
    // ❌ CS0165 Use of unassigned local variable 'errorMessage'
    //      ↓↓↓↓↓↓↓↓↓↓↓↓
    true => errorMessage,
    false => $"The backup path has been validated by {instanceName}",
    _ => "Checking the backup path..."
};