Fix incorrect rule class digest when creating rules through macros.

In 9f2cab5, I made a change of how we are getting th digest for rule
classes. We used to get them from a `StarlarkThread` and we are getting them
from the inner Starlark function on the call stack. In a simple case, that is
correct since that inner call is the call to the `rule` function, however when
we have a rule class created thrugh a macro, it will not account for `.bzl`
files which pass values to it. Please consider an example of:

`def.bzl`:
```
def create_rule_class(function):
  return rule(implementation=function)
```

We will include the transitive digest of `def.bzl`, but not of the callers to
`create_rule_class`. This means that if `function` is defined outside of the
transitively loaded moduesl of `def.bzl`, the rule class definition environment
digest will not account for changes to `function` itself. As a result of this,
the hash is incorrect since it will produce the same value for different
implementations.

Change the digest collection to be based on the outermost Starlark function
call instead. This will take the callers into the macros into account and,
consequently, include `.bzl` file with passed `function` implementation in the
digest. Please note that for the simple case of declaring a rule, the innermost
and outermost frame is the same, hence that case has the same semanticss.

PiperOrigin-RevId: 331231462

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
@@ -80,6 +81,7 @@
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi;
+import com.google.devtools.build.lib.syntax.Debug;
 import com.google.devtools.build.lib.syntax.Dict;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Identifier;
@@ -97,6 +99,7 @@
 import java.util.Collection;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
+import javax.annotation.Nullable;
 
 /** A helper class to provide an easier API for Starlark rule definitions. */
 public class StarlarkRuleClassFunctions implements StarlarkRuleFunctionsApi<Artifact> {
@@ -346,11 +349,20 @@ public StarlarkCallable rule(
         .requiresHostConfigurationFragmentsByStarlarkBuiltinName(
             Sequence.cast(hostFragments, String.class, "host_fragments"));
     builder.setConfiguredTargetFunction(implementation);
-    // Information about the .bzl module containing the call that created the rule class.
+    // Obtain the rule definition environment (RDE) from the .bzl module being initialized by the
+    // calling thread -- the label and transitive source digest of the .bzl module of the outermost
+    // function in the call stack.
+    //
+    // If this thread is initializing a BUILD file, then the toplevel function's Module has
+    // no BazelModuleContext. Such rules cannot be instantiated, so it's ok to use a
+    // dummy label and RDE in that case (but not to crash).
     BazelModuleContext bzlModule =
-        BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
+        BazelModuleContext.of(getModuleOfOutermostStarlarkFunction(thread));
     builder.setRuleDefinitionEnvironmentLabelAndDigest(
-        bzlModule.label(), bzlModule.bzlTransitiveDigest());
+        bzlModule != null
+            ? bzlModule.label()
+            : Label.createUnvalidated(PackageIdentifier.EMPTY_PACKAGE_ID, "dummy_label"),
+        bzlModule != null ? bzlModule.bzlTransitiveDigest() : new byte[0]);
 
     builder.addRequiredToolchains(parseToolchains(toolchains, thread));
     builder.useToolchainTransition(useToolchainTransition);
@@ -405,6 +417,20 @@ public StarlarkCallable rule(
     return new StarlarkRuleFunction(builder, type, attributes, thread.getCallerLocation());
   }
 
+  /**
+   * Returns the module (file) of the outermost enclosing Starlark function on the call stack or
+   * null if none of the active calls are functions defined in Starlark.
+   */
+  @Nullable
+  private static Module getModuleOfOutermostStarlarkFunction(StarlarkThread thread) {
+    for (Debug.Frame fr : Debug.getCallStack(thread)) {
+      if (fr.getFunction() instanceof StarlarkFunction) {
+        return ((StarlarkFunction) fr.getFunction()).getModule();
+      }
+    }
+    return null;
+  }
+
   private static void checkAttributeName(String name) throws EvalException {
     if (!Identifier.isValid(name)) {
       throw Starlark.errorf("attribute name `%s` is not a valid identifier.", name);

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.packages.AdvertisedProviderSet;
 import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -1846,4 +1847,124 @@ public void testCreateExecGroup() throws Exception {
     assertThat(group.execCompatibleWith())
         .containsExactly(makeLabel("//constraint:cv1"), makeLabel("//constraint:cv2"));
   }
+
+  @Test
+  public void ruleDefinitionEnvironmentDigest_unaffectedByTargetAttrValueChange() throws Exception {
+    scratch.file(
+        "r/def.bzl",
+        "def _r(ctx): return struct(value=ctx.attr.text)",
+        "r = rule(implementation=_r, attrs={'text': attr.string()})");
+    scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r', text='old')");
+    byte[] oldDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    scratch.deleteFile("r/BUILD");
+    scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r', text='new')");
+    // Signal SkyFrame to discover changed files.
+    skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
+    byte[] newDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    assertThat(newDigest).isEqualTo(oldDigest);
+  }
+
+  @Test
+  public void ruleDefinitionEnvironmentDigest_accountsForFunctionWhenCreatingRuleWithAMacro()
+      throws Exception {
+    scratch.file("r/create.bzl", "def create(impl): return rule(implementation=impl)");
+    scratch.file(
+        "r/def.bzl",
+        "load(':create.bzl', 'create')",
+        "def f(ctx): return struct(value='OLD')",
+        "r = create(f)");
+    scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r')");
+    byte[] oldDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    scratch.deleteFile("r/def.bzl");
+    scratch.file(
+        "r/def.bzl",
+        "load(':create.bzl', 'create')",
+        "def f(ctx): return struct(value='NEW')",
+        "r = create(f)");
+    // Signal SkyFrame to discover changed files.
+    skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
+    byte[] newDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    assertThat(newDigest).isNotEqualTo(oldDigest);
+  }
+
+  @Test
+  public void ruleDefinitionEnvironmentDigest_accountsForAttrsWhenCreatingRuleWithMacro()
+      throws Exception {
+    scratch.file(
+        "r/create.bzl",
+        "def f(ctx): return struct(value=ctx.attr.to_json())",
+        "def create(attrs): return rule(implementation=f, attrs=attrs)");
+    scratch.file("r/def.bzl", "load(':create.bzl', 'create')", "r = create({})");
+    scratch.file("r/BUILD", "load(':def.bzl', 'r')", "r(name='r')");
+    byte[] oldDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    scratch.deleteFile("r/def.bzl");
+    scratch.file(
+        "r/def.bzl",
+        "load(':create.bzl', 'create')",
+        "r = create({'value': attr.string(default='')})");
+    // Signal SkyFrame to discover changed files.
+    skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE);
+    byte[] newDigest =
+        createRuleContext("//r:r")
+            .getRuleContext()
+            .getRule()
+            .getRuleClassObject()
+            .getRuleDefinitionEnvironmentDigest();
+
+    assertThat(newDigest).isNotEqualTo(oldDigest);
+  }
+
+  /**
+   * This test is crucial for correctness of {@link RuleClass#getRuleDefinitionEnvironmentDigest}
+   * since we use a dummy bzl transitive digest in that case. It is correct to do that only because
+   * a rule class created by a BUILD thread cannot be instantiated.
+   */
+  @Test
+  public void ruleClassDefinedInBuildFile_fails() throws Exception {
+    reporter.removeHandler(failFastHandler);
+    reporter.addHandler(ev.getEventCollector());
+    scratch.file("r/create.bzl", "def create(impl): return rule(implementation=impl)");
+    scratch.file("r/def.bzl", "load(':create.bzl', 'create')", "r = create({})");
+    scratch.file("r/impl.bzl", "def make_struct(ctx): return struct(value='hello')");
+    scratch.file(
+        "r/BUILD",
+        "load(':create.bzl', 'create')",
+        "load(':impl.bzl', 'make_struct')",
+        "r = create(make_struct)",
+        "r(name='r')");
+
+    getConfiguredTarget("//r:r");
+
+    ev.assertContainsError("Error in rule: Invalid rule class hasn't been exported by a bzl file");
+  }
 }