Skip to content

Commit 7e8e660

Browse files
stereotype441commit-bot@chromium.org
stereotype441
authored and
[email protected]
committed
Migration: do not use always or never in already-migrated types.
Previously, when the code being migrated referred to a nullable or a non-nullable type in an already-migrated library, we would use the `always` or `never` graph node to represent the nullability of the type. This was problematic, because it made it difficult for instrumentation to pinpoint precisely which element in an already-migrated library was the cause of an expresion being null-checked, or a type being made non-nullable. We now create a fresh nullability node for each non-nullable type coming from already-migrated code, with an edge to `always` or `never` to ensure that the fresh node has the correct nullability, so that instrumentation can see precisely which element it came from. Since the migration tool doesn't traverse the ASTs of already-migrated code, we can't report an AST node that caused such an edge, so an `element` getter has been added EdgeOriginInfo to allow us to report the element that caused the edge. As before, types associated with already-migrated code are reported to instrumentation via InstrumentationListener.externalDecoratedType. As a new enhancement, bounds of generic parameters in already-migrated code are reported to instrumentation via InstrumentationListener.externalDecoratedTypeParameterBound. Change-Id: Ided4e96e2920f8d9062688f5a6fda29b9b71dd12 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/124000 Reviewed-by: Brian Wilkerson <[email protected]> Reviewed-by: Mike Fairhurst <[email protected]>
1 parent e4a6091 commit 7e8e660

10 files changed

Lines changed: 333 additions & 109 deletions

‎pkg/analysis_server/lib/src/edit/nnbd_migration/instrumentation_listener.dart‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ class InstrumentationListener implements NullabilityMigrationInstrumentation {
2929
data.externalDecoratedType[element] = decoratedType;
3030
}
3131

32+
@override
33+
void externalDecoratedTypeParameterBound(
34+
TypeParameterElement typeParameter, DecoratedTypeInfo decoratedType) {
35+
// TODO(paulberry): make use of this information.
36+
}
37+
3238
@override
3339
void fix(SingleNullabilityFix fix, Iterable<FixReasonInfo> reasons) {
3440
_sourceInfo(fix.source).fixes[fix] =

‎pkg/nnbd_migration/lib/instrumentation.dart‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'package:analyzer/dart/element/type.dart';
88
import 'package:analyzer/src/generated/source.dart';
99
import 'package:nnbd_migration/nnbd_migration.dart';
1010
import 'package:nnbd_migration/nullability_state.dart';
11+
import 'package:nnbd_migration/src/decorated_type.dart';
1112

1213
/// Information exposed to the migration client about the set of nullability
1314
/// nodes decorating a type in the program being migrated.
@@ -99,20 +100,32 @@ abstract class EdgeInfo implements FixReasonInfo {
99100
/// Information exposed to the migration client about the location in source
100101
/// code that led an edge to be introduced into the nullability graph.
101102
abstract class EdgeOriginInfo {
103+
/// If the proximate cause of the edge being introduced into the graph
104+
/// corresponds to the type of an element in an already migrated-library, the
105+
/// corresponding element; otherwise `null`.
106+
///
107+
/// Note that either [node] or [element] will always be non-null.
108+
Element get element;
109+
102110
/// The kind of origin represented by this info.
103111
EdgeOriginKind get kind;
104112

105-
/// The AST node that led the edge to be introduced into the nullability
106-
/// graph.
113+
/// If the proximate cause of the edge being introduced into the graph
114+
/// corresponds to an AST node in a source file that is being migrated, the
115+
/// corresponding AST node; otherwise `null`.
116+
///
117+
/// Note that either [node] or [element] will always be non-null.
107118
AstNode get node;
108119

109-
/// The source file that [node] appears in.
120+
/// If [node] is non-null, the source file that it appears in. Otherwise
121+
/// `null`.
110122
Source get source;
111123
}
112124

113125
/// An enumeration of the various kinds of edge origins created by the migration
114126
/// engine.
115127
enum EdgeOriginKind {
128+
alreadyMigratedType,
116129
alwaysNullableType,
117130
compoundAssignment,
118131
defaultValue,
@@ -153,6 +166,12 @@ abstract class NullabilityMigrationInstrumentation {
153166
/// of the element.
154167
void externalDecoratedType(Element element, DecoratedTypeInfo decoratedType);
155168

169+
/// Called whenever reference is made to an [typeParameter] outside of the
170+
/// code being migrated, to report the nullability nodes associated with the
171+
/// bound of the type parameter.
172+
void externalDecoratedTypeParameterBound(
173+
TypeParameterElement typeParameter, DecoratedTypeInfo decoratedType);
174+
156175
/// Called whenever a fix is decided upon, to report the reasons for the fix.
157176
void fix(SingleNullabilityFix fix, Iterable<FixReasonInfo> reasons);
158177

‎pkg/nnbd_migration/lib/src/already_migrated_code_decorator.dart‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'package:analyzer/dart/element/type.dart';
88
import 'package:analyzer/src/dart/element/type.dart';
99
import 'package:analyzer/src/generated/resolver.dart';
1010
import 'package:nnbd_migration/src/decorated_type.dart';
11+
import 'package:nnbd_migration/src/edge_origin.dart';
1112
import 'package:nnbd_migration/src/nullability_node.dart';
1213

1314
/// This class transforms ordinary [DartType]s into their corresponding
@@ -22,53 +23,61 @@ class AlreadyMigratedCodeDecorator {
2223

2324
/// Transforms [type], which should have come from code that has already been
2425
/// migrated to NNBD, into the corresponding [DecoratedType].
25-
DecoratedType decorate(DartType type) {
26+
DecoratedType decorate(DartType type, Element element) {
2627
if (type.isVoid || type.isDynamic) {
27-
return DecoratedType(type, _graph.always);
28+
var node = NullabilityNode.forAlreadyMigrated();
29+
_graph.makeNullable(node, AlwaysNullableTypeOrigin.forElement(element));
30+
return DecoratedType(type, node);
2831
}
2932
NullabilityNode node;
3033
var nullabilitySuffix = (type as TypeImpl).nullabilitySuffix;
3134
if (nullabilitySuffix == NullabilitySuffix.question) {
32-
node = _graph.always;
35+
node = NullabilityNode.forAlreadyMigrated();
36+
_graph.makeNullable(node, AlreadyMigratedTypeOrigin.forElement(element));
3337
} else {
3438
// Currently, all types passed to this method have nullability suffix `star`
3539
// because (a) we don't yet have a migrated SDK, and (b) we haven't added
3640
// support to the migrator for analyzing packages that have already been
3741
// migrated with NNBD enabled.
3842
// TODO(paulberry): fix this assertion when things change.
3943
assert(nullabilitySuffix == NullabilitySuffix.star);
40-
node = _graph.never;
44+
node = NullabilityNode.forAlreadyMigrated();
45+
_graph.makeNonNullable(
46+
node, AlreadyMigratedTypeOrigin.forElement(element));
4147
}
4248
if (type is FunctionType) {
4349
var typeFormalBounds = type.typeFormals.map((e) {
4450
var bound = e.bound;
4551
if (bound == null) {
46-
return decorate((_typeProvider.objectType as TypeImpl)
47-
.withNullability(NullabilitySuffix.question));
52+
return decorate(
53+
(_typeProvider.objectType as TypeImpl)
54+
.withNullability(NullabilitySuffix.question),
55+
element);
4856
} else {
49-
return decorate(bound);
57+
return decorate(bound, element);
5058
}
5159
}).toList();
5260
var positionalParameters = <DecoratedType>[];
5361
var namedParameters = <String, DecoratedType>{};
5462
for (var parameter in type.parameters) {
5563
if (parameter.isPositional) {
56-
positionalParameters.add(decorate(parameter.type));
64+
positionalParameters.add(decorate(parameter.type, element));
5765
} else {
58-
namedParameters[parameter.name] = decorate(parameter.type);
66+
namedParameters[parameter.name] = decorate(parameter.type, element);
5967
}
6068
}
6169
return DecoratedType(type, node,
6270
typeFormalBounds: typeFormalBounds,
63-
returnType: decorate(type.returnType),
71+
returnType: decorate(type.returnType, element),
6472
namedParameters: namedParameters,
6573
positionalParameters: positionalParameters);
6674
} else if (type is InterfaceType) {
6775
var typeParameters = type.element.typeParameters;
6876
if (typeParameters.isNotEmpty) {
6977
assert(type.typeArguments.length == typeParameters.length);
70-
return DecoratedType(type, node,
71-
typeArguments: type.typeArguments.map(decorate).toList());
78+
return DecoratedType(type, node, typeArguments: [
79+
for (var t in type.typeArguments) decorate(t, element)
80+
]);
7281
}
7382
return DecoratedType(type, node);
7483
} else if (type is TypeParameterType) {
@@ -96,6 +105,6 @@ class AlreadyMigratedCodeDecorator {
96105
// Add FutureOr<T> as a supertype of Future<T>.
97106
allSupertypes.add(_typeProvider.futureOrType2(type.typeArguments.single));
98107
}
99-
return allSupertypes.map(decorate);
108+
return [for (var t in allSupertypes) decorate(t, class_)];
100109
}
101110
}

‎pkg/nnbd_migration/lib/src/edge_origin.dart‎

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

55
import 'package:analyzer/dart/ast/ast.dart';
6+
import 'package:analyzer/dart/element/element.dart';
67
import 'package:analyzer/src/generated/source.dart';
78
import 'package:nnbd_migration/instrumentation.dart';
89

10+
/// Edge origin resulting from a type in already-migrated code.
11+
///
12+
/// For example, in the Map class in dart:core:
13+
/// V? operator [](Object key);
14+
///
15+
/// this class is used for the edge connecting `always` to the return type of
16+
/// `operator []`, due to the fact that dart:core has already been migrated and
17+
/// the type is explicitly nullable.
18+
///
19+
/// Note that since a single element can have a complex type, it is likely that
20+
/// multiple edges will be created with an [AlreadyMigratedTypeOrigin] pointing
21+
/// to the same type. To distinguish which edge corresponds to which part of
22+
/// the element's type, use the callbacks
23+
/// [NullabilityMigrationInstrumentation.externalDecoratedType] and
24+
/// [NullabilityMigrationInstrumentation.externalDecoratedTypeParameterBound].
25+
class AlreadyMigratedTypeOrigin extends EdgeOrigin {
26+
AlreadyMigratedTypeOrigin.forElement(Element element)
27+
: super.forElement(element);
28+
29+
@override
30+
EdgeOriginKind get kind => EdgeOriginKind.alreadyMigratedType;
31+
}
32+
933
/// Edge origin resulting from the use of a type that is always nullable.
1034
///
1135
/// For example, in the following code snippet:
@@ -17,6 +41,9 @@ import 'package:nnbd_migration/instrumentation.dart';
1741
class AlwaysNullableTypeOrigin extends EdgeOrigin {
1842
AlwaysNullableTypeOrigin(Source source, AstNode node) : super(source, node);
1943

44+
AlwaysNullableTypeOrigin.forElement(Element element)
45+
: super.forElement(element);
46+
2047
@override
2148
EdgeOriginKind get kind => EdgeOriginKind.alwaysNullableType;
2249
}
@@ -53,7 +80,14 @@ abstract class EdgeOrigin extends EdgeOriginInfo {
5380
@override
5481
final AstNode node;
5582

56-
EdgeOrigin(this.source, this.node);
83+
@override
84+
final Element element;
85+
86+
EdgeOrigin(this.source, this.node) : element = null;
87+
88+
EdgeOrigin.forElement(this.element)
89+
: source = null,
90+
node = null;
5791
}
5892

5993
/// Edge origin resulting from the relationship between a field formal parameter

‎pkg/nnbd_migration/lib/src/nullability_node.dart‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,19 @@ class NullabilityGraph {
158158
return _sourcesBeingMigrated.contains(source);
159159
}
160160

161+
/// Creates a graph edge that will try to force the given [node] to be
162+
/// non-nullable.
163+
void makeNonNullable(NullabilityNode node, EdgeOrigin origin) {
164+
connect(node, never, origin, hard: true);
165+
}
166+
167+
/// Creates a graph edge that will try to force the given [node] to be
168+
/// nullable.
169+
void makeNullable(NullabilityNode node, EdgeOrigin origin,
170+
{List<NullabilityNode> guards: const []}) {
171+
connect(always, node, origin, guards: guards);
172+
}
173+
161174
/// Record source as code that is being migrated.
162175
void migrating(Source source) {
163176
_sourcesBeingMigrated.add(source);
@@ -447,6 +460,11 @@ abstract class NullabilityNode implements NullabilityNodeInfo {
447460
/// List of edges that have this node as their destination.
448461
final _upstreamEdges = <NullabilityEdge>[];
449462

463+
/// Creates a [NullabilityNode] representing the nullability of a variable
464+
/// whose type comes from an already-migrated library.
465+
factory NullabilityNode.forAlreadyMigrated() =>
466+
_NullabilityNodeSimple('migrated');
467+
450468
/// Creates a [NullabilityNode] representing the nullability of an expression
451469
/// which is nullable iff two other nullability nodes are both nullable.
452470
///
@@ -494,6 +512,8 @@ abstract class NullabilityNode implements NullabilityNodeInfo {
494512
/// annotate the nullability of that type.
495513
String get debugSuffix => '?($this)';
496514

515+
Iterable<EdgeInfo> get downstreamEdges => _downstreamEdges;
516+
497517
/// After nullability propagation, this getter can be used to query whether
498518
/// the type associated with this node should be considered "exact nullable".
499519
@visibleForTesting

‎pkg/nnbd_migration/lib/src/variables.dart‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,10 @@ class Variables implements VariableRecorder, VariableRepository {
9292
'have been stored by the NodeBuilder via '
9393
'recordTypeParameterBound');
9494
}
95-
decoratedType = _alreadyMigratedCodeDecorator
96-
.decorate(typeParameter.bound ?? DynamicTypeImpl.instance);
95+
decoratedType = _alreadyMigratedCodeDecorator.decorate(
96+
typeParameter.bound ?? DynamicTypeImpl.instance, typeParameter);
97+
instrumentation?.externalDecoratedTypeParameterBound(
98+
typeParameter, decoratedType);
9799
_decoratedTypeParameterBounds[typeParameter] = decoratedType;
98100
}
99101
return decoratedType;
@@ -181,9 +183,11 @@ class Variables implements VariableRecorder, VariableRepository {
181183

182184
DecoratedType decoratedType;
183185
if (element is FunctionTypedElement) {
184-
decoratedType = _alreadyMigratedCodeDecorator.decorate(element.type);
186+
decoratedType =
187+
_alreadyMigratedCodeDecorator.decorate(element.type, element);
185188
} else if (element is VariableElement) {
186-
decoratedType = _alreadyMigratedCodeDecorator.decorate(element.type);
189+
decoratedType =
190+
_alreadyMigratedCodeDecorator.decorate(element.type, element);
187191
} else {
188192
// TODO(paulberry)
189193
throw UnimplementedError('Decorating ${element.runtimeType}');
@@ -199,7 +203,6 @@ class Variables implements VariableRecorder, VariableRepository {
199203
var result = <ClassElement, DecoratedType>{};
200204
for (var decoratedSupertype
201205
in _alreadyMigratedCodeDecorator.getImmediateSupertypes(class_)) {
202-
assert(identical(decoratedSupertype.node, _graph.never));
203206
var class_ = (decoratedSupertype.type as InterfaceType).element;
204207
result[class_] = decoratedSupertype;
205208
}

0 commit comments

Comments
 (0)