Skip to content

Commit 5167499

Browse files
stereotype441commit-bot@chromium.org
authored andcommitted
Migration: record the "origin" of each edge in the NullabilityEdge object.
This paves the way for being able to explain to the user exactly why the migration tool made any given nullability decision. Change-Id: I553e4881fdb37b238a066fcfba7c21ae919d7d6e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106381 Reviewed-by: Brian Wilkerson <[email protected]>
1 parent dc059d7 commit 5167499

6 files changed

Lines changed: 175 additions & 63 deletions

File tree

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'package:analyzer/src/generated/source.dart';
6+
7+
/// Edge origin resulting from the use of a type that is always nullable.
8+
///
9+
/// For example, in the following code snippet:
10+
/// void f(dynamic x) {}
11+
///
12+
/// this class is used for the edge connecting `always` to the type of f's `x`
13+
/// parameter, due to the fact that the `dynamic` type is always considered
14+
/// nullable.
15+
class AlwaysNullableTypeOrigin extends EdgeOriginWithLocation {
16+
AlwaysNullableTypeOrigin(Source source, int offset) : super(source, offset);
17+
}
18+
19+
/// Common interface for classes providing information about how an edge came
20+
/// to be; that is, what was found in the source code that led the migration
21+
/// tool to create the edge.
22+
abstract class EdgeOrigin {}
23+
24+
/// Common base class for edge origins that are associated with a single
25+
/// location in the source code.
26+
abstract class EdgeOriginWithLocation extends EdgeOrigin {
27+
/// The source file containing the code construct that led to the edge.
28+
final Source source;
29+
30+
/// The offset within the source file of the code construct that led to the
31+
/// edge.
32+
final int offset;
33+
34+
EdgeOriginWithLocation(this.source, this.offset);
35+
}
36+
37+
/// Edge origin resulting from a class that is instantiated to bounds.
38+
///
39+
/// For example, in the following code snippet:
40+
/// class C<T extends Object> {}
41+
/// C x;
42+
///
43+
/// this class is used for the edge connecting the type of x's type parameter
44+
/// with the type bound in the declaration of C.
45+
class InstantiateToBoundsOrigin extends EdgeOriginWithLocation {
46+
InstantiateToBoundsOrigin(Source source, int offset) : super(source, offset);
47+
}
48+
49+
/// Edge origin resulting from a call site that does not supply a named
50+
/// parameter.
51+
///
52+
/// For example, in the following code snippet:
53+
/// void f({int i}) {}
54+
/// main() {
55+
/// f();
56+
/// }
57+
///
58+
/// this class is used for the edge connecting `always` to the type of f's `i`
59+
/// parameter, due to the fact that the call to `f` implicitly passes a null
60+
/// value for `i`.
61+
class NamedParameterNotSuppliedOrigin extends EdgeOriginWithLocation {
62+
NamedParameterNotSuppliedOrigin(Source source, int offset)
63+
: super(source, offset);
64+
}
65+
66+
/// Edge origin resulting from the presence of a non-null assertion.
67+
///
68+
/// For example, in the following code snippet:
69+
/// void f(int i) {
70+
/// assert(i != null);
71+
/// }
72+
///
73+
/// this class is used for the edge connecting the type of f's `i` parameter to
74+
/// `never`, due to the assert statement proclaiming that `i` is not `null`.
75+
class NonNullAssertionOrigin extends EdgeOriginWithLocation {
76+
NonNullAssertionOrigin(Source source, int offset) : super(source, offset);
77+
}
78+
79+
/// Edge origin resulting from the presence of an explicit nullability hint
80+
/// comment.
81+
///
82+
/// For example, in the following code snippet:
83+
/// void f(int/*?*/ i) {}
84+
///
85+
/// this class is used for the edge connecting `always` to the type of f's `i`
86+
/// parameter, due to the presence of the `/*?*/` comment.
87+
class NullabilityCommentOrigin extends EdgeOriginWithLocation {
88+
NullabilityCommentOrigin(Source source, int offset) : super(source, offset);
89+
}
90+
91+
/// Edge origin resulting from the presence of an optional formal parameter.
92+
///
93+
/// For example, in the following code snippet:
94+
/// void f({int i}) {}
95+
///
96+
/// this class is used for the edge connecting `always` to the type of f's `i`
97+
/// parameter, due to the fact that `i` is optional and has no initializer.
98+
class OptionalFormalParameterOrigin extends EdgeOriginWithLocation {
99+
OptionalFormalParameterOrigin(Source source, int offset)
100+
: super(source, offset);
101+
}

pkg/nnbd_migration/lib/src/expression_checks.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// BSD-style license that can be found in the LICENSE file.
44

55
import 'package:analyzer_plugin/protocol/protocol_common.dart';
6+
import 'package:nnbd_migration/src/edge_origin.dart';
67
import 'package:nnbd_migration/src/nullability_node.dart';
78
import 'package:nnbd_migration/src/potential_modification.dart';
89

@@ -13,7 +14,7 @@ import 'package:nnbd_migration/src/potential_modification.dart';
1314
/// TODO(paulberry): the only check we support now is [nullCheck], which checks
1415
/// that the expression is not null. We need to add other checks, e.g. to check
1516
/// that a List<int?> is actually a List<int>.
16-
class ExpressionChecks extends PotentialModification {
17+
class ExpressionChecks extends PotentialModification implements EdgeOrigin {
1718
/// Source offset where a trailing `!` might need to be inserted.
1819
final int offset;
1920

pkg/nnbd_migration/lib/src/graph_builder.dart

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import 'package:nnbd_migration/src/node_builder.dart';
1919
import 'package:nnbd_migration/src/nullability_node.dart';
2020
import 'package:nnbd_migration/src/variables.dart';
2121

22+
import 'edge_origin.dart';
23+
2224
/// Visitor that builds nullability graph edges by examining code to be
2325
/// migrated.
2426
///
@@ -154,8 +156,9 @@ class GraphBuilder extends GeneralizingAstVisitor<DecoratedType> {
154156
if (identical(_conditionInfo?.condition, node.condition)) {
155157
if (!_inConditionalControlFlow &&
156158
_conditionInfo.trueDemonstratesNonNullIntent != null) {
157-
_conditionInfo.trueDemonstratesNonNullIntent
158-
?.recordNonNullIntent(_guards, _graph);
159+
_graph.connect(_conditionInfo.trueDemonstratesNonNullIntent,
160+
_graph.never, NonNullAssertionOrigin(_source, node.offset),
161+
hard: true);
159162
}
160163
}
161164
node.message?.accept(this);
@@ -271,9 +274,11 @@ class GraphBuilder extends GeneralizingAstVisitor<DecoratedType> {
271274
// Nothing to do; the implicit default value of `null` will never be
272275
// reached.
273276
} else {
274-
NullabilityNode.recordAssignment(_graph.always,
275-
getOrComputeElementType(node.declaredElement).node, _guards, _graph,
276-
hard: false);
277+
_graph.connect(
278+
_graph.always,
279+
getOrComputeElementType(node.declaredElement).node,
280+
OptionalFormalParameterOrigin(_source, node.offset),
281+
guards: _guards);
277282
}
278283
} else {
279284
_handleAssignment(
@@ -607,11 +612,13 @@ $stackTrace''');
607612
throw new StateError('No type annotation for type name '
608613
'${typeName.toSource()}, offset=${typeName.offset}');
609614
}
615+
var origin = InstantiateToBoundsOrigin(_source, typeName.offset);
610616
for (int i = 0; i < instantiatedType.typeArguments.length; i++) {
611617
_unionDecoratedTypes(
612618
instantiatedType.typeArguments[i],
613619
_variables.decoratedElementType(element.typeParameters[i],
614-
create: true));
620+
create: true),
621+
origin);
615622
}
616623
} else {
617624
for (int i = 0; i < typeArguments.length; i++) {
@@ -648,9 +655,8 @@ $stackTrace''');
648655
void _checkAssignment(DecoratedType destinationType, DecoratedType sourceType,
649656
ExpressionChecks expressionChecks,
650657
{@required bool hard}) {
651-
NullabilityNode.recordAssignment(
652-
sourceType.node, destinationType.node, _guards, _graph,
653-
hard: hard);
658+
_graph.connect(sourceType.node, destinationType.node, expressionChecks,
659+
guards: _guards, hard: hard);
654660
// TODO(paulberry): generalize this.
655661
if ((_isSimple(sourceType) || destinationType.type.isObject) &&
656662
_isSimple(destinationType)) {
@@ -732,7 +738,8 @@ $stackTrace''');
732738
// Any parameters not supplied must be optional.
733739
for (var entry in calleeType.namedParameters.entries) {
734740
if (suppliedNamedParameters.contains(entry.key)) continue;
735-
entry.value.node.recordNamedParameterNotSupplied(_guards, _graph);
741+
entry.value.node.recordNamedParameterNotSupplied(_guards, _graph,
742+
NamedParameterNotSuppliedOrigin(_source, argumentList.offset));
736743
}
737744
}
738745

@@ -812,8 +819,9 @@ $stackTrace''');
812819
return false;
813820
}
814821

815-
void _unionDecoratedTypes(DecoratedType x, DecoratedType y) {
816-
_graph.union(x.node, y.node);
822+
void _unionDecoratedTypes(
823+
DecoratedType x, DecoratedType y, EdgeOrigin origin) {
824+
_graph.union(x.node, y.node, origin);
817825
if (x.typeArguments.isNotEmpty ||
818826
y.typeArguments.isNotEmpty ||
819827
x.returnType != null ||

pkg/nnbd_migration/lib/src/node_builder.dart

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import 'package:nnbd_migration/src/decorated_type.dart';
1616
import 'package:nnbd_migration/src/expression_checks.dart';
1717
import 'package:nnbd_migration/src/nullability_node.dart';
1818

19+
import 'edge_origin.dart';
20+
1921
/// Visitor that builds nullability nodes based on visiting code to be migrated.
2022
///
2123
/// The return type of each `visit...` method is a [DecoratedType] indicating
@@ -58,7 +60,7 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType> {
5860
? new DecoratedType(
5961
DynamicTypeImpl.instance,
6062
NullabilityNode.forInferredDynamicType(
61-
_graph, enclosingNode.offset))
63+
_graph, _source, enclosingNode.offset))
6264
: type.accept(this);
6365
}
6466

@@ -167,7 +169,8 @@ $stackTrace''');
167169
var type = node.type;
168170
if (type.isVoid || type.isDynamic) {
169171
var nullabilityNode = NullabilityNode.forTypeAnnotation(node.end);
170-
_graph.connect(_graph.always, nullabilityNode);
172+
_graph.connect(_graph.always, nullabilityNode,
173+
AlwaysNullableTypeOrigin(_source, node.offset));
171174
var decoratedType =
172175
DecoratedTypeAnnotation(type, nullabilityNode, node.offset);
173176
_variables.recordDecoratedTypeAnnotation(_source, node, decoratedType,
@@ -218,12 +221,16 @@ $stackTrace''');
218221
positionalParameters: positionalParameters,
219222
namedParameters: namedParameters);
220223
_variables.recordDecoratedTypeAnnotation(_source, node, decoratedType);
221-
switch (_classifyComment(node.endToken.next.precedingComments)) {
224+
var commentToken = node.endToken.next.precedingComments;
225+
switch (_classifyComment(commentToken)) {
222226
case _NullabilityComment.bang:
223-
_graph.connect(decoratedType.node, _graph.never, hard: true);
227+
_graph.connect(decoratedType.node, _graph.never,
228+
NullabilityCommentOrigin(_source, commentToken.offset),
229+
hard: true);
224230
break;
225231
case _NullabilityComment.question:
226-
_graph.connect(_graph.always, decoratedType.node);
232+
_graph.connect(_graph.always, decoratedType.node,
233+
NullabilityCommentOrigin(_source, commentToken.offset));
227234
break;
228235
case _NullabilityComment.none:
229236
break;
@@ -238,8 +245,10 @@ $stackTrace''');
238245
DecoratedType visitTypeParameter(TypeParameter node) {
239246
var element = node.declaredElement;
240247
var decoratedBound = node.bound?.accept(this) ??
241-
DecoratedType(element.bound ?? _typeProvider.objectType,
242-
NullabilityNode.forInferredDynamicType(_graph, node.offset));
248+
DecoratedType(
249+
element.bound ?? _typeProvider.objectType,
250+
NullabilityNode.forInferredDynamicType(
251+
_graph, _source, node.offset));
243252
_variables.recordDecoratedElementType(element, decoratedBound);
244253
return null;
245254
}

0 commit comments

Comments
 (0)