Ensure repository names are valid workspace names

RELNOTES: Repository rules must use names that are valid workspace names.

--
MOS_MIGRATED_REVID=113197588

4 files changed, 32 insertions, 19 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
index b771763eb2..ebafff7355 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
@@ -31,7 +31,7 @@ import java.util.Map;
  */
 public class ExternalPackageBuilder {
 
-  public ExternalPackageBuilder createAndAddRepositoryRule(
+  public Rule createAndAddRepositoryRule(
       Package.Builder pkg,
       RuleClass ruleClass,
       RuleClass bindRuleClass,
@@ -42,17 +42,17 @@ public class ExternalPackageBuilder {
 
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
-    Rule tempRule =
+    Rule rule =
         RuleFactory.createRule(
             pkg, ruleClass, attributeValues, eventHandler, ast, ast.getLocation(), /*env=*/ null);
     pkg.addEvents(eventHandler.getEvents());
-    overwriteRule(pkg, tempRule);
+    overwriteRule(pkg, rule);
     for (Map.Entry<String, Label> entry :
-        ruleClass.getExternalBindingsFunction().apply(tempRule).entrySet()) {
+        ruleClass.getExternalBindingsFunction().apply(rule).entrySet()) {
       Label nameLabel = Label.parseAbsolute("//external:" + entry.getKey());
-      addBindRule(pkg, bindRuleClass, nameLabel, entry.getValue(), tempRule.getLocation());
+      addBindRule(pkg, bindRuleClass, nameLabel, entry.getValue(), rule.getLocation());
     }
-    return this;
+    return rule;
   }
 
   public void addBindRule(
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index d48aa7316c..ca8fbdeb3b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -58,6 +58,7 @@ import javax.annotation.Nullable;
  */
 public class WorkspaceFactory {
   public static final String BIND = "bind";
+  private static final Pattern LEGAL_WORKSPACE_NAME = Pattern.compile("^\\p{Alpha}\\w*$");
 
   private final LegacyBuilder builder;
   
@@ -166,6 +167,13 @@ public class WorkspaceFactory {
     localReporter.clear();
   }
 
+  private static void checkWorkspaceName(String name, FuncallExpression ast) throws EvalException {
+    Matcher matcher = LEGAL_WORKSPACE_NAME.matcher(name);
+    if (!matcher.matches()) {
+      throw new EvalException(ast.getLocation(), name + " is not a legal workspace name");
+    }
+  }
+
   @SkylarkSignature(name = "workspace", objectType = Object.class, returnType = SkylarkList.class,
       doc = "Sets the name for this workspace. Workspace names should be a Java-package-style "
           + "description of the project, using underscores as separators, e.g., "
@@ -182,12 +190,7 @@ public class WorkspaceFactory {
               "workspace", FunctionSignature.namedOnly("name"), BuiltinFunction.USE_AST_ENV) {
             public Object invoke(String name, FuncallExpression ast, Environment env)
                 throws EvalException {
-              Pattern legalWorkspaceName = Pattern.compile("^\\p{Alpha}\\w*$");
-              Matcher matcher = legalWorkspaceName.matcher(name);
-              if (!matcher.matches()) {
-                throw new EvalException(
-                    ast.getLocation(), name + " is not a legal workspace name");
-              }
+              checkWorkspaceName(name, ast);
               String errorMessage = LabelValidator.validateTargetName(name);
               if (errorMessage != null) {
                 throw new EvalException(ast.getLocation(), errorMessage);
@@ -247,9 +250,10 @@ public class WorkspaceFactory {
           Builder builder = PackageFactory.getContext(env, ast).pkgBuilder;
           RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
           RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
-          builder
+          Rule rule = builder
               .externalPackageData()
               .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
+          checkWorkspaceName(rule.getName(), ast);
         } catch (
             RuleFactory.InvalidRuleException | Package.NameConflictException | LabelSyntaxException
                 e) {
diff --git a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
index 12898477d4..d431290358 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
@@ -41,35 +41,35 @@ public class ExternalPackageTest extends BuildViewTestCase {
   public void testMultipleRulesWithSameName() throws Exception {
     FileSystemUtils.writeIsoLatin1(workspacePath,
         "local_repository(",
-        "    name = 'my-rule',",
+        "    name = 'my_rule',",
         "    path = '/foo/bar',",
         ")",
         "new_local_repository(",
-        "    name = 'my-rule',",
+        "    name = 'my_rule',",
         "    path = '/foo/bar',",
         "    build_file = 'baz',",
         ")");
 
     invalidatePackages();
     // Make sure the second rule "wins."
-    assertEquals("new_local_repository rule", getTarget("//external:my-rule").getTargetKind());
+    assertEquals("new_local_repository rule", getTarget("//external:my_rule").getTargetKind());
   }
 
   @Test
   public void testOverridingBindRules() throws Exception {
     FileSystemUtils.writeIsoLatin1(workspacePath,
         "bind(",
-        "    name = 'my-rule',",
+        "    name = 'my_rule',",
         "    actual = '//foo:bar',",
         ")",
         "new_local_repository(",
-        "    name = 'my-rule',",
+        "    name = 'my_rule',",
         "    path = '/foo/bar',",
         "    build_file = 'baz',",
         ")");
 
     invalidatePackages();
     // Make sure the second rule "wins."
-    assertEquals("new_local_repository rule", getTarget("//external:my-rule").getTargetKind());
+    assertEquals("new_local_repository rule", getTarget("//external:my_rule").getTargetKind());
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
index d2d8b7c78e..db5e867df1 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -69,6 +69,15 @@ public class WorkspaceFactoryTest {
     assertThat(helper.getParserError()).contains("a.b.c is not a legal workspace name");
   }
 
+  @Test
+  public void testIllegalRepoName() throws Exception {
+    WorkspaceFactoryHelper helper = parse("local_repository(",
+        "    name = 'foo/bar',",
+        "    path = '/foo/bar',",
+        ")");
+    assertThat(helper.getParserError()).contains("foo/bar is not a legal workspace name");
+  }
+
   private WorkspaceFactoryHelper parse(String... args) {
     return new WorkspaceFactoryHelper(args);
   }