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 1c96271190a256..d61ce28086af41 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 @@ -23,8 +23,8 @@ import com.google.common.collect.Lists; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.packages.Package.Builder.MacroFrame; import com.google.devtools.build.lib.packages.TargetDefinitionContext.NameConflictException; +import com.google.devtools.build.lib.packages.TargetRegistrationEnvironment.MacroFrame; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; 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 434712ed2fe38a..473153c00c1544 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 @@ -18,7 +18,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.collect.BiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -69,7 +68,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -150,11 +148,6 @@ public enum ConfigSettingVisibilityPolicy { /** Sentinel value for package overhead being empty. */ private static final long PACKAGE_OVERHEAD_UNSET = -1; - /** Used for constructing macro namespace violation error messages. */ - private static final String MACRO_NAMING_RULES = - "Name must be the same as the macro's name, or the macro's name followed by '_'" - + " (recommended), '-', or '.', and a non-empty string."; - // ==== General package metadata fields ==== private final Metadata metadata; @@ -550,26 +543,12 @@ public ImmutableSortedMap getTargets() { return targets; } - /** Common getTargets implementation, accessible by {@link Package.Builder}. */ - private static Set getTargets(BiMap targetMap) { - return targetMap.values(); - } - /** * Returns a (read-only, ordered) iterable of all the targets belonging to this package which are * instances of the specified class. */ public Iterable getTargets(Class targetClass) { - return getTargets(targets, targetClass); - } - - /** - * Common getTargets implementation, accessible by both {@link Package} and {@link - * Package.Builder}. - */ - private static Iterable getTargets( - Map targetMap, Class targetClass) { - return Iterables.filter(targetMap.values(), targetClass); + return Iterables.filter(targets.values(), targetClass); } /** @@ -606,7 +585,9 @@ public void checkMacroNamespaceCompliance(Target target) throws MacroNamespaceVi String.format( "Target %s declared in symbolic macro '%s' violates macro naming rules and cannot be" + " built. %s", - target.getLabel(), macroNamespaceViolated, MACRO_NAMING_RULES)); + target.getLabel(), + macroNamespaceViolated, + TargetRegistrationEnvironment.MACRO_NAMING_RULES)); } } @@ -1008,7 +989,7 @@ public static Builder newExternalPackageBuilderForBzlmod( * A builder for {@link Package} objects. Only intended to be used by {@link PackageFactory} and * {@link com.google.devtools.build.lib.skyframe.PackageFunction}. */ - public static class Builder extends TargetDefinitionContext { + public static class Builder extends TargetRegistrationEnvironment { /** * A bundle of options affecting package construction, that is not specific to any particular @@ -1097,7 +1078,7 @@ default boolean precomputeTransitiveLoads() { @Nullable private String ioExceptionMessage = null; @Nullable private IOException ioException = null; @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null; - private boolean containsErrors = false; + // A package's FailureDetail field derives from the events on its Builder's event handler. // During package deserialization, those events are unavailable, because those events aren't // serialized [*]. Its FailureDetail value is serialized, however. During deserialization, that @@ -1116,15 +1097,6 @@ default boolean precomputeTransitiveLoads() { private final Map environmentGroups = new HashMap<>(); - // All targets added to the package. - // - // 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 targets = - new SnapshottableBiMap<>(target -> target instanceof Rule); - // The snapshot of {@link #targets} for use in rule finalizer macros. Contains all // non-finalizer-instantiated rule targets (i.e. all rule targets except for those instantiated // in a finalizer or in a macro called from a finalizer). @@ -1132,10 +1104,6 @@ default boolean precomputeTransitiveLoads() { // Initialized by expandAllRemainingMacros() and reset to null by beforeBuild(). @Nullable private Map rulesSnapshotViewForFinalizers; - // All instances of symbolic macros created during package construction, indexed by id (not - // name). - private final Map macros = new LinkedHashMap<>(); - /** * Ids of all symbolic macros that have been declared but not yet evaluated. * @@ -1148,109 +1116,6 @@ default boolean precomputeTransitiveLoads() { */ private final Set unexpandedMacros = new LinkedHashSet<>(); - /** - * Represents the innermost currently executing symbolic macro, or null if none are running. - * - *

Logically, this is the top entry of a stack of frames where each frame corresponds to a - * nested symbolic macro invocation. In actuality, symbolic macros do not necessarily run - * eagerly when they are invoked, so this is not really a call stack per se. We leave it to the - * pkgbuilder client to set the current frame, so that the choice of whether to push and pop, or - * process a worklist of queued evaluations, is up to them. - * - *

The state of this field is used to determine what Starlark APIs are available (see user - * documentation on {@code macro()} at {@link StarlarkRuleFunctionsApi#macro}), and to help - * enforce naming requirements on targets and macros. - */ - private MacroFrame currentMacroFrame = null; - - /** - * Represents the state of a running symbolic macro (see {@link #currentMacroFrame}). - * Semi-opaque. - */ - static class MacroFrame { - final MacroInstance macroInstance; - // Most name conflicts are caught by checking the keys of the `targets` and `macros` maps. - // It is not a conflict for a target or macro to have the same name as the macro it is - // declared in, yet such a target or macro may still conflict with siblings in the same macro. - // We use this bool to track whether or not a newly introduced macro, M, having the same name - // as its parent (the current macro), would clash with an already defined sibling of M. - private boolean mainSubmacroHasBeenDefined = false; - - MacroFrame(MacroInstance macroInstance) { - this.macroInstance = macroInstance; - } - } - - private enum NameConflictCheckingPolicy { - UNKNOWN, - NOT_GUARANTEED, - ENABLED; - } - - /** - * Whether to do all validation checks for name clashes among targets, macros, and output file - * prefixes. - * - *

The {@code NOT_GUARANTEED} value should only be used when the package data has already - * been validated, e.g. in package deserialization. - * - *

Setting it to {@code NOT_GUARANTEED} does not necessarily turn off *all* checking, just - * some of the more expensive ones. Do not rely on being able to violate these checks. - */ - private NameConflictCheckingPolicy nameConflictCheckingPolicy = - NameConflictCheckingPolicy.UNKNOWN; - - /** - * Stores labels for each rule so that we don't have to call the costly {@link Rule#getLabels} - * twice (once for {@link #checkForInputOutputConflicts} and once for {@link #beforeBuild}). - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. - */ - // TODO(#19922): Technically we don't need to store entries for rules that were created by - // macros; see rulesCreatedInMacros, below. - @Nullable private Map> ruleLabels = new HashMap<>(); - - /** - * Stores labels of rule targets that were created in symbolic macros. We don't implicitly - * create input files on behalf of such targets (though they may still be created on behalf of - * other targets not in macros). - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. - */ - // TODO(#19922): This can be eliminated once we have Targets directly store a reference to the - // MacroInstance that instantiated them. (This is a little nontrivial because we'd like to avoid - // simply adding a new field to Target subclasses, and instead want to combine it with the - // existing Package-typed field.) - @Nullable private Set rulesCreatedInMacros = new HashSet<>(); - - /** - * A map from names of targets declared in a symbolic macro which violate macro naming rules, - * such as "lib%{name}-src.jar" implicit outputs in java rules, to the name of the macro - * instance where they were declared. - * - *

This field is null if name conflict checking is disabled. The content of the map is - * manipulated only in {@link #checkRuleAndOutputs}. - */ - @Nullable - private LinkedHashMap macroNamespaceViolatingTargets = new LinkedHashMap<>(); - - /** - * A map from target name to the (innermost) macro instance that declared it. See {@link - * Package#targetsToDeclaringMacros}. - */ - private LinkedHashMap 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. - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. The content of the map is manipulated only in {@link #checkRuleAndOutputs}. - */ - @Nullable private Map outputFilePrefixes = new HashMap<>(); - private final List registeredExecutionPlatforms = new ArrayList<>(); private final List registeredToolchains = new ArrayList<>(); @@ -1678,25 +1543,6 @@ void setIOException(IOException e, String message, DetailedExitCode detailedExit setContainsErrors(); } - /** - * Declares that errors were encountering while loading this package. If called, {@link - * #addEvent} or {@link #addEvents} should already have been called with an {@link Event} of - * type {@link EventKind#ERROR} that includes a {@link FailureDetail}. - */ - // TODO(bazel-team): For simplicity it would be nice to replace this with - // getLocalEventHandler().hasErrors(), since that would prevent the kind of inconsistency where - // we have reported an ERROR event but not called setContainsErrors(), or vice versa. - public void setContainsErrors() { - // TODO(bazel-team): Maybe do Preconditions.checkState(localEventHandler.hasErrors()). - // Maybe even assert that it has a FailureDetail, though that's a linear scan unless we - // customize the event handler. - this.containsErrors = true; - } - - public boolean containsErrors() { - return containsErrors; - } - void setFailureDetailOverride(FailureDetail failureDetail) { failureDetailOverride = failureDetail; } @@ -1800,59 +1646,14 @@ MacroInstance createMacro( return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth); } - /** - * Inserts a target into the targets map. Returns the previous target if one was present, or - * null. - */ - @CanIgnoreReturnValue - @Nullable - private Target addOrReplaceTarget(Target target) { - Target existing = targets.put(target.getName(), target); - if (currentMacroFrame != null) { - targetsToDeclaringMacros.put(target.getName(), currentMacroFrame.macroInstance); - } - return existing; - } - - @Nullable - Target getTarget(String name) { - return targets.get(name); - } - - /** - * Replaces a target in the {@link Package} under construction with a new target with the same - * name and belonging to the same package. - * - *

Requires that {@link #disableNameConflictChecking} was not called. - * - *

A hack needed for {@link WorkspaceFactoryHelper}. - */ + @Override void replaceTarget(Target newTarget) { - ensureNameConflictChecking(); - - Preconditions.checkArgument( - targets.containsKey(newTarget.getName()), - "No existing target with name '%s' in the targets map", - newTarget.getName()); Preconditions.checkArgument( newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg` "Replacement target belongs to package '%s', expected '%s'", newTarget.getPackage(), pkg); - Target oldTarget = addOrReplaceTarget(newTarget); - if (newTarget instanceof Rule) { - List

The returned {@link Iterable} will be deterministically ordered, in the order the rule - * instance targets were instantiated. - */ - private Iterable getRules() { - return Package.getTargets(targets, Rule.class); - } - /** * Creates an input file target in this package with the specified name, if it does not yet * exist. @@ -2063,92 +1854,18 @@ void addEnvironmentGroup( } } - /** - * Turns off (some) conflict checking for name clashes between targets, macros, and output file - * prefixes. (It is not guaranteed to disable all checks, since it is intended as an - * optimization and not for semantic effect.) - * - *

This should only be done for data that has already been validated, e.g. during package - * deserialization. Do not call this unless you know what you're doing. - * - *

This method must be called prior to {@link #addRuleUnchecked}. It may not be called, - * neither before nor after, a call to {@link #addRule} or {@link #replaceTarget}. - */ - @CanIgnoreReturnValue - Builder disableNameConflictChecking() { - Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED; - this.ruleLabels = null; - this.rulesCreatedInMacros = null; - this.macroNamespaceViolatingTargets = null; - this.outputFilePrefixes = null; - return this; - } - - private void ensureNameConflictChecking() { - Preconditions.checkState( - nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED; - } - - /** - * Adds a rule and its outputs to the targets map, and propagates the error bit from the rule to - * the package. - */ - private void addRuleInternal(Rule rule) { + @Override + protected void addRuleInternal(Rule rule) { Preconditions.checkArgument(rule.getPackage() == pkg); - for (OutputFile outputFile : rule.getOutputFiles()) { - addOrReplaceTarget(outputFile); - } - addOrReplaceTarget(rule); - if (rule.containsErrors()) { - setContainsErrors(); - } + super.addRuleInternal(rule); } - /** - * Adds a rule without certain validation checks. Requires that {@link - * #disableNameConflictChecking} was already called. - */ - void addRuleUnchecked(Rule rule) { - Preconditions.checkState( - nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED); - addRuleInternal(rule); - } - - /** - * Adds a rule, subject to the usual validation checks. Requires that {@link - * #disableNameConflictChecking} was not called. - */ - void addRule(Rule rule) throws NameConflictException { - ensureNameConflictChecking(); - - List

Note that this function examines only the current macro frame, not any parent frames; and - * thus returns true even if the current non-finalizer macro was called within a finalizer - * macro. - */ - public boolean currentlyInNonFinalizerMacro() { - return currentMacroFrame != null - ? !currentMacroFrame.macroInstance.getMacroClass().isFinalizer() - : false; - } - - /** - * Returns true if a symbolic macro is running and the current macro frame is a rule finalizer. - */ - public boolean currentlyInFinalizer() { - return currentMacroFrame != null - ? currentMacroFrame.macroInstance.getMacroClass().isFinalizer() - : false; - } - - /** - * Sets the current macro frame and returns the old one. - * - *

Either the new or old frame may be null, indicating no currently running symbolic macro. - */ - @Nullable - MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { - MacroFrame prev = currentMacroFrame; - currentMacroFrame = frame; - 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 @@ -2463,281 +2139,6 @@ Package build(boolean discoverAssumedInputFiles) throws NoSuchPackageException { return finishBuild(); } - /** - * Adds an input file to this package. - * - *

There must not already be a target with the same name (i.e., this is not idempotent). - */ - private void addInputFile(InputFile inputFile) { - Target prev = addOrReplaceTarget(inputFile); - Preconditions.checkState(prev == null); - } - - /** - * Precondition check for {@link #addRule} (to be called before the rule and its outputs are in - * the targets map). Verifies that: - * - *

    - *
  • The added rule's name, and the names of its output files, are not the same as the name - * of any target already declared in the package. - *
  • The added rule's output files list does not contain the same name twice. - *
  • The added rule does not have an input file and an output file that share the same name. - *
  • For each of the added rule's output files, no directory prefix of that file matches the - * name of another output file in the package; and conversely, the file is not itself a - * prefix for another output file. (This check statefully mutates the {@code - * outputFilePrefixes} field.) - *
- */ - // TODO(bazel-team): We verify that all prefixes of output files are distinct from other output - // file names, but not that they're distinct from other target names in the package. What - // happens if you define an input file "abc" and output file "abc/xyz"? - private void checkRuleAndOutputs(Rule rule, List