Move override check to the createAndOverrideRule function

So we actually test for override also from skylark repositories.

Fixes #3908.

Change-Id: I7650a17834a6915a73c89df46989f72aa2f56920
PiperOrigin-RevId: 176645218

5 files changed, 59 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index ef6391b254..95ab59942c 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -175,7 +175,12 @@ public class SkylarkRepositoryModule {
         @SuppressWarnings("unchecked")
         Map<String, Object> attributeValues = (Map<String, Object>) args[0];
         return WorkspaceFactoryHelper.createAndAddRepositoryRule(
-            context.getBuilder(), ruleClass, null, attributeValues, ast);
+            context.getBuilder(),
+            ruleClass,
+            null,
+            attributeValues,
+            ast,
+            (Boolean) env.lookup("$allow_override"));
       } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
         throw new EvalException(ast.getLocation(), e.getMessage());
       }
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 2d7176aba8..dae4393b7c 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
@@ -335,7 +335,7 @@ public class WorkspaceFactory {
                   // This effectively adds a "local_repository(name = "<ws>", path = ".")"
                   // definition to the WORKSPACE file.
                   WorkspaceFactoryHelper.createAndAddRepositoryRule(
-                      builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast);
+                      builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast, allowOverride);
                 } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
                   throw new EvalException(ast.getLocation(), e.getMessage());
                 }
@@ -451,21 +451,11 @@ public class WorkspaceFactory {
           throws EvalException, InterruptedException {
         try {
           Package.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 =
               WorkspaceFactoryHelper.createAndAddRepositoryRule(
-                  builder, ruleClass, bindRuleClass, kwargs, ast);
+                  builder, ruleClass, bindRuleClass, kwargs, ast, allowOverride);
           if (!isLegalWorkspaceName(rule.getName())) {
             throw new EvalException(
                 ast.getLocation(), rule + "'s name field must be a legal workspace name");
@@ -515,6 +505,7 @@ public class WorkspaceFactory {
       workspaceEnv.setupDynamic(
           PackageFactory.PKG_CONTEXT,
           new PackageFactory.PackageContext(builder, null, localReporter, AttributeContainer::new));
+      workspaceEnv.setupDynamic("$allow_override", allowOverride);
     } catch (EvalException e) {
       throw new AssertionError(e);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
index 42239f6506..25e5bb8f54 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.Builder;
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import java.util.Map;
 
@@ -34,10 +35,20 @@ public class WorkspaceFactoryHelper {
       RuleClass ruleClass,
       RuleClass bindRuleClass,
       Map<String, Object> kwargs,
-      FuncallExpression ast)
+      FuncallExpression ast,
+      boolean allowOverride)
       throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
-          InterruptedException {
-
+          InterruptedException, EvalException {
+    if (!allowOverride
+        && kwargs.containsKey("name")
+        && pkg.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")
+              + "')");
+    }
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
     Rule rule =
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index 7b716c3f3a..350249de05 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -86,7 +86,7 @@ public class SkylarkRepositoryContextTest {
     ast.setLocation(Location.BUILTIN);
     Rule rule =
         WorkspaceFactoryHelper.createAndAddRepositoryRule(
-            packageBuilder, buildRuleClass(attributes), null, kwargs, ast);
+            packageBuilder, buildRuleClass(attributes), null, kwargs, ast, false);
     HttpDownloader downloader = Mockito.mock(HttpDownloader.class);
     context =
         new SkylarkRepositoryContext(
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
index cd636a9f08..ad9c11a26a 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
@@ -407,4 +407,39 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase {
     // Just request the last external repository to force the whole loading.
     getConfiguredTarget("@foo//:bar");
   }
+
+  // Regression test for https://github.com/bazelbuild/bazel/issues/3908
+  @Test
+  public void testSkylarkCannotOverrideRules() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("WORKSPACE").getPathString(),
+        ImmutableList.<String>builder()
+            .addAll(analysisMock.getWorkspaceContents(mockToolsConfig))
+            .add("local_repository(name = 'local_repo', path = '/local_repo')") //
+            .add("load('//:test.bzl', 'my_repo')") //
+            .add("my_repo(name = 'local_repo')") //
+            .add("local_repository(name = 'foo', path = '/local_repo')")
+            .build());
+    scratch.file(
+        rootDirectory.getRelative("test.bzl").getPathString(), //
+        "def _impl():", //
+        "  print('BLEH')", //
+        "", //
+        "my_repo = repository_rule(_impl)");
+    scratch.file(rootDirectory.getRelative("BUILD").getPathString());
+    scratch.file("/local_repo/WORKSPACE");
+    scratch.file(
+        "/local_repo/BUILD",
+        "filegroup(name = 'test', srcs = [], visibility = ['//visibility:public'])");
+    // Just request the last external repository to force the whole loading.
+    try {
+      invalidatePackages();
+      getConfiguredTarget("@foo//:test");
+      fail();
+    } catch (AssertionError e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Cannot redefine repository after any load statement in the WORKSPACE file");
+    }
+  }
 }