lint: enable iface and revive, drop maintidx - #2193
Conversation
28ca35e to
31208a5
Compare
|
@atharrva01 could you please rebase and squash your commits to one? |
31208a5 to
726568e
Compare
|
On the two Coveralls failures in the last run, I don't think they came from this change. Nothing here adds or removes a coverable statement. Every deletion is an interface The one deleted file with real statements is the Both flags also move around by roughly a point on unrelated PRs:
43.57% is inside that range and above #2174, which passed. These are the The branch was also 9 commits behind main, so the base build it compared against was stale. |
46d428a to
7d47f6f
Compare
Next step of LFDT-Panurus#1991, after LFDT-Panurus#2114. Covers iface, plus maintidx and revive, the two that were left open. I made the call on both rather than leave them blocked, happy to change either. Each linter was measured on its own, uncapped, across every module in GO_MODULES. iface reports 95 issues: 66 unused, 17 opaque, 12 identical. Enabled with only identical. unused only looks inside the declaring package. The driver pattern puts SPI interfaces in dedicated driver and api packages that are implemented and consumed from other modules, so they are unused at home by construction. I counted cross-package references for all 66: 61 are referenced by between 1 and 79 files elsewhere in the tree, driver.Driver among them. Five had none and were genuinely dead, deleted here: tcc.Agent endorsement.ChannelMSPManager metrics.Collector fabtokenv1.FSCPlatform driver.TokenNotifierDriver ChannelMSPManagerProvider still claimed in its comment to resolve a ChannelMSPManager while actually returning fsc.MSPManager, so that comment is corrected too. opaque wants a constructor to return its concrete type, but in 14 of its 17 reports that type is unexported, so callers could not name the result and revive's unexported-return rule in the same config would reject it. One is impossible outright: the concrete type is lazy.lazyProvider, an unexported generic in fabric-smart-client. The two exported ones are NewQuantityFromUInt64, a deliberate member of the polymorphic Quantity constructor family next to ToQuantity, and NewIdemixCASupport. identical reports six pairs. Three are real: htlc.Vault and multisig.Vault are byte-identical duplicates of TokenVault in the same file and nothing referenced either of them, so both are deleted. driver.ValidatorLedger has the same method set as driver.Ledger and two references against Ledger's 54, one of them the token.Ledger alias. Its generated mock was identical to mock.Ledger modulo the name. Merged into Ledger, mock deleted, token.Ledger now aliases driver.Ledger. A stray duplicate counterfeiter directive for Validator went with it. Three are deliberate and carry a specific nolint with the reason: Input and LedgerToken are different concepts that happen to share GetOwner, CleanupLeadership and RecoveryLeadership are parallel on purpose because the two take different advisory locks, and TransactionDB and AuditDB are separate DI bindings resolved by type. revive is enabled, but not with enable-all-rules. That reports 3452 issues over 688 files, which is not something anyone can review in one go. The config now lists 38 rules explicitly, all of which are already clean tree-wide, so revive lands as a ratchet against new code rather than as a backlog. Four fixes were needed to get there: one comment spacing in a zkatdlog test and three in txgen's error.go. The rules left out are named in a comment in the config with their current counts, so the next step does not have to re-measure. The big ones are exported (1568), unused-parameter (362), unused-receiver (334) and import-shadowing (328). Three more are left out because another enabled linter already covers them, line-length-limit by lll, imports-blocklist by depguard, file-header by goheader, and six because they are no-ops at their default settings. Dropping enable-all-rules also removes the nine "disabled: true" entries, which only existed to switch rules back off, and argument-limit and function-result-limit, which were tuned but have 103 and 49 violations and so belong with the deferred set. maintidx has its settings block deleted rather than enabled. It reports 19 functions and every one of them would have to be suppressed: 12 are in _test.go, 4 are in the shared testutils and dbtest helpers, 1 is Prove in the CSP range proof, and 2 are the fungible suite entry point and its topology. The score is driven by Halstead volume, so a long table-driven test or a flat block of declarative topology wiring scores badly for being long rather than for being tangled, and table-driven tests are the style AGENTS.md asks for. A linter that starts at a 100% suppression rate is measuring the wrong thing here. gocognit is still configured and unenabled, and at min-complexity 15 it targets branching directly, which is the check actually wanted. I tried it the other way first, with path exclusions for tests plus nolints on the rest. It worked, but warn-unused: true then logs an unused-exclusion warning in the seven modules where the paths do not match, so it traded 19 suppressions for noise in every other module. That leaves gocognit and wrapcheck as the only settings blocks for linters that are not enabled. golangci-lint run is clean on all nine modules, make checks passes, and tests pass on the touched packages. Deleting the seven exported interfaces is a breaking change for anyone referencing them out of tree. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
7d47f6f to
ae4b7dc
Compare
Next step of #1991, after #2114. Covers
iface, plus the two you were waiting on me to askabout,
maintidxandrevive. I went ahead and made the call on both rather than leave themblocked, happy to change either.
Measured each linter on its own this time, uncapped, across every module in
GO_MODULES.iface
95 issues: 66
unused, 17opaque, 12identical. Enabled with onlyidentical.unusedis disabled. It only looks inside the declaring package, and the driver patternputs SPI interfaces in dedicated
driverandapipackages that are implemented and consumedfrom other modules, so they are unused at home by construction. I counted cross-package
references for all 66: 61 are referenced by between 1 and 79 files elsewhere in the tree,
driver.Driveramong them. Five had none and were genuinely dead:Agentnetwork/fabric/tccChannelMSPManagernetwork/fabricx/endorsementCollectornwo/txgen/service/metricsFSCPlatformnwo/token/generators/crypto/fabtokenv1TokenNotifierDriverstorage/db/driverDeleted.
ChannelMSPManagerProvideralso still claimed in its comment to resolve aChannelMSPManagerwhile actually returningfsc.MSPManager, so that comment is fixed.opaqueis disabled. It wants constructors to return their concrete type, but in 14 of its17 reports that type is unexported, so callers could not name the result and revive's
unexported-returnrule in the same config would reject it. One is impossible outright: theconcrete type is
lazy.lazyProvider, an unexported generic in fabric-smart-client. The twoexported ones are
NewQuantityFromUInt64, a deliberate member of the polymorphicQuantityconstructor family next to
ToQuantity, andNewIdemixCASupport.identicalis enabled. Six pairs, three real:htlc.Vaultandmultisig.Vaultare byte-identical duplicates ofTokenVaultin the samefile, and nothing referenced either of them. Deleted.
driver.ValidatorLedgerhas the same method set asdriver.Ledger, and two referencesagainst
Ledger's 54, one of which is thetoken.Ledgeralias. Its generated mock wasidentical to
mock.Ledgermodulo the name. Merged intoLedger, mock deleted,token.Ledgernow aliasesdriver.Ledger. A stray duplicate counterfeiter directive forValidatorwent with it.The other three are deliberate and carry a specific
nolintwith the reason:InputandLedgerTokenare different concepts that happen to shareGetOwner,CleanupLeadershipandRecoveryLeadershipare parallel on purpose because the two take different advisory locks, andTransactionDBandAuditDBare separate DI bindings resolved by type.revive
Enabled, but not with
enable-all-rules. That reports 3452 issues over 688 files, which isnot reviewable in one change. The config now lists 38 rules explicitly, all already clean
tree-wide, so revive lands as a ratchet against new code rather than as a backlog. Four fixes
were needed to get there, one comment spacing in a zkatdlog test and three in txgen's
error.go.The rules left out are listed in a comment in the config with their current counts so the next
step does not have to re-measure. The big ones:
Three more are left out because another enabled linter already covers them (
line-length-limitby
lll,imports-blocklistbydepguard,file-headerbygoheader) and six because theyare no-ops at their default settings.
Dropping
enable-all-rulesalso removes the ninedisabled: trueentries, which only existedto switch rules back off, and
argument-limitandfunction-result-limit, which were tuned buthave 103 and 49 violations and so belong with the deferred set.
maintidx
Settings block deleted rather than enabled. It reports 19 functions and every one would have to
be suppressed: 12 in
_test.go, 4 in the sharedtestutilsanddbtesthelpers, 1 isProvein the CSP range proof, and 2 are the fungible suite entry point and its topology. The score is
driven by Halstead volume, so a long table-driven test or a flat block of declarative topology
wiring scores badly for being long rather than for being tangled, and table-driven tests are the
style AGENTS.md asks for. A linter that starts at a 100% suppression rate is measuring the wrong
thing here.
gocognitis still configured and unenabled, and atmin-complexity: 15it targetsbranching directly, which is the check actually wanted.
I tried it the other way first, with path exclusions for tests plus nolints on the rest. It
worked, but
warn-unused: truethen logs an unused-exclusion warning in the seven modules wherethe paths do not match, so it traded 19 suppressions for noise in every other module.
After this
gocognitandwrapcheckare the only settings blocks left for linters that are not enabled.golangci-lint runis clean on all nine modules,make checkspasses, and tests pass on thetouched packages.
Deleting the seven exported interfaces is a breaking change for anyone referencing them out of
tree.