Completion. Issue 35449. Don't suggest @VisibleForTesting instance members when not available.

Bug: #35449
Change-Id: I7e02d7fbbc9477ad98dd87327adf3b562fc0fde8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358328
Commit-Queue: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: scheglov

pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart

@@ -18,7 +18,6 @@ import 'package:analysis_server/src/services/completion/dart/record_literal_cont
 import 'package:analysis_server/src/services/completion/dart/suggestion_builder.dart';
 import 'package:analysis_server/src/services/completion/dart/suggestion_collector.dart';
 import 'package:analysis_server/src/services/completion/dart/uri_contributor.dart';
-import 'package:analysis_server/src/utilities/extensions/object.dart';
 import 'package:analysis_server/src/utilities/selection.dart';
 import 'package:analyzer/dart/analysis/features.dart';
 import 'package:analyzer/dart/analysis/results.dart';
@@ -39,7 +38,6 @@ import 'package:analyzer/src/dartdoc/dartdoc_directive_info.dart';
 import 'package:analyzer/src/generated/source.dart' show SourceFactory;
 import 'package:analyzer/src/util/file_paths.dart' as file_paths;
 import 'package:analyzer/src/util/performance/operation_performance.dart';
-import 'package:analyzer/src/workspace/pub.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart' as protocol;
 import 'package:analyzer_plugin/src/utilities/completion/completion_target.dart';
 import 'package:analyzer_plugin/src/utilities/completion/optype.dart';
@@ -412,12 +410,6 @@ class DartCompletionRequest {
   /// Answer the [DartType] for Object in dart:core
   InterfaceType get objectType => libraryElement.typeProvider.objectType;
 
-  /// Returns the [PubPackage] of the file where completion is requested.
-  /// Or `null` if the package is not [PubPackage].
-  PubPackage? get pubPackage {
-    return fileState.workspacePackage.ifTypeOrNull();
-  }
-
   /// The length of the text to be replaced if the remainder of the identifier
   /// containing the cursor is to be replaced when the suggestion is applied
   /// (that is, the number of characters in the existing identifier).

pkg/analysis_server/lib/src/services/completion/dart/declaration_helper.dart

@@ -17,6 +17,7 @@ import 'package:analyzer/src/dart/element/member.dart';
 import 'package:analyzer/src/dart/element/type_algebra.dart';
 import 'package:analyzer/src/dart/resolver/scope.dart';
 import 'package:analyzer/src/utilities/extensions/element.dart';
+import 'package:analyzer/src/workspace/pub.dart';
 
 /// A helper class that produces candidate suggestions for all of the
 /// declarations that are in scope at the completion location.
@@ -1031,10 +1032,11 @@ class DeclarationHelper {
     }
 
     if (element.isInternal) {
-      if (request.pubPackage case var pubPackage?) {
-        if (!pubPackage.contains(element.librarySource)) {
-          return false;
-        }
+      switch (request.fileState.workspacePackage) {
+        case PubPackage pubPackage:
+          if (!pubPackage.contains(element.librarySource)) {
+            return false;
+          }
       }
     }
 
@@ -1057,6 +1059,23 @@ class DeclarationHelper {
       }
     }
 
+    if (element.isVisibleForTesting) {
+      if (element.library != requestLibrary) {
+        var fileState = request.fileState;
+        switch (fileState.workspacePackage) {
+          case PubPackage pubPackage:
+            // Must be in the same package.
+            if (!pubPackage.contains(element.librarySource)) {
+              return false;
+            }
+            // Must be in the `test` directory.
+            if (!pubPackage.isInTestDirectory(fileState.resource)) {
+              return false;
+            }
+        }
+      }
+    }
+
     return true;
   }
 

pkg/analysis_server/test/analysis_server_base.dart

@@ -207,6 +207,9 @@ abstract class ContextResolutionTest with ResourceProviderMixin {
 
 class PubPackageAnalysisServerTest extends ContextResolutionTest
     with MockPackagesMixin, ConfigurationFilesMixin, TestMacros {
+  // TODO(scheglov): Consider turning it back into a getter.
+  late String testFilePath = '$testPackageLibPath/test.dart';
+
   // If experiments are needed,
   // add `import 'package:analyzer/dart/analysis/features.dart';`
   // and list the necessary experiments here.
@@ -227,8 +230,6 @@ class PubPackageAnalysisServerTest extends ContextResolutionTest
 
   String get testFileContent => testFile.readAsStringSync();
 
-  String get testFilePath => '$testPackageLibPath/test.dart';
-
   String get testPackageLibPath => '$testPackageRootPath/lib';
 
   Folder get testPackageRoot => getFolder(testPackageRootPath);

pkg/analysis_server/test/services/completion/dart/location/property_access_expression_test.dart

@@ -550,6 +550,177 @@ suggestions
     kind: setter
   f02
     kind: setter
+''');
+  }
+
+  Future<void> test_isVisibleForTesting_method_otherPackage() async {
+    var otherRoot = getFolder('$packagesRootPath/other');
+    newFile('${otherRoot.path}/lib/a.dart', r'''
+import 'package:meta/meta.dart';
+
+class A {
+  void f01() {}
+
+  @visibleForTesting
+  void f02() {}
+}
+''');
+
+    writeTestPackageConfig(
+      config: PackageConfigFileBuilder()
+        ..add(
+          name: 'other',
+          rootPath: otherRoot.path,
+        ),
+      meta: true,
+    );
+
+    await computeSuggestions('''
+import 'package:other/a.dart''
+
+void f() {
+  A().^
+}
+''');
+
+    assertResponse(r'''
+suggestions
+  f01
+    kind: methodInvocation
+''');
+  }
+
+  Future<void> test_isVisibleForTesting_method_otherPackage_test() async {
+    var otherRoot = getFolder('$packagesRootPath/other');
+    newFile('${otherRoot.path}/lib/a.dart', r'''
+import 'package:meta/meta.dart';
+
+class A {
+  void f01() {}
+
+  @visibleForTesting
+  void f02() {}
+}
+''');
+
+    writeTestPackageConfig(
+      config: PackageConfigFileBuilder()
+        ..add(
+          name: 'other',
+          rootPath: otherRoot.path,
+        ),
+      meta: true,
+    );
+
+    testFilePath = '$testPackageTestPath/test.dart';
+    await computeSuggestions('''
+import 'package:other/a.dart''
+
+void f() {
+  A().^
+}
+''');
+
+    assertResponse(r'''
+suggestions
+  f01
+    kind: methodInvocation
+''');
+  }
+
+  Future<void> test_isVisibleForTesting_method_sameLibrary() async {
+    writeTestPackageConfig(
+      meta: true,
+    );
+
+    await computeSuggestions('''
+import 'package:meta/meta.dart';
+
+class A {
+  void f01() {}
+
+  @visibleForTesting
+  void f02() {}
+}
+
+void f(A a) {
+  a.^
+}
+''');
+
+    assertResponse(r'''
+suggestions
+  f01
+    kind: methodInvocation
+  f02
+    kind: methodInvocation
+''');
+  }
+
+  Future<void>
+      test_isVisibleForTesting_method_samePackage_otherLibrary() async {
+    writeTestPackageConfig(
+      meta: true,
+    );
+
+    newFile('$testPackageLibPath/a.dart', r'''
+import 'package:meta/meta.dart';
+
+class A {
+  void f01() {}
+
+  @visibleForTesting
+  void f02() {}
+}
+''');
+
+    await computeSuggestions('''
+import 'a.dart';
+
+void f(A a) {
+  a.^
+}
+''');
+
+    assertResponse(r'''
+suggestions
+  f01
+    kind: methodInvocation
+''');
+  }
+
+  Future<void>
+      test_isVisibleForTesting_method_samePackage_otherLibrary_test() async {
+    writeTestPackageConfig(
+      meta: true,
+    );
+
+    newFile('$testPackageLibPath/a.dart', r'''
+import 'package:meta/meta.dart';
+
+class A {
+  void f01() {}
+
+  @visibleForTesting
+  void f02() {}
+}
+''');
+
+    testFilePath = '$testPackageTestPath/test.dart';
+    await computeSuggestions('''
+import 'package:test/a.dart';
+
+void f(A a) {
+  a.^
+}
+''');
+
+    assertResponse(r'''
+suggestions
+  f01
+    kind: methodInvocation
+  f02
+    kind: methodInvocation
 ''');
   }
 }

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

@@ -1800,7 +1800,7 @@ class _InvalidAccessVerifier {
       }
     }
 
-    bool hasVisibleForTesting = _hasVisibleForTesting(element);
+    var hasVisibleForTesting = element.isVisibleForTesting;
     if (hasVisibleForTesting) {
       if (_inTestDirectory || _inExportDirective(node)) {
         return;
@@ -1935,17 +1935,6 @@ class _InvalidAccessVerifier {
     return false;
   }
 
-  bool _hasVisibleForTesting(Element element) {
-    if (element.hasVisibleForTesting) {
-      return true;
-    }
-    if (element is PropertyAccessorElement) {
-      var variable = element.variable2;
-      return variable != null && variable.hasVisibleForTesting;
-    }
-    return false;
-  }
-
   bool _hasVisibleOutsideTemplate(Element element) {
     if (element.hasVisibleOutsideTemplate) {
       return true;

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

@@ -7,6 +7,7 @@ import 'package:analyzer/dart/element/nullability_suffix.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/ast/ast.dart';
 import 'package:analyzer/src/dart/element/element.dart';
+import 'package:meta/meta.dart';
 
 extension ElementExtension on Element {
   // TODO(scheglov): Maybe just add to `Element`?
@@ -17,7 +18,7 @@ extension ElementExtension on Element {
     return null;
   }
 
-  /// Whether the element is effectively internal.
+  /// Whether the element is effectively [internal].
   bool get isInternal {
     if (hasInternal) {
       return true;
@@ -31,7 +32,7 @@ extension ElementExtension on Element {
     return false;
   }
 
-  /// Whether the element is effectively protected.
+  /// Whether the element is effectively [protected].
   bool get isProtected {
     final self = this;
     if (self is PropertyAccessorElement &&
@@ -52,6 +53,20 @@ extension ElementExtension on Element {
     return false;
   }
 
+  /// Whether the element is effectively [visibleForTesting].
+  bool get isVisibleForTesting {
+    if (hasVisibleForTesting) {
+      return true;
+    }
+    if (this case PropertyAccessorElement accessor) {
+      var variable = accessor.variable2;
+      if (variable != null && variable.hasVisibleForTesting) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   List<Element> get withAugmentations {
     final result = <Element>[];
     Element? current = this;

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

@@ -456,6 +456,13 @@ class PubPackage extends WorkspacePackage {
     return false;
   }
 
+  /// Whether [file] is in the `test` directory of this package.
+  bool isInTestDirectory(File file) {
+    var resourceProvider = workspace.provider;
+    var packageRoot = resourceProvider.getFolder(root);
+    return packageRoot.getChildAssumingFolder('test').contains(file.path);
+  }
+
   @override
   Packages packagesAvailableTo(String libraryPath) {
     // TODO(brianwilkerson): Consider differentiating based on whether the

pkg/analyzer/test/src/workspace/pub_test.dart

@@ -724,6 +724,24 @@ class PubPackageTest extends WorkspacePackageTest {
     expect(package, isNull);
   }
 
+  test_isInTestDirectory() {
+    var pubPackage = myPackage as PubPackage;
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageRootPath/lib/a.dart'),
+      ),
+      isFalse,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageRootPath/test/a.dart'),
+      ),
+      isTrue,
+    );
+  }
+
   void test_packagesAvailableTo() {
     var libraryPath = convertPath('/workspace/lib/test.dart');
     var package = findPackage(libraryPath)!;