Uneachable code / Dead code on nested if

[FMorschel]

Describe the issue
There are two lints which could be used for this situation. Most probably Uneachable code.

To Reproduce
If I have a code similar to this:

void fn(final bool variable) {
  if (variable == true) {
    print('doing something');
    if (variable == false) {
      print('doing something else');
    }
  }
}

Expected behavior
I expected the block inside if (variable == false) would be warned with Uneachable code. I've tried with non-final variables first and I think that could be a case where this is true if I can replace the value of the variable (but shouldn't dart analysis see that that was an Uneachable code in that case as well, since I've not updated the value). But with final variables that is not possible at all.

[lrhn]

This requires special knowledge about bool, to know that there is no value which is equal to both true and false.

That's not unreasonable, we have and (will) use that information in other places, like switches over bool values.
(In that regard, bool can be treated like an enum with two values with primitive equality, so we know that every bool value is equal to precisely one of those two.)

[srawlins]

I think this would have been caught (or... there was aspiration that this would have been caught) by the now defunct invariant_booleans linter rule. It became really bogged down with bugs, false positives, false negatives, etc.

I don't see us implementing support for such dead code any time soon.

But I've thought about it: one such neat way to implement it robustly (hopefully) would be if Dart's flow analysis code had hooks and/or an attribute map on which we could hang information. Because implementing checks like this well require flow analysis, but it would be a shame to implement flow analysis n times for n lint rules + language features.

[bwilkerson]

@stereotype441 Should we create an area-fe-analyzer-shared to cover issues related to code that's shared between the two?

[stereotype441]

@srawlins said:

But I've thought about it: one such neat way to implement it robustly (hopefully) would be if Dart's flow analysis code had hooks and/or an attribute map on which we could hang information. Because implementing checks like this well require flow analysis, but it would be a shame to implement flow analysis n times for n lint rules + language features.

and @bwilkerson said:

@stereotype441 Should we create an area-fe-analyzer-shared to cover issues related to code that's shared between the two?

I have no objection to an area-fe-analyzer-shared tag in general, however as to this specific issue I'm not sure that implementing this logic in flow analysis is necessarily the best approach.

Flow analysis is highly constrained in what it can do effectively for four major reasons:

• It has to operate in parallel with type inference, meaning it has to fit into the single-pass design of the type inference engine in the CFE and the analyzer. In addition to adding complexity, this means, for instance, that it has to do an approximate analysis of loops, because it can't visit the body of a loop multiple times and iterate to a fixed point.
• It has to draw all its conclusions based purely on local information (so that changes in one part of the program don't cause unexpected changes to inferred types elsewhere). For example, in a mixed-mode program we can't guarantee that nonNullableValue != null is true; but since we don't want inferred types to depend on whether the program is mixed-mode, flow analysis has to assume that nonNullableValue != null might be false, even in fully sound programs.
• It doesn't have any special understanding of methods and types declared outside of the SDK. This means, for example, that it can't make use of behavioural invariants like "this method always returns false if its argument is null" or "this method won't invoke its callback after it returns".
• Any improvements to flow analysis have to be gated on the language version, so that old published packages don't acquire compile-time errors when they are analyzed by a new version of Dart.

Also, flow analysis contains extra complexity that's not present in other parts of the analyzer because it has to be able to work with both the analyzer and CFE representations of code.

I have long dreamt of adding a facility to the analyzer that does basic block analysis of a function, reduces each basic block to a set of equations involving types and boolean values, and then iterates those equations to a fixed point. This analysis wouldn't have any of the drawbacks above, and could be used as the basis for lints and dead code analysis, but not for compile-time errors. Ideally, we would design it with an eye toward adding more sophisticated "symbolic execution" features in the future, such as verifying function preconditions and postconditions, detecting out-of-range array accesses, and checking that the program doesn't violate important Flutter design invariants. I've had some hallway discussions with @bwilkerson and @pq about this, and I hope to sketch out a brief design proposal for it once my work on Dart 3.0 is wrapped up.

My preference would be to consider feature requests like this one as design goals for this new analysis facility, rather than candidates for inclusion in flow analysis.

[bwilkerson]

That sound reasonable to me.

[srawlins]

Yeah this sounds good to me. It makes sense that we shouldn't hook into the (currently) only flow analysis engine we have. It'd be great to have an additional flow analysis system that we can use when calculating diagnostics like these to answer "can X have been modified within this block at this statement?" or "has X only been assigned non-nullable values at this statement?"

I imagine this will be written in a 3rd party plugin before the analyzer proper.