From 7f6d3a13b63b702a6c56010610c3f0018490dfec Mon Sep 17 00:00:00 2001 From: vladmos Date: Mon, 17 Jul 2017 23:15:21 +0200 Subject: Improve attribute and index error messages for targets Instead of "object of type 'Target'" a representation of the target can be used for improved readability. PiperOrigin-RevId: 162267961 --- .../build/lib/analysis/AbstractConfiguredTarget.java | 8 ++++++-- .../build/lib/analysis/RuleConfiguredTarget.java | 5 +++-- .../build/lib/rules/objc/ObjcSkylarkTest.java | 3 ++- .../build/lib/skylark/SkylarkRuleContextTest.java | 4 +++- .../SkylarkRuleImplementationFunctionsTest.java | 20 +++++++++++--------- 5 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java index 8464ded1a9..fb6fcf1a43 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; +import com.google.devtools.build.lib.syntax.Printer; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import javax.annotation.Nullable; @@ -128,8 +129,11 @@ public abstract class AbstractConfiguredTarget if (declaredProvider != null) { return declaredProvider; } - throw new EvalException(loc, String.format( - "Object of type Target doesn't contain declared provider %s", + throw new EvalException(loc, Printer.format( + "%r%s doesn't contain declared provider '%s'", + this, + getTarget().getAssociatedRule() == null ? "" + : " (rule '" + getTarget().getAssociatedRule().getRuleClass() + "')", constructor.getPrintableName())); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java index d7af3d42de..8f6b61cbc3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.SkylarkClassObject; import com.google.devtools.build.lib.rules.SkylarkApiProvider; import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; +import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.util.Preconditions; import java.util.function.Consumer; import javax.annotation.Nullable; @@ -113,8 +114,8 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget { @Override public String errorMessage(String name) { - return String.format("target (rule class of '%s') doesn't have provider '%s'.", - getTarget().getRuleClass(), name); + return Printer.format("%r (rule '%s') doesn't have provider '%s'", + this, getTarget().getRuleClass(), name); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java index 4dea91f381..3f7e492882 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcSkylarkTest.java @@ -130,7 +130,8 @@ public class ObjcSkylarkTest extends ObjcRuleTestCase { assertThat(e).hasMessageThat().contains("dep.objc"); assertThat(e) .hasMessageThat() - .contains("target (rule class of 'cc_library') doesn't have provider 'objc'."); + .contains(" (rule 'cc_library') " + + "doesn't have provider 'objc'"); } } 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 5dde93b421..0431e99845 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 @@ -1552,7 +1552,9 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { fail("Should have failed due to trying to access actions of a rule not marked " + "_skylark_testable"); } catch (Exception e) { - assertThat(e).hasMessage("Object of type Target doesn't contain declared provider Actions"); + assertThat(e).hasMessageThat().contains( + " (rule 'undertest_rule') doesn't contain " + + "declared provider 'Actions'"); } } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java index d539354696..72ae65e0b4 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java @@ -704,16 +704,16 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { @Test public void testRunfilesBadMapGenericType() throws Exception { checkErrorContains( - "expected type 'string' for 'symlinks' key " + "but got type 'int' instead", + "expected type 'string' for 'symlinks' key but got type 'int' instead", "ruleContext.runfiles(symlinks = {123: ruleContext.files.srcs[0]})"); checkErrorContains( - "expected type 'File' for 'symlinks' value " + "but got type 'int' instead", + "expected type 'File' for 'symlinks' value but got type 'int' instead", "ruleContext.runfiles(symlinks = {'some string': 123})"); checkErrorContains( - "expected type 'string' for 'root_symlinks' key " + "but got type 'int' instead", + "expected type 'string' for 'root_symlinks' key but got type 'int' instead", "ruleContext.runfiles(root_symlinks = {123: ruleContext.files.srcs[0]})"); checkErrorContains( - "expected type 'File' for 'root_symlinks' value " + "but got type 'int' instead", + "expected type 'File' for 'root_symlinks' value but got type 'int' instead", "ruleContext.runfiles(root_symlinks = {'some string': 123})"); } @@ -844,7 +844,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { public void testNoSuchProviderErrorMessage() throws Exception { checkErrorContains( createRuleContext("//foo:bar"), - "target (rule class of 'java_library') " + "doesn't have provider 'my_provider'.", + " (rule 'java_library') doesn't have provider 'my_provider'", "ruleContext.attr.srcs[0].my_provider"); } @@ -1345,7 +1345,8 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { } catch (AssertionError expected) { assertThat(expected) .hasMessageThat() - .contains("Object of type Target doesn't " + "contain declared provider unused_provider"); + .contains(" (rule 'foo_rule') doesn't contain " + + "declared provider 'unused_provider'"); } } @@ -1391,7 +1392,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { assertThat(expected) .hasMessageThat() .contains( - "Type Target only supports indexing " + "by object constructors, got string instead"); + "Type Target only supports indexing by object constructors, got string instead"); } } @@ -1421,7 +1422,8 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { } catch (AssertionError expected) { assertThat(expected) .hasMessageThat() - .contains("Object of type Target doesn't " + "contain declared provider unused_provider"); + .contains(" doesn't contain " + + "declared provider 'unused_provider'"); } } @@ -1504,7 +1506,7 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase { assertThat(expected) .hasMessageThat() .contains( - "Type Target only supports querying by object " + "constructors, got string instead"); + "Type Target only supports querying by object constructors, got string instead"); } } -- cgit v1.2.3