bazelbuild / bazelbuild/bazel-skylib

bzl plugin does not update `deps` on existing `bzl_library` targets

Open
#637 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.