Fix `equals()` and `hashCode()` for artifacts: artifacts of different classes are not equal.

Also validate that there are no tree and file artifacts with the same exec path.

Fixes #4668.

Closes #5284.

Change-Id: Id97c0407a476a5bfc697b4ca7b858e3d0c0f8c75
PiperOrigin-RevId: 198540425

11 files changed, 149 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index 95714cdc8a..f18e14f0d0 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -228,7 +228,7 @@ public class Artifact
       throw new IllegalArgumentException(
           "it is illegal to create an artifact with an empty execPath");
     }
-    this.hashCode = execPath.hashCode();
+    this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
     this.root = root;
     this.execPath = execPath;
     this.rootRelativePath = rootRelativePath;
@@ -641,6 +641,9 @@ public class Artifact
     if (!(other instanceof Artifact)) {
       return false;
     }
+    if (!getClass().equals(other.getClass())) {
+      return false;
+    }
     // We don't bother to check root in the equivalence relation, because we
     // assume that no root is an ancestor of another one.
     Artifact that = (Artifact) other;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
index e417853ce1..ee6063ec6f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
@@ -154,5 +154,11 @@ public interface AnalysisEnvironment extends ActionRegistry {
    */
   ImmutableSet<Artifact> getOrphanArtifacts();
 
+  /**
+   * Returns the set of tree artifacts that have the same exec path as some other artifacts. Should
+   * only be called after the ConfiguredTarget is created.
+   */
+  ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles();
+
   ActionKeyContext getActionKeyContext();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
index d41125096f..8a3faea0a3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
@@ -164,11 +164,45 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment {
   @Override
   public ImmutableSet<Artifact> getOrphanArtifacts() {
     if (!allowRegisteringActions) {
-      return ImmutableSet.<Artifact>of();
+      return ImmutableSet.of();
     }
     return ImmutableSet.copyOf(getOrphanArtifactMap().keySet());
   }
 
+  @Override
+  public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
+    if (!allowRegisteringActions) {
+      return ImmutableSet.of();
+    }
+
+    boolean hasTreeArtifacts = false;
+    for (Artifact artifact : artifacts.keySet()) {
+      if (artifact.isTreeArtifact()) {
+        hasTreeArtifacts = true;
+        break;
+      }
+    }
+    if (!hasTreeArtifacts) {
+      return ImmutableSet.of();
+    }
+
+    HashSet<PathFragment> collect = new HashSet<>();
+    for (Artifact artifact : artifacts.keySet()) {
+      if (!artifact.isSourceArtifact() && !artifact.isTreeArtifact()) {
+        collect.add(artifact.getExecPath());
+      }
+    }
+
+    ImmutableSet.Builder<Artifact> sameExecPathTreeArtifacts = ImmutableSet.builder();
+    for (Artifact artifact : artifacts.keySet()) {
+      if (artifact.isTreeArtifact() && collect.contains(artifact.getExecPath())) {
+        sameExecPathTreeArtifacts.add(artifact);
+      }
+    }
+
+    return sameExecPathTreeArtifacts.build();
+  }
+
   private Map<Artifact, String> getOrphanArtifactMap() {
     // Construct this set to avoid poor performance under large --runs_per_test.
     Set<Artifact> artifactsWithActions = new HashSet<>();
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 2d3478ab3b..ca706fc105 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
@@ -554,7 +554,7 @@ public final class RuleContext extends TargetContext
    * which this target (which must be an OutputFile or a Rule) is associated.
    */
   public Artifact createOutputArtifact() {
-    return internalCreateOutputArtifact(getTarget());
+    return internalCreateOutputArtifact(getTarget(), OutputFile.Kind.FILE);
   }
 
   /**
@@ -563,7 +563,7 @@ public final class RuleContext extends TargetContext
    * @see #createOutputArtifact()
    */
   public Artifact createOutputArtifact(OutputFile out) {
-    return internalCreateOutputArtifact(out);
+    return internalCreateOutputArtifact(out, out.getKind());
   }
 
   /**
@@ -572,13 +572,22 @@ public final class RuleContext extends TargetContext
    * {@link #createOutputArtifact(OutputFile)} can have a more specific
    * signature.
    */
-  private Artifact internalCreateOutputArtifact(Target target) {
+  private Artifact internalCreateOutputArtifact(Target target, OutputFile.Kind outputFileKind) {
     Preconditions.checkState(
         target.getLabel().getPackageIdentifier().equals(getLabel().getPackageIdentifier()),
         "Creating output artifact for target '%s' in different package than the rule '%s' "
             + "being analyzed", target.getLabel(), getLabel());
     ArtifactRoot root = getBinOrGenfilesDirectory();
-    return getPackageRelativeArtifact(target.getName(), root);
+    PathFragment packageRelativePath = getPackageDirectory()
+        .getRelative(PathFragment.create(target.getName()));
+    switch (outputFileKind) {
+      case FILE:
+        return getDerivedArtifact(packageRelativePath, root);
+      case FILESET:
+        return getAnalysisEnvironment().getFilesetArtifact(packageRelativePath, root);
+      default:
+        throw new IllegalStateException();
+    }
   }
 
   /**
@@ -604,6 +613,14 @@ public final class RuleContext extends TargetContext
    * Creates an artifact in a directory that is unique to the package that contains the rule, thus
    * guaranteeing that it never clashes with artifacts created by rules in other packages.
    */
+  public Artifact getPackageRelativeTreeArtifact(String relative, ArtifactRoot root) {
+    return getPackageRelativeTreeArtifact(PathFragment.create(relative), root);
+  }
+
+  /**
+   * Creates an artifact in a directory that is unique to the package that contains the rule, thus
+   * guaranteeing that it never clashes with artifacts created by rules in other packages.
+   */
   public Artifact getBinArtifact(String relative) {
     return getBinArtifact(PathFragment.create(relative));
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
index 95e48a390a..43de2518e0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
@@ -13,30 +13,38 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.syntax.EvalException;
 
-
 /**
  * Utility class to validate results of executing Skylark rules and aspects.
  */
 public class SkylarkProviderValidationUtil {
-  public static void checkOrphanArtifacts(RuleContext ruleContext) throws EvalException {
+  public static void validateArtifacts(RuleContext ruleContext) throws EvalException {
+    ImmutableSet<Artifact> treeArtifactsConflictingWithFiles =
+        ruleContext.getAnalysisEnvironment().getTreeArtifactsConflictingWithFiles();
+    if (!treeArtifactsConflictingWithFiles.isEmpty()) {
+      throw new EvalException(
+          null,
+          "The following directories were also declared as files:\n"
+              + artifactsDescription(treeArtifactsConflictingWithFiles));
+    }
+
     ImmutableSet<Artifact> orphanArtifacts =
         ruleContext.getAnalysisEnvironment().getOrphanArtifacts();
     if (!orphanArtifacts.isEmpty()) {
-      throw new EvalException(null, "The following files have no generating action:\n"
-          + Joiner.on("\n").join(Iterables.transform(orphanArtifacts,
-          new Function<Artifact, String>() {
-            @Override
-            public String apply(Artifact artifact) {
-              return artifact.getRootRelativePathString();
-            }
-          })));
+      throw new EvalException(
+          null,
+          "The following files have no generating action:\n"
+              + artifactsDescription(orphanArtifacts));
     }
   }
+
+  private static String artifactsDescription(ImmutableSet<Artifact> artifacts) {
+    return Joiner.on("\n")
+        .join(Iterables.transform(artifacts, Artifact::getRootRelativePathString));
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
index d118a4ac5e..a13b2818f0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -105,14 +105,22 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi {
   @Override
   public Artifact declareDirectory(String filename, Object sibling) throws EvalException {
     context.checkMutable("actions.declare_directory");
+    Artifact result;
     if (Runtime.NONE.equals(sibling)) {
-      return ruleContext.getPackageRelativeTreeArtifact(
-          PathFragment.create(filename), newFileRoot());
+      result =
+          ruleContext.getPackageRelativeTreeArtifact(PathFragment.create(filename), newFileRoot());
     } else {
       PathFragment original = ((Artifact) sibling).getRootRelativePath();
       PathFragment fragment = original.replaceName(filename);
-      return ruleContext.getTreeArtifact(fragment, newFileRoot());
+      result = ruleContext.getTreeArtifact(fragment, newFileRoot());
     }
+    if (!result.isTreeArtifact()) {
+      throw new EvalException(
+          null,
+          String.format(
+              "'%s' has already been declared as a regular file, not directory.", filename));
+    }
+    return result;
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index 2c9b62fe6b..1a687c16c1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -122,7 +122,7 @@ public final class SkylarkRuleConfiguredTargetUtil {
         return null;
       }
       ConfiguredTarget configuredTarget = createTarget(skylarkRuleContext, target);
-      SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
+      SkylarkProviderValidationUtil.validateArtifacts(ruleContext);
       checkDeclaredProviders(configuredTarget, advertisedProviders, location);
       return configuredTarget;
     } catch (EvalException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
index 101cb1b394..90f6707d9a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
@@ -22,14 +22,35 @@ import com.google.devtools.build.lib.events.Location;
  */
 public final class OutputFile extends FileTarget {
 
+  /**
+   * A kind of output file.
+   *
+   * The FILESET kind is only supported for a non-open-sourced {@code fileset} rule.
+   */
+  public enum Kind {
+    FILE,
+    FILESET
+  }
+
   private final Rule generatingRule;
+  private final Kind kind;
 
   /**
    * Constructs an output file with the given label, which must be in the given
    * package.
    */
   OutputFile(Package pkg, Label label, Rule generatingRule) {
+    this(pkg, label, Kind.FILE, generatingRule);
+  }
+
+  /**
+   * Constructs an output file with the given label, which must be in the given
+   * package.
+   */
+  OutputFile(Package pkg, Label label,
+      Kind kind, Rule generatingRule) {
     super(pkg, label);
+    this.kind = kind;
     this.generatingRule = generatingRule;
   }
 
@@ -50,6 +71,13 @@ public final class OutputFile extends FileTarget {
     return generatingRule;
   }
 
+  /**
+   * Returns the kind of this output file.
+   */
+  public Kind getKind() {
+    return kind;
+  }
+
   @Override
   public String getTargetKind() {
     return targetKind();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index c7cca63e85..37c8767a1e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -607,7 +607,7 @@ public final class Rule implements Target, DependencyFilter.AttributeInfoProvide
       reportWarning("target '" + getName() + "' is both a rule and a file; please choose "
                     + "another name for the rule", eventHandler);
     }
-    OutputFile outputFile = new OutputFile(pkg, label, this);
+    OutputFile outputFile = new OutputFile(pkg, label, ruleClass.getOutputFileKind(), this);
     outputFilesBuilder.add(outputFile);
     return outputFile;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 0abecd2fdc..d83246871d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -615,6 +615,7 @@ public class RuleClass {
     private final Map<String, Attribute> attributes = new LinkedHashMap<>();
     private final Set<Label> requiredToolchains = new HashSet<>();
     private boolean supportsPlatforms = true;
+    private OutputFile.Kind outputFileKind = OutputFile.Kind.FILE;
 
     /**
      * Constructs a new {@code RuleClassBuilder} using all attributes from all
@@ -734,6 +735,7 @@ public class RuleClass {
           supportsConstraintChecking,
           requiredToolchains,
           supportsPlatforms,
+          outputFileKind,
           attributes.values());
     }
 
@@ -1080,6 +1082,18 @@ public class RuleClass {
     }
 
     /**
+     * Sets the kind of output files this rule creates.
+     * DO NOT USE! This only exists to support the non-open-sourced {@code fileset} rule.
+     * {@see OutputFile.Kind}.
+     */
+    public Builder setOutputFileKind(OutputFile.Kind outputFileKind) {
+      this.outputFileKind = outputFileKind;
+      return this;
+    }
+
+
+
+    /**
      * Declares that instances of this rule are compatible with the specified environments,
      * in addition to the defaults declared by their environment groups. This can be overridden
      * by rule-specific declarations. See
@@ -1264,6 +1278,7 @@ public class RuleClass {
   @Nullable private final Label ruleDefinitionEnvironmentLabel;
 
   @Nullable private final String ruleDefinitionEnvironmentHashCode;
+  private final OutputFile.Kind outputFileKind;
 
   /**
    * The set of configuration fragments which are legal for this rule's implementation to access.
@@ -1328,6 +1343,7 @@ public class RuleClass {
       boolean supportsConstraintChecking,
       Set<Label> requiredToolchains,
       boolean supportsPlatforms,
+      OutputFile.Kind  outputFileKind,
       Collection<Attribute> attributes) {
     this.name = name;
     this.key = key;
@@ -1350,6 +1366,7 @@ public class RuleClass {
     this.optionReferenceFunction = optionReferenceFunction;
     this.ruleDefinitionEnvironmentLabel = ruleDefinitionEnvironmentLabel;
     this.ruleDefinitionEnvironmentHashCode = ruleDefinitionEnvironmentHashCode;
+    this.outputFileKind = outputFileKind;
     validateNoClashInPublicNames(attributes);
     this.attributes = ImmutableList.copyOf(attributes);
     this.workspaceOnly = workspaceOnly;
@@ -2174,6 +2191,11 @@ public class RuleClass {
     return supportsPlatforms;
   }
 
+  @Nullable
+  public OutputFile.Kind  getOutputFileKind() {
+    return outputFileKind;
+  }
+
   public static boolean isThirdPartyPackage(PackageIdentifier packageIdentifier) {
     if (!packageIdentifier.getRepository().isMain()) {
       return false;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index fad7c84150..fa0eefb361 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -146,7 +146,7 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
     }
 
     ConfiguredAspect configuredAspect = builder.build();
-    SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
+    SkylarkProviderValidationUtil.validateArtifacts(ruleContext);
     return configuredAspect;
   }