Make error messages about illegal dependencies involving aliases clearer.

RELNOTES: None.
PiperOrigin-RevId: 190759949

3 files changed, 55 insertions, 26 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
index 3633fd2190..c704b83ea6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
@@ -66,12 +66,41 @@ public final class AliasProvider implements TransitiveInfoProvider {
     return aliasChain;
   }
 
-  public static String printLabelWithAliasChain(ConfiguredTargetAndData target) {
+  /** The way {@link #describeTargetWithAliases(ConfiguredTargetAndData, TargetMode) reports the
+   * kind of a target. */
+  public enum TargetMode {
+    WITH_KIND,      // Specify the kind of the target
+    WITHOUT_KIND,   // Only say "target"
+  }
+
+  /**
+   * Prints a nice description of a target.
+   *
+   * Also adds the aliases it was reached through, if any.
+   *
+   * @param target the target to describe
+   * @param targetMode how to express the kind of the target
+   * @return
+   */
+  public static String describeTargetWithAliases(
+      ConfiguredTargetAndData target, TargetMode targetMode) {
+    String kind = targetMode == TargetMode.WITH_KIND
+        ? target.getTarget().getTargetKind() : "target";
     AliasProvider aliasProvider = target.getConfiguredTarget().getProvider(AliasProvider.class);
-    String suffix = aliasProvider == null
-        ? ""
-        : " (aliased through '" + Joiner.on("' -> '").join(aliasProvider.getAliasChain()) + "')";
+    if (aliasProvider == null) {
+      return kind + " '" + target.getTarget().getLabel() + "'";
+    }
+
+    ImmutableList<Label> aliasChain = aliasProvider.getAliasChain();
+    StringBuilder result = new StringBuilder();
+    result.append("alias '" + aliasChain.get(0) + "'");
+    result.append(" referring to " + kind  + " '" + target.getTarget().getLabel() + "'");
+    if (aliasChain.size() > 1) {
+      result.append(" through '"
+          + Joiner.on("' -> '").join(aliasChain.subList(1, aliasChain.size()))
+          + "'");
+    }
 
-    return "'" + target.getTarget().getLabel() + "'" + suffix;
+    return result.toString();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 6bab955fca..c8c477524f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -41,6 +41,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.PrerequisiteValidator;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
@@ -1679,26 +1680,25 @@ public final class RuleContext extends TargetContext
     }
 
     private String badPrerequisiteMessage(
-        String targetKind, ConfiguredTargetAndData prerequisite, String reason, boolean isWarning) {
-      String msgPrefix = targetKind != null ? targetKind + " " : "";
+        ConfiguredTargetAndData prerequisite, String reason, boolean isWarning) {
+      String targetKind = prerequisite.getTarget().getTargetKind();
       String msgReason = reason != null ? " (" + reason + ")" : "";
       if (isWarning) {
         return String.format(
-            "%s%s is unexpected here%s; continuing anyway",
-            msgPrefix, AliasProvider.printLabelWithAliasChain(prerequisite),
+            "%s is unexpected here%s; continuing anyway",
+            AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND),
             msgReason);
       }
-      return String.format("%s%s is misplaced here%s",
-          msgPrefix, AliasProvider.printLabelWithAliasChain(prerequisite), msgReason);
+      return String.format("%s is misplaced here%s",
+          AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND), msgReason);
     }
 
     private void reportBadPrerequisite(
         Attribute attribute,
-        String targetKind,
         ConfiguredTargetAndData prerequisite,
         String reason,
         boolean isWarning) {
-      String message = badPrerequisiteMessage(targetKind, prerequisite, reason, isWarning);
+      String message = badPrerequisiteMessage(prerequisite, reason, isWarning);
       if (isWarning) {
         attributeWarning(attribute.getName(), message);
       } else {
@@ -1716,8 +1716,7 @@ public final class RuleContext extends TargetContext
 
         String reason = attribute.getValidityPredicate().checkValid(rule, prerequisiteRule);
         if (reason != null) {
-          reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(),
-              prerequisite, reason, false);
+          reportBadPrerequisite(attribute, prerequisite, reason, false);
         }
       }
 
@@ -1742,7 +1741,7 @@ public final class RuleContext extends TargetContext
               }
             } else {
               // The file exists but has a bad extension
-              reportBadPrerequisite(attribute, "file", prerequisite,
+              reportBadPrerequisite(attribute, prerequisite,
                   "expected " + attribute.getAllowedFileTypesPredicate(), false);
             }
           }
@@ -1880,7 +1879,6 @@ public final class RuleContext extends TargetContext
         // but maybe prerequisite provides required providers? do not reject yet.
         unfulfilledRequirements.add(
             badPrerequisiteMessage(
-                prerequisite.getTarget().getTargetKind(),
                 prerequisite,
                 "expected " + attribute.getAllowedRuleClassesPredicate(),
                 false));
@@ -1901,7 +1899,6 @@ public final class RuleContext extends TargetContext
         Predicate<RuleClass> allowedRuleClasses = attribute.getAllowedRuleClassesPredicate();
         reportBadPrerequisite(
             attribute,
-            prerequisite.getTarget().getTargetKind(),
             prerequisite,
             allowedRuleClasses == Predicates.<RuleClass>alwaysTrue()
                 ? null
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 bfb346e421..a4f2f3d011 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
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.bazel.rules;
 
 import com.google.devtools.build.lib.analysis.AliasProvider;
+import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -56,18 +57,20 @@ public class BazelPrerequisiteValidator
       if (!context.getConfiguration().checkVisibility()) {
         errorMessage =
             String.format(
-                "Target '%s' violates visibility of target "
+                "Target '%s' violates visibility of "
                     + "%s. Continuing because --nocheck_visibility is active",
-                rule.getLabel(), AliasProvider.printLabelWithAliasChain(prerequisite));
+                rule.getLabel(), AliasProvider.describeTargetWithAliases(prerequisite,
+                    TargetMode.WITHOUT_KIND));
         context.ruleWarning(errorMessage);
       } else {
         // Oddly enough, we use reportError rather than ruleError here.
         errorMessage =
             String.format(
-                "Target %s is not visible from target '%s'. Check "
+                "%s is not visible from target '%s'. Check "
                     + "the visibility declaration of the former target if you think "
                     + "the dependency is legitimate",
-                AliasProvider.printLabelWithAliasChain(prerequisite), rule.getLabel());
+                AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND),
+                rule.getLabel());
         context.reportError(rule.getLocation(), errorMessage);
       }
       // We can always post the visibility error as, regardless of the value of keep going,
@@ -91,8 +94,8 @@ public class BazelPrerequisiteValidator
                 + rule.getRuleClass()
                 + " rule "
                 + rule.getLabel()
-                + ": package group "
-                + AliasProvider.printLabelWithAliasChain(prerequisite)
+                + ": "
+                + AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND)
                 + " is misplaced here "
                 + "(they are only allowed in the visibility attribute)");
       }
@@ -116,8 +119,8 @@ public class BazelPrerequisiteValidator
       String message =
           "non-test target '"
               + rule.getLabel()
-              + "' depends on testonly target "
-              + AliasProvider.printLabelWithAliasChain(prerequisite)
+              + "' depends on testonly "
+              + AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND)
               + " and doesn't have testonly attribute set";
       if (thisPackage.startsWith("experimental/")) {
         context.ruleWarning(message);