Fix #441 under-invalidation: hash PACKAGE_GROUP targets and follow visibility edges

cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt

@@ -611,10 +611,35 @@ class BazelQueryService(
       Build.Target.Discriminator.RULE -> BazelTarget.Rule(target)
       Build.Target.Discriminator.SOURCE_FILE -> BazelTarget.SourceFile(target)
       Build.Target.Discriminator.GENERATED_FILE -> BazelTarget.GeneratedFile(target)
+      Build.Target.Discriminator.PACKAGE_GROUP -> packageGroupToTarget(target)
       else -> {
         logger.w { "Unsupported target type in the build graph: ${target.type.name}" }
         null
       }
     }
   }
+
+  /**
+   * Lowers a PACKAGE_GROUP query target into a synthetic RULE target so it flows through the
+   * regular rule-hashing pipeline instead of being dropped (issue #441): the group's `packages`
+   * allow-list becomes a hashed attribute, and its `includes` become rule_inputs so an edit to a
+   * nested package_group propagates to every group that includes it. [RuleHasher] separately
+   * follows rules' `visibility` attributes to these targets, which is what carries a group edit to
+   * the rules the group gates -- and from there, transitively, to their dependents.
+   */
+  private fun packageGroupToTarget(target: Build.Target): BazelTarget.Rule {
+    val packageGroup = target.packageGroup
+    val rule =
+        Build.Rule.newBuilder()
+            .setName(packageGroup.name)
+            .setRuleClass("package_group")
+            .addAttribute(
+                Build.Attribute.newBuilder()
+                    .setName("packages")
+                    .setType(Build.Attribute.Discriminator.STRING_LIST)
+                    .addAllStringListValue(packageGroup.containedPackageList))
+            .addAllRuleInput(packageGroup.includedPackageGroupList)
+    return BazelTarget.Rule(
+        Build.Target.newBuilder().setType(Build.Target.Discriminator.RULE).setRule(rule).build())
+  }
 }

cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt

@@ -89,6 +89,37 @@ class BazelRule(private val rule: Build.Rule) {
 
   val name: String = rule.name
 
+  /**
+   * True for `package_group` targets. `bazel query` reports them under their own PACKAGE_GROUP
+   * discriminator; [BazelQueryService] lowers them to synthetic rules with this rule class so they
+   * participate in hashing (issue #441).
+   */
+  val isPackageGroup: Boolean
+    get() = rule.ruleClass == "package_group"
+
+  /**
+   * The package_group labels referenced by this rule's `visibility` attribute. A visibility value
+   * is either a package_group label or one of the special forms -- `//visibility:public`,
+   * `//visibility:private`, or a package spec (`//foo:__pkg__` / `//foo:__subpackages__`) -- which
+   * name no target of their own and are filtered out here.
+   *
+   * Visibility labels are "nodep" labels: `bazel query` never lists them in `rule_input`, so
+   * [ruleInputList] cannot surface them and [com.bazel_diff.hash.RuleHasher] follows them
+   * explicitly instead (issue #441).
+   */
+  fun visibilityPackageGroupLabels(): List<String> =
+      rule.attributeList
+          .firstOrNull { it.name == "visibility" }
+          ?.stringListValueList
+          .orEmpty()
+          .filter {
+            !it.startsWith("//visibility:") &&
+                !it.endsWith(":__pkg__") &&
+                !it.endsWith(":__subpackages__")
+          }
+          .distinct()
+          .sorted()
+
   /**
    * Macro instantiation frames (`"<path>:<line>:<col>: <fn>"`) naming the BUILD/`.bzl` files that
    * produced this rule. Empty unless the query ran with `--proto:instantiation_stack`.

cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt

@@ -167,6 +167,35 @@ class RuleHasher(
               }
             }
           }
+
+          // `visibility` labels are "nodep" labels: `bazel query` does not list them in
+          // `rule_input`, so a referenced package_group's *contents* would otherwise never reach
+          // this rule's digest -- editing a group's `packages` list can flip a dependent of this
+          // rule from building to failing visibility analysis while every hash stays unchanged
+          // (issue #441). Follow package_group visibility entries as dep edges; the special
+          // `//visibility:*` forms and `__pkg__`/`__subpackages__` package specs carry no target
+          // and are already filtered out by [BazelRule.visibilityPackageGroupLabels]. A label
+          // whose group is outside the queried graph is skipped, matching how it was (silently)
+          // invisible before package_group support.
+          if ("visibility" !in ignoredAttrs) {
+            for (packageGroupLabel in rule.visibilityPackageGroupLabels()) {
+              val packageGroup = allRulesMap[packageGroupLabel] ?: continue
+              if (!packageGroup.isPackageGroup || packageGroup.name == rule.name) continue
+              putDirectBytes(packageGroupLabel.toByteArray())
+              val packageGroupHash =
+                  digest(
+                      packageGroup,
+                      allRulesMap,
+                      ruleHashes,
+                      sourceDigests,
+                      seedHash,
+                      packageBzlSeeds,
+                      depPathClone,
+                      ignoredAttrs,
+                      modifiedFilepaths)
+              putTransitiveBytes(packageGroupLabel, packageGroupHash.overallDigest)
+            }
+          }
         }
 
     return finalHashValue.also { ruleHashes[rule.name] = it }

cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt

@@ -76,6 +76,49 @@ class BazelRuleTest {
     assertThat(BazelRule(rulePb).tags()).isEqualTo(emptySet<String>())
   }
 
+  // Backs package_group support (issue #441): only real package_group labels survive the
+  // filter -- the special `//visibility:*` forms and `__pkg__`/`__subpackages__` package specs
+  // name no target of their own.
+  @Test
+  fun testVisibilityPackageGroupLabelsFiltersSpecialForms() {
+    val rulePb =
+        Rule.newBuilder()
+            .setRuleClass("genrule")
+            .setName("//lib:thing")
+            .addAttribute(
+                Attribute.newBuilder()
+                    .setType(Attribute.Discriminator.STRING_LIST)
+                    .setName("visibility")
+                    .addStringListValue("//visibility:public")
+                    .addStringListValue("//other:__pkg__")
+                    .addStringListValue("//other:__subpackages__")
+                    .addStringListValue("//lib:zgroup")
+                    .addStringListValue("//lib:consumers")
+                    .addStringListValue("//lib:consumers")
+                    .build())
+            .build()
+
+    // Deduped and sorted for a deterministic digest contribution.
+    assertThat(BazelRule(rulePb).visibilityPackageGroupLabels())
+        .isEqualTo(listOf("//lib:consumers", "//lib:zgroup"))
+  }
+
+  @Test
+  fun testVisibilityPackageGroupLabelsEmptyWhenAttributeAbsent() {
+    val rulePb = Rule.newBuilder().setRuleClass("java_library").setName("//pkg:lib").build()
+
+    assertThat(BazelRule(rulePb).visibilityPackageGroupLabels()).isEqualTo(emptyList<String>())
+  }
+
+  @Test
+  fun testIsPackageGroup() {
+    val packageGroupPb = Rule.newBuilder().setRuleClass("package_group").setName("//lib:pg").build()
+    val rulePb = Rule.newBuilder().setRuleClass("java_library").setName("//pkg:lib").build()
+
+    assertThat(BazelRule(packageGroupPb).isPackageGroup).isEqualTo(true)
+    assertThat(BazelRule(rulePb).isPackageGroup).isEqualTo(false)
+  }
+
   // Fix for https://github.com/Tinder/bazel-diff/issues/359
   //
   // Under --useCquery, BazelRule.ruleInputList() now folds each ConfiguredRuleInput's

cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt

@@ -1460,17 +1460,16 @@ class E2ETest {
   }
 
   @Test
-  fun testPackageGroupChangeIsNotImpacted_reproducerForIssue441() {
-    // Reproducer for https://github.com/Tinder/bazel-diff/issues/441
+  fun testPackageGroupChangeImpactsConsumers_regressionForIssue441() {
+    // Regression test for the under-invalidation (false-negative) half of
+    // https://github.com/Tinder/bazel-diff/issues/441.
     //
-    // The under-invalidation (false-negative) half of the issue. bazel-diff only
-    // keeps targets whose query `Discriminator` is RULE, SOURCE_FILE, or
-    // GENERATED_FILE; every other type is dropped
-    // (BazelQueryService.toBazelTarget -> logs "Unsupported target type" and
-    // returns null). `PACKAGE_GROUP` is one such dropped type, so a change to a
-    // package_group's `packages` list is never reflected in any hash -- even
-    // though it genuinely alters downstream visibility and can flip a consumer
-    // from building to failing.
+    // bazel-diff used to keep only targets whose query `Discriminator` is RULE,
+    // SOURCE_FILE, or GENERATED_FILE; `PACKAGE_GROUP` was dropped
+    // (BazelQueryService.toBazelTarget logged "Unsupported target type" and
+    // returned null), so a change to a package_group's `packages` list was never
+    // reflected in any hash -- even though it genuinely alters downstream
+    // visibility and can flip a consumer from building to failing.
     //
     // The `package_group_dropped` workspace:
     //   //lib:consumers -- a package_group (created by a macro in lib/defs.bzl)
@@ -1485,13 +1484,12 @@ class E2ETest {
     // //consumer:use_thing successfully, workspace B fails visibility analysis
     // for it. The BUILD files and every native rule are byte-for-byte identical
     // across the checkouts, so the sole semantic change rides entirely on the
-    // (dropped) PACKAGE_GROUP target.
+    // PACKAGE_GROUP target.
     //
-    // Current (buggy) behaviour: bazel-diff reports ZERO impacted targets. When
-    // #441's under-invalidation is fixed (e.g. by supporting PACKAGE_GROUP
-    // targets and their reverse-visibility dependents), the changed package_group
-    // -- and/or //consumer:use_thing -- should start appearing here and this
-    // assertion will flag that the behaviour changed.
+    // Fixed behaviour: the package_group is lowered to a synthetic rule and
+    // hashed, and RuleHasher follows the (nodep) `visibility` edge from
+    // //lib:thing to it -- so the group, the rule it gates, and that rule's
+    // transitive dependents are all reported.
     val workspaceA = copyTestWorkspace("package_group_dropped")
     val workspaceB = copyTestWorkspace("package_group_dropped")
 
@@ -1534,20 +1532,18 @@ class E2ETest {
         filterBazelDiffInternalTargets(
             impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet())
 
-    // The genuinely-affected consumer is NOT reported -- pinning the false
-    // negative. (It flips from building to failing visibility analysis, yet its
-    // hash, and every other rule/source-file hash, is unchanged.)
-    assertThat(impacted.contains("//consumer:use_thing")).isEqualTo(false)
-
-    // Nothing at all is reported: the package_group is dropped, so the change is
-    // completely invisible to bazel-diff. This is exactly the under-invalidation
-    // #441 describes.
+    // The changed package_group itself, the rule it gates (via the visibility
+    // edge), and that rule's transitive dependents -- including generated files
+    // -- are all reported. In particular //consumer:use_thing, which really does
+    // flip from building to failing visibility analysis, is now surfaced.
     assertThat(impacted)
-        .transform(
-            "editing a dropped PACKAGE_GROUP's `packages` list should surface an impacted target, but got: $impacted") {
-              it.size
-            }
-        .isEqualTo(0)
+        .isEqualTo(
+            setOf(
+                "//lib:consumers",
+                "//lib:thing",
+                "//lib:thing.txt",
+                "//consumer:use_thing",
+                "//consumer:use_thing.txt"))
   }
 
   /**

cli/src/test/resources/workspaces/package_group_dropped/consumer/BUILD

@@ -1,8 +1,9 @@
 # Depends on //lib:thing, whose visibility is controlled by the //lib:consumers
 # package_group. Emptying that package_group's `packages` list (the only change
-# between the two checkouts) would make this target fail visibility analysis in
-# a real build -- yet bazel-diff reports zero impacted targets because the
-# package_group is dropped from the hashed graph. See //lib:BUILD.
+# between the two checkouts) makes this target fail visibility analysis in a
+# real build -- and bazel-diff now reports it as impacted, via the
+# //lib:consumers -> //lib:thing visibility edge and the ordinary dep edge from
+# here to //lib:thing. See //lib:BUILD.
 genrule(
     name = "use_thing",
     srcs = ["//lib:thing"],

cli/src/test/resources/workspaces/package_group_dropped/lib/BUILD

@@ -1,21 +1,20 @@
-# Reproducer workspace for https://github.com/Tinder/bazel-diff/issues/441
+# Regression-test workspace for the under-invalidation (false-negative) half of
+# https://github.com/Tinder/bazel-diff/issues/441.
 #
-# The under-invalidation (false-negative) half of the issue: bazel-diff drops
-# any target whose query `Discriminator` is not RULE / SOURCE_FILE /
-# GENERATED_FILE. `PACKAGE_GROUP` is one such dropped type
-# (BazelQueryService.toBazelTarget -> logs "Unsupported target type" and
-# returns null). Because the package_group is never hashed, a change to its
-# `packages` list -- which genuinely alters downstream visibility and can flip
-# a consumer from building to failing -- is invisible to bazel-diff, so
-# `get-impacted-targets` reports NOTHING.
+# bazel-diff used to drop any target whose query `Discriminator` is not RULE /
+# SOURCE_FILE / GENERATED_FILE, so a `PACKAGE_GROUP` change -- which genuinely
+# alters downstream visibility and can flip a consumer from building to failing
+# -- was invisible and `get-impacted-targets` reported NOTHING. Package_groups
+# are now lowered to synthetic rules and hashed, and RuleHasher follows the
+# (nodep) `visibility` edge from a gated rule to its package_group.
 #
 # `consumers` is created by a macro (see defs.bzl) whose allow-list lives in
 # that `.bzl`. Between the two checkouts we edit ONLY defs.bzl (ALLOWED -> []),
 # so the BUILD files and `:thing` stay byte-identical; the only semantic change
-# rides entirely on the dropped PACKAGE_GROUP target. `:thing` is declared
-# directly (NOT via the macro) so its per-rule `.bzl` seed never picks up
-# defs.bzl -- isolating the dropped-package_group bug from the separate `.bzl`
-# provenance behaviour that is the over-invalidation half of #441.
+# rides entirely on the PACKAGE_GROUP target. `:thing` is declared directly
+# (NOT via the macro) so its per-rule `.bzl` seed never picks up defs.bzl --
+# isolating the package_group behaviour from the separate `.bzl` provenance
+# behaviour that is the over-invalidation half of #441.
 
 load(":defs.bzl", "define_consumers")
 

cli/src/test/resources/workspaces/package_group_dropped/lib/defs.bzl

@@ -4,7 +4,7 @@ The `packages` allow-list lives here (not in the BUILD file) so that the ONLY
 change between the two checkouts is to a `.bzl` that feeds a `package_group`.
 The BUILD files, and every native rule in them, are byte-for-byte identical
 across the two revisions -- so the sole semantic change is carried entirely by
-the (dropped) PACKAGE_GROUP target. This mirrors issue #441's macro-created
+the PACKAGE_GROUP target. This mirrors issue #441's macro-created
 package_group experiment. Flip `ALLOWED` to `[]` in the "after" checkout to
 revoke `//consumer`'s visibility of `//lib:thing`.
 """