Skip to content

Commit

Permalink
[6.2.0] TargetPattern parsing fixes (bazelbuild#17945)
Browse files Browse the repository at this point in the history 
* Tests for TargetPattern parsing, and some sanity fixes

- TargetPatternTest isn't testing much right now. This CL changes it to actually assert the result of parsing is as expected, and also test a few different parser setups (in main repo, with a relative directory, in a non-main repo).
  - This necessitated adding #toString methods to all TargetPattern subclasses.
  - Also rearranged the parameters in the constructors of all subclasses so that `originalPattern` is always the first parameter.
- Removed the always-true `checkWildcardConflict` parameter in `TargetsInPackage`.
- Removed the weak check before `interpretPathAsTarget` as it was not only weak, but wrong. It made parsing `Bar\\java` a failure at the repo root but a success in a subdirectory.

Work towards bazelbuild#4385

PiperOrigin-RevId: 520630430
Change-Id: Icee98f38134fe274ba800d2e949c83f0ae18c247

* Switch TargetPattern.Parser to use LabelParser

- Target patterns and labels have very similar syntax, yet are parsed completely separately. Furthermore, the code for target pattern parsing wasn't written with repos in mind, causing some corner cases such as bazelbuild#4385.
- This CL augments LabelParser to deal with some syntax specific to target patterns (mostly the '...' thing), and switches TargetPattern.Parser to use LabelParser instead.
- This fixes some inconsistencies between the two and eliminates the bug linked above.
- Added LabelParserTest to make sure that the table in the javadoc of LabelParser.Parts#parse is actually accurate.
  - Switched LabelParser.Parts to use AutoValue instead to enable testing.
- Some more cleanup in TargetPattern.Parser; removing outdated fields/methods/etc.

Fixes bazelbuild#4385.

RELNOTES: `@foo` labels can now be used on the command line as the top-level target (that is, `bazel build @foo` now works). Double-dot syntax is now forbidden (`bazel build ../foo` will no longer work).
PiperOrigin-RevId: 520902549
Change-Id: I38d6400381a3c4ac4c370360578702d5dd870909
  • Loading branch information
@Wyverald
Wyverald authored Apr 4, 2023
1 parent d3b59ad commit fb4a0c2
Show file tree
Hide file tree
Showing 11 changed files with 537 additions and 353 deletions.
32 changes: 19 additions & 13 deletions src/main/java/com/google/devtools/build/lib/cmdline/Label.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,12 @@ abstract static class PackageContextImpl implements PackageContext {}
*/
public static Label parseCanonical(String raw) throws LabelSyntaxException {
Parts parts = Parts.parse(raw);
parts.checkPkgDoesNotEndWithTripleDots();
parts.checkPkgIsAbsolute();
RepositoryName repoName =
parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo);
parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo());
return createUnvalidated(
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target);
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target());
}

public static Label parseCanonicalUnchecked(String raw) {
Expand All @@ -139,18 +140,18 @@ public static Label parseCanonicalUnchecked(String raw) {
/** Computes the repo name for the label, within the context of a current repo. */
private static RepositoryName computeRepoNameWithRepoContext(
Parts parts, RepoContext repoContext) {
if (parts.repo == null) {
if (parts.repo() == null) {
// Certain package names when used without a "@" part are always absolutely in the main repo,
// disregarding the current repo and repo mappings.
return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg)
return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg())
? RepositoryName.MAIN
: repoContext.currentRepo();
}
if (parts.repoIsCanonical) {
if (parts.repoIsCanonical()) {
// This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar").
return RepositoryName.createUnvalidated(parts.repo);
return RepositoryName.createUnvalidated(parts.repo());
}
return repoContext.repoMapping().get(parts.repo);
return repoContext.repoMapping().get(parts.repo());
}

/**
Expand All @@ -162,10 +163,11 @@ private static RepositoryName computeRepoNameWithRepoContext(
public static Label parseWithRepoContext(String raw, RepoContext repoContext)
throws LabelSyntaxException {
Parts parts = Parts.parse(raw);
parts.checkPkgDoesNotEndWithTripleDots();
parts.checkPkgIsAbsolute();
RepositoryName repoName = computeRepoNameWithRepoContext(parts, repoContext);
return createUnvalidated(
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target);
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target());
}

/**
Expand All @@ -178,14 +180,15 @@ public static Label parseWithRepoContext(String raw, RepoContext repoContext)
public static Label parseWithPackageContext(String raw, PackageContext packageContext)
throws LabelSyntaxException {
Parts parts = Parts.parse(raw);
parts.checkPkgDoesNotEndWithTripleDots();
// pkg is either absolute or empty
if (!parts.pkg.isEmpty()) {
if (!parts.pkg().isEmpty()) {
parts.checkPkgIsAbsolute();
}
RepositoryName repoName = computeRepoNameWithRepoContext(parts, packageContext);
PathFragment pkgFragment =
parts.pkgIsAbsolute ? PathFragment.create(parts.pkg) : packageContext.packageFragment();
return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target);
parts.pkgIsAbsolute() ? PathFragment.create(parts.pkg()) : packageContext.packageFragment();
return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target());
}

/**
Expand Down Expand Up @@ -251,7 +254,7 @@ public static Label parseAbsoluteUnchecked(String absName) {
public static Label create(String packageName, String targetName) throws LabelSyntaxException {
return createUnvalidated(
PackageIdentifier.parse(packageName),
validateAndProcessTargetName(packageName, targetName));
validateAndProcessTargetName(packageName, targetName, /* pkgEndsWithTripleDots= */ false));
}

/**
Expand All @@ -263,7 +266,10 @@ public static Label create(PackageIdentifier packageId, String targetName)
throws LabelSyntaxException {
return createUnvalidated(
packageId,
validateAndProcessTargetName(packageId.getPackageFragment().getPathString(), targetName));
validateAndProcessTargetName(
packageId.getPackageFragment().getPathString(),
targetName,
/* pkgEndsWithTripleDots= */ false));
}

/**
Expand Down
149 changes: 91 additions & 58 deletions src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.devtools.build.lib.cmdline;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.devtools.build.lib.util.StringUtilities;
import com.google.errorprone.annotations.FormatMethod;
import javax.annotation.Nullable;
Expand All @@ -26,82 +28,83 @@ private LabelParser() {}
* Contains the parsed elements of a label string. The parts are validated (they don't contain
* invalid characters). See {@link #parse} for valid label patterns.
*/
static final class Parts {
@AutoValue
abstract static class Parts {
/**
* The {@code @repo} or {@code @@canonical_repo} part of the string (sans any leading
* {@literal @}s); can be null if it doesn't have such a part (i.e. if it doesn't start with a
* {@literal @}).
*/
@Nullable final String repo;
@Nullable
abstract String repo();
/**
* Whether the repo part is using the canonical repo syntax (two {@literal @}s) or not (one
* {@literal @}). If there is no repo part, this is false.
*/
final boolean repoIsCanonical;
abstract boolean repoIsCanonical();
/**
* Whether the package part of the string is prefixed by double-slash. This can only be false if
* the repo part is missing.
*/
final boolean pkgIsAbsolute;
/** The package part of the string (sans double-slash, if any). */
final String pkg;
abstract boolean pkgIsAbsolute();
/**
* The package part of the string (sans the leading double-slash, if present; also sans the
* final '...' segment, if present).
*/
abstract String pkg();
/** Whether the package part of the string ends with a '...' segment. */
abstract boolean pkgEndsWithTripleDots();
/** The target part of the string (sans colon). */
final String target;
abstract String target();
/** The original unparsed raw string. */
final String raw;
abstract String raw();

private Parts(
@Nullable String repo,
boolean repoIsCanonical,
boolean pkgIsAbsolute,
String pkg,
String target,
String raw) {
this.repo = repo;
this.repoIsCanonical = repoIsCanonical;
this.pkgIsAbsolute = pkgIsAbsolute;
this.pkg = pkg;
this.target = target;
this.raw = raw;
}

private static Parts validateAndCreate(
@VisibleForTesting
static Parts validateAndCreate(
@Nullable String repo,
boolean repoIsCanonical,
boolean pkgIsAbsolute,
String pkg,
boolean pkgEndsWithTripleDots,
String target,
String raw)
throws LabelSyntaxException {
validateRepoName(repo);
validatePackageName(pkg, target);
return new Parts(
return new AutoValue_LabelParser_Parts(
repo,
repoIsCanonical,
pkgIsAbsolute,
pkg,
validateAndProcessTargetName(pkg, target),
pkgEndsWithTripleDots,
validateAndProcessTargetName(pkg, target, pkgEndsWithTripleDots),
raw);
}

/**
* Parses a raw label string into parts. The logic can be summarized by the following table:
*
* {@code
* raw | repo | repoIsCanonical | pkgIsAbsolute | pkg | target
* ----------------------+--------+-----------------+---------------+-----------+-----------
* foo/bar | null | false | false | "" | "foo/bar"
* //foo/bar | null | false | true | "foo/bar" | "bar"
* @repo | "repo" | false | true | "" | "repo"
* @@repo | "repo" | true | true | "" | "repo"
* @repo//foo/bar | "repo" | false | true | "foo/bar" | "bar"
* @@repo//foo/bar | "repo" | true | true | "foo/bar" | "bar"
* :quux | null | false | false | "" | "quux"
* foo/bar:quux | null | false | false | "foo/bar" | "quux"
* //foo/bar:quux | null | false | true | "foo/bar" | "quux"
* @repo//foo/bar:quux | "repo" | false | true | "foo/bar" | "quux"
* @@repo//foo/bar:quux | "repo" | true | true | "foo/bar" | "quux"
* }
* <pre>{@code
* raw | repo | repoIs- | pkgIs- | pkg | pkgEndsWith- | target
* | | Canonical | Absolute | | TripleDots |
* ----------------------+--------+-----------+----------+-----------+--------------+-----------
* "foo/bar" | null | false | false | "" | false | "foo/bar"
* "..." | null | false | false | "" | true | ""
* "...:all" | null | false | false | "" | true | "all"
* "foo/..." | null | false | false | "foo" | true | ""
* "//foo/bar" | null | false | true | "foo/bar" | false | "bar"
* "//foo/..." | null | false | true | "foo" | true | ""
* "//foo/...:all" | null | false | true | "foo" | true | "all"
* "//foo/all" | null | false | true | "foo/all" | false | "all"
* "@repo" | "repo" | false | true | "" | false | "repo"
* "@@repo" | "repo" | true | true | "" | false | "repo"
* "@repo//foo/bar" | "repo" | false | true | "foo/bar" | false | "bar"
* "@@repo//foo/bar" | "repo" | true | true | "foo/bar" | false | "bar"
* ":quux" | null | false | false | "" | false | "quux"
* "foo/bar:quux" | null | false | false | "foo/bar" | false | "quux"
* "//foo/bar:quux" | null | false | true | "foo/bar" | false | "quux"
* "@repo//foo/bar:quux" | "repo" | false | true | "foo/bar" | false | "quux"
* }</pre>
*/
static Parts parse(String rawLabel) throws LabelSyntaxException {
@Nullable final String repo;
Expand All @@ -116,9 +119,10 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
return validateAndCreate(
repo,
repoIsCanonical,
/*pkgIsAbsolute=*/ true,
/*pkg=*/ "",
/*target=*/ repo,
/* pkgIsAbsolute= */ true,
/* pkg= */ "",
/* pkgEndsWithTripleDots= */ false,
/* target= */ repo,
rawLabel);
} else {
repo = rawLabel.substring(repoIsCanonical ? 2 : 1, doubleSlashIndex);
Expand All @@ -136,20 +140,39 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
final String pkg;
final String target;
final int colonIndex = rawLabel.indexOf(':', startOfPackage);
if (colonIndex >= 0) {
pkg = rawLabel.substring(startOfPackage, colonIndex);
target = rawLabel.substring(colonIndex + 1);
} else if (pkgIsAbsolute) {
// Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar".
pkg = rawLabel.substring(startOfPackage);
// The target name is the last package segment (works even if `pkg` contains no slash)
target = pkg.substring(pkg.lastIndexOf('/') + 1);
} else {
final String rawPkg =
rawLabel.substring(startOfPackage, colonIndex >= 0 ? colonIndex : rawLabel.length());
final boolean pkgEndsWithTripleDots = rawPkg.endsWith("/...") || rawPkg.equals("...");
if (colonIndex < 0 && pkgEndsWithTripleDots) {
// Special case: if the entire label ends in '...', the target name is empty.
pkg = stripTrailingTripleDots(rawPkg);
target = "";
} else if (colonIndex < 0 && !pkgIsAbsolute) {
// Special case: the label "foo/bar" is synonymous with ":foo/bar".
pkg = "";
target = rawLabel.substring(startOfPackage);
} else {
pkg = stripTrailingTripleDots(rawPkg);
if (colonIndex >= 0) {
target = rawLabel.substring(colonIndex + 1);
} else {
// Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar".
// The target name is the last package segment (works even if `pkg` contains no slash)
target = pkg.substring(pkg.lastIndexOf('/') + 1);
}
}
return validateAndCreate(
repo, repoIsCanonical, pkgIsAbsolute, pkg, pkgEndsWithTripleDots, target, rawLabel);
}

private static String stripTrailingTripleDots(String pkg) {
if (pkg.endsWith("/...")) {
return pkg.substring(0, pkg.length() - 4);
}
return validateAndCreate(repo, repoIsCanonical, pkgIsAbsolute, pkg, target, rawLabel);
if (pkg.equals("...")) {
return "";
}
return pkg;
}

private static void validateRepoName(@Nullable String repo) throws LabelSyntaxException {
Expand All @@ -167,8 +190,14 @@ private static void validatePackageName(String pkg, String target) throws LabelS
}

void checkPkgIsAbsolute() throws LabelSyntaxException {
if (!pkgIsAbsolute) {
throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw);
if (!pkgIsAbsolute()) {
throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw());
}
}

void checkPkgDoesNotEndWithTripleDots() throws LabelSyntaxException {
if (pkgEndsWithTripleDots()) {
throw syntaxErrorf("invalid label '%s': package name cannot contain '...'", raw());
}
}
}
Expand All @@ -183,8 +212,12 @@ private static String perhapsYouMeantMessage(String pkg, String target) {
return pkg.endsWith('/' + target) ? " (perhaps you meant \":" + target + "\"?)" : "";
}

static String validateAndProcessTargetName(String pkg, String target)
throws LabelSyntaxException {
static String validateAndProcessTargetName(
String pkg, String target, boolean pkgEndsWithTripleDots) throws LabelSyntaxException {
if (pkgEndsWithTripleDots && target.isEmpty()) {
// Allow empty target name if the package part ends in '...'.
return target;
}
String targetError = LabelValidator.validateTargetName(target);
if (targetError != null) {
throw syntaxErrorf(
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ public static PackageIdentifier parse(String input) throws LabelSyntaxException
}
LabelParser.Parts parts = LabelParser.Parts.parse(input + ":dummy_target");
RepositoryName repoName =
parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo);
return create(repoName, PathFragment.create(parts.pkg));
parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo());
return create(repoName, PathFragment.create(parts.pkg()));
}

public RepositoryName getRepository() {
Expand Down
Loading

0 comments on commit fb4a0c2

Please sign in to comment.