diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md index 536b4e0d055f6b..28592bb0fa2a39 100644 --- a/site/docs/skylark/backward-compatibility.md +++ b/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` + diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index 43d984628b3fe6..ecde5243512226 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/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; diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 42da1f3b6728d8..4af330c7c7ef7f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/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) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java index bbf75110783a9b..93aa4545548d5c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java +++ b/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 { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java index 725d20193a3d1a..c9a341e42ade7e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java +++ b/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 { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index da528dc915b485..4a0543feeb9110 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -675,13 +675,7 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles Set containingPkgLookupKeys = Sets.newHashSet(); Map 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; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java index e16b5e987d027e..dd1e50af1ca30e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java +++ b/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)); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index d6eb28e144c5a8..9fba5285f394c9 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/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); diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java index fdfb30bdb8ce66..6ea333fa0fff3d 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java +++ b/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")); + } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 8246bf7f745608..661ab406739837 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/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()) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java index dc61c61df4d3e8..93cf9e8324359f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; @@ -25,6 +26,7 @@ import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions; import com.google.devtools.build.lib.pkgcache.PackageCacheOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Path; @@ -262,4 +264,160 @@ public void testLoadFromExternalRepoInWorkspaceFileAllowed() throws Exception { assertThat(result.hasError()).isFalse(); } + + @Test + public void testLoadUsingLabelThatDoesntCrossesBoundaryOfPackage() + throws Exception { + scratch.file("a/BUILD"); + scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')"); + scratch.file("a/b/b.bzl", "b = 42"); + + checkSuccessfulLookup("//a:a.bzl"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfSamePkg() + throws Exception { + scratch.file("a/BUILD"); + scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')"); + scratch.file("a/b/BUILD", ""); + scratch.file("a/b/b.bzl", "b = 42"); + + checkSuccessfulLookup("//a:a.bzl"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfSamePkg() + throws Exception { + setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries"); + + scratch.file("a/BUILD"); + scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')"); + scratch.file("a/b/BUILD", ""); + scratch.file("a/b/b.bzl", "b = 42"); + + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a:a.bzl"), false); + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertThat(result.hasError()).isTrue(); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .isInstanceOf(SkylarkImportFailedException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .hasMessageThat() + .contains( + "Label '//a:b/b.bzl' crosses boundary of subpackage 'a/b' (perhaps you meant to put " + + "the colon here: '//a/b:b.bzl'?)"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfDifferentPkgUnder() + throws Exception { + scratch.file("a/BUILD"); + scratch.file("a/a.bzl", "load('//a/b:c/c.bzl', 'c')"); + scratch.file("a/b/BUILD", ""); + scratch.file("a/b/c/BUILD", ""); + scratch.file("a/b/c/c.bzl", "c = 42"); + + checkSuccessfulLookup("//a:a.bzl"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgUnder() + throws Exception { + setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries"); + + scratch.file("a/BUILD"); + scratch.file("a/a.bzl", "load('//a/b:c/c.bzl', 'c')"); + scratch.file("a/b/BUILD", ""); + scratch.file("a/b/c/BUILD", ""); + scratch.file("a/b/c/c.bzl", "c = 42"); + + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a:a.bzl"), false); + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertThat(result.hasError()).isTrue(); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .isInstanceOf(SkylarkImportFailedException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .hasMessageThat() + .contains( + "Label '//a/b:c/c.bzl' crosses boundary of subpackage 'a/b/c' (perhaps you meant to " + + "put the colon here: '//a/b/c:c.bzl'?)"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfDifferentPkgAbove() + throws Exception { + scratch.file("a/b/BUILD"); + scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')"); + scratch.file("a/BUILD"); + scratch.file("a/c/c/c.bzl", "c = 42"); + + // With the default of + // --incompatible_disallow_load_labels_to_cross_subpackage_boundaries=false, + // SkylarkImportLookupValue(//a/b:b.bzl) has an error because ASTFileLookupValue(//a/c:c/c.bzl) + // because package //a/c doesn't exist. The behavior with + // --incompatible_disallow_load_labels_to_cross_subpackage_boundaries=true is stricter, but we + // still have an explicit test for this case so that way we don't forget to think about it when + // we address the TODO in ASTFuleLookupFunction. + + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a/b:b.bzl"), false); + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertThat(result.hasError()).isTrue(); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .isInstanceOf(SkylarkImportFailedException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .hasMessageThat() + .contains( + "Extension file not found. Unable to load package for '//a/c:c/c.bzl': BUILD file not " + + "found on package path"); + } + + @Test + public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgAbove() + throws Exception { + setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries"); + + scratch.file("a/b/BUILD"); + scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')"); + scratch.file("a/BUILD"); + scratch.file("a/c/c/c.bzl", "c = 42"); + + SkyKey skylarkImportLookupKey = + SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a/b:b.bzl"), false); + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter); + assertThat(result.hasError()).isTrue(); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .isInstanceOf(SkylarkImportFailedException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skylarkImportLookupKey) + .hasExceptionThat() + .hasMessageThat() + .contains( + "Label '//a/c:c/c.bzl' crosses boundary of package 'a' (perhaps you meant to put the " + + "colon here: '//a:c/c/c.bzl'?)"); + } } diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh index 5e1d76642c2f41..99bb0c2be5ea90 100755 --- a/src/test/shell/integration/loading_phase_test.sh +++ b/src/test/shell/integration/loading_phase_test.sh @@ -348,4 +348,27 @@ function test_no_package_loading_on_benign_workspace_file_changes() { expect_log "//$pkg/foo:shname2" } +function test_incompatible_disallow_load_labels_to_cross_package_boundaries() { + local -r pkg="${FUNCNAME}" + mkdir -p "$pkg" || fail "could not create \"$pkg\"" + + mkdir "$pkg"/foo + echo "load(\"//$pkg/foo/a:b/b.bzl\", \"b\")" > "$pkg"/foo/BUILD + mkdir -p "$pkg"/foo/a/b + touch "$pkg"/foo/a/BUILD + touch "$pkg"/foo/a/b/BUILD + echo "b = 42" > "$pkg"/foo/a/b/b.bzl + + bazel query "$pkg/foo:BUILD" >& "$TEST_log" || fail "Expected success" + expect_log "//$pkg/foo:BUILD" + + bazel query \ + --incompatible_disallow_load_labels_to_cross_package_boundaries=true \ + "$pkg/foo:BUILD" >& "$TEST_log" && fail "Expected failure" + expect_log "Label '//$pkg/foo/a:b/b.bzl' crosses boundary of subpackage '$pkg/foo/a/b'" + + bazel query "$pkg/foo:BUILD" >& "$TEST_log" || fail "Expected success" + expect_log "//$pkg/foo:BUILD" +} + run_suite "Integration tests of ${PRODUCT_NAME} using loading/analysis phases."