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 c0ff8a35a25923..3ec4d376bd3156 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 @@ -882,7 +882,8 @@ public static Builder newPackageBuilder( @Nullable ImmutableMap generatorMap, // TODO(bazel-team): See Builder() constructor comment about use of null for this param. @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, - @Nullable Globber globber) { + @Nullable Globber globber, + boolean enableNameConflictChecking) { // Determine whether this is for a repo rule package. We shouldn't actually have to do this // because newPackageBuilder() is supposed to only be called for normal packages. Unfortunately // serialization still uses the same code path for deserializing BUILD and WORKSPACE files, @@ -912,7 +913,8 @@ public static Builder newPackageBuilder( cpuBoundSemaphore, packageOverheadEstimator, generatorMap, - globber); + globber, + enableNameConflictChecking); } public static Builder newExternalPackageBuilder( @@ -944,7 +946,8 @@ public static Builder newExternalPackageBuilder( /* cpuBoundSemaphore= */ null, packageOverheadEstimator, /* generatorMap= */ null, - /* globber= */ null); + /* globber= */ null, + /* enableNameConflictChecking= */ true); } public static Builder newExternalPackageBuilderForBzlmod( @@ -973,7 +976,8 @@ public static Builder newExternalPackageBuilderForBzlmod( /* cpuBoundSemaphore= */ null, PackageOverheadEstimator.NOOP_ESTIMATOR, /* generatorMap= */ null, - /* globber= */ null) + /* globber= */ null, + /* enableNameConflictChecking= */ true) .setLoads(ImmutableList.of()); } @@ -1090,7 +1094,8 @@ private Builder( @Nullable Semaphore cpuBoundSemaphore, PackageOverheadEstimator packageOverheadEstimator, @Nullable ImmutableMap generatorMap, - @Nullable Globber globber) { + @Nullable Globber globber, + boolean enableNameConflictChecking) { super( metadata, new Package(metadata), @@ -1100,7 +1105,8 @@ private Builder( mainRepositoryMapping, cpuBoundSemaphore, generatorMap, - globber); + globber, + enableNameConflictChecking); this.precomputeTransitiveLoads = precomputeTransitiveLoads; this.noImplicitFileExport = noImplicitFileExport; this.packageOverheadEstimator = packageOverheadEstimator; @@ -1422,11 +1428,6 @@ Rule getNonFinalizerInstantiatedRule(String name) { } } - // For Package deserialization. - void disableNameConflictChecking() { - recorder.disableNameConflictChecking(); - } - public void addRuleUnchecked(Rule rule) { Preconditions.checkArgument(rule.getPackage() == pkg); recorder.addRuleUnchecked(rule); diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index 3ce05f46d70fe6..726df4e1e0aaa7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -257,7 +257,8 @@ public Package.Builder newPackageBuilder( packageOverheadEstimator, generatorMap, configSettingVisibilityPolicy, - globber); + globber, + /* enableNameConflictChecking= */ true); } /** Returns a new {@link NonSkyframeGlobber}. */ diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java b/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java index db1a488e3c5cec..5b8d567aacaada 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java @@ -87,7 +87,7 @@ public abstract class TargetDefinitionContext extends StarlarkThreadContext { // The container object on which targets and macro instances are added and conflicts are // detected. - protected final TargetRecorder recorder = new TargetRecorder(); + protected final TargetRecorder recorder; // Initialized from outside but also potentially set by `workspace()` function in WORKSPACE // file. @@ -175,7 +175,8 @@ public T intern(T sample) { RepositoryMapping mainRepositoryMapping, @Nullable Semaphore cpuBoundSemaphore, @Nullable ImmutableMap generatorMap, - @Nullable Globber globber) { + @Nullable Globber globber, + boolean enableNameConflictChecking) { super(() -> mainRepositoryMapping); this.metadata = metadata; this.pkg = pkg; @@ -203,6 +204,8 @@ public T intern(T sample) { this.generatorMap = (generatorMap == null) ? ImmutableMap.of() : generatorMap; this.globber = globber; + this.recorder = new TargetRecorder(enableNameConflictChecking); + // Add target for the BUILD file itself. // (This may be overridden by an exports_file declaration.) // TODO: #19922 - Figure out exactly where this line belongs once TargetDefinitionContext is a diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetRecorder.java b/src/main/java/com/google/devtools/build/lib/packages/TargetRecorder.java index 3bfef01442da5c..eae483f443c926 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetRecorder.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetRecorder.java @@ -91,61 +91,41 @@ static class MacroFrame { } } - 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 Package.Builder#checkForInputOutputConflicts} and once for {@link * Package.Builder#beforeBuild}). * - *

This field is null if name conflict checking is disabled. It is also null after the package - * is built. + *

This field is null if name conflict checking is disabled. */ // 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<>(); + @Nullable private final Map> ruleLabels; /** * 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. + *

This field is null if name conflict checking is disabled. */ // 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<>(); + @Nullable private final Set rulesCreatedInMacros; /** * 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}. + *

Outside of package deserialization, the content of the map is manipulated only in {@link + * #checkRuleAndOutputs}. During deserialization, this map may also be populated by calling {@link + * #putAllMacroNamespaceViolatingTargets}. */ - @Nullable - private LinkedHashMap macroNamespaceViolatingTargets = new LinkedHashMap<>(); + private final LinkedHashMap macroNamespaceViolatingTargets = + new LinkedHashMap<>(); /** * A map from target name to the (innermost) macro instance that declared it. See {@link @@ -158,10 +138,31 @@ private enum NameConflictCheckingPolicy { * 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}. + *

This field is null if name conflict checking is disabled. The content of the map is + * manipulated only in {@link #checkRuleAndOutputs}. + */ + @Nullable private final Map outputFilePrefixes; + + /** + * Constructs a {@link TargetRecorder}. + * + * @param enableNameConflictChecking whether to perform all validation checks for name clashes + * among targets, macros, and output file prefixes. This should only be disabled when the + * package has already been validated, e.g. in package deserialization. Setting it to false + * does not necessarily turn off *all* checking, just some of the more expensive ones. Do not + * rely on being able to violate these checks. */ - @Nullable private Map outputFilePrefixes = new HashMap<>(); + public TargetRecorder(boolean enableNameConflictChecking) { + if (enableNameConflictChecking) { + this.ruleLabels = new HashMap<>(); + this.rulesCreatedInMacros = new HashSet<>(); + this.outputFilePrefixes = new HashMap<>(); + } else { + this.ruleLabels = null; + this.rulesCreatedInMacros = null; + this.outputFilePrefixes = null; + } + } public Map getTargetMap() { return targetMap; @@ -224,6 +225,10 @@ public boolean containsErrors() { return containsErrors; } + private boolean isNameConflictCheckingEnabled() { + return ruleLabels != null; + } + /** * Inserts a target into {@code targetMap}. Returns the previous target if one was present, or * null. @@ -296,12 +301,14 @@ public void unwrapSnapshottableBiMap() { * *

There must already be an existing target by the same name. * - *

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

Requires that the constructor was called with {@code enableNameConflictChecking} set to + * true. * *

A hack needed for {@link WorkspaceFactoryHelper}. */ public void replaceTarget(Target newTarget) { - ensureNameConflictChecking(); + Preconditions.checkState( + isNameConflictCheckingEnabled(), "Expected name conflict checking to be enabled"); Preconditions.checkArgument( targetMap.containsKey(newTarget.getName()), @@ -336,32 +343,6 @@ public Iterable getRules() { return Iterables.filter(targetMap.values(), Rule.class); } - /** - * 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}. - */ - public void disableNameConflictChecking() { - Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED; - this.ruleLabels = null; - this.rulesCreatedInMacros = null; - this.macroNamespaceViolatingTargets = null; - this.outputFilePrefixes = null; - } - - public 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. @@ -377,21 +358,22 @@ private void addRuleInternal(Rule rule) { } /** - * Adds a rule without certain validation checks. Requires that {@link - * #disableNameConflictChecking} was already called. + * Adds a rule without certain validation checks. Requires that the constructor was called with + * {@code enableNameConflictChecking} set to false. */ public void addRuleUnchecked(Rule rule) { Preconditions.checkState( - nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED); + !isNameConflictCheckingEnabled(), "Expected name conflict checking to be disabled"); addRuleInternal(rule); } /** - * Adds a rule, subject to the usual validation checks. Requires that {@link - * #disableNameConflictChecking} was not called. + * Adds a rule, subject to the usual validation checks. Requires that the constructor was called + * with {@code enableNameConflictChecking} set to true. */ public void addRule(Rule rule) throws NameConflictException { - ensureNameConflictChecking(); + Preconditions.checkState( + isNameConflictCheckingEnabled(), "Expected name conflict checking to be enabled"); List