Skip to content

Commit

Permalink
Check symbolic macro instances for name conflicts with other objects …
Browse files Browse the repository at this point in the history 
…in a package

The maps of targets and macro instances are tracked separately in the Package.Builder, but we need to enforce that their keys are disjoint (since macro names will be used as the basis for deciding which macro to expand to obtain the target for a given label.) This CL accomplishes this by updating the name conflict utility methods to conveniently check both at the same time.

Package.java:
- createInputFile(): Rephrase to use checkNameConflict (now checkForExistingName()). This improves readability and avoids adding new complexity to this method, at the expense of an extra redundant lookup in the targets map.
- beforeBuild(): Don't implicitly create an input file if it is the name of a macro.
- checkForConflicts(): Rename checkRuleAndOutputs(). Construct a bespoke (and more streamlined) exception message, so we can eliminate the nameConflict() helper.
- checkNameConflict(): Rename checkForExistingName(). Split into overloads for a Target and MacroInstance arg, both of which dispatch to unified logic for checking the targets/macros maps and constructing the appropriate error message. This absorbs the nameConflict() helper.

OutputFileTest:
- Update test case for new error message.

PackageFactoryTest:
- Test cases for conflicts between macros and each of: rule targets, generated outputs, environment groups, package groups, explicitly declared input files (exports_files), and other macros, all in both declaration orders (to exercise different code paths).
- Check that macros *can* conflict with output prefixes, because that's a behavior we have for targets, whether or not it's a good one (I honestly don't understand it). Didn't bother doing both orders here.
- Check that macros prevent implicit creation of input files by the same name.

Work toward #19922.

PiperOrigin-RevId: 597836620
Change-Id: Iee110b4db0dfcd09b72d43e6c83f435003a45e16
  • Loading branch information
@brandjon @copybara-github
brandjon authored and copybara-github committed Jan 12, 2024
1 parent ed1accb commit b953026
Show file tree
Hide file tree
Showing 3 changed files with 256 additions and 62 deletions.
154 changes: 96 additions & 58 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>This could be an exact match, or in the case of output files, a prefix clash (one output
* file is a prefix of another).
* <p>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) {
Expand Down Expand Up @@ -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.
*
* <p>This should only be disabled for data that has already been validated, e.g. in package
* deserialization.
* <p>The {@code NOT_GUARANTEED} value should only be used when the package data has already
* been validated, e.g. in package deserialization.
*
* <p>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.
* <p>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;
Expand All @@ -953,7 +955,7 @@ private enum NameConflictCheckingPolicy {
* file having that prefix. Used for error reporting.
*
* <p>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<String, OutputFile> outputFilePrefixes = new HashMap<>();

Expand Down Expand Up @@ -1425,8 +1427,8 @@ private Iterable<Rule> 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 {
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -1509,7 +1508,7 @@ void addPackageGroup(
repoRootMeansCurrentRepo,
eventHandler,
location);
checkNameConflict(group);
checkForExistingName(group);
targets.put(group.getName(), group);

if (group.containsErrors()) {
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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.)
*
* <p>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.
Expand All @@ -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;
}

Expand All @@ -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);
}

Expand All @@ -1649,13 +1651,14 @@ void addRule(Rule rule) throws NameConflictException {
ensureNameConflictChecking();

List<Label> labels = rule.getLabels();
checkForConflicts(rule, labels);
checkRuleAndOutputs(rule, labels);
addRuleInternal(rule);
ruleLabels.put(rule, labels);
}

/** Adds a symbolic macro instance to the package. */
public void addMacro(MacroInstance macro) throws NameConflictException {
// TODO(#19922): Add name conflict checking!
checkForExistingName(macro);
macros.put(macro.getName(), macro);
// TODO(#19922): Push to a queue of unexpanded macros, read those when expanding macros as
// part of monolithic package evaluation (but not lazy macro evaluation).
Expand Down Expand Up @@ -1722,6 +1725,7 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
for (Label label : labels) {
if (label.getPackageIdentifier().equals(pkg.getPackageIdentifier())
&& !targets.containsKey(label.getName())
&& !macros.containsKey(label.getName())
&& !newInputFiles.containsKey(label.getName())) {
Location loc = rule.getLocation();
newInputFiles.put(
Expand Down Expand Up @@ -1816,11 +1820,12 @@ private void addInputFile(InputFile inputFile) {
}

/**
* Precondition check for {@link #addRule}. Verifies that:
* Precondition check for {@link #addRule} (to be called before the rule and its outputs are in
* the targets map). Verifies that:
*
* <ul>
* <li>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.
* of any target/macro already declared in the package.
* <li>The added rule's output files list does not contain the same name twice.
* <li>The added rule does not have an input file and an output file that share the same name.
* <li>For each of the added rule's output files, no directory prefix of that file matches the
Expand All @@ -1832,12 +1837,12 @@ private void addInputFile(InputFile inputFile) {
// 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 checkForConflicts(Rule rule, List<Label> labels) throws NameConflictException {
private void checkRuleAndOutputs(Rule rule, List<Label> labels) throws NameConflictException {
Preconditions.checkNotNull(outputFilePrefixes); // ensured by addRule's precondition

// Check the name of the new rule itself.
String ruleName = rule.getName();
checkNameConflict(rule);
checkForExistingName(rule);

ImmutableList<OutputFile> outputFiles = rule.getOutputFiles();
Map<String, OutputFile> outputFilesByName =
Expand All @@ -1846,13 +1851,16 @@ private void checkForConflicts(Rule rule, List<Label> labels) throws NameConflic
// Check the new rule's output files, both for direct conflicts and prefix conflicts.
for (OutputFile outputFile : outputFiles) {
String outputFileName = outputFile.getName();
// Check for duplicate within a single rule. (Can't use checkNameConflict since this rule's
// outputs aren't in the target map yet.)
// Check for duplicate within a single rule. (Can't use checkForExistingName since this
// rule's outputs aren't in the target map yet.)
if (outputFilesByName.put(outputFileName, outputFile) != null) {
throw nameConflict(outputFile, outputFile); // Duplicate within a single rule.
throw new NameConflictException(
String.format(
"rule '%s' has more than one generated file named '%s'",
ruleName, outputFileName));
}
// Check for conflict with any other already added target.
checkNameConflict(outputFile);
// Check for conflict with any other already added target/macro.
checkForExistingName(outputFile);
// TODO(bazel-team): We also need to check for a conflict between an output file and its own
// rule, which is not yet in the targets map.

Expand Down Expand Up @@ -1892,34 +1900,64 @@ private void checkForConflicts(Rule rule, List<Label> labels) throws NameConflic
}

/**
* Throws {@link NameConflictException} if a target with the given target's name is already
* present in the package.
* Throws {@link NameConflictException} if the given target's name matches an existing target or
* macro in the package.
*/
private void checkNameConflict(Target added) throws NameConflictException {
Target existing = targets.get(added.getName());
if (existing != null) {
throw nameConflict(added, existing);
}
private void checkForExistingName(Target added) throws NameConflictException {
checkForExistingName(added.getName(), added);
}

/**
* Throws {@link NameConflictException} if the given macro's name matches an existing target or
* macro in the package.
*/
private void checkForExistingName(MacroInstance added) throws NameConflictException {
checkForExistingName(added.getName(), added);
}

/** Returns a {@link NameConflictException} about two targets having the same name. */
private static NameConflictException nameConflict(Target added, Target existing) {
String subject = String.format("%s '%s'", added.getTargetKind(), added.getName());
if (added instanceof OutputFile) {
subject += " in rule '" + ((OutputFile) added).getGeneratingRule().getName() + "'";
private void checkForExistingName(String name, Object added) throws NameConflictException {
Object existing = targets.get(name);
if (existing == null) {
existing = macros.get(name);
}
if (existing == null) {
return;
}

String object =
existing instanceof OutputFile
? String.format(
"generated file from rule '%s'",
((OutputFile) existing).getGeneratingRule().getName())
: existing.getTargetKind();
// Format error message subject and object, which are either Targets or MacroInstances.

return new NameConflictException(
String.format(
"%s conflicts with existing %s, defined at %s",
subject, object, existing.getLocation()));
String subject;
if (added instanceof Target) {
subject =
String.format("%s '%s'", ((Target) added).getTargetKind(), ((Target) added).getName());
if (added instanceof OutputFile) {
subject += " in rule '" + ((OutputFile) added).getGeneratingRule().getName() + "'";
}
} else if (added instanceof MacroInstance) {
subject = String.format("macro '%s'", ((MacroInstance) added).getName());
} else {
throw new IllegalArgumentException("Unexpected object type: " + added.getClass());
}

String object;
if (existing instanceof Target) {
object =
existing instanceof OutputFile
? String.format(
"generated file from rule '%s'",
((OutputFile) existing).getGeneratingRule().getName())
: ((Target) existing).getTargetKind();
object += ", defined at " + ((Target) existing).getLocation();
} else if (existing instanceof MacroInstance) {
// TODO(#19922): Add definition location info for the existing object, like we have in the
// case for rules.
object = "macro";
} else {
throw new AssertionError();
}

throw new NameConflictException(
String.format("%s conflicts with existing %s", subject, object));
}

/**
Expand Down
11 changes: 7 additions & 4 deletions src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,15 @@ public void testOutputFileNameConflictsWithExistingRule() throws Exception {
@Test
public void testDuplicateOutputFilesInSameRule() throws Exception {
scratch.file(
"two_outs/BUILD", "genrule(name='a', cmd='ls >$(location out)',outs=['out', 'out'])");
"two_outs/BUILD",
"genrule(",
" name='a',",
" cmd='ls >$(location out)',",
" outs=['out', 'out'],",
")");
reporter.removeHandler(failFastHandler);
getTarget("//two_outs:BUILD");
assertContainsEvent(
"generated file 'out' in rule 'a' conflicts with "
+ "existing generated file from rule 'a'");
assertContainsEvent("rule 'a' has more than one generated file named 'out'");
}

@Test
Expand Down
Loading

0 comments on commit b953026

Please sign in to comment.