bazel packages: prep for AttributeContainer optimization

Details:
- RuleFactory:
  - don't plumb AttributeContainer through create{RuleUnchecked,AndAddRule}.
  - avoid overloading of createAndAddRule.
  - inline and simplify setRuleAttributeValue
    (only one of two callers wants 'visibility' logic)
  - inline and simplify getAttributeNoncomputedDefaultValue
    (check both name and type like we do for licenses)
- AttributeContainer:
  - inline setAttributeValueByName into sole use (for $implicit_test).
  - add TODO comments.
  - make isAttributeValueExplicitlySpecified private
PiperOrigin-RevId: 318841494

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -48,7 +48,6 @@
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
-import com.google.devtools.build.lib.packages.AttributeContainer;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.AttributeValueSource;
 import com.google.devtools.build.lib.packages.BazelModuleContext;
@@ -679,12 +678,7 @@ public Object call(StarlarkThread thread, Tuple<Object> args, Dict<String, Objec
                   + "Rules may be instantiated only in a BUILD thread.");
         }
         RuleFactory.createAndAddRule(
-            pkgContext,
-            ruleClass,
-            attributeValues,
-            thread.getSemantics(),
-            thread.getCallStack(),
-            new AttributeContainer(ruleClass));
+            pkgContext, ruleClass, attributeValues, thread.getSemantics(), thread.getCallStack());
       } catch (InvalidRuleException | NameConflictException e) {
         throw new EvalException(null, e.getMessage());
       }

src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java

@@ -27,23 +27,27 @@
  * be a robust public interface, but rather just an input to {@link AttributeMap} instances. Use
  * those instances for all domain-level attribute access.
  */
+// TODO(adonovan): eliminate this class. 99% of all external calls to its methods are of the form
+// rule.getAttributeContainer().foo(), so it's just needless indirection for the caller, and
+// needless indirection in the representation. Instead, make Rule implement an interface with the
+// two methods getAttr and isAttributeValueExplicitlySpecified; it already has those methods.
+// The only time the AttributeContainer needs to be distinct from the Rule itself is in the
+// WorkspaceFactory.setParent hack. Perhaps we can eliminate that?
 public final class AttributeContainer {
 
   private final RuleClass ruleClass;
 
-  // Attribute values, keyed by attribute index:
+  // Attribute values, keyed by attribute index.
   private final Object[] attributeValues;
 
-  // Holds a list of attribute indices.
+  // Holds a list of attribute indices (+1, to make them nonzero).
   // The first byte gives the length of the list.
   // The list records which attributes were set explicitly in the BUILD file.
   // The list may be padded with zeros at the end.
   private byte[] state;
 
-  /**
-   * Create a container for a rule of the given rule class.
-   */
-  public AttributeContainer(RuleClass ruleClass) {
+  /** Creates a container for a rule of the given rule class. */
+  AttributeContainer(RuleClass ruleClass) {
     int n = ruleClass.getAttributeCount();
     if (n > 254) {
       // We reserve the zero byte as a hole/sentinel inside state[].
@@ -66,10 +70,8 @@ public Object getAttr(String attrName) {
     return idx != null ? attributeValues[idx] : null;
   }
 
-  /**
-   * See {@link #isAttributeValueExplicitlySpecified(String)}.
-   */
-  public boolean isAttributeValueExplicitlySpecified(Attribute attribute) {
+  /** See {@link #isAttributeValueExplicitlySpecified(String)}. */
+  boolean isAttributeValueExplicitlySpecified(Attribute attribute) {
     return isAttributeValueExplicitlySpecified(attribute.getName());
   }
 
@@ -144,12 +146,4 @@ void setAttributeValue(Attribute attribute, Object value, boolean explicit) {
       setExplicit(index);
     }
   }
-
-  // This sets the attribute "explicitly" as if it came from the BUILD file.
-  // At present, the sole use of this is for the test_suite.$implicit_tests
-  // attribute, which is synthesized during package loading.  We do want to
-  // consider that "explicitly set" so that it appears in query output.
-  void setAttributeValueByName(String attrName, Object value) {
-    setAttributeValue(ruleClass.getAttributeByName(attrName), value, true);
-  }
 }

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -1232,15 +1232,15 @@ void setDefaultRestrictedTo(List<Label> environments, String attrName, Location
      * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
      * associated with this {@link Builder}.
      *
-     * <p>The created {@link Rule} will have no attribute values, no output files, and therefore
-     * will be in an invalid state.
+     * <p>The created {@link Rule} will have no output files and therefore will be in an invalid
+     * state.
      */
     Rule createRule(
         Label label,
         RuleClass ruleClass,
         Location location,
         List<StarlarkThread.CallStackEntry> callstack,
-        AttributeContainer attributeContainer) {
+        AttributeContainer attributeContainer) { // required by WorkspaceFactory.setParent hack
       return new Rule(
           pkg, label, ruleClass, location, callStackBuilder.of(callstack), attributeContainer);
     }
@@ -1255,15 +1255,14 @@ Rule createRule(
         RuleClass ruleClass,
         Location location,
         List<StarlarkThread.CallStackEntry> callstack,
-        AttributeContainer attributeContainer,
         ImplicitOutputsFunction implicitOutputsFunction) {
       return new Rule(
           pkg,
           label,
           ruleClass,
           location,
           callStackBuilder.of(callstack),
-          attributeContainer,
+          new AttributeContainer(ruleClass),
           implicitOutputsFunction);
     }
 
@@ -1514,7 +1513,8 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
         }
 
         // "test_suite" rules have the idiosyncratic semantics of implicitly
-        // depending on all tests in the package, iff tests=[] and suites=[].
+        // depending on all tests in the package, iff tests=[] and suites=[],
+        // which is about 20% of >1M test_suite instances in Google's corpus.
         // Note, we implement this here when the Package is fully constructed,
         // since clearly this information isn't available at Rule construction
         // time, as forward references are permitted.
@@ -1536,7 +1536,13 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
       if (!implicitTestSuiteRuleInstances.isEmpty()) {
         Collections.sort(labelsOfTestTargets);
         for (Rule rule : implicitTestSuiteRuleInstances) {
-          rule.setAttributeValueByName("$implicit_tests", labelsOfTestTargets);
+          // Pretend the test_suite.$implicit_tests attribute
+          // (which is synthesized during package loading)
+          // is explicitly set so that it appears in query output.
+          rule.setAttributeValue(
+              rule.getRuleClassObject().getAttributeByName("$implicit_tests"),
+              labelsOfTestTargets,
+              /*explicit=*/ true);
         }
       }
       return this;

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -386,8 +386,7 @@ public NoneType call(StarlarkThread thread, Tuple<Object> args, Dict<String, Obj
             ruleClass,
             new BuildLangTypedAttributeValuesMap(kwargs),
             thread.getSemantics(),
-            thread.getCallStack(),
-            new AttributeContainer(ruleClass));
+            thread.getCallStack());
       } catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {
         throw new EvalException(null, e.getMessage());
       }

src/main/java/com/google/devtools/build/lib/packages/Rule.java

@@ -127,10 +127,6 @@ void setAttributeValue(Attribute attribute, Object value, boolean explicit) {
     attributes.setAttributeValue(attribute, value, explicit);
   }
 
-  void setAttributeValueByName(String attrName, Object value) {
-    attributes.setAttributeValueByName(attrName, value);
-  }
-
   void setContainsErrors() {
     this.containsErrors = true;
   }