For symbolic macros, implicitly append the current location to target's visibility attr The design of Macro-Aware Visibility says that a target is visible to the locations given in its visibility attribute, plus the location where the target is declared (its BUILD file, or the package of the .bzl where the current symbolic macro was exported). This CL automatically concatenates the declaration location into the materialized visibility attribute of targets that are declared inside symbolic macros. Since we're munging the visibility attribute, this is a loading-time effect, and the result can be seen in `bazel query` output (or in our unit tests, with Target#getVisibility). When we add a visibility attribute to symbolic macros, they will have the same behavior, with the result that the visibility argument seen in macro implementation functions will include the caller's location. Targets that are not declared within a symbolic macro are unaffected, and will continue to show visibility attribute values that do not necessarily include their package explicitly. A subsequent change will work around this behavioral difference between targets in macros and out of macros, by adding the declaration location for the latter at analysis time, in the visibility provider. We went with this hybrid approach in order to avoid a massive and immediate change to all targets' visibilities. It's not clear whether we need to change this in the future for consistency. Changes: - Add a map in Package, from target name to its innermost declaring macro. Add a couple TODOs for future optimization. Add a corresponding utility function Target#getDeclaringMacro. - Add Package#visibilityWithCurrentMacro(), a utility function for munging a visibility attribute value to append the location of the currently executing symbolic macro's definition. Apply this to rule declarations in RuleFactory, and also to exports_files in StarlarkNativeModule. - Clean up RuleFactory#generatorAttributesForMacros by moving attribute map reconstruction into the caller where it can be handled alongside visibility. Clarify comments. - Add TODO comment about reserved macro attr names. - Add visibility attr tests to PackageFactoryTest. Also rename a helper method and add another helper method for unrelated symbolic macro tests. Work toward #19922. PiperOrigin-RevId: 665419955 Change-Id: I98ae1acdfbed3662ae00624d663dac1d35b9a8ec
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 3acf9d0..75b2973 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
@@ -55,6 +55,10 @@ * <p>Of these, {@code name} is special cased as an actual attribute, and the rest do not exist. */ // Keep in sync with `macro()`'s `attrs` user documentation in StarlarkRuleFunctionsApi. + // TODO: #19922 - Current thinking is that when/if we allow macros to have built-in attributes or + // inherit the attributes of a wrapped rule class, then we'll allow the attrs dict to shadow any + // inherited attr. In that case we can relax this list to be just "name" and "visibility", which + // are always innately meaningful to a macro and should never be user-defined. public static final ImmutableSet<String> RESERVED_MACRO_ATTR_NAMES = ImmutableSet.of("name", "visibility", "deprecation", "tags", "testonly", "features");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 0ac5aff..f886a15 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -155,6 +155,27 @@ // in {@link #finishInit} when name conflict checking is disabled. @Nullable private ImmutableMap<String, String> macroNamespaceViolatingTargets; + /** + * A map from names of targets declared in a symbolic macro to the (innermost) macro instance + * where they were declared. + */ + // TODO: #19922 - This likely subsumes macroNamespaceViolatingTargets, since we can just map the + // target to its macro and then check whether it is in the macro's namespace. + // + // TODO: #19922 - Don't maintain this extra map of all macro-instantiated targets. We have a + // couple options: + // 1) Have Target store a reference to its declaring MacroInstance directly. To avoid adding a + // field to that class (a not insignificant cost), we can merge it with the reference to its + // package: If we're not in a macro, we point to the package, and if we are, we point to the + // innermost macro, and hop to the MacroInstance to get a reference to the Package (or parent + // macro). + // 2) To support lazy macro evaluation, we'll probably need a prefix trie in Package to find the + // macros whose namespaces contain the requested target name. For targets that respect their + // macro's namespace, we could just look them up in the trie. This assumes we already know + // whether the target is well-named, which we wouldn't if we got rid of + // macroNamespaceViolatingTargets. + private ImmutableMap<String, MacroInstance> targetsToDeclaringMacros; + public PackageArgs getPackageArgs() { return metadata.packageArgs; } @@ -367,6 +388,7 @@ builder.macroNamespaceViolatingTargets != null ? ImmutableMap.copyOf(builder.macroNamespaceViolatingTargets) : ImmutableMap.of(); + this.targetsToDeclaringMacros = ImmutableSortedMap.copyOf(builder.targetsToDeclaringMacros); this.failureDetail = builder.getFailureDetail(); this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms); this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains); @@ -718,6 +740,15 @@ } /** + * Returns the (innermost) symbolic macro instance that declared the given target, or null if the + * target was not created in a symbolic macro or no such target by the given name exists. + */ + @Nullable + public MacroInstance getDeclaringMacroForTarget(String target) { + return targetsToDeclaringMacros.get(target); + } + + /** * How to enforce visibility on <code>config_setting</code> See {@link * ConfigSettingVisibilityPolicy} for details. */ @@ -1000,6 +1031,8 @@ // // We use SnapshottableBiMap to help track insertion order of Rule targets, for use by // native.existing_rules(). + // + // Use addOrReplaceTarget() to add new entries. private BiMap<String, Target> targets = new SnapshottableBiMap<>(target -> target instanceof Rule); @@ -1106,6 +1139,12 @@ @Nullable private Map<String, String> macroNamespaceViolatingTargets = new HashMap<>(); /** + * A map from target name to the (innermost) macro instance that declared it. See {@link + * Package#targetsToDeclaringMacros}. + */ + private LinkedHashMap<String, MacroInstance> targetsToDeclaringMacros = new LinkedHashMap<>(); + + /** * The collection of the prefixes of every output file. Maps each prefix to an arbitrary output * file having that prefix. Used for error reporting. * @@ -1665,7 +1704,11 @@ @CanIgnoreReturnValue @Nullable private Target addOrReplaceTarget(Target target) { - return targets.put(target.getName(), target); + Target existing = targets.put(target.getName(), target); + if (currentMacroFrame != null) { + targetsToDeclaringMacros.put(target.getName(), currentMacroFrame.macroInstance); + } + return existing; } @Nullable @@ -2032,6 +2075,36 @@ return prev; } + /** + * If we are currently executing a symbolic macro, returns the result of unioning the given + * visibility with the location of the innermost macro's code. Otherwise, returns the given + * visibility unmodified. + * + * <p>The location of the macro's code is considered to be the package containing the .bzl file + * from which the macro's {@code MacroClass} was exported. + */ + RuleVisibility copyAppendingCurrentMacroLocation(RuleVisibility visibility) { + if (currentMacroFrame == null) { + return visibility; + } + MacroClass macroClass = currentMacroFrame.macroInstance.getMacroClass(); + PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier(); + Label newVisibilityItem = Label.createUnvalidated(macroLocation, "__pkg__"); + + if (visibility.equals(RuleVisibility.PRIVATE)) { + // Private is dropped. + return PackageGroupsRuleVisibility.create(ImmutableList.of(newVisibilityItem)); + } else if (visibility.equals(RuleVisibility.PUBLIC)) { + // Public is idempotent. + return visibility; + } else { + ImmutableList.Builder<Label> items = new ImmutableList.Builder<>(); + items.addAll(visibility.getDeclaredLabels()); + items.add(newVisibilityItem); + return PackageGroupsRuleVisibility.create(items.build()); + } + } + void addRegisteredExecutionPlatforms(List<TargetPattern> platforms) { this.registeredExecutionPlatforms.addAll(platforms); } @@ -2144,9 +2217,11 @@ macroConflictsFound |= nameIsWithinMacroNamespace(name, macro.getName()); } if (!macroConflictsFound) { - Location loc = rule.getLocation(); + Location loc = rule.getLocation(); newInputFiles.put( name, + // Targets added this way are not in any macro, so + // copyAppendingCurrentMacroLocation() munging isn't applicable. noImplicitFileExport ? new PrivateVisibilityInputFile(pkg, label, loc) : new InputFile(pkg, label, loc));
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 3a0f8d1..05b4707 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
@@ -24,8 +24,10 @@ import com.google.devtools.build.lib.packages.TargetDefinitionContext.NameConflictException; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.List; import java.util.Map; import java.util.Set; +import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.NoneType; @@ -83,8 +85,26 @@ ruleClass + " cannot be in the WORKSPACE file " + "(used by " + label + ")"); } - BuildLangTypedAttributeValuesMap attributes = - generatorAttributesForMacros(pkgBuilder, attributeValues, callstack); + // Add the generator_name attribute, and possibly append the declaration location to the + // visibility attribute. + BuildLangTypedAttributeValuesMap processedAttributes; + @Nullable String generatorName = getGeneratorName(pkgBuilder, attributeValues, callstack); + @Nullable List<Label> modifiedVisibility = getModifiedVisibility(pkgBuilder, attributeValues); + // Don't bother copying anything if nothing changed. + if (generatorName != null || modifiedVisibility != null) { + ImmutableMap.Builder<String, Object> builder = + ImmutableMap.builderWithExpectedSize(attributeValues.attributeValues.size() + 2); + builder.putAll(attributeValues.attributeValues); + if (generatorName != null) { + builder.put("generator_name", generatorName); + } + if (modifiedVisibility != null) { + builder.put("visibility", modifiedVisibility); + } + processedAttributes = new BuildLangTypedAttributeValuesMap(builder.buildKeepingLast()); + } else { + processedAttributes = attributeValues; + } // The raw stack is of the form [<toplevel>@BUILD:1, macro@lib.bzl:1, cc_library@<builtin>]. // Pop the innermost frame for the rule, since it's obvious. @@ -92,7 +112,7 @@ try { return ruleClass.createRule( - pkgBuilder, label, attributes, failOnUnknownAttributes, callstack); + pkgBuilder, label, processedAttributes, failOnUnknownAttributes, callstack); } catch (LabelSyntaxException | CannotPrecomputeDefaultsException e) { throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage()); } @@ -207,45 +227,91 @@ } /** - * If the rule was created by a macro, sets the appropriate value for the generator_name attribute - * and returns all attributes. + * Given the call stack and attribute values of a rule being instantiated, computes and returns + * the value of the special {@code generator_name} attribute to be added, or returns null if it + * shouldn't be added. * - * <p>Otherwise, it returns the given attributes without any changes. + * <p>The {@code generator_name} attribute is set for targets instantiated within a legacy macro + * (and which are not also within a symbolic macro). Its value is the name argument of the + * top-level macro on the call stack, if its value can be determined statically (see {@link + * PackageFactory#checkBuildSyntax}), or just the name of the target otherwise. */ - private static BuildLangTypedAttributeValuesMap generatorAttributesForMacros( + // TODO: #19922 - Should we set generator_name on targets created by a symbolic macro instantiated + // within a legacy macro? Otherwise tooling may think those targets were not created in a macro. + @Nullable + private static String getGeneratorName( Package.Builder pkgBuilder, BuildLangTypedAttributeValuesMap args, ImmutableList<CallStackEntry> stack) { - // The "generator" of a rule is the function (sometimes called "macro") outermost in the call - // stack. For rules with generators, the stack must contain at least two entries: - // 0: the outermost function (e.g. a BUILD file), - // 1: the function called by it (e.g. a "macro" in a .bzl file). + // The "generator" of a rule is the function outermost in the call stack (regardless of whether + // or not it was passed a "name" parameter). For rules with generators, the stack must contain + // at least two entries: + // 0: the outermost function (e.g. a BUILD file), + // 1: the function called by it (e.g. a "macro" in a .bzl file). // optionally followed by other Starlark or built-in functions, and finally the rule // instantiation function. if (stack.size() < 2 || !stack.get(1).location.file().endsWith(".bzl")) { - // Not instantiated by a Starlark macro. - // (Edge case not handled: BUILD file calls helper(cc_library) defined in an .scl file, and - // helper instantiates the rule that's passed as an argument.) - return args; + // Not instantiated by a legacy macro. + // TODO: #19922 - This stack inspection logic doesn't work for symbolic macros, where it will + // likely incorrectly discriminate between targets created in the implementation function + // directly and targets created in a helper function called from the implementation function. + // TODO(bazel-team): Tolerate ".scl" extension in the above if? An .scl file can instantiate a + // rule if the rule function is passed as an argument. + return null; } if (args.containsAttributeNamed("generator_name")) { - // generator_name is explicitly set. Return early to avoid a map key conflict. + // generator_name is explicitly set. Don't override it. // TODO(b/274802222): Should this be prohibited? - return args; + return null; } - ImmutableMap.Builder<String, Object> builder = - ImmutableMap.builderWithExpectedSize(args.attributeValues.size() + 1); - builder.putAll(args.attributeValues); - String generatorName = pkgBuilder.getGeneratorNameByLocation(stack.get(0).location); if (generatorName == null) { + // Fall back on target name (meh). generatorName = (String) args.getAttributeValue("name"); } - builder.put("generator_name", generatorName); + return generatorName; + } - return new BuildLangTypedAttributeValuesMap(builder.buildOrThrow()); + /** + * Given the attribute values of the rule being instantiated, computes and returns the new value + * for its visibility attribute, or null if no change is needed. + * + * <p>For targets created inside one or more symbolic macros, the new visibility value is whatever + * the original visibility attribute was (possibly the package's default visibility), unioned with + * the package where the innermost currently executing symbolic macro was exported. + * + * <p>For targets not created inside one or more symbolic macros, no change is made to the + * visibility attribute at this time, but during analysis the target's package will be added to + * its visibility provider. + */ + @Nullable + private static List<Label> getModifiedVisibility( + Package.Builder pkgBuilder, BuildLangTypedAttributeValuesMap args) { + if (pkgBuilder.getCurrentMacroFrame() == null) { + return null; + } + + RuleVisibility visibility = null; + Object uncheckedVisibilityAttr = args.getAttributeValue("visibility"); + if (uncheckedVisibilityAttr == null) { + // TODO: #19922 - Don't use default_visibility, we're in a symbolic macro. + visibility = pkgBuilder.getPartialPackageArgs().defaultVisibility(); + } else { + try { + List<Label> visibilityAttr = + BuildType.LABEL_LIST.convert( + uncheckedVisibilityAttr, "visibility attribute", pkgBuilder.getLabelConverter()); + visibility = RuleVisibility.parse(visibilityAttr); + } catch (EvalException ex) { + // Can't modify the visibility attribute because it's invalid. Let it be caught in + // RuleClass#populateDefinedRuleAttributeValues. + return null; + } + } + + return pkgBuilder.copyAppendingCurrentMacroLocation(visibility).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 5d8f064..d22e074 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
@@ -553,6 +553,7 @@ : RuleVisibility.parse( BuildType.LABEL_LIST.convert( visibilityO, "'exports_files' operand", pkgBuilder.getLabelConverter())); + visibility = pkgBuilder.copyAppendingCurrentMacroLocation(visibility); // TODO(bazel-team): is licenses plural or singular? License license = BuildType.LICENSE.convertOptional(licensesO, "'exports_files' operand"); @@ -565,6 +566,11 @@ } try { InputFile inputFile = pkgBuilder.createInputFile(file, loc); + // TODO: #19922 - The use of identity inequality in this visibility check seems suspect, + // since the same logical visibility may have multiple RuleVisibility instances. But it's + // unclear why we want to support idempotent exports_files() with the same logical + // visibility at all. With Macro-Aware Visibility, it becomes possible for two identical + // visibility lines to declare different actual visibility values depending on context. if (inputFile.isVisibilitySpecified() && inputFile.getVisibility() != visibility) { throw Starlark.errorf( "visibility for exported file '%s' declared twice", inputFile.getName());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Target.java b/src/main/java/com/google/devtools/build/lib/packages/Target.java index b086eb5..a8a9954 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Target.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Target.java
@@ -33,10 +33,22 @@ Package getPackage(); /** + * Returns the innermost symbolic macro that declared this target, or null if it was declared + * outside any symbolic macro (i.e. directly in a BUILD file or only in one or more legacy + * macros). + */ + default MacroInstance getDeclaringMacro() { + // TODO: #19922 - We might replace Package#getDeclaringMacroForTarget by storing a reference to + // the declaring macro in implementations of this interface (sharing memory with the field for + // the package). + return getPackage().getDeclaringMacroForTarget(getName()); + } + + /** * Returns the rule associated with this target, if any. * - * If this is a Rule, returns itself; it this is an OutputFile, returns its - * generating rule; if this is an input file, returns null. + * <p>If this is a Rule, returns itself; it this is an OutputFile, returns its generating rule; if + * this is an input file, returns null. */ @Nullable Rule getAssociatedRule();
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java index 0fdd3f6..eaa286b 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1174,8 +1174,10 @@ /** * Defines a symbolic macro "my_macro" in //pkg:my_macro.bzl, and enables the experimental flag. + * + * <p>The macro does not define any targets. */ - private void defineMacroBzl() throws Exception { + private void defineEmptyMacroBzl() throws Exception { setBuildLanguageOptions("--experimental_enable_first_class_macros"); scratch.file( "pkg/my_macro.bzl", @@ -1190,7 +1192,7 @@ public void testSymbolicMacro_duplicateMacroNamesDisallowed() throws Exception { // However, note that duplicates are allowed if one is a submacro of the other. // See SymbolicMacroTest#submacroMayHaveSameNameAsAncestorMacros for coverage of that. - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1202,7 +1204,7 @@ @Test public void testSymbolicMacro_macroAndRuleClash_macroDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "target 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1214,7 +1216,7 @@ @Test public void testSymbolicMacro_macroAndRuleClash_ruleDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing target", """ @@ -1226,7 +1228,7 @@ @Test public void testSymbolicMacro_macroAndOutputClash_macroDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "target 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1238,7 +1240,7 @@ @Test public void testSymbolicMacro_macroAndOutputClash_outputDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing target", """ @@ -1255,7 +1257,7 @@ // restrictive in the future, and to the extent we restrict collisions between macro names and // target names (i.e., exclusive prefixes), we should also ensure output prefixes can't collide // with macros. - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalSuccess( """ load(":my_macro.bzl", "my_macro") @@ -1267,7 +1269,7 @@ @Test public void testSymbolicMacro_macroAndEnvironmentGroupClash_macroDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "target 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1281,7 +1283,7 @@ @Test public void testSymbolicMacro_macroAndEnvironmentGroupClash_environmentGroupDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing target", """ @@ -1294,7 +1296,7 @@ @Test public void testSymbolicMacro_macroAndPackageGroupClash_macroDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "target 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1307,7 +1309,7 @@ @Test public void testSymbolicMacro_macroAndPackageGroupClash_packageGroupDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing target", """ @@ -1319,7 +1321,7 @@ @Test public void testSymbolicMacro_macroAndInputClash_macroDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "target 'foo' conflicts with an existing macro (and was not created by it)", """ @@ -1331,7 +1333,7 @@ @Test public void testSymbolicMacro_macroAndInputClash_inputDeclaredFirst() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); expectEvalError( "macro 'foo' conflicts with an existing target", """ @@ -1344,7 +1346,7 @@ @Test public void testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespaces() throws Exception { - defineMacroBzl(); + defineEmptyMacroBzl(); scratch.file( "pkg/BUILD", """ @@ -1357,7 +1359,7 @@ """); // You can't implicitly make an input file with a name that foo could've defined. (You can still // have an explicit exports_files() do it.) - Package pkg = loadPackage("pkg"); + Package pkg = loadPackageAndAssertSuccess("pkg"); assertThat(pkg.getTargets()).doesNotContainKey("foo"); assertThat(pkg.getTargets()).doesNotContainKey("foo_bar"); assertThat(pkg.getTarget("foo_")).isInstanceOf(InputFile.class); @@ -1389,7 +1391,7 @@ ) """); - Package pkg = loadPackage("pkg"); + Package pkg = loadPackageAndAssertSuccess("pkg"); assertThat(pkg.getTargets()).containsKey("src_A.txt"); assertThat(pkg.getTargets()).doesNotContainKey("src_B.txt"); } @@ -1419,7 +1421,7 @@ outer_macro(name = "abc") """); - Package pkg = loadPackage("pkg"); + Package pkg = loadPackageAndAssertSuccess("pkg"); assertThat(pkg.getTargets()).containsKey("abc"); assertThat(pkg.getMacrosById().keySet()).containsExactly("abc:1", "abc:2", "abc:3"); } @@ -1539,6 +1541,233 @@ // TODO: #19922 - Add tests for graceful failure when the macro stack is too deep or there are too // many macros overall, for both eager and deferred evaluation. + /** + * Asserts that the target's RuleVisibility contains exactly the given labels. + * + * <p>For rule targets, this is effectively its visibility attribute, as possibly modified by the + * munging to include the declaration location. + */ + private void assertVisibilityIs(Target target, String... visibilityLabels) { + ImmutableList.Builder<Label> labels = ImmutableList.builder(); + for (String item : visibilityLabels) { + labels.add(Label.parseCanonicalUnchecked(item)); + } + assertThat(target.getVisibility().getDeclaredLabels()) + // Values are sorted by virtue of visibility being a label_list. + .containsExactlyElementsIn(labels.build()); + } + + private void enableMacrosAndUsePrivateVisibility() throws Exception { + setBuildLanguageOptions("--experimental_enable_first_class_macros"); + // BuildViewTestCase makes everything public by default. + setPackageOptions("--default_visibility=private"); + } + + @Test + public void testDeclarationVisibilityUnioning_onlyOccursWithinMacros() throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.cc_library( + name = name, + visibility = ["//other_pkg:__pkg__"], + ) + my_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//lib:macro.bzl", "my_macro") + + cc_library( + name = "foo", + visibility = ["//other_pkg:__pkg__"], + ) + my_macro(name = "bar") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo"), "//other_pkg:__pkg__"); + assertVisibilityIs(pkg.getTarget("bar"), "//other_pkg:__pkg__", "//lib:__pkg__"); + } + + @Test + public void testDeclarationVisibilityUnioning_usesInnermostMacroLocation() throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("inner/BUILD"); + scratch.file( + "inner/macro.bzl", + """ + def _impl(name): + native.cc_library( + name = name, + visibility = ["//other_pkg:__pkg__"], + ) + inner_macro = macro(implementation = _impl) + """); + scratch.file("outer/BUILD"); + scratch.file( + "outer/macro.bzl", + """ + load("//inner:macro.bzl", "inner_macro") + def _impl(name): + inner_macro(name = name) + outer_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//outer:macro.bzl", "outer_macro") + + outer_macro(name = "foo") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo"), "//other_pkg:__pkg__", "//inner:__pkg__"); + } + + // TODO: #19922 - Invert this behavior, default visibility should not propagate to within a macro. + @Test + public void testDeclarationVisibilityUnioning_respectsPackageDefaultVisibility() + throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.cc_library(name = name) + my_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//lib:macro.bzl", "my_macro") + package(default_visibility = ["//other_pkg:__pkg__"]) + + cc_library(name = "foo") + my_macro(name = "bar") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo"), "//other_pkg:__pkg__"); + assertVisibilityIs(pkg.getTarget("bar"), "//other_pkg:__pkg__", "//lib:__pkg__"); + } + + @Test + public void testDeclarationVisibilityUnioning_worksWithPublicPrivateAndDuplicateVisibilities() + throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.cc_library( + name = name + "_public", + visibility = ["//visibility:public"], + ) + native.cc_library( + name = name + "_private", + visibility = ["//visibility:private"], + ) + native.cc_library( + name = name + "_selfvisible", + visibility = ["//lib:__pkg__"], + ) + my_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//lib:macro.bzl", "my_macro") + + my_macro(name = "foo") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo_public"), "//visibility:public"); + assertVisibilityIs(pkg.getTarget("foo_private"), "//lib:__pkg__"); + // Visibility is a label_list. Label lists don't do duplicate elimination. (Nor can we eliminate + // all logical redundancies anyway, since visibilities may refer to redundant package groups.) + assertVisibilityIs(pkg.getTarget("foo_selfvisible"), "//lib:__pkg__", "//lib:__pkg__"); + } + + @Test + public void testDeclarationVisibilityUnioning_appliesToExportsFiles() throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.exports_files([name + "_exported"]) + native.exports_files([name + "_internal"], visibility = ["//visibility:private"]) + my_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//lib:macro.bzl", "my_macro") + + my_macro(name = "foo") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo_exported"), "//visibility:public"); + assertVisibilityIs(pkg.getTarget("foo_internal"), "//lib:__pkg__"); + } + + @Test + public void testDeclarationVisibilityUnioning_hasNoEffectOnPackageGroups() throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.package_group(name = name) + my_macro = macro(implementation = _impl) + """); + scratch.file( + "pkg/BUILD", + """ + load("//lib:macro.bzl", "my_macro") + + my_macro(name = "foo") + """); + + Package pkg = loadPackageAndAssertSuccess("pkg"); + assertVisibilityIs(pkg.getTarget("foo"), "//visibility:public"); + } + + @Test + public void testDeclarationVisibilityUnioning_failsGracefullyOnInvalidVisibility() + throws Exception { + enableMacrosAndUsePrivateVisibility(); + scratch.file("lib/BUILD"); + scratch.file( + "lib/macro.bzl", + """ + def _impl(name): + native.cc_library( + name = name, + visibility = ["//visibility:not_a_valid_specifier"], + ) + my_macro = macro(implementation = _impl) + """); + expectEvalError( + "//pkg:foo Invalid visibility label '//visibility:not_a_valid_specifier'", + """ + load("//lib:macro.bzl", "my_macro") + + my_macro(name = "foo") + """); + } + @Test public void testGlobPatternExtractor() throws Exception { StarlarkFile file = @@ -1785,4 +2014,10 @@ private Package loadPackage(String pkgid) throws Exception { return getTarget("//" + pkgid + ":BUILD").getPackage(); } + + private Package loadPackageAndAssertSuccess(String pkgid) throws Exception { + Package pkg = loadPackage(pkgid); + assertThat(pkg.containsErrors()).isFalse(); + return pkg; + } }