Prohibit calls to native.environment() in symbolic macros

environment_group() is a special non-rule callable which can only be
invoked at top level in a BUILD file (there is no
native.environment_group()) and which creates a special target that refers
to environment rule targets via unresolved labels.

This means that when we allow lazy macro expansion, if an environment
target is declared in the scope of a macro, during constraint enforcement
the environment group might or might not find the environment target
depending on whether or not the symbolic macro happened to be expanded.

By far the simplest solution is to prohibit calls to native.environment()
in symbolic macros.

Working towards bazelbuild#23852

PiperOrigin-RevId: 682141401
Change-Id: Ic32e58ade1db01e1de53e747c43544a4327dbc5e

src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java

@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.NoConfigTransition;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
 import com.google.devtools.build.lib.packages.Types;
 import com.google.devtools.build.lib.util.FileTypeSet;
@@ -72,6 +73,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
   public Metadata getMetadata() {
     return RuleDefinition.Metadata.builder()
         .name(ConstraintConstants.ENVIRONMENT_RULE)
+        // Not allowed in symbolic macros: lazy expansion of symbolic macros could hide environment
+        // targets from environment groups.
+        .type(RuleClassType.BUILD_ONLY)
         .ancestors(BaseRuleClasses.NativeBuildRule.class)
         .factoryClass(Environment.class)
         .build();

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

@@ -362,9 +362,10 @@ public void checkAttributes(Map<String, Attribute> attributes) {
       },
 
       /**
-       * Normal rules are instantiable by BUILD files. Their names must therefore obey the rules for
-       * identifiers in the BUILD language. In addition, {@link TargetUtils#isTestRuleName} must
-       * return false for the name.
+       * Normal rules are instantiable by BUILD files, possibly via a macro (symbolic or legacy), in
+       * which case the rule's symbol is namespaced under {@code native}. Normal rule names must
+       * therefore obey the rules for identifiers in the BUILD language. In addition, {@link
+       * TargetUtils#isTestRuleName} must return false for the name.
        */
       NORMAL {
         @Override
@@ -393,6 +394,22 @@ public void checkAttributes(Map<String, Attribute> attributes) {
         }
       },
 
+      /**
+       * Normal rules with the additional restriction that they can only be instantiated by BUILD
+       * files or legacy macros - but not symbolic macros.
+       */
+      BUILD_ONLY {
+        @Override
+        public void checkName(String name) {
+          NORMAL.checkName(name);
+        }
+
+        @Override
+        public void checkAttributes(Map<String, Attribute> attributes) {
+          NORMAL.checkAttributes(attributes);
+        }
+      },
+
       /**
        * Workspace rules can only be instantiated from a WORKSPACE file. Their names obey the rule
        * for identifiers.

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

@@ -323,7 +323,8 @@ public static ImmutableMap<String, BuiltinRuleFunction> buildRuleFunctions(
     for (String ruleClassName : ruleClassMap.keySet()) {
       RuleClass cl = ruleClassMap.get(ruleClassName);
       if (cl.getRuleClassType() == RuleClassType.NORMAL
-          || cl.getRuleClassType() == RuleClassType.TEST) {
+          || cl.getRuleClassType() == RuleClassType.TEST
+          || cl.getRuleClassType() == RuleClassType.BUILD_ONLY) {
         result.put(ruleClassName, new BuiltinRuleFunction(cl));
       }
     }
@@ -347,7 +348,10 @@ public NoneType call(StarlarkThread thread, Tuple args, Dict<String, Object> kwa
         throw Starlark.errorf("unexpected positional arguments");
       }
       try {
-        Package.Builder pkgBuilder = Package.Builder.fromOrFail(thread, "rules");
+        Package.Builder pkgBuilder =
+            ruleClass.getRuleClassType() != RuleClassType.BUILD_ONLY
+                ? Package.Builder.fromOrFail(thread, "rules")
+                : Package.Builder.fromOrFailAllowBuildOnly(thread, ruleClass.getName() + " rule");
         RuleFactory.createAndAddRule(
             pkgBuilder,
             ruleClass,

src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java

@@ -685,6 +685,11 @@ public void macroCannotCallExistingRules() throws Exception {
     doCannotCallApiTest("existing_rules()", "native.existing_rules()");
   }
 
+  @Test
+  public void macroCannotCallEnvironmentRuleFunction() throws Exception {
+    doCannotCallApiTest("environment rule", "native.environment(name = 'foo')");
+  }
+
   // There are other symbols that must not be called from within symbolic macros, but we don't test
   // them because they can't be obtained from a symbolic macro implementation anyway, since they are
   // not under `native` (at least, for BUILD-loaded .bzl files) and because symbolic macros can't