Azure / Azure/unbounded

Dead exported code in internal/ is invisible to `unused`; evaluate x/tools deadcode

Open
#670 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
28
Forks
11
Avg merge
1d 8h
Merged PRs (30d)
55

Description

Found during review of #668.

`internal/net/netlink` has 7 of 22 exported `LinkManager` methods with **no production caller**. `make lint` is clean, and `unused` is enabled. It cannot see them, and no setting changes that.

## The evidence

| Method | Production callers | Test callers |
|---|---|---|
| `EnsureIPIPInterfaceWithRemote` | 0 | 1 |
| `EnsureGeneveInterfaceWithRemote` | 0 | 1 |
| `EnsureBridge` | 0 | 1 |
| `EnsureGeneveInterfaceWithCache` | 0 | 1 |
| `EnsureIPIPInterfaceWithCache` | 0 | 1 |
| `SetLinkNoARP` | 0 | **0** |
| `GetAddresses` | 0 | **0** |

`SetLinkNoARP` and `GetAddresses` have zero references anywhere in the tree.

The live tunnel paths are `EnsureIPIPExternalInterface`, `EnsureGeneveInterface`, `EnsureVXLANInterface`, `EnsureWireGuardInterface` and `EnsureDummyInterface`. The live CNI bridge path is `Exists`/`EnsureMTUWithCache`/`EnsureBridgePortMTUs`/`EnsureBridgePodMTUs` (`mtu.go:19-55`) — `EnsureBridge` is not part of it.

## Why `unused` cannot catch this

Not a gap, a documented design decision. From `honnef.co/go/tools@v0.8.0`, `unused/unused.go:48-62`:

```
- packages use:
- (1.1) exported named types
- (1.2) exported functions (but not methods!)
- named types use:
- (2.1) exported methods
```

The package uses `LinkManager` because it is an exported named type (1.1), and `LinkManager` uses every one of its exported methods (2.1). They are transitively live **because the type is exported**. That the package is `internal/`, and so provably has no external consumers, is not considered.

Three things follow:

- golangci-lint's `unused` settings (`field-writes-are-uses`, `post-statements-are-reads`, `exported-fields-are-used`, `parameters-are-used`, `local-variables-are-used`, `generated-is-used`) do not touch this.
- staticcheck's whole-program mode was **removed**; it survives only in source comments.
- This affects every exported identifier on an exported type across `internal/`, not just this one file.

## Proposal: evaluate `golang.org/x/tools/cmd/deadcode`

Rapid Type Analysis from `main`, which is the analysis `unused` deliberately does not do. Already in the module graph as `x/tools v0.49.0`, and it has `-json`, `-tags`, `-filter` and `-whylive`.

The repo has a clean precedent in `make vulncheck`: run the analyser in JSON mode and let `hack/cmd/vulncheck-gate` own the verdict. `go.mod:5-8` already carries `tool` directives for `govulncheck` and `controller-gen`, so `deadcode` would be a third, version-pinned alongside them.

**Suggested first step: a report, not a gate.** Add the tool directive and a `make deadcode` target, measure the findings across the tree, and only then decide whether a gate and allowlist are worth it. Building a gate around a findings count nobody has seen is a guess.

## Caveats to carry into that work

- Roots are `main` packages only — 31 in this repo.
- Results are valid for a **single** GOOS/GOARCH/tags configuration. This tree has `linux`, `!linux`, `cgo`, `!cgo`, `e2e`, `integrationtest` and `storageboundary`, so one run will report `!linux` code as dead.
- It does not understand `//go:linkname`.
- Its own doc: *"just because a function is reported as dead does not mean it is unconditionally safe to delete it"* — a dead method may still be needed to satisfy an interface.

## The trap worth recording

Without `-test`, code reachable only from tests reads as dead, which is exactly what surfaces these seven. With `-test`, five of them look **live**, because #668 added tests that call them.

Writing tests for dead code defeats the detection. That is an argument for deleting on evidence rather than covering, and a reason to prefer the no-`-test` run for this particular question.

## Scope

1. Add the tool directive and a `make deadcode` report target; measure.
2. Decide on a gate, based on the numbers.
3. Remove the 7 methods above (deliberately **not** done in #668, which stays scoped to error classification).

Contributor guide

Open the contributing guide

Research direction

Start with go.mod's existing tool directives and the make vulncheck target, then review golang.org/x/tools/cmd/deadcode and its JSON output. Add the version-pinned directive and a report-only make deadcode target, run it without -test across the relevant configurations, and document the findings before deciding whether a gate is justified.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
build-system, tooling
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.