Migration: prevent AngularDart view/content fields from being marked late.
Change-Id: I525a160c90f2bff8851c175ff34edf684538a927
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/230301
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
diff --git a/pkg/nnbd_migration/lib/src/node_builder.dart b/pkg/nnbd_migration/lib/src/node_builder.dart
index 2dad8d1..0f1e8f4 100644
--- a/pkg/nnbd_migration/lib/src/node_builder.dart
+++ b/pkg/nnbd_migration/lib/src/node_builder.dart
@@ -476,7 +476,7 @@
var type = decoratedType.positionalParameters![0];
_variables!.recordDecoratedElementType(declaredElement.variable, type,
soft: true);
- if (_hasAngularChildAnnotation(node.metadata)) {
+ if (_getAngularAnnotation(node.metadata) == _AngularAnnotation.child) {
_graph.makeNullable(
type!.node!, AngularAnnotationOrigin(source, node));
}
@@ -686,9 +686,18 @@
_variables!.recordDecoratedElementType(declaredElement, type);
variable.initializer?.accept(this);
if (parent is FieldDeclaration) {
- if (_hasAngularChildAnnotation(parent.metadata)) {
- _graph.makeNullable(
- type.node!, AngularAnnotationOrigin(source, node));
+ var angularAnnotation = _getAngularAnnotation(parent.metadata);
+ if (angularAnnotation != null) {
+ switch (angularAnnotation) {
+ case _AngularAnnotation.child:
+ _graph.makeNullable(
+ type.node!, AngularAnnotationOrigin(source, node));
+ break;
+ case _AngularAnnotation.children:
+ _graph.preventLate(
+ type.node!, AngularAnnotationOrigin(source, node));
+ break;
+ }
}
}
}
@@ -712,6 +721,26 @@
);
}
+ /// Determines if the given [metadata] contains a reference to one of the
+ /// Angular annotations that we have special behaviors for. If it does,
+ /// returns an enumerated value describing the type of annotation.
+ _AngularAnnotation? _getAngularAnnotation(NodeList<Annotation> metadata) {
+ for (var annotation in metadata) {
+ var element = annotation.element;
+ if (element is ConstructorElement) {
+ var name = element.enclosingElement.name;
+ if (_isAngularUri(element.librarySource.uri)) {
+ if (name == 'ViewChild' || name == 'ContentChild') {
+ return _AngularAnnotation.child;
+ } else if (name == 'ViewChildren' || name == 'ContentChildren') {
+ return _AngularAnnotation.children;
+ }
+ }
+ }
+ }
+ return null;
+ }
+
/// Common handling of function and method declarations.
DecoratedType _handleExecutableDeclaration(
AstNode node,
@@ -938,23 +967,6 @@
.recordDecoratedDirectSupertypes(declaredElement, decoratedSupertypes);
}
- /// Determines if the given [metadata] contains a reference to one of the
- /// Angular annotations `ViewChild` or `ContentChild`, either of which implies
- /// nullability of the underlying property.
- bool _hasAngularChildAnnotation(NodeList<Annotation> metadata) {
- for (var annotation in metadata) {
- var element = annotation.element;
- if (element is ConstructorElement) {
- var name = element.enclosingElement.name;
- if ((name == 'ViewChild' || name == 'ContentChild') &&
- _isAngularUri(element.librarySource.uri)) {
- return true;
- }
- }
- }
- return false;
- }
-
/// Determines whether the given [uri] comes from the Angular package.
bool _isAngularUri(Uri uri) {
if (uri.scheme != 'package') return false;
@@ -993,3 +1005,15 @@
throw UnimplementedError(buffer.toString());
}
}
+
+/// Enum describing the kinds of annotations supplied by the angular package for
+/// which we have special migration behaviors.
+enum _AngularAnnotation {
+ /// Either the `@ViewChild` or `@ContentChild` annotation. Fields with these
+ /// annotations should always be nullable and should never be late.
+ child,
+
+ /// Either the `@ViewChildren` or `@ContentChildren` annotation. Fields with
+ /// these annotations should never be late.
+ children,
+}
diff --git a/pkg/nnbd_migration/lib/src/nullability_node.dart b/pkg/nnbd_migration/lib/src/nullability_node.dart
index 8030833..f730d53 100644
--- a/pkg/nnbd_migration/lib/src/nullability_node.dart
+++ b/pkg/nnbd_migration/lib/src/nullability_node.dart
@@ -337,6 +337,12 @@
_pathsBeingMigrated.add(source.fullName);
}
+ /// Creates a graph edge that will try to prevent the given [node] from being
+ /// marked `late`.
+ void preventLate(NullabilityNode node, EdgeOrigin origin) {
+ connect(node, always, origin);
+ }
+
/// Determines the nullability of each node in the graph by propagating
/// nullability information from one node to another.
PropagationResult propagate() {
diff --git a/pkg/nnbd_migration/test/abstract_context.dart b/pkg/nnbd_migration/test/abstract_context.dart
index 3d5267f..51c7632 100644
--- a/pkg/nnbd_migration/test/abstract_context.dart
+++ b/pkg/nnbd_migration/test/abstract_context.dart
@@ -60,12 +60,18 @@
class ContentChild {
const ContentChild(Object selector, {Object? read});
}
+class ContentChildren {
+ const ContentChildren(Object selector, {Object? read});
+}
class Optional {
const Optional();
}
class ViewChild {
const ViewChild(Object selector, {Object? read});
}
+class ViewChildren {
+ const ViewChildren(Object selector, {Object? read});
+}
''');
if (internalUris) {
addPackageFile('angular', 'angular.dart', '''
diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart
index 6e5e609..2301e5b 100644
--- a/pkg/nnbd_migration/test/api_test.dart
+++ b/pkg/nnbd_migration/test/api_test.dart
@@ -387,6 +387,88 @@
await _checkSingleFileChanges(content, expected);
}
+ Future<void> test_angular_contentChild_field_not_late() async {
+ addAngularPackage();
+ var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class MyComponent {
+ @ContentChild('foo')
+ Element bar;
+ Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar.id;
+ h() => baz.id;
+}
+''';
+ // `late` heuristics are disabled for `bar` since it's marked with
+ // `ContentChild`. But they do apply to `baz`.
+ var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class MyComponent {
+ @ContentChild('foo')
+ Element? bar;
+ late Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar!.id;
+ h() => baz.id;
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
+ Future<void> test_angular_contentChildren_field_not_late() async {
+ addAngularPackage();
+ var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class myComponent {
+ @ContentChildren('foo')
+ Element bar;
+ Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar.id;
+ h() => baz.id;
+}
+''';
+ // `late` heuristics are disabled for `bar` since it's marked with
+ // `ContentChildren`. But they do apply to `baz`.
+ var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class myComponent {
+ @ContentChildren('foo')
+ Element? bar;
+ late Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar!.id;
+ h() => baz.id;
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
Future<void> test_angular_optional_constructor_param() async {
addAngularPackage();
var content = '''
@@ -508,6 +590,47 @@
await _checkSingleFileChanges(content, expected);
}
+ Future<void> test_angular_viewChild_field_not_late() async {
+ addAngularPackage();
+ var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class MyComponent {
+ @ViewChild('foo')
+ Element bar;
+ Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar.id;
+ h() => baz.id;
+}
+''';
+ // `late` heuristics are disabled for `bar` since it's marked with
+ // `ViewChild`. But they do apply to `baz`.
+ var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class MyComponent {
+ @ViewChild('foo')
+ Element? bar;
+ late Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar!.id;
+ h() => baz.id;
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
Future<void> test_angular_viewChild_setter() async {
addAngularPackage();
var content = '''
@@ -531,6 +654,47 @@
await _checkSingleFileChanges(content, expected);
}
+ Future<void> test_angular_viewChildren_field_not_late() async {
+ addAngularPackage();
+ var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class myComponent {
+ @ViewChildren('foo')
+ Element bar;
+ Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar.id;
+ h() => baz.id;
+}
+''';
+ // `late` heuristics are disabled for `bar` since it's marked with
+ // `ViewChildren`. But they do apply to `baz`.
+ var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+class myComponent {
+ @ViewChildren('foo')
+ Element? bar;
+ late Element baz;
+
+ f(Element e) {
+ bar = e;
+ baz = e;
+ }
+ g() => bar!.id;
+ h() => baz.id;
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
Future<void> test_argumentError_checkNotNull_implies_non_null_intent() async {
var content = '''
void f(int i) {
