Flow analysis: do not treat null vars as equivalent to `null`.
When analyzing code like this with the
`allowLocalBooleanVarsToPromote` flag enabled, don't treat a variable
containing `null` as equivalent to a literal `null`. For example:
f(int? x) {
var y = null;
if (x != y) {
print(x + 1); // ERROR: x must be null checked.
}
}
The rationale is that flow analysis doesn't promote variables that are
known to be `null` to the `Null` type, so it seems like it would be
inconsistent if we used the fact that they're `null` to promote other
variables.
Bug: https://github.com/dart-lang/language/issues/1274
Change-Id: Icfa14104936a26732353921e3a29a840c53d4d52
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176560
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
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 818914f..2ef2bf1 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
@@ -298,7 +298,7 @@
ExpressionInfo<Variable, Type> invert() =>
new ExpressionInfo<Variable, Type>(after, ifFalse, ifTrue);
- ExpressionInfo<Variable, Type> rebaseForward(
+ ExpressionInfo<Variable, Type>? rebaseForward(
TypeOperations<Variable, Type> typeOperations,
FlowModel<Variable, Type> base) =>
new ExpressionInfo(base, ifTrue.rebaseForward(typeOperations, base),
@@ -3757,11 +3757,11 @@
_storeExpressionVariable(expression, variable);
VariableModel<Variable, Type> variableModel = _current.infoFor(variable);
if (allowLocalBooleanVarsToPromote) {
- ExpressionInfo<Variable, Type>? expressionInfo =
- variableModel.ssaNode?.expressionInfo;
+ ExpressionInfo<Variable, Type>? expressionInfo = variableModel
+ .ssaNode?.expressionInfo
+ ?.rebaseForward(typeOperations, _current);
if (expressionInfo != null) {
- _storeExpressionInfo(
- expression, expressionInfo.rebaseForward(typeOperations, _current));
+ _storeExpressionInfo(expression, expressionInfo);
}
}
return variableModel.promotedTypes?.last;
@@ -3951,10 +3951,10 @@
}
@override
- ExpressionInfo<Variable, Type> rebaseForward(
+ ExpressionInfo<Variable, Type>? rebaseForward(
TypeOperations<Variable, Type> typeOperations,
FlowModel<Variable, Type> base) =>
- new _NullInfo(base);
+ null;
}
/// [_FlowContext] representing a language construct for which flow analysis
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 e9740e2..f8c1de7 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
@@ -2586,18 +2586,11 @@
]);
});
- test('variableRead() restores the notion of whether a value is null', () {
- // This is not a necessary part of the design of
- // https://github.com/dart-lang/language/issues/1274; it's more of a happy
- // accident of how it was implemented: since we store an ExpressionInfo
- // object in the SSA node to keep track of the consequences of the
- // variable's value on flow analysis, and since the _NullInfo type is a
- // subtype of ExpressionInfo, that means that when a literal `null` is
- // written to a variable, and we read it later, we recognize the value as
- // being `null` for purposes of comparison to other other variables. Even
- // though this feature is a happy accident of the implementation strategy,
- // it's important to test it to make sure it doesn't regress, since users
- // might come to rely on it.
+ test("variableRead() doesn't restore the notion of whether a value is null",
+ () {
+ // Note: we have the available infrastructure to do this if we want, but
+ // we think it will give an inconsistent feel because comparisons like
+ // `if (i == null)` *don't* promote.
var h = Harness(allowLocalBooleanVarsToPromote: true);
var x = Var('x', 'int?');
var y = Var('y', 'int?');
@@ -2611,7 +2604,9 @@
checkNotPromoted(x),
checkNotPromoted(y),
], [
- checkPromoted(x, 'int'),
+ // Even though x != y and y is known to contain the value `null`, we
+ // don't promote x.
+ checkNotPromoted(x),
checkNotPromoted(y),
]),
]);
