Analyzer: add two checks for the @internal annotation.

The meta package does not yet have an `internal` constant;
this CL just adds one to the mock packages for testing.

Two checks are implemented:

* Hint if an @internal annotation is found on an element which is
  already part of a package's public API (based on file path).
* Hint if an element annotated with @internal is exported from a
  package's public API.

The notion of "public API" is also implemented for each type of Package:
BasicPackage, BazelPackage, GnPackage, PackageBuildPackage,
and PubPackage.

Bug: #28066
Change-Id: Ifc2709028afcd241f59e802f5952539f717704c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163126
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: srawlins

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

@@ -534,6 +534,9 @@ abstract class Element implements AnalysisTarget {
   /// Return `true` if this element has an annotation of the form `@factory`.
   bool get hasFactory;
 
+  /// Return `true` if this element has an annotation of the form `@internal`.
+  bool get hasInternal;
+
   /// Return `true` if this element has an annotation of the form `@isTest`.
   bool get hasIsTest;
 
@@ -717,6 +720,10 @@ abstract class ElementAnnotation implements ConstantEvaluationTarget {
   /// subclasses as being immutable.
   bool get isImmutable;
 
+  /// Return `true` if this annotation marks the associated element as being
+  /// internal to its package.
+  bool get isInternal;
+
   /// Return `true` if this annotation marks the associated member as running
   /// a single test.
   bool get isIsTest;

pkg/analyzer/lib/error/error.dart

@@ -492,10 +492,12 @@ const List<ErrorCode> errorCodeValues = [
   HintCode.INFERENCE_FAILURE_ON_UNINITIALIZED_VARIABLE,
   HintCode.INFERENCE_FAILURE_ON_UNTYPED_PARAMETER,
   HintCode.INVALID_ANNOTATION_TARGET,
+  HintCode.INVALID_EXPORT_OF_INTERNAL_ELEMENT,
   HintCode.INVALID_FACTORY_ANNOTATION,
   HintCode.INVALID_FACTORY_METHOD_DECL,
   HintCode.INVALID_FACTORY_METHOD_IMPL,
   HintCode.INVALID_IMMUTABLE_ANNOTATION,
+  HintCode.INVALID_INTERNAL_ANNOTATION,
   HintCode.INVALID_LANGUAGE_VERSION_OVERRIDE_AT_SIGN,
   HintCode.INVALID_LANGUAGE_VERSION_OVERRIDE_EQUALS,
   HintCode.INVALID_LANGUAGE_VERSION_OVERRIDE_GREATER,

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

@@ -2313,6 +2313,10 @@ class ElementAnnotationImpl implements ElementAnnotation {
   /// as being immutable.
   static const String _IMMUTABLE_VARIABLE_NAME = "immutable";
 
+  /// The name of the top-level variable used to mark an element as being
+  /// internal to its package.
+  static const String _INTERNAL_VARIABLE_NAME = "internal";
+
   /// The name of the top-level variable used to mark a constructor as being
   /// literal.
   static const String _LITERAL_VARIABLE_NAME = "literal";
@@ -2459,6 +2463,12 @@ class ElementAnnotationImpl implements ElementAnnotation {
       element.name == _IMMUTABLE_VARIABLE_NAME &&
       element.library?.name == _META_LIB_NAME;
 
+  @override
+  bool get isInternal =>
+      element is PropertyAccessorElement &&
+      element.name == _INTERNAL_VARIABLE_NAME &&
+      element.library?.name == _META_LIB_NAME;
+
   @override
   bool get isIsTest =>
       element is PropertyAccessorElement &&
@@ -2747,6 +2757,18 @@ abstract class ElementImpl implements Element {
     return _cachedHashCode ??= location.hashCode;
   }
 
+  @override
+  bool get hasInternal {
+    var metadata = this.metadata;
+    for (var i = 0; i < metadata.length; i++) {
+      var annotation = metadata[i];
+      if (annotation.isInternal) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   @override
   bool get hasIsTest {
     var metadata = this.metadata;
@@ -5995,6 +6017,9 @@ class MultiplyDefinedElementImpl implements MultiplyDefinedElement {
   @override
   bool get hasFactory => false;
 
+  @override
+  bool get hasInternal => false;
+
   @override
   bool get hasIsTest => false;
 

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

@@ -459,6 +459,9 @@ abstract class Member implements Element {
   @override
   bool get hasFactory => _declaration.hasFactory;
 
+  @override
+  bool get hasInternal => _declaration.hasInternal;
+
   @override
   bool get hasIsTest => _declaration.hasIsTest;
 

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

@@ -576,6 +576,19 @@ class HintCode extends AnalyzerErrorCode {
       'INVALID_ANNOTATION_TARGET',
       "The annotation '{0}' can only be used on {1}");
 
+  /**
+   * This hint is generated anywhere where an element annotated with `@internal`
+   * is exported as a part of a package's public API.
+   *
+   * Parameters:
+   * 0: the name of the element
+   */
+  static const HintCode INVALID_EXPORT_OF_INTERNAL_ELEMENT = HintCode(
+      'INVALID_EXPORT_OF_INTERNAL_ELEMENT',
+      "The member '{0}' can't be exported as a part of a package's public "
+          "API.",
+      correction: "Try using a hide clause to hide '{0}'.");
+
   /**
    * This hint is generated anywhere a @factory annotation is associated with
    * anything other than a method.
@@ -612,6 +625,15 @@ class HintCode extends AnalyzerErrorCode {
       'INVALID_IMMUTABLE_ANNOTATION',
       "Only classes can be annotated as being immutable.");
 
+  /**
+   * This hint is generated anywhere a @internal annotation is associated with
+   * an element found in a package's public API.
+   */
+  static const HintCode INVALID_INTERNAL_ANNOTATION = HintCode(
+      'INVALID_INTERNAL_ANNOTATION',
+      "Only public elements in a package's private API can be annotated as "
+          "being internal.");
+
   /// Invalid Dart language version comments don't follow the specification [1].
   /// If a comment begins with "@dart" or "dart" (letters in any case),
   /// followed by optional whitespace, followed by optional non-alphanumeric,
@@ -922,7 +944,7 @@ class HintCode extends AnalyzerErrorCode {
 
   /**
    * This hint is generated anywhere where a member annotated with `@protected`
-   * is used outside an instance member of a subclass.
+   * is used outside of an instance member of a subclass.
    *
    * Parameters:
    * 0: the name of the member

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

@@ -21,6 +21,7 @@ import 'package:analyzer/src/dart/element/type.dart';
 import 'package:analyzer/src/dart/element/type_system.dart';
 import 'package:analyzer/src/dart/resolver/body_inference_context.dart';
 import 'package:analyzer/src/dart/resolver/exit_detector.dart';
+import 'package:analyzer/src/dart/resolver/scope.dart';
 import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/generated/constant.dart';
 import 'package:analyzer/src/generated/engine.dart';
@@ -113,6 +114,11 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
     );
   }
 
+  bool get _inPublicPackageApi {
+    return _workspacePackage != null &&
+        _workspacePackage.sourceIsInPublicApi(_currentLibrary.source);
+  }
+
   @override
   void visitAnnotation(Annotation node) {
     ElementAnnotation element = node.elementAnnotation;
@@ -132,6 +138,35 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
         _errorReporter.reportErrorForNode(
             HintCode.INVALID_IMMUTABLE_ANNOTATION, node, []);
       }
+    } else if (element.isInternal) {
+      var parentElement = parent is Declaration ? parent.declaredElement : null;
+      if (parent is TopLevelVariableDeclaration) {
+        for (VariableDeclaration variable in parent.variables.variables) {
+          if (Identifier.isPrivateName(variable.declaredElement.name)) {
+            _errorReporter.reportErrorForNode(
+                HintCode.INVALID_INTERNAL_ANNOTATION, variable, []);
+          }
+        }
+      } else if (parent is FieldDeclaration) {
+        for (VariableDeclaration variable in parent.fields.variables) {
+          if (Identifier.isPrivateName(variable.declaredElement.name)) {
+            _errorReporter.reportErrorForNode(
+                HintCode.INVALID_INTERNAL_ANNOTATION, variable, []);
+          }
+        }
+      } else if (parent is ConstructorDeclaration) {
+        var class_ = parent.declaredElement.enclosingElement;
+        if (class_.isPrivate || (parentElement?.isPrivate ?? false)) {
+          _errorReporter.reportErrorForNode(
+              HintCode.INVALID_INTERNAL_ANNOTATION, node, []);
+        }
+      } else if (parentElement?.isPrivate ?? false) {
+        _errorReporter
+            .reportErrorForNode(HintCode.INVALID_INTERNAL_ANNOTATION, node, []);
+      } else if (_inPublicPackageApi) {
+        _errorReporter
+            .reportErrorForNode(HintCode.INVALID_INTERNAL_ANNOTATION, node, []);
+      }
     } else if (element.isLiteral == true) {
       if (parent is! ConstructorDeclaration ||
           (parent as ConstructorDeclaration).constKeyword == null) {
@@ -319,6 +354,7 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
   @override
   void visitExportDirective(ExportDirective node) {
     _checkForDeprecatedMemberUse(node.uriElement, node);
+    _checkForInternalExport(node);
     super.visitExportDirective(node);
   }
 
@@ -994,6 +1030,31 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
     }
   }
 
+  /// Check that the namespace exported by [node] does not include any elements
+  /// annotated with `@internal`.
+  void _checkForInternalExport(ExportDirective node) {
+    if (!_inPublicPackageApi) return;
+
+    var libraryElement = node.uriElement;
+    if (libraryElement == null) return;
+    if (libraryElement.hasInternal) {
+      _errorReporter.reportErrorForNode(
+          HintCode.INVALID_EXPORT_OF_INTERNAL_ELEMENT,
+          node,
+          [libraryElement.displayName]);
+    }
+    var exportNamespace =
+        NamespaceBuilder().createExportNamespaceForDirective(node.element);
+    exportNamespace.definedNames.forEach((String name, Element element) {
+      if (element.hasInternal) {
+        _errorReporter.reportErrorForNode(
+            HintCode.INVALID_EXPORT_OF_INTERNAL_ELEMENT,
+            node,
+            [element.displayName]);
+      }
+    });
+  }
+
   void _checkForInvalidFactory(MethodDeclaration decl) {
     // Check declaration.
     // Note that null return types are expected to be flagged by other analyses.

pkg/analyzer/lib/src/test_utilities/mock_packages.dart

@@ -28,6 +28,7 @@ const _AlwaysThrows alwaysThrows = const _AlwaysThrows();
 const _DoNotStore doNotStore = _DoNotStore();
 const _Factory factory = const _Factory();
 const Immutable immutable = const Immutable();
+const _Internal internal = const Internal();
 const _Literal literal = const _Literal();
 const _MustCallSuper mustCallSuper = const _MustCallSuper();
 const _NonVirtual nonVirtual = const _NonVirtual();
@@ -50,6 +51,9 @@ class Immutable {
   final String reason;
   const Immutable([this.reason]);
 }
+class _Internal {
+  const Internal();
+}
 class _Literal {
   const _Literal();
 }

pkg/analyzer/lib/src/workspace/basic.dart

@@ -80,4 +80,12 @@ class BasicWorkspacePackage extends WorkspacePackage {
   @override
   Map<String, List<Folder>> packagesAvailableTo(String libraryPath) =>
       workspace.packageMap;
+
+  @override
+  bool sourceIsInPublicApi(Source source) {
+    // Since every source file in a BasicPackage is in the same directory, they
+    // are all in the public API of the package. A file in a subdirectory
+    // is in a separate package.
+    return true;
+  }
 }

pkg/analyzer/lib/src/workspace/bazel.dart

@@ -260,6 +260,19 @@ class BazelWorkspace extends Workspace
   WorkspacePackage findPackageFor(String filePath) {
     path.Context context = provider.pathContext;
     Folder folder = provider.getFolder(context.dirname(filePath));
+    if (!context.isWithin(root, folder.path)) {
+      return null;
+    }
+
+    // Handle files which are given with their location in "bazel-bin", etc.
+    // This does not typically happen during usual analysis, but it still could,
+    // and it can come up in tests.
+    if ([genfiles, ...binPaths]
+        .any((binPath) => context.isWithin(binPath, folder.path))) {
+      var relative = context.relative(filePath, from: root);
+      return findPackageFor(
+          context.joinAll([root, ...context.split(relative).skip(1)]));
+    }
 
     while (true) {
       Folder parent = folder.parent;
@@ -300,7 +313,8 @@ class BazelWorkspace extends Workspace
       // [folder]'s path, relative to [root]. For example, "foo/bar".
       String relative = context.relative(folder.path, from: root);
       for (String bin in binPaths) {
-        Folder binChild = provider.getFolder(context.join(bin, relative));
+        Folder binChild =
+            provider.getFolder(context.normalize(context.join(bin, relative)));
         if (binChild.exists &&
             binChild.getChildren().any((c) => c.path.endsWith('.packages'))) {
           // [folder]'s sister folder within [bin] contains a ".packages" file.
@@ -548,4 +562,41 @@ class BazelWorkspacePackage extends WorkspacePackage {
   //  lists.
   Map<String, List<Folder>> packagesAvailableTo(String libraryPath) =>
       <String, List<Folder>>{};
+
+  @override
+  bool sourceIsInPublicApi(Source source) {
+    var filePath = filePathFromSource(source);
+    if (filePath == null) return false;
+
+    var libFolder = workspace.provider.pathContext.join(root, 'lib');
+    if (workspace.provider.pathContext.isWithin(libFolder, filePath)) {
+      // A file in "$root/lib" is public iff it is not in "$root/lib/src".
+      var libSrcFolder = workspace.provider.pathContext.join(libFolder, 'src');
+      return !workspace.provider.pathContext.isWithin(libSrcFolder, filePath);
+    }
+
+    var relativeRoot =
+        workspace.provider.pathContext.relative(root, from: workspace.root);
+    for (var binPath in workspace.binPaths) {
+      libFolder =
+          workspace.provider.pathContext.join(binPath, relativeRoot, 'lib');
+      if (workspace.provider.pathContext.isWithin(libFolder, filePath)) {
+        // A file in "$bin/lib" is public iff it is not in "$bin/lib/src".
+        var libSrcFolder =
+            workspace.provider.pathContext.join(libFolder, 'src');
+        return !workspace.provider.pathContext.isWithin(libSrcFolder, filePath);
+      }
+    }
+
+    libFolder = workspace.provider.pathContext
+        .join(workspace.genfiles, relativeRoot, 'lib');
+    if (workspace.provider.pathContext.isWithin(libFolder, filePath)) {
+      // A file in "$genfiles/lib" is public iff it is not in
+      // "$genfiles/lib/src".
+      var libSrcFolder = workspace.provider.pathContext.join(libFolder, 'src');
+      return !workspace.provider.pathContext.isWithin(libSrcFolder, filePath);
+    }
+
+    return false;
+  }
 }

pkg/analyzer/lib/src/workspace/gn.dart

@@ -237,4 +237,17 @@ class GnWorkspacePackage extends WorkspacePackage {
   @override
   Map<String, List<Folder>> packagesAvailableTo(String libraryPath) =>
       workspace.packageMap;
+
+  @override
+  bool sourceIsInPublicApi(Source source) {
+    var filePath = filePathFromSource(source);
+    if (filePath == null) return false;
+    var libFolder = workspace.provider.pathContext.join(root, 'lib');
+    if (workspace.provider.pathContext.isWithin(libFolder, filePath)) {
+      var libSrcFolder =
+          workspace.provider.pathContext.join(root, 'lib', 'src');
+      return !workspace.provider.pathContext.isWithin(libSrcFolder, filePath);
+    }
+    return false;
+  }
 }