External repositories: disallow use of unexported repository rules

Treat repository rules the same way as build rules: they may only be used,
if exported by a Skylark file. It has never been intended to create external
repositories by anonymous rules, and, in fact, for properly recording resolved
information, it is necessary that all repository rules used can be referred
to by an accessible name.

Change-Id: Ib9259d58be66b033721a6f591656c45889f49931
PiperOrigin-RevId: 204872735

src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java

@@ -36,12 +36,9 @@ import com.google.devtools.build.lib.packages.WorkspaceFactoryHelper;
 import com.google.devtools.build.lib.skylarkbuildapi.repository.RepositoryModuleApi;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.syntax.BaseFunction;
-import com.google.devtools.build.lib.syntax.DotExpression;
 import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.Expression;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
-import com.google.devtools.build.lib.syntax.Identifier;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import java.util.Map;
@@ -124,22 +121,13 @@ public class SkylarkRepositoryModule implements RepositoryModuleApi {
         Object[] args, FuncallExpression ast, com.google.devtools.build.lib.syntax.Environment env)
         throws EvalException, InterruptedException {
       String ruleClassName = null;
-      Expression function = ast.getFunction();
-      // If the function ever got exported (the common case), we take the name
-      // it was exprted to. Only in the not intended case of calling an unexported
-      // repository function through an exported macro, we fall back, for lack of
-      // alternatives, to the name in the local context.
-      // TODO(b/111199163): we probably should disallow the use of non-exported
-      // repository rules anyway.
+      // If the function ever got exported, we take the name it was exported to.
       if (isExported()) {
         ruleClassName = exportedName;
-      } else if (function instanceof Identifier) {
-        ruleClassName = ((Identifier) function).getName();
-      } else if (function instanceof DotExpression) {
-        ruleClassName = ((DotExpression) function).getField().getName();
       } else {
-        // TODO: Remove the wrong assumption that a  "function name" always exists and is relevant
-        throw new IllegalStateException("Function is not an identifier or method call");
+        throw new EvalException(ast.getLocation(),
+            "Use of unexported repository rule; this repository rule class has not been exported"
+            + "by a Skylark file");
       }
       try {
         RuleClass ruleClass = builder.build(ruleClassName, ruleClassName);

src/test/shell/bazel/skylark_repository_test.sh

@@ -1070,6 +1070,28 @@ EOF
   bazel build //external:reg &> $TEST_log || fail "Couldn't build repo"
 }
 
+function test_unexported_rule() {
+  cat > repo.bzl <<'EOF'
+def _trivial_rule_impl(ctx):
+  ctx.file("BUILD","genrule(name='hello', outs=['hello.txt'], cmd=' echo hello world > $@')")
+
+def use_hidden_rule(name=""):
+  repository_rule(
+    implementation = _trivial_rule_impl,
+    attrs = {},
+  )(name=name)
+EOF
+  touch BUILD
+  cat > WORKSPACE <<'EOF'
+load("//:repo.bzl", "use_hidden_rule")
+
+use_hidden_rule(name="foo")
+EOF
+  bazel build @foo//... > "${TEST_log}" 2>&1 && fail "Expected failure" || :
+
+  expect_log 'unexported'
+}
+
 function tear_down() {
   shutdown_server
   if [ -d "${TEST_TMPDIR}/server_dir" ]; then