Fix #441 under-invalidation: hash PACKAGE_GROUP targets and follow visibility edges#443
Open
tinder-maxwellelliott wants to merge 1 commit into
Open
Fix #441 under-invalidation: hash PACKAGE_GROUP targets and follow visibility edges#443tinder-maxwellelliott wants to merge 1 commit into
tinder-maxwellelliott wants to merge 1 commit into
Conversation
…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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the under-invalidation (false-negative) half of #441, pinned by the reproducer in #442: a change to a
package_group'spackageslist — which can flip a downstream consumer from building to failing visibility analysis — was completely invisible to bazel-diff, becauseBazelQueryService.toBazelTargetdropped every query target whoseDiscriminatorwas notRULE/SOURCE_FILE/GENERATED_FILE.How
Two parts, because hashing the group alone is not enough —
visibilitylabels are nodep labels, sobazel querynever lists them inrule_input, and the group's digest would be stranded with no edge to the rules it gates:PACKAGE_GROUPquery targets to synthetic rules (BazelQueryService.packageGroupToTarget): rule classpackage_group, thepackagesallow-list as a hashedSTRING_LISTattribute, andincludesasrule_inputs (so editing a nested group propagates to every group that includes it). This mirrors how bzlmod repos are already lowered to synthetic//external:*rules, and lets package_groups ride the existing hashing pipeline unchanged.RuleHasher: package_group labels found in a rule'svisibilityattribute are followed as dep edges, so the gated rule — and transitively its dependents — re-hash when the group changes. The special//visibility:public|privateforms and__pkg__/__subpackages__package specs name no target and are filtered out (BazelRule.visibilityPackageGroupLabels); a label whose group is outside the queried graph is skipped, matching its pre-fix invisibility. The edge is suppressed whenvisibilityis in--ignoredRuleHashingAttributes.Hash stability
Existing workspaces without package_groups referenced in
visibilityproduce byte-for-byte identical hashes: only resolvable package_group labels contribute bytes, and//visibility:*-only fixtures are unaffected (verified by the untouched expectations of every other E2E test).Tests
testPackageGroupChangeImpactsConsumers_regressionForIssue441, asserting the exact impacted set://lib:consumers,//lib:thing,//lib:thing.txt,//consumer:use_thing,//consumer:use_thing.txt— the group, the rule it gates, and its transitive dependents, where before the set was empty.visibilityPackageGroupLabels(special-form filtering, dedupe + sort determinism) andisPackageGroup.bazel test //...is green locally except 4 pre-existing cquery/Maven E2E failures that also fail on master in this environment (sandboxedrules_jvm_externalresolution), unrelated to this change.Out of scope
exports_files(..., visibility = ...): source-file targets don't yet follow package_group edges; same class of gap, left as a follow-up.ENVIRONMENT_GROUPtargets remain dropped (near-dead feature; same lowering technique would apply if ever needed).Refs #441, #442.
🤖 Generated with Claude Code