fix public_member_api_docs to not overreport elements marked @internal

(#4536)
dart-lang · Jul 5, 2023 · 45632a8 · 45632a8
1 parent 02d83f0
commit 45632a8
Show file tree

Hide file tree

Showing 4 changed files with 142 additions and 49 deletions.
diff --git a/lib/src/extensions.dart b/lib/src/extensions.dart
@@ -21,34 +21,32 @@ class EnumLikeClassDescription {
   Map<DartObject, Set<FieldElement>> get enumConstants => {..._enumConstants};
 }
 
-extension AstNodeNullableExtension on AstNode? {
-  bool get isFieldNameShortcut {
-    var node = this;
-    if (node is NullCheckPattern) node = node.parent;
-    if (node is NullAssertPattern) node = node.parent;
-    return node is PatternField && node.name != null && node.name?.name == null;
-  }
+extension AstNodeExtension on AstNode {
+  Iterable<AstNode> get childNodes => childEntities.whereType<AstNode>();
 
-  /// Return `true` if the expression is null aware, or if one of its recursive
-  /// targets is null aware.
-  bool containsNullAwareInvocationInChain() {
+  bool get isEffectivelyPrivate {
     var node = this;
-    if (node is PropertyAccess) {
-      if (node.isNullAware) return true;
-      return node.target.containsNullAwareInvocationInChain();
-    } else if (node is MethodInvocation) {
-      if (node.isNullAware) return true;
-      return node.target.containsNullAwareInvocationInChain();
-    } else if (node is IndexExpression) {
-      if (node.isNullAware) return true;
-      return node.target.containsNullAwareInvocationInChain();
+    if (node.isInternal) return true;
+    if (node is ClassDeclaration) {
+      var classElement = node.declaredElement;
+      if (classElement != null) {
+        if (classElement.isSealed) return true;
+        if (classElement.isAbstract) {
+          if (classElement.isFinal) return true;
+          if (classElement.isInterface) return true;
+        }
+      }
     }
     return false;
   }
-}
 
-extension AstNodeExtension on AstNode {
-  Iterable<AstNode> get childNodes => childEntities.whereType<AstNode>();
+  bool get isInternal {
+    var parent = thisOrAncestorOfType<CompilationUnitMember>();
+    if (parent == null) return false;
+
+    var element = parent.declaredElement;
+    return element != null && element.hasInternal;
+  }
 
   /// Builds the list resulting from traversing the node in DFS and does not
   /// include the node itself.
@@ -79,6 +77,32 @@ extension AstNodeExtension on AstNode {
   }
 }
 
+extension AstNodeNullableExtension on AstNode? {
+  bool get isFieldNameShortcut {
+    var node = this;
+    if (node is NullCheckPattern) node = node.parent;
+    if (node is NullAssertPattern) node = node.parent;
+    return node is PatternField && node.name != null && node.name?.name == null;
+  }
+
+  /// Return `true` if the expression is null aware, or if one of its recursive
+  /// targets is null aware.
+  bool containsNullAwareInvocationInChain() {
+    var node = this;
+    if (node is PropertyAccess) {
+      if (node.isNullAware) return true;
+      return node.target.containsNullAwareInvocationInChain();
+    } else if (node is MethodInvocation) {
+      if (node.isNullAware) return true;
+      return node.target.containsNullAwareInvocationInChain();
+    } else if (node is IndexExpression) {
+      if (node.isNullAware) return true;
+      return node.target.containsNullAwareInvocationInChain();
+    }
+    return false;
+  }
+}
+
 extension BlockExtension on Block {
   /// Returns the last statement of this block, or `null` if this is empty.
   ///

diff --git a/lib/src/rules/library_private_types_in_public_api.dart b/lib/src/rules/library_private_types_in_public_api.dart
@@ -8,6 +8,7 @@ import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 
 import '../analyzer.dart';
+import '../extensions.dart';
 
 const _desc = r'Avoid using private types in public APIs.';
 
@@ -95,18 +96,13 @@ class Validator extends SimpleAstVisitor<void> {
       return;
     }
 
+    var parent = node.parent;
+
     // Enum constructors are effectively private so don't visit their params.
-    if (node.parent is EnumDeclaration) return;
+    if (parent is EnumDeclaration) return;
 
     // Select modified class types are also effectively private.
-    var enclosingElement = node.declaredElement?.enclosingElement;
-    if (enclosingElement is ClassElement) {
-      if (enclosingElement.isSealed) return;
-      if (enclosingElement.isAbstract) {
-        if (enclosingElement.isInterface) return;
-        if (enclosingElement.isFinal) return;
-      }
-    }
+    if (parent != null && parent.isEffectivelyPrivate) return;
 
     node.parameters.accept(this);
   }

diff --git a/lib/src/rules/public_member_api_docs.dart b/lib/src/rules/public_member_api_docs.dart
@@ -166,6 +166,8 @@ class _Visitor extends SimpleAstVisitor {
 
   @override
   void visitClassDeclaration(ClassDeclaration node) {
+    var element = node.declaredElement;
+    if (element == null || element.hasInternal) return;
     _visitMembers(node, node.name, node.members);
   }
 
@@ -229,22 +231,16 @@ class _Visitor extends SimpleAstVisitor {
     if (inPrivateMember(node) || isPrivate(node.name)) return;
     var parent = node.parent;
     if (parent is EnumDeclaration) return;
-    if (parent is ClassDeclaration) {
-      var classElement = parent.declaredElement;
-      if (classElement != null) {
-        if (classElement.isSealed) return;
-        if (classElement.isAbstract) {
-          if (classElement.isFinal) return;
-          if (classElement.isInterface) return;
-        }
-      }
-    }
+    if (parent != null && parent.isEffectivelyPrivate) return;
 
     check(node);
   }
 
   @override
   void visitEnumConstantDeclaration(EnumConstantDeclaration node) {
+    // todo(pq): update this to be called from the parent (like with visitMembers)
+    if (node.isInternal) return;
+
     if (!inPrivateMember(node) && !isPrivate(node.name)) {
       check(node);
     }
@@ -253,28 +249,30 @@ class _Visitor extends SimpleAstVisitor {
   @override
   void visitEnumDeclaration(EnumDeclaration node) {
     if (isPrivate(node.name)) return;
+    if (node.isInternal) return;
 
     check(node);
     checkMethods(node.members);
   }
 
   @override
   void visitExtensionDeclaration(ExtensionDeclaration node) {
-    if (node.name == null || isPrivate(node.name)) {
-      return;
-    }
+    if (node.name == null || isPrivate(node.name)) return;
+    if (node.isInternal) return;
 
     check(node);
     checkMethods(node.members);
   }
 
   @override
   void visitFieldDeclaration(FieldDeclaration node) {
-    if (!inPrivateMember(node)) {
-      for (var field in node.fields.variables) {
-        if (!isPrivate(field.name)) {
-          check(field);
-        }
+    // todo(pq): update this to be called from the parent (like with visitMembers)
+    if (node.isInternal) return;
+    if (inPrivateMember(node)) return;
+
+    for (var field in node.fields.variables) {
+      if (!isPrivate(field.name)) {
+        check(field);
       }
     }
   }
@@ -295,6 +293,7 @@ class _Visitor extends SimpleAstVisitor {
 
   @override
   void visitMixinDeclaration(MixinDeclaration node) {
+    if (node.isInternal) return;
     _visitMembers(node, node.name, node.members);
   }
 

diff --git a/test/rules/public_member_api_docs_test.dart b/test/rules/public_member_api_docs_test.dart
@@ -14,6 +14,9 @@ main() {
 
 @reflectiveTest
 class PublicMemberApiDocsTest extends LintRuleTest {
+  @override
+  bool get addMetaPackageDep => true;
+
   @override
   String get lintRule => 'public_member_api_docs';
 
@@ -105,6 +108,77 @@ extension E on Object {
     ]);
   }
 
+  /// https://github.com/dart-lang/linter/issues/4521
+  test_internalClass() async {
+    await assertDiagnostics(r'''
+import 'package:meta/meta.dart';
+
+@internal
+class C {
+  int? f;
+  int get i => 0;
+  C();
+  void m() {}
+}
+''', [
+      // Technically not in the private API but we can ignore that for testing.
+      error(WarningCode.INVALID_INTERNAL_ANNOTATION, 34, 9),
+    ]);
+  }
+
+  /// https://github.com/dart-lang/linter/issues/4521
+  test_internalEnum() async {
+    await assertDiagnostics(r'''
+import 'package:meta/meta.dart';
+
+@internal
+enum E {
+  a, b, c;
+  int get i => 0;
+  void m() {}
+  
+  const E();
+}
+''', [
+      // Technically not in the private API but we can ignore that for testing.
+      error(WarningCode.INVALID_INTERNAL_ANNOTATION, 34, 9),
+    ]);
+  }
+
+  /// https://github.com/dart-lang/linter/issues/4521
+  test_internalExtension() async {
+    await assertDiagnostics(r'''
+import 'package:meta/meta.dart';
+
+@internal
+extension X on Object {
+  static int? f;
+  static int get i => 0;
+  void e() {}
+}
+''', [
+      // Technically not in the private API but we can ignore that for testing.
+      error(WarningCode.INVALID_INTERNAL_ANNOTATION, 34, 9),
+    ]);
+  }
+
+  /// https://github.com/dart-lang/linter/issues/4521
+  test_internalMixin() async {
+    await assertDiagnostics(r'''
+import 'package:meta/meta.dart';
+
+@internal
+mixin M {
+  int? f;
+  int get i => 0;
+  void m() {}
+}
+''', [
+      // Technically not in the private API but we can ignore that for testing.
+      error(WarningCode.INVALID_INTERNAL_ANNOTATION, 34, 9),
+    ]);
+  }
+
   test_mixin_method() async {
     await assertDiagnostics(r'''
 /// A mixin M.