From d0498fb0fb60db79a76ef1a7d613645d32fad393 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 4 Jan 2024 07:14:42 -0800 Subject: [PATCH] Clarify name conflict checking invariants Previously, it was possible in theory, but not in practice, to call both addRule() and addRuleUnchecked() on the same Package.Builder. This CL removes that possibility, and in doing so makes it easier to reason about whether the ruleLabels map will be null. It also makes the outputFilePrefixes map nullable along the same lines as ruleLabels. Both maps are only used for validation, and so are irrelevant to Builders that use addRuleUnchecked. For completeness/correctness, replaceTarget is also updated to mention its interaction with this invariant. Work toward #19922. PiperOrigin-RevId: 595696336 Change-Id: I3965033784025e3dd6cfc85ec0b591a21c24de8c --- .../devtools/build/lib/packages/Package.java | 103 +++++++++++++----- 1 file changed, 73 insertions(+), 30 deletions(-) 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 2d58cf8eb62f93..50eb695027b44f 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 @@ -887,14 +887,37 @@ default boolean precomputeTransitiveLoads() { new SnapshottableBiMap<>(target -> target instanceof Rule); private final Map environmentGroups = new HashMap<>(); + private enum NameConflictCheckingPolicy { + UNKNOWN, + DISABLED, + ENABLED; + } + + /** + * Whether to do validation checks for name clashes between targets and between output file + * prefixes. This should only be disabled for data that has already been validated, e.g. in + * package deserialization. + */ + 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}). * - *

Remains {@code null} when rules are added via {@link #addRuleUnchecked}, which occurs with - * package deserialization. Set back to {@code null} after building. + *

This field is null if name conflict checking is disabled. It is also null after the + * package is built. + */ + @Nullable private Map> ruleLabels = new HashMap<>(); + + /** + * 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. */ - @Nullable private Map> ruleLabels = null; + @Nullable private Map outputFilePrefixes = new HashMap<>(); private final List registeredExecutionPlatforms = new ArrayList<>(); private final List registeredToolchains = new ArrayList<>(); @@ -912,16 +935,6 @@ default boolean precomputeTransitiveLoads() { /** True iff the "package" function has already been called in this package. */ private boolean packageFunctionUsed; - /** - * The collection of the prefixes of every output file. Maps every prefix to an output file - * whose prefix it is. - * - *

This is needed to make the output file prefix conflict check be reasonably fast. However, - * since it can potentially take a lot of memory and is useless after the package has been - * loaded, it isn't passed to the package itself. - */ - private final Map outputFilePrefixes = new HashMap<>(); - private final Interner> listInterner = new ThreadCompatibleInterner<>(); private final ImmutableMap generatorMap; @@ -1306,9 +1319,13 @@ Target getTarget(String 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}. */ void replaceTarget(Target newTarget) { + ensureNameConflictChecking(); + Preconditions.checkArgument( targets.containsKey(newTarget.getName()), "No existing target with name '%s' in the targets map", @@ -1319,7 +1336,7 @@ void replaceTarget(Target newTarget) { newTarget.getPackage(), pkg); Target oldTarget = targets.put(newTarget.getName(), newTarget); - if (newTarget instanceof Rule && ruleLabels != null) { + if (newTarget instanceof Rule) { List

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. * - *

Don't call this function 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}. */ - void addRuleUnchecked(Rule rule) { + @CanIgnoreReturnValue + Builder disableNameConflictChecking() { + Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); + this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.DISABLED; + this.ruleLabels = null; + this.outputFilePrefixes = null; + return this; + } + + private void ensureNameConflictChecking() { + Preconditions.checkState(nameConflictCheckingPolicy != NameConflictCheckingPolicy.DISABLED); + this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED; + } + + private void addRuleInternal(Rule rule) { Preconditions.checkArgument(rule.getPackage() == pkg); for (OutputFile outputFile : rule.getOutputFiles()) { targets.put(outputFile.getName(), outputFile); @@ -1562,13 +1598,25 @@ void addRuleUnchecked(Rule rule) { } } + /** + * Adds a rule without certain validation checks. Requires that {@link + * #disableNameConflictChecking} was already called. + */ + void addRuleUnchecked(Rule rule) { + Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.DISABLED); + 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