Enforce @VisibleForTesting annotations.

In particular, enforce that a method annotated with @VisibleForTesting can only
be referenced from within the declaring library, or within a file that has
"/test" in its path. This allows @VisibleForTesting methods to be accessed from
test files, or files in "/testing" folders, etc.

Bug: #28273
Change-Id: I3c19f73be330c49face7e3203879742d514bca74
Reviewed-on: https://dart-review.googlesource.com/27201
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: srawlins

pkg/analyzer/lib/dart/element/element.dart

@@ -665,6 +665,10 @@ abstract class Element implements AnalysisTarget, ResolutionTarget {
    */
   bool get isSynthetic;
 
+  /// Return `true` if this element has an annotation of the form
+  /// '@visibleForTesting'.
+  bool get isVisibleForTesting;
+
   /**
    * Return the kind of element that this is.
    */
@@ -873,6 +877,10 @@ abstract class ElementAnnotation
    */
   bool get isRequired;
 
+  /// Return `true` if this annotation marks the associated member as being
+  /// visible for testing.
+  bool get isVisibleForTesting;
+
   /**
    * Return a representation of the value of this annotation, forcing the value
    * to be computed if it had not previously been computed, or `null` if the

pkg/analyzer/lib/error/error.dart

@@ -261,6 +261,7 @@ const List<ErrorCode> errorCodeValues = const [
   HintCode.INVALID_METHOD_OVERRIDE_TYPE_PARAMETER_BOUND,
   HintCode.INVALID_REQUIRED_PARAM,
   HintCode.INVALID_USE_OF_PROTECTED_MEMBER,
+  HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER,
   HintCode.IS_DOUBLE,
   HintCode.IS_INT,
   HintCode.IS_NOT_DOUBLE,

pkg/analyzer/lib/src/dart/element/element.dart

@@ -2874,6 +2874,10 @@ class ElementAnnotationImpl implements ElementAnnotation {
    */
   static String _REQUIRED_VARIABLE_NAME = "required";
 
+  /// The name of the top-level variable used to mark a method as being
+  /// visible for testing.
+  static String _VISIBLE_FOR_TESTING_VARIABLE_NAME = "visibleForTesting";
+
   /**
    * The element representing the field, variable, or constructor being used as
    * an annotation.
@@ -2988,6 +2992,12 @@ class ElementAnnotationImpl implements ElementAnnotation {
           element.name == _REQUIRED_VARIABLE_NAME &&
           element.library?.name == _META_LIB_NAME;
 
+  @override
+  bool get isVisibleForTesting =>
+      element is PropertyAccessorElement &&
+      element.name == _VISIBLE_FOR_TESTING_VARIABLE_NAME &&
+      element.library?.name == _META_LIB_NAME;
+
   /**
    * Get the library containing this annotation.
    */
@@ -3259,6 +3269,10 @@ abstract class ElementImpl implements Element {
     setModifier(Modifier.SYNTHETIC, isSynthetic);
   }
 
+  @override
+  bool get isVisibleForTesting => metadata
+      .any((ElementAnnotation annotation) => annotation.isVisibleForTesting);
+
   @override
   LibraryElement get library =>
       getAncestor((element) => element is LibraryElement);
@@ -7464,6 +7478,9 @@ class MultiplyDefinedElementImpl implements MultiplyDefinedElement {
   @override
   bool get isSynthetic => true;
 
+  @override
+  bool get isVisibleForTesting => false;
+
   @override
   ElementKind get kind => ElementKind.ERROR;
 

pkg/analyzer/lib/src/dart/element/handle.dart

@@ -377,6 +377,9 @@ abstract class ElementHandle implements Element {
   @override
   bool get isSynthetic => actualElement.isSynthetic;
 
+  @override
+  bool get isVisibleForTesting => actualElement.isVisibleForTesting;
+
   @override
   LibraryElement get library =>
       getAncestor((element) => element is LibraryElement);

pkg/analyzer/lib/src/dart/element/member.dart

@@ -423,6 +423,9 @@ abstract class Member implements Element {
   @override
   bool get isSynthetic => _baseElement.isSynthetic;
 
+  @override
+  bool get isVisibleForTesting => _baseElement.isVisibleForTesting;
+
   @override
   ElementKind get kind => _baseElement.kind;
 

pkg/analyzer/lib/src/dart/error/hint_codes.dart

@@ -303,6 +303,16 @@ class HintCode extends ErrorCode {
       "The member '{0}' can only be used within instance members of subclasses "
       "of '{1}'.");
 
+  /// This hint is generated anywhere where a member annotated with
+  /// `@visibleForTesting` is used outside the defining library, or a test.
+  ///
+  /// Parameters:
+  /// 0: the name of the member
+  /// 1: the name of the defining class
+  static const HintCode INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER =
+      const HintCode('INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER',
+          "The member '{0}' can only be used within '{1}' or a test.");
+
   /**
    * Hint for the `x is double` type checks.
    */

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

@@ -32,6 +32,7 @@ import 'package:analyzer/src/generated/static_type_analyzer.dart';
 import 'package:analyzer/src/generated/testing/element_factory.dart';
 import 'package:analyzer/src/generated/type_system.dart';
 import 'package:analyzer/src/generated/utilities_dart.dart';
+import 'package:path/path.dart' as path;
 
 export 'package:analyzer/src/dart/resolver/inheritance_manager.dart';
 export 'package:analyzer/src/dart/resolver/scope.dart';
@@ -48,6 +49,8 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
 
   static String _TO_INT_METHOD_NAME = "toInt";
 
+  static final _testDir = '${path.separator}test${path.separator}';
+
   /**
    * The class containing the AST nodes being visited, or `null` if we are not in the scope of
    * a class.
@@ -353,7 +356,7 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
   @override
   Object visitSimpleIdentifier(SimpleIdentifier node) {
     _checkForDeprecatedMemberUseAtIdentifier(node);
-    _checkForInvalidProtectedMemberAccess(node);
+    _checkForInvalidAccess(node);
     return super.visitSimpleIdentifier(node);
   }
 
@@ -902,11 +905,17 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
         decl.name, [decl.name.toString()]);
   }
 
-  /**
-   * Produces a hint if the given identifier is a protected closure, field or
-   * getter/setter, method closure or invocation accessed outside a subclass.
-   */
-  void _checkForInvalidProtectedMemberAccess(SimpleIdentifier identifier) {
+  /// Produces a hint if [identifier] is accessed from an invalid location. In
+  /// particular:
+  ///
+  /// * if the given identifier is a protected closure, field or
+  ///   getter/setter, method closure or invocation accessed outside a subclass,
+  ///   or accessed outside the library wherein the identifier is declared, or
+  /// * if the given identifier is a closure, field, getter, setter, method
+  ///   closure or invocation which is annotated with `visibleForTesting`, and
+  ///   is accessed outside of the defining library, and the current library
+  ///   does not have the word 'test' in its name.
+  void _checkForInvalidAccess(SimpleIdentifier identifier) {
     if (identifier.inDeclarationContext()) {
       return;
     }
@@ -925,28 +934,60 @@ class BestPracticesVerifier extends RecursiveAstVisitor<Object> {
       return false;
     }
 
+    bool isVisibleForTesting(Element element) {
+      if (element == null) {
+        return false;
+      }
+      if (element.isVisibleForTesting) {
+        return true;
+      }
+      if (element is PropertyAccessorElement &&
+          element.enclosingElement is ClassElement &&
+          element.variable.isVisibleForTesting) {
+        return true;
+      }
+      return false;
+    }
+
     bool inCommentReference(SimpleIdentifier identifier) =>
         identifier.getAncestor((AstNode node) => node is CommentReference) !=
         null;
 
     bool inCurrentLibrary(Element element) =>
         element.library == _currentLibrary;
 
+    bool inTestDirectory(LibraryElement library) =>
+        library.definingCompilationUnit.source.fullName.contains(_testDir);
+
     Element element = identifier.bestElement;
-    if (isProtected(element) &&
-        !inCurrentLibrary(element) &&
-        !inCommentReference(identifier)) {
+    if (isProtected(element)) {
+      if (inCurrentLibrary(element) || inCommentReference(identifier)) {
+        // The access is valid; even if [element] is also marked
+        // `visibleForTesting`, the "visibilities" are unioned.
+        return;
+      }
       ClassElement definingClass = element.enclosingElement;
       ClassDeclaration accessingClass =
           identifier.getAncestor((AstNode node) => node is ClassDeclaration);
-      if (accessingClass == null ||
-          !_hasTypeOrSuperType(accessingClass.element, definingClass.type)) {
+      if (_hasTypeOrSuperType(accessingClass?.element, definingClass.type)) {
+        return;
+      } else {
         _errorReporter.reportErrorForNode(
             HintCode.INVALID_USE_OF_PROTECTED_MEMBER,
             identifier,
             [identifier.name.toString(), definingClass.name]);
       }
     }
+    if (isVisibleForTesting(element) &&
+        !inCurrentLibrary(element) &&
+        !inTestDirectory(_currentLibrary) &&
+        !inCommentReference(identifier)) {
+      Element definingClass = element.enclosingElement;
+      _errorReporter.reportErrorForNode(
+          HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER,
+          identifier,
+          [identifier.name.toString(), definingClass.name]);
+    }
   }
 
   /**

pkg/analyzer/test/generated/hint_code_test.dart

@@ -38,10 +38,7 @@ const _Literal literal = const _Literal();
 const _MustCallSuper mustCallSuper = const _MustCallSuper();
 const _Protected protected = const _Protected();
 const Required required = const Required();
-class Required {
-  final String reason;
-  const Required([this.reason]);
-}
+const _VisibleForTesting visibleForTesting = const _VisibleForTesting();
 
 class Immutable {
   final String reason;
@@ -62,9 +59,12 @@ class _MustCallSuper {
 class _Protected {
   const _Protected();
 }
-class _Required {
+class Required {
   final String reason;
-  const _Required([this.reason]);
+  const Required([this.reason]);
+}
+class _VisibleForTesting {
+  const _VisibleForTesting();
 }
 '''
       ],
@@ -1923,6 +1923,141 @@ main() {
     verify([source]);
   }
 
+  test_invalidUseOfVisibleForTestingMember_method() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  @visibleForTesting
+  void a(){ }
+}
+''');
+    Source source2 = addNamedSource('/lib2.dart', r'''
+import 'lib1.dart';
+
+class B {
+  void b() => new A().a();
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertErrors(source2, [HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER]);
+    verify([source, source2]);
+  }
+
+  test_invalidUseOfVisibleForTestingMember_method_OK() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  @visibleForTesting
+  void a(){ }
+}
+''');
+    Source source2 = addNamedSource('/test/test1.dart', r'''
+import '../lib1.dart';
+
+class B {
+  void b() => new A().a();
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertNoErrors(source2);
+    verify([source, source2]);
+  }
+
+  test_invalidUseProtectedAndForTesting_method_OK() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  @protected
+  @visibleForTesting
+  void a(){ }
+}
+''');
+    Source source2 = addNamedSource('/lib2.dart', r'''
+import 'lib1.dart';
+
+class B extends A {
+  void b() => new A().a();
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertNoErrors(source2);
+    verify([source, source2]);
+  }
+
+  test_invalidUseOfVisibleForTestingMember_propertyAccess() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  @visibleForTesting
+  int get a => 7;
+
+  @visibleForTesting
+  set b(_) => 7;
+}
+''');
+    Source source2 = addNamedSource('/lib2.dart', r'''
+import 'lib1.dart';
+
+void main() {
+  new A().a;
+  new A().b = 6;
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertErrors(source2, [
+      HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER,
+      HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER
+    ]);
+    verify([source, source2]);
+  }
+
+  test_invalidUseOfVisibleForTestingMember_constructor() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  int _x;
+
+  @visibleForTesting
+  A.forTesting(this._x);
+}
+''');
+    Source source2 = addNamedSource('/lib2.dart', r'''
+import 'lib1.dart';
+
+void main() {
+  new A.forTesting(0);
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertErrors(source2, [HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER]);
+    verify([source, source2]);
+  }
+
+  test_invalidUseOfVisibleForTestingMember_topLevelFunction() async {
+    Source source = addNamedSource('/lib1.dart', r'''
+import 'package:meta/meta.dart';
+
+@visibleForTesting
+int fn0() => 1;
+''');
+    Source source2 = addNamedSource('/lib2.dart', r'''
+import 'lib1.dart';
+
+void main() {
+  fn0();
+}
+''');
+    await computeAnalysisResult(source);
+    await computeAnalysisResult(source2);
+    assertErrors(source2, [HintCode.INVALID_USE_OF_VISIBLE_FOR_TESTING_MEMBER]);
+    verify([source, source2]);
+  }
+
   test_isDouble() async {
     AnalysisOptionsImpl options = new AnalysisOptionsImpl();
     options.dart2jsHint = true;