Refactor RuleVisibility validation and concatenation
This CL simplifies RuleVisibility concatenation by inlining concatWithElement() into concatWithPackage(). The latter was the only caller of the former in production. The implementation no longer has to handle the case where the RHS is public or private -- something that was not needed in practice, but which was required in abstract for the deleted method's API to be correct. It also now doesn't need to parse an untrusted label, so there's no need to catch or throw EvalException.
concatWithPackage() is also made non-static, which looks nicer at the call sites.
The validate() method is renamed to checkForVisibilityMisspelling(), since that's the only thing it does now. (A previous CL removed its other job of detecting formerly illegal combinations of public/private visibility with other items in a list.)
Added an equals() and hashCode() while we're at it, to avoid depending on singleton identity (I'm not sure whether @SerializationConstant lets us assume that). It's based on just getDeclaredLabels() since that should superset any information returned by getDependencyLabels(). Changed RuleVisibility from an interface to an abstract class too -- I would've made it `sealed`, but the singleton PUBLIC and PRIVATE anonymous subclasses prevent that and it's not worth making them named classes.
MacroInstance.java
- Implement the TODO. (I went with "getDefinitionPackage" over "getDefiningPackage" because the latter might be confused with the package that defines the macro *instance*.)
MacroClass.java
- Condense to if-expr form now that the else branch is more concise.
RuleVisibilityTest.java
- Add pkg() helper for concatenation() test case.
- Remove assertEqual() now that we have a RuleVisibility#equals() method.
- Rename "B" -> "B_PG" for clarity/contrast with "A" and "C".
- Delete test cases for concatWithElement that are no longer applicable (where the RHS is public or private visibility)
Work toward #23855.
PiperOrigin-RevId: 689400241
Change-Id: I6b7a1c9f87c4b3c9e5cd99d5dc4d906b5d798d28
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
index 7bc2640..3f54d1f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
@@ -250,18 +250,11 @@
parsedVisibility = RuleVisibility.parse(liftedVisibility);
}
// Concatenate the visibility (as previously populated) with the instantiation site's location.
- PackageIdentifier instantiatingLoc;
- if (parentMacroFrame == null) {
- instantiatingLoc = pkgBuilder.getPackageIdentifier();
- } else {
- instantiatingLoc =
- parentMacroFrame
- .macroInstance
- .getMacroClass()
- .getDefiningBzlLabel()
- .getPackageIdentifier();
- }
- parsedVisibility = RuleVisibility.concatWithPackage(parsedVisibility, instantiatingLoc);
+ PackageIdentifier instantiatingLoc =
+ parentMacroFrame == null
+ ? pkgBuilder.getPackageIdentifier()
+ : parentMacroFrame.macroInstance.getDefinitionPackage();
+ parsedVisibility = parsedVisibility.concatWithPackage(instantiatingLoc);
attrValues.put("visibility", parsedVisibility.getDeclaredLabels());
// Populate defaults for the rest, and validate that no mandatory attr was missed.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
index 417ec32..05cc112 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
@@ -181,21 +181,14 @@
}
/**
- * Returns a {@code RuleVisibility} representing the result of concatenating this macro's {@link
- * MacroClass}'s definition location to the given {@code visibility}.
+ * Returns the package containing the .bzl file from which this macro instance's macro class was
+ * exported.
*
- * <p>The definition location of a macro class is the package containing the .bzl file from which
- * the macro class was exported.
- *
- * <p>Logically, this represents the visibility that a target would have, if it were passed the
- * given value for its {@code visibility} attribute, and if the target were declared directly in
- * this macro (i.e. not in a submacro).
+ * <p>This is considered to be the place where the macro's code lives, and is used as the place
+ * where a target is instantiated for the purposes of Macro-Aware Visibility.
*/
- // TODO: #19922 - Maybe refactor this to getDefinitionLocation and let the caller do the concat,
- // so we don't have to basically repeat it in MacroClass#instantiateMacro.
- public RuleVisibility concatDefinitionLocationToVisibility(RuleVisibility visibility) {
- PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier();
- return RuleVisibility.concatWithPackage(visibility, macroLocation);
+ public PackageIdentifier getDefinitionPackage() {
+ return macroClass.getDefiningBzlLabel().getPackageIdentifier();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
index 048367e..e3b55a8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
@@ -23,7 +23,7 @@
/** A rule visibility that allows visibility to a list of package groups. */
@AutoValue
-public abstract class PackageGroupsRuleVisibility implements RuleVisibility {
+public abstract class PackageGroupsRuleVisibility extends RuleVisibility {
public abstract ImmutableList<Label> getPackageGroups();
public abstract PackageGroupContents getDirectPackages();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 3aa6c4a..a989416 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -311,7 +311,7 @@
}
}
- return currentMacro.concatDefinitionLocationToVisibility(visibility).getDeclaredLabels();
+ return visibility.concatWithPackage(currentMacro.getDefinitionPackage()).getDeclaredLabels();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
index 0c4ca66..71b585f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
@@ -17,8 +17,8 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
-import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.List;
+import java.util.Objects;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
@@ -32,14 +32,14 @@
*
* <p>All implementations of this interface are immutable.
*/
-public interface RuleVisibility {
+public abstract class RuleVisibility {
/**
* Returns the list of all labels comprising this visibility.
*
* <p>This includes labels that are not loadable, such as //visibility:public and //foo:__pkg__.
*/
- List<Label> getDeclaredLabels();
+ public abstract List<Label> getDeclaredLabels();
/**
* Same as {@link #getDeclaredLabels}, but excludes labels that cannot be loaded.
@@ -48,23 +48,25 @@
* order to determine the complete set of packages represented by this visibility. (Additional
* {@code package_group}s may need to be loaded due to their {@code includes} attribute.)
*/
- List<Label> getDependencyLabels();
-
- @SerializationConstant Label PUBLIC_LABEL = Label.parseCanonicalUnchecked("//visibility:public");
+ public abstract List<Label> getDependencyLabels();
@SerializationConstant
- Label PRIVATE_LABEL = Label.parseCanonicalUnchecked("//visibility:private");
+ public static final Label PUBLIC_LABEL = Label.parseCanonicalUnchecked("//visibility:public");
+
+ @SerializationConstant
+ public static final Label PRIVATE_LABEL = Label.parseCanonicalUnchecked("//visibility:private");
// Constant for memory efficiency; see b/370873477.
@SerializationConstant
- ImmutableList<Label> PUBLIC_DECLARED_LABELS = ImmutableList.of(PUBLIC_LABEL);
+ public static final ImmutableList<Label> PUBLIC_DECLARED_LABELS = ImmutableList.of(PUBLIC_LABEL);
// Constant for memory efficiency; see b/370873477.
@SerializationConstant
- ImmutableList<Label> PRIVATE_DECLARED_LABELS = ImmutableList.of(PRIVATE_LABEL);
+ public static final ImmutableList<Label> PRIVATE_DECLARED_LABELS =
+ ImmutableList.of(PRIVATE_LABEL);
@SerializationConstant
- RuleVisibility PUBLIC =
+ public static final RuleVisibility PUBLIC =
new RuleVisibility() {
@Override
public ImmutableList<Label> getDeclaredLabels() {
@@ -83,7 +85,7 @@
};
@SerializationConstant
- RuleVisibility PRIVATE =
+ public static final RuleVisibility PRIVATE =
new RuleVisibility() {
@Override
public ImmutableList<Label> getDeclaredLabels() {
@@ -143,19 +145,30 @@
return null;
}
- @CanIgnoreReturnValue
- private static Label validate(Label label) throws EvalException {
+ /**
+ * Throws if the label is in the special {@code //visibility} package but is neither {@code
+ * //visibility:__pkg__} nor {@code //visibility:__subpackages__}.
+ *
+ * <p>The caller is presumed to have already handled the cases where it is {@code
+ * //visibility:public} or {@code //visibility:private}. If those labels make it to this method it
+ * will throw.
+ *
+ * <p>{@code //visibility:__pkg__} and {@code //visibility:__subpackages__} are only useful in the
+ * rare case that there exists a literal {@code //visibility} package in the build. It is
+ * disallowed to refer to any package groups declared in {@code //visibility}. This restriction
+ * lets us presume that any label in {@code //visibility} besides these four cases is an
+ * accidental misspelling.
+ */
+ private static void checkForVisibilityMisspelling(Label label) throws EvalException {
if (label.getPackageIdentifier().equals(PUBLIC_LABEL.getPackageIdentifier())
&& PackageSpecification.fromLabel(label) == null) {
- // In other words, if the label is in //visibility and is not //visibility:public,
- // //visibility:private, or (for the unusual case where //visibility exists as a package)
- // //visibility:__pkg__ or //visibility:__subpackages__
+ // Suggest just public/private, as that's way more common than //visibility:__pkg__ or
+ // //visibility:__subpackages__.
throw Starlark.errorf(
"Invalid visibility label '%s'; did you mean //visibility:public or"
+ " //visibility:private?",
label);
}
- return label;
}
/**
@@ -179,7 +192,7 @@
} else if (label.equals(PRIVATE_LABEL)) {
numPrivateLabels++;
} else {
- validate(label);
+ checkForVisibilityMisspelling(label);
}
}
if (hasPublicLabel) {
@@ -202,56 +215,51 @@
}
/**
- * Returns a {@code RuleVisibility} representing the logical result of concatenating the given
- * {@code visibility} with the additional {@code element}.
+ * Returns a {@code RuleVisibility} representing the logical result of concatenating this
+ * visibility's label list with a singleton list containing the given package.
*
- * <p>If {@code element} or {@code visibility} is public, the result is public. If {@code element}
- * or {@code visibility} is private, the result is {@code visibility} or a visibility consisting
- * solely of {@code element}, respectively.
- *
- * <p>If {@code element} is already present in {@code visibility}, the result is just {@code
- * visibility}.
- *
- * @throws EvalException if there's a problem parsing {@code element} into a visibility
+ * <p>Public and private visibilities are normalized as in {@link #validateAndSimplify}. In
+ * addition, the new item is not concatenated if it is already present as an item in this
+ * visibility's list.
*/
- static RuleVisibility concatWithElement(RuleVisibility visibility, Label element)
- throws EvalException {
- if (visibility.equals(RuleVisibility.PRIVATE)) {
+ RuleVisibility concatWithPackage(PackageIdentifier packageIdentifier) {
+ Label pkgItem = Label.createUnvalidated(packageIdentifier, "__pkg__");
+
+ if (this.equals(RuleVisibility.PRIVATE)) {
// Left-side private is dropped.
- return parse(ImmutableList.of(element));
- } else if (element.equals(PRIVATE_LABEL)) {
- // Right-side private is dropped.
- return visibility;
- } else if (visibility.equals(RuleVisibility.PUBLIC) || element.equals(PUBLIC_LABEL)) {
+ return parseUnchecked(ImmutableList.of(pkgItem));
+ } else if (this.equals(RuleVisibility.PUBLIC)) {
// Public is idempotent.
return RuleVisibility.PUBLIC;
} else {
- List<Label> items = visibility.getDeclaredLabels();
- if (items.contains(element)) {
- return visibility;
+ List<Label> items = getDeclaredLabels();
+ if (items.contains(pkgItem)) {
+ return this;
} else {
ImmutableList.Builder<Label> newItems = new ImmutableList.Builder<>();
newItems.addAll(items);
- newItems.add(validate(element));
+ newItems.add(pkgItem);
return parseUnchecked(newItems.build());
}
}
}
- /**
- * Convenience wrapper for {@link #concatWithElement} where the added element is the given
- * package.
- *
- * <p>Unlike that method, this does not throw EvalException.
- */
- static RuleVisibility concatWithPackage(
- RuleVisibility visibility, PackageIdentifier packageIdentifier) {
- Label pkgItem = Label.createUnvalidated(packageIdentifier, "__pkg__");
- try {
- return concatWithElement(visibility, pkgItem);
- } catch (EvalException ex) {
- throw new AssertionError(
- String.format("Unreachable; couldn't parse %s as visibility", pkgItem), ex);
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ } else if (!(other instanceof RuleVisibility)) {
+ return false;
+ } else {
+ // PackageGroupsRuleVisibility is not allowed to contain the special public/private items, so
+ // we don't have to worry about that overlapping with our singleton PUBLIC/PRIVATE instances
+ // here.
+ return getDeclaredLabels().equals(((RuleVisibility) other).getDeclaredLabels());
}
}
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(getClass(), getDeclaredLabels());
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
index 5302e2e..a4ac2b0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
@@ -556,7 +556,7 @@
visibilityO, "'exports_files' operand", pkgBuilder.getLabelConverter()));
MacroInstance currentMacro = pkgBuilder.currentMacro();
if (currentMacro != null) {
- visibility = currentMacro.concatDefinitionLocationToVisibility(visibility);
+ visibility = visibility.concatWithPackage(currentMacro.getDefinitionPackage());
}
// TODO(bazel-team): is licenses plural or singular?
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
index b197f8e..13d69ae 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
@@ -15,11 +15,11 @@
package com.google.devtools.build.lib.packages;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.packages.RuleVisibility.concatWithElement;
import static org.junit.Assert.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import net.starlark.java.eval.EvalException;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -33,6 +33,10 @@
return Label.parseCanonicalUnchecked(labelString);
}
+ private static PackageIdentifier pkg(String labelString) {
+ return label(labelString).getPackageIdentifier();
+ }
+
private static RuleVisibility ruleVisibility(String... labelStrings) {
ImmutableList.Builder<Label> labels = ImmutableList.builder();
for (String labelString : labelStrings) {
@@ -41,15 +45,9 @@
return RuleVisibility.parseUnchecked(labels.build());
}
- // Needed because RuleVisibility has no equals() override.
- private static void assertEqual(RuleVisibility vis1, RuleVisibility vis2) {
- assertThat(vis1.getDependencyLabels()).isEqualTo(vis2.getDependencyLabels());
- assertThat(vis1.getDeclaredLabels()).isEqualTo(vis2.getDeclaredLabels());
- }
-
private static final String A = "//a:__pkg__";
// Package group labels are represented differently than __pkg__ labels, so cover both cases.
- private static final String B = "//b:pkggroup";
+ private static final String B_PG = "//b:pkggroup";
private static final String C = "//c:__pkg__";
private static final String PUBLIC = "//visibility:public";
private static final String PRIVATE = "//visibility:private";
@@ -87,7 +85,7 @@
public void validateAndSimplify_simplifiesPublic() throws Exception {
assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(A), label(PUBLIC))))
.containsExactly(label(PUBLIC));
- assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(B))))
+ assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(B_PG))))
.containsExactly(label(PUBLIC));
assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(PRIVATE))))
.containsExactly(label(PUBLIC));
@@ -99,8 +97,8 @@
public void validateAndSimplify_simplifiesPrivate() throws Exception {
assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(A), label(PRIVATE))))
.containsExactly(label(A));
- assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PRIVATE), label(B))))
- .containsExactly(label(B));
+ assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PRIVATE), label(B_PG))))
+ .containsExactly(label(B_PG));
assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PRIVATE), label(PRIVATE))))
.containsExactly(label(PRIVATE));
}
@@ -160,23 +158,19 @@
@Test
public void concatenation() throws Exception {
- assertEqual(concatWithElement(ruleVisibility(A, B), label(C)), ruleVisibility(A, B, C));
- assertEqual(concatWithElement(ruleVisibility(A, B), label(PUBLIC)), PUBLIC_VIS);
- assertEqual(concatWithElement(ruleVisibility(A, B), label(PRIVATE)), ruleVisibility(A, B));
+ assertThat(ruleVisibility(A, B_PG).concatWithPackage(pkg(C)))
+ .isEqualTo(ruleVisibility(A, B_PG, C));
- assertEqual(concatWithElement(PUBLIC_VIS, label(C)), PUBLIC_VIS);
- assertEqual(concatWithElement(PUBLIC_VIS, label(PUBLIC)), PUBLIC_VIS);
- assertEqual(concatWithElement(PUBLIC_VIS, label(PRIVATE)), PUBLIC_VIS);
+ assertThat(PUBLIC_VIS.concatWithPackage(pkg(C))).isEqualTo(PUBLIC_VIS);
- assertEqual(concatWithElement(PRIVATE_VIS, label(C)), ruleVisibility(C));
- assertEqual(concatWithElement(PRIVATE_VIS, label(PUBLIC)), PUBLIC_VIS);
- assertEqual(concatWithElement(PRIVATE_VIS, label(PRIVATE)), PRIVATE_VIS);
+ assertThat(PRIVATE_VIS.concatWithPackage(pkg(C))).isEqualTo(ruleVisibility(C));
// Duplicates are not added, though they are preserved.
- assertEqual(concatWithElement(ruleVisibility(A, B), label(A)), ruleVisibility(A, B));
- assertEqual(
- concatWithElement(ruleVisibility(A, B, B, A), label(A)), ruleVisibility(A, B, B, A));
- assertEqual(
- concatWithElement(ruleVisibility(A, B, B, A), label(C)), ruleVisibility(A, B, B, A, C));
+ assertThat(ruleVisibility(A, B_PG).concatWithPackage(pkg(A)))
+ .isEqualTo(ruleVisibility(A, B_PG));
+ assertThat(ruleVisibility(A, B_PG, B_PG, A).concatWithPackage(pkg(A)))
+ .isEqualTo(ruleVisibility(A, B_PG, B_PG, A));
+ assertThat(ruleVisibility(A, B_PG, B_PG, A).concatWithPackage(pkg(C)))
+ .isEqualTo(ruleVisibility(A, B_PG, B_PG, A, C));
}
}
