Allow ' ', '(', ')' and '$' in labels

This just add the special characters in labels and fixes the
associated tests, left is the hard part to test adding
those characters everywhere.

This is experimental and several characters will break at several
location especial in the runfiles manifest file.

Follow-ups: Resolve quoting then test, test more and add even more tests.

Issue found during development:
  Parentheses are not accepted in exclude pattern in globs
  Building a binary includes build-runfiles that relies on the runfiles
  manifest format so the added test would fails with a java_binary
  instead of a library.

--
Change-Id: I9c87273a90318b931c61bdb86f1066962819960a
Reviewed-on: https://cr.bazel.build/9055
PiperOrigin-RevId: 149108027
MOS_MIGRATED_REVID=149108027

6 files changed, 23 insertions, 31 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
index 3766a76755..69805058d7 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
@@ -279,8 +279,6 @@ public class LabelTest {
 
   @Test
   public void testBadCharacters() throws Exception {
-    assertSyntaxError("package names may contain only",
-                      "//foo/bar baz");
     assertSyntaxError("target names may not contain ':'",
                       "//foo:bar:baz");
     assertSyntaxError("target names may not contain ':'",
@@ -289,12 +287,6 @@ public class LabelTest {
                       "//foo/bar::");
     assertSyntaxError("target names may not contain '&'",
                       "//foo:bar&");
-    assertSyntaxError("target names may not contain '$'",
-                      "//foo/bar:baz$a");
-    assertSyntaxError("target names may not contain '('",
-                      "//foo/bar:baz(foo)");
-    assertSyntaxError("target names may not contain ')'",
-                      "//foo/bar:bazfoo)");
     // Warning: if these assertions are false, tools that assume that they can safely quote labels
     // may need to be fixed. Please consult with bazel-dev before loosening these restrictions.
     assertSyntaxError("target names may not contain '''", "//foo/bar:baz'foo");
@@ -357,6 +349,7 @@ public class LabelTest {
     Label.parseAbsolute("//package:foo.bar");
     Label.parseAbsolute("//package:foo@bar");
     Label.parseAbsolute("//package:foo~bar");
+    Label.parseAbsolute("//$( ):$( )");
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
index fcf887a5e2..17e6ba5a12 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
@@ -53,17 +53,15 @@ public class LabelValidatorTest {
     assertNull(LabelValidator.validatePackageName("a/.b"));
     assertNull(LabelValidator.validatePackageName("a/b."));
     assertNull(LabelValidator.validatePackageName("a/b.."));
+    assertNull(LabelValidator.validatePackageName("a$( )/b.."));
 
     // Bad:
     assertEquals(
         "package names may not start with '/'", LabelValidator.validatePackageName("/foo"));
     assertEquals("package names may not end with '/'", LabelValidator.validatePackageName("foo/"));
-    assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("bar baz"));
     assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("foo:bar"));
     assertEquals(
         LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("baz@12345"));
-    assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("baz(foo)"));
-    assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("bazfoo)"));
 
     assertEquals(
         LabelValidator.PACKAGE_NAME_DOT_ERROR, LabelValidator.validatePackageName("bar/../baz"));
@@ -97,20 +95,12 @@ public class LabelValidatorTest {
 
     assertEquals("target names may not end with '/'",
                  LabelValidator.validateTargetName("foo/"));
-    assertEquals("target names may not contain ' '",
-                 LabelValidator.validateTargetName("bar baz"));
     assertEquals("target names may not contain ':'",
                  LabelValidator.validateTargetName("bar:baz"));
     assertEquals("target names may not contain ':'",
                  LabelValidator.validateTargetName("bar:"));
     assertEquals("target names may not contain '&'",
                  LabelValidator.validateTargetName("bar&"));
-    assertEquals("target names may not contain '$'",
-                 LabelValidator.validateTargetName("baz$a"));
-    assertEquals("target names may not contain '('",
-                 LabelValidator.validateTargetName("baz(foo)"));
-    assertEquals("target names may not contain ')'",
-                 LabelValidator.validateTargetName("bazfoo)"));
   }
 
   @Test
@@ -122,6 +112,13 @@ public class LabelValidatorTest {
         LabelValidator.validateAbsoluteLabel("@repo//foo:bar"));
     assertEquals(new PackageAndTarget("foo", "bar"),
         LabelValidator.validateAbsoluteLabel("@//foo:bar"));
+    emptyPackage = new PackageAndTarget("", "b$() ar");
+    assertEquals(emptyPackage, LabelValidator.validateAbsoluteLabel("//:b$() ar"));
+    assertEquals(emptyPackage, LabelValidator.validateAbsoluteLabel("@repo//:b$() ar"));
+    assertEquals(new PackageAndTarget("f$( )oo", "b$() ar"),
+        LabelValidator.validateAbsoluteLabel("@repo//f$( )oo:b$() ar"));
+    assertEquals(new PackageAndTarget("f$( )oo", "b$() ar"),
+        LabelValidator.validateAbsoluteLabel("@//f$( )oo:b$() ar"));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
index 978f203e86..c5b0d31544 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
@@ -294,14 +294,13 @@ public class BuildTypeTest {
   @Test
   public void testSelectorWrongType() throws Exception {
     ImmutableMap<String, String> input = ImmutableMap.of(
-        "//conditions:a", "not a label",
-        "//conditions:b", "also not a label",
+        "//conditions:a", "not a/../label", "//conditions:b", "also not a/../label",
         BuildType.Selector.DEFAULT_CONDITION_KEY, "whatever");
     try {
       new Selector<>(input, null, currentRule, BuildType.LABEL);
       fail("Expected Selector instantiation to fail since the input isn't a selection of labels");
     } catch (ConversionException e) {
-      assertThat(e.getMessage()).contains("invalid label 'not a label'");
+      assertThat(e.getMessage()).contains("invalid label 'not a/../label'");
     }
   }
 
@@ -311,13 +310,13 @@ public class BuildTypeTest {
   @Test
   public void testSelectorKeyIsNotALabel() throws Exception {
     ImmutableMap<String, String> input = ImmutableMap.of(
-        "not a label", "//a:a",
+        "not a/../label", "//a:a",
         BuildType.Selector.DEFAULT_CONDITION_KEY, "whatever");
     try {
       new Selector<>(input, null, currentRule, BuildType.LABEL);
       fail("Expected Selector instantiation to fail since the key isn't a label");
     } catch (ConversionException e) {
-      assertThat(e.getMessage()).contains("invalid label 'not a label'");
+      assertThat(e.getMessage()).contains("invalid label 'not a/../label'");
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 836ed6c8da..3573827c81 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -114,13 +114,16 @@ public class PackageFactoryTest extends PackageFactoryTestBase {
   @Test
   public void testBadPackageName() throws Exception {
     try {
-      packages.createPackage("not even a legal label", emptyBuildFile("not even a legal label"));
+      // PathFragment parsing de-double slashes, and normalization of the path fragment removes
+      // up reference (/../), so use triple dot /.../ that will always be a forbidden package name.
+      packages.createPackage("not even a legal/.../label",
+          emptyBuildFile("not even a legal/.../label"));
       fail();
     } catch (NoSuchPackageException e) {
       assertThat(e.getMessage())
           .contains(
-              "no such package 'not even a legal label': "
-                  + "illegal package name: 'not even a legal label' ");
+              "no such package 'not even a legal/.../label': "
+                  + "illegal package name: 'not even a legal/.../label' ");
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 2966fc8fd7..f8b3d16621 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -801,8 +801,8 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   @Test
   public void testLabelGetRelativeSyntaxError() throws Exception {
     checkErrorContains(
-        "invalid target name 'bad syntax': target names may not contain ' '",
-        "Label('//foo:bar').relative('bad syntax')");
+        "invalid target name 'bad//syntax': target names may not contain '//' path separators",
+        "Label('//foo:bar').relative('bad//syntax')");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
index a4e42c7a32..26d9a3955b 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
@@ -256,10 +256,10 @@ public class TypeTest {
   @Test
   public void testInvalidLabel() throws Exception {
     try {
-      BuildType.LABEL.convert("not a label", null, currentRule);
+      BuildType.LABEL.convert("not//a label", null, currentRule);
       fail();
     } catch (Type.ConversionException e) {
-      MoreAsserts.assertContainsWordsWithQuotes(e.getMessage(), "not a label");
+      MoreAsserts.assertContainsWordsWithQuotes(e.getMessage(), "not//a label");
     }
   }