Cleanup and refactor RuleErrorConsumer.
This removes the need for AbstractRuleErrorConsumer, and moves the logic of the post() method of RuleContext into RuleContext proper, as it circumvents normal error handling flows.
RELNOTES: None.
PiperOrigin-RevId: 215627666
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/EventHandlingErrorReporter.java b/src/main/java/com/google/devtools/build/lib/analysis/EventHandlingErrorReporter.java
index 9257a75..adfe113 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/EventHandlingErrorReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/EventHandlingErrorReporter.java
@@ -16,8 +16,8 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.AbstractRuleErrorConsumer;
import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.RuleErrorConsumer;
/**
* Base class for implementations of {@link
@@ -25,9 +25,9 @@
*
* <p>Do not create new implementations of this class - instead, use {@link RuleContext} in Native
* rule definitions, and {@link com.google.devtools.build.lib.analysis.skylark.SkylarkErrorReporter}
- * in Skylark API definitions. For use in testing, extend {@link AbstractRuleErrorConsumer} instead.
+ * in Skylark API definitions. For use in testing, implement {@link RuleErrorConsumer} instead.
*/
-public abstract class EventHandlingErrorReporter extends AbstractRuleErrorConsumer {
+public abstract class EventHandlingErrorReporter implements RuleErrorConsumer {
private final String ruleClassNameForLogging;
private final AnalysisEnvironment env;
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 4f80a32..f1902f2 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
@@ -1664,12 +1664,13 @@
return mapBuilder.build();
}
+ /**
+ * Post a raw event to the analysis environment's event handler. This circumvents normal
+ * error and warning reporting functionality to post events, and should only be used
+ * in rare cases where a custom event needs to be handled.
+ */
public void post(Postable event) {
- reporter.post(event);
- }
-
- public void reportError(Location location, String message) {
- reporter.reportError(location, message);
+ env.getEventHandler().post(event);
}
@Override
@@ -1988,21 +1989,15 @@
}
/** Helper class for reporting errors and warnings. */
- public static final class ErrorReporter extends EventHandlingErrorReporter
+ private static final class ErrorReporter extends EventHandlingErrorReporter
implements RuleErrorConsumer {
- private final AnalysisEnvironment env;
private final Rule rule;
ErrorReporter(AnalysisEnvironment env, Rule rule, String ruleClassNameForLogging) {
super(ruleClassNameForLogging, env);
- this.env = env;
this.rule = rule;
}
- public void post(Postable event) {
- env.getEventHandler().post(event);
- }
-
@Override
protected String getMacroMessageAppendix(String attrName) {
return rule.wasCreatedByMacro()
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 a4f2f3d..bb37e43 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
@@ -71,7 +71,7 @@
+ "the dependency is legitimate",
AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND),
rule.getLabel());
- context.reportError(rule.getLocation(), errorMessage);
+ context.ruleError(errorMessage);
}
// We can always post the visibility error as, regardless of the value of keep going,
// that target will not be built.
@@ -86,8 +86,8 @@
requiredProviders.getDescription().contains("PackageSpecificationProvider");
// TODO(plf): Add the PackageSpecificationProvider to the 'visibility' attribute.
if (!attrName.equals("visibility") && !containsPackageSpecificationProvider) {
- context.reportError(
- rule.getAttributeLocation(attrName),
+ context.attributeError(
+ attrName,
"in "
+ attrName
+ " attribute of "
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractRuleErrorConsumer.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractRuleErrorConsumer.java
deleted file mode 100644
index df039d5..0000000
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractRuleErrorConsumer.java
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright 2017 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.packages;
-
-import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
-
-/** Defines several helper methods to avoid duplication between {@link RuleErrorConsumer}s. */
-public abstract class AbstractRuleErrorConsumer implements RuleErrorConsumer {
-
- @Override
- public RuleErrorException throwWithRuleError(String message) throws RuleErrorException {
- ruleError(message);
- throw new RuleErrorException();
- }
-
- @Override
- public RuleErrorException throwWithAttributeError(String attrName, String message)
- throws RuleErrorException {
- attributeError(attrName, message);
- throw new RuleErrorException();
- }
-
- @Override
- public void assertNoErrors() throws RuleErrorException {
- if (hasErrors()) {
- throw new RuleErrorException();
- }
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleErrorConsumer.java b/src/main/java/com/google/devtools/build/lib/packages/RuleErrorConsumer.java
index 9e1683e..8b63425 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleErrorConsumer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleErrorConsumer.java
@@ -53,7 +53,10 @@
* invoke {@link #ruleError} instead to collect additional error information before ending the
* invocation.
*/
- RuleErrorException throwWithRuleError(String message) throws RuleErrorException;
+ default RuleErrorException throwWithRuleError(String message) throws RuleErrorException {
+ ruleError(message);
+ throw new RuleErrorException();
+ }
/**
* Convenience function to report attribute-specific errors in the current rule, and then throw a
@@ -64,8 +67,11 @@
* <p>If the name of the attribute starts with <code>$</code>
* it is replaced with a string <code>(an implicit dependency)</code>.
*/
- RuleErrorException throwWithAttributeError(String attrName, String message)
- throws RuleErrorException;
+ default RuleErrorException throwWithAttributeError(String attrName, String message)
+ throws RuleErrorException {
+ attributeError(attrName, message);
+ throw new RuleErrorException();
+ }
/**
* Returns whether this instance is known to have errors at this point during analysis. Do not
@@ -77,5 +83,9 @@
* No-op if {@link #hasErrors} is false, throws {@link RuleErrorException} if it is true.
* This provides a convenience to early-exit of configured target creation if there are errors.
*/
- void assertNoErrors() throws RuleErrorException;
+ default void assertNoErrors() throws RuleErrorException {
+ if (hasErrors()) {
+ throw new RuleErrorException();
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
index acf57fd..5ee1f3c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
@@ -143,9 +143,9 @@
toId(
Iterables.getOnlyElement(result.getTopLevelTargetsWithConfigs())
.getConfiguration()),
- "target '//bar:bar' is not visible from target '//foo:foo'. "
- + "Check the visibility declaration of the former target if you think the "
- + "dependency is legitimate"));
+ "in sh_library rule //foo:foo: target '//bar:bar' is not visible from target "
+ + "'//foo:foo'. Check the visibility declaration of the former target if you "
+ + "think the dependency is legitimate"));
}
// TODO(ulfjack): Add more tests for
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
index 92f5616..77db2ba 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.LocationExpander.LocationFunction;
import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.packages.AbstractRuleErrorConsumer;
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import java.util.ArrayList;
import java.util.List;
@@ -30,8 +29,7 @@
/** Unit tests for {@link LocationExpander}. */
@RunWith(JUnit4.class)
public class LocationExpanderTest {
- private static final class Capture extends AbstractRuleErrorConsumer
- implements RuleErrorConsumer {
+ private static final class Capture implements RuleErrorConsumer {
private final List<String> warnsOrErrors = new ArrayList<>();
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
index 647030a..0025cb2 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
@@ -33,7 +33,6 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
-import com.google.devtools.build.lib.packages.AbstractRuleErrorConsumer;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
@@ -74,8 +73,7 @@
};
/** A faked {@link RuleErrorConsumer} that validates that only expected errors were reported. */
- public static final class FakeRuleErrorConsumer extends AbstractRuleErrorConsumer
- implements RuleErrorConsumer {
+ public static final class FakeRuleErrorConsumer implements RuleErrorConsumer {
private String ruleErrorMessage = null;
private String attributeErrorAttribute = null;
private String attributeErrorMessage = null;
