Best current solution to handle all types of generic directives refs #86 (#95)

MichaelRFairhurst · web-flow · commit 593afa40db6a · 2016-11-28T16:13:59.000-08:00
* Best current solution to handle all types of generic directives refs #86 In the future, maybe there's a way to know the directive's parameters based on how its used in the template. For now, use the upper bounds because those are the lowest types and therefore can be relied upon in the template, and avoids the 'T is not subtype of String' errors. Refactoring: changed event typechecking to go off of a new type field on InputElement so that it could be deparameterized consistently at collection time rather than deparameterized everywhere its used. Same for outputs, but those already had an eventType field that was used everywhere. Opening new issue #91 for some tricky subtleties in generic chains. Looks like this code should some day use `instatiateToBounds` but that will not do much typing without strong mode turned on, and has a bug that is open in their repo. * Review feedback from jmesserly
diff --git a/analyzer_plugin/lib/src/model.dart b/analyzer_plugin/lib/src/model.dart
@@ -172,6 +172,8 @@ class InputElement extends AngularElementImpl {
 
   final dart.PropertyAccessorElement setter;
 
+  final dart.DartType setterType;
+
   /**
    * The [SourceRange] where [setter] is referenced in the input declaration.
    * May be the same as this element offset/length in shorthand variants where
@@ -180,7 +182,7 @@ class InputElement extends AngularElementImpl {
   final SourceRange setterRange;
 
   InputElement(String name, int nameOffset, int nameLength, Source source,
-      this.setter, this.setterRange)
+      this.setter, this.setterRange, this.setterType)
       : super(name, nameOffset, nameLength, source);
 
   @override
diff --git a/analyzer_plugin/lib/src/resolver.dart b/analyzer_plugin/lib/src/resolver.dart
@@ -698,12 +698,15 @@ class TemplateResolver {
         AngularWarningCode unboundErrorCode;
         var matched = false;
         if (attribute.bound == AttributeBoundType.output) {
+          // Set the event type to dynamic, for if we don't match anything
+          eventType = typeProvider.dynamicType;
           unboundErrorCode = AngularWarningCode.NONEXIST_OUTPUT_BOUND;
           for (AbstractDirective directive in directives) {
             for (OutputElement output in directive.outputs) {
+              // TODO what if this matches two directives?
               if (output.name == attribute.propertyName) {
-                // TODO what if this matches two directives?
                 eventType = output.eventType;
+                // Parameterized directives, use the lower bound
                 matched = true;
               }
             }
@@ -733,7 +736,9 @@ class TemplateResolver {
             for (OutputElement output in directive.outputs) {
               if (output.name == attribute.propertyName + "Change") {
                 outputMatched = true;
-                if (!output.eventType.isAssignableTo(expression.bestType)) {
+                var eventType = output.eventType;
+
+                if (!eventType.isAssignableTo(expression.bestType)) {
                   errorListener.onError(new AnalysisError(
                       templateSource,
                       attribute.valueOffset,
@@ -770,7 +775,8 @@ class TemplateResolver {
             for (InputElement input in directive.inputs) {
               if (input.name == attribute.propertyName) {
                 var attrType = expression.bestType;
-                var inputType = input.setter.variable.type;
+                var inputType = input.setterType;
+
                 if (!attrType.isAssignableTo(inputType)) {
                   errorListener.onError(new AnalysisError(
                       templateSource,
diff --git a/analyzer_plugin/lib/src/tasks.dart b/analyzer_plugin/lib/src/tasks.dart
@@ -406,8 +406,14 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
       PropertyAccessorElement setter =
           _resolveSetter(classElement, expression, name);
 
-      return new InputElement(boundName, boundRange.offset, boundRange.length,
-          target.source, setter, nameRange);
+      return new InputElement(
+          boundName,
+          boundRange.offset,
+          boundRange.length,
+          target.source,
+          setter,
+          nameRange,
+          _getSetterType(classElement, setter));
     } else {
       // TODO(mfairhurst) report a warning
       return null;
@@ -427,7 +433,7 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
       PropertyAccessorElement getter =
           _resolveGetter(classElement, expression, name);
 
-      var eventType = getEventType(getter, name);
+      var eventType = getEventType(classElement, getter, name);
 
       return new OutputElement(boundName, boundRange.offset, boundRange.length,
           target.source, getter, nameRange, eventType);
@@ -437,15 +443,26 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
     }
   }
 
-  DartType getEventType(PropertyAccessorElement getter, String name) {
+  DartType _getSetterType(
+      ClassElement classElement, PropertyAccessorElement setter) {
+    if (setter != null && setter.variable != null) {
+      var type = setter.variable.type;
+      return _deparameterizeType(classElement, type);
+    }
+
+    return null;
+  }
+
+  DartType getEventType(
+      ClassElement classElement, PropertyAccessorElement getter, String name) {
     if (getter != null && getter.type != null) {
       var returnType = getter.type.returnType;
       if (returnType != null && returnType is InterfaceType) {
         dart.DartType streamType = typeProvider.streamType;
         dart.DartType streamedType =
             context.typeSystem.mostSpecificTypeArgument(returnType, streamType);
         if (streamedType != null) {
-          return streamedType;
+          return _deparameterizeType(classElement, streamedType);
         } else {
           errorReporter.reportErrorForNode(
               AngularWarningCode.OUTPUT_MUST_BE_EVENTEMITTER,
@@ -463,6 +480,25 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
     return typeProvider.dynamicType;
   }
 
+  DartType _deparameterizeType(
+      ClassElement classElement, DartType parameterizedType) {
+    if (parameterizedType == null) {
+      return null;
+    }
+
+    // See #91 for discussion about bugs related to bounds
+    var getBound = (p) {
+      return p.bound == null
+          ? typeProvider.dynamicType
+          : p.bound.resolveToBound(typeProvider.dynamicType);
+    };
+
+    var getType = (p) => p.type;
+    var bounds = classElement.typeParameters.map(getBound).toList();
+    var parameters = classElement.typeParameters.map(getType).toList();
+    return parameterizedType.substitute2(bounds, parameters);
+  }
+
   List<InputElement> _parseHeaderInputs(
       ClassElement classElement, ast.Annotation node) {
     ast.ListLiteral descList = _getListLiteralNamedArgument(
@@ -504,8 +540,12 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
    * the given `@Input` or `@Output` [annotation], and add it to the
    * [inputElements] or [outputElements] array.
    */
-  _parseMemberInputOrOutput(ast.ClassMember node, ast.Annotation annotation,
-      List<InputElement> inputElements, List<OutputElement> outputElements) {
+  _parseMemberInputOrOutput(
+      ClassElement classElement,
+      ast.ClassMember node,
+      ast.Annotation annotation,
+      List<InputElement> inputElements,
+      List<OutputElement> outputElements) {
     // analyze the annotation
     final isInput = _isAngularAnnotation(annotation, 'Input');
     final isOutput = _isAngularAnnotation(annotation, 'Output');
@@ -568,9 +608,15 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
 
     if (isInput) {
       inputElements.add(new InputElement(
-          name, nameOffset, nameLength, target.source, property, null));
+          name,
+          nameOffset,
+          nameLength,
+          target.source,
+          property,
+          null,
+          _getSetterType(classElement, property)));
     } else {
-      var eventType = getEventType(property, name);
+      var eventType = getEventType(classElement, property, name);
       outputElements.add(new OutputElement(name, nameOffset, nameLength,
           target.source, property, null, eventType));
     }
@@ -585,7 +631,7 @@ class BuildUnitDirectivesTask extends SourceBasedAnalysisTask
     for (ast.ClassMember member in node.members) {
       for (ast.Annotation annotation in member.metadata) {
         _parseMemberInputOrOutput(
-            member, annotation, inputElements, outputElements);
+            node.element, member, annotation, inputElements, outputElements);
       }
     }
   }
@@ -1369,8 +1415,14 @@ class _BuildStandardHtmlComponentsVisitor extends RecursiveAstVisitor {
       String name = accessor.displayName;
       if (!inputMap.containsKey(name)) {
         if (accessor.isSetter) {
-          inputMap[name] = new InputElement(name, accessor.nameOffset,
-              accessor.nameLength, accessor.source, accessor, null);
+          inputMap[name] = new InputElement(
+              name,
+              accessor.nameOffset,
+              accessor.nameLength,
+              accessor.source,
+              accessor,
+              null,
+              accessor.variable.type);
         }
       }
     });
diff --git a/analyzer_plugin/test/resolver_test.dart b/analyzer_plugin/test/resolver_test.dart
@@ -552,6 +552,184 @@ class TestPanel {
         AngularWarningCode.CSS_UNIT_BINDING_NOT_NUMBER, code, "notNumber");
   }
 
+  void test_expression_inputAndOutputBinding_genericDirective_ok() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  String string;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T> {
+  @Output() EventEmitter<T> output;
+  @Input() T input;
+
+  @Output() EventEmitter<T> twoWayChange;
+  @Input() T twoWay;
+}
+''');
+    var code = r"""
+<generic-comp (output)='$event.length' [input]="string" [(twoWay)]="string"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    errorListener.assertNoErrors();
+  }
+
+  void test_expression_inputAndOutputBinding_genericDirective_chain_ok() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  String string;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends E, E> {
+  @Output() EventEmitter<T> output;
+  @Input() T input;
+
+  @Output() EventEmitter<T> twoWayChange;
+  @Input() T twoWay;
+}
+''');
+    var code = r"""
+<generic-comp (output)='$event.length' [input]="string" [(twoWay)]="string"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    errorListener.assertNoErrors();
+  }
+
+  void test_expression_inputAndOutputBinding_genericDirective_nested_ok() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  List<String> stringList;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T> {
+  @Output() EventEmitter<List<T>> output;
+  @Input() List<T> input;
+
+  @Output() EventEmitter<List<T>> twoWayChange;
+  @Input() List<T> twoWay;
+}
+''');
+    var code = r"""
+<generic-comp (output)='$event[0].length' [input]="stringList" [(twoWay)]="stringList"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    errorListener.assertNoErrors();
+  }
+
+  void test_expression_inputBinding_genericDirective_lowerBoundTypeError() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  int notString;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends String> {
+  @Input() T string;
+}
+''');
+    var code = r"""
+<generic-comp [string]="notString"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    assertErrorInCodeAtPosition(
+        AngularWarningCode.INPUT_BINDING_TYPE_ERROR, code, "notString");
+  }
+
+  void test_expression_input_genericDirective_lowerBoundChainTypeError() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  int notString;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends O, O extends String> {
+  @Input() T string;
+}
+''');
+    var code = r"""
+<generic-comp [string]="notString"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    assertErrorInCodeAtPosition(
+        AngularWarningCode.INPUT_BINDING_TYPE_ERROR, code, "notString");
+  }
+
+  void test_expression_input_genericDirective_lowerBoundNestedTypeError() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  List<int> notStringList;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends String> {
+  @Input() List<T> stringList;
+}
+''');
+    var code = r"""
+<generic-comp [stringList]="notStringList"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    assertErrorInCodeAtPosition(
+        AngularWarningCode.INPUT_BINDING_TYPE_ERROR, code, "notStringList");
+  }
+
+  void test_expression_outputBinding_genericDirective_lowerBoundTypeError() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  takeInt(int i) {}
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends String> {
+  @Output() EventEmitter<T> string;
+}
+''');
+    var code = r"""
+<generic-comp (string)="takeInt($event)"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    assertErrorInCodeAtPosition(
+        StaticWarningCode.ARGUMENT_TYPE_NOT_ASSIGNABLE, code, r"$event");
+  }
+
+  void test_expression_twoWayBinding_genericDirective_lowerBoundTypeError() {
+    _addDartSource(r'''
+@Component(selector: 'test-panel',
+    directives: const [GenericComponent], templateUrl: 'test_panel.html')
+class TestPanel {
+  int anInt;
+}
+@Component(selector: 'generic-comp', template: '')
+class GenericComponent<T extends String> {
+  @Output() EventEmitter<T> stringChange;
+  @Input() dynamic string;
+}
+''');
+    var code = r"""
+<generic-comp [(string)]="anInt"></generic-comp>
+""";
+    _addHtmlSource(code);
+    _resolveSingleTemplate(dartSource);
+    assertErrorInCodeAtPosition(
+        AngularWarningCode.TWO_WAY_BINDING_OUTPUT_TYPE_ERROR, code, "anInt");
+  }
+
   void test_inheritedFields() {
     _addDartSource(r'''
 class BaseComponent {
diff --git a/analyzer_plugin/test/tasks_test.dart b/analyzer_plugin/test/tasks_test.dart
