Automated rollback of commit 5a9befc.

RELNOTES: None

*** Reason for rollback ***

PiperOrigin-RevId: 201686843

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

@@ -155,22 +155,17 @@ public static Label parseAbsolute(
       repo = absName;
       absName = "//:" + absName.substring(1);
     }
-    String error = RepositoryName.validate(repo);
-    if (error != null) {
-      throw new LabelSyntaxException(
-          "invalid repository name '" + StringUtilities.sanitizeControlChars(repo) + "': " + error);
-    }
     try {
       LabelValidator.PackageAndTarget labelParts = LabelValidator.parseAbsoluteLabel(absName);
-      PackageIdentifier pkgId =
-          validatePackageName(
-              labelParts.getPackageName(), labelParts.getTargetName(), repo, repositoryMapping);
-      PathFragment packageFragment = pkgId.getPackageFragment();
+      PackageIdentifier pkgIdWithoutRepo =
+          validatePackageName(labelParts.getPackageName(), labelParts.getTargetName());
+      PathFragment packageFragment = pkgIdWithoutRepo.getPackageFragment();
       if (repo.isEmpty() && ABSOLUTE_PACKAGE_NAMES.contains(packageFragment)) {
-        pkgId =
-            PackageIdentifier.create(getGlobalRepoName("@", repositoryMapping), packageFragment);
+        repo = "@";
       }
-      return create(pkgId, labelParts.getTargetName());
+      RepositoryName globalRepoName = getGlobalRepoName(repo, repositoryMapping);
+      return create(
+          PackageIdentifier.create(globalRepoName, packageFragment), labelParts.getTargetName());
     } catch (BadLabelException e) {
       throw new LabelSyntaxException(e.getMessage());
     }
@@ -294,25 +289,15 @@ private static String validateTargetName(PackageIdentifier packageIdentifier, St
     return name;
   }
 
-  private static PackageIdentifier validatePackageName(String packageIdentifier, String name)
-      throws LabelSyntaxException {
-    return validatePackageName(
-        packageIdentifier, name, /* repo= */ null, /* repositoryMapping= */ null);
-  }
-
   /**
    * Validates the given package name and returns a canonical {@link PackageIdentifier} instance if
    * it is valid. Otherwise it throws a SyntaxException.
    */
-  private static PackageIdentifier validatePackageName(
-      String packageIdentifier,
-      String name,
-      String repo,
-      ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
+  private static PackageIdentifier validatePackageName(String packageIdentifier, String name)
       throws LabelSyntaxException {
     String error = null;
     try {
-      return PackageIdentifier.parse(packageIdentifier, repo, repositoryMapping);
+      return PackageIdentifier.parse(packageIdentifier);
     } catch (LabelSyntaxException e) {
       error = e.getMessage();
       error = "invalid package name '" + packageIdentifier + "': " + error;

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

@@ -45,14 +45,15 @@ public final class LabelValidator {
   // Package names allow all 7-bit ASCII characters except
   // 0-31 (control characters)
   // 58 ':' (colon) - target name separator
+  // 64 '@' (at-sign) - workspace name prefix
   // 92 '\' (backslash) - directory separator (on Windows); may be allowed in the future
   // 127 (delete)
   /** Matches characters allowed in package name. */
   private static final CharMatcher ALLOWED_CHARACTERS_IN_PACKAGE_NAME =
       CharMatcher.inRange('0', '9')
           .or(CharMatcher.inRange('a', 'z'))
           .or(CharMatcher.inRange('A', 'Z'))
-          .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./;<=>?@[]^_`{|}~"))
+          .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./;<=>?[]^_`{|}~"))
           .precomputed();
 
   /**
@@ -70,7 +71,7 @@ public final class LabelValidator {
   @VisibleForTesting
   static final String PACKAGE_NAME_ERROR =
       "package names may contain A-Z, a-z, 0-9, or any of ' !\"#$%&'()*+,-./;<=>?[]^_`{|}~'"
-          + " (most 127-bit ascii characters except 0-31, 127, ':', or '\\')";
+          + " (most 127-bit ascii characters except 0-31, 127, ':', '@', or '\\')";
 
   @VisibleForTesting
   static final String PACKAGE_NAME_DOT_ERROR =

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

@@ -16,7 +16,6 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ComparisonChain;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -120,17 +119,10 @@ private PackageIdentifier(RepositoryName repository, PathFragment pkgName) {
   }
 
   public static PackageIdentifier parse(String input) throws LabelSyntaxException {
-    return parse(input, /* repo= */ null, /* repositoryMapping= */ null);
-  }
-
-  public static PackageIdentifier parse(
-      String input, String repo, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
-      throws LabelSyntaxException {
+    String repo;
     String packageName;
     int packageStartPos = input.indexOf("//");
-    if (repo != null) {
-      packageName = input;
-    } else if (input.startsWith("@") && packageStartPos > 0) {
+    if (input.startsWith("@") && packageStartPos > 0) {
       repo = input.substring(0, packageStartPos);
       packageName = input.substring(packageStartPos + 2);
     } else if (input.startsWith("@")) {
@@ -153,13 +145,7 @@ public static PackageIdentifier parse(
       throw new LabelSyntaxException(error);
     }
 
-    if (repositoryMapping != null) {
-      RepositoryName repositoryName = RepositoryName.create(repo);
-      repositoryName = repositoryMapping.getOrDefault(repositoryName, repositoryName);
-      return create(repositoryName, PathFragment.create(packageName));
-    } else {
-      return create(repo, PathFragment.create(packageName));
-    }
+    return create(repo, PathFragment.create(packageName));
   }
 
   public RepositoryName getRepository() {

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

@@ -31,6 +31,8 @@
 @RunWith(JUnit4.class)
 public class LabelTest {
 
+  private static final String BAD_PACKAGE_CHARS =
+      "package names may contain A-Z, a-z, 0-9, or any of";
   private static final String INVALID_TARGET_NAME = "invalid target name";
   private static final String INVALID_PACKAGE_NAME = "invalid package name";
 
@@ -57,18 +59,6 @@ public void testAbsolute() throws Exception {
       assertThat(l.getPackageName()).isEmpty();
       assertThat(l.getName()).isEqualTo("foo");
     }
-    {
-      Label l = Label.parseAbsolute("//@foo");
-      assertThat(l.getPackageIdentifier().getRepository().getName()).isEqualTo("@");
-      assertThat(l.getPackageName()).isEqualTo("@foo");
-      assertThat(l.getName()).isEqualTo("@foo");
-    }
-    {
-      Label l = Label.parseAbsolute("//xyz/@foo:abc");
-      assertThat(l.getPackageIdentifier().getRepository().getName()).isEqualTo("@");
-      assertThat(l.getPackageName()).isEqualTo("xyz/@foo");
-      assertThat(l.getName()).isEqualTo("abc");
-    }
   }
 
   private static String parseCommandLine(String label, String prefix) throws LabelSyntaxException {
@@ -374,6 +364,16 @@ public void testSomeGoodLabels() throws Exception {
     Label.parseAbsolute("//$( ):$( )");
   }
 
+  /**
+   * Regression test: we previously expanded the set of characters which are considered label chars
+   * to include "@" (see test above). An unexpected side-effect is that "@D" in genrule(cmd) was
+   * considered to be a valid relative label! The fix is to forbid "@x" in package names.
+   */
+  @Test
+  public void testAtVersionIsIllegal() throws Exception {
+    assertSyntaxError(BAD_PACKAGE_CHARS, "//foo/bar@123:baz");
+  }
+
   @Test
   public void testDoubleSlashPathSeparator() throws Exception {
     assertSyntaxError("package names may not contain '//' path separators",
@@ -443,21 +443,8 @@ public void testInvalidRepo() throws Exception {
       Label.parseAbsolute("foo//bar/baz:bat/boo");
       fail();
     } catch (LabelSyntaxException e) {
-      assertThat(e)
-          .hasMessageThat()
-          .isEqualTo("invalid repository name 'foo': workspace names must start with '@'");
-    }
-  }
-
-  @Test
-  public void testInvalidRepoWithColon() throws Exception {
-    try {
-      Label.parseAbsolute("@foo:xyz");
-      fail();
-    } catch (LabelSyntaxException e) {
-      assertThat(e)
-          .hasMessageThat()
-          .containsMatch("invalid repository name '@foo:xyz': workspace names may contain only");
+      assertThat(e).hasMessage(
+          "invalid repository name 'foo': workspace names must start with '@'");
     }
   }
 

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

@@ -74,7 +74,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();
@@ -92,6 +91,8 @@ public void testValidatePackageName() throws Exception {
         .isEqualTo("package names may not end with '/'");
     assertThat(LabelValidator.validatePackageName("foo:bar"))
         .isEqualTo(LabelValidator.PACKAGE_NAME_ERROR);
+    assertThat(LabelValidator.validatePackageName("baz@12345"))
+        .isEqualTo(LabelValidator.PACKAGE_NAME_ERROR);
 
     assertThat(LabelValidator.validatePackageName("bar/../baz"))
         .isEqualTo(LabelValidator.PACKAGE_NAME_DOT_ERROR);
@@ -170,12 +171,6 @@ public void testValidateAbsoluteLabel() throws Exception {
         .isEqualTo(new PackageAndTarget("f$( )oo", "b$() ar"));
     assertThat(LabelValidator.validateAbsoluteLabel("@//f$( )oo:b$() ar"))
         .isEqualTo(new PackageAndTarget("f$( )oo", "b$() ar"));
-    assertThat(LabelValidator.validateAbsoluteLabel("//f@oo"))
-        .isEqualTo(new PackageAndTarget("f@oo", "f@oo"));
-    assertThat(LabelValidator.validateAbsoluteLabel("//@foo"))
-        .isEqualTo(new PackageAndTarget("@foo", "@foo"));
-    assertThat(LabelValidator.validateAbsoluteLabel("//@foo:@bar"))
-        .isEqualTo(new PackageAndTarget("@foo", "@bar"));
   }
 
   @Test

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

@@ -50,7 +50,7 @@ public void testPassingValidations() throws TargetParsingException {
   @Test
   public void testInvalidPatterns() throws TargetParsingException {
     try {
-      parse("Bar\\java");
+      parse("Bar@java");
       fail();
     } catch (TargetParsingException expected) {
     }

src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java

@@ -499,8 +499,7 @@ public void assertInvalidIncludeDirectoryMessage(String entry, String messageReg
   @Test
   public void testInvalidIncludeDirectory() throws Exception {
     assertInvalidIncludeDirectoryMessage("%package(//a", "has an unrecognized %prefix%");
-    assertInvalidIncludeDirectoryMessage(
-        "%package(//a:@@a)%", "The package '//a:@@a' is not valid");
+    assertInvalidIncludeDirectoryMessage("%package(//a@@a)%", "The package '//a@@a' is not valid");
     assertInvalidIncludeDirectoryMessage(
         "%package(//a)%foo", "The path in the package.*is not valid");
     assertInvalidIncludeDirectoryMessage(