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=97679963

7 files changed, 165 insertions, 35 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 8afcbb7d55..1a52b5ed20 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(null, null).deserialize(in);
+      deserializedPkg = new PackageDeserializer().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.
    */
-  Label getBuildFileLabel() {
+  public 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 e4bc250333..d5a612efa5 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,7 +34,6 @@ 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;
 
@@ -55,11 +54,23 @@ public class PackageDeserializer {
 
   private static final Logger LOG = Logger.getLogger(PackageDeserializer.class.getName());
 
-  // Workaround for Java serialization not allowing to pass in a context manually.
+  /**
+   * 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.
   // volatile is needed to ensure that the objects are published safely.
-  // TODO(bazel-team): Subclass ObjectOutputStream to pass through environment variables.
-  public static volatile RuleClassProvider defaultRuleClassProvider;
-  public static volatile FileSystem defaultDeserializerFileSystem;
+  // TODO(bazel-team): Subclass ObjectOutputStream to pass this through instead.
+  public static volatile PackageDeserializationEnvironment defaultPackageDeserializationEnvironment;
 
   private class Context {
     private final Package.Builder packageBuilder;
@@ -120,15 +131,9 @@ public class PackageDeserializer {
       }
     }
 
-    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));
-      }
-
+    void deserializeRule(Build.Rule rulePb) throws PackageDeserializationException {
+      Location ruleLocation = deserializeLocation(rulePb.getParseableLocation());
+      RuleClass ruleClass = packageDeserializationEnvironment.getRuleClass(rulePb, ruleLocation);
       Map<String, ParsedAttributeValue> attributeValues = new HashMap<>();
       for (Build.Attribute attrPb : rulePb.getAttributeList()) {
         Type<?> type = ruleClass.getAttributeByName(attrPb.getName()).getType();
@@ -136,7 +141,6 @@ public class PackageDeserializer {
       }
 
       Label ruleLabel = deserializeLabel(rulePb.getName());
-      Location ruleLocation = deserializeLocation(rulePb.getParseableLocation());
       try {
         Rule rule = ruleClass.createRuleWithParsedAttributeValues(
             ruleLabel, packageBuilder, ruleLocation, attributeValues,
@@ -150,8 +154,7 @@ public class PackageDeserializer {
     }
   }
 
-  private final FileSystem fileSystem;
-  private final RuleClassProvider ruleClassProvider;
+  private final PackageDeserializationEnvironment packageDeserializationEnvironment;
 
   @Immutable
   private static final class ExplicitLocation extends Location {
@@ -196,15 +199,16 @@ public class PackageDeserializer {
     }
   }
 
-  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);
+  /**
+   * Creates a {@link PackageDeserializer} using {@link #defaultPackageDeserializationEnvironment}.
+   */
+  public PackageDeserializer() {
+    this.packageDeserializationEnvironment = defaultPackageDeserializationEnvironment;
+  }
+
+  public PackageDeserializer(PackageDeserializationEnvironment packageDeserializationEnvironment) {
+    this.packageDeserializationEnvironment =
+        Preconditions.checkNotNull(packageDeserializationEnvironment);
   }
 
   /**
@@ -310,7 +314,7 @@ public class PackageDeserializer {
    */
   private void deserializeInternal(Build.Package packagePb, StoredEventHandler eventHandler,
       Package.Builder builder, InputStream in) throws PackageDeserializationException, IOException {
-    Path buildFile = fileSystem.getPath(packagePb.getBuildFilePath());
+    Path buildFile = packageDeserializationEnvironment.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 16b318cb62..4f1a7a3008 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,6 +158,7 @@ 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) {
@@ -165,6 +166,7 @@ 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) {
@@ -393,6 +395,7 @@ 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 653e283e84..b566ba7ba4 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,11 +35,17 @@ 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.Map;
+import java.util.Set;
 
 /**
  * Shared code used in proto buffer output for rules and rule classes.
@@ -48,9 +54,13 @@ 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).
    */
-  private static final Map<Type<?>, Discriminator> TYPE_MAP
-      = new ImmutableMap.Builder<Type<?>, Discriminator>()
+  static final ImmutableMap<Type<?>, Discriminator> TYPE_MAP =
+      new ImmutableMap.Builder<Type<?>, Discriminator>()
           .put(INTEGER, Discriminator.INTEGER)
           .put(DISTRIBUTIONS, Discriminator.DISTRIBUTION_SET)
           .put(LABEL, Discriminator.LABEL)
@@ -75,11 +85,71 @@ 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));
+    Preconditions.checkArgument(TYPE_MAP.containsKey(type), 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 0286796470..e7cbf9edec 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,6 +31,7 @@ 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;
@@ -339,6 +340,29 @@ 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.
+        }
       };
 
       /**
@@ -690,6 +714,11 @@ 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.
      */
@@ -789,6 +818,20 @@ 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"
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index 924bd04c9c..55c65ee4e3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -118,13 +118,15 @@ public class SkylarkRuleClassFunctions {
   });
 
   // TODO(bazel-team): Remove the code duplication (BaseRuleClasses and this class).
-  private static final RuleClass baseRule =
+  /** Parent rule class for non-test Skylark rules. */
+  public static final RuleClass baseRule =
       BaseRuleClasses.commonCoreAndSkylarkAttributes(
           new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true))
           .add(attr("expect_failure", STRING))
           .build();
 
-  private static final RuleClass testBaseRule =
+  /** Parent rule class for test Skylark rules. */
+  public static final RuleClass testBaseRule =
       new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
           .add(attr("size", STRING).value("medium").taggable()
               .nonconfigurable("used in loading phase rule validation logic"))
diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto
index 6780d4d844..9fb59b5c6d 100644
--- a/src/main/protobuf/build.proto
+++ b/src/main/protobuf/build.proto
@@ -136,6 +136,11 @@ message Attribute {
   // Whether the attribute was explicitly specified
   optional bool explicitly_specified = 13;
 
+  // If this attribute has a string value or a string list value, then this
+  // may be set to indicate that the value may be treated as a label that
+  // isn't a dependency of this attribute's rule.
+  optional bool nodep = 20;
+
   // The type of attribute.  This message is used for all of the different
   // attribute types so the discriminator helps for figuring out what is
   // stored in the message.
@@ -231,6 +236,9 @@ message Rule {
 
   // The location of the target in the BUILD file in a machine-parseable form.
   optional Location parseable_location = 8;
+
+  // The rule's class's public by default value.
+  optional bool public_by_default = 9;
 }
 
 // Summary of all transitive dependencies of 'rule,' where each dependent