Implement the "sound flow analysis" feature #60438

eernstg · 2025-03-31T14:25:41Z

This issue tracks the implementation of the sound flow analysis as discussed in dart-lang/language#3100.

eernstg · 2025-03-31T14:28:26Z

@stereotype441, do we need several issues in order to track this effort?

stereotype441 · 2025-03-31T16:39:30Z

@stereotype441, do we need several issues in order to track this effort?

One issue seems sufficient to me 😃. Thanks for filing this!

…sis. Previously, flow analysis was configured by passing a set of named booleans to its constructor, each to enable or disable a separate language feature. This change merges the flow analysis configuration with the `TypeAnalyzerOptions` class (which was already being used for configuring the shared `TypeAnalyzer` class). This should make it easier to add language features to the shared code base in the future, since there will be one common place where all features will be configured. To avoid duplicating the logic that creates `TypeAnalyzerOptions`, I've had to do a bit of minor surgery to the clients: - In the test harness in `_fe_analyzer_shared`, there is a common method (`Harness.computeTypeAnalyzerOptions`) that constructs `TypeAnalyzerOptions` based on the harness configuration. It is called from a few different tests. - In `pkg/analyzer`, there is a common method (`computeTypeAnalyzerOptions`) that constructs `TypeAnalyzerOptions` based on a `FeatureSet`. It is used by `LibraryAnalyzer.analyzeForCompletion`, `LibraryAnalyzer._resolveFile`, and the late variable `AstResolver._typeAnalyzerOptions`. - In `pkg/front_end`, I've moved computation of `TypeAnalyzerOptions` from the `InferenceVisitorImpl` constructor to the `TypeInferrerImpl` constructor; the options are then passed to the `InferenceVisitorImpl` by `_createInferenceVisitor`. I'm doing this work now as preparation for adding support for sound flow analysis (#60438), so that I can add the logic to enable it in a clean way. Bug: #60438 Change-Id: Ib845194adb404b4c0a3feeff17a14ae641d515eb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419940 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This change updates the flow analysis logic for `is` and `as` expressions so that when the language feature `sound-flow-analysis` is enabled, the static type of the operand is compared to the type to the right of the `is` or `as` keyword. If one of the types is non-nullable and the other type is `Null`, then the type test is known to fail. For an `as` expression, this means that the code path following the expression will be marked as unreachable. For an `is` expression, this means that any code paths that assume it evaluates to `true` will be marked as unreachable. Note that these new behaviors break assumptions made by three pre-existing flow analysis tests. I was able to adjust one of the tests ("equalityOp_end does not set reachability for `this`") to preserve its old behavior. The other two tests became redundant, so I removed them. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ib3a9e96bd39cf7df4c6c297568763c0f25bc9e39 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420164 Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]> Commit-Queue: Paul Berry <[email protected]>

The class `_EqualityCheckResult`, and its subtypes, were introduced prior to support for patterns. This change makes `_EqualityCheckResult` into a sealed class and changes the logic that consumes it to use `switch` statements rather than `is` tests. The resulting logic is equivalent, but I believe it's easier to read. This paves the way for adding a new kind of `_EqualityCheckResult`, which I'll need to do to support the new `sound-flow-analysis` feature. (The new kind of `_EqualityCheckResult` will be used in the circumstance where the two operands of an equality comparison are guaranteed to be unequal because of their types). Bug: #60438 Change-Id: I217da37847beff5a43bddb5ed734cc3e4c74970a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420184 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This change updates the flow analysis logic for `==` and `!=` expressions, `==` and `!=` patterns, and constant patterns, so that when the language feature `sound-flow-analysis` is enabled, the static types of the two expressions being compared are checked for nullability. If one of the types is non-nullable and the other type is `Null`, then it is known that the values will be unequal. Note that these new behaviors break assumptions made by several pre-existing flow analysis tests. I was able to adjust some of the tests to preserve their old behavior, either by adjusting expectations or running the test with `sound-flow-analysis` disabled. Some other tests became redundant, so I removed them. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ib65477e064bb8dcd761542ebe187843fe265a24b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420463 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]>

This change updates the flow analysis logic for `?.` expressions, so that when the language feature `sound-flow-analysis` is enabled, the static type of the target is checked for nullability. If it's non-nullable, then the "shortcut" control flow path (the control flow path in which the null-aware operation is not executed) is considered unreachable. One pre-existing flow analysis test was made redundant by this change. Another needed a minor tweak to continue passing. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: I3cd92dd49d4393b40f0ec888b643f0353de5e2d4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420466 Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This change updates the flow analysis logic for `??` and `??=` expressions, so that when the language feature `sound-flow-analysis` is enabled, the static type of the left hand side is checked for nullability. If it's non-nullable, then the right hand side of the expresison is considered unreachable. These new behaviors break assumptions made by two pre-existing flow analysis tests. I changed those tests to run with `sound-flow-analysis` disabled. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: I33d6d256bd3c41b764245f50ad34eb5c8b33878e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420740 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]>

It turns out that the flow analysis logic for null-aware map entries has always presumed sound null safety. That is, given a map entry with a null-aware key (`{?x: y}`), flow analysis assumed that if the key was non-nullable, then the value was guaranteed to execute. However, another piece of logic that could have been implemented, and wasn't, was that if the key had static type `Null`, then the value was guaranteed _not_ to execute. This logic would have been sound even without assuming sound null safety (because even in unsound null safety mode, the type `Null` was only inhabited by the value `null`). This change implements the missing logic. Even though it doesn't strictly depend on the assumption sound null safety, it still makes sense to guard it by the `sound-flow-analysis` flag, because (a) it's a potentially breaking change, and (b) it brings the flow analysis behavior of null-aware map entries into alignment with the other behaviors that are being implemented as part of the `sound-flow-analysis` feature. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ie711f582660a31a411be0dc339995df140feb04f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420800 Reviewed-by: Chloe Stefantsova <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This change updates the flow analysis logic for null check patterns, so that when the language feature `sound-flow-analysis` is enabled, the matched value type is checked for nullability. If it's non-nullable, then the null check pattern is known to succeed. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: Ie68d98d95e30053f992a3f8db6ccdd3978960eb7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421583 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>

This change updates the flow analysis logic for map patterns, so that when the language feature `sound-flow-analysis` is enabled, an empty map pattern is considered to match a non-nullable map. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: I3f5a79c00cfe91d37528a3790b5cedb9a8f010fc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421584 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This change updates the logic in the flow analysis method `promoteForPattern`, so that when the language feature `sound-flow-analysis` is enabled, the following additional behaviors are added: - If the matched value type is non-nullable, and the pattern implicitly performs an `is Null` test, then the pattern is known not to match. - If the matched value type is `Null`, and the pattern implicitly performs an `is T` test, where `T` is a non-nullable type, then the pattern is known not to match. Note that this reasoning step is sound regardless of whether the program is running with sound null safety enabled, but since it is a new reasoning step, it only takes place if the `sound-flow-analysis` feature is enabled. There is no behavioral change if the feature `sound-flow-analysis` is disabled. Bug: #60438 Change-Id: I5a6e8def050c95b6c1ad01d37584d17a0cd590c8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421900 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>

These unit tests exercise flow analysis behaviors for patterns that one might plausibly assume are part of the `sound-flow-analysis` feature, but actually have been present ever since the `patterns` feature was introduced. Adding these tests helps me be confident that the behavior of flow analysis after `sound-flow-analysis` has all of the soundness behaviors I expect; even though some of those behaviors aren't new. Bug: #60438 Change-Id: I77a269907c67d643d0ef23d4f76545e36761bbfc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421960 Reviewed-by: Chloe Stefantsova <[email protected]> Commit-Queue: Paul Berry <[email protected]>

Prior to this change, a `do` loop such as this one: do { return; } while (true); would be flagged with two DEAD_CODE warnings: one covering the text `} while (true);` (which makes sense), and one covering the text `do {` (which doesn't make sense at all, since the top of the `do` loop is reachable). Digging through the code and git history, I've found no good reason for the DEAD_CODE warning at `do {` to exist. It looks like it was added as part of a bug fix from an external contributor and not spotted during code review (https://dart-review.googlesource.com/c/sdk/+/266389). With the unnecessary dead code range removed, some of the tests of the analysis server's `remove_dead_code` fix become simpler (they don't need to use an `errorFilter` to select which dead code range to act on). Others become unnecessary (since they were verifying that `remove_dead_code` properly handled the bogus dead code range). Others simply needed their `errorFilter` changed to select the appropriate dead code range. Making this fix paves the way for improving the `remove_dead_code` fix, which I want to do as part of shipping the `sound-flow-analysis` feature. Change-Id: Iee0bc187441bc4be76a8fa57198155dff3731bc8 Bug: #60438 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/423421 Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Samuel Rawlins <[email protected]> Commit-Queue: Paul Berry <[email protected]>

The tests that don't end in `_disabled_test.dart` exercise all the new behaviors introduced with the `sound-flow-analysis` feature. The tests that _do_ end in `_disabled_test.dart` verify that when the language version is 3.8, the old behavior of flow analysis is preserved. For the sake of thoroughness, I've also elected to include tests for a few behaviors that aren't new, just because they seem logically related to the new behaviors (for example, in the tests for the `==` and `!=` operator, I've included a test that `null == null` is known to evaluate to `true`, even though this behavior existed even before the `sound-flow-analysis` feature was implemented). These tests are marked with the comment "(this behavior predates sound-flow-analysis)" in the `_disabled_test.dart` files. Bug: #60438 Change-Id: If2895355bb1a2eeb2415a5a1012d73ae87c244dd Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/423040 Reviewed-by: Erik Ernst <[email protected]> Commit-Queue: Paul Berry <[email protected]>

This class is part of the public API, and contains the functionality that's common to ForElement and ForStatement. Adding this class allows us to change the type of `ForLoopParts.parent` from `AstNode?` to `ForLoop`. I intend to make use of this in a follow-up CL that makes improvements to the "remove dead code" fix, as part of my work on sound flow analysis. Change-Id: I93ca15d2eeea495064b70690584b7a91058510f7 Bug: #60438 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/423560 Reviewed-by: Konstantin Shcheglov <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Paul Berry <[email protected]>

Special care has to be taken when reporting dead code for `for` statements and `for` elements, because the "updaters" part of a for-loop appears before the body in source, but is executed after the body. This means that if the end of the loop body is unreachable, then a separate dead_code range might need to be generated to cover the updaters. Previously, this was accomplished by special-case logic in `NullSafetyDeadCodeVerifier.flowEnd`: if the parent of `_firstDeadNode` was a `ForStatement` (or a `Block` inside a `ForStatement`), extra logic would be triggered to emit a dead_code range to cover the updaters. This mostly worked, but it didn't handle a `Block` inside a `Block` inside a `ForStatement`, and I couldn't figure out a good way to generalize it to handle `ForElement`s. The new approach is for the resolver to make two additional calls into `NullSafetyDeadCodeVerifier`, just after visiting the "condition" part of the loop and just before visiting the updaters. Combining together information gathered at these two times, the `NullSafetyDeadCodeVerifier` can determine whether or not it's necessary to emit a separate dead_code range to cover the updaters. Pre-existing tests in `dead_code_test.dart` still pass, and I've added a bunch of tests of the new functionality. These improvements pave the way for improvements to the analysis server's "remove dead code" fix, which I want to do in order to prepare for the rollout of sound flow analysis. Bug: #60438 Change-Id: I6d2c43fd7c06adcd4de22d42144b033319e949a3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/424180 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>

Previously, `RemoveDeadCode` worked by starting with the `coveringNode` (the largest node that fully contains the dead code warning), and then using pattern matching to handle special cases where the span of the dead code warning didn't precisely match a single AST node. I'm working on adding functionality to `RemoveDeadCode` to support the new sound flow analysis feature, and I've found this approach difficult to build on, because the span of a dead_code warning is (by design) not precisely associated with a single AST node. This change reworks `RemoveDeadCode` based on the insight that the most important thing about a dead_code warning is where in the source code it _begins_. So it starts by finding the smallest AST node that contains the beginning of the dead code range, and then checks whether the dead code range starts at the beginning of that node. If it _doesn't_, that only happens in a few special cases, so they are handled in an ad hoc fashion: - The node is a binary `||`, `&&`, or `??` operation whose RHS is dead (in this case the dead code range starts with the binary operator). - The node is a block whose statements are all live, but whose end is dead (in this case the dead code range starts with the `}` of the block). - The node is a `do` statement whose body is live, but whose condition is dead (in this case the dead code range starts with the `while` keyword). In the remaining cases, where the dead code range starts at the beginning of the node that was found, `RemoveDeadCode` starts by walking up the syntax tree to see if any ancestor nodes are fully dead; if they are, it moves to the biggest fully dead ancestor. Then it determines whether the resulting node can be removed by looking at its type and the type of its parent. (For example, a `Statement` can be removed if its parent is a `Block`). There are a few notable behavioral differences from the previous implementation: - Previously, `RemoveDeadCode` would try to remove a dead `Statement` regardless of the type of its parent, so `if (false) return;` would get transformed into `if (false)`, changing the meaning of the code that follows. - Dead code elimination now works just as well for `ForElement`s as it does for `ForStatement`s. I've added test cases to cover these new behaviors. In a follow-up commit, I will add more functionality to `RemoveDeadCode` to specifically address the kinds of dead code that are expected to occur when sound flow analysis is enabled. Bug: #60438 Change-Id: I3c2428c905358a881a7eee9a0f06f48270980695 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/424200 Reviewed-by: Samuel Rawlins <[email protected]> Commit-Queue: Paul Berry <[email protected]>

…-analysis. The following improvements are made to the "remove dead code" quick fix: - Dead code removal can now replace conditional expressions like `condition ? live : dead` or `condition ? dead : live` with just the live subexpression, provided that the condition appears free of side effects. - Dead code removal can now replace `if` statements like `if (condition) live; else dead;` or `if (condition) dead; else live;` with just the live statement, provided that the condition appears free of side effects. - Dead code removal can now fully remove `if` statements like `if (condition) dead;`, provided that the condition appears free of side effects. Extra care is taken around opening and closing braces to try to preserve reasonable formatting as much as possible. These features were chosen by examining the set of new dead code warnings that pop up inside google3 if the sound-flow-analysis language feature is enabled. They are sufficient to allow 98% of those new warnings to be addressed by the quick fix. This should help customers adjust their code quickly when moving to a language version that enables this language feature. The notion that a condition "appears free of side effects" is defined to include casts, non-null assertions, and getter invocations. It is of course possible that a getter invocation, a cast, or a non-null assertion could have a side effect that is important, but it would be difficult to build the necessary static analysis to detect these cases, and in practice, they don't happen very often. Considering that the "remove dead code" quick fix is not usable in an automated fashion (it must be explicitly requested by the user in their editor for each piece of affected dead code), I believe this is a good tradeoff. Bug: #60438 Change-Id: Ied43500c4b7a729bbd686708ab10e80e72f22088 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425180 Commit-Queue: Paul Berry <[email protected]> Reviewed-by: Samuel Rawlins <[email protected]>

eernstg added area-dart-model For issues related to conformance to the language spec in the parser, compilers or the CLI analyzer. implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) labels Mar 31, 2025

eernstg mentioned this issue Mar 31, 2025

Change flow analysis to assume sound null safety once unsound null safety support is fully removed dart-lang/language#3100

Open

stereotype441 self-assigned this Mar 31, 2025

stereotype441 changed the title ~~Implement the sound flow analysis~~ Implement the "sound flow analysis" feature Apr 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the "sound flow analysis" feature #60438

Implement the "sound flow analysis" feature #60438

eernstg commented Mar 31, 2025 •

edited

Loading

eernstg commented Mar 31, 2025

stereotype441 commented Mar 31, 2025

Implement the "sound flow analysis" feature #60438

Implement the "sound flow analysis" feature #60438

Comments

eernstg commented Mar 31, 2025 • edited Loading

eernstg commented Mar 31, 2025

stereotype441 commented Mar 31, 2025

eernstg commented Mar 31, 2025 •

edited

Loading