Clean up Label validation, and introduce a factory method for constructing a Label without validation.
RELNOTES: None
PiperOrigin-RevId: 164479651
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
index f8e5643..9e54fc1 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -123,7 +123,7 @@
try {
LabelValidator.PackageAndTarget labelParts = LabelValidator.parseAbsoluteLabel(absName);
PackageIdentifier pkgIdWithoutRepo =
- validate(labelParts.getPackageName(), labelParts.getTargetName());
+ validatePackageName(labelParts.getPackageName(), labelParts.getTargetName());
PathFragment packageFragment = pkgIdWithoutRepo.getPackageFragment();
if (repo.isEmpty() && ABSOLUTE_PACKAGE_NAMES.contains(packageFragment)) {
repo = "@";
@@ -164,7 +164,7 @@
* @throws LabelSyntaxException if either of the arguments was invalid.
*/
public static Label create(String packageName, String targetName) throws LabelSyntaxException {
- return LABEL_INTERNER.intern(new Label(packageName, targetName));
+ return create(validatePackageName(packageName, targetName), targetName);
}
/**
@@ -173,7 +173,18 @@
*/
public static Label create(PackageIdentifier packageId, String targetName)
throws LabelSyntaxException {
- return LABEL_INTERNER.intern(new Label(packageId, targetName));
+ return createUnvalidated(packageId, validateTargetName(packageId, targetName));
+ }
+
+ /**
+ * Similar factory to above, but does not perform target name validation.
+ *
+ * <p>Only call this method if you know what you're doing; in particular, don't call it on
+ * arbitrary {@code targetName} inputs
+ */
+
+ public static Label createUnvalidated(PackageIdentifier packageId, String targetName) {
+ return LABEL_INTERNER.intern(new Label(packageId, StringCanonicalizer.intern(targetName)));
}
/**
@@ -213,13 +224,17 @@
}
/**
- * Validates the given target name and returns a canonical String instance if it is valid.
- * Otherwise it throws a SyntaxException.
+ * Validates the given target name and returns a normalized name if it is valid. Otherwise it
+ * throws a SyntaxException.
*/
- private static String canonicalizeTargetName(String name) throws LabelSyntaxException {
+ private static String validateTargetName(PackageIdentifier packageIdentifier, String name)
+ throws LabelSyntaxException {
String error = LabelValidator.validateTargetName(name);
if (error != null) {
error = "invalid target name '" + StringUtilities.sanitizeControlChars(name) + "': " + error;
+ if (packageIdentifier.getPackageFragment().getPathString().endsWith("/" + name)) {
+ error += " (perhaps you meant \":" + name + "\"?)";
+ }
throw new LabelSyntaxException(error);
}
@@ -227,15 +242,14 @@
if (name.endsWith("/.")) {
name = name.substring(0, name.length() - 2);
}
-
- return StringCanonicalizer.intern(name);
+ return name;
}
/**
* Validates the given package name and returns a canonical {@link PackageIdentifier} instance
* if it is valid. Otherwise it throws a SyntaxException.
*/
- private static PackageIdentifier validate(String packageIdentifier, String name)
+ private static PackageIdentifier validatePackageName(String packageIdentifier, String name)
throws LabelSyntaxException {
String error = null;
try {
@@ -262,34 +276,12 @@
/** Precomputed hash code. */
private final int hashCode;
- /**
- * Constructor from a package name, target name. Both are checked for validity
- * and a SyntaxException is thrown if either is invalid.
- * TODO(bazel-team): move the validation to {@link PackageIdentifier}. Unfortunately, there are a
- * bazillion tests that use invalid package names (taking advantage of the fact that calling
- * Label(PathFragment, String) doesn't validate the package name).
- */
- private Label(String packageIdentifier, String name) throws LabelSyntaxException {
- this(validate(packageIdentifier, name), name);
- }
-
- private Label(PackageIdentifier packageIdentifier, String name)
- throws LabelSyntaxException {
+ private Label(PackageIdentifier packageIdentifier, String name) {
Preconditions.checkNotNull(packageIdentifier);
Preconditions.checkNotNull(name);
this.packageIdentifier = packageIdentifier;
- try {
- this.name = canonicalizeTargetName(name);
- } catch (LabelSyntaxException e) {
- // This check is just for a more helpful error message
- // i.e. valid target name, invalid package name, colon-free label form
- // used => probably they meant "//foo:bar.c" not "//foo/bar.c".
- if (packageIdentifier.getPackageFragment().getPathString().endsWith("/" + name)) {
- throw new LabelSyntaxException(e.getMessage() + " (perhaps you meant \":" + name + "\"?)");
- }
- throw e;
- }
+ this.name = name;
this.hashCode = hashCode(this.name, this.packageIdentifier);
}
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 ec58068..9e4ab19 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
@@ -444,7 +444,7 @@
return events;
}
- /** Returns an (immutable, unordered) view of all the targets belonging to this package. */
+ /** Returns an (immutable, ordered) view of all the targets belonging to this package. */
public ImmutableSortedKeyMap<String, Target> getTargets() {
return targets;
}
@@ -457,7 +457,7 @@
}
/**
- * Returns a (read-only, unordered) iterator of all the targets belonging
+ * Returns a (read-only, ordered) iterable of all the targets belonging
* to this package which are instances of the specified class.
*/
public <T extends Target> Iterable<T> getTargets(Class<T> targetClass) {
@@ -1312,12 +1312,14 @@
// current instance here.
buildFile = (InputFile) Preconditions.checkNotNull(targets.get(buildFileLabel.getName()));
- List<Rule> rules = Lists.newArrayList(getTargets(Rule.class));
+ // The Iterable returned by getTargets is sorted, so when we build up the list of tests by
+ // processing it in order below, that list will be sorted too.
+ Iterable<Rule> sortedRules = Lists.newArrayList(getTargets(Rule.class));
if (discoverAssumedInputFiles) {
// All labels mentioned in a rule that refer to an unknown target in the
// current package are assumed to be InputFiles, so let's create them:
- for (final Rule rule : rules) {
+ for (final Rule rule : sortedRules) {
AggregatingAttributeMapper.of(rule).visitLabels(new AcceptsLabelAttribute() {
@Override
public void acceptLabelAttribute(Label label, Attribute attribute) {
@@ -1332,18 +1334,17 @@
// Note, we implement this here when the Package is fully constructed,
// since clearly this information isn't available at Rule construction
// time, as forward references are permitted.
- List<Label> allTests = new ArrayList<>();
- for (Rule rule : rules) {
+ List<Label> sortedTests = new ArrayList<>();
+ for (Rule rule : sortedRules) {
if (TargetUtils.isTestRule(rule) && !TargetUtils.hasManualTag(rule)) {
- allTests.add(rule.getLabel());
+ sortedTests.add(rule.getLabel());
}
}
- Collections.sort(allTests);
- for (Rule rule : rules) {
+ for (Rule rule : sortedRules) {
AttributeMap attributes = NonconfigurableAttributeMapper.of(rule);
if (rule.getRuleClass().equals("test_suite")
&& attributes.get("tests", BuildType.LABEL_LIST).isEmpty()) {
- rule.setAttributeValueByName("$implicit_tests", allTests);
+ rule.setAttributeValueByName("$implicit_tests", sortedTests);
}
}
return this;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 1a7cc36d..b751f87 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -473,13 +473,29 @@
*/
void populateOutputFiles(EventHandler eventHandler, Package.Builder pkgBuilder)
throws LabelSyntaxException, InterruptedException {
+ populateOutputFilesInternal(eventHandler, pkgBuilder, /*performChecks=*/ true);
+ }
+
+ void populateOutputFilesUnchecked(EventHandler eventHandler, Package.Builder pkgBuilder)
+ throws InterruptedException {
+ try {
+ populateOutputFilesInternal(eventHandler, pkgBuilder, /*performChecks=*/ false);
+ } catch (LabelSyntaxException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ void populateOutputFilesInternal(
+ EventHandler eventHandler, Package.Builder pkgBuilder, boolean performChecks)
+ throws LabelSyntaxException, InterruptedException {
Preconditions.checkState(outputFiles == null);
// Order is important here: implicit before explicit
ImmutableList.Builder<OutputFile> outputFilesBuilder = ImmutableList.builder();
ImmutableListMultimap.Builder<String, OutputFile> outputFileMapBuilder =
ImmutableListMultimap.builder();
- populateImplicitOutputFiles(eventHandler, pkgBuilder, outputFilesBuilder);
- populateExplicitOutputFiles(eventHandler, outputFilesBuilder, outputFileMapBuilder);
+ populateImplicitOutputFiles(eventHandler, pkgBuilder, outputFilesBuilder, performChecks);
+ populateExplicitOutputFiles(
+ eventHandler, outputFilesBuilder, outputFileMapBuilder, performChecks);
outputFiles = outputFilesBuilder.build();
outputFileMap = outputFileMapBuilder.build();
}
@@ -488,7 +504,8 @@
private void populateExplicitOutputFiles(
EventHandler eventHandler,
ImmutableList.Builder<OutputFile> outputFilesBuilder,
- ImmutableListMultimap.Builder<String, OutputFile> outputFileMapBuilder)
+ ImmutableListMultimap.Builder<String, OutputFile> outputFileMapBuilder,
+ boolean performChecks)
throws LabelSyntaxException {
NonconfigurableAttributeMapper nonConfigurableAttributes =
NonconfigurableAttributeMapper.of(this);
@@ -499,11 +516,22 @@
Label outputLabel = nonConfigurableAttributes.get(name, BuildType.OUTPUT);
if (outputLabel != null) {
addLabelOutput(
- attribute, outputLabel, eventHandler, outputFilesBuilder, outputFileMapBuilder);
+ attribute,
+ outputLabel,
+ eventHandler,
+ outputFilesBuilder,
+ outputFileMapBuilder,
+ performChecks);
}
} else if (type == BuildType.OUTPUT_LIST) {
for (Label label : nonConfigurableAttributes.get(name, BuildType.OUTPUT_LIST)) {
- addLabelOutput(attribute, label, eventHandler, outputFilesBuilder, outputFileMapBuilder);
+ addLabelOutput(
+ attribute,
+ label,
+ eventHandler,
+ outputFilesBuilder,
+ outputFileMapBuilder,
+ performChecks);
}
}
}
@@ -516,23 +544,31 @@
private void populateImplicitOutputFiles(
EventHandler eventHandler,
Package.Builder pkgBuilder,
- ImmutableList.Builder<OutputFile> outputFilesBuilder)
+ ImmutableList.Builder<OutputFile> outputFilesBuilder,
+ boolean performChecks)
throws InterruptedException {
try {
RawAttributeMapper attributeMap = RawAttributeMapper.of(this);
for (String out : implicitOutputsFunction.getImplicitOutputs(attributeMap)) {
- try {
- addOutputFile(pkgBuilder.createLabel(out), eventHandler, outputFilesBuilder);
- } catch (LabelSyntaxException e) {
- reportError(
- "illegal output file name '"
- + out
- + "' in rule "
- + getLabel()
- + " due to: "
- + e.getMessage(),
- eventHandler);
+ Label label;
+ if (performChecks) {
+ try {
+ label = pkgBuilder.createLabel(out);
+ } catch (LabelSyntaxException e) {
+ reportError(
+ "illegal output file name '"
+ + out
+ + "' in rule "
+ + getLabel()
+ + " due to: "
+ + e.getMessage(),
+ eventHandler);
+ continue;
+ }
+ } else {
+ label = Label.createUnvalidated(pkgBuilder.getPackageIdentifier(), out);
}
+ addOutputFile(label, eventHandler, outputFilesBuilder);
}
} catch (EvalException e) {
reportError(String.format("In rule %s: %s", getLabel(), e.print()), eventHandler);
@@ -544,16 +580,19 @@
Label label,
EventHandler eventHandler,
ImmutableList.Builder<OutputFile> outputFilesBuilder,
- ImmutableListMultimap.Builder<String, OutputFile> outputFileMapBuilder)
+ ImmutableListMultimap.Builder<String, OutputFile> outputFileMapBuilder,
+ boolean performChecks)
throws LabelSyntaxException {
- if (!label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) {
- throw new IllegalStateException("Label for attribute " + attribute
- + " should refer to '" + pkg.getName()
- + "' but instead refers to '" + label.getPackageFragment()
- + "' (label '" + label.getName() + "')");
- }
- if (label.getName().equals(".")) {
- throw new LabelSyntaxException("output file name can't be equal '.'");
+ if (performChecks) {
+ if (!label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) {
+ throw new IllegalStateException("Label for attribute " + attribute
+ + " should refer to '" + pkg.getName()
+ + "' but instead refers to '" + label.getPackageFragment()
+ + "' (label '" + label.getName() + "')");
+ }
+ if (label.getName().equals(".")) {
+ throw new LabelSyntaxException("output file name can't be equal '.'");
+ }
}
OutputFile outputFile = addOutputFile(label, eventHandler, outputFilesBuilder);
outputFileMapBuilder.put(attribute.getName(), outputFile);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 625130f..efc9d3c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1498,7 +1498,7 @@
Location location,
AttributeContainer attributeContainer,
ImplicitOutputsFunction implicitOutputsFunction)
- throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
+ throws InterruptedException, CannotPrecomputeDefaultsException {
Rule rule = pkgBuilder.createRule(
ruleLabel,
this,
@@ -1506,7 +1506,7 @@
attributeContainer,
implicitOutputsFunction);
populateRuleAttributeValues(rule, pkgBuilder, attributeValues, NullEventHandler.INSTANCE);
- rule.populateOutputFiles(NullEventHandler.INSTANCE, pkgBuilder);
+ rule.populateOutputFilesUnchecked(NullEventHandler.INSTANCE, pkgBuilder);
return rule;
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
index 5939573..2781b0f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
@@ -403,15 +402,11 @@
// Also add the BUILD file of the subinclude.
if (buildFiles) {
- try {
- addIfUniqueLabel(
- getSubincludeTarget(subinclude.getLocalTargetLabel("BUILD"), pkg),
- seenLabels,
- dependentFiles);
-
- } catch (LabelSyntaxException e) {
- throw new AssertionError("BUILD should always parse as a target name", e);
- }
+ addIfUniqueLabel(
+ getSubincludeTarget(
+ Label.createUnvalidated(subinclude.getPackageIdentifier(), "BUILD"), pkg),
+ seenLabels,
+ dependentFiles);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 8055158..f68fb30 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -38,7 +38,6 @@
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.cmdline.TargetPattern;
@@ -778,15 +777,11 @@
if (buildFiles) {
// Also add the BUILD file of the subinclude.
- try {
- addIfUniqueLabel(
- getSubincludeTarget(subinclude.getLocalTargetLabel("BUILD"), pkg),
- seenLabels,
- dependentFiles);
-
- } catch (LabelSyntaxException e) {
- throw new AssertionError("BUILD should always parse as a target name", e);
- }
+ addIfUniqueLabel(
+ getSubincludeTarget(
+ Label.createUnvalidated(subinclude.getPackageIdentifier(), "BUILD"), pkg),
+ seenLabels,
+ dependentFiles);
}
}
}
