Simplify computation of Package#sourceRoot

This field is initialized in `finishInit()`, but doesn't depend on anything discovered during BUILD file evaluation. So we may as well compute it during the constructor call and remove some complexity from this class.

This CL should not cause a behavioral change.

Structural changes:
- `sourceRoot` is now final.
- The existing method `computeSourceRoot()` is inlined and eliminated. A new static method by the same name is factored out of the relevant part of `finishInit()`. The effect is that the method boundary now encapsulates the actual job of determining the source root, whereas previously this work both spanned multiple methods and intermingled with unrelated tasks.
- Validation of the consistency between `buildFilename` and `isRepoRulePackage` is moved from `computeSourceRoot()` down into `Metadata`'s constructor. `isWorkspaceFile()` and `isModuleDotBazelFile()` are inlined into their sole call sites.

Changes to the body (hope this helps for reading the diff):
- The `baseName` local var is no longer needed.
- The trivial true branch of the outer `if` statement is turned into an early exit at the top of the method.
- Rewrote the early `return`s from the inlined method as `if`-`else`s that initialize a local var `sourceRoot`.
- The `throw` IAE statement is turned into a `Preconditions.checkArgument()`.
- Renamed local var `packageDirectory` -> `pkgDirFragment`, factored some other local vars up top.
- Added TODO about simplifying `current`'s initialization (didn't want to risk it in this CL).

Some test cases were previously creating `Package.Builder`s with bad BUILD filenames but not actually constructing the final package. This was allowed because the validation was deferred until `finishInit()`, which didn't run. Fixed those test cases to pass, now that the validation of BUILD filenames happens up front.

Work toward #19922. Drive-by simplification while doing other refactoring of `Package`'s fields.

PiperOrigin-RevId: 676390008
Change-Id: I1c928924adea5c5caaa6d056de50cb98ce17b79a

src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java

@@ -164,6 +164,8 @@ public PathFragment getPackageFragment() {
    * Returns a path to the source code for this package relative to the corresponding source root.
    * Returns pkgName for all repositories.
    */
+  // TODO(bazel-team): This name is misleading, since the returned object is not a source root.
+  // Maybe "getSourceRootRelative()"?
   public PathFragment getSourceRoot() {
     return pkgName;
   }

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -159,6 +159,8 @@ public enum ConfigSettingVisibilityPolicy {
 
   private final Metadata metadata;
 
+  private final Optional<Root> sourceRoot;
+
   // For BUILD files, this is initialized immediately. For WORKSPACE files, it is known only after
   // Starlark evaluation of the WORKSPACE file has finished.
   private String workspaceName;
@@ -172,11 +174,6 @@ public enum ConfigSettingVisibilityPolicy {
   // Mutated during BUILD file evaluation (but not by symbolic macro evaluation).
   private ImmutableMap<String, String> makeEnv;
 
-  // Computed in finishInit.
-  // TODO(bazel-team): There's no real reason this can't be computed sooner, reducing the set of
-  // things determined during/after BUILD file evaluation.
-  private Optional<Root> sourceRoot;
-
   // These two fields are mutually exclusive. Which one is set depends on
   // PackageSettings#precomputeTransitiveLoads. See Package.Builder#setLoads.
   @Nullable private ImmutableList<Module> directLoads;
@@ -282,6 +279,7 @@ public enum ConfigSettingVisibilityPolicy {
   // BUILD evaluation.
   private Package(Metadata metadata) {
     this.metadata = metadata;
+    this.sourceRoot = computeSourceRoot(metadata);
   }
 
   // ==== General package metadata accessors ====
@@ -704,45 +702,60 @@ public MacroInstance getDeclaringMacroForTarget(String target) {
 
   // ==== Initialization ====
 
+  private static Optional<Root> computeSourceRoot(Metadata metadata) {
+    if (metadata.isRepoRulePackage()) {
+      return Optional.empty();
+    }
+
+    RootedPath buildFileRootedPath = metadata.buildFilename();
+    Root buildFileRoot = buildFileRootedPath.getRoot();
+    PathFragment pkgIdFragment = metadata.packageIdentifier().getSourceRoot();
+    PathFragment pkgDirFragment = buildFileRootedPath.getRootRelativePath().getParentDirectory();
+
+    Root sourceRoot;
+    if (pkgIdFragment.equals(pkgDirFragment)) {
+      // Fast path: BUILD file path and package name are the same, don't create an extra root.
+      sourceRoot = buildFileRoot;
+    } else {
+      // TODO(bazel-team): Can this expr be simplified to just pkgDirFragment?
+      PathFragment current = buildFileRootedPath.asPath().asFragment().getParentDirectory();
+      for (int i = 0, len = pkgIdFragment.segmentCount(); i < len && current != null; i++) {
+        current = current.getParentDirectory();
+      }
+      if (current == null || current.isEmpty()) {
+        // This is never really expected to work. The below check should fail.
+        sourceRoot = buildFileRoot;
+      } else {
+        // Note that current is an absolute path.
+        sourceRoot = Root.fromPath(buildFileRoot.getRelative(current));
+      }
+    }
+
+    Preconditions.checkArgument(
+        sourceRoot.asPath() != null
+            && sourceRoot.getRelative(pkgIdFragment).equals(metadata.getPackageDirectory()),
+        "Invalid BUILD file name for package '%s': %s (in source %s with packageDirectory %s and"
+            + " package identifier source root %s)",
+        metadata.packageIdentifier(),
+        metadata.buildFilename(),
+        sourceRoot,
+        metadata.getPackageDirectory(),
+        metadata.packageIdentifier().getSourceRoot());
+
+    return Optional.of(sourceRoot);
+  }
+
   /**
    * Completes the initialization of this package. Only after this method may a package by shared
    * publicly.
    */
   private void finishInit(Builder builder) {
-    String baseName = metadata.buildFilename().getRootRelativePath().getBaseName();
-
     this.containsErrors |= builder.containsErrors;
     if (directLoads == null && transitiveLoads == null) {
       Preconditions.checkState(containsErrors, "Loads not set for error-free package");
       builder.setLoads(ImmutableList.of());
     }
 
-    if (isWorkspaceFile(baseName) || isModuleDotBazelFile(baseName)) {
-      Preconditions.checkState(isRepoRulePackage());
-      this.sourceRoot = Optional.empty();
-    } else {
-      Root sourceRoot =
-          computeSourceRoot(metadata.buildFilename(), metadata.packageIdentifier().getSourceRoot());
-      if (sourceRoot.asPath() == null
-          || !sourceRoot
-              .getRelative(metadata.packageIdentifier().getSourceRoot())
-              .equals(metadata.getPackageDirectory())) {
-        throw new IllegalArgumentException(
-            "Invalid BUILD file name for package '"
-                + metadata.packageIdentifier()
-                + "': "
-                + metadata.buildFilename()
-                + " (in source "
-                + sourceRoot
-                + " with packageDirectory "
-                + metadata.getPackageDirectory()
-                + " and package identifier source root "
-                + metadata.packageIdentifier().getSourceRoot()
-                + ")");
-      }
-      this.sourceRoot = Optional.of(sourceRoot);
-    }
-
     this.workspaceName = builder.workspaceName;
 
     this.makeEnv = ImmutableMap.copyOf(builder.makeEnv);
@@ -774,34 +787,6 @@ private void finishInit(Builder builder) {
     this.packageOverhead = overheadEstimate.orElse(PACKAGE_OVERHEAD_UNSET);
   }
 
-  private static boolean isWorkspaceFile(String baseFileName) {
-    return baseFileName.equals(LabelConstants.WORKSPACE_DOT_BAZEL_FILE_NAME.getPathString())
-        || baseFileName.equals(LabelConstants.WORKSPACE_FILE_NAME.getPathString());
-  }
-
-  private static boolean isModuleDotBazelFile(String baseFileName) {
-    return baseFileName.equals(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME.getPathString());
-  }
-
-  private static Root computeSourceRoot(
-      RootedPath buildFileRootedPath, PathFragment packageFragment) {
-    PathFragment packageDirectory = buildFileRootedPath.getRootRelativePath().getParentDirectory();
-    if (packageFragment.equals(packageDirectory)) {
-      // Fast path: BUILD file path and package name are the same, don't create an extra root.
-      return buildFileRootedPath.getRoot();
-    }
-    PathFragment current = buildFileRootedPath.asPath().asFragment().getParentDirectory();
-    for (int i = 0, len = packageFragment.segmentCount(); i < len && current != null; i++) {
-      current = current.getParentDirectory();
-    }
-    if (current == null || current.isEmpty()) {
-      // This is never really expected to work. The check below in #finishInit should fail.
-      return buildFileRootedPath.getRoot();
-    }
-    // Note that current is an absolute path.
-    return Root.fromPath(buildFileRootedPath.getRoot().getRelative(current));
-  }
-
   // TODO(bazel-team): This is a mutation, but Package is supposed to be immutable. In practice it
   // seems like this is only called during Package construction (including deserialization),
   // principally via Rule#reportError. I would bet that all of the callers have access to a
@@ -2770,6 +2755,16 @@ public record Metadata(
       Preconditions.checkNotNull(repositoryMapping);
       Preconditions.checkNotNull(associatedModuleName);
       Preconditions.checkNotNull(associatedModuleVersion);
+
+      // Check for consistency between isRepoRulePackage and whether the buildFilename is a
+      // WORKSPACE / MODULE.bazel file.
+      String baseName = buildFilename.asPath().getBaseName();
+      boolean isWorkspaceFile =
+          baseName.equals(LabelConstants.WORKSPACE_DOT_BAZEL_FILE_NAME.getPathString())
+              || baseName.equals(LabelConstants.WORKSPACE_FILE_NAME.getPathString());
+      boolean isModuleDotBazelFile =
+          baseName.equals(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME.getPathString());
+      Preconditions.checkArgument(isRepoRulePackage == (isWorkspaceFile || isModuleDotBazelFile));
     }
 
     /**

src/test/java/com/google/devtools/build/lib/packages/PackageTest.java

@@ -162,7 +162,8 @@ private Package.Builder pkgBuilder(String name) {
         PackageSettings.DEFAULTS,
         PackageIdentifier.createInMainRepo(name),
         /* filename= */ RootedPath.toRootedPath(
-            Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name)),
+            Root.fromPath(fileSystem.getPath("/irrelevantRoot")),
+            PathFragment.create(name + "/BUILD")),
         "workspace",
         Optional.empty(),
         Optional.empty(),

src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java

@@ -219,7 +219,7 @@ private static void assertAttr(RuleClass ruleClass, String attrName, Type<?> typ
 
   @Test
   public void testOutputFileNotEqualDot() {
-    Path myPkgPath = scratch.resolve("/workspace/mypkg");
+    Path myPkgPath = scratch.resolve("/workspace/mypkg/BUILD");
     Package.Builder pkgBuilder = newBuilder(PackageIdentifier.createInMainRepo("mypkg"), myPkgPath);
 
     Map<String, Object> attributeValues = new HashMap<>();