Error-out if the label in a 'load' statement crosses a subpackage bou…

…ndary. This behavior is guarded by a flag (default value is "don't error out"), and will eventually become the only behavior.

This implements the feature in #6408.

RELNOTES: None
PiperOrigin-RevId: 217402217

site/docs/skylark/backward-compatibility.md

@@ -58,6 +58,7 @@ guarded behind flags in the current release:
 *   [Disable legacy C++ configuration API](#disable-legacy-c-configuration-api)
 *   [Disable legacy C++ toolchain API](#disable-legacy-c-toolchain-api)
 *   [Disallow `cfg = "data"`](#disallow-cfg--data)
+*   [Load label cannot cross package boundaries](#load-label-cannot-cross-package-boundaries)
 
 
 ### Dictionary concatenation
@@ -956,4 +957,24 @@ fail with an error.
 *   Default: `false`
 *   Introduced in: `0.16.0`
 
+
+### Load label cannot cross package boundaries
+
+Previously, the label argument to the `load` statement (the first argument) was
+checked to ensure that it referenced an existing package but it was not checked
+to ensure that it didn't cross a package boundary.
+
+For example, in
+
+```python
+load('//a:b/c.bzl', 'doesntmatter')
+```
+
+if this flag is set to `true`, the above statement will be in error if `//a/b`
+is a package; in such a case, the correct way to reference `c.bzl` via a label
+would be `//a/b:c.bzl`.
+
+*   Flag: `--incompatible_disallow_load_labels_to_cross_package_boundaries`
+*   Default: `false`
+
 <!-- Add new options here -->

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

@@ -639,6 +639,23 @@ public static String print(@Nullable Label label) {
     return label == null ? "(unknown)" : label.toString();
   }
 
+  /**
+   * Returns a {@link PathFragment} corresponding to the directory in which {@code label} would
+   * reside, if it were interpreted to be a path.
+   */
+  public static PathFragment getContainingDirectory(Label label) {
+    PathFragment pkg = label.getPackageFragment();
+    String name = label.getName();
+    if (name.equals(".")) {
+      return pkg;
+    }
+    if (PathFragment.isNormalizedRelativePath(name) && !PathFragment.containsSeparator(name)) {
+      // Optimize for the common case of a label like '//pkg:target'.
+      return pkg;
+    }
+    return pkg.getRelative(name).getParentDirectory();
+  }
+
   @Override
   public boolean isImmutable() {
     return true;

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

@@ -264,6 +264,19 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
   )
   public boolean incompatibleDisallowLegacyJavaInfo;
 
+  @Option(
+      name = "incompatible_disallow_load_labels_to_cross_package_boundaries",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+          OptionMetadataTag.INCOMPATIBLE_CHANGE,
+          OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help = "If set to true, the label argument to 'load' cannot cross a package boundary."
+  )
+  public boolean incompatibleDisallowLoadLabelsToCrossPackageBoundaries;
+
   @Option(
       name = "incompatible_generate_javacommon_source_jar",
       defaultValue = "false",
@@ -520,6 +533,8 @@ public SkylarkSemantics toSkylarkSemantics() {
         .incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
         .incompatibleDisallowFileType(incompatibleDisallowFileType)
         .incompatibleDisallowLegacyJavaInfo(incompatibleDisallowLegacyJavaInfo)
+        .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(
+            incompatibleDisallowLoadLabelsToCrossPackageBoundaries)
         .incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
         .incompatibleDisallowSlashOperator(incompatibleDisallowSlashOperator)
         .incompatibleExpandDirectories(incompatibleExpandDirectories)

src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java

@@ -59,6 +59,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionExcept
     PathFragment filePathFragment = fileLabel.toPathFragment();
 
     // Determine whether the package designated by fileLabel exists.
+    // TODO(bazel-team): After --incompatible_disallow_load_labels_to_cross_package_boundaries is
+    // removed and the new behavior is unconditional, we can instead safely assume the package
+    // exists and pass in the Root in the SkyKey and therefore this dep can be removed.
     SkyKey pkgSkyKey = PackageLookupValue.key(fileLabel.getPackageIdentifier());
     PackageLookupValue pkgLookupValue = null;
     try {

src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java

@@ -15,9 +15,11 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Interner;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.skyframe.AbstractSkyKey;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -47,6 +49,40 @@ public static Key key(PackageIdentifier id) {
     return Key.create(id);
   }
 
+  static String getErrorMessageForLabelCrossingPackageBoundary(
+      Root pkgRoot,
+      Label label,
+      ContainingPackageLookupValue containingPkgLookupValue) {
+    PackageIdentifier containingPkg = containingPkgLookupValue.getContainingPackageName();
+    boolean crossesPackageBoundaryBelow =
+        containingPkg.getSourceRoot().startsWith(label.getPackageIdentifier().getSourceRoot());
+    PathFragment labelNameFragment = PathFragment.create(label.getName());
+    String message = String.format("Label '%s' crosses boundary of %spackage '%s'",
+        label,
+        crossesPackageBoundaryBelow ? "sub" : "",
+        containingPkg);
+    Root containingRoot = containingPkgLookupValue.getContainingPackageRoot();
+    if (pkgRoot.equals(containingRoot)) {
+      PathFragment containingPkgFragment = containingPkg.getPackageFragment();
+      PathFragment labelNameInContainingPackage =
+          crossesPackageBoundaryBelow
+              ? labelNameFragment.subFragment(
+                  containingPkgFragment.segmentCount()
+                      - label.getPackageFragment().segmentCount(),
+                  labelNameFragment.segmentCount())
+              : label.toPathFragment().relativeTo(containingPkgFragment);
+      message += " (perhaps you meant to put the colon here: '";
+      if (containingPkg.getRepository().isDefault() || containingPkg.getRepository().isMain()) {
+        message += "//";
+      }
+      message += containingPkg + ":" + labelNameInContainingPackage + "'?)";
+    } else {
+      message += " (have you deleted " + containingPkg + "/BUILD? "
+          + "If so, use the --deleted_packages=" + containingPkg + " option)";
+    }
+    return message;
+  }
+
   @AutoCodec.VisibleForSerialization
   @AutoCodec
   static class Key extends AbstractSkyKey<PackageIdentifier> {

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -675,13 +675,7 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
     Set<SkyKey> containingPkgLookupKeys = Sets.newHashSet();
     Map<Target, SkyKey> targetToKey = new HashMap<>();
     for (Target target : pkgBuilder.getTargets()) {
-      PathFragment dir = getContainingDirectory(target.getLabel());
-      if (dir == null) {
-        throw new IllegalStateException(
-            String.format(
-                "Null pkg for label %s as path fragment %s in pkg %s",
-                target.getLabel(), target.getLabel().getPackageFragment(), pkgId));
-      }
+      PathFragment dir = Label.getContainingDirectory(target.getLabel());
       if (dir.equals(pkgDir)) {
         continue;
       }
@@ -707,27 +701,18 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
       ContainingPackageLookupValue containingPackageLookupValue =
           getContainingPkgLookupValueAndPropagateInconsistentFilesystemExceptions(
               pkgId, containingPkgLookupValues.get(key), env);
-        if (maybeAddEventAboutLabelCrossingSubpackage(pkgBuilder, pkgRoot, target.getLabel(),
-          target.getLocation(), containingPackageLookupValue)) {
+      if (maybeAddEventAboutLabelCrossingSubpackage(
+          pkgBuilder,
+          pkgRoot,
+          target.getLabel(),
+          target.getLocation(),
+          containingPackageLookupValue)) {
         pkgBuilder.removeTarget(target);
         pkgBuilder.setContainsErrors();
       }
     }
   }
 
-  private static PathFragment getContainingDirectory(Label label) {
-    PathFragment pkg = label.getPackageFragment();
-    String name = label.getName();
-    if (name.equals(".")) {
-      return pkg;
-    }
-    if (PathFragment.isNormalizedRelativePath(name) && !PathFragment.containsSeparator(name)) {
-      // Optimize for the common case of a label like '//pkg:target'.
-      return pkg;
-    }
-    return pkg.getRelative(name).getParentDirectory();
-  }
-
   @Nullable
   private static ContainingPackageLookupValue
       getContainingPkgLookupValueAndPropagateInconsistentFilesystemExceptions(
@@ -774,24 +759,8 @@ private static boolean maybeAddEventAboutLabelCrossingSubpackage(
       // exceptions), it reaches here, and we tolerate it.
       return false;
     }
-    PathFragment labelNameFragment = PathFragment.create(label.getName());
-    String message = String.format("Label '%s' crosses boundary of subpackage '%s'",
-        label, containingPkg);
-    Root containingRoot = containingPkgLookupValue.getContainingPackageRoot();
-    if (pkgRoot.equals(containingRoot)) {
-      PathFragment labelNameInContainingPackage = labelNameFragment.subFragment(
-          containingPkg.getPackageFragment().segmentCount()
-              - label.getPackageFragment().segmentCount(),
-          labelNameFragment.segmentCount());
-      message += " (perhaps you meant to put the colon here: '";
-      if (containingPkg.getRepository().isDefault() || containingPkg.getRepository().isMain()) {
-        message += "//";
-      }
-      message += containingPkg + ":" + labelNameInContainingPackage + "'?)";
-    } else {
-      message += " (have you deleted " + containingPkg + "/BUILD? "
-          + "If so, use the --deleted_packages=" + containingPkg + " option)";
-    }
+    String message = ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+        pkgRoot, label, containingPkgLookupValue);
     pkgBuilder.addEvent(Event.error(location, message));
     return true;
   }

src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java

@@ -207,6 +207,34 @@ private SkylarkImportLookupValue computeInternal(
       return null;
     }
 
+    if (skylarkSemantics.incompatibleDisallowLoadLabelsToCrossPackageBoundaries()) {
+      PathFragment dir = Label.getContainingDirectory(fileLabel);
+      PackageIdentifier dirId =
+          PackageIdentifier.create(fileLabel.getPackageIdentifier().getRepository(), dir);
+      ContainingPackageLookupValue containingPackageLookupValue;
+      try {
+        containingPackageLookupValue = (ContainingPackageLookupValue) env.getValueOrThrow(
+            ContainingPackageLookupValue.key(dirId),
+            BuildFileNotFoundException.class,
+            InconsistentFilesystemException.class);
+      } catch (BuildFileNotFoundException e) {
+        throw SkylarkImportFailedException.errorReadingFile(
+            fileLabel.toPathFragment(),
+            new ErrorReadingSkylarkExtensionException(e));
+      }
+      if (containingPackageLookupValue == null) {
+        return null;
+      }
+      if (!containingPackageLookupValue.hasContainingPackage()) {
+        throw SkylarkImportFailedException.noBuildFile(fileLabel.toPathFragment());
+      }
+      if (!containingPackageLookupValue.getContainingPackageName().equals(
+          fileLabel.getPackageIdentifier())) {
+        throw SkylarkImportFailedException.labelCrossesPackageBoundary(
+            fileLabel, containingPackageLookupValue);
+      }
+    }
+
     // Load the AST corresponding to this file.
     ASTFileLookupValue astLookupValue;
     try {
@@ -534,6 +562,20 @@ static SkylarkImportFailedException noBuildFile(PathFragment file) {
               + " Note that this BUILD file does not need to do anything except exist.", file));
     }
 
+    static SkylarkImportFailedException labelCrossesPackageBoundary(
+        Label fileLabel,
+        ContainingPackageLookupValue containingPackageLookupValue) {
+      return new SkylarkImportFailedException(
+          ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+              // We don't actually know the proper Root to pass in here (since we don't e.g. know
+              // the root of the bzl/BUILD file that is trying to load 'fileLabel'). Therefore we
+              // just pass in the Root of the containing package in order to still get a useful
+              // error message for the user.
+              containingPackageLookupValue.getContainingPackageRoot(),
+              fileLabel,
+              containingPackageLookupValue));
+    }
+
     static SkylarkImportFailedException skylarkErrors(PathFragment file) {
       return new SkylarkImportFailedException(String.format("Extension '%s' has errors", file));
     }

src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java

@@ -124,6 +124,8 @@ public boolean isFeatureEnabledBasedOnTogglingFlags(
 
   public abstract boolean incompatibleDisallowLegacyJavaInfo();
 
+  public abstract boolean incompatibleDisallowLoadLabelsToCrossPackageBoundaries();
+
   public abstract boolean incompatibleDisallowOldStyleArgsAdd();
 
   public abstract boolean incompatibleDisallowSlashOperator();
@@ -189,6 +191,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDisallowDictPlus(false)
           .incompatibleDisallowFileType(false)
           .incompatibleDisallowLegacyJavaInfo(false)
+          .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(false)
           .incompatibleDisallowOldStyleArgsAdd(false)
           .incompatibleDisallowSlashOperator(false)
           .incompatibleExpandDirectories(false)
@@ -245,6 +248,8 @@ public abstract static class Builder {
 
     public abstract Builder incompatibleDisallowLegacyJavaInfo(boolean value);
 
+    public abstract Builder incompatibleDisallowLoadLabelsToCrossPackageBoundaries(boolean value);
+
     public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value);
 
     public abstract Builder incompatibleDisallowSlashOperator(boolean value);

src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java

@@ -469,4 +469,16 @@ public void testGetWorkspaceRoot() throws Exception {
     label = Label.parseAbsolute("@repo//bar/baz", ImmutableMap.of());
     assertThat(label.getWorkspaceRoot()).isEqualTo("external/repo");
   }
+
+  @Test
+  public void testGetContainingDirectory() {
+    assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a:b")))
+        .isEqualTo(PathFragment.create("a"));
+    assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a/b:c")))
+        .isEqualTo(PathFragment.create("a/b"));
+    assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a:b/c")))
+        .isEqualTo(PathFragment.create("a/b"));
+    assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a/b/c")))
+        .isEqualTo(PathFragment.create("a/b/c"));
+  }
 }

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

@@ -137,6 +137,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex
         "--incompatible_disallow_dict_plus=" + rand.nextBoolean(),
         "--incompatible_disallow_filetype=" + rand.nextBoolean(),
         "--incompatible_disallow_legacy_javainfo=" + rand.nextBoolean(),
+        "--incompatible_disallow_load_labels_to_cross_package_boundaries=" + rand.nextBoolean(),
         "--incompatible_disallow_old_style_args_add=" + rand.nextBoolean(),
         "--incompatible_disallow_slash_operator=" + rand.nextBoolean(),
         "--incompatible_expand_directories=" + rand.nextBoolean(),
@@ -180,6 +181,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleDisallowDictPlus(rand.nextBoolean())
         .incompatibleDisallowFileType(rand.nextBoolean())
         .incompatibleDisallowLegacyJavaInfo(rand.nextBoolean())
+        .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(rand.nextBoolean())
         .incompatibleDisallowOldStyleArgsAdd(rand.nextBoolean())
         .incompatibleDisallowSlashOperator(rand.nextBoolean())
         .incompatibleExpandDirectories(rand.nextBoolean())