Clean up analysis error reporting a bit:

1. Make --analysis_warnings_as_errors a no-op.
2. Stop doing a sanity check that we didn't succeed in analysis even with errors emitted.
3. As a result, emit an error about shared actions to the proper listener.

--
MOS_MIGRATED_REVID=92262247

4 files changed, 11 insertions, 171 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 1ad6f324c4..8ea61af5a1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -45,12 +45,9 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.events.DelegatingEventHandler;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.StoredEventHandler;
-import com.google.devtools.build.lib.events.WarningsAsErrorsEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -155,6 +152,8 @@ public class BuildView {
     public boolean keepGoing;
 
     @Option(name = "analysis_warnings_as_errors",
+            deprecationWarning = "analysis_warnings_as_errors is now a no-op and will be removed in"
+                + " an upcoming Blaze release",
             defaultValue = "false",
             category = "strategy",
             help = "Treat visible analysis warnings as errors.")
@@ -550,34 +549,6 @@ public class BuildView {
       BuildConfigurationCollection configurations, Options viewOptions,
       TopLevelArtifactContext topLevelOptions, EventHandler eventHandler, EventBus eventBus)
           throws ViewCreationFailedException, InterruptedException {
-
-    // Detect errors during analysis and don't attempt a build.
-    //
-    // (Errors reported during the previous step, package loading, that do
-    // not cause the visitation of the transitive closure to abort, are
-    // recoverable.  For example, an error encountered while evaluating an
-    // irrelevant rule in a visited package causes an error to be reported,
-    // but visitation still succeeds.)
-    ErrorCollector errorCollector = null;
-    if (!viewOptions.keepGoing) {
-      eventHandler = errorCollector = new ErrorCollector(eventHandler);
-    }
-
-    // Treat analysis warnings as errors, to enable strict builds.
-    //
-    // Warnings reported during analysis are converted to errors, ultimately
-    // triggering failure. This check needs to be added after the keep-going check
-    // above so that it is invoked first (FIFO eventHandler chain). This way, detected
-    // warnings are converted to errors first, and then the proper error handling
-    // logic is invoked.
-    WarningsAsErrorsEventHandler warningsHandler = null;
-    if (viewOptions.analysisWarningsAsErrors) {
-      eventHandler = warningsHandler = new WarningsAsErrorsEventHandler(eventHandler);
-    }
-
-    skyframeBuildView.setWarningListener(eventHandler);
-    skyframeExecutor.setErrorEventListener(eventHandler);
-
     LOG.info("Starting analysis");
     pollInterruptedStatus();
 
@@ -631,7 +602,6 @@ public class BuildView {
         });
 
     prepareToBuild(new SkyframePackageRootResolver(skyframeExecutor));
-    skyframeBuildView.setWarningListener(warningsHandler);
     skyframeExecutor.injectWorkspaceStatusData();
     Collection<ConfiguredTarget> configuredTargets;
     try {
@@ -651,49 +621,12 @@ public class BuildView {
       LOG.info(msg);
     }
 
-    postUpdateValidation(errorCollector, warningsHandler);
-
     AnalysisResult result = createResult(loadingResult, topLevelOptions,
         viewOptions, configuredTargets, analysisSuccessful);
     LOG.info("Finished analysis");
     return result;
   }
 
-  // Validates that the update has been done correctly
-  private void postUpdateValidation(ErrorCollector errorCollector,
-      WarningsAsErrorsEventHandler warningsHandler) throws ViewCreationFailedException {
-    if (warningsHandler != null && warningsHandler.warningsEncountered()) {
-      throw new ViewCreationFailedException("Warnings being treated as errors");
-    }
-
-    if (errorCollector != null && !errorCollector.getEvents().isEmpty()) {
-      // This assertion ensures that if any errors were reported during the
-      // initialization phase, the call to configureTargets will fail with a
-      // ViewCreationFailedException.  Violation of this invariant leads to
-      // incorrect builds, because the fact that errors were encountered is not
-      // properly recorded in the view (i.e. the graph of configured targets).
-      // Rule errors must be reported via RuleConfiguredTarget.reportError,
-      // which causes the rule's hasErrors() flag to be set, and thus the
-      // hasErrors() flag of anything that depends on it transitively.  If the
-      // toplevel rule hasErrors, then analysis is aborted and we do not
-      // proceed to the execution phase of a build.
-      //
-      // Reporting errors directly through the Reporter does not set the error
-      // flag, so analysis may succeed spuriously, allowing the execution
-      // phase to begin with unpredictable consequences.
-      //
-      // The use of errorCollector (rather than an ErrorSensor) makes the
-      // assertion failure messages more informative.
-      // Note we tolerate errors iff --keep-going, because some of the
-      // requested targets may have had problems during analysis, but that's ok.
-      StringBuilder message = new StringBuilder("Unexpected errors reported during analysis:");
-      for (Event event : errorCollector.getEvents()) {
-        message.append('\n').append(event);
-      }
-      throw new IllegalStateException(message.toString());
-    }
-  }
-
   private AnalysisResult createResult(LoadingResult loadingResult,
       TopLevelArtifactContext topLevelOptions, BuildView.Options viewOptions,
       Collection<ConfiguredTarget> configuredTargets, boolean analysisSuccessful)
@@ -1020,34 +953,4 @@ public class BuildView {
     skyframeAnalysisWasDiscarded = true;
     skyframeExecutor.clearAnalysisCache(topLevelTargets);
   }
-
-  /********************************************************************
-   *                                                                  *
-   *                  'blaze dump' related functions                  *
-   *                                                                  *
-   ********************************************************************/
-
-  /**
-   * Collects and stores error events while also forwarding them to another eventHandler.
-   */
-  public static class ErrorCollector extends DelegatingEventHandler {
-    private final List<Event> events;
-
-    public ErrorCollector(EventHandler delegate) {
-      super(delegate);
-      this.events = Lists.newArrayList();
-    }
-
-    public List<Event> getEvents() {
-      return events;
-    }
-
-    @Override
-    public void handle(Event e) {
-      super.handle(e);
-      if (e.getKind() == EventKind.ERROR) {
-        events.add(e);
-      }
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/events/WarningsAsErrorsEventHandler.java b/src/main/java/com/google/devtools/build/lib/events/WarningsAsErrorsEventHandler.java
deleted file mode 100644
index 91a215098f..0000000000
--- a/src/main/java/com/google/devtools/build/lib/events/WarningsAsErrorsEventHandler.java
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright 2014 Google Inc. 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.events;
-
-/**
- * Passes through any events, and converts any warnings to errors.
- */
-public final class WarningsAsErrorsEventHandler extends DelegatingEventHandler {
-
-  boolean warningsEncountered = false;
-
-  public WarningsAsErrorsEventHandler(EventHandler eventHandler) {
-    super(eventHandler);
-  }
-
-  @Override
-  public synchronized void handle(Event e) {
-    if (e.getKind() == EventKind.WARNING) {
-      warningsEncountered = true;
-      super.handle(new Event(EventKind.ERROR, e.getLocation(), e.getMessage()));
-    } else {
-      super.handle(e);
-    }
-  }
-
-  public boolean warningsEncountered() {
-    return warningsEncountered;
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 59b242f364..d094eba092 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -78,7 +78,6 @@ public final class SkyframeBuildView {
 
   private final ConfiguredTargetFactory factory;
   private final ArtifactFactory artifactFactory;
-  @Nullable private EventHandler warningListener;
   private final SkyframeExecutor skyframeExecutor;
   private final Runnable legacyDataCleaner;
   private final BinTools binTools;
@@ -109,10 +108,6 @@ public final class SkyframeBuildView {
     skyframeExecutor.setArtifactFactoryAndBinTools(artifactFactory, binTools);
   }
 
-  public void setWarningListener(@Nullable EventHandler warningListener) {
-    this.warningListener = warningListener;
-  }
-
   public void resetEvaluatedConfiguredTargetKeysSet() {
     evaluatedConfiguredTargets.clear();
   }
@@ -249,14 +244,13 @@ public final class SkyframeBuildView {
         } else {
           root = maybeGetConfiguredTargetCycleCulprit(errorInfo.getCycleInfo());
         }
-        if (warningListener != null) {
-          Exception cause = errorInfo.getException();
-          if (cause instanceof ActionConflictException) {
-            ((ActionConflictException) cause).reportTo(warningListener);
-          }
-          warningListener.handle(Event.warn("errors encountered while analyzing target '"
-              + label + "': it will not be built"));
+        Exception cause = errorInfo.getException();
+        if (cause instanceof ActionConflictException) {
+          ((ActionConflictException) cause).reportTo(skyframeExecutor.getReporter());
         }
+        skyframeExecutor.getReporter().handle(
+            Event.warn("errors encountered while analyzing target '"
+                + label + "': it will not be built"));
         eventBus.post(new AnalysisFailureEvent(
             LabelAndConfiguration.of(label.getLabel(), label.getConfiguration()), root));
       }
@@ -269,10 +263,9 @@ public final class SkyframeBuildView {
         ex.rethrowTyped();
       } catch (MutableActionGraph.ActionConflictException ace) {
         ace.reportTo(skyframeExecutor.getReporter());
-        if (warningListener != null) {
-          warningListener.handle(Event.warn("errors encountered while analyzing target '"
-              + bad.getKey().getOwner().getLabel() + "': it will not be built"));
-        }
+        skyframeExecutor.getReporter()
+            .handle(Event.warn("errors encountered while analyzing target '"
+                + bad.getKey().getOwner().getLabel() + "': it will not be built"));
       } catch (ArtifactPrefixConflictException apce) {
         if (reportedExceptions.add(apce)) {
           skyframeExecutor.getReporter().handle(Event.error(apce.getMessage()));
@@ -327,11 +320,6 @@ public final class SkyframeBuildView {
     return artifactFactory;
   }
 
-  @Nullable
-  EventHandler getWarningListener() {
-    return warningListener;
-  }
-
   /**
    * Because we don't know what build-info artifacts this configured target may request, we
    * conservatively register a dep on all of them.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 6e668aeede..a675bc8623 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -798,9 +798,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   public void setSkyframeBuildView(SkyframeBuildView skyframeBuildView) {
     this.skyframeBuildView = skyframeBuildView;
     this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
-    if (skyframeBuildView.getWarningListener() != null) {
-      setErrorEventListener(skyframeBuildView.getWarningListener());
-    }
   }
 
   /**
@@ -811,13 +808,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   /**
-   * Sets the eventHandler to use for reporting errors.
-   */
-  public void setErrorEventListener(EventHandler eventHandler) {
-    this.errorEventListener = eventHandler;
-  }
-
-  /**
    * Sets the path for action log buffers.
    */
   public void setActionOutputRoot(Path actionOutputRoot) {