Improved error reporting in RuleContext:
- Unified duplicate code from RuleContext and RuleContext.Builder in a new class, RuleContext.ErrorReporter
- Added the BUILD file location to error/warning messages if the offending rule was created by a macro
--
MOS_MIGRATED_REVID=103934375
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 10395cb..b280086 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
@@ -137,6 +137,7 @@
private final ImmutableSet<String> features;
private final Map<String, Attribute> attributeMap;
private final BuildConfiguration hostConfiguration;
+ private final ErrorReporter reporter;
private ActionOwner actionOwner;
@@ -157,6 +158,7 @@
this.features = getEnabledFeatures();
this.attributeMap = attributeMap;
this.hostConfiguration = builder.hostConfiguration;
+ reporter = builder.reporter;
}
private ImmutableSet<String> getEnabledFeatures() {
@@ -391,7 +393,7 @@
*/
@Override
public void ruleError(String message) {
- reportError(rule.getLocation(), prefixRuleMessage(message));
+ reporter.ruleError(message);
}
/**
@@ -400,7 +402,7 @@
*/
@Override
public void ruleWarning(String message) {
- reportWarning(rule.getLocation(), prefixRuleMessage(message));
+ reporter.ruleWarning(message);
}
/**
@@ -412,11 +414,7 @@
*/
@Override
public void attributeError(String attrName, String message) {
- reportError(rule.getAttributeLocation(attrName),
- prefixAttributeMessage(Attribute.isImplicit(attrName)
- ? "(an implicit dependency)"
- : attrName,
- message));
+ reporter.attributeError(attrName, message);
}
/**
@@ -427,30 +425,7 @@
*/
@Override
public void attributeWarning(String attrName, String message) {
- reportWarning(rule.getAttributeLocation(attrName),
- prefixAttributeMessage(Attribute.isImplicit(attrName)
- ? "(an implicit dependency)"
- : attrName,
- message));
- }
-
- private String prefixAttributeMessage(String attrName, String message) {
- return "in " + attrName + " attribute of "
- + rule.getRuleClass() + " rule "
- + getLabel() + ": " + message;
- }
-
- private String prefixRuleMessage(String message) {
- return "in " + rule.getRuleClass() + " rule "
- + getLabel() + ": " + message;
- }
-
- private void reportError(Location location, String message) {
- getAnalysisEnvironment().getEventHandler().handle(Event.error(location, message));
- }
-
- private void reportWarning(Location location, String message) {
- getAnalysisEnvironment().getEventHandler().handle(Event.warn(location, message));
+ reporter.attributeWarning(attrName, message);
}
/**
@@ -1237,15 +1212,21 @@
return features;
}
+ @Override
+ public String toString() {
+ return "RuleContext(" + getLabel() + ", " + getConfiguration() + ")";
+ }
+
/**
* Builder class for a RuleContext.
*/
- public static final class Builder {
+ public static final class Builder implements RuleErrorConsumer {
private final AnalysisEnvironment env;
private final Rule rule;
private final BuildConfiguration configuration;
private final BuildConfiguration hostConfiguration;
private final PrerequisiteValidator prerequisiteValidator;
+ private final ErrorReporter reporter;
private ListMultimap<Attribute, ConfiguredTarget> prerequisiteMap;
private Set<ConfigMatchingProvider> configConditions;
private NestedSet<PackageSpecification> visibility;
@@ -1258,6 +1239,7 @@
this.configuration = Preconditions.checkNotNull(configuration);
this.hostConfiguration = Preconditions.checkNotNull(hostConfiguration);
this.prerequisiteValidator = prerequisiteValidator;
+ reporter = new ErrorReporter(env, rule);
}
RuleContext build() {
@@ -1405,41 +1387,32 @@
return mapBuilder.build();
}
- private String prefixRuleMessage(String message) {
- return String.format("in %s rule %s: %s", rule.getRuleClass(), rule.getLabel(), message);
- }
-
- private String maskInternalAttributeNames(String name) {
- return Attribute.isImplicit(name) ? "(an implicit dependency)" : name;
- }
-
- private String prefixAttributeMessage(String attrName, String message) {
- return String.format("in %s attribute of %s rule %s: %s",
- maskInternalAttributeNames(attrName), rule.getRuleClass(), rule.getLabel(), message);
- }
-
public void reportError(Location location, String message) {
- env.getEventHandler().handle(Event.error(location, message));
+ reporter.reportError(location, message);
}
+ @Override
public void ruleError(String message) {
- reportError(rule.getLocation(), prefixRuleMessage(message));
+ reporter.ruleError(message);
}
+ @Override
public void attributeError(String attrName, String message) {
- reportError(rule.getAttributeLocation(attrName), prefixAttributeMessage(attrName, message));
+ reporter.attributeError(attrName, message);
}
public void reportWarning(Location location, String message) {
- env.getEventHandler().handle(Event.warn(location, message));
+ reporter.reportWarning(location, message);
}
+ @Override
public void ruleWarning(String message) {
- env.getEventHandler().handle(Event.warn(rule.getLocation(), prefixRuleMessage(message)));
+ reporter.ruleWarning(message);
}
+ @Override
public void attributeWarning(String attrName, String message) {
- reportWarning(rule.getAttributeLocation(attrName), prefixAttributeMessage(attrName, message));
+ reporter.attributeWarning(attrName, message);
}
private void reportBadPrerequisite(Attribute attribute, String targetKind,
@@ -1584,8 +1557,92 @@
}
}
- @Override
- public String toString() {
- return "RuleContext(" + getLabel() + ", " + getConfiguration() + ")";
+ /**
+ * Helper class for reporting errors and warnings.
+ */
+ public static final class ErrorReporter implements RuleErrorConsumer {
+ private final AnalysisEnvironment env;
+ private final Rule rule;
+
+ ErrorReporter(AnalysisEnvironment env, Rule rule) {
+ this.env = env;
+ this.rule = rule;
+ }
+
+ public void reportError(Location location, String message) {
+ env.getEventHandler().handle(Event.error(location, message));
+ }
+
+ @Override
+ public void ruleError(String message) {
+ reportError(rule.getLocation(), prefixRuleMessage(message));
+ }
+
+ @Override
+ public void attributeError(String attrName, String message) {
+ reportError(getAttributeLocation(attrName), completeAttributeMessage(attrName, message));
+ }
+
+ public void reportWarning(Location location, String message) {
+ env.getEventHandler().handle(Event.warn(location, message));
+ }
+
+ @Override
+ public void ruleWarning(String message) {
+ env.getEventHandler().handle(Event.warn(rule.getLocation(), prefixRuleMessage(message)));
+ }
+
+ @Override
+ public void attributeWarning(String attrName, String message) {
+ reportWarning(getAttributeLocation(attrName), completeAttributeMessage(attrName, message));
+ }
+
+ private String prefixRuleMessage(String message) {
+ return String.format("in %s rule %s: %s", rule.getRuleClass(), rule.getLabel(), message);
+ }
+
+ private String maskInternalAttributeNames(String name) {
+ return Attribute.isImplicit(name) ? "(an implicit dependency)" : name;
+ }
+
+ /**
+ * Prefixes the given message with details about the rule and appends details about the macro
+ * that created this rule, if applicable.
+ */
+ private String completeAttributeMessage(String attrName, String message) {
+ // Appends a note to the given message if the offending rule was created by a macro.
+ String macroMessageAppendix =
+ wasCreatedByMacro()
+ ? String.format(
+ ". Since this rule was created by the macro '%s', the error might have been "
+ + "caused by the macro implementation in %s",
+ getGeneratorFunction(), rule.getAttributeLocation(attrName))
+ : "";
+
+ return String.format("in %s attribute of %s rule %s: %s%s",
+ maskInternalAttributeNames(attrName), rule.getRuleClass(), rule.getLabel(), message,
+ macroMessageAppendix);
+ }
+
+ /**
+ * Returns the location of the specified attribute.
+ *
+ * <p>If the rule was created by a macro, we return the location from the BUILD file instead.
+ */
+ private Location getAttributeLocation(String attrName) {
+ // TODO(bazel-team): We assume that macros have set the rule location to the generator
+ // location. It would be better to read the "generator_location" attribute (which is currently
+ // not implemented).
+ return wasCreatedByMacro() ? rule.getLocation() : rule.getAttributeLocation(attrName);
+ }
+
+ private boolean wasCreatedByMacro() {
+ String generator = getGeneratorFunction();
+ return generator != null && !generator.isEmpty();
+ }
+
+ private String getGeneratorFunction() {
+ return (String) rule.getAttributeContainer().getAttr("generator_function");
+ }
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index b7b778a..42e12bd 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -71,6 +71,7 @@
"exports_files(['JavaBuilder_deploy.jar','SingleJar_deploy.jar',",
" 'JavaBuilderCanary_deploy.jar', 'ijar', 'GenClass_deploy.jar'])");
config.create("tools/cpp/BUILD",
+ "cc_library(name = 'stl')",
"filegroup(name = 'toolchain', ",
" srcs = [':cc-compiler-local', ':cc-compiler-darwin', ':cc-compiler-piii',",
" ':cc-compiler-armeabi-v7a', ':empty'],",
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 9259ad7..b12dd9f 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
@@ -72,6 +72,99 @@
")");
}
+ private void setUpAttributeErrorTest() throws Exception {
+ scratch.file("test/BUILD",
+ "load('/test/macros', 'macro_native_rule', 'macro_skylark_rule', 'skylark_rule')",
+ "macro_native_rule(name = 'm_native',",
+ " deps = [':jlib'])",
+ "macro_skylark_rule(name = 'm_skylark',",
+ " deps = [':jlib'])",
+ "java_library(name = 'jlib',",
+ " srcs = ['bla.java'])",
+ "cc_library(name = 'cclib',",
+ " deps = [':jlib'])",
+ "skylark_rule(name = 'skyrule',",
+ " deps = [':jlib'])");
+ scratch.file("test/macros.bzl",
+ "def _impl(ctx):",
+ " return",
+ "skylark_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(providers = ['some_provider'], allow_files=True)",
+ " }",
+ ")",
+ "def macro_native_rule(name, deps): ",
+ " native.cc_library(name = name, deps = deps)",
+ "def macro_skylark_rule(name, deps):",
+ " skylark_rule(name = name, deps = deps)");
+ reporter.removeHandler(failFastHandler);
+ }
+
+ public void testNativeRuleAttributeErrorWithMacro() throws Exception {
+ setUpAttributeErrorTest();
+ try {
+ createRuleContext("//test:m_native");
+ fail("Should have failed because of invalid dependency");
+ } catch (Exception ex) {
+ // Macro creates native rule -> location points to the rule and the message contains details
+ // about the macro.
+ assertContainsEvent(
+ "ERROR /workspace/test/BUILD:2:1: in deps attribute of cc_library rule //test:m_native: "
+ + "java_library rule '//test:jlib' is misplaced here (expected ");
+ // Skip the part of the error message that has details about the allowed deps since the mocks
+ // for the mac tests might have different values for them.
+ assertContainsEvent(". Since this "
+ + "rule was created by the macro 'macro_native_rule', the error might have been caused "
+ + "by the macro implementation in /workspace/test/macros.bzl:10:41");
+ }
+ }
+ public void testSkylarkRuleAttributeErrorWithMacro() throws Exception {
+ setUpAttributeErrorTest();
+ try {
+ createRuleContext("//test:m_skylark");
+ fail("Should have failed because of invalid attribute value");
+ } catch (Exception ex) {
+ // Macro creates Skylark rule -> location points to the rule and the message contains details
+ // about the macro.
+ assertContainsEvent(
+ "ERROR /workspace/test/BUILD:4:1: in deps attribute of skylark_rule rule "
+ + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. "
+ + "Since this rule was created by the macro 'macro_skylark_rule', the error might have "
+ + "been caused by the macro implementation in /workspace/test/macros.bzl:12:36");
+ }
+ }
+ public void testNativeRuleAttributeErrorWithoutMacro() throws Exception {
+ setUpAttributeErrorTest();
+ try {
+ createRuleContext("//test:cclib");
+ fail("Should have failed because of invalid dependency");
+ } catch (Exception ex) {
+ // Native rule WITHOUT macro -> location points to the attribute and there is no mention of
+ // 'macro' at all.
+ assertContainsEvent("ERROR /workspace/test/BUILD:9:10: in deps attribute of "
+ + "cc_library rule //test:cclib: java_library rule '//test:jlib' is misplaced here "
+ + "(expected ");
+ // Skip the part of the error message that has details about the allowed deps since the mocks
+ // for the mac tests might have different values for them.
+ assertDoesNotContainEvent("Since this rule was created by the macro");
+ }
+ }
+
+ public void testSkylarkRuleAttributeErrorWithoutMacro() throws Exception {
+ setUpAttributeErrorTest();
+ try {
+ createRuleContext("//test:skyrule");
+ fail("Should have failed because of invalid dependency");
+ } catch (Exception ex) {
+ // Skylark rule WITHOUT macro -> location points to the attribute and there is no mention of
+ // 'macro' at all.
+ assertContainsEvent("ERROR /workspace/test/BUILD:11:10: in deps attribute of "
+ + "skylark_rule rule //test:skyrule: '//test:jlib' does not have mandatory provider "
+ + "'some_provider'");
+ }
+ }
+
public void testGetPrerequisiteArtifacts() throws Exception {
SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
Object result = evalRuleContextCode(ruleContext, "ruleContext.files.srcs");
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
index c2c01d2..a864694 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
@@ -359,7 +359,9 @@
String expectedEvent,
Set<EventKind> kinds) {
for (Event event : eventCollector) {
- if (event.getMessage().contains(expectedEvent) && kinds.contains(event.getKind())) {
+ // We want to be able to check for the location and the message type (error / warning).
+ // Consequently, we use toString() instead of getMessage().
+ if (event.toString().contains(expectedEvent) && kinds.contains(event.getKind())) {
return event;
}
}
