csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

[Proposal]: Improved Definite Assignment Analysis

Open RikkiGibson opened this issue 3 years ago • 2 comments

Improved Definite Assignment Analysis

  • [x] Proposed
  • [x] Prototype: Completed
  • [x] Implementation: Completed
  • [x] Specification: Completed

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

  • https://github.com/dotnet/csharplang/discussions/801
  • https://github.com/dotnet/roslyn/issues/45582
    • Links to 4 other issues where people were affected by this.
if ((c != null && c.M(out object obj1)) == true)
{
    obj1.ToString(); // undesired error
}

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.

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

Conditional access coalesced to a bool constant

  • https://github.com/dotnet/csharplang/discussions/916
  • https://github.com/dotnet/csharplang/issues/3365

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

  • https://github.com/dotnet/roslyn/issues/4272

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

  • [ ] There are impacts on switch expressions that should be specified: https://github.com/dotnet/csharplang/discussions/4240#discussioncomment-343395

  • [ ] It could be useful to also allow certain expressions using lifted relational operators to benefit: https://github.com/dotnet/roslyn/pull/52425#discussion_r610938838

public class C {
    public int M0(object obj) => 0;

    public static void M1(C? c) {
        int x, y;
        _ = c?.M0(x = y = 0) <= 0
            ? x.ToString() // no warning
            : y.ToString(); // warning
    }
}

Design meetings

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

RikkiGibson avatar Feb 25 '21 00:02 RikkiGibson

The feature is now implemented and merged into the compiler.

RikkiGibson avatar Jun 11 '21 00:06 RikkiGibson

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

jnm2 avatar Jan 06 '24 17:01 jnm2