Move --incompatible_existing_rules_immutable_view option to the grave…

…yard.

It has been enabled by default since Bazel 6.0 (#14717).

Removing the flag simplifies native.existing_rules() behavior considerably,
making it easier to add new finalizer macro semantics. Working towards
#23160

PiperOrigin-RevId: 663963764
Change-Id: I82308093cbf2f4befb53cd4ea19dfc75e14e19af

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

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<Target> targets = pkgBuilder.getTargets();
-      Mutability mu = thread.mutability();
-      Dict.Builder<String, Dict<String, Object>> 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<String, Object> getRuleDict(Rule rule, Mutability mu) throws EvalException {
-    Dict.Builder<String, Object> 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()}.

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";

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java

@@ -106,8 +106,6 @@ Sequence<?> glob(
               + " <code>x</code> supporting dict-like iteration, <code>len(x)</code>, <code>name in"
               + " x</code>, <code>x[name]</code>, <code>x.get(name)</code>, <code>x.items()</code>,"
               + " <code>x.keys()</code>, and <code>x.values()</code>." //
-              + "<p>If the <code>--noincompatible_existing_rules_immutable_view</code> flag is set,"
-              + " instead returns a new mutable dict with the same content." //
               + "<p>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(
               + " <code>x</code> supporting dict-like iteration, <code>len(x)</code>, <code>name in"
               + " x</code>, <code>x[name]</code>, <code>x.get(name)</code>, <code>x.items()</code>,"
               + " <code>x.keys()</code>, and <code>x.values()</code>." //
-              + "<p>If the <code>--noincompatible_existing_rules_immutable_view</code> flag is set,"
-              + " instead returns a new mutable dict with the same content." //
               + "<p><em>Note: If possible, avoid using this function. It makes BUILD files brittle"
-              + " and order-dependent. Furthermore, if the"
-              + " </em><code>--noincompatible_existing_rules_immutable_view</code><em> flag is set,"
-              + " this function may be very expensive, especially if called within a loop.</em>",
+              + " and order-dependent.",
       useStarlarkThread = true)
   Object existingRules(StarlarkThread thread) throws EvalException;
 

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.
- *
- * <p>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");
-    }
-  }
 }