Do not crash if 'bind' and external repo generate the same //external: target.

Work towards #3676.

The behavior is still incorrect (we should in fact disallow this), but
at least there is no hard crash.

Change-Id: I5181dba73ad725d20b2ea82b2f19e86664b9dbff
PiperOrigin-RevId: 181954820

3 files changed, 37 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index beb12bfa35..76fc7d314b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -157,9 +157,14 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       if (prerequisiteTarget instanceof Rule) {
         Rule prerequisiteRule = (Rule) prerequisiteTarget;
         String thisDeprecation =
-            NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING);
+            NonconfigurableAttributeMapper.of(rule).has("deprecation", Type.STRING)
+                ? NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING)
+                : null;
         String thatDeprecation =
-            NonconfigurableAttributeMapper.of(prerequisiteRule).get("deprecation", Type.STRING);
+            NonconfigurableAttributeMapper.of(prerequisiteRule).has("deprecation", Type.STRING)
+                ? NonconfigurableAttributeMapper.of(prerequisiteRule)
+                    .get("deprecation", Type.STRING)
+                : null;
         if (shouldEmitDeprecationWarningFor(
             thisDeprecation, thisPackage, thatDeprecation, thatPackage, forAspect)) {
           errors.ruleWarning("target '" + rule.getLabel() +  "' depends on deprecated target '"
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
index 2a62df3ff7..afdcb18003 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
@@ -127,6 +127,7 @@ public class BazelPrerequisiteValidator
 
   private static boolean isTestOnlyRule(Target target) {
     return (target instanceof Rule)
+        && (NonconfigurableAttributeMapper.of((Rule) target)).has("testonly", Type.BOOLEAN)
         && (NonconfigurableAttributeMapper.of((Rule) target)).get("testonly", Type.BOOLEAN);
   }
 }
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..aec222e596 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,33 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase {
     // Just request the last external repository to force the whole loading.
     getConfiguredTarget("@foo//:bar");
   }
+
+  @Test
+  public void testBindAndRepoSameNameDoesNotCrash() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file("/repo2/data.txt", "data");
+    scratch.file("/repo2/BUILD", "load('@//:rulez.bzl', 'r')", "r(name = 'z')");
+    scratch.file("/repo2/WORKSPACE");
+
+    scratch.file(
+        "rulez.bzl",
+        "def _impl(ctx):",
+        "    pass",
+        "r = rule(_impl, attrs = { 'deps' : attr.label_list() })");
+    scratch.file("BUILD", "load(':rulez.bzl', 'r')", "r(name = 'x', deps = ['//external:zlib'])");
+
+    scratch.overwriteFile(
+        rootDirectory.getRelative("WORKSPACE").getPathString(),
+        new ImmutableList.Builder<String>()
+            .addAll(analysisMock.getWorkspaceContents(mockToolsConfig))
+            .add("bind(name = 'zlib', actual = '@zlib//:z')")
+            .add("local_repository(name = 'zlib', path = '/repo2')")
+            .build());
+    invalidatePackages();
+    getConfiguredTarget("//:x");
+    assertContainsEvent(
+        "Target '//external:zlib' is not visible from target '//:x'. "
+            + "Check the visibility declaration of the former target if you think the "
+            + "dependency is legitimate");
+  }
 }