From 3a1daaecae0b4a0f091c35e4c5fcb48b822b80c8 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Wed, 22 Jul 2026 12:16:39 -0400 Subject: [PATCH] Fix #441 (under-invalidation): hash PACKAGE_GROUP targets and follow visibility edges A change to a package_group's `packages` list can flip a downstream consumer from building to failing visibility analysis, yet bazel-diff reported zero impacted targets: `BazelQueryService.toBazelTarget` dropped every query target whose `Discriminator` was not RULE / SOURCE_FILE / GENERATED_FILE, so PACKAGE_GROUP targets never reached the hashed graph. PR #442 pinned this false negative with a reproducer. Two-part fix: * `BazelQueryService` now lowers a PACKAGE_GROUP query target to a synthetic rule (rule class `package_group`, `packages` allow-list as a hashed STRING_LIST attribute, `includes` as rule_inputs) so it flows through the existing rule-hashing pipeline, mirroring how bzlmod repos are lowered to synthetic //external:* rules. * `visibility` labels are "nodep" labels -- `bazel query` never lists them in `rule_input` -- so hashing the group alone would strand its digest with no edge to the rules it gates. `RuleHasher` now follows package_group labels found in a rule's `visibility` attribute as dep edges (the special `//visibility:*` forms and `__pkg__` / `__subpackages__` package specs are filtered out; labels outside the queried graph are skipped, matching their pre-fix invisibility). Respects --ignoredRuleHashingAttributes for `visibility`. The #442 reproducer is flipped into a regression test: the changed group, the rule it gates, and that rule's transitive dependents (including generated files) are now all reported. Existing hashes are unaffected for workspaces without package_groups referenced in visibility, since only resolvable package_group labels contribute bytes. Fixes the under-invalidation half of #441. The over-invalidation half (print-only macro edits) remains open as a design question, as does source-file visibility (`exports_files(..., visibility = ...)`), which still bypasses package_group edges. Co-Authored-By: Claude Fable 5 --- .../com/bazel_diff/bazel/BazelQueryService.kt | 25 +++++++++ .../kotlin/com/bazel_diff/bazel/BazelRule.kt | 31 +++++++++++ .../kotlin/com/bazel_diff/hash/RuleHasher.kt | 29 ++++++++++ .../com/bazel_diff/bazel/BazelRuleTest.kt | 43 +++++++++++++++ .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 54 +++++++++---------- .../package_group_dropped/consumer/BUILD | 7 +-- .../package_group_dropped/lib/BUILD | 25 +++++---- .../package_group_dropped/lib/defs.bzl | 2 +- 8 files changed, 170 insertions(+), 46 deletions(-) diff --git a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt index 327fcfea..40d1583c 100644 --- a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt +++ b/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()) + } } diff --git a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt index 36ba37b6..9156e440 100644 --- a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt +++ b/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 = + rule.attributeList + .firstOrNull { it.name == "visibility" } + ?.stringListValueList + .orEmpty() + .filter { + !it.startsWith("//visibility:") && + !it.endsWith(":__pkg__") && + !it.endsWith(":__subpackages__") + } + .distinct() + .sorted() + /** * Macro instantiation frames (`"::: "`) naming the BUILD/`.bzl` files that * produced this rule. Empty unless the query ran with `--proto:instantiation_stack`. diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index 3fc4f9ba..ff0c034a 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/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 } diff --git a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt index 179e0eba..1d5e431b 100644 --- a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt @@ -76,6 +76,49 @@ class BazelRuleTest { assertThat(BazelRule(rulePb).tags()).isEqualTo(emptySet()) } + // 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()) + } + + @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 diff --git a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt index 2c559ecf..f2cb98a2 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/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")) } /** diff --git a/cli/src/test/resources/workspaces/package_group_dropped/consumer/BUILD b/cli/src/test/resources/workspaces/package_group_dropped/consumer/BUILD index 27235857..74b8a303 100644 --- a/cli/src/test/resources/workspaces/package_group_dropped/consumer/BUILD +++ b/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"], diff --git a/cli/src/test/resources/workspaces/package_group_dropped/lib/BUILD b/cli/src/test/resources/workspaces/package_group_dropped/lib/BUILD index d541653e..5cc00312 100644 --- a/cli/src/test/resources/workspaces/package_group_dropped/lib/BUILD +++ b/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") diff --git a/cli/src/test/resources/workspaces/package_group_dropped/lib/defs.bzl b/cli/src/test/resources/workspaces/package_group_dropped/lib/defs.bzl index 55b1dbbb..9063c9d5 100644 --- a/cli/src/test/resources/workspaces/package_group_dropped/lib/defs.bzl +++ b/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`. """