bazelbuild / bazelbuild/bazel-skylib
bzl plugin does not update `deps` on existing `bzl_library` targets
- Dominant language
- Starlark
- Stars
- 444
- Forks
- 202
- PR merge metrics
- No merged PRs in 30d
Description
The `bzl` Gazelle plugin sets `deps` correctly when it first generates a `bzl_library`, but never updates them afterwards. Once a target exists in a BUILD file, its `deps` are frozen no matter how the `load()` statements in its `srcs` change.
## Reproduction
1. Run Gazelle with the `bzl` plugin so that it generates a `bzl_library`.
2. Add a new `load()` statement to a `.bzl` file whose `bzl_library` already exists.
3. Re-run Gazelle.
The new dependency is never added. Deleting a correct dep and re-running does not restore it either.
## Cause
`Resolve()` computes the right value and calls `r.SetAttr("deps", deps)`, so the resolution itself is fine. The problem is the `KindInfo` in `gazelle/bzl/gazelle.go`:
```go
var kinds = map[string]rule.KindInfo{
"bzl_library": {
NonEmptyAttrs: map[string]bool{"srcs": true, "deps": true},
MergeableAttrs: map[string]bool{"srcs": true},
},
}
```
`deps` appears in neither `MergeableAttrs` nor `ResolveAttrs`. Gazelle's merger selects `MergeableAttrs` in the pre-resolve phase and `ResolveAttrs` in the post-resolve phase (`merger/merger.go`, `getMergeAttrs`). Since `deps` is resolved in the post-resolve phase and no `ResolveAttrs` is declared, the computed value is discarded for any rule that already exists. A newly generated rule has nothing to merge against, which is why fresh targets look correct.
Gazelle's own Go language declares `ResolveAttrs: map[string]bool{"deps": true}` in `language/go/kinds.go`.
## Existing fix
#621 already proposes exactly this change and adds a regression fixture. It has been open since May with no review.
## A caveat worth knowing before #621 lands
I applied #621's change locally against a repo with roughly 100 `bzl_library` targets. It behaves well overall, and notably it also removes stale deps left over from loads that no longer exist, which is a second class of drift.
It does, however, surface #559. Enabling `ResolveAttrs` means the guessed labels for cross-repository loads now get written onto existing targets, replacing correct hand-written ones. In our case the source has:
```starlark
load("@rules_cc//cc/common:cc_common.bzl", "cc_common")
load("@rules_cc//cc/common:cc_info.bzl", "CcInfo")
```
The correct dependency is `@rules_cc//cc/common` (the package's `:common` target). With #621 applied, Gazelle rewrites it to the guessed labels, neither of which exists:
```diff
- "@rules_cc//cc/common",
+ "@rules_cc//cc/common:cc_common",
+ "@rules_cc//cc/common:cc_info",
```
```
ERROR: no such target '@@rules_cc+//cc/common:cc_common': target 'cc_common' not declared in package 'cc/common' (did you mean common, or cc_common.bzl?)
```
Today that mis-guess is mostly harmless, because an existing target's `deps` are never rewritten. #621 removes that accidental protection, so #559 becomes load-bearing for any repo that loads `.bzl` files from an external repository whose `bzl_library` target name does not match the file name.
This is not an argument against #621, which fixes a real bug. It is a note that the two issues interact, and that #559 may deserve attention alongside it. Marking the affected `deps` with `# keep` is a workable local mitigation in the meantime.
Contributor guide
Research direction
Read gazelle/bzl/gazelle.go and merger/merger.go to trace how bzl_library attributes are selected before and after resolution. Compare the regression fixture proposed in #621, reproduce the existing-target case, and verify that added and removed load() dependencies are updated without overlooking the cross-repository label issue in #559.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- build-system, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 56/100