From ee3fa9dfd0aaf26a226eb6af7dceb0f55649f54a Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 25 Sep 2024 11:42:05 -0700 Subject: [PATCH] Move macro visibility appending out of Package.Builder `Package.Builder` has a utility method `copyAppendingCurrentMacroLocation`, that doesn't really need to be there. Ahead of further refactoring of this class, I figured I'd spin off this method. (The purpose of the method is to determine the munged visibility values for targets declared within symbolic macros.) The method is re-expressed as two helpers in `RuleVisibility` and one in `MacroInstance`, each of which does a narrower task more suited for its new home. Made the convenience method `Builder#currentMacro` public, since it's much terser than `getCurrentMacroFrame().macroInstance`. Added a drive-by TODO in `StarlarkNativeModule`. Work toward #19922. PiperOrigin-RevId: 678786589 Change-Id: I5cc190bd6ef908a3e67094eacd13e9c91ef43d7c --- .../build/lib/packages/MacroInstance.java | 16 ++++ .../devtools/build/lib/packages/Package.java | 32 +------ .../build/lib/packages/RuleFactory.java | 5 +- .../build/lib/packages/RuleVisibility.java | 48 ++++++++++- .../lib/packages/StarlarkNativeModule.java | 5 +- .../lib/packages/RuleVisibilityTest.java | 86 +++++++++++++++++++ 6 files changed, 157 insertions(+), 35 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java 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 9092f0ab86e2c6..fa47339bfa444e 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 @@ -149,6 +149,22 @@ public ImmutableMap getAttrValues() { return attrValues; } + /** + * Returns a {@code RuleVisibility} representing the result of concatenating this macro's {@link + * MacroClass}'s definition location to the given {@code visibility}. + * + *

The definition location of a macro class is the package containing the .bzl file from which + * the macro class was exported. + * + *

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). + */ + public RuleVisibility concatDefinitionLocationToVisibility(RuleVisibility visibility) { + PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier(); + return RuleVisibility.concatWithPackage(visibility, macroLocation); + } + /** * Visits all labels appearing in non-implicit attributes of {@link Type.LabelClass#DEPENDENCY} * label type, i.e. ignoring nodep labels. 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 1b8efc197751ea..1955df7b67ed0b 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 @@ -1635,7 +1635,7 @@ public Globber getGlobber() { * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro. */ @Nullable - private MacroInstance currentMacro() { + public MacroInstance currentMacro() { MacroFrame frame = regEnv.getCurrentMacroFrame(); return frame == null ? null : frame.macroInstance; } @@ -1961,36 +1961,6 @@ public void markMacroComplete(MacroInstance macro) { } } - /** - * 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. - * - *

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 (currentMacro() == null) { - return visibility; - } - MacroClass macroClass = currentMacro().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

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. + * + * @throws EvalException if there's a problem parsing {@code element} into a visibility + */ + static RuleVisibility concatWithElement(RuleVisibility visibility, Label element) + throws EvalException { + if (visibility.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)) { + // Public is idempotent. + return RuleVisibility.PUBLIC; + } else { + ImmutableList.Builder

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); + } + } +} 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 4cc70c65a806d0..3f296c821699c9 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,7 +553,10 @@ public NoneType exportsFiles( : RuleVisibility.parse( BuildType.LABEL_LIST.convert( visibilityO, "'exports_files' operand", pkgBuilder.getLabelConverter())); - visibility = pkgBuilder.copyAppendingCurrentMacroLocation(visibility); + MacroInstance currentMacro = pkgBuilder.currentMacro(); + if (currentMacro != null) { + visibility = currentMacro.concatDefinitionLocationToVisibility(visibility); + } // TODO(bazel-team): is licenses plural or singular? License license = BuildType.LICENSE.convertOptional(licensesO, "'exports_files' operand"); 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 new file mode 100644 index 00000000000000..49db8e0f098fb4 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java @@ -0,0 +1,86 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.cmdline.Label; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link RuleVisibility}. */ +@RunWith(JUnit4.class) +public final class RuleVisibilityTest { + + private static Label label(String labelString) { + return Label.parseCanonicalUnchecked(labelString); + } + + private static RuleVisibility ruleVisibility(String... labelStrings) { + ImmutableList.Builder