From f66037acce9222718a0158e3f61590f5608a39a2 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Wed, 11 Aug 2021 01:55:41 +0000 Subject: [PATCH] Migration: add support for Angular's `@ViewChild()` and `@ContentChild()` annotations. Bug: https://github.com/dart-lang/sdk/issues/45661 Change-Id: Ie77d720677fb032585e17fe13358afe597e702bd Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/209820 Reviewed-by: Samuel Rawlins Reviewed-by: Konstantin Shcheglov Commit-Queue: Paul Berry --- pkg/nnbd_migration/lib/instrumentation.dart | 2 +- pkg/nnbd_migration/lib/src/edge_origin.dart | 25 ++-- pkg/nnbd_migration/lib/src/node_builder.dart | 33 ++++- pkg/nnbd_migration/test/abstract_context.dart | 6 + pkg/nnbd_migration/test/api_test.dart | 116 ++++++++++++++++++ 5 files changed, 166 insertions(+), 16 deletions(-) diff --git a/pkg/nnbd_migration/lib/instrumentation.dart b/pkg/nnbd_migration/lib/instrumentation.dart index 05719e79669..c2700c15499 100644 --- a/pkg/nnbd_migration/lib/instrumentation.dart +++ b/pkg/nnbd_migration/lib/instrumentation.dart @@ -240,6 +240,7 @@ abstract class EdgeOriginInfo { enum EdgeOriginKind { alreadyMigratedType, alwaysNullableType, + angularAnnotation, argumentErrorCheckNotNull, callTearOff, compoundAssignment, @@ -272,7 +273,6 @@ enum EdgeOriginKind { nonNullableUsage, nonNullAssertion, nullabilityComment, - optionalAnnotation, optionalFormalParameter, parameterInheritance, quiverCheckNotNull, diff --git a/pkg/nnbd_migration/lib/src/edge_origin.dart b/pkg/nnbd_migration/lib/src/edge_origin.dart index 9e9c5ecb8b2..ea9e4b1f0e3 100644 --- a/pkg/nnbd_migration/lib/src/edge_origin.dart +++ b/pkg/nnbd_migration/lib/src/edge_origin.dart @@ -64,6 +64,19 @@ class AlwaysNullableTypeOrigin extends EdgeOrigin { EdgeOriginKind get kind => EdgeOriginKind.alwaysNullableType; } +/// Edge origin resulting from the presence of an Angular annotation such as +/// `@Optional()`, `@ViewChild(...)`, or `@ContentChild(...)`. +class AngularAnnotationOrigin extends EdgeOrigin { + AngularAnnotationOrigin(Source? source, AstNode node) : super(source, node); + + @override + String get description => + "annotated with an Angular annotation indicating nullability"; + + @override + EdgeOriginKind get kind => EdgeOriginKind.angularAnnotation; +} + /// Edge origin resulting from the presence of a call to /// `ArgumentError.checkNotNull`. /// @@ -542,18 +555,6 @@ class NullabilityCommentOrigin extends EdgeOrigin { EdgeOriginKind get kind => EdgeOriginKind.nullabilityComment; } -/// Edge origin resulting from the presence of an Angular `@Optional()` -/// annotation. -class OptionalAnnotationOrigin extends EdgeOrigin { - OptionalAnnotationOrigin(Source? source, AstNode node) : super(source, node); - - @override - String get description => "annotated with Angular's @Optional() annotation"; - - @override - EdgeOriginKind get kind => EdgeOriginKind.optionalAnnotation; -} - /// Edge origin resulting from the presence of an optional formal parameter. /// /// For example, in the following code snippet: diff --git a/pkg/nnbd_migration/lib/src/node_builder.dart b/pkg/nnbd_migration/lib/src/node_builder.dart index 7203bba9441..05cb79d2459 100644 --- a/pkg/nnbd_migration/lib/src/node_builder.dart +++ b/pkg/nnbd_migration/lib/src/node_builder.dart @@ -465,9 +465,13 @@ class NodeBuilder extends GeneralizingAstVisitor _variables!.recordDecoratedElementType( declaredElement.variable, decoratedType.returnType); } else { - _variables!.recordDecoratedElementType( - declaredElement.variable, decoratedType.positionalParameters![0], + var type = decoratedType.positionalParameters![0]; + _variables!.recordDecoratedElementType(declaredElement.variable, type, soft: true); + if (_hasAngularChildAnnotation(node.metadata)) { + _graph.makeNullable( + type!.node!, AngularAnnotationOrigin(source, node)); + } } } return null; @@ -668,6 +672,12 @@ class NodeBuilder extends GeneralizingAstVisitor _variables!.recordDecoratedElementType(declaredElement, type); variable.initializer?.accept(this); } + var parent = node.parent; + if (parent is FieldDeclaration) { + if (_hasAngularChildAnnotation(parent.metadata)) { + _graph.makeNullable(type!.node!, AngularAnnotationOrigin(source, node)); + } + } return null; } @@ -800,7 +810,7 @@ class NodeBuilder extends GeneralizingAstVisitor element.enclosingElement.name == 'Optional' && _isAngularUri(element.librarySource.uri)) { _graph.makeNullable( - decoratedType!.node!, OptionalAnnotationOrigin(source, node)); + decoratedType!.node!, AngularAnnotationOrigin(source, node)); } } if (declaredElement.isNamed) { @@ -893,6 +903,23 @@ class NodeBuilder extends GeneralizingAstVisitor .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 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; diff --git a/pkg/nnbd_migration/test/abstract_context.dart b/pkg/nnbd_migration/test/abstract_context.dart index 49a0590314c..88ee43441b8 100644 --- a/pkg/nnbd_migration/test/abstract_context.dart +++ b/pkg/nnbd_migration/test/abstract_context.dart @@ -55,9 +55,15 @@ class AbstractContextTest with ResourceProviderMixin { addPackageFile( internalUris ? 'third_party.dart_src.angular.angular' : 'angular', 'angular.dart', ''' +class ContentChild { + const ContentChild(Object selector, {Object? read}); +} class Optional { const Optional(); } +class ViewChild { + const ViewChild(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 7f86efe4409..5307de5b79f 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -356,6 +356,37 @@ g() { await _checkSingleFileChanges(content, expected); } + Future test_angular_contentChild_field() async { + addAngularPackage(); + var content = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ContentChild('foo') + Element bar; +} +'''; + var expected = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ContentChild('foo') + Element? bar; +} +'''; + await _checkSingleFileChanges(content, expected); + } + Future test_angular_optional_constructor_param() async { addAngularPackage(); var content = ''' @@ -415,6 +446,91 @@ class MyComponent { await _checkSingleFileChanges(content, expected); } + Future test_angular_viewChild_field() async { + addAngularPackage(); + var content = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ViewChild('foo') + Element bar; +} +'''; + var expected = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ViewChild('foo') + Element? bar; +} +'''; + await _checkSingleFileChanges(content, expected); + } + + Future test_angular_viewChild_field_internal() async { + addAngularPackage(internalUris: true); + var content = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ViewChild('foo') + Element bar; +} +'''; + var expected = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + // Initialize this.bar in the constructor just so the migration tool doesn't + // decide to make it nullable due to the lack of initializer. + MyComponent(this.bar); + + @ViewChild('foo') + Element? bar; +} +'''; + await _checkSingleFileChanges(content, expected); + } + + Future test_angular_viewChild_setter() async { + addAngularPackage(); + var content = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + @ViewChild('foo') + set bar(Element element) {} +} +'''; + var expected = ''' +import 'dart:html'; +import 'package:angular/angular.dart'; + +class MyComponent { + @ViewChild('foo') + set bar(Element? element) {} +} +'''; + await _checkSingleFileChanges(content, expected); + } + Future test_argumentError_checkNotNull_implies_non_null_intent() async { var content = ''' void f(int i) {