diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 0473798b8bcc39..624362c0669a72 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -583,6 +583,14 @@ public static final class AllCommandGraveyardOptions extends OptionsBase { effectTags = {OptionEffectTag.NO_OP}, help = "No-op.") public boolean announceProfilePath; + + @Option( + name = "incompatible_existing_rules_immutable_view", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + help = "No-op.") + public boolean incompatibleExistingRulesImmutableView; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index 17127f199e823f..5d8f06458ae6b0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -435,15 +435,8 @@ public Object existingRule(String name, StarlarkThread thread) throws EvalExcept Package.Builder pkgBuilder = Package.Builder.fromOrFailDisallowSymbolicMacros(thread, "existing_rule()"); Target target = pkgBuilder.getTarget(name); - if (target instanceof Rule /* `instanceof` also verifies that target != null */) { - Rule rule = (Rule) target; - if (thread - .getSemantics() - .getBool(BuildLanguageOptions.INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW)) { - return new ExistingRuleView(rule); - } else { - return getRuleDict(rule, thread.mutability()); - } + if (target instanceof Rule rule) { + return new ExistingRuleView(rule); } else { return Starlark.NONE; } @@ -507,21 +500,7 @@ public Object existingRules(StarlarkThread thread) throws EvalException { } Package.Builder pkgBuilder = Package.Builder.fromOrFailDisallowSymbolicMacros(thread, "existing_rules()"); - if (thread - .getSemantics() - .getBool(BuildLanguageOptions.INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW)) { - return new ExistingRulesView(pkgBuilder.getRulesSnapshotView()); - } else { - Collection targets = pkgBuilder.getTargets(); - Mutability mu = thread.mutability(); - Dict.Builder> rules = Dict.builder(); - for (Target t : targets) { - if (t instanceof Rule) { - rules.put(t.getName(), getRuleDict((Rule) t, mu)); - } - } - return rules.build(mu); - } + return new ExistingRulesView(pkgBuilder.getRulesSnapshotView()); } @Override @@ -658,26 +637,6 @@ public String moduleVersion(StarlarkThread thread) throws EvalException { return pkgBuilder.getAssociatedModuleVersion().orElse(null); } - private static Dict getRuleDict(Rule rule, Mutability mu) throws EvalException { - Dict.Builder values = Dict.builder(); - - for (Attribute attr : rule.getAttributes()) { - if (!isPotentiallyExportableAttribute(rule.getRuleClassObject(), attr.getName())) { - continue; - } - - Object val = starlarkifyValue(mu, rule.getAttr(attr.getName()), rule.getPackage()); - if (val == null) { - continue; - } - values.put(attr.getName(), val); - } - - values.put("name", rule.getName()); - values.put("kind", rule.getRuleClass()); - return values.build(mu); - } - /** * Returns true if the given attribute of a rule class is generally allowed to be exposed via * {@code native.existing_rule()} and {@code native.existing_rules()}. diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index b8a3f878e5a699..54c27c8ae368dd 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -227,17 +227,6 @@ public final class BuildLanguageOptions extends OptionsBase { + " function.") public boolean experimentalIsolatedExtensionUsages; - @Option( - name = "incompatible_existing_rules_immutable_view", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, - effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS, OptionEffectTag.LOADING_AND_ANALYSIS}, - metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - help = - "If set to true, native.existing_rule and native.existing_rules return lightweight" - + " immutable view objects instead of mutable dicts.") - public boolean incompatibleExistingRulesImmutableView; - @Option( name = "incompatible_stop_exporting_build_file_path", defaultValue = "false", @@ -762,8 +751,6 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(ENABLE_BZLMOD, enableBzlmod) .setBool(ENABLE_WORKSPACE, enableWorkspace) .setBool(EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, experimentalIsolatedExtensionUsages) - .setBool( - INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW, incompatibleExistingRulesImmutableView) .setBool(EXPERIMENTAL_ACTION_RESOURCE_SET, experimentalActionResourceSet) .setBool(EXPERIMENTAL_GOOGLE_LEGACY_API, experimentalGoogleLegacyApi) .setBool(EXPERIMENTAL_PLATFORMS_API, experimentalPlatformsApi) @@ -873,8 +860,6 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String ENABLE_WORKSPACE = "+enable_workspace"; public static final String EXPERIMENTAL_ISOLATED_EXTENSION_USAGES = "-experimental_isolated_extension_usages"; - public static final String INCOMPATIBLE_EXISTING_RULES_IMMUTABLE_VIEW = - "+incompatible_existing_rules_immutable_view"; public static final String EXPERIMENTAL_GOOGLE_LEGACY_API = "-experimental_google_legacy_api"; public static final String EXPERIMENTAL_PLATFORMS_API = "-experimental_platforms_api"; public static final String EXPERIMENTAL_REPO_REMOTE_EXEC = "-experimental_repo_remote_exec"; diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java index 8244b4bf063187..8ed05f17d2755a 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java @@ -106,8 +106,6 @@ Sequence glob( + " x supporting dict-like iteration, len(x), name in" + " x, x[name], x.get(name), x.items()," + " x.keys(), and x.values()." // - + "

If the --noincompatible_existing_rules_immutable_view flag is set," - + " instead returns a new mutable dict with the same content." // + "

The result contains an entry for each attribute, with the exception of private" + " ones (whose names do not start with a letter) and a few unrepresentable legacy" + " attribute types. In addition, the dict contains entries for the rule instance's" @@ -145,12 +143,8 @@ Sequence glob( + " x supporting dict-like iteration, len(x), name in" + " x, x[name], x.get(name), x.items()," + " x.keys(), and x.values()." // - + "

If the --noincompatible_existing_rules_immutable_view flag is set," - + " instead returns a new mutable dict with the same content." // + "

Note: If possible, avoid using this function. It makes BUILD files brittle" - + " and order-dependent. Furthermore, if the" - + " --noincompatible_existing_rules_immutable_view flag is set," - + " this function may be very expensive, especially if called within a loop.", + + " and order-dependent.", useStarlarkThread = true) Object existingRules(StarlarkThread thread) throws EvalException; diff --git a/src/test/java/com/google/devtools/build/lib/packages/NativeExistingRulesTest.java b/src/test/java/com/google/devtools/build/lib/packages/NativeExistingRulesTest.java index f2ea0428fb7705..7d52622ff37b3c 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/NativeExistingRulesTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/NativeExistingRulesTest.java @@ -30,34 +30,15 @@ import net.starlark.java.eval.StarlarkList; import net.starlark.java.eval.StarlarkValue; import net.starlark.java.eval.Tuple; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for {@code native.existing_rule} and {@code native.existing_rules} functions. - * - *

This class covers the legacy behavior where the {@code - * --incompatible_existing_rules_immutable_view} flag is disabled. The enabled case is covered by - * the subclass, {@link WithImmutableView}. - */ +/** Tests for {@code native.existing_rule} and {@code native.existing_rules} functions. */ @RunWith(JUnit4.class) public class NativeExistingRulesTest extends BuildViewTestCase { private TestStarlarkBuiltin testStarlarkBuiltin; // initialized by createRuleClassProvider() - // Intended to be overridden by this test case's subclasses. Note that overriding of JUnit's - // @Before methods is not recommended. - protected void setupOptions() throws Exception { - // --noincompatible_existing_rules_immutable_view is the default; set it explicitly for clarity. - setBuildLanguageOptions("--noincompatible_existing_rules_immutable_view"); - } - - @Before - public final void setUp() throws Exception { - setupOptions(); - } - @StarlarkBuiltin(name = "test") private static final class TestStarlarkBuiltin implements StarlarkValue { @@ -408,7 +389,7 @@ def f(): } @Test - public void existingRule_returnsObjectWithCorrectMutability() throws Exception { + public void existingRule_returnsImmutableObject() throws Exception { scratch.file( "test/BUILD", """ @@ -422,11 +403,12 @@ public void existingRule_returnsObjectWithCorrectMutability() throws Exception { def f(): native.config_setting(name = "x", define_values = {"key": "value"}) r = native.existing_rule("x") - r["no_such_attribute"] = "foo" - r["define_values"]["key"] = 123 - """); // mutate the dict + r["no_such_attribute"] = 123 + """); // mutate the view - assertThat(getConfiguredTarget("//test:BUILD")).isNotNull(); // no error on mutation + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//test:BUILD")).isNull(); // mutation fails + assertContainsEvent("can only assign an element in a dictionary or a list"); } @Test @@ -664,7 +646,7 @@ def save(name, object): } @Test - public void existingRules_returnsObjectWithCorrectMutability() throws Exception { + public void existingRules_returnsImmutableObject() throws Exception { scratch.file( "test/BUILD", """ @@ -681,7 +663,32 @@ def f(): rs["no_such_rule"] = {"name": "no_such_rule", "kind": "config_setting"} """); // mutate - assertThat(getConfiguredTarget("//test:BUILD")).isNotNull(); // no error on mutation + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//test:BUILD")).isNull(); // mutation fails + assertContainsEvent("can only assign an element in a dictionary or a list"); + } + + @Test + public void existingRules_returnsDeeplyImmutableView() throws Exception { + scratch.file( + "test/BUILD", + """ + load("inc.bzl", "f") + + f() + """); + scratch.file( + "test/inc.bzl", + """ + def f(): + native.config_setting(name = "x", define_values = {"key": "value"}) + rs = native.existing_rules() + rs["x"]["define_values"]["key"] = 123 + """); // mutate an attribute value within the view + + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//test:BUILD")).isNull(); + assertContainsEvent("trying to mutate a frozen dict value"); } @Test @@ -835,88 +842,4 @@ def save(name, object): .containsAtLeast( "name", "bar", "kind", "test_library", "srcs", StarlarkList.immutableOf(":bar.cc")); } - - /** - * Tests for {@code native.existing_rule} and {@code native.existing_rules} Starlark functions - * with the {@code --incompatible_existing_rules_immutable_view} flag set. - */ - @RunWith(JUnit4.class) - public static final class WithImmutableView extends NativeExistingRulesTest { - - @Override - protected void setupOptions() throws Exception { - setBuildLanguageOptions("--incompatible_existing_rules_immutable_view"); - } - - @Test - @Override - public void existingRule_returnsObjectWithCorrectMutability() throws Exception { - scratch.file( - "test/BUILD", - """ - load("inc.bzl", "f") - - f() - """); - scratch.file( - "test/inc.bzl", - """ - def f(): - native.config_setting(name = "x", define_values = {"key": "value"}) - r = native.existing_rule("x") - r["no_such_attribute"] = 123 - """); // mutate the view - - reporter.removeHandler(failFastHandler); - assertThat(getConfiguredTarget("//test:BUILD")).isNull(); // mutation fails - assertContainsEvent("can only assign an element in a dictionary or a list"); - } - - @Test - @Override - public void existingRules_returnsObjectWithCorrectMutability() throws Exception { - scratch.file( - "test/BUILD", - """ - load("inc.bzl", "f") - - f() - """); - scratch.file( - "test/inc.bzl", - """ - def f(): - native.config_setting(name = "x", define_values = {"key": "value"}) - rs = native.existing_rules() - rs["no_such_rule"] = {"name": "no_such_rule", "kind": "config_setting"} - """); // mutate - - reporter.removeHandler(failFastHandler); - assertThat(getConfiguredTarget("//test:BUILD")).isNull(); // mutation fails - assertContainsEvent("can only assign an element in a dictionary or a list"); - } - - @Test - public void existingRules_returnsDeeplyImmutableView() throws Exception { - scratch.file( - "test/BUILD", - """ - load("inc.bzl", "f") - - f() - """); - scratch.file( - "test/inc.bzl", - """ - def f(): - native.config_setting(name = "x", define_values = {"key": "value"}) - rs = native.existing_rules() - rs["x"]["define_values"]["key"] = 123 - """); // mutate an attribute value within the view - - reporter.removeHandler(failFastHandler); - assertThat(getConfiguredTarget("//test:BUILD")).isNull(); - assertContainsEvent("trying to mutate a frozen dict value"); - } - } }