From 5c82594a0c337f68b46ada68fe0d5d52eeb88fc8 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 3 Oct 2024 20:17:18 -0700 Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/issues/23852 PiperOrigin-RevId: 682141401 Change-Id: Ic32e58ade1db01e1de53e747c43544a4327dbc5e --- .../analysis/constraints/EnvironmentRule.java | 4 ++++ .../build/lib/packages/RuleClass.java | 23 ++++++++++++++++--- .../build/lib/packages/RuleFactory.java | 8 +++++-- .../build/lib/analysis/SymbolicMacroTest.java | 5 ++++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java index 2d51c4f9729caf..1aef0c16a8c938 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java +++ b/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(); 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 89174c2edb457c..2079659ba97ca7 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 @@ -362,9 +362,10 @@ public void checkAttributes(Map 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 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 attributes) { + NORMAL.checkAttributes(attributes); + } + }, + /** * Workspace rules can only be instantiated from a WORKSPACE file. Their names obey the rule * for identifiers. 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 07de534cc2aaba..3aa6c4addb4945 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 @@ -323,7 +323,8 @@ public static ImmutableMap 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 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, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java index 18dc31877aefa3..2ce82243dc566b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java +++ b/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