Forbid overloading of a repository outside of the first part of the workspace file

Fixes #824.

--
MOS_MIGRATED_REVID=114326952

4 files changed, 77 insertions, 22 deletions

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 90f4ea87c8..d300653fb7 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
@@ -103,7 +103,7 @@ public class WorkspaceFactory {
       RuleClassProvider ruleClassProvider,
       ImmutableList<EnvironmentExtension> environmentExtensions,
       Mutability mutability) {
-    this(builder, ruleClassProvider, environmentExtensions, mutability, null, null);
+    this(builder, ruleClassProvider, environmentExtensions, mutability, true, null, null);
   }
 
   // TODO(bazel-team): document installDir
@@ -120,6 +120,7 @@ public class WorkspaceFactory {
       RuleClassProvider ruleClassProvider,
       ImmutableList<EnvironmentExtension> environmentExtensions,
       Mutability mutability,
+      boolean allowOverride,
       @Nullable Path installDir,
       @Nullable Path workspaceDir) {
     this.builder = builder;
@@ -127,7 +128,7 @@ public class WorkspaceFactory {
     this.installDir = installDir;
     this.workspaceDir = workspaceDir;
     this.environmentExtensions = environmentExtensions;
-    this.workspaceFunctions = createWorkspaceFunctions(ruleClassProvider);
+    this.workspaceFunctions = createWorkspaceFunctions(ruleClassProvider, allowOverride);
   }
 
   /**
@@ -325,18 +326,29 @@ public class WorkspaceFactory {
    * specified package context.
    */
   private static BuiltinFunction newRuleFunction(
-      final RuleFactory ruleFactory, final String ruleClassName) {
+      final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) {
     return new BuiltinFunction(
         ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_AST_ENV) {
       public Object invoke(Map<String, Object> kwargs, FuncallExpression ast, Environment env)
           throws EvalException, InterruptedException {
         try {
           Builder builder = PackageFactory.getContext(env, ast).pkgBuilder;
+          if (!allowOverride
+              && kwargs.containsKey("name")
+              && builder.targets.containsKey(kwargs.get("name"))) {
+            throw new EvalException(
+                ast.getLocation(),
+                "Cannot redefine repository after any load statement in the WORKSPACE file"
+                    + " (for repository '"
+                    + kwargs.get("name")
+                    + "')");
+          }
           RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
           RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
-          Rule rule = builder
-              .externalPackageData()
-              .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
+          Rule rule =
+              builder
+                  .externalPackageData()
+                  .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
           if (!isLegalWorkspaceName(rule.getName())) {
             throw new EvalException(
                 ast.getLocation(), rule + "'s name field must be a legal workspace name");
@@ -352,13 +364,13 @@ public class WorkspaceFactory {
   }
 
   private static ImmutableMap<String, BaseFunction> createWorkspaceFunctions(
-      RuleClassProvider ruleClassProvider) {
+      RuleClassProvider ruleClassProvider, boolean allowOverride) {
     ImmutableMap.Builder<String, BaseFunction> mapBuilder = ImmutableMap.builder();
     RuleFactory ruleFactory = new RuleFactory(ruleClassProvider);
     mapBuilder.put(BIND, newBindFunction(ruleFactory));
     for (String ruleClass : ruleFactory.getRuleClassNames()) {
       if (!ruleClass.equals(BIND)) {
-        BaseFunction ruleFunction = newRuleFunction(ruleFactory, ruleClass);
+        BaseFunction ruleFunction = newRuleFunction(ruleFactory, ruleClass, allowOverride);
         mapBuilder.put(ruleClass, ruleFunction);
       }
     }
@@ -405,7 +417,7 @@ public class WorkspaceFactory {
   }
 
   public static ClassObject newNativeModule(RuleClassProvider ruleClassProvider) {
-    return newNativeModule(createWorkspaceFunctions(ruleClassProvider));
+    return newNativeModule(createWorkspaceFunctions(ruleClassProvider, false));
   }
 
   static {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 2ab3267d87..8536558087 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -87,6 +87,7 @@ public class WorkspaceFileFunction implements SkyFunction {
               ruleClassProvider,
               packageFactory.getEnvironmentExtensions(),
               mutability,
+              key.getIndex() == 0,
               directories.getEmbeddedBinariesRoot(),
               directories.getWorkspace());
       if (key.getIndex() > 0) {
@@ -108,8 +109,6 @@ public class WorkspaceFileFunction implements SkyFunction {
       if (importResult == null) {
         return null;
       }
-      // TODO(dmarting): give a nice error message when redefining a repository name and
-      // getIndex() > 0.
       parser.execute(ast, importResult.importMap);
     } catch (PackageFunctionException | NameConflictException e) {
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
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 9cab77d4f5..9f60fd249f 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
@@ -109,6 +109,7 @@ public class WorkspaceFactoryTest {
           TestRuleClassProvider.getRuleClassProvider(),
           ImmutableList.<PackageFactory.EnvironmentExtension>of(),
           Mutability.create("test"),
+          true,
           root,
           root);
       Exception exception = null;
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 7f9e9e1713..4040b86faf 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -81,7 +81,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         ")");
   }
 
-  private void setUpAttributeErrorTest() throws Exception   {
+  private void setUpAttributeErrorTest() throws Exception {
     scratch.file("test/BUILD",
         "load('/test/macros', 'macro_native_rule', 'macro_skylark_rule', 'skylark_rule')",
         "macro_native_rule(name = 'm_native',",
@@ -121,7 +121,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
       // about the macro.
       assertContainsEvent(
           "ERROR /workspace/test/BUILD:2:1: in deps attribute of cc_library rule //test:m_native: "
-          + "java_library rule '//test:jlib' is misplaced here (expected ");
+              + "java_library rule '//test:jlib' is misplaced here (expected ");
       // Skip the part of the error message that has details about the allowed deps since the mocks
       // for the mac tests might have different values for them.
       assertContainsEvent(". Since this "
@@ -141,9 +141,9 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
       // about the macro.
       assertContainsEvent(
           "ERROR /workspace/test/BUILD:4:1: in deps attribute of skylark_rule rule "
-          + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. "
-          + "Since this rule was created by the macro 'macro_skylark_rule', the error might have "
-          + "been caused by the macro implementation in /workspace/test/macros.bzl:12:36");
+              + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. "
+              + "Since this rule was created by the macro 'macro_skylark_rule', the error might "
+              + "have been caused by the macro implementation in /workspace/test/macros.bzl:12:36");
     }
   }
 
@@ -216,8 +216,8 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     getConfiguredTarget("//test:skyrule");
     assertContainsEvent(
         "ERROR /workspace/test/BUILD:3:10: Label '//test:sub/my_sub_lib.h' crosses boundary of "
-        + "subpackage 'test/sub' (perhaps you meant to put the colon here: "
-        + "'//test/sub:my_sub_lib.h'?)");
+            + "subpackage 'test/sub' (perhaps you meant to put the colon here: "
+            + "'//test/sub:my_sub_lib.h'?)");
   }
 
   @Test
@@ -650,10 +650,11 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
   @Test
   public void testParamFileSuffix() throws Exception {
     SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
-    Object result = evalRuleContextCode(
-        ruleContext,
-        "ruleContext.new_file(ruleContext.files.tools[0], "
-        + "ruleContext.files.tools[0].basename + '.params')");
+    Object result =
+        evalRuleContextCode(
+            ruleContext,
+            "ruleContext.new_file(ruleContext.files.tools[0], "
+                + "ruleContext.files.tools[0].basename + '.params')");
     PathFragment fragment = ((Artifact) result).getRootRelativePath();
     assertEquals("foo/t.exe.params", fragment.getPathString());
   }
@@ -755,4 +756,46 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     invalidatePackages();
     assertThat(getConfiguredTarget("@r1//:test")).isNotNull();
   }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void testLoadBlockRepositoryRedefinition() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file("/bar/WORKSPACE");
+    scratch.file("/bar/bar.txt");
+    scratch.file("/bar/BUILD", "filegroup(name = 'baz', srcs = ['bar.txt'])");
+    scratch.file("/baz/WORKSPACE");
+    scratch.file("/baz/baz.txt");
+    scratch.file("/baz/BUILD", "filegroup(name = 'baz', srcs = ['baz.txt'])");
+    scratch.overwriteFile(
+        "WORKSPACE",
+        "local_repository(name = 'foo', path = '/bar')",
+        "local_repository(name = 'foo', path = '/baz')");
+    invalidatePackages();
+    assertThat(
+            (List<Label>)
+                getConfiguredTarget("@foo//:baz")
+                    .getTarget()
+                    .getAssociatedRule()
+                    .getAttributeContainer()
+                    .getAttr("srcs"))
+        .contains(Label.parseAbsolute("@foo//:baz.txt"));
+
+    scratch.overwriteFile("BUILD");
+    scratch.overwriteFile("bar.bzl", "dummy = 1");
+    scratch.overwriteFile(
+        "WORKSPACE",
+        "local_repository(name = 'foo', path = '/bar')",
+        "load('//:bar.bzl', 'dummy')",
+        "local_repository(name = 'foo', path = '/baz')");
+    try {
+      invalidatePackages();
+      createRuleContext("@foo//:baz");
+      fail("Should have failed because repository 'foo' is overloading after a load!");
+    } catch (Exception ex) {
+      assertContainsEvent(
+          "ERROR /workspace/WORKSPACE:3:1: Cannot redefine repository after any load statement "
+              + "in the WORKSPACE file (for repository 'foo')");
+    }
+  }
 }