You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Since #342, and more narrowly since #436, bazel-diff treats the contents of .bzl files in a native rule's macro instantiation stack as part of that rule's identity. As a result, any edit to the macro source marks the generated rule as impacted, even when evaluation produces the same rule and package state.
Editing a macro to include a debug print statement is a good example that distinguishes between source changes and Bazel's loaded package state. It's a change according to the former but not the latter. Macros run during package loading and no longer exist during analysis or execution. If the macro produces the same loaded package state, a test defined by that package state can't be affected by whether the print statement was there.
Removing .bzl provenance from hashes right now would introduce real false negatives because the ordinary query proto is not a complete representation of loaded-package semantics. I'll provide a concrete configurable-attribute example below.
However, with further changes that I'll describe below, I believe bazel-diff could (optionally, perhaps) remove its .bzl provenance from hashes safely, and base them instead on loaded package state, resulting in
Then a revision adds only the following statement to the macro body:
print("instantiating", name)
The generated rule has the same class, attributes, inputs, and outputs. With bazel-diff before #342, no targets are impacted under either Bazel 6.5.0 or 7.0.0. With the current provenance hashing, //:generated and //:generated.txt are impacted under both versions.
This does not appear to be a Bazel 6-to-7 query behavior change. In both versions, query represents the Starlark rule-definition digest as the synthetic $rule_implementation_hash attribute. The Build.Rule.skylark_environment_hash_code field is not populated by these query invocations (it's populated by Bazel's Packages.RuleFormatter, which query doesn't use).
For a native rule created by a macro, there is no rule implementation digest to change. #436 instead hashes the .bzl paths found in the rule's instantiation stack:
Why the print-only revision case looks non-semantic after the loading phase
Bazel documents legacy macros as loading-phase functions and says that, by the end of loading, macros no longer exist and Bazel sees the concrete instantiated rules:
After a successful package loading phase, an ordinary macro's source text and provenance have no independent build semantics. A macro edit can affect analysis, execution, or tests only by changing the resulting loaded package state.
Changing an emitted attribute, target set, source-file declaration, package group, package error state, or other package data can be meaningful. Adding a print(), changing comments, or refactoring computation that produces the same package state is not.
Besides extra test execution, source-level invalidation can be costly for widely used macros. #436 substantially reduced the scope compared with the whole-package and whole-workspace implementations, but every generated rule using an edited macro still changes its hash even when the post-edit loaded package is identical.
Important soundness counterexample: flattened select()
The normal query proto is lossy. For example, consider a macro-generated native genrule whose scalar cmd is configurable:
Enabling $internal_attr_hash still reports no impacted targets for the print-only native-macro edit.
This is expected from Bazel's query options: --proto:flatten_selects defaults to true, list selectors are lossibly flattened to their possible-value union, and scalar selectors are extremely lossily flattened to null:
The strongest affordance Bazel provides to compensate for this problem is $internal_attr_hash, which was added specifically to determine whether a rule changed "in a potentially meaningful way across queries":
Its implementation hashes the raw selector representation even if the displayed attribute was flattened. It also includes the Starlark rule-definition digest, package error state, and certain package-wide data:
So, .bzl provenance currently compensates for genuine query-observability gaps, but more broadly than necessary for at least this class of change.
Loaded package state is broader than rule attributes
Even unflattened rule attributes are not a complete semantic model. As a separate experiment, I changed only the packages list of a macro-created package_group. An unchanged downstream rule went from building successfully to failing visibility analysis. The consumer rule's attributes and the source file's visibility label were unchanged; the changed state was the PACKAGE_GROUP target.
Bazel's query proto represented the package-group change, but bazel-diff v35.0.0 discarded it as an unsupported target type:
In that reproducer, v35.0.0 reported no impacted targets. This means the current implementation can simultaneously over-invalidate print-only macro edits and under-invalidate other macro-driven package changes. I am happy to report the package-group case separately if that would be easier to track.
Possible direction
Would bazel-diff's maintainers be open to distinguishing two policies?
A conservative provenance policy, equivalent to the current behavior, for users who prefer source-level invalidation for any macro edit.
An policy that invalidates only when Bazel exposes a potentially meaningful package or rule change.
A policy implementation for the latter could investigate:
Requesting --proto:include_synthetic_attribute_hash for rules.
Hashing semantically relevant SourceFile fields, including visibility, package groups, licenses, features, and package error state.
Supporting PACKAGE_GROUP and possibly ENVIRONMENT_GROUP targets and their dependency relationships.
How this policy interacts with other bazel-diff features would also need some attention. For example, $internal_attr_hash includes every rule attribute, conflicting with the intentions behind bazel-diff's --ignoredRuleHashingAttributes option.
I would be interested in contributing a PR, but I would first like to understand which correctness contract bazel-diff wants its hashes to provide: source-provenance affectedness, loaded-package-state affectedness, or selectable versions of both. I'd especially like to understand the use cases for source-provenance affectedness, aside from compensation for soundness issues like the flattened select issue described above.
Summary
Since #342, and more narrowly since #436, bazel-diff treats the contents of
.bzlfiles in a native rule's macro instantiation stack as part of that rule's identity. As a result, any edit to the macro source marks the generated rule as impacted, even when evaluation produces the same rule and package state.Editing a macro to include a debug print statement is a good example that distinguishes between source changes and Bazel's loaded package state. It's a change according to the former but not the latter. Macros run during package loading and no longer exist during analysis or execution. If the macro produces the same loaded package state, a test defined by that package state can't be affected by whether the print statement was there.
Removing
.bzlprovenance from hashes right now would introduce real false negatives because the ordinary query proto is not a complete representation of loaded-package semantics. I'll provide a concrete configurable-attribute example below.However, with further changes that I'll describe below, I believe bazel-diff could (optionally, perhaps) remove its
.bzlprovenance from hashes safely, and base them instead on loaded package state, resulting inCurrent behavior
Consider a macro that creates a native rule:
Then a revision adds only the following statement to the macro body:
The generated rule has the same class, attributes, inputs, and outputs. With bazel-diff before #342, no targets are impacted under either Bazel 6.5.0 or 7.0.0. With the current provenance hashing,
//:generatedand//:generated.txtare impacted under both versions.This does not appear to be a Bazel 6-to-7 query behavior change. In both versions, query represents the Starlark rule-definition digest as the synthetic
$rule_implementation_hashattribute. TheBuild.Rule.skylark_environment_hash_codefield is not populated by these query invocations (it's populated by Bazel'sPackages.RuleFormatter, which query doesn't use).For a native rule created by a macro, there is no rule implementation digest to change. #436 instead hashes the
.bzlpaths found in the rule's instantiation stack:https://github.com/Tinder/bazel-diff/blob/v35.0.0/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt#L28-L59
Why the print-only revision case looks non-semantic after the loading phase
Bazel documents legacy macros as loading-phase functions and says that, by the end of loading, macros no longer exist and Bazel sees the concrete instantiated rules:
https://bazel.build/versions/7.5.0/extending/macros
The broader evaluation model describes analysis as consuming the graph produced by the loading phase:
https://bazel.build/versions/9.0.0/extending/concepts#evaluation-model
My understanding can be summarized like this:
Changing an emitted attribute, target set, source-file declaration, package group, package error state, or other package data can be meaningful. Adding a
print(), changing comments, or refactoring computation that produces the same package state is not.Besides extra test execution, source-level invalidation can be costly for widely used macros. #436 substantially reduced the scope compared with the whole-package and whole-workspace implementations, but every generated rule using an edited macro still changes its hash even when the post-edit loaded package is identical.
Important soundness counterexample: flattened
select()The normal query proto is lossy. For example, consider a macro-generated native
genrulewhose scalarcmdis configurable:In a subsequent revision, swap the values for
:leftand:rightwithout changing the keys or possible-value set.With Bazel 8.5.1 I observed:
bazel query --output=streamed_jsonprotooutput is semantically identical between the two revisions. Thecmdattribute has no value.bazel build --define mode=left //:chosenproducesLEFTbefore the edit andRIGHTafterward.bazel query --noproto:flatten_selectsexposes the changed selector map.bazel query --proto:include_synthetic_attribute_hashemits a different$internal_attr_hash.-co=--proto:include_synthetic_attribute_hash, reports//:chosenand//:chosen.txt.$internal_attr_hashstill reports no impacted targets for the print-only native-macro edit.This is expected from Bazel's query options:
--proto:flatten_selectsdefaults to true, list selectors are lossibly flattened to their possible-value union, and scalar selectors are extremely lossily flattened to null:https://github.com/bazelbuild/bazel/blob/8.5.1/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java#L244-L281
The strongest affordance Bazel provides to compensate for this problem is
$internal_attr_hash, which was added specifically to determine whether a rule changed "in a potentially meaningful way across queries":bazelbuild/bazel@4107a4c
Its implementation hashes the raw selector representation even if the displayed attribute was flattened. It also includes the Starlark rule-definition digest, package error state, and certain package-wide data:
https://github.com/bazelbuild/bazel/blob/8.5.1/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java#L36-L149
So,
.bzlprovenance currently compensates for genuine query-observability gaps, but more broadly than necessary for at least this class of change.Loaded package state is broader than rule attributes
Even unflattened rule attributes are not a complete semantic model. As a separate experiment, I changed only the
packageslist of a macro-createdpackage_group. An unchanged downstream rule went from building successfully to failing visibility analysis. The consumer rule's attributes and the source file's visibility label were unchanged; the changed state was thePACKAGE_GROUPtarget.Bazel's query proto represented the package-group change, but bazel-diff v35.0.0 discarded it as an unsupported target type:
https://github.com/Tinder/bazel-diff/blob/v35.0.0/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt#L609-L617
In that reproducer, v35.0.0 reported no impacted targets. This means the current implementation can simultaneously over-invalidate print-only macro edits and under-invalidate other macro-driven package changes. I am happy to report the package-group case separately if that would be easier to track.
Possible direction
Would bazel-diff's maintainers be open to distinguishing two policies?
A policy implementation for the latter could investigate:
--proto:include_synthetic_attribute_hashfor rules.SourceFilefields, including visibility, package groups, licenses, features, and package error state.PACKAGE_GROUPand possiblyENVIRONMENT_GROUPtargets and their dependency relationships.How this policy interacts with other bazel-diff features would also need some attention. For example,
$internal_attr_hashincludes every rule attribute, conflicting with the intentions behind bazel-diff's--ignoredRuleHashingAttributesoption.I would be interested in contributing a PR, but I would first like to understand which correctness contract bazel-diff wants its hashes to provide: source-provenance affectedness, loaded-package-state affectedness, or selectable versions of both. I'd especially like to understand the use cases for source-provenance affectedness, aside from compensation for soundness issues like the flattened select issue described above.