Change representation of ExpressionChecks.
Previously, this class held on to a ConstraintVariable indicating
whether a null check should be inserted. Now it holds on to
NullabilityNodes indicating whether the source and destination of an
assignment are nullable, and it can infer from this whether a null
check needs to be inserted.
This paves the way for removing the old constraint solving logic in
favor of a more direct nullability propagation approach using
nullability nodes.
Change-Id: I96b95c400ed40bd12f9c9dcbb4c09a69849e51c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100885
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@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 a723c9a..7127e82 100644
--- a/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
+++ b/pkg/analysis_server/lib/src/nullability/constraint_gatherer.dart
@@ -432,7 +432,10 @@
if (expression != null) {
checkNotNull = CheckExpression(expression);
_variables.recordExpressionChecks(
- _source, expression, ExpressionChecks(checkNotNull));
+ _source,
+ expression,
+ ExpressionChecks(
+ expression.end, sourceType.node, destinationType.node, _guards));
}
NullabilityNode.recordAssignment(sourceType.node, destinationType.node,
checkNotNull, _guards, _constraints, _inConditionalControlFlow);
diff --git a/pkg/analysis_server/lib/src/nullability/expression_checks.dart b/pkg/analysis_server/lib/src/nullability/expression_checks.dart
index 14f47db..bc533d6 100644
--- a/pkg/analysis_server/lib/src/nullability/expression_checks.dart
+++ b/pkg/analysis_server/lib/src/nullability/expression_checks.dart
@@ -2,6 +2,7 @@
// 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.
+import 'package:analysis_server/src/nullability/nullability_node.dart';
import 'package:analysis_server/src/nullability/transitional_api.dart';
import 'package:analyzer_plugin/protocol/protocol_common.dart';
@@ -13,16 +14,34 @@
/// that the expression is not null. We need to add other checks, e.g. to check
/// that a List<int?> is actually a List<int>.
class ExpressionChecks extends PotentialModification {
- /// Constraint variable whose value will be `true` if this expression requires
- /// a null check.
- final CheckExpression nullCheck;
+ /// Source offset where a trailing `!` might need to be inserted.
+ final int offset;
- ExpressionChecks(this.nullCheck);
+ /// Nullability node indicating whether the expression's value is nullable.
+ final NullabilityNode valueNode;
+
+ /// Nullability node indicating whether the expression's context requires a
+ /// nullable value.
+ final NullabilityNode contextNode;
+
+ /// Nullability nodes guarding execution of the expression. If any of the
+ /// nodes in this list turns out to be non-nullable, the expression is dead
+ /// code and will be removed by the migration tool.
+ final List<NullabilityNode> guards;
+
+ ExpressionChecks(this.offset, this.valueNode, this.contextNode,
+ Iterable<NullabilityNode> guards)
+ : guards = guards.toList();
@override
- bool get isEmpty => !nullCheck.value;
+ bool get isEmpty {
+ for (var guard in guards) {
+ if (!guard.isNullable) return true;
+ }
+ return !valueNode.isNullable || contextNode.isNullable;
+ }
@override
Iterable<SourceEdit> get modifications =>
- nullCheck.value ? [SourceEdit(nullCheck.offset, 0, '!')] : [];
+ isEmpty ? [] : [SourceEdit(offset, 0, '!')];
}
diff --git a/pkg/analysis_server/lib/src/nullability/nullability_node.dart b/pkg/analysis_server/lib/src/nullability/nullability_node.dart
index fd7d82b..f993264 100644
--- a/pkg/analysis_server/lib/src/nullability/nullability_node.dart
+++ b/pkg/analysis_server/lib/src/nullability/nullability_node.dart
@@ -69,10 +69,10 @@
/// the new nullability node behave consistently with the old nodes.
/// TODO(paulberry): this should become unnecessary once constraint solving is
/// performed directly using [NullabilityNode] objects.
- NullabilityNode.forSubstitution(Constraints constraints,
- NullabilityNode innerNode, NullabilityNode outerNode)
- : this._(ConstraintVariable.or(
- constraints, innerNode?.nullable, outerNode.nullable));
+ factory NullabilityNode.forSubstitution(
+ Constraints constraints,
+ NullabilityNode innerNode,
+ NullabilityNode outerNode) = NullabilityNodeForSubstitution._;
/// Creates a [NullabilityNode] representing the nullability of a type
/// annotation appearing explicitly in the user's program.
@@ -90,7 +90,7 @@
/// After constraint solving, this getter can be used to query whether the
/// type associated with this node should be considered nullable.
- bool get isNullable => nullable.value;
+ bool get isNullable => nullable == null ? false : nullable.value;
/// Indicates whether this node is associated with a named parameter for which
/// nullability migration needs to decide whether it is optional or required.
@@ -199,3 +199,26 @@
constraints.record(conditions, consequence);
}
}
+
+/// Derived class for nullability nodes that arise from type variable
+/// substitution.
+class NullabilityNodeForSubstitution extends NullabilityNode {
+ /// Nullability node representing the inner type of the substitution.
+ ///
+ /// For example, if this NullabilityNode arose from substituting `int*` for
+ /// `T` in the type `T*`, [innerNode] is the nullability corresponding to the
+ /// `*` in `int*`.
+ final NullabilityNode innerNode;
+
+ /// Nullability node representing the outer type of the substitution.
+ ///
+ /// For example, if this NullabilityNode arose from substituting `int*` for
+ /// `T` in the type `T*`, [innerNode] is the nullability corresponding to the
+ /// `*` in `T*`.
+ final NullabilityNode outerNode;
+
+ NullabilityNodeForSubstitution._(
+ Constraints constraints, this.innerNode, this.outerNode)
+ : super._(ConstraintVariable.or(
+ constraints, innerNode?.nullable, outerNode.nullable));
+}
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 46f94cb..315cea2 100644
--- a/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
+++ b/pkg/analysis_server/test/src/nullability/migration_visitor_test.dart
@@ -65,32 +65,17 @@
/// assignment (if the value is being assigned), or `null` if the value is
/// being used in a circumstance where `null` is not permitted. [guards] is
/// a list of nullability nodes for which there are enclosing if statements
- /// checking that the corresponding values are non-null. [genericNode], if
- /// present, is the node representing the nullability of a type variables,
- /// where the assignment is in a context that makes contravariant use of the
- /// type variable.
+ /// checking that the corresponding values are non-null.
void assertNullCheck(
ExpressionChecks expressionChecks, NullabilityNode valueNode,
- {NullabilityNode contextNode,
- List<NullabilityNode> guards = const [],
- NullabilityNode genericNode}) {
- var conditions = <ConstraintVariable>[];
- for (var guard in guards) {
- conditions.add(guard.nullable);
- }
- conditions.add(valueNode.nullable);
- var consequences = <ConstraintVariable>[];
- if (genericNode != null) {
- consequences.add(genericNode.nullable);
- }
- if (contextNode != null) {
- consequences.add(contextNode.nullable);
- }
- consequences.add(expressionChecks.nullCheck);
- var consequence = consequences.skip(1).fold(consequences.first, _either);
+ {NullabilityNode contextNode, List<NullabilityNode> guards = const []}) {
+ expect(expressionChecks.valueNode, same(valueNode));
if (contextNode == null) {
- assertConstraint(conditions, consequence);
+ expect(expressionChecks.contextNode, same(NullabilityNode.never));
+ } else {
+ expect(expressionChecks.contextNode, same(contextNode));
}
+ expect(expressionChecks.guards, guards);
}
/// Gets the [ExpressionChecks] associated with the expression whose text
@@ -636,8 +621,13 @@
var nullable_i = decoratedTypeAnnotation('int i').node;
var nullable_c_t = decoratedTypeAnnotation('C<int>').typeArguments[0].node;
var nullable_t = decoratedTypeAnnotation('T t').node;
- assertNullCheck(checkExpression('i/*check*/'), nullable_i,
- genericNode: nullable_c_t, contextNode: nullable_t);
+ var check_i = checkExpression('i/*check*/');
+ var nullable_c_t_or_nullable_t =
+ check_i.contextNode as NullabilityNodeForSubstitution;
+ expect(nullable_c_t_or_nullable_t.innerNode, same(nullable_c_t));
+ expect(nullable_c_t_or_nullable_t.outerNode, same(nullable_t));
+ assertNullCheck(check_i, nullable_i,
+ contextNode: nullable_c_t_or_nullable_t);
}
test_methodInvocation_parameter_generic() async {
