From f6318a6779ebc5487e875d1c6a2b1e96a5c808fd Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Thu, 19 Sep 2024 13:05:19 -0700
Subject: [PATCH] Make setContainsErrors() and setIOException() void

The self-return isn't used anywhere. Anyway, that idiom is more appropriate for true builders where the information is known up-front; here it works more as a stateful mutation.

Using void simplifies pulling these methods up to a base class in a follow-up CL.

Work toward #19922.

PiperOrigin-RevId: 676531919
Change-Id: Id13f600a167ac13183129a7e87bff97e519adf97
---
 .../devtools/build/lib/packages/Package.java     | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index e38adce1fa783c..434712ed2fe38a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1097,8 +1097,6 @@ default boolean precomputeTransitiveLoads() {
     @Nullable private String ioExceptionMessage = null;
     @Nullable private IOException ioException = null;
     @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null;
-    // TODO(#19922): Consider having separate containsErrors fields on Metadata and Package. In that
-    // case, this field is replaced by the one on Metadata.
     private boolean containsErrors = false;
     // A package's FailureDetail field derives from the events on its Builder's event handler.
     // During package deserialization, those events are unavailable, because those events aren't
@@ -1673,11 +1671,11 @@ void setComputationSteps(long n) {
       pkg.computationSteps = n;
     }
 
-    Builder setIOException(IOException e, String message, DetailedExitCode detailedExitCode) {
+    void setIOException(IOException e, String message, DetailedExitCode detailedExitCode) {
       this.ioException = e;
       this.ioExceptionMessage = message;
       this.ioExceptionDetailedExitCode = detailedExitCode;
-      return setContainsErrors();
+      setContainsErrors();
     }
 
     /**
@@ -1688,13 +1686,11 @@ Builder setIOException(IOException e, String message, DetailedExitCode detailedE
     // TODO(bazel-team): For simplicity it would be nice to replace this with
     // getLocalEventHandler().hasErrors(), since that would prevent the kind of inconsistency where
     // we have reported an ERROR event but not called setContainsErrors(), or vice versa.
-    @CanIgnoreReturnValue
-    public Builder setContainsErrors() {
+    public void setContainsErrors() {
       // TODO(bazel-team): Maybe do Preconditions.checkState(localEventHandler.hasErrors()).
       // Maybe even assert that it has a FailureDetail, though that's a linear scan unless we
       // customize the event handler.
-      containsErrors = true;
-      return this;
+      this.containsErrors = true;
     }
 
     public boolean containsErrors() {
@@ -1720,7 +1716,7 @@ FailureDetail getFailureDetail() {
         if (detailedExitCode != null && detailedExitCode.getFailureDetail() != null) {
           return detailedExitCode.getFailureDetail();
         }
-        if (containsErrors) {
+        if (containsErrors()) {
           if (undetailedEvents == null) {
             undetailedEvents = new ArrayList<>();
           }
@@ -2106,7 +2102,7 @@ private void addRuleInternal(Rule rule) {
       }
       addOrReplaceTarget(rule);
       if (rule.containsErrors()) {
-        this.setContainsErrors();
+        setContainsErrors();
       }
     }