Skip to content

operator, orca, playpen: delete unreferenced helpers - #681

Open
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-orca-operator-playpen
Open

operator, orca, playpen: delete unreferenced helpers#681
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-orca-operator-playpen

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.


operator, orca, playpen: delete unreferenced helpers

operator:

  • component.ApplyOperations and ExecutionResult.Dropped have no reference.
  • override.ProblemsErr and override.AnyKeyLevel likewise; ProblemsErr was the
    package's only use of the errors import.

override.PermittedPaths, override.ProtectedPaths and override.ValidateErr are
reported by make deadcode without -test and are kept. They are called from
override_examples_test.go, which compares the documented allowlist against the
declared one, so they are the mechanism that keeps the docs honest rather than
leftovers from a deleted caller.

orca:

  • app.WithSkipCachestoreSelfTest set a skipCacheSelfTest field that nothing
    else ever set, which made the branch it guarded unreachable and the
    cachestore self-test unconditional in every configuration including the
    integration harness. The option, the field and the dead branch go, and the
    self-test now reads as the single path it already was. The struct comment
    that advertised the escape hatch is corrected.
  • inttest loses four harness affordances with no caller: AllBlobs,
    Azurite.UploadPageBlob (the file's only use of the azblob blob, container
    and pageblob packages), CountingInternalHandlerWrap.CountAcross and
    CountingOrigin.Heads.

The seven remaining app.WithX options and cluster.WithPeerSource /
WithHTTPClient are dead in the default configuration and live under
integrationtest, so they stay. That difference is exactly why make deadcode
scans both tag sets.

playpen: Playpen.WireGuardPrivateKey had no caller. The AllocateOptions field of
the same name is unaffected and still read.

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.

operator:

  - component.ApplyOperations and ExecutionResult.Dropped have no reference.
  - override.ProblemsErr and override.AnyKeyLevel likewise; ProblemsErr was the
    package's only use of the errors import.

override.PermittedPaths, override.ProtectedPaths and override.ValidateErr are
reported by `make deadcode` without -test and are kept. They are called from
override_examples_test.go, which compares the documented allowlist against the
declared one, so they are the mechanism that keeps the docs honest rather than
leftovers from a deleted caller.

orca:

  - app.WithSkipCachestoreSelfTest set a skipCacheSelfTest field that nothing
    else ever set, which made the branch it guarded unreachable and the
    cachestore self-test unconditional in every configuration including the
    integration harness. The option, the field and the dead branch go, and the
    self-test now reads as the single path it already was. The struct comment
    that advertised the escape hatch is corrected.
  - inttest loses four harness affordances with no caller: AllBlobs,
    Azurite.UploadPageBlob (the file's only use of the azblob blob, container
    and pageblob packages), CountingInternalHandlerWrap.CountAcross and
    CountingOrigin.Heads.

The seven remaining app.WithX options and cluster.WithPeerSource /
WithHTTPClient are dead in the default configuration and live under
integrationtest, so they stay. That difference is exactly why `make deadcode`
scans both tag sets.

playpen: Playpen.WireGuardPrivateKey had no caller. The AllocateOptions field of
the same name is unaffected and still read.

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.

1 participant