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 2b0d617bb56c05..5b052c075aed87 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 @@ -116,10 +116,12 @@ public class Package { private static final long PACKAGE_OVERHEAD_UNSET = -1; /** - * An exception used when a target's name conflicts with other targets in the package. + * An exception used when the name of a target or symbolic macro clashes with another entity + * defined in the package. * - *

This could be an exact match, or in the case of output files, a prefix clash (one output - * file is a prefix of another). + *

Common examples of conflicts include two targets or symbolic macros sharing the same name, + * and one output file being a prefix of another. See {@link #checkForExistingName} and {@link + * #checkRuleAndOutputs} for more details. */ public static final class NameConflictException extends Exception { private NameConflictException(String message) { @@ -922,19 +924,19 @@ default boolean precomputeTransitiveLoads() { private enum NameConflictCheckingPolicy { UNKNOWN, - DISABLED, + NOT_GUARANTEED, ENABLED; } /** - * Whether to do validation checks for name clashes between targets and between output file + * Whether to do all validation checks for name clashes among targets, macros, and output file * prefixes. * - *

This should only be disabled for data that has already been validated, e.g. in package - * deserialization. + *

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

Even disabling this does not turn off *all* checking, just some of the more expensive - * ones. Do not rely on being able to violate these checks. + *

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; @@ -953,7 +955,7 @@ private enum NameConflictCheckingPolicy { * 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 #checkForConflicts}. + * package is built. The content of the map is manipulated only in {@link #checkRuleAndOutputs}. */ @Nullable private Map outputFilePrefixes = new HashMap<>(); @@ -1425,8 +1427,8 @@ private Iterable getRules() { * @param targetName name of the input file. This must be a valid target name as defined by * {@link com.google.devtools.build.lib.cmdline.LabelValidator#validateTargetName}. * @return the newly-created {@code InputFile}, or the old one if it already existed. - * @throws NameConflictException if the name was already taken by another target that is not an - * input file + * @throws NameConflictException if the name was already taken by another target/macro that is + * not an input file * @throws IllegalArgumentException if the name is not a valid label */ InputFile createInputFile(String targetName, Location location) throws NameConflictException { @@ -1444,12 +1446,9 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl "FileTarget in package " + pkg.getName() + " has illegal name: " + targetName, e); } - if (existing == null) { - addInputFile(inputFile); - return inputFile; - } else { - throw nameConflict(inputFile, existing); - } + checkForExistingName(inputFile); + addInputFile(inputFile); + return inputFile; } /** @@ -1509,7 +1508,7 @@ void addPackageGroup( repoRootMeansCurrentRepo, eventHandler, location); - checkNameConflict(group); + checkForExistingName(group); targets.put(group.getName(), group); if (group.containsErrors()) { @@ -1559,7 +1558,7 @@ void addEnvironmentGroup( EnvironmentGroup group = new EnvironmentGroup(createLabel(name), pkg, environments, defaults, location); - checkNameConflict(group); + checkForExistingName(group); targets.put(group.getName(), group); // Invariant: once group is inserted into targets, it must also: @@ -1594,8 +1593,9 @@ void addEnvironmentGroup( } /** - * Turns off conflict checking for name clashes between targets, or between output file - * prefixes. + * 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. @@ -1606,14 +1606,15 @@ void addEnvironmentGroup( @CanIgnoreReturnValue Builder disableNameConflictChecking() { Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.DISABLED; + this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED; this.ruleLabels = null; this.outputFilePrefixes = null; return this; } private void ensureNameConflictChecking() { - Preconditions.checkState(nameConflictCheckingPolicy != NameConflictCheckingPolicy.DISABLED); + Preconditions.checkState( + nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED); this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED; } @@ -1637,7 +1638,8 @@ private void addRuleInternal(Rule rule) { * #disableNameConflictChecking} was already called. */ void addRuleUnchecked(Rule rule) { - Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.DISABLED); + Preconditions.checkState( + nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED); addRuleInternal(rule); } @@ -1649,13 +1651,14 @@ void addRule(Rule rule) throws NameConflictException { ensureNameConflictChecking(); List