Issue 63382. Use WorkspacePackage.isInTestDirectory() instead of CompilationUnitExtension.

Bug: https://github.com/dart-lang/sdk/issues/63382
Change-Id: I3c1d2495f9cb6804c73d1f36314556186f825a67
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/505064
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>

pkg/analysis_server/lib/src/services/snippets/dart_snippet_request.dart

@@ -37,6 +37,9 @@ class DartSnippetRequest {
   /// The path of the file snippets are being requested for.
   final String filePath;
 
+  /// Whether [file] is in a "test" directory of its workspace package.
+  final bool isInTestDirectory;
+
   /// The offset within the source at which snippets are being
   /// requested for.
   final int offset;
@@ -55,7 +58,9 @@ class DartSnippetRequest {
       content = unit.content,
       compilationUnit = unit.unit,
       libraryElement = unit.libraryElement,
-      filePath = unit.path {
+      filePath = unit.path,
+      isInTestDirectory =
+          unit is ResolvedUnitResultImpl && unit.fileState.isInTestDirectory {
     var target = CompletionTarget.forOffset(unit.unit, offset);
     context = _getContext(target);
     replacementRange = target.computeReplacementRange(
@@ -75,7 +80,8 @@ class DartSnippetRequest {
        content = unit.content,
        compilationUnit = unit.parsedUnit,
        libraryElement = unit.libraryFragment.element,
-       filePath = unit.path {
+       filePath = unit.path,
+       isInTestDirectory = unit.fileState.isInTestDirectory {
     var target = CompletionTarget.forOffset(unit.parsedUnit, offset);
     context = _getContext(target);
     replacementRange = target.computeReplacementRange(

pkg/analysis_server/lib/src/services/snippets/snippet_producer.dart

@@ -11,7 +11,6 @@ import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/nullability_suffix.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/analysis/session_helper.dart';
-import 'package:analyzer/src/utilities/extensions/ast.dart';
 import 'package:analyzer/src/utilities/extensions/flutter.dart';
 import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_dart.dart'
     show DartFileEditBuilderImpl;
@@ -43,7 +42,7 @@ abstract class DartSnippetProducer extends SnippetProducer {
       .getAnalysisOptionsForFile(request.file)
       .codeStyleOptions;
 
-  bool get isInTestDirectory => request.compilationUnit.inTestDir;
+  bool get isInTestDirectory => request.isInTestDirectory;
 }
 
 abstract class FlutterSnippetProducer extends DartSnippetProducer {

pkg/analysis_server/test/services/snippets/dart/main_function_test.dart

@@ -25,7 +25,7 @@ class MainFunctionTest extends DartSnippetProducerTest {
   String get prefix => MainFunction.prefix;
 
   Future<void> test_noParams_testFolder() => testInFile(
-    convertPath('$testPackageLibPath/test/foo_test.dart'),
+    convertPath('$testPackageTestPath/foo_test.dart'),
     expectArgsParameter: false,
   );
 

pkg/analysis_server/test/services/snippets/dart/test_definition_test.dart

@@ -26,7 +26,7 @@ class TestDefinitionTest extends DartSnippetProducerTest {
   String get prefix => TestDefinition.prefix;
 
   Future<void> test_import_dart() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   test^
@@ -45,7 +45,7 @@ void f() {
   }
 
   Future<void> test_import_dart_existing() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:test/test.dart';
 
@@ -66,7 +66,7 @@ void f() {
   }
 
   Future<void> test_inTestFile() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   test^
@@ -109,7 +109,7 @@ class TestWithFlutterDefinitionTest extends DartSnippetProducerTest {
   String get prefix => TestDefinition.prefix;
 
   Future<void> test_import_flutter() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   test^
@@ -128,7 +128,7 @@ void f() {
   }
 
   Future<void> test_import_flutter_existing() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:flutter_test/flutter_test.dart';
 
@@ -151,7 +151,7 @@ void f() {
   /// Ensure we don't import package:flutter_test if package:test is already
   /// imported.
   Future<void> test_import_flutter_existingDart() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:test/test.dart';
 

pkg/analysis_server/test/services/snippets/dart/test_group_definition_test.dart

@@ -26,7 +26,7 @@ class TestGroupDefinitionTest extends DartSnippetProducerTest {
   String get prefix => TestGroupDefinition.prefix;
 
   Future<void> test_import_dart() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   group^
@@ -45,7 +45,7 @@ void f() {
   }
 
   Future<void> test_import_dart_existing() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:test/test.dart';
 
@@ -66,7 +66,7 @@ void f() {
   }
 
   Future<void> test_inTestFile() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   group^
@@ -109,7 +109,7 @@ class TestGroupWithFlutterDefinitionTest extends DartSnippetProducerTest {
   String get prefix => TestGroupDefinition.prefix;
 
   Future<void> test_import_flutter() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 void f() {
   group^
@@ -128,7 +128,7 @@ void f() {
   }
 
   Future<void> test_import_flutter_existing() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:flutter_test/flutter_test.dart';
 
@@ -151,7 +151,7 @@ void f() {
   /// Ensure we don't import package:flutter_test if package:test is already
   /// imported.
   Future<void> test_import_flutter_existingDart() async {
-    testFilePath = convertPath('$testPackageLibPath/test/foo_test.dart');
+    testFilePath = convertPath('$testPackageTestPath/foo_test.dart');
     var code = r'''
 import 'package:test/test.dart';
 

pkg/analyzer/lib/src/dart/analysis/file_state.dart

@@ -561,6 +561,11 @@ class FileState {
   @override
   int get hashCode => uri.hashCode;
 
+  /// Whether this file is in a "test" directory of its workspace package.
+  bool get isInTestDirectory {
+    return workspacePackage?.isInTestDirectory(resource) ?? false;
+  }
+
   FileKind get kind => _kind!;
 
   /// Return information about line in the file.

pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart

@@ -520,10 +520,10 @@ class LibraryAnalyzer {
         diagnosticReporter,
         _typeProvider,
         _libraryElement,
-        unit,
         typeSystem: _typeSystem,
         analysisOptions: _analysisOptions,
         workspacePackage: _library.file.workspacePackage,
+        inTestDirectory: fileAnalysis.file.isInTestDirectory,
       ),
     );
 

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

@@ -101,11 +101,11 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
   BestPracticesVerifier(
     this._diagnosticReporter,
     TypeProviderImpl typeProvider,
-    this._currentLibrary,
-    CompilationUnit unit, {
+    this._currentLibrary, {
     required TypeSystemImpl typeSystem,
     required AnalysisOptions analysisOptions,
     required WorkspacePackageImpl? workspacePackage,
+    required bool inTestDirectory,
   }) : _nullType = typeProvider.nullType,
        _typeSystem = typeSystem,
        _strictInference = analysisOptions.strictInference,
@@ -149,9 +149,9 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
        ),
        _invalidAccessVerifier = _InvalidAccessVerifier(
          _diagnosticReporter,
-         unit,
          _currentLibrary,
          workspacePackage,
+         inTestDirectory: inTestDirectory,
        ),
        _mustCallSuperVerifier = MustCallSuperVerifier(_diagnosticReporter),
        _nullSafeApiVerifier = NullSafeApiVerifier(
@@ -1602,13 +1602,13 @@ class _InvalidAccessVerifier {
 
   _InvalidAccessVerifier(
     this._diagnosticReporter,
-    CompilationUnit unit,
     this._library,
-    this._workspacePackage,
-  ) : _inTemplateSource = _library.firstFragment.source.fullName.contains(
-        _templateExtension,
-      ),
-      _inTestDirectory = unit.inTestDir;
+    this._workspacePackage, {
+    required bool inTestDirectory,
+  }) : _inTemplateSource = _library.firstFragment.source.fullName.contains(
+         _templateExtension,
+       ),
+       _inTestDirectory = inTestDirectory;
 
   /// Produces a warning if [identifier] is accessed from an invalid location.
   ///

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

@@ -135,32 +135,6 @@ extension AstNodeNullableExtension on AstNode? {
   }
 }
 
-extension CompilationUnitExtension on CompilationUnit {
-  /// Whether this [CompilationUnit] is found in a "test" directory.
-  bool get inTestDir {
-    var declaredFragment = this.declaredFragment;
-    if (declaredFragment == null) return false;
-    var pathContext =
-        declaredFragment.element.session.resourceProvider.pathContext;
-    var path = declaredFragment.source.fullName;
-    return switch (pathContext.separator) {
-      '/' => const [
-        '/test/',
-        '/integration_test/',
-        '/test_driver/',
-        '/testing/',
-      ].any(path.contains),
-      r'\' => const [
-        r'\test\',
-        r'\integration_test\',
-        r'\test_driver\',
-        r'\testing\',
-      ].any(path.contains),
-      _ => false,
-    };
-  }
-}
-
 extension ExpressionExtension on Expression {
   /// Whether this expression is found in a [CommentReference].
   bool get inCommentReference =>

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

@@ -638,7 +638,7 @@ class BlazeWorkspacePackage extends WorkspacePackageImpl {
     // "in a test directory."
     return root.shortName == 'testing' ||
         root.path.contains('/testing/') ||
-        root.getChildAssumingFolder('test').contains(file.path);
+        super.isInTestDirectory(file);
   }
 
   @override

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

@@ -493,6 +493,19 @@ class PubPackage extends WorkspacePackageImpl {
     return _sdkVersionConstraint;
   }
 
+  Folder? get _generatedPackageRoot {
+    var packageName = _name;
+    if (packageName == null) {
+      return null;
+    }
+
+    var generatedRoot = _generatedPathParts.fold(root, (current, segment) {
+      return current.getChildAssumingFolder(segment);
+    });
+
+    return generatedRoot.getChildAssumingFolder(packageName);
+  }
+
   @override
   bool contains(Source source) {
     var uri = source.uri;
@@ -513,7 +526,13 @@ class PubPackage extends WorkspacePackageImpl {
 
   @override
   bool isInTestDirectory(File file) {
-    return root.getChildAssumingFolder('test').contains(file.path);
+    if (super.isInTestDirectory(file)) {
+      return true;
+    }
+
+    var generatedPackageRoot = _generatedPackageRoot;
+    return generatedPackageRoot != null &&
+        isInTestDirectoryUnder(generatedPackageRoot, file);
   }
 
   @override
@@ -530,27 +549,24 @@ class PubPackage extends WorkspacePackageImpl {
   bool sourceIsInPublicApi(Source source) {
     var filePath = filePathFromSource(source);
     if (filePath == null) return false;
-    var libFolder = root.getChildAssumingFolder('lib');
-    if (libFolder.contains(filePath)) {
+
+    bool sourceIsInPublicApiUnder(Folder root, String filePath) {
+      var libFolder = root.getChildAssumingFolder('lib');
+      if (!libFolder.contains(filePath)) {
+        return false;
+      }
+
       // A file in "$root/lib" is public iff it is not in "$root/lib/src".
       var libSrcFolder = libFolder.getChildAssumingFolder('src');
       return !libSrcFolder.contains(filePath);
     }
 
-    Folder intermediateFolder = root;
-    for (String part in _generatedPathParts) {
-      intermediateFolder = intermediateFolder.getChildAssumingFolder(part);
+    if (sourceIsInPublicApiUnder(root, filePath)) {
+      return true;
     }
-    libFolder = intermediateFolder
-        .getChildAssumingFolder('test')
-        .getChildAssumingFolder('lib');
 
-    if (libFolder.contains(filePath)) {
-      // A file in "$generated/lib" is public iff it is not in
-      // "$generated/lib/src".
-      var libSrcFolder = libFolder.getChildAssumingFolder('src');
-      return !libSrcFolder.contains(filePath);
-    }
-    return false;
+    var generatedPackageRoot = _generatedPackageRoot;
+    return generatedPackageRoot != null &&
+        sourceIsInPublicApiUnder(generatedPackageRoot, filePath);
   }
 }

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

@@ -69,6 +69,13 @@ abstract class Workspace {
 /// understand whether arbitrary file paths represent libraries declared within
 /// a given package in a Workspace.
 abstract class WorkspacePackageImpl implements WorkspacePackage {
+  static const Set<String> _testDirectoryNames = {
+    'test',
+    'integration_test',
+    'test_driver',
+    'testing',
+  };
+
   @override
   bool get canHavePublicApi => true;
 
@@ -113,7 +120,14 @@ abstract class WorkspacePackageImpl implements WorkspacePackage {
 
   @override
   bool isInTestDirectory(File file) {
-    return false;
+    return isInTestDirectoryUnder(root, file);
+  }
+
+  @protected
+  bool isInTestDirectoryUnder(Folder root, File file) {
+    return _testDirectoryNames.any((name) {
+      return root.getChildAssumingFolder(name).contains(file.path);
+    });
   }
 
   /// Return a map from the names of packages to the absolute and normalized

pkg/analyzer/test/src/diagnostics/assignment_of_do_not_store_test.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/src/diagnostic/diagnostic.dart' as diag;
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
@@ -18,6 +19,9 @@ main() {
 
 @reflectiveTest
 class AssignmentOfDoNotStoreInTestsTest extends PubPackageResolutionTest {
+  @override
+  File get testFile => getFile('$testPackageRootPath/test/test.dart');
+
   @override
   void setUp() {
     super.setUp();
@@ -25,8 +29,7 @@ class AssignmentOfDoNotStoreInTestsTest extends PubPackageResolutionTest {
   }
 
   test_noHintsInTestDir() async {
-    // Code that is in a test dir (the default for PubPackageResolutionTests)
-    // should not trigger the hint.
+    // Code that is in a test dir should not trigger the hint.
     // (See:https://github.com/dart-lang/sdk/issues/45594)
     await resolveTestCodeWithDiagnostics(r'''
 import 'package:meta/meta.dart';

pkg/analyzer/test/src/diagnostics/invalid_use_of_visible_for_testing_member_test.dart

@@ -10,6 +10,9 @@ import '../dart/resolution/context_collection_resolution.dart';
 main() {
   defineReflectiveSuite(() {
     defineReflectiveTests(InvalidUseOfVisibleForTestingMemberTest);
+    defineReflectiveTests(
+      InvalidUseOfVisibleForTestingMemberWithTestInAncestorPathTest,
+    );
   });
 }
 
@@ -472,3 +475,33 @@ void f() {
 ''');
   }
 }
+
+@reflectiveTest
+class InvalidUseOfVisibleForTestingMemberWithTestInAncestorPathTest
+    extends PubPackageResolutionTest {
+  @override
+  String get testPackageRootPath => '/home/test/my';
+
+  @override
+  void setUp() {
+    super.setUp();
+    writeTestPackageConfigWithMeta();
+  }
+
+  test_method_inAncestorTestDir() async {
+    newFile('$testPackageLibPath/lib1.dart', r'''
+import 'package:meta/meta.dart';
+class A {
+  @visibleForTesting
+  void a() {}
+}
+''');
+
+    await resolveTestCodeWithDiagnostics(r'''
+import 'lib1.dart';
+void f() => A().a();
+//              ^
+// [diag.invalidUseOfVisibleForTestingMember] The member 'a' can only be used within 'package:test/lib1.dart' or a test.
+''');
+  }
+}

pkg/analyzer/test/src/diagnostics/return_of_do_not_store_test.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analyzer/file_system/file_system.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import '../dart/resolution/context_collection_resolution.dart';
@@ -17,6 +18,9 @@ main() {
 
 @reflectiveTest
 class ReturnOfDoNotStoreInTestsTest extends PubPackageResolutionTest {
+  @override
+  File get testFile => getFile('$testPackageRootPath/test/test.dart');
+
   @override
   void setUp() {
     super.setUp();
@@ -24,8 +28,7 @@ class ReturnOfDoNotStoreInTestsTest extends PubPackageResolutionTest {
   }
 
   test_noHintsInTestDir() async {
-    // Code that is in a test dir (the default for PubPackageResolutionTests)
-    // should not trigger the hint.
+    // Code that is in a test dir should not trigger the hint.
     // (See:https://github.com/dart-lang/sdk/issues/45594)
     await resolveTestCodeWithDiagnostics(r'''
 import 'package:meta/meta.dart';

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

@@ -816,6 +816,71 @@ class PubPackageTest extends WorkspacePackageTest {
       pubPackage.isInTestDirectory(getFile('$myPackageRootPath/test/a.dart')),
       isTrue,
     );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageRootPath/integration_test/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageRootPath/test_driver/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageRootPath/testing/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageGeneratedPath/my/lib/a.dart'),
+      ),
+      isFalse,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageGeneratedPath/my/test/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageGeneratedPath/my/integration_test/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageGeneratedPath/my/test_driver/a.dart'),
+      ),
+      isTrue,
+    );
+
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myPackageGeneratedPath/my/testing/a.dart'),
+      ),
+      isTrue,
+    );
+
+    // Note, workspace-root relative path.
+    // Even though it has `test` segment, it has nothing with out package.
+    expect(
+      pubPackage.isInTestDirectory(
+        getFile('$myWorkspacePath/test/my/lib/a.dart'),
+      ),
+      isFalse,
+    );
   }
 
   void test_packagesAvailableTo() {
@@ -828,6 +893,22 @@ class PubPackageTest extends WorkspacePackageTest {
     );
   }
 
+  test_sourceIsInPublicApi() {
+    var pubPackage = myPackage as PubPackage;
+
+    bool isInPublicApi(String path) {
+      return pubPackage.sourceIsInPublicApi(FileSource(newFile(path, '')));
+    }
+
+    expect(isInPublicApi('$myPackageRootPath/lib/a.dart'), isTrue);
+    expect(isInPublicApi('$myPackageRootPath/lib/src/a.dart'), isFalse);
+    expect(isInPublicApi('$myPackageRootPath/test/a.dart'), isFalse);
+
+    expect(isInPublicApi('$myPackageGeneratedPath/my/lib/a.dart'), isTrue);
+    expect(isInPublicApi('$myPackageGeneratedPath/my/lib/src/a.dart'), isFalse);
+    expect(isInPublicApi('$myPackageGeneratedPath/test/lib/a.dart'), isFalse);
+  }
+
   Source _sourceWithFileUri(String path) {
     return FileSource(newFile(path, ''), toUri(path));
   }