Remove special handling of name attribute. Fixes #278

The name attribute gets special treatment in the codebase, in that
it's not simply yet another attribute but stored in it's own field.

Thus, every callside dealing with attributes needs to be aware of
this special case and explicitly handle the name attribute. It's
easy to see that this can lead to bugs. For example, querying for
the name attribute is currently broken due the querying code not
being aware of the special case [1].

Discussions with experienced bazel developers came to the conclusion
that there is no need (anymore) to treat the name attribute specially
and thus we decided it's best to remove the special treatment and
handle the name attribute as any other attribute.

This change removes the handling of name attributes and also adds a test
case to verify that bug [1] is fixed.

[1] https://github.com/bazelbuild/bazel/issues/278

--
PiperOrigin-RevId: 147446345
MOS_MIGRATED_REVID=147446345

7 files changed, 44 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index ebffbf8e42..8739918377 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -151,6 +151,7 @@ public class BaseRuleClasses {
       return RuleDefinition.Metadata.builder()
           .name("$test_base_rule")
           .type(RuleClassType.ABSTRACT)
+          .ancestors(RootRule.class)
           .build();
     }
   }
@@ -160,8 +161,6 @@ public class BaseRuleClasses {
    */
   public static RuleClass.Builder commonCoreAndSkylarkAttributes(RuleClass.Builder builder) {
     return builder
-        .add(attr("name", STRING)
-            .nonconfigurable("Rule name"))
         // The visibility attribute is special: it is a nodep label, and loading the
         // necessary package groups is handled by {@link LabelVisitor#visitTargetVisibility}.
         // Package groups always have the null configuration so that they are not duplicated
@@ -198,15 +197,38 @@ public class BaseRuleClasses {
         );
   }
 
+  public static RuleClass.Builder nameAttribute(RuleClass.Builder builder) {
+    return builder.add(attr("name", STRING).nonconfigurable("Rule name"));
+  }
+
   /**
-   * Common parts of rules.
+   * Ancestor of every rule.
+   *
+   * <p>Adds the name attribute to every rule.
+   */
+  public static final class RootRule implements RuleDefinition {
+
+    @Override
+    public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
+        return nameAttribute(builder).build();
+    }
+
+    @Override
+    public Metadata getMetadata() {
+      return RuleDefinition.Metadata.builder()
+          .name("$root_rule")
+          .type(RuleClassType.ABSTRACT)
+          .build();
+    }
+  }
+
+  /**
+   * Common parts of some rules.
    */
   public static final class BaseRule implements RuleDefinition {
     @Override
     public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
       return commonCoreAndSkylarkAttributes(builder)
-          // The name attribute is handled specially, so it does not appear here.
-          //
           // Aggregates the labels of all {@link ConfigRuleClasses} rules this rule uses (e.g.
           // keys for configurable attributes). This is specially populated in
           // {@RuleClass#populateRuleAttributeValues}.
@@ -233,12 +255,13 @@ public class BaseRuleClasses {
       return RuleDefinition.Metadata.builder()
           .name("$base_rule")
           .type(RuleClassType.ABSTRACT)
+          .ancestors(RootRule.class)
           .build();
     }
   }
 
   /**
-   * Common ancestor class for all rules.
+   * Common ancestor class for some rules.
    */
   public static final class RuleBase implements RuleDefinition {
     @Override
@@ -283,6 +306,7 @@ public class BaseRuleClasses {
       return RuleDefinition.Metadata.builder()
           .name("$binary_base_rule")
           .type(RuleClassType.ABSTRACT)
+          .ancestors(RootRule.class)
           .build();
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index c325ba81bb..339bdc5ec5 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -124,6 +124,7 @@ public class SkylarkRepositoryModule {
           builder.addOrOverrideAttribute(attr("$local", BOOLEAN).defaultValue(local).build());
           builder.addOrOverrideAttribute(attr("$environ", STRING_LIST)
               .defaultValue(environ).build());
+          BaseRuleClasses.nameAttribute(builder);
           BaseRuleClasses.commonCoreAndSkylarkAttributes(builder);
           builder.add(attr("expect_failure", STRING));
           if (attrs != Runtime.NONE) {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
index 18cc9fa2a3..90b963bb79 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -320,6 +320,7 @@ public class BazelRuleClassProvider {
       new RuleSet() {
         @Override
         public void init(Builder builder) {
+          builder.addRuleDefinition(new BaseRuleClasses.RootRule());
           builder.addRuleDefinition(new BaseRuleClasses.BaseRule());
           builder.addRuleDefinition(new BaseRuleClasses.RuleBase());
           builder.addRuleDefinition(new BaseRuleClasses.BinaryBaseRule());
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 5bd4d83f60..1b3ea9c061 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
@@ -1421,11 +1421,6 @@ public final class RuleClass {
       Rule rule, AttributeValuesMap attributeValues, EventHandler eventHandler) {
     BitSet definedAttrIndices = new BitSet();
     for (String attributeName : attributeValues.getAttributeNames()) {
-      // The attribute named "name" was handled in a special way already.
-      if (attributeName.equals("name")) {
-        continue;
-      }
-
       Object attributeValue = attributeValues.getAttributeValue(attributeName);
       // Ignore all None values.
       if (attributeValue == Runtime.NONE) {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
index 7b053163cc..81f2dd1183 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
@@ -418,6 +418,12 @@ public abstract class OutputFormatter implements Serializable {
 
           RawAttributeMapper attributeMap = RawAttributeMapper.of(rule);
           for (Attribute attr : rule.getAttributes()) {
+            // Ignore the "name" attribute here, as we already print it above.
+            // This is not strictly necessary, but convention has it that the
+            // name attribute is printed first.
+            if ("name".equals(attr.getName())) {
+              continue;
+            }
             if (attributeMap.isConfigurable(attr.getName(), attr.getType())) {
               continue; // TODO(bazel-team): handle configurable attributes.
             }
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 36648c4701..3cc9decfe6 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
@@ -122,9 +122,10 @@ public class SkylarkRuleClassFunctions {
   /** Parent rule class for non-executable 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();
+          BaseRuleClasses.nameAttribute(
+            new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true))
+            .add(attr("expect_failure", STRING)))
+            .build();
 
   /** Parent rule class for executable non-test Skylark rules. */
   public static final RuleClass binaryBaseRule =
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceBaseRule.java
index 581559ca39..2130d1a67a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceBaseRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceBaseRule.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.rules.repository;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.syntax.Type.STRING;
 
+import com.google.devtools.build.lib.analysis.BaseRuleClasses.RootRule;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -42,6 +43,7 @@ public class WorkspaceBaseRule implements RuleDefinition {
     return RuleDefinition.Metadata.builder()
         .name("$workspace_base_rule")
         .type(RuleClassType.ABSTRACT)
+        .ancestors(RootRule.class)
         .build();
   }
 }