Rollback of commit 4dbd628dc1384e5dce8c036e2b6bf93dd974bf04.

[]*** Reason for rollback ***

Large number of newly broken targets found by []

*** Original change description ***

Provide placeholder rule class for deserialized Skylark rules

At this time, Skylark-defined rule classes don't get serialized, and
aren't available at package deserialization time. To allow packages
with Skylark-defined rule classes to deserialize, we provide a
placeholder rule class implementation for deserialized Skylark rules.

--
MOS_MIGRATED_REVID=97716849

5 files changed, 33 insertions, 153 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 1a52b5ed20..8afcbb7d55 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
@@ -241,7 +241,7 @@ public class Package implements Serializable {
 
   private void readObject(ObjectInputStream in) throws IOException {
     try {
-      deserializedPkg = new PackageDeserializer().deserialize(in);
+      deserializedPkg = new PackageDeserializer(null, null).deserialize(in);
     } catch (PackageDeserializationException e) {
       throw new IllegalStateException(e);
     }
@@ -450,7 +450,7 @@ public class Package implements Serializable {
    * though not necessarily: data in a subdirectory of a test package may use a
    * different filename to avoid inadvertently creating a new package.
    */
-  public Label getBuildFileLabel() {
+  Label getBuildFileLabel() {
     return buildFile.getLabel();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
index d5a612efa5..e4bc250333 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.syntax.GlobCriteria;
 import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.Label.SyntaxException;
+import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -54,23 +55,11 @@ public class PackageDeserializer {
 
   private static final Logger LOG = Logger.getLogger(PackageDeserializer.class.getName());
 
-  /**
-   * Provides the deserializer with tools it needs to build a package from its serialized form.
-   */
-  public interface PackageDeserializationEnvironment {
-
-    /** Converts the serialized package's path string into a {@link Path} object. */
-    Path getPath(String buildFilePath);
-
-    /** Returns a {@link RuleClass} object for the serialized rule. */
-    RuleClass getRuleClass(Build.Rule rulePb, Location ruleLocation);
-  }
-
-  // Workaround for Java serialization making it tough to pass in a deserialization environment
-  // manually.
+  // Workaround for Java serialization not allowing to pass in a context manually.
   // volatile is needed to ensure that the objects are published safely.
-  // TODO(bazel-team): Subclass ObjectOutputStream to pass this through instead.
-  public static volatile PackageDeserializationEnvironment defaultPackageDeserializationEnvironment;
+  // TODO(bazel-team): Subclass ObjectOutputStream to pass through environment variables.
+  public static volatile RuleClassProvider defaultRuleClassProvider;
+  public static volatile FileSystem defaultDeserializerFileSystem;
 
   private class Context {
     private final Package.Builder packageBuilder;
@@ -131,9 +120,15 @@ public class PackageDeserializer {
       }
     }
 
-    void deserializeRule(Build.Rule rulePb) throws PackageDeserializationException {
-      Location ruleLocation = deserializeLocation(rulePb.getParseableLocation());
-      RuleClass ruleClass = packageDeserializationEnvironment.getRuleClass(rulePb, ruleLocation);
+    void deserializeRule(Build.Rule rulePb)
+        throws PackageDeserializationException {
+      String ruleClassName = rulePb.getRuleClass();
+      RuleClass ruleClass = ruleClassProvider.getRuleClassMap().get(ruleClassName);
+      if (ruleClass == null) {
+        throw new PackageDeserializationException(
+            String.format("Invalid rule class '%s'", ruleClassName));
+      }
+
       Map<String, ParsedAttributeValue> attributeValues = new HashMap<>();
       for (Build.Attribute attrPb : rulePb.getAttributeList()) {
         Type<?> type = ruleClass.getAttributeByName(attrPb.getName()).getType();
@@ -141,6 +136,7 @@ public class PackageDeserializer {
       }
 
       Label ruleLabel = deserializeLabel(rulePb.getName());
+      Location ruleLocation = deserializeLocation(rulePb.getParseableLocation());
       try {
         Rule rule = ruleClass.createRuleWithParsedAttributeValues(
             ruleLabel, packageBuilder, ruleLocation, attributeValues,
@@ -154,7 +150,8 @@ public class PackageDeserializer {
     }
   }
 
-  private final PackageDeserializationEnvironment packageDeserializationEnvironment;
+  private final FileSystem fileSystem;
+  private final RuleClassProvider ruleClassProvider;
 
   @Immutable
   private static final class ExplicitLocation extends Location {
@@ -199,16 +196,15 @@ public class PackageDeserializer {
     }
   }
 
-  /**
-   * Creates a {@link PackageDeserializer} using {@link #defaultPackageDeserializationEnvironment}.
-   */
-  public PackageDeserializer() {
-    this.packageDeserializationEnvironment = defaultPackageDeserializationEnvironment;
-  }
-
-  public PackageDeserializer(PackageDeserializationEnvironment packageDeserializationEnvironment) {
-    this.packageDeserializationEnvironment =
-        Preconditions.checkNotNull(packageDeserializationEnvironment);
+  public PackageDeserializer(FileSystem fileSystem, RuleClassProvider ruleClassProvider) {
+    if (fileSystem == null) {
+      fileSystem = defaultDeserializerFileSystem;
+    }
+    this.fileSystem = Preconditions.checkNotNull(fileSystem);
+    if (ruleClassProvider == null) {
+      ruleClassProvider = defaultRuleClassProvider;
+    }
+    this.ruleClassProvider = Preconditions.checkNotNull(ruleClassProvider);
   }
 
   /**
@@ -314,7 +310,7 @@ public class PackageDeserializer {
    */
   private void deserializeInternal(Build.Package packagePb, StoredEventHandler eventHandler,
       Package.Builder builder, InputStream in) throws PackageDeserializationException, IOException {
-    Path buildFile = packageDeserializationEnvironment.getPath(packagePb.getBuildFilePath());
+    Path buildFile = fileSystem.getPath(packagePb.getBuildFilePath());
     Preconditions.checkNotNull(buildFile);
     Context context = new Context(buildFile, builder);
     builder.setFilename(buildFile);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
index 4f1a7a3008..16b318cb62 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
@@ -158,7 +158,6 @@ public class PackageSerializer {
       if (singleAttributeValue != null) {
         attrPb.setStringValue(singleAttributeValue.toString());
       }
-      attrPb.setNodep(type == NODEP_LABEL);
     } else if (type == STRING_LIST || type == LABEL_LIST || type == NODEP_LABEL_LIST
         || type == OUTPUT_LIST || type == DISTRIBUTIONS) {
       for (Object value : values) {
@@ -166,7 +165,6 @@ public class PackageSerializer {
           attrPb.addStringListValue(entry.toString());
         }
       }
-      attrPb.setNodep(type == NODEP_LABEL_LIST);
     } else if (type == INTEGER_LIST) {
       for (Object value : values) {
         for (Integer entry : (Collection<Integer>) value) {
@@ -395,7 +393,6 @@ public class PackageSerializer {
     builder.setName(rule.getLabel().toString());
     builder.setRuleClass(rule.getRuleClass());
     builder.setParseableLocation(serializeLocation(rule.getLocation()));
-    builder.setPublicByDefault(rule.getRuleClassObject().isPublicByDefault());
     for (Attribute attribute : rule.getAttributes()) {
       PackageSerializer.addAttributeToProto(builder, attribute,
           getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()),
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
index b566ba7ba4..653e283e84 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
@@ -35,17 +35,11 @@ import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
 import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT;
 import static com.google.devtools.build.lib.packages.Type.TRISTATE;
 
-import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Discriminator;
 
-import java.util.Set;
+import java.util.Map;
 
 /**
  * Shared code used in proto buffer output for rules and rule classes.
@@ -54,13 +48,9 @@ public class ProtoUtils {
   /**
    * This map contains all attribute types that are recognized by the protocol
    * output formatter.
-   *
-   * <p>If you modify this map, please ensure that {@link #getTypeFromDiscriminator} can still
-   * resolve a {@link Discriminator} value to exactly one {@link Type} (using an optional nodep
-   * hint, as described below).
    */
-  static final ImmutableMap<Type<?>, Discriminator> TYPE_MAP =
-      new ImmutableMap.Builder<Type<?>, Discriminator>()
+  private static final Map<Type<?>, Discriminator> TYPE_MAP
+      = new ImmutableMap.Builder<Type<?>, Discriminator>()
           .put(INTEGER, Discriminator.INTEGER)
           .put(DISTRIBUTIONS, Discriminator.DISTRIBUTION_SET)
           .put(LABEL, Discriminator.LABEL)
@@ -85,71 +75,11 @@ public class ProtoUtils {
           .put(STRING_DICT_UNARY, Discriminator.STRING_DICT_UNARY)
           .build();
 
-  static final ImmutableSet<Type<?>> NODEP_TYPES =
-      ImmutableSet.of(NODEP_LABEL, NODEP_LABEL_LIST);
-
-  static final ImmutableSetMultimap<Discriminator, Type<?>> INVERSE_TYPE_MAP =
-      TYPE_MAP.asMultimap().inverse();
-
   /**
    * Returns the appropriate Attribute.Discriminator value from an internal attribute type.
    */
   public static Discriminator getDiscriminatorFromType(Type<?> type) {
-    Preconditions.checkArgument(TYPE_MAP.containsKey(type), type);
+    Preconditions.checkArgument(TYPE_MAP.containsKey(type));
     return TYPE_MAP.get(type);
   }
-
-  /**
-   * Returns the appropriate set of internal attribute types from an Attribute.Discriminator value.
-   */
-  public static ImmutableSet<Type<?>> getTypesFromDiscriminator(Discriminator discriminator) {
-    Preconditions.checkArgument(INVERSE_TYPE_MAP.containsKey(discriminator), discriminator);
-    return INVERSE_TYPE_MAP.get(discriminator);
-  }
-
-  /**
-   * Returns the appropriate internal attribute type from an Attribute.Discriminator value, given
-   * an optional nodeps hint.
-   */
-  public static Type<?> getTypeFromDiscriminator(Discriminator discriminator,
-      Optional<Boolean> nodeps, String ruleClassName, String attrName) {
-    Preconditions.checkArgument(INVERSE_TYPE_MAP.containsKey(discriminator));
-    ImmutableSet<Type<?>> possibleTypes = ProtoUtils.getTypesFromDiscriminator(discriminator);
-    Type<?> preciseType;
-    if (possibleTypes.size() == 1) {
-      preciseType = Iterables.getOnlyElement(possibleTypes);
-    } else {
-      // If there is more than one possible type associated with the discriminator, then the
-      // discriminator must be either Discriminator.STRING or Discriminator.STRING_LIST.
-      //
-      // If it is Discriminator.STRING, then its possible Type<?>s are {NODEP_LABEL, STRING}. The
-      // nodeps hint must be present in order to distinguish between them. If nodeps is true,
-      // then the Type<?> must be NODEP_LABEL, and if false, it must be STRING.
-      //
-      // A similar relation holds for the Discriminator value STRING_LIST, and its possible
-      // Type<?>s {NODEP_LABEL_LIST, STRING_LIST}.
-
-      Preconditions.checkArgument(nodeps.isPresent(),
-          "Nodeps hint is required when discriminator is associated with more than one type."
-              + " Discriminator: \"%s\", Rule class: \"%s\", Attr: \"%s\"", discriminator,
-          ruleClassName, attrName);
-      if (nodeps.get()) {
-        Set<Type<?>> nodepType = Sets.filter(possibleTypes, Predicates.in(NODEP_TYPES));
-        Preconditions.checkState(nodepType.size() == 1,
-            "There should be exactly one NODEP type associated with discriminator \"%s\""
-                + ", but found these: %s. Rule class: \"%s\", Attr: \"%s\"", discriminator,
-            nodepType, ruleClassName, attrName);
-        preciseType = Iterables.getOnlyElement(nodepType);
-      } else {
-        Set<Type<?>> notNodepType =
-            Sets.filter(possibleTypes, Predicates.not(Predicates.in(NODEP_TYPES)));
-        Preconditions.checkState(notNodepType.size() == 1,
-            "There should be exactly one non-NODEP type associated with discriminator \"%s\""
-                + ", but found these: %s. Rule class: \"%s\", Attr: \"%s\"", discriminator,
-            notNodepType, ruleClassName, attrName);
-        preciseType = Iterables.getOnlyElement(notNodepType);
-      }
-    }
-    return preciseType;
-  }
 }
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 e7cbf9edec..0286796470 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
@@ -31,7 +31,6 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Ordering;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.syntax.Argument;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.Environment;
@@ -340,29 +339,6 @@ public final class RuleClass {
                 attribute.getName(), attribute.getType());
           }
         }
-      },
-
-      /**
-       * Placeholder rules are only instantiated when packages which refer to non-native rule
-       * classes are deserialized. At this time, non-native rule classes can't be serialized. To
-       * prevent crashes on deserialization, when a package containing a rule with a non-native rule
-       * class is deserialized, the rule is assigned a placeholder rule class. This is compatible
-       * with our limited set of package serialization use cases.
-       *
-       * Placeholder rule class names obey the rule for identifiers.
-       */
-      PLACEHOLDER {
-        @Override
-        public void checkName(String name) {
-          Preconditions.checkArgument(RULE_NAME_PATTERN.matcher(name).matches(), name);
-        }
-
-        @Override
-        public void checkAttributes(Map<String, Attribute> attributes) {
-          // No required attributes; this rule class cannot have the wrong set of attributes now
-          // because, if it did, the rule class would have failed to build before the package
-          // referring to it was serialized.
-        }
       };
 
       /**
@@ -714,11 +690,6 @@ public final class RuleClass {
       }
     }
 
-    /** True if the rule class contains an attribute named {@code name}. */
-    public boolean contains(String name) {
-      return attributes.containsKey(name);
-    }
-
     /**
      * Sets the rule implementation function. Meant for Skylark usage.
      */
@@ -818,20 +789,6 @@ public final class RuleClass {
     }
   }
 
-  public static Builder createPlaceholderBuilder(final String name, final Location ruleLocation,
-      ImmutableList<RuleClass> parents) {
-    return new Builder(name, RuleClassType.PLACEHOLDER, /*skylark=*/true,
-        parents.toArray(new RuleClass[parents.size()])).factory(
-        new ConfiguredTargetFactory<Object, Object>() {
-          @Override
-          public Object create(Object ruleContext) throws InterruptedException {
-            throw new IllegalStateException(
-                "Cannot create configured targets from rule with placeholder class named \"" + name
-                    + "\" at " + ruleLocation);
-          }
-        });
-  }
-
   private final String name; // e.g. "cc_library"
 
   /**