Make ActionOwner a final class, since all non-test implementations were basically doing the same thing with it.

This simplifies the code and avoids unnecessary re-wrapping, which saves memory.

--
MOS_MIGRATED_REVID=117693050

8 files changed, 82 insertions, 233 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 7ce6b212c0..3196d70530 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -26,7 +26,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 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.Location;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -123,7 +122,7 @@ public abstract class AbstractAction implements Action, SkylarkValue {
       Iterable<Artifact> outputs) {
     Preconditions.checkNotNull(owner);
     // TODO(bazel-team): Use RuleContext.actionOwner here instead
-    this.owner = new ActionOwnerDescription(owner);
+    this.owner = owner;
     this.tools = CollectionUtils.makeImmutable(tools);
     this.inputs = CollectionUtils.makeImmutable(inputs);
     this.outputs = ImmutableSet.copyOf(outputs);
@@ -427,59 +426,4 @@ public abstract class AbstractAction implements Action, SkylarkValue {
     return getInputs();
   }
 
-  /**
-   * A copying implementation of {@link ActionOwner}.
-   *
-   * <p>ConfiguredTargets implement ActionOwner themselves, but we do not want actions
-   * to keep direct references to configured targets just for a label and a few strings.
-   */
-  @Immutable
-  private static class ActionOwnerDescription implements ActionOwner {
-
-    private final Location location;
-    private final Label label;
-    private final String configurationMnemonic;
-    private final String configurationChecksum;
-    private final String targetKind;
-    private final String additionalProgressInfo;
-
-    private ActionOwnerDescription(ActionOwner originalOwner) {
-      this.location = originalOwner.getLocation();
-      this.label = originalOwner.getLabel();
-      this.configurationMnemonic = originalOwner.getConfigurationMnemonic();
-      this.configurationChecksum = originalOwner.getConfigurationChecksum();
-      this.targetKind = originalOwner.getTargetKind();
-      this.additionalProgressInfo = originalOwner.getAdditionalProgressInfo();
-    }
-
-    @Override
-    public Location getLocation() {
-      return location;
-    }
-
-    @Override
-    public Label getLabel() {
-      return label;
-    }
-
-    @Override
-    public String getConfigurationMnemonic() {
-      return configurationMnemonic;
-    }
-
-    @Override
-    public String getConfigurationChecksum() {
-      return configurationChecksum;
-    }
-
-    @Override
-    public String getTargetKind() {
-      return targetKind;
-    }
-
-    @Override
-    public String getAdditionalProgressInfo() {
-      return additionalProgressInfo;
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractActionOwner.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractActionOwner.java
deleted file mode 100644
index 0acfa529d8..0000000000
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractActionOwner.java
+++ /dev/null
@@ -1,60 +0,0 @@
-// Copyright 2014 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.actions;
-
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Location;
-
-/**
- * An action owner base class that provides default implementations for some of
- * the {@link ActionOwner} methods.
- */
-public abstract class AbstractActionOwner implements ActionOwner {
-
-  @Override
-  public String getAdditionalProgressInfo() {
-    return null;
-  }
-
-  @Override
-  public Location getLocation() {
-    return null;
-  }
-
-  @Override
-  public Label getLabel() {
-    return null;
-  }
-
-  @Override
-  public String getTargetKind() {
-    return "empty target kind";
-  }
-
-  /**
-   * An action owner for special cases. Usage is strongly discouraged. 
-   */
-  public static final ActionOwner SYSTEM_ACTION_OWNER = new AbstractActionOwner() {
-    @Override
-    public String getConfigurationMnemonic() {
-      return "system";
-    }
-
-    @Override
-    public final String getConfigurationChecksum() {
-      return "system";
-    }
-  };
-}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java b/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
index b82b02505a..b0b163e396 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
@@ -15,52 +15,86 @@ package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.util.Preconditions;
+
+import javax.annotation.Nullable;
 
 /**
- * The owner of an action is responsible for reporting conflicts in the action
- * graph (two actions attempting to generate the same artifact).
+ * Contains metadata used for reporting the progress and status of an action.
  *
- * Typically an action's owner is the RuleConfiguredTarget instance responsible
- * for creating it, but to avoid coupling between the view and actions
- * packages, the RuleConfiguredTarget is hidden behind this interface, which
- * exposes only the error reporting functionality.
+ * <p>Morally an action's owner is the RuleConfiguredTarget instance responsible for creating it,
+ * but to avoid storing heavyweight analysis objects in actions, and to avoid coupling between the
+ * analysis and actions packages, the RuleConfiguredTarget provides an instance of this class.
  */
-public interface ActionOwner {
+public final class ActionOwner {
+  /** An action owner for special cases. Usage is strongly discouraged. */
+  public static final ActionOwner SYSTEM_ACTION_OWNER =
+      new ActionOwner(null, null, "system", "empty target kind", "system", null);
 
-  /**
-   * Returns the location of this ActionOwner, if any; null otherwise.
-   */
-  Location getLocation();
+  @Nullable private final Label label;
+  @Nullable private final Location location;
+  @Nullable private final String mnemonic;
+  @Nullable private final String targetKind;
+  private final String configurationChecksum;
+  @Nullable private final String additionalProgressInfo;
+
+  public ActionOwner(
+      @Nullable Label label,
+      @Nullable Location location,
+      @Nullable String mnemonic,
+      @Nullable String targetKind,
+      String configurationChecksum,
+      @Nullable String additionalProgressInfo) {
+    this.label = label;
+    this.location = location;
+    this.mnemonic = mnemonic;
+    this.targetKind = targetKind;
+    this.configurationChecksum = Preconditions.checkNotNull(configurationChecksum);
+    this.additionalProgressInfo = additionalProgressInfo;
+  }
+
+  /** Returns the location of this ActionOwner, if any; null otherwise. */
+  @Nullable
+  public Location getLocation() {
+    return location;
+  }
 
   /**
    * Returns the label for this ActionOwner, if any; null otherwise.
    */
-  Label getLabel();
+  @Nullable
+  public Label getLabel() {
+    return label;
+  }
 
-  /**
-   * Returns the configuration's mnemonic.
-   */
-  String getConfigurationMnemonic();
+  /** Returns the configuration's mnemonic. */
+  @Nullable
+  public String getConfigurationMnemonic() {
+    return mnemonic;
+  }
 
   /**
    * Returns the short cache key for the configuration of the action owner.
    *
-   * <p>Special action owners that are not targets can return any string here as long as it is
-   * constant. If the configuration is null, this should return "null".
-   *
-   * <p>These requirements exist so that {@link ActionOwner} instances are consistent with
-   * {@code BuildView.ActionOwnerIdentity(ConfiguredTargetValue)}.
+   * <p>Special action owners that are not targets can return any string here. If the
+   * underlying configuration is null, this should return "null".
    */
-  String getConfigurationChecksum();
+  public String getConfigurationChecksum() {
+    return configurationChecksum;
+  }
 
-  /**
-   * Returns the target kind (rule class name) for this ActionOwner, if any; null otherwise.
-   */
-  String getTargetKind();
+  /** Returns the target kind (rule class name) for this ActionOwner, if any; null otherwise. */
+  @Nullable
+  public String getTargetKind() {
+    return targetKind;
+  }
 
   /**
    * Returns additional information that should be displayed in progress messages, or {@code null}
    * if nothing should be added.
    */
-  String getAdditionalProgressInfo();
+  @Nullable
+  String getAdditionalProgressInfo() {
+    return additionalProgressInfo;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildInfoHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildInfoHelper.java
deleted file mode 100644
index 3510eb0bff..0000000000
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildInfoHelper.java
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright 2014 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.analysis;
-
-import com.google.devtools.build.lib.actions.AbstractActionOwner;
-import com.google.devtools.build.lib.actions.ActionOwner;
-
-// TODO(bazel-team): move BUILD_INFO_ACTION_OWNER somewhere else and remove this class.
-/**
- * Helper class for the CompatibleWriteBuildInfoAction, which holds the
- * methods for generating build information.
- * Abstracted away to allow non-action code to also generate build info under
- * --nobuild or --check_up_to_date.
- */
-public abstract class BuildInfoHelper {
-  /** ActionOwner for BuildInfoActions. */
-  public static final ActionOwner BUILD_INFO_ACTION_OWNER =
-      AbstractActionOwner.SYSTEM_ACTION_OWNER;
-}
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 1b696855e3..273d56229f 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
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
@@ -130,7 +131,7 @@ public final class RuleContext extends TargetContext
     }
   }
 
-  static final String HOST_CONFIGURATION_PROGRESS_TAG = "for host";
+  private static final String HOST_CONFIGURATION_PROGRESS_TAG = "for host";
 
   private final Rule rule;
   private final ListMultimap<String, ConfiguredTarget> targetMap;
@@ -286,7 +287,7 @@ public final class RuleContext extends TargetContext
   @Override
   public ActionOwner getActionOwner() {
     if (actionOwner == null) {
-      actionOwner = new RuleActionOwner(rule, getConfiguration());
+      actionOwner = createActionOwner(rule, getConfiguration());
     }
     return actionOwner;
   }
@@ -361,55 +362,15 @@ public final class RuleContext extends TargetContext
     return getAnalysisEnvironment().getBuildInfo(this, key);
   }
 
-  // TODO(bazel-team): This class could be simpler if Rule and BuildConfiguration classes
-  // were immutable. Then we would need to store only references those two.
-  @Immutable
-  private static final class RuleActionOwner implements ActionOwner {
-    private final Label label;
-    private final Location location;
-    private final String mnemonic;
-    private final String targetKind;
-    private final String configurationChecksum;
-    private final boolean hostConfiguration;
-
-    private RuleActionOwner(Rule rule, BuildConfiguration configuration) {
-      this.label = rule.getLabel();
-      this.location = rule.getLocation();
-      this.targetKind = rule.getTargetKind();
-      this.mnemonic = configuration.getMnemonic();
-      this.configurationChecksum = configuration.checksum();
-      this.hostConfiguration = configuration.isHostConfiguration();
-    }
-
-    @Override
-    public Location getLocation() {
-      return location;
-    }
-
-    @Override
-    public Label getLabel() {
-      return label;
-    }
-
-    @Override
-    public String getConfigurationMnemonic() {
-      return mnemonic;
-    }
-
-    @Override
-    public String getConfigurationChecksum() {
-      return configurationChecksum;
-    }
-
-    @Override
-    public String getTargetKind() {
-      return targetKind;
-    }
-
-    @Override
-    public String getAdditionalProgressInfo() {
-      return hostConfiguration ? HOST_CONFIGURATION_PROGRESS_TAG : null;
-    }
+  @VisibleForTesting
+  public static ActionOwner createActionOwner(Rule rule, BuildConfiguration configuration) {
+    return new ActionOwner(
+        rule.getLabel(),
+        rule.getLocation(),
+        configuration.getMnemonic(),
+        rule.getTargetKind(),
+        configuration.checksum(),
+        configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index c47388b6da..51b582cc50 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -23,6 +23,7 @@ import com.google.common.eventbus.Subscribe;
 import com.google.devtools.build.lib.actions.ActionContextProvider;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
@@ -32,7 +33,6 @@ import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.actions.SimpleActionContextProvider;
 import com.google.devtools.build.lib.analysis.BuildInfo;
-import com.google.devtools.build.lib.analysis.BuildInfoHelper;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Key;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.KeyType;
@@ -80,7 +80,9 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
         Path workspace,
         Artifact stableStatus,
         Artifact volatileStatus) {
-      super(BuildInfoHelper.BUILD_INFO_ACTION_OWNER, Artifact.NO_ARTIFACTS,
+      super(
+          ActionOwner.SYSTEM_ACTION_OWNER,
+          Artifact.NO_ARTIFACTS,
           ImmutableList.of(stableStatus, volatileStatus));
       this.options = Preconditions.checkNotNull(options);
       this.stableStatus = stableStatus;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java
index da42dfd6e3..b0151eb8b4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java
@@ -19,8 +19,8 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.BuildInfoHelper;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -64,8 +64,7 @@ public final class WriteBuildInfoHeaderAction extends AbstractFileWriteAction {
    */
   public WriteBuildInfoHeaderAction(Collection<Artifact> inputs,
       Artifact output, boolean writeVolatileInfo, boolean writeStableInfo) {
-    super(BuildInfoHelper.BUILD_INFO_ACTION_OWNER,
-        inputs, output, /*makeExecutable=*/false);
+    super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, output, /*makeExecutable=*/ false);
     valueArtifacts = ImmutableList.copyOf(inputs);
     if (!inputs.isEmpty()) {
       // With non-empty inputs we should not generate both volatile and non-volatile data
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java
index a60c322d6b..4439a040ce 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java
@@ -19,8 +19,8 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.BuildInfoHelper;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Key;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
@@ -126,7 +126,7 @@ public class WriteBuildInfoPropertiesAction extends AbstractFileWriteAction {
   public WriteBuildInfoPropertiesAction(Collection<Artifact> inputs, Artifact output,
       BuildInfoPropertiesTranslator keyTranslations, boolean includeVolatile,
       boolean includeNonVolatile, TimestampFormatter timestampFormatter) {
-    super(BuildInfoHelper.BUILD_INFO_ACTION_OWNER, inputs, output, /* makeExecutable= */false);
+    super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, output, /* makeExecutable= */ false);
     this.keyTranslations = keyTranslations;
     this.includeVolatile = includeVolatile;
     this.includeNonVolatile = includeNonVolatile;