Skip to content

hack: delete unreferenced dev-tooling helpers - #682

Open
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-hack-tooling
Open

hack: delete unreferenced dev-tooling helpers#682
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-hack-tooling

Conversation

@plombardi89

Copy link
Copy Markdown
Collaborator

Part of the #670 dead-code sweep, split by component. The tooling that produced these findings is #673; this PR is deletions only and is independent of the other eight — the file sets are disjoint, so they can be reviewed and merged in any order and in parallel.

Why two analyzers were needed to find this

unused cannot see dead exported code under internal/. That is a documented design decision, not a gap: unused/unused.go:48-62 holds that a package uses its exported named types and a named type uses its exported methods, so every method on an exported type is transitively live because the type is exported.

x/tools/cmd/deadcode does not close that gap on its own either. x/tools/go/callgraph/rta/rta.go:281-287,433-437 — converting a value to an interface materializes its runtime type and marks every exported method of that type reachable, because reflection could call any of them. One var i any = x anywhere buys all of x's exported methods a clean bill of health.

So #673 adds both deadcode and hack/cmd/unreferenced, which resolves identifiers instead of tracing reachability. Neither subsumes the other, and findings below are attributed to whichever one saw them.


hack: delete unreferenced dev-tooling helpers

Zero-reference removals in the development tooling:

  • notice/internal/npm.WithFS. The identically named gomod.WithFS is used by
    gomod's tests and stays.
  • notice/internal/testutil.BSD3License. MITLicense, Apache2License and
    WriteTree in the same file are all used.
  • relctl/version.GitRepo.Dir.
  • relctl/gh.Client.API, .Name and .Owner. These had callers - three lines in
    client_test.go - but the test is in package gh, so it can read api, owner
    and repo directly. Exported accessors that exist only for a same-package
    test are surface with no reader.
  • relctl's actionsError, an unexported constant in a const group whose other
    members are live, which is why unused never reported it.

orcadev:

  • hostPortFromURL had no caller outside its own test; both go.
  • percentile sorted its input and delegated to percentileSorted. Only its
    test called it; computeLatencyStats, the live path, calls percentileSorted
    directly having sorted once. The wrapper goes and TestPercentile moves to
    percentileSorted with pre-sorted input, which keeps the p0-to-min and
    p100-to-max edge cases that computeLatencyStats' own test does not cover.

Refs #670


Verification

go build ./..., go vet ./..., golangci-lint over the touched trees (0 issues), make fmt producing no diff, and go build -tags e2e,integrationtest,storageboundary ./.... Full suite runs in CI.

The eight deletion branches were reassembled onto the tooling commits and diffed against the original combined branch: byte-identical, so nothing was lost or duplicated in the split.

Zero-reference removals in the development tooling:

  - notice/internal/npm.WithFS. The identically named gomod.WithFS is used by
    gomod's tests and stays.
  - notice/internal/testutil.BSD3License. MITLicense, Apache2License and
    WriteTree in the same file are all used.
  - relctl/version.GitRepo.Dir.
  - relctl/gh.Client.API, .Name and .Owner. These had callers - three lines in
    client_test.go - but the test is in package gh, so it can read api, owner
    and repo directly. Exported accessors that exist only for a same-package
    test are surface with no reader.
  - relctl's actionsError, an unexported constant in a const group whose other
    members are live, which is why `unused` never reported it.

orcadev:

  - hostPortFromURL had no caller outside its own test; both go.
  - percentile sorted its input and delegated to percentileSorted. Only its
    test called it; computeLatencyStats, the live path, calls percentileSorted
    directly having sorted once. The wrapper goes and TestPercentile moves to
    percentileSorted with pre-sorted input, which keeps the p0-to-min and
    p100-to-max edge cases that computeLatencyStats' own test does not cover.

Refs #670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants