Remove Package.AbstractBuilder.

This simplifies the code a little, and prepares for properly
serializing ExternalPackage.

--
MOS_MIGRATED_REVID=91673213

7 files changed, 72 insertions, 93 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackage.java b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackage.java
index 0ca70c0cb9..68d098bbb0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackage.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackage.java
@@ -101,7 +101,7 @@ public class ExternalPackage extends Package {
    * Given a workspace file path, creates an ExternalPackage.
    */
   public static class Builder
-      extends AbstractBuilder<ExternalPackage, Builder> {
+      extends Package.Builder {
     private Map<Label, Binding> bindMap = Maps.newLinkedHashMap();
     private Map<RepositoryName, Rule> repositoryMap = Maps.newLinkedHashMap();
 
@@ -111,9 +111,8 @@ public class ExternalPackage extends Package {
       setMakeEnv(new MakeEnvironment.Builder());
     }
 
-    @Override
-    protected Builder self() {
-      return this;
+    protected ExternalPackage externalPackage() {
+      return (ExternalPackage) pkg;
     }
 
     @Override
@@ -126,9 +125,11 @@ public class ExternalPackage extends Package {
               + ", which can't happen: " + e.getMessage());
         }
       }
-      pkg.bindMap = ImmutableMap.copyOf(bindMap);
-      pkg.repositoryMap = ImmutableMap.copyOf(repositoryMap);
-      return super.build();
+      externalPackage().bindMap = ImmutableMap.copyOf(bindMap);
+      externalPackage().repositoryMap = ImmutableMap.copyOf(repositoryMap);
+
+      Package base = super.build();
+      return (ExternalPackage) base;
     }
 
     /**
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 48b3e8966c..4565dc6dc7 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
@@ -327,7 +327,7 @@ public class Package implements Serializable {
    * <p>Only after this method is called can this package be considered "complete"
    * and be shared publicly.
    */
-  protected void finishInit(AbstractBuilder<?, ?> builder) {
+  protected void finishInit(Builder builder) {
     // If any error occurred during evaluation of this package, consider all
     // rules in the package to be "in error" also (even if they were evaluated
     // prior to the error).  This behaviour is arguably stricter than need be,
@@ -500,7 +500,7 @@ public class Package implements Serializable {
 
   /**
    * Common getTargets implementation, accessible by both {@link Package} and
-   * {@link Package.AbstractBuilder}.
+   * {@link Package.Builder}.
    */
   private static Collection<Target> getTargets(Map<String, Target> targetMap) {
     return Collections.unmodifiableCollection(targetMap.values());
@@ -516,7 +516,7 @@ public class Package implements Serializable {
 
   /**
    * Common getTargets implementation, accessible by both {@link Package} and
-   * {@link Package.AbstractBuilder}.
+   * {@link Package.Builder}.
    */
   private static <T extends Target> Iterable<T> getTargets(Map<String, Target> targetMap,
       Class<T> targetClass) {
@@ -725,42 +725,19 @@ public class Package implements Serializable {
         out.println();
       }
     }
-
-    // TODO(bazel-team): (2009) perhaps dump also:
-    // - subincludes
-    // - globs
-    // - containsErrors
-    // - makeEnv
   }
 
   /**
-   * Builder class for {@link Package}.
+   * Builder class for {@link Package} that does its own globbing.
    *
-   * <p>Should only be used by the package loading and the package deserialization machineries.
+   * <p>Despite its name, this is the normal builder used when parsing BUILD files.
    */
-  static class Builder extends AbstractBuilder<Package, Builder> {
-    Builder(PackageIdentifier packageId) {
-      super(new Package(packageId));
-    }
-
-    @Override
-    protected Builder self() {
-      return this;
-    }
-  }
-
-  /** Builder class for {@link Package} that does its own globbing. */
-  public static class LegacyBuilder extends AbstractBuilder<Package, LegacyBuilder> {
+  public static class LegacyBuilder extends Builder {
 
     private Globber globber = null;
 
     LegacyBuilder(PackageIdentifier packageId) {
-      super(AbstractBuilder.newPackage(packageId));
-    }
-
-    @Override
-    protected LegacyBuilder self() {
-      return this;
+      super(packageId);
     }
 
     /**
@@ -791,14 +768,14 @@ public class Package implements Serializable {
     }
   }
 
-  abstract static class AbstractBuilder<P extends Package, B extends AbstractBuilder<P, B>> {
+  static class Builder {
     /**
      * The output instance for this builder. Needs to be instantiated and
      * available with name info throughout initialization. All other settings
      * are applied during {@link #build}. See {@link Package#Package(String)}
      * and {@link Package#finishInit} for details.
      */
-    protected P pkg;
+    protected Package pkg;
 
     protected Path filename = null;
     private Label buildFileLabel = null;
@@ -846,7 +823,7 @@ public class Package implements Serializable {
       }
     };
 
-    protected AbstractBuilder(P pkg) {
+    protected Builder(Package pkg) {
       this.pkg = pkg;
       if (pkg.getName().startsWith("javatests/")) {
         setDefaultTestonly(true);
@@ -856,8 +833,7 @@ public class Package implements Serializable {
     protected static Package newPackage(PackageIdentifier packageId) {
       return new Package(packageId);
     }
-
-    protected abstract B self();
+    Builder(PackageIdentifier id) { this(newPackage(id)); }
 
     protected PackageIdentifier getPackageIdentifier() {
       return pkg.getPackageIdentifier();
@@ -866,7 +842,7 @@ public class Package implements Serializable {
     /**
      * Sets the name of this package's BUILD file.
      */
-    B setFilename(Path filename) {
+    Builder setFilename(Path filename) {
       this.filename = filename;
       try {
         buildFileLabel = createLabel(filename.getBaseName());
@@ -875,7 +851,7 @@ public class Package implements Serializable {
         // This can't actually happen.
         throw new AssertionError("Package BUILD file has an illegal name: " + filename);
       }
-      return self();
+      return this;
     }
 
     public Label getBuildFileLabel() {
@@ -889,9 +865,9 @@ public class Package implements Serializable {
     /**
      * Sets this package's Make environment.
      */
-    B setMakeEnv(MakeEnvironment.Builder makeEnv) {
+    Builder setMakeEnv(MakeEnvironment.Builder makeEnv) {
       this.makeEnv = makeEnv;
-      return self();
+      return this;
     }
 
     MakeEnvironment.Builder getMakeEnvironment() {
@@ -902,40 +878,40 @@ public class Package implements Serializable {
      * Sets the default visibility for this package. Called at most once per
      * package from PackageFactory.
      */
-    B setDefaultVisibility(RuleVisibility visibility) {
+    Builder setDefaultVisibility(RuleVisibility visibility) {
       this.defaultVisibility = visibility;
       this.defaultVisibilitySet = true;
-      return self();
+      return this;
     }
 
     /**
      * Sets whether the default visibility is set in the BUILD file.
      */
-    B setDefaultVisibilitySet(boolean defaultVisibilitySet) {
+    Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) {
       this.defaultVisibilitySet = defaultVisibilitySet;
-      return self();
+      return this;
     }
 
     /** Sets the default value of 'testonly'. Rule-level 'testonly' will override this. */
-    B setDefaultTestonly(boolean defaultTestonly) {
+    Builder setDefaultTestonly(boolean defaultTestonly) {
       pkg.setDefaultTestOnly(defaultTestonly);
-      return self();
+      return this;
     }
 
     /**
      * Sets the default value of 'deprecation'. Rule-level 'deprecation' will append to this.
      */
-    B setDefaultDeprecation(String defaultDeprecation) {
+    Builder setDefaultDeprecation(String defaultDeprecation) {
       pkg.setDefaultDeprecation(defaultDeprecation);
-      return self();
+      return this;
     }
 
     /**
      * Uses the workspace name from {@code //external} to set this package's workspace name.
      */
-    B setWorkspaceName(String workspaceName) {
+    Builder setWorkspaceName(String workspaceName) {
       pkg.workspaceName = workspaceName;
-      return self();
+      return this;
     }
 
     /**
@@ -952,60 +928,60 @@ public class Package implements Serializable {
     /**
      * Sets the default header checking mode.
      */
-    public B setDefaultHdrsCheck(String hdrsCheck) {
+    public Builder setDefaultHdrsCheck(String hdrsCheck) {
       // Note that this setting is propagated directly to the package because
       // other code needs the ability to read this info directly from the
       // under-construction package. See {@link Package#setDefaultHdrsCheck}.
       pkg.setDefaultHdrsCheck(hdrsCheck);
-      return self();
+      return this;
     }
 
     /**
      * Sets the default value of copts. Rule-level copts will append to this.
      */
-    public B setDefaultCopts(List<String> defaultCopts) {
+    public Builder setDefaultCopts(List<String> defaultCopts) {
       this.defaultCopts = defaultCopts;
-      return self();
+      return this;
     }
 
-    public B addFeatures(Iterable<String> features) {
+    public Builder addFeatures(Iterable<String> features) {
       Iterables.addAll(this.features, features);
-      return self();
+      return this;
     }
 
     /**
      * Declares that errors were encountering while loading this package.
      */
-    public B setContainsErrors() {
+    public Builder setContainsErrors() {
       containsErrors = true;
-      return self();
+      return this;
     }
 
     public boolean containsErrors() {
       return containsErrors;
     }
 
-    B setContainsTemporaryErrors() {
+    Builder setContainsTemporaryErrors() {
       setContainsErrors();
       containsTemporaryErrors = true;
-      return self();
+      return this;
     }
 
-    public B addEvents(Iterable<Event> events) {
+    public Builder addEvents(Iterable<Event> events) {
       for (Event event : events) {
         addEvent(event);
       }
-      return self();
+      return this;
     }
 
-    public B addEvent(Event event) {
+    public Builder addEvent(Event event) {
       this.events.add(event);
-      return self();
+      return this;
     }
 
-    B setSkylarkFileDependencies(ImmutableList<Label> skylarkFileDependencies) {
+    Builder setSkylarkFileDependencies(ImmutableList<Label> skylarkFileDependencies) {
       this.skylarkFileDependencies = skylarkFileDependencies;
-      return self();
+      return this;
     }
 
     /**
@@ -1277,7 +1253,7 @@ public class Package implements Serializable {
       }
     }
 
-    private B beforeBuild() {
+    private Builder beforeBuild() {
       Preconditions.checkNotNull(pkg);
       Preconditions.checkNotNull(filename);
       Preconditions.checkNotNull(buildFileLabel);
@@ -1325,19 +1301,19 @@ public class Package implements Serializable {
           rule.setAttributeValueByName("$implicit_tests", allTests);
         }
       }
-      return self();
+      return this;
     }
 
     /** Intended to be used only by {@link PackageFunction}. */
-    public B buildPartial() {
+    public Builder buildPartial() {
       if (alreadyBuilt) {
-        return self();
+        return this;
       }
       return beforeBuild();
     }
 
     /** Intended to be used only by {@link PackageFunction}. */
-    public P finishBuild() {
+    public Package finishBuild() {
       if (alreadyBuilt) {
         return pkg;
       }
@@ -1361,7 +1337,7 @@ public class Package implements Serializable {
       return pkg;
     }
 
-    public P build() {
+    public Package build() {
       if (alreadyBuilt) {
         return pkg;
       }
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 960432db85..3c082a57b0 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
@@ -24,7 +24,7 @@ import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.License.LicenseParsingException;
-import com.google.devtools.build.lib.packages.Package.AbstractBuilder.GeneratedLabelConflict;
+import com.google.devtools.build.lib.packages.Package.Builder.GeneratedLabelConflict;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.RuleClass.ParsedAttributeValue;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index fb0282f224..aa9b8112d7 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1170,8 +1170,9 @@ public final class PackageFactory {
     Environment pkgEnv = new Environment(globalEnv, eventHandler);
 
     Package.LegacyBuilder pkgBuilder =
-        new Package.LegacyBuilder(packageId)
-        .setGlobber(globber)
+        new Package.LegacyBuilder(packageId);
+
+    pkgBuilder.setGlobber(globber)
         .setFilename(buildFilePath)
         .setMakeEnv(pkgMakeEnv)
         .setDefaultVisibility(defaultVisibility)
@@ -1239,8 +1240,9 @@ public final class PackageFactory {
     Environment pkgEnv = new Environment();
 
     Package.LegacyBuilder pkgBuilder =
-        new Package.LegacyBuilder(packageId)
-        .setFilename(buildFilePath)
+        new Package.LegacyBuilder(packageId);
+
+    pkgBuilder.setFilename(buildFilePath)
         .setMakeEnv(pkgMakeEnv)
         .setDefaultVisibility(defaultVisibility)
         // "defaultVisibility" comes from the command line. Let's give the BUILD file a chance to
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 c3035413ef..75101f25d4 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
@@ -484,7 +484,7 @@ public final class Rule implements Target {
    * will retain the relative order in which they were declared.
    */
   void populateOutputFiles(EventHandler eventHandler,
-      Package.AbstractBuilder<?, ?> pkgBuilder) throws SyntaxException {
+      Package.Builder pkgBuilder) throws SyntaxException {
     Preconditions.checkState(outputFiles == null);
     // Order is important here: implicit before explicit
     outputFiles = Lists.newArrayList();
@@ -520,7 +520,7 @@ public final class Rule implements Target {
    * of the rule's "name", "srcs", and other attributes.
    */
   private void populateImplicitOutputFiles(EventHandler eventHandler,
-      Package.AbstractBuilder<?, ?> pkgBuilder) {
+      Package.Builder pkgBuilder) {
     try {
       for (String out : ruleClass.getImplicitOutputsFunction().getImplicitOutputs(attributeMap)) {
         try {
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 c06b06b009..b4273c5d4e 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
@@ -1103,7 +1103,7 @@ public final class RuleClass {
   /**
    * Helper function for {@link RuleFactory#createAndAddRule}.
    */
-  Rule createRuleWithLabel(Package.AbstractBuilder<?, ?> pkgBuilder, Label ruleLabel,
+  Rule createRuleWithLabel(Package.Builder pkgBuilder, Label ruleLabel,
       Map<String, Object> attributeValues, EventHandler eventHandler, FuncallExpression ast,
       Location location) throws SyntaxException {
     Rule rule = pkgBuilder.newRuleWithLabel(ruleLabel, this, null, location);
@@ -1111,7 +1111,7 @@ public final class RuleClass {
     return rule;
   }
 
-  private void createRuleCommon(Rule rule, Package.AbstractBuilder<?, ?> pkgBuilder,
+  private void createRuleCommon(Rule rule, Package.Builder pkgBuilder,
       Map<String, Object> attributeValues, EventHandler eventHandler, FuncallExpression ast)
           throws SyntaxException {
     populateRuleAttributeValues(
@@ -1153,7 +1153,7 @@ public final class RuleClass {
    */
   @SuppressWarnings("unchecked")
   Rule createRuleWithParsedAttributeValues(Label label,
-      Package.AbstractBuilder<?, ?> pkgBuilder, Location ruleLocation,
+      Package.Builder pkgBuilder, Location ruleLocation,
       Map<String, ParsedAttributeValue> attributeValues, EventHandler eventHandler)
           throws SyntaxException{
     Rule rule = pkgBuilder.newRuleWithLabel(label, this, null, ruleLocation);
@@ -1191,7 +1191,7 @@ public final class RuleClass {
    * location information with each rule attribute.
    */
   private void populateRuleAttributeValues(Rule rule,
-                                           Package.AbstractBuilder<?, ?> pkgBuilder,
+                                           Package.Builder pkgBuilder,
                                            Map<String, Object> attributeValues,
                                            EventHandler eventHandler,
                                            FuncallExpression ast) {
@@ -1319,7 +1319,7 @@ public final class RuleClass {
    * but does not have a declared license.
    */
   private static void checkThirdPartyRuleHasLicense(Rule rule,
-      Package.AbstractBuilder<?, ?> pkgBuilder, EventHandler eventHandler) {
+      Package.Builder pkgBuilder, EventHandler eventHandler) {
     if (rule.getLabel().getPackageName().startsWith("third_party/")) {
       License license = rule.getLicense();
       if (license == null) {
@@ -1391,7 +1391,7 @@ public final class RuleClass {
    * evaluated in second pass.)
    */
   private static Object getAttributeNoncomputedDefaultValue(Attribute attr,
-      Package.AbstractBuilder<?, ?> pkgBuilder) {
+      Package.Builder pkgBuilder) {
     if (attr.getName().equals("licenses")) {
       return pkgBuilder.getDefaultLicense();
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 24e7be3a55..d5d1a46e8d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -69,7 +69,7 @@ public class RuleFactory {
    * <p>It is the caller's responsibility to add the rule to the package (the caller may choose not
    * to do so if, for example, the rule has errors).</p>
    */
-  static Rule createRule(Package.AbstractBuilder<?, ?> pkgBuilder, RuleClass ruleClass,
+  static Rule createRule(Package.Builder pkgBuilder, RuleClass ruleClass,
       Map<String, Object> attributeValues, EventHandler eventHandler, FuncallExpression ast,
       Location location)
       throws InvalidRuleException, NameConflictException {
@@ -124,7 +124,7 @@ public class RuleFactory {
    *         reason (e.g. no <code>name</code> attribute is defined)
    * @throws NameConflictException
    */
-  static Rule createAndAddRule(Package.AbstractBuilder<?, ?> pkgBuilder,
+  static Rule createAndAddRule(Package.Builder pkgBuilder,
                   RuleClass ruleClass,
                   Map<String, Object> attributeValues,
                   EventHandler eventHandler,