Track nonNullIntent in unconditional control flow.
In a future CL this will be used to prevent nullability from being
propagated into function parameters where it's clearly not intended.
Change-Id: I34430162db33f99e850ca2c11e5bca8736feb94f
Reviewed-on: https://dart-review.googlesource.com/c/93041
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Dan Rubel <danrubel@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
diff --git a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
index 12f4ca5..ec6f294 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
@@ -71,6 +71,12 @@
/// code that can be proven unreachable by the migration tool.
final _guards = <ConstraintVariable>[];
+ /// Indicates whether the statement or expression being visited is within
+ /// conditional control flow. If `true`, this means that the enclosing
+ /// function might complete normally without executing the current statement
+ /// or expression.
+ bool _inConditionalControlFlow = false;
+
ConstraintGatherer(TypeProvider typeProvider, this._variables,
this._constraints, this._source, this._permissive, this.assumptions)
: _notNullType = DecoratedType(typeProvider.objectType, null),
@@ -124,9 +130,9 @@
DecoratedType visitAssertStatement(AssertStatement node) {
_handleAssignment(_notNullType, node.condition);
if (identical(_conditionInfo?.condition, node.condition)) {
- // TODO(paulberry): should only do this if in unconditional control flow.
- if (_conditionInfo.trueChecksNonNull != null) {
- _recordFact(_conditionInfo.trueChecksNonNull);
+ if (!_inConditionalControlFlow &&
+ _conditionInfo.trueDemonstratesNonNullIntent != null) {
+ _recordFact(_conditionInfo.trueDemonstratesNonNullIntent);
}
}
node.message?.accept(this);
@@ -137,6 +143,7 @@
DecoratedType visitBinaryExpression(BinaryExpression node) {
switch (node.operator.type) {
case TokenType.EQ_EQ:
+ case TokenType.BANG_EQ:
assert(node.leftOperand is! NullLiteral); // TODO(paulberry)
var leftType = node.leftOperand.accept(this);
node.rightOperand.accept(this);
@@ -145,10 +152,13 @@
// TODO(paulberry): only set falseChecksNonNull in unconditional
// control flow
bool isPure = node.leftOperand is SimpleIdentifier;
- _conditionInfo = _ConditionInfo(node,
+ var conditionInfo = _ConditionInfo(node,
isPure: isPure,
trueGuard: leftType.nullable,
- falseChecksNonNull: leftType.nullAsserts);
+ falseDemonstratesNonNullIntent: leftType.nonNullIntent);
+ _conditionInfo = node.operator.type == TokenType.EQ_EQ
+ ? conditionInfo
+ : conditionInfo.not(node);
}
return _nonNullableBoolType;
case TokenType.PLUS:
@@ -234,6 +244,7 @@
// TODO(paulberry): should the use of a boolean in an if-statement be
// treated like an implicit `assert(b != null)`? Probably.
_handleAssignment(_notNullType, node.condition);
+ _inConditionalControlFlow = true;
ConstraintVariable trueGuard;
ConstraintVariable falseGuard;
if (identical(_conditionInfo?.condition, node.condition)) {
@@ -514,16 +525,24 @@
/// If not `null`, the [ConstraintVariable] whose value should be set to
/// `true` if [condition] is asserted to be `true`.
- final ConstraintVariable trueChecksNonNull;
+ final ConstraintVariable trueDemonstratesNonNullIntent;
/// If not `null`, the [ConstraintVariable] whose value should be set to
/// `true` if [condition] is asserted to be `false`.
- final ConstraintVariable falseChecksNonNull;
+ final ConstraintVariable falseDemonstratesNonNullIntent;
_ConditionInfo(this.condition,
{@required this.isPure,
this.trueGuard,
this.falseGuard,
- this.trueChecksNonNull,
- this.falseChecksNonNull});
+ this.trueDemonstratesNonNullIntent,
+ this.falseDemonstratesNonNullIntent});
+
+ /// Returns a new [_ConditionInfo] describing the boolean "not" of `this`.
+ _ConditionInfo not(Expression condition) => _ConditionInfo(condition,
+ isPure: isPure,
+ trueGuard: falseGuard,
+ falseGuard: trueGuard,
+ trueDemonstratesNonNullIntent: falseDemonstratesNonNullIntent,
+ falseDemonstratesNonNullIntent: trueDemonstratesNonNullIntent);
}
diff --git a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
index 1715526..d3a89538 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_variable_gatherer.dart
@@ -117,6 +117,8 @@
DecoratedType visitSimpleFormalParameter(SimpleFormalParameter node) {
var type = decorateType(node.type);
var declaredElement = node.declaredElement;
+ assert(type.nonNullIntent == null);
+ type.nonNullIntent = NonNullIntent(node.offset);
_variables.recordDecoratedElementType(declaredElement, type);
if (declaredElement.isNamed) {
_currentFunctionType.namedParameters[declaredElement.name] = type;
@@ -147,11 +149,9 @@
var nullable = node.question == null
? TypeIsNullable(node.end)
: ConstraintVariable.always;
- // TODO(paulberry): decide whether to assign a variable for nullAsserts
- var nullAsserts = null;
var decoratedType = DecoratedTypeAnnotation(
type, nullable, _source, node.end,
- nullAsserts: nullAsserts, typeArguments: typeArguments);
+ typeArguments: typeArguments);
_variables.recordDecoratedTypeAnnotation(node, decoratedType);
return decoratedType;
}
diff --git a/pkg/analysis_server/lib/src/nullability/decorated_type.dart b/pkg/analysis_server/lib/src/nullability/decorated_type.dart
index fff240f..53c023b 100644
--- a/pkg/analysis_server/lib/src/nullability/decorated_type.dart
+++ b/pkg/analysis_server/lib/src/nullability/decorated_type.dart
@@ -23,10 +23,11 @@
/// migrated) forces this type to be non-nullable.
final ConstraintVariable nullable;
- /// [ConstraintVariable] whose value will be set to `true` if migration
- /// determines that a `null` value stored in an expression having this type
- /// will unconditionally lead to an assertion failure.
- final ConstraintVariable nullAsserts;
+ /// [ConstraintVariable] whose value will be set to `true` if the usage of
+ /// this type suggests that it is intended to be non-null (because of the
+ /// presence of a statement or expression that would unconditionally lead to
+ /// an exception being thrown in the case of a `null` value at runtime).
+ ConstraintVariable nonNullIntent;
/// If `this` is a function type, the [DecoratedType] of its return type.
final DecoratedType returnType;
@@ -57,8 +58,7 @@
final List<DecoratedType> typeArguments;
DecoratedType(this.type, this.nullable,
- {this.nullAsserts,
- this.returnType,
+ {this.returnType,
this.positionalParameters = const [],
this.namedParameters = const {},
this.namedParameterOptionalVariables = const {},
@@ -156,20 +156,14 @@
newPositionalParameters.add(positionalParameters[i]
._substitute(constraints, substitution, undecoratedParameterType));
}
- // TODO(paulberry): what do we do for nullAsserts here?
- var nullAsserts = null;
return DecoratedType(undecoratedResult, nullable,
- nullAsserts: nullAsserts,
returnType: returnType._substitute(
constraints, substitution, undecoratedResult.returnType),
positionalParameters: newPositionalParameters);
} else if (type is TypeParameterType) {
var inner = substitution[type.element];
- // TODO(paulberry): what do we do for nullAsserts here?
- var nullAsserts = null;
return DecoratedType(undecoratedResult,
- ConstraintVariable.or(constraints, inner?.nullable, nullable),
- nullAsserts: nullAsserts);
+ ConstraintVariable.or(constraints, inner?.nullable, nullable));
} else if (type is VoidType) {
return this;
}
@@ -191,10 +185,8 @@
DecoratedTypeAnnotation(
DartType type, ConstraintVariable nullable, this.source, this._offset,
- {ConstraintVariable nullAsserts,
- List<DecoratedType> typeArguments = const []})
- : super(type, nullable,
- nullAsserts: nullAsserts, typeArguments: typeArguments);
+ {List<DecoratedType> typeArguments = const []})
+ : super(type, nullable, typeArguments: typeArguments);
@override
bool get isEmpty =>
@@ -205,6 +197,17 @@
isEmpty ? [] : [SourceEdit(_offset, 0, '?')];
}
+/// Type of a [ConstraintVariable] representing the fact that a type is intended
+/// to be non-null.
+class NonNullIntent extends ConstraintVariable {
+ final int _offset;
+
+ NonNullIntent(this._offset);
+
+ @override
+ toString() => 'nonNullIntent($_offset)';
+}
+
/// Type of a [ConstraintVariable] representing the fact that a type is
/// nullable.
class TypeIsNullable extends ConstraintVariable {
diff --git a/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart b/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
index e64f358..88c6a34 100644
--- a/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
+++ b/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
@@ -71,6 +71,16 @@
assert(ConstraintVariable.always.value, isTrue);
}
+ test_assert_demonstrates_non_null_intent() async {
+ await analyze('''
+void f(int i) {
+ assert(i != null);
+}
+''');
+
+ assertConstraint([], decoratedTypeAnnotation('int i').nonNullIntent);
+ }
+
test_binaryExpression_add_left_check() async {
await analyze('''
int f(int i, int j) => i + j;
@@ -416,6 +426,34 @@
checkExpression('b) {}').nullCheck);
}
+ test_if_conditional_control_flow_after() async {
+ // Asserts after ifs don't demonstrate non-null intent.
+ // TODO(paulberry): if both branches complete normally, they should.
+ await analyze('''
+void f(bool b, int i) {
+ if (b) return;
+ assert(i != null);
+}
+''');
+
+ assertNoConstraints(decoratedTypeAnnotation('int i').nonNullIntent);
+ }
+
+ test_if_conditional_control_flow_within() async {
+ // Asserts inside ifs don't demonstrate non-null intent.
+ await analyze('''
+void f(bool b, int i) {
+ if (b) {
+ assert(i != null);
+ } else {
+ assert(i != null);
+ }
+}
+''');
+
+ assertNoConstraints(decoratedTypeAnnotation('int i').nonNullIntent);
+ }
+
test_if_guard_equals_null() async {
await analyze('''
int f(int i, int j, int k) {
@@ -737,6 +775,7 @@
expect(decoratedFunctionType('f').positionalParameters[0],
same(decoratedType));
expect(decoratedType.nullable, isNotNull);
+ expect(decoratedType.nonNullIntent, isNotNull);
}
test_topLevelFunction_returnType_implicit_dynamic() async {
