Validation of `@protected` method invocations.

pkg/analyzer/lib/src/generated/resolver.dart

Index: pkg/analyzer/lib/src/generated/resolver.dart
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index 59359c19e033baa505730e881a6fa2a963e29a0b..66f6fc4235e2cb8221d8b650429184a141b0ce89 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -206,6 +206,7 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
   @override
   Object visitMethodInvocation(MethodInvocation node) {
     _checkForCanBeNullAfterNullAware(node.realTarget, node.operator);
+    _checkForInvalidProtectedMethodCalls(node);
     return super.visitMethodInvocation(node);
   }
 
@@ -608,6 +609,38 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
   }
 
   /**
+   * Produces a hint if the given invocation is of a protected method outside
+   * a subclass instance method.
+   */
+  void _checkForInvalidProtectedMethodCalls(MethodInvocation node) {
+    Element element = node.methodName.staticElement;

[Brian Wilkerson, 2016/02/23 17:45:56]

Given that this is a hint, I suspect that we want to use "bestElement" rather
than "staticElement".

[pquitslund, 2016/02/23 22:59:07]

Done.

+    if (element == null || !element.isProtected) {
+      return;
+    }
+
+    ClassElement definingClass = element.enclosingElement;
+
+    MethodDeclaration decl =
+        node.getAncestor((AstNode node) => node is MethodDeclaration);
+    if (decl == null) {
+      _errorReporter.reportErrorForNode(
+          HintCode.INVALID_USE_OF_PROTECTED_MEMBER,
+          node,
+          [node.methodName.toString(), definingClass.name]);
+      return;
+    }
+
+    ClassElement invokingClass = decl.element?.enclosingElement;
+    if (invokingClass != null &&
+        !invokingClass.type.isSubtypeOf(definingClass.type)) {

[Brian Wilkerson, 2016/02/23 17:45:56]

The comment for the annotation says "extend or mix in", but the use of
"isSubtypeOf" will also allow classes that implement the interface of the
defining class, such as:

class Foo {
  @protected
  void m1() {}
}

class Bar implements Foo {
  void m2() {
    m1(); // Expect a hint here.
  }
}

I think we need to explicitly walk the superclass chain (looking at mixins) in
order to get the right behavior.

[pquitslund, 2016/02/23 22:59:07]

Fixed.

+      _errorReporter.reportErrorForNode(
+          HintCode.INVALID_USE_OF_PROTECTED_MEMBER,
+          node,
+          [node.methodName.toString(), definingClass.name]);
+    }
+  }
+
+  /**
    * Check that the imported library does not define a loadLibrary function. The import has already
    * been determined to be deferred when this is called.
    *
@@ -8387,26 +8420,6 @@ class ResolverVisitor extends ScopedVisitor {
     return null;
   }
 
-  void _inferArgumentTypesFromContext(InvocationExpression node) {
-    DartType contextType = node.staticInvokeType;
-    if (contextType is FunctionType) {
-      DartType originalType = node.function.staticType;
-      DartType returnContextType = InferenceContext.getType(node);
-      TypeSystem ts = typeSystem;
-      if (returnContextType != null &&
-          node.typeArguments == null &&
-          originalType is FunctionType &&
-          originalType.typeFormals.isNotEmpty &&
-          ts is StrongTypeSystemImpl) {
-
-        contextType = ts.inferGenericFunctionCall(typeProvider, originalType,
-            DartType.EMPTY_LIST, DartType.EMPTY_LIST, returnContextType);
-      }
-
-      InferenceContext.setType(node.argumentList, contextType);
-    }
-  }
-
   @override
   Object visitNamedExpression(NamedExpression node) {
     InferenceContext.setType(node.expression, InferenceContext.getType(node));
@@ -8734,6 +8747,25 @@ class ResolverVisitor extends ScopedVisitor {
     return null;
   }
 
+  void _inferArgumentTypesFromContext(InvocationExpression node) {
+    DartType contextType = node.staticInvokeType;
+    if (contextType is FunctionType) {
+      DartType originalType = node.function.staticType;
+      DartType returnContextType = InferenceContext.getType(node);
+      TypeSystem ts = typeSystem;
+      if (returnContextType != null &&
+          node.typeArguments == null &&
+          originalType is FunctionType &&
+          originalType.typeFormals.isNotEmpty &&
+          ts is StrongTypeSystemImpl) {
+        contextType = ts.inferGenericFunctionCall(typeProvider, originalType,
+            DartType.EMPTY_LIST, DartType.EMPTY_LIST, returnContextType);
+      }
+
+      InferenceContext.setType(node.argumentList, contextType);
+    }
+  }
+
   void _inferFormalParameterList(FormalParameterList node, DartType type) {
     if (typeAnalyzer.inferFormalParameterList(node, type)) {
       // TODO(leafp): This gets dropped on the floor if we're in the field