Flag additional code as unreachable due to types Null and Never

[stereotype441]

Change

In present-day Dart, the following flow analysis behaviours only take effect if the expression e is a reference to a local variable:

• Control flow after an expression of the form e ?? other or e ??= other, where e has static type Null and other has static type Never, is considered unreachable.
• Control flow predicated on an expression of the form e is Never evaluating to true is considered unreachable.
• Control flow predicated on an expression of the form e is! Never evaluating to false is considered unreachable.
• Control flow on the RHS of a null-aware access such as e?.property..., e?.property = ... or e?.method(...), where e has static type Null, is considered unreachable (Note: this can arise in the presence of extension methods).

I propose to change this so that these behaviours take effect regardless of the form of the expression e.

Rationale

There are two motivations for this change:

• Improved user experience. Flagging code as unreachable helps users spot bugs. There's no principled reason why the above behaviours should have only applied to local variable references; generalizing the logic to apply to all expressions allows us to help users spot more mistakes.
• Tool evolution. The flow analysis portion of the language implementation is complex, and it's going to get more complex with the addition of field promotion (Enable promotion for private final fields and fields on non-escaping private classes language#2020). This change simplifies the implementation of flow analysis (by allowing the notions of reachability and promotability to be more cleanly separated); this in turn should simplify the implementation of field promotion.

Impact

The primary expected user impact would be the appearance of "unreachable code" hints that did not appear previously. A secondary possible impact would be that type inference would assign a more accurate type to an implicitly typed variable, as a result of an unreachable code branch being excluded from analysis; this could in turn cause static errors, or (in highly contrived scenarios) runtime failures. I've prototyped this change in Google's internal codebase, and it caused zero visible change, so both of these impacts are expected to be rare.

Mitigation

If a user gets a new "unreachable code" hint, they may either delete the unreachable code or ignore the error (e.g. using an // ignore comment). In the unlikely event that unreachable code detection leads to a change in type inference, and that in turn leads to a static error or runtime failure, the user can mitigate the problem through the use of an explicit type.

(Note: previous discussion here: dart-lang/language#2394)

[itsjustkevin]

@vsmenon @grouma @Hixie what are your thoughts on this breaking change request?

[Hixie]

I don't understand how this is breaking. It seems like an obvious win with no downside.

[stereotype441]

I don't understand how this is breaking. It seems like an obvious win with no downside.

@Hixie The breakage scenarios are very minor:

1. A user could have their continuous integration set up to fail the build in the event of an "unreachable code" hint, in which case code like this would fail to build:

foo(int Function() f) {
  if (f() is Never) {
    print('unreachable');
  }
}

I suppose you could argue that this is only a breakage in a very pedantic technical sense.

2. Code that was previously not known to be unreachable might have contributed to type analysis; once it is recognized as unreachable the type analysis might change in a way that breaks other things. For example:

foo(int Function() f) {
  int? i;
  if (f() is Never) {
    print('hello'); // (1)
  } else {
    i = 0;
  }
  var j = i; // (2)
  j = null; // (3)
}

Before the change, (1) is not recognized as unreachable, therefore at (2), i is considered to be potentially null, so the inferred type of j is int?. Therefore the assignment at (3) is valid.

After the change, (1) is recognized to be unreachable, therefore at (2), i is considered to be promoted to non-nullable int, so the inferred type of j is int. Therefore the assignment at (3) is invalid.

Technically this is breaking since code that used to comple and execute without a problem now contains a compile-time error. But I would be very surprised if a situation like this arose in practice.

[Hixie]

Fair enough. Seems fine to me either way. :-)

[itsjustkevin]

Friendly bump @grouma and @vsmenon

[vsmenon]

lgtm

[grouma]

LGTM

[itsjustkevin]

Marking this breaking change request approved based on the comments.

[stereotype441]

As of b7567b1 this change has landed.

[stereotype441]

Note: this change wound up having a slightly larger effect than anticipated: as of b7567b1, a type test of the form v is Never (where v is a local variable) no longer promotes v to type Never.

This a small enough change that I don't think it's necessary to re-open the breaking change process, but I wanted it documented here. I've sent https://dart-review.googlesource.com/c/sdk/+/256880 for review, to update the CHANGELOG and add a test that exercises the new behavior.