Add hints for Angular constructor arguments

The rules are:

1. Arguments annotated with `@Optional` must be nullable (that part
   worked before this change).
2. Arguments annotated with `@Attribute` must be nullable if it's
   a constructor of a component (`@Attribute` can also appear on
   arguments of directives constructors, it doesn't provide any signal
   in the latter case).
3. Arguments of constructors of components or `@Injectable` classes,
   that have neither `@Optional` nor `@Attribute`, must be non-nullable.

Tested:
  Added new test cases to `api_test.dart`
Change-Id: I0f06a128e0f6ec49d39cc6bf025fe1796938f1f1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/272900
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Ilya Yanok <[email protected]>

Author: Ilya Yanok

pkg/nnbd_migration/lib/instrumentation.dart

@@ -251,6 +251,7 @@ enum EdgeOriginKind {
   alreadyMigratedType,
   alwaysNullableType,
   angularAnnotation,
+  angularConstructorArgument,
   argumentErrorCheckNotNull,
   assignmentFromAngularInjectorGet,
   builtValueNullableAnnotation,

pkg/nnbd_migration/lib/src/edge_origin.dart

@@ -64,7 +64,7 @@ class AlwaysNullableTypeOrigin extends EdgeOrigin {
 }
 
 /// Edge origin resulting from the presence of an Angular annotation such as
-/// `@Optional()`, `@ViewChild(...)`, or `@ContentChild(...)`.
+/// `@Optional()`, `@Attribute(...)`, `@ViewChild(...)`, or `@ContentChild(...)`.
 class AngularAnnotationOrigin extends EdgeOrigin {
   AngularAnnotationOrigin(super.source, AstNode super.node);
 
@@ -76,6 +76,21 @@ class AngularAnnotationOrigin extends EdgeOrigin {
   EdgeOriginKind get kind => EdgeOriginKind.angularAnnotation;
 }
 
+/// Edge origin resulting from `@Component(...)` or `@Injectable()` class
+/// constructor arguments having neither `@Optional()` nor `@Attribute()`
+/// annotation.
+class AngularConstructorArgumentOrigin extends EdgeOrigin {
+  AngularConstructorArgumentOrigin(super.source, AstNode super.node);
+
+  @override
+  String get description =>
+      'Component or @Injectable() class constructor arguments without'
+      ' @Optional/@Attribute annotations must be non-nullable';
+
+  @override
+  EdgeOriginKind get kind => EdgeOriginKind.angularConstructorArgument;
+}
+
 /// Edge origin resulting from the presence of a call to
 /// `ArgumentError.checkNotNull`.
 ///

pkg/nnbd_migration/lib/src/node_builder.dart

@@ -57,6 +57,10 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
   /// Otherwise `null`.
   NullabilityNodeTarget? _target;
 
+  /// [ClassDeclaration] for the current class or `null` if we are currently
+  /// not inside a class declaration.
+  ClassDeclaration? _classDeclaration;
+
   final NullabilityMigrationListener? listener;
 
   final NullabilityMigrationInstrumentation? instrumentation;
@@ -80,6 +84,16 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
     return NullabilityNodeTarget.text('unknown');
   }
 
+  bool get _isInjectable =>
+      _classDeclaration?.metadata
+          .any((ann) => _isAngularConstructor(ann.element, 'Injectable')) ??
+      false;
+  bool get _isInsideAngularComponent =>
+      _classDeclaration?.metadata
+          .toList()
+          .any((ann) => _isAngularConstructor(ann.element, 'Component')) ??
+      false;
+
   @override
   DecoratedType? visitAsExpression(AsExpression node) {
     node.expression.accept(this);
@@ -130,7 +144,9 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
     node.metadata.accept(this);
     node.typeParameters?.accept(this);
     node.nativeClause?.accept(this);
+    _classDeclaration = node;
     node.members.accept(this);
+    _classDeclaration = null;
     var classElement = node.declaredElement!;
     _handleSupertypeClauses(node, classElement, node.extendsClause?.superclass,
         node.withClause, node.implementsClause, null);
@@ -841,15 +857,26 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
       _handleNullabilityHint(node, decoratedType);
     }
     _variables.recordDecoratedElementType(declaredElement, decoratedType);
+    var isAnnotated = false;
     for (var annotation in node.metadata) {
       var element = annotation.element;
-      if (element is ConstructorElement &&
-          element.enclosingElement.name == 'Optional' &&
-          _isAngularUri(element.librarySource.uri)) {
-        _graph.makeNullable(
-            decoratedType!.node, AngularAnnotationOrigin(source, node));
+      if (_isAngularConstructor(element, 'Optional') ||
+          _isAngularConstructor(element, 'Attribute')) {
+        isAnnotated = true;
+        if (_isAngularConstructor(element, 'Optional') ||
+            _isInsideAngularComponent) {
+          _graph.makeNullable(
+              decoratedType!.node, AngularAnnotationOrigin(source, node));
+        }
       }
     }
+    if (declaredElement.enclosingElement is ConstructorElement &&
+        (_isInsideAngularComponent || _isInjectable) &&
+        !isAnnotated) {
+      _graph.makeNonNullable(
+          decoratedType!.node, AngularConstructorArgumentOrigin(source, node));
+    }
+
     if (declaredElement.isNamed) {
       _namedParameters![declaredElement.name] = decoratedType!;
     } else {
@@ -938,6 +965,13 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
         declaredElement, decoratedSupertypes);
   }
 
+  /// Determines whether [element] is a constructor named [name] from Angular
+  /// package.
+  bool _isAngularConstructor(Element? element, String name) =>
+      element is ConstructorElement &&
+      element.enclosingElement.name == name &&
+      _isAngularUri(element.librarySource.uri);
+
   /// Determines whether the given [uri] comes from the Angular package.
   bool _isAngularUri(Uri uri) {
     if (!uri.isScheme('package')) return false;

pkg/nnbd_migration/test/abstract_context.dart

@@ -56,6 +56,15 @@ class AbstractContextTest with ResourceProviderMixin {
     addPackageFile(
         internalUris ? 'third_party.dart_src.angular.angular' : 'angular',
         'angular.dart', '''
+class Component {
+  const Component({
+    required String selector,
+    // optional arguments skipped
+  })
+}
+class Attribute {
+  const Attribute(String attributeName);
+}
 class ContentChild {
   const ContentChild(Object selector, {Object? read});
 }
@@ -74,6 +83,9 @@ class ViewChildren {
 class Injector {
   dynamic get(Object token, [Object? notFoundValue]) => null;
 }
+class Injectable {
+  const Injectable();
+}
 ''');
     if (internalUris) {
       addPackageFile('angular', 'angular.dart', '''

pkg/nnbd_migration/test/api_test.dart

@@ -356,6 +356,66 @@ g() {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_angular_component_attribute() async {
+    addAngularPackage();
+    var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Component(
+  selector: 'my-component'
+)
+class MyComponent {
+  int foo;
+  MyComponent(@Attribute('foo') this.foo);
+}
+''';
+    var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Component(
+  selector: 'my-component'
+)
+class MyComponent {
+  int? foo;
+  MyComponent(@Attribute('foo') this.foo);
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
+  Future<void> test_angular_component_constructor() async {
+    addAngularPackage();
+    var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Component(
+  selector: 'my-component'
+)
+class MyComponent {
+  int foo;
+  MyComponent(this.foo);
+  void nullifyFoo() { foo = null; }
+}
+''';
+    var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Component(
+  selector: 'my-component'
+)
+class MyComponent {
+  int? foo;
+  MyComponent(int this.foo);
+  void nullifyFoo() { foo = null; }
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_angular_contentChild_field() async {
     addAngularPackage();
     var content = '''
@@ -469,6 +529,33 @@ class myComponent {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_angular_injectable_constructor() async {
+    addAngularPackage();
+    var content = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Injectable()
+class MyClass {
+  int foo;
+  MyClass(this.foo);
+  void nullifyFoo() { foo = null; }
+}
+''';
+    var expected = '''
+import 'dart:html';
+import 'package:angular/angular.dart';
+
+@Injectable()
+class MyClass {
+  int? foo;
+  MyClass(int this.foo);
+  void nullifyFoo() { foo = null; }
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void> test_angular_optional_constructor_param() async {
     addAngularPackage();
     var content = '''