Allow : in target and filenames if not the first character

site/en/concepts/labels.md

@@ -136,7 +136,8 @@ file; the name of a file is its pathname relative to the directory containing
 the `BUILD` file.
 
 Target names must be composed entirely of characters drawn from the set `a`–`z`,
-`A`–`Z`, `0`–`9`, and the punctuation symbols `!%-@^_"#$&'()*-+,;<=>?[]{|}~/.`.
+`A`–`Z`, `0`–`9`, and the punctuation symbols `!%-@^_"#$&'()*-+,:;<=>?[]{|}~/.`.
+They must not start with `:`.
 
 Filenames must be relative pathnames in normal form, which means they must
 neither start nor end with a slash (for example, `/foo` and `foo/` are

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

@@ -101,7 +101,9 @@ static Parts validateAndCreate(
      * "@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"
+     * ":qu:ux"              | null   | false     | false    | ""        | false        | "qu:ux"
      * "foo/bar:quux"        | null   | false     | false    | "foo/bar" | false        | "quux"
+     * "foo/bar:qu:ux"       | null   | false     | false    | "foo/bar" | false        | "qu:ux"
      * "//foo/bar:quux"      | null   | false     | true     | "foo/bar" | false        | "quux"
      * "@repo//foo/bar:quux" | "repo" | false     | true     | "foo/bar" | false        | "quux"
      * }</pre>

src/main/java/com/google/devtools/build/lib/cmdline/LabelValidator.java

@@ -26,7 +26,6 @@ public final class LabelValidator {
 
   // Target names allow all 7-bit ASCII characters except
   // 0-31 (control characters)
-  // 58 ':' (colon)
   // 92 '\' (backslash) - directory separator (on Windows); may be allowed in the future
   // 127 (delete)
   /** Matches punctuation in target names which requires quoting in a blaze query. */
@@ -36,11 +35,11 @@ public final class LabelValidator {
   /**
    * Matches punctuation in target names which doesn't require quoting in a blaze query.
    *
-   * Note that . is also allowed in target names, and doesn't require quoting, but has restrictions
-   * on its surrounding characters; see {@link #validateTargetName(String)}.
+   * <p>Note that . is also allowed in target names, and doesn't require quoting, but has
+   * restrictions on its surrounding characters; see {@link #validateTargetName(String)}. The same
+   * applies to :.
    */
-  private static final CharMatcher PUNCTUATION_NOT_REQUIRING_QUOTING =
-      CharMatcher.anyOf("!%-@^_`");
+  private static final CharMatcher PUNCTUATION_NOT_REQUIRING_QUOTING = CharMatcher.anyOf("!%-@^_`");
 
   // Package names allow all 7-bit ASCII characters except
   // 0-31 (control characters)
@@ -52,7 +51,7 @@ public final class LabelValidator {
       CharMatcher.inRange('0', '9')
           .or(CharMatcher.inRange('a', 'z'))
           .or(CharMatcher.inRange('A', 'Z'))
-          .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./;<=>?@[]^_`{|}~"))
+          .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./:;<=>?@[]^_`{|}~"))
           .precomputed();
 
   /**
@@ -153,6 +152,8 @@ public static String validateTargetName(String targetName) {
     char c = targetName.charAt(0);
     if (c == '/') {
       return "target names may not start with '/'";
+    } else if (c == ':') {
+      return "target names may not start with ':'";
     } else if (c == '.') {
       if (targetName.startsWith("../") || targetName.equals("..")) {
         return "target names may not contain up-level references '..'";
@@ -174,7 +175,7 @@ public static String validateTargetName(String targetName) {
       if (ALWAYS_ALLOWED_TARGET_CHARACTERS.matches(c)) {
         continue;
       }
-      if (c == '.') {
+      if (c == '.' || c == ':') {
         continue;
       }
       if (c == '/') {

src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java

@@ -68,8 +68,7 @@ public static Rule createRule(
     String name = (String) nameObject;
     Label label;
     try {
-      // Test that this would form a valid label name -- in particular, this
-      // catches cases where Makefile variables $(foo) appear in "name".
+      // Test that this would form a valid label name.
       label = pkgBuilder.createLabel(name);
     } catch (LabelSyntaxException e) {
       throw new InvalidRuleException("illegal rule name: " + name + ": " + e.getMessage());

src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java

@@ -117,8 +117,8 @@ public Sequence<?> glob(
 
     ArrayList<String> result = new ArrayList<>(matches.size());
     for (String match : matches) {
-      if (match.charAt(0) == '@') {
-        // Add explicit colon to disambiguate from external repository.
+      if (match.charAt(0) == '@' || match.contains(":")) {
+        // Add explicit colon to disambiguate from external repository or target reference.
         match = ":" + match;
       }
       result.add(match);

src/test/java/com/google/devtools/build/lib/cmdline/LabelParserTest.java

@@ -56,8 +56,12 @@ public void parserTable() throws Exception {
             validateAndCreate("repo", true, true, "foo/bar", false, "bar", "@@repo//foo/bar"));
     assertThat(parse(":quux"))
         .isEqualTo(validateAndCreate(null, false, false, "", false, "quux", ":quux"));
+    assertThat(parse(":qu:ux"))
+        .isEqualTo(validateAndCreate(null, false, false, "", false, "qu:ux", ":qu:ux"));
     assertThat(parse("foo/bar:quux"))
         .isEqualTo(validateAndCreate(null, false, false, "foo/bar", false, "quux", "foo/bar:quux"));
+    assertThat(parse("foo/bar:qu:ux"))
+        .isEqualTo(validateAndCreate(null, false, false, "foo/bar", false, "qu:ux", "foo/bar:qu:ux"));
     assertThat(parse("//foo/bar:quux"))
         .isEqualTo(
             validateAndCreate(null, false, true, "foo/bar", false, "quux", "//foo/bar:quux"));

src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java

@@ -68,7 +68,6 @@ public void testValidatePackageName() throws Exception {
     assertThat(LabelValidator.validatePackageName("foo,bar")).isNull();
     assertThat(LabelValidator.validatePackageName("foo-bar")).isNull();
     assertThat(LabelValidator.validatePackageName("foo.bar")).isNull();
-    assertThat(LabelValidator.validatePackageName("foo+bar")).isNull();
     assertThat(LabelValidator.validatePackageName("foo;bar")).isNull();
     assertThat(LabelValidator.validatePackageName("foo<bar")).isNull();
     assertThat(LabelValidator.validatePackageName("foo=bar")).isNull();
@@ -145,13 +144,13 @@ public void testValidateTargetName() throws Exception {
 
     assertThat(LabelValidator.validateTargetName("foo/bar")).isNull();
     assertThat(LabelValidator.validateTargetName("foo@bar")).isNull();
+    assertThat(LabelValidator.validateTargetName("bar:baz")).isNull();
+    assertThat(LabelValidator.validateTargetName("bar:")).isNull();
 
     assertThat(LabelValidator.validateTargetName("foo/"))
         .isEqualTo("target names may not end with '/'");
-    assertThat(LabelValidator.validateTargetName("bar:baz"))
-        .isEqualTo("target names may not contain ':'");
-    assertThat(LabelValidator.validateTargetName("bar:"))
-        .isEqualTo("target names may not contain ':'");
+    assertThat(LabelValidator.validateTargetName(":foo"))
+        .isEqualTo("target names may not start with ':'");
   }
 
   @Test
@@ -176,6 +175,8 @@ public void testValidateAbsoluteLabel() throws Exception {
         .isEqualTo(new PackageAndTarget("@foo", "@foo"));
     assertThat(LabelValidator.validateAbsoluteLabel("//@foo:@bar"))
         .isEqualTo(new PackageAndTarget("@foo", "@bar"));
+    assertThat(LabelValidator.validateAbsoluteLabel("@repo//f$( )oo:b$() :ar"))
+        .isEqualTo(new PackageAndTarget("f$( )oo", "b$() :ar"));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java

@@ -531,20 +531,24 @@ public void testGlobOrderStableWithNonSkyframeAndSkyframeComponents() throws Exc
   }
 
   @Test
-  public void globEscapesAt() throws Exception {
+  public void globEscapesAtAndColon() throws Exception {
     scratch.file("foo/BUILD", "filegroup(name = 'foo', srcs = glob(['*.txt']))");
     scratch.file("foo/@f.txt");
+    scratch.file("foo/f@.txt");
+    scratch.file("foo/f:.txt");
     preparePackageLoading(rootDirectory);
     SkyKey skyKey = PackageIdentifier.createInMainRepo("foo");
-    assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:@f.txt");
+    assertSrcs(
+        validPackageWithoutErrors(skyKey), "foo", "//foo:@f.txt", "//foo:f:.txt", "//foo:f@.txt");
 
     scratch.overwriteFile("foo/BUILD", "filegroup(name = 'foo', srcs = glob(['*.txt'])) # comment");
     getSkyframeExecutor()
         .invalidateFilesUnderPathForTesting(
             reporter,
             ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
             Root.fromPath(rootDirectory));
-    assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:@f.txt");
+    assertSrcs(
+        validPackageWithoutErrors(skyKey), "foo", "//foo:@f.txt", "//foo:f:.txt", "//foo:f@.txt");
   }
 
   /**