From eec56c088efc2e0b982cc84b5b6a0102ff9df8dc Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Wed, 4 Jun 2025 05:17:21 -0700 Subject: [PATCH] [flow analysis] Mark `false` branches of trivial `is` tests unreachable. When an `is` test is trivially satisfied (i.e. `expr is T`, when the static type of `expr` is a subtype of `T`), the `is` test is guaranteed by soundness to evaluate to `true`, so any code path that follows from the `is` test evaluating to `false` is unreachable. This reasoning wasn't valid prior to sound null safety, because in mixed mode programs, it was possible for an expression to evaluate to `null` even if its static type wasn't nullable, and hence `expr is T` might evaluate to `false` even if the static type of `expr` was a subtype of `T`. So this change is gated on the `sound-flow-analysis` language flag (which is enabled in Dart 3.9). Fixes https://github.com/dart-lang/sdk/issues/60718. Change-Id: I66a65580b738162f23b6fb468b71fcac66bfbb95 Bug: https://github.com/dart-lang/sdk/issues/60718 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431740 Commit-Queue: Paul Berry Reviewed-by: Konstantin Shcheglov --- .../lib/src/flow_analysis/flow_analysis.dart | 36 +++- .../flow_analysis/flow_analysis_test.dart | 63 ++++++- ...ly_satisfied_type_check_disabled_test.dart | 160 ++++++++++++++++++ .../trivially_satisfied_type_check_test.dart | 160 ++++++++++++++++++ 4 files changed, 413 insertions(+), 6 deletions(-) create mode 100644 tests/language/sound_flow_analysis/trivially_satisfied_type_check_disabled_test.dart create mode 100644 tests/language/sound_flow_analysis/trivially_satisfied_type_check_test.dart diff --git a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart index fa068069a94..ccd1a264406 100644 --- a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart +++ b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart @@ -3137,10 +3137,16 @@ class FlowModel { Type factoredType = helper.typeOperations.factor(previousType, type); Type? typeIfFalse; + bool ifFalseIsUnreachable = false; if (helper.typeOperations.isBottomType(factoredType)) { - // Promoting to `Never` would mark the code as unreachable. But it might - // be reachable due to mixed mode unsoundness. So don't promote. + // Do not promote to `Never` (even if it would be sound to do so); it's + // not useful. typeIfFalse = null; + if (helper.typeAnalyzerOptions.soundFlowAnalysisEnabled) { + ifFalseIsUnreachable = true; + } else { + // The code path might be reachable due to mixed mode unsoundness. + } } else if (!helper.isValidPromotionStep( previousType: previousType, newType: factoredType, @@ -3158,6 +3164,10 @@ class FlowModel { typeIfFalse, ); + if (ifFalseIsUnreachable) { + ifFalse = ifFalse.setUnreachable(); + } + return new ExpressionInfo( type: helper.boolType, ifTrue: ifTrue, @@ -5791,6 +5801,11 @@ class _FlowAnalysisImpl< isExpression, isNot ? expressionInfo._invert() : expressionInfo, ); + } else if (_isTypeCheckGuaranteedToSucceedWithSoundNullSafety( + staticType: subExpressionType, + checkedType: checkedType, + )) { + booleanLiteral(isExpression, !isNot); } } } @@ -7229,6 +7244,23 @@ class _FlowAnalysisImpl< } } + /// Determines whether an expression having the given [staticType] is + /// guaranteed to fail an `is` or `as` check using [checkedType] due to sound + /// null safety. + /// + /// If [TypeAnalyzerOptions.soundFlowAnalysisEnabled] is `false`, this method + /// will return `false` regardless of its input. This reflects the fact that + /// in language versions prior to the introduction of sound flow analysis, + /// flow analysis assumed that the program might be executing in unsound null + /// safety mode. + bool _isTypeCheckGuaranteedToSucceedWithSoundNullSafety({ + required Type staticType, + required Type checkedType, + }) { + if (!typeAnalyzerOptions.soundFlowAnalysisEnabled) return false; + return typeOperations.isSubtypeOf(staticType, checkedType); + } + FlowModel _join(FlowModel? first, FlowModel? second) => FlowModel.join(this, first, second); diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart index fbc4509f5ac..979b9cc5e7b 100644 --- a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart +++ b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart @@ -1611,6 +1611,8 @@ main() { String? expectedPromotedTypeThen, String? expectedPromotedTypeElse, { bool inverted = false, + bool expectedReachableThen = true, + bool expectedReachableElse = true, }) { var x = Var('x'); late SsaNode ssaBeforePromotion; @@ -1620,12 +1622,12 @@ main() { if_( x.is_(tryPromoteType, isInverted: inverted), [ - checkReachable(true), + checkReachable(expectedReachableThen), checkPromoted(x, expectedPromotedTypeThen), getSsaNodes((nodes) => expect(nodes[x], same(ssaBeforePromotion))), ], [ - checkReachable(true), + checkReachable(expectedReachableElse), checkPromoted(x, expectedPromotedTypeElse), getSsaNodes((nodes) => expect(nodes[x], same(ssaBeforePromotion))), ], @@ -1642,11 +1644,18 @@ main() { }); test('isExpression_end does not promote to a supertype', () { - _checkIs('int', 'int?', null, null); + _checkIs('int', 'int?', null, null, expectedReachableElse: false); }); test('isExpression_end does not promote to a supertype, inverted', () { - _checkIs('int', 'int?', null, null, inverted: true); + _checkIs( + 'int', + 'int?', + null, + null, + inverted: true, + expectedReachableThen: false, + ); }); test('isExpression_end does not promote to an unrelated type', () { @@ -12631,6 +12640,52 @@ main() { checkPromoted(x, 'num'), ]); }); + + group('False branch for trivially satisfied "is" test:', () { + group('When enabled, sets unreachable:', () { + test('Promotable target', () { + var x = Var('x'); + h.run([ + declare(x, initializer: expr('int')), + if_(x.is_('int', isInverted: true), [ + checkNotPromoted(x), + checkReachable(false), + ]), + ]); + }); + + test('Non-promotable target', () { + h.run([ + if_(expr('int').is_('int', isInverted: true), [ + checkReachable(false), + ]), + ]); + }); + }); + + group('When disabled, leaves reachable:', () { + test('Promotable target', () { + h.disableSoundFlowAnalysis(); + var x = Var('x'); + h.run([ + declare(x, initializer: expr('int')), + if_(x.is_('int', isInverted: true), [ + checkNotPromoted(x), + checkReachable(true), + ]), + ]); + }); + + test('Non-promotable target', () { + h.disableSoundFlowAnalysis(); + h.run([ + if_(expr('int').is_('int', isInverted: true), [ + checkReachable(true), + ]), + ]); + }); + }); + }); }); group('Demotion and type of interest promotion:', () { diff --git a/tests/language/sound_flow_analysis/trivially_satisfied_type_check_disabled_test.dart b/tests/language/sound_flow_analysis/trivially_satisfied_type_check_disabled_test.dart new file mode 100644 index 00000000000..8a0ab5de277 --- /dev/null +++ b/tests/language/sound_flow_analysis/trivially_satisfied_type_check_disabled_test.dart @@ -0,0 +1,160 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Exercises flow analysis of a trivially satisfied type check (an `is` test +// that is guaranteed to succeed) when `sound-flow-analysis` is disabled. + +// @dart = 3.8 + +import '../static_type_helper.dart'; + +// If `x` is of type `T`, flow analysis does not consider `x is T` to be +// guaranteed to evaluate to `true`. +testIsExact({required int intValue, required int Function() intFunction}) { + { + // is int + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intValue is int) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } + + { + // is int + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intFunction() is int) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } +} + +// If `x` is of type `T`, flow analysis does not consider `x is! T` to be +// guaranteed to evaluate to `false`. +testIsNotExact({required int intValue, required int Function() intFunction}) { + { + // is! int + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intValue is! int) { + shouldBeDemoted1 = null; + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } + + { + // is! int + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intFunction() is! int) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } +} + +// If `x` is of type `T`, and `T <: U`, flow analysis does not consider `x is U` +// to be guaranteed to evaluate to `true`. +testIsSupertype({required int intValue, required int Function() intFunction}) { + { + // is num + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intValue is num) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } + + { + // is num + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intFunction() is num) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } +} + +// If `x` is of type `T`, and `T <: U`, flow analysis does not consider `x is! +// U` to be guaranteed to evaluate to `false`. +testIsNotSupertype({ + required int intValue, + required int Function() intFunction, +}) { + { + // is! num + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intValue is! num) { + shouldBeDemoted1 = null; + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } + + { + // is! num + int? shouldBeDemoted1 = 0; + int? shouldBeDemoted2 = 0; + if (intFunction() is! num) { + shouldBeDemoted1 = null; + } else { + shouldBeDemoted2 = null; + } + shouldBeDemoted1.expectStaticType>(); + shouldBeDemoted2.expectStaticType>(); + } +} + +main() { + testIsExact(intValue: 0, intFunction: () => 0); + testIsNotExact(intValue: 0, intFunction: () => 0); + testIsSupertype(intValue: 0, intFunction: () => 0); + testIsNotSupertype(intValue: 0, intFunction: () => 0); +} diff --git a/tests/language/sound_flow_analysis/trivially_satisfied_type_check_test.dart b/tests/language/sound_flow_analysis/trivially_satisfied_type_check_test.dart new file mode 100644 index 00000000000..f2ff8311ba3 --- /dev/null +++ b/tests/language/sound_flow_analysis/trivially_satisfied_type_check_test.dart @@ -0,0 +1,160 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Exercises flow analysis of a trivially satisfied type check (an `is` test +// that is guaranteed to succeed) when `sound-flow-analysis` is enabled. + +// SharedOptions=--enable-experiment=sound-flow-analysis + +import '../static_type_helper.dart'; + +// If `x` is of type `T`, flow analysis considers `x is T` to be guaranteed to +// evaluate to `true`. +testIsExact({required int intValue, required int Function() intFunction}) { + { + // is int + int? shouldBePromoted; + int? shouldNotBeDemoted = 0; + if (intValue is int) { + shouldBePromoted = 0; // Reachable + } else { + shouldNotBeDemoted = null; // Unreachable + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } + shouldBePromoted.expectStaticType>(); + shouldNotBeDemoted.expectStaticType>(); + } + + { + // is int + int? shouldBePromoted; + int? shouldNotBeDemoted = 0; + if (intFunction() is int) { + shouldBePromoted = 0; // Reachable + } else { + shouldNotBeDemoted = null; // Unreachable + } + shouldBePromoted.expectStaticType>(); + shouldNotBeDemoted.expectStaticType>(); + } +} + +// If `x` is of type `T`, flow analysis considers `x is! T` to be guaranteed +// to evaluate to `false`. +testIsNotExact({required int intValue, required int Function() intFunction}) { + { + // is! int + int? shouldNotBeDemoted = 0; + int? shouldBePromoted; + if (intValue is! int) { + shouldNotBeDemoted = null; // Unreachable + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } else { + shouldBePromoted = 0; // Reachable + } + shouldNotBeDemoted.expectStaticType>(); + shouldBePromoted.expectStaticType>(); + } + + { + // is! int + int? shouldNotBeDemoted = 0; + int? shouldBePromoted; + if (intFunction() is! int) { + shouldNotBeDemoted = null; // Unreachable + } else { + shouldBePromoted = 0; // Reachable + } + shouldNotBeDemoted.expectStaticType>(); + shouldBePromoted.expectStaticType>(); + } +} + +// If `x` is of type `T`, and `T <: U`, flow analysis considers `x is U` to be +// guaranteed to evaluate to `true`. +testIsSupertype({required int intValue, required int Function() intFunction}) { + { + // is num + int? shouldBePromoted; + int? shouldNotBeDemoted = 0; + if (intValue is num) { + shouldBePromoted = 0; // Reachable + } else { + shouldNotBeDemoted = null; // Unreachable + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } + shouldBePromoted.expectStaticType>(); + shouldNotBeDemoted.expectStaticType>(); + } + + { + // is num + int? shouldBePromoted; + int? shouldNotBeDemoted = 0; + if (intFunction() is num) { + shouldBePromoted = 0; // Reachable + } else { + shouldNotBeDemoted = null; // Unreachable + } + shouldBePromoted.expectStaticType>(); + shouldNotBeDemoted.expectStaticType>(); + } +} + +// If `x` is of type `T`, and `T <: U`, flow analysis considers `x is! U` to be +// guaranteed to evaluate to `false`. +testIsNotSupertype({ + required int intValue, + required int Function() intFunction, +}) { + { + // is! num + int? shouldNotBeDemoted = 0; + int? shouldBePromoted; + if (intValue is! num) { + shouldNotBeDemoted = null; // Unreachable + // `intValue` is not promoted to `Never`. Note: since + // `Never.expectStaticType` will silently succeed, we check this + // by first wrapping `intValue` in a list, and then checking the type of + // the list. + [intValue].expectStaticType>>(); + } else { + shouldBePromoted = 0; // Reachable + } + shouldNotBeDemoted.expectStaticType>(); + shouldBePromoted.expectStaticType>(); + } + + { + // is! num + int? shouldNotBeDemoted = 0; + int? shouldBePromoted; + if (intFunction() is! num) { + shouldNotBeDemoted = null; // Unreachable + } else { + shouldBePromoted = 0; // Reachable + } + shouldNotBeDemoted.expectStaticType>(); + shouldBePromoted.expectStaticType>(); + } +} + +main() { + testIsExact(intValue: 0, intFunction: () => 0); + testIsNotExact(intValue: 0, intFunction: () => 0); + testIsSupertype(intValue: 0, intFunction: () => 0); + testIsNotSupertype(intValue: 0, intFunction: () => 0); +}