Skip to content

Commit fb4a0c2

Browse files
WyveraldWyverald
authored
[6.2.0] TargetPattern parsing fixes (#17945)
* 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 #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 #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 #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
1 parent d3b59ad commit fb4a0c2

11 files changed

Lines changed: 537 additions & 353 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/cmdline/Label.java‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,12 @@ abstract static class PackageContextImpl implements PackageContext {}
121121
*/
122122
public static Label parseCanonical(String raw) throws LabelSyntaxException {
123123
Parts parts = Parts.parse(raw);
124+
parts.checkPkgDoesNotEndWithTripleDots();
124125
parts.checkPkgIsAbsolute();
125126
RepositoryName repoName =
126-
parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo);
127+
parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo());
127128
return createUnvalidated(
128-
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target);
129+
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target());
129130
}
130131

131132
public static Label parseCanonicalUnchecked(String raw) {
@@ -139,18 +140,18 @@ public static Label parseCanonicalUnchecked(String raw) {
139140
/** Computes the repo name for the label, within the context of a current repo. */
140141
private static RepositoryName computeRepoNameWithRepoContext(
141142
Parts parts, RepoContext repoContext) {
142-
if (parts.repo == null) {
143+
if (parts.repo() == null) {
143144
// Certain package names when used without a "@" part are always absolutely in the main repo,
144145
// disregarding the current repo and repo mappings.
145-
return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg)
146+
return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg())
146147
? RepositoryName.MAIN
147148
: repoContext.currentRepo();
148149
}
149-
if (parts.repoIsCanonical) {
150+
if (parts.repoIsCanonical()) {
150151
// This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar").
151-
return RepositoryName.createUnvalidated(parts.repo);
152+
return RepositoryName.createUnvalidated(parts.repo());
152153
}
153-
return repoContext.repoMapping().get(parts.repo);
154+
return repoContext.repoMapping().get(parts.repo());
154155
}
155156

156157
/**
@@ -162,10 +163,11 @@ private static RepositoryName computeRepoNameWithRepoContext(
162163
public static Label parseWithRepoContext(String raw, RepoContext repoContext)
163164
throws LabelSyntaxException {
164165
Parts parts = Parts.parse(raw);
166+
parts.checkPkgDoesNotEndWithTripleDots();
165167
parts.checkPkgIsAbsolute();
166168
RepositoryName repoName = computeRepoNameWithRepoContext(parts, repoContext);
167169
return createUnvalidated(
168-
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target);
170+
PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target());
169171
}
170172

171173
/**
@@ -178,14 +180,15 @@ public static Label parseWithRepoContext(String raw, RepoContext repoContext)
178180
public static Label parseWithPackageContext(String raw, PackageContext packageContext)
179181
throws LabelSyntaxException {
180182
Parts parts = Parts.parse(raw);
183+
parts.checkPkgDoesNotEndWithTripleDots();
181184
// pkg is either absolute or empty
182-
if (!parts.pkg.isEmpty()) {
185+
if (!parts.pkg().isEmpty()) {
183186
parts.checkPkgIsAbsolute();
184187
}
185188
RepositoryName repoName = computeRepoNameWithRepoContext(parts, packageContext);
186189
PathFragment pkgFragment =
187-
parts.pkgIsAbsolute ? PathFragment.create(parts.pkg) : packageContext.packageFragment();
188-
return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target);
190+
parts.pkgIsAbsolute() ? PathFragment.create(parts.pkg()) : packageContext.packageFragment();
191+
return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target());
189192
}
190193

191194
/**
@@ -251,7 +254,7 @@ public static Label parseAbsoluteUnchecked(String absName) {
251254
public static Label create(String packageName, String targetName) throws LabelSyntaxException {
252255
return createUnvalidated(
253256
PackageIdentifier.parse(packageName),
254-
validateAndProcessTargetName(packageName, targetName));
257+
validateAndProcessTargetName(packageName, targetName, /* pkgEndsWithTripleDots= */ false));
255258
}
256259

257260
/**
@@ -263,7 +266,10 @@ public static Label create(PackageIdentifier packageId, String targetName)
263266
throws LabelSyntaxException {
264267
return createUnvalidated(
265268
packageId,
266-
validateAndProcessTargetName(packageId.getPackageFragment().getPathString(), targetName));
269+
validateAndProcessTargetName(
270+
packageId.getPackageFragment().getPathString(),
271+
targetName,
272+
/* pkgEndsWithTripleDots= */ false));
267273
}
268274

269275
/**

‎src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java‎

Lines changed: 91 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

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

17+
import com.google.auto.value.AutoValue;
18+
import com.google.common.annotations.VisibleForTesting;
1719
import com.google.devtools.build.lib.util.StringUtilities;
1820
import com.google.errorprone.annotations.FormatMethod;
1921
import javax.annotation.Nullable;
@@ -26,82 +28,83 @@ private LabelParser() {}
2628
* Contains the parsed elements of a label string. The parts are validated (they don't contain
2729
* invalid characters). See {@link #parse} for valid label patterns.
2830
*/
29-
static final class Parts {
31+
@AutoValue
32+
abstract static class Parts {
3033
/**
3134
* The {@code @repo} or {@code @@canonical_repo} part of the string (sans any leading
3235
* {@literal @}s); can be null if it doesn't have such a part (i.e. if it doesn't start with a
3336
* {@literal @}).
3437
*/
35-
@Nullable final String repo;
38+
@Nullable
39+
abstract String repo();
3640
/**
3741
* Whether the repo part is using the canonical repo syntax (two {@literal @}s) or not (one
3842
* {@literal @}). If there is no repo part, this is false.
3943
*/
40-
final boolean repoIsCanonical;
44+
abstract boolean repoIsCanonical();
4145
/**
4246
* Whether the package part of the string is prefixed by double-slash. This can only be false if
4347
* the repo part is missing.
4448
*/
45-
final boolean pkgIsAbsolute;
46-
/** The package part of the string (sans double-slash, if any). */
47-
final String pkg;
49+
abstract boolean pkgIsAbsolute();
50+
/**
51+
* The package part of the string (sans the leading double-slash, if present; also sans the
52+
* final '...' segment, if present).
53+
*/
54+
abstract String pkg();
55+
/** Whether the package part of the string ends with a '...' segment. */
56+
abstract boolean pkgEndsWithTripleDots();
4857
/** The target part of the string (sans colon). */
49-
final String target;
58+
abstract String target();
5059
/** The original unparsed raw string. */
51-
final String raw;
60+
abstract String raw();
5261

53-
private Parts(
54-
@Nullable String repo,
55-
boolean repoIsCanonical,
56-
boolean pkgIsAbsolute,
57-
String pkg,
58-
String target,
59-
String raw) {
60-
this.repo = repo;
61-
this.repoIsCanonical = repoIsCanonical;
62-
this.pkgIsAbsolute = pkgIsAbsolute;
63-
this.pkg = pkg;
64-
this.target = target;
65-
this.raw = raw;
66-
}
67-
68-
private static Parts validateAndCreate(
62+
@VisibleForTesting
63+
static Parts validateAndCreate(
6964
@Nullable String repo,
7065
boolean repoIsCanonical,
7166
boolean pkgIsAbsolute,
7267
String pkg,
68+
boolean pkgEndsWithTripleDots,
7369
String target,
7470
String raw)
7571
throws LabelSyntaxException {
7672
validateRepoName(repo);
7773
validatePackageName(pkg, target);
78-
return new Parts(
74+
return new AutoValue_LabelParser_Parts(
7975
repo,
8076
repoIsCanonical,
8177
pkgIsAbsolute,
8278
pkg,
83-
validateAndProcessTargetName(pkg, target),
79+
pkgEndsWithTripleDots,
80+
validateAndProcessTargetName(pkg, target, pkgEndsWithTripleDots),
8481
raw);
8582
}
8683

8784
/**
8885
* Parses a raw label string into parts. The logic can be summarized by the following table:
8986
*
90-
* {@code
91-
* raw | repo | repoIsCanonical | pkgIsAbsolute | pkg | target
92-
* ----------------------+--------+-----------------+---------------+-----------+-----------
93-
* foo/bar | null | false | false | "" | "foo/bar"
94-
* //foo/bar | null | false | true | "foo/bar" | "bar"
95-
* @repo | "repo" | false | true | "" | "repo"
96-
* @@repo | "repo" | true | true | "" | "repo"
97-
* @repo//foo/bar | "repo" | false | true | "foo/bar" | "bar"
98-
* @@repo//foo/bar | "repo" | true | true | "foo/bar" | "bar"
99-
* :quux | null | false | false | "" | "quux"
100-
* foo/bar:quux | null | false | false | "foo/bar" | "quux"
101-
* //foo/bar:quux | null | false | true | "foo/bar" | "quux"
102-
* @repo//foo/bar:quux | "repo" | false | true | "foo/bar" | "quux"
103-
* @@repo//foo/bar:quux | "repo" | true | true | "foo/bar" | "quux"
104-
* }
87+
* <pre>{@code
88+
* raw | repo | repoIs- | pkgIs- | pkg | pkgEndsWith- | target
89+
* | | Canonical | Absolute | | TripleDots |
90+
* ----------------------+--------+-----------+----------+-----------+--------------+-----------
91+
* "foo/bar" | null | false | false | "" | false | "foo/bar"
92+
* "..." | null | false | false | "" | true | ""
93+
* "...:all" | null | false | false | "" | true | "all"
94+
* "foo/..." | null | false | false | "foo" | true | ""
95+
* "//foo/bar" | null | false | true | "foo/bar" | false | "bar"
96+
* "//foo/..." | null | false | true | "foo" | true | ""
97+
* "//foo/...:all" | null | false | true | "foo" | true | "all"
98+
* "//foo/all" | null | false | true | "foo/all" | false | "all"
99+
* "@repo" | "repo" | false | true | "" | false | "repo"
100+
* "@@repo" | "repo" | true | true | "" | false | "repo"
101+
* "@repo//foo/bar" | "repo" | false | true | "foo/bar" | false | "bar"
102+
* "@@repo//foo/bar" | "repo" | true | true | "foo/bar" | false | "bar"
103+
* ":quux" | null | false | false | "" | false | "quux"
104+
* "foo/bar:quux" | null | false | false | "foo/bar" | false | "quux"
105+
* "//foo/bar:quux" | null | false | true | "foo/bar" | false | "quux"
106+
* "@repo//foo/bar:quux" | "repo" | false | true | "foo/bar" | false | "quux"
107+
* }</pre>
105108
*/
106109
static Parts parse(String rawLabel) throws LabelSyntaxException {
107110
@Nullable final String repo;
@@ -116,9 +119,10 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
116119
return validateAndCreate(
117120
repo,
118121
repoIsCanonical,
119-
/*pkgIsAbsolute=*/ true,
120-
/*pkg=*/ "",
121-
/*target=*/ repo,
122+
/* pkgIsAbsolute= */ true,
123+
/* pkg= */ "",
124+
/* pkgEndsWithTripleDots= */ false,
125+
/* target= */ repo,
122126
rawLabel);
123127
} else {
124128
repo = rawLabel.substring(repoIsCanonical ? 2 : 1, doubleSlashIndex);
@@ -136,20 +140,39 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
136140
final String pkg;
137141
final String target;
138142
final int colonIndex = rawLabel.indexOf(':', startOfPackage);
139-
if (colonIndex >= 0) {
140-
pkg = rawLabel.substring(startOfPackage, colonIndex);
141-
target = rawLabel.substring(colonIndex + 1);
142-
} else if (pkgIsAbsolute) {
143-
// Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar".
144-
pkg = rawLabel.substring(startOfPackage);
145-
// The target name is the last package segment (works even if `pkg` contains no slash)
146-
target = pkg.substring(pkg.lastIndexOf('/') + 1);
147-
} else {
143+
final String rawPkg =
144+
rawLabel.substring(startOfPackage, colonIndex >= 0 ? colonIndex : rawLabel.length());
145+
final boolean pkgEndsWithTripleDots = rawPkg.endsWith("/...") || rawPkg.equals("...");
146+
if (colonIndex < 0 && pkgEndsWithTripleDots) {
147+
// Special case: if the entire label ends in '...', the target name is empty.
148+
pkg = stripTrailingTripleDots(rawPkg);
149+
target = "";
150+
} else if (colonIndex < 0 && !pkgIsAbsolute) {
148151
// Special case: the label "foo/bar" is synonymous with ":foo/bar".
149152
pkg = "";
150153
target = rawLabel.substring(startOfPackage);
154+
} else {
155+
pkg = stripTrailingTripleDots(rawPkg);
156+
if (colonIndex >= 0) {
157+
target = rawLabel.substring(colonIndex + 1);
158+
} else {
159+
// Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar".
160+
// The target name is the last package segment (works even if `pkg` contains no slash)
161+
target = pkg.substring(pkg.lastIndexOf('/') + 1);
162+
}
163+
}
164+
return validateAndCreate(
165+
repo, repoIsCanonical, pkgIsAbsolute, pkg, pkgEndsWithTripleDots, target, rawLabel);
166+
}
167+
168+
private static String stripTrailingTripleDots(String pkg) {
169+
if (pkg.endsWith("/...")) {
170+
return pkg.substring(0, pkg.length() - 4);
151171
}
152-
return validateAndCreate(repo, repoIsCanonical, pkgIsAbsolute, pkg, target, rawLabel);
172+
if (pkg.equals("...")) {
173+
return "";
174+
}
175+
return pkg;
153176
}
154177

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

169192
void checkPkgIsAbsolute() throws LabelSyntaxException {
170-
if (!pkgIsAbsolute) {
171-
throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw);
193+
if (!pkgIsAbsolute()) {
194+
throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw());
195+
}
196+
}
197+
198+
void checkPkgDoesNotEndWithTripleDots() throws LabelSyntaxException {
199+
if (pkgEndsWithTripleDots()) {
200+
throw syntaxErrorf("invalid label '%s': package name cannot contain '...'", raw());
172201
}
173202
}
174203
}
@@ -183,8 +212,12 @@ private static String perhapsYouMeantMessage(String pkg, String target) {
183212
return pkg.endsWith('/' + target) ? " (perhaps you meant \":" + target + "\"?)" : "";
184213
}
185214

186-
static String validateAndProcessTargetName(String pkg, String target)
187-
throws LabelSyntaxException {
215+
static String validateAndProcessTargetName(
216+
String pkg, String target, boolean pkgEndsWithTripleDots) throws LabelSyntaxException {
217+
if (pkgEndsWithTripleDots && target.isEmpty()) {
218+
// Allow empty target name if the package part ends in '...'.
219+
return target;
220+
}
188221
String targetError = LabelValidator.validateTargetName(target);
189222
if (targetError != null) {
190223
throw syntaxErrorf(

‎src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,8 @@ public static PackageIdentifier parse(String input) throws LabelSyntaxException
121121
}
122122
LabelParser.Parts parts = LabelParser.Parts.parse(input + ":dummy_target");
123123
RepositoryName repoName =
124-
parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo);
125-
return create(repoName, PathFragment.create(parts.pkg));
124+
parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo());
125+
return create(repoName, PathFragment.create(parts.pkg()));
126126
}
127127

128128
public RepositoryName getRepository() {

0 commit comments

Comments
 (0)