Ensure that the rule class synthesized by a call to `testing.analysis…

…_test` has a "definition environment digest" that differs by target.

We were previously incorrectly using the same digest for all calls. See the added code comment for the reasoning behind our new approach. tl;dr - We're still incorrect overall but we're now less incorrect. I added a TODO to fully fix this flaw.

PiperOrigin-RevId: 576938053
Change-Id: Ifd65c90c9bfd2e716ed1fcc1260b4eb17bc645a2

src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
 import com.google.devtools.build.lib.starlarkbuildapi.test.TestingModuleApi;
+import com.google.devtools.build.lib.util.Fingerprint;
 import java.util.regex.Pattern;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
@@ -90,13 +91,31 @@ public void analysisTest(
 
     LabelConverter labelConverter = LabelConverter.forBzlEvaluatingThread(thread);
 
-    // TODO(b/291752414): The digest of the rule class is incorrect. It is usually based on the
-    // transitive digest of the .bzl being initialized. But since we're in a BUILD-evaluating
-    // thread, we should use the transitive digest of the BUILD file. This is not currently
-    // computed, but could be added to PackageFunction. In the meantime, we use a dummy empty digest
-    // for all analysis_test-generated rule classes.
+    // Each call to analysis_test defines a rule class (the code right below this comment here) and
+    // then instantiates a *single* target of that rule class (the code at the end of this method).
+    //
+    // For normal Starlark-defined rule classes we're supposed to pass in the label of the bzl file
+    // being initialized at the time the rule class is defined, as well as the transitive digest of
+    // that bzl and all bzls it loads (for purposes of being sensitive to e.g. changes to the rule
+    // class's implementation function). For analysis_test this is currently infeasible because
+    // there is no such bzl file (since we're in a BUILD-evaluating thread) and we don't currently
+    // track the transitive digest of BUILD files and the bzls they load.
+    //
+    // In acknowledge of this infeasibility, we used to use a constant digest for all calls to
+    // analysis_test. This caused issues due to how the digest is used as part of the cache key of
+    // deserialized rule classes. To address that, we now use the combo of the package name and the
+    // target name (this works since we don't currently try to deserialize the same rule class
+    // produced at different source versions). See http://b/291752414#comment6.
+    //
+    // The digest is also used for purposes to detecting changes to a rule class across source
+    // versions; see blaze_query.Rule.skylark_environment_hash_code. So we're still incorrect there.
+    // See http://b/291752414#comment9 and http://b/291752414#comment10.
+    // TODO(b/291752414): Fix.
     Label dummyBzlFile = Label.createUnvalidated(PackageIdentifier.EMPTY_PACKAGE_ID, "dummy_label");
-    byte[] dummyDigest = new byte[0];
+    Fingerprint fingerprint = new Fingerprint();
+    fingerprint.addString(pkgContext.getLabel().getPackageName());
+    fingerprint.addString(name);
+    byte[] transitiveDigestToUse = fingerprint.digestAndReset();
 
     StarlarkRuleFunction starlarkRuleFunction =
         StarlarkRuleClassFunctions.createRule(
@@ -105,7 +124,7 @@ public void analysisTest(
             thread.getCallerLocation(),
             callStack,
             dummyBzlFile,
-            dummyDigest,
+            transitiveDigestToUse,
             labelConverter,
             thread.getSemantics(),
             // rule() parameters.

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

@@ -3947,7 +3947,7 @@ public void testAnalysisTestAttrs() throws Exception {
 
   /** Tests two analysis_test calls with same name. */
   @Test
-  public void testAnalysisTestDuplicateName() throws Exception {
+  public void testAnalysisTestDuplicateName_samePackage() throws Exception {
     scratch.file(
         "p/a.bzl",
         "def impl(ctx): ",
@@ -3982,6 +3982,53 @@ public void testAnalysisTestDuplicateName() throws Exception {
             + " with existing my_test_target_test rule");
   }
 
+  // Regression test for b/291752414 (Digest for Starlark-defined rules is wrong for analysis_test).
+  @Test
+  public void testAnalysisTestDuplicateName_differentAttrs_differentPackage() throws Exception {
+    scratch.file("p/BUILD");
+    scratch.file(
+        "p/make.bzl",
+        "def impl(ctx): ",
+        "  return  [AnalysisTestResultInfo(",
+        "    success = True,",
+        "    message = ''",
+        "  )]",
+        "def make(name, additional_string_attr_name):",
+        "  testing.analysis_test(",
+        "    name = name, ",
+        "    implementation = impl,",
+        "    attrs = {additional_string_attr_name: attr.string()},",
+        "    attr_values = {additional_string_attr_name: 'whatever'}",
+        "  )");
+    scratch.file(
+        "p1/BUILD", //
+        "load('//p:make.bzl','make')",
+        "make(name = 'my_test_target', additional_string_attr_name = 'p1')");
+    scratch.file(
+        "p2/BUILD", //
+        "load('//p:make.bzl','make')",
+        "make(name = 'my_test_target', additional_string_attr_name = 'p2')");
+    scratch.file(
+        "s/BUILD", //
+        "test_suite(name = 'suite', tests = ['//p1:my_test_target', '//p2:my_test_target'])");
+
+    // Confirm we can [transitively] analyze both targets together without errors.
+    getConfiguredTarget("//s:suite");
+
+    // Also confirm the definition environment digests differ for the rule classes synthesized under
+    // the hood for these two targets.
+    Rule p1Target =
+        (Rule)
+            getPackageManager()
+                .getTarget(ev.getEventHandler(), Label.parseCanonical("//p1:my_test_target"));
+    Rule p2Target =
+        (Rule)
+            getPackageManager()
+                .getTarget(ev.getEventHandler(), Label.parseCanonical("//p2:my_test_target"));
+    assertThat(p1Target.getRuleClassObject().getRuleDefinitionEnvironmentDigest())
+        .isNotEqualTo(p2Target.getRuleClassObject().getRuleDefinitionEnvironmentDigest());
+  }
+
   /**
    * Tests analysis_test call with a name that is not Starlark identifier (but still a good target
    * name).