lint: reactivate several disabled linters#10872
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the codebase by leveraging the per-iteration loop variable scoping introduced in Go 1.22. By enabling the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🔴 PR Severity: CRITICAL
🔴 Critical (10 files)
🟠 High (5 files)
🟢 Low (1 file)
AnalysisThis PR enables the `copyloopvar` linter and removes redundant loop variable shadow copies (e.g., `x := x`) that were needed before Go 1.22 but are now unnecessary since loop variables are scoped per-iteration. The change is purely mechanical — no logic is altered, only boilerplate removed. Despite the low-risk nature of the change, the severity is CRITICAL because the highest-severity files touched include core packages such as `lnwallet/`, `contractcourt/`, `channeldb/`, and `funding/`. Per classification rules, the highest-severity file determines the PR severity. No severity bump was applied: 16 non-test/non-generated files (< 20 threshold) and 41 lines changed (< 500 threshold). To override, add a `severity-override-{critical,high,medium,low}` label. |
There was a problem hiding this comment.
Code Review
This pull request removes obsolete loop variable rebindings across numerous test and production files, aligning with Go's modern loop semantics, and disables the copyloopvar linter. The review feedback correctly identifies several obsolete comments explaining the removed rebinding logic that should be cleaned up to maintain code clarity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
invoices/invoiceregistry.go (512)
Since the loop variable rebinding (addEvent := addEvent) has been removed, the comment explaining why the variable is being rebound is now obsolete and should be removed to keep the codebase clean and accurate.
References
- Comments must explain the intention and context of the code accurately. Obsolete comments explaining removed logic should be cleaned up. (link)
invoices/invoiceregistry.go (526)
Since the loop variable rebinding (settleEvent := settleEvent) has been removed, the comment explaining why the variable is being rebound is now obsolete and should be removed.
References
- Comments must explain the intention and context of the code accurately. Obsolete comments explaining removed logic should be cleaned up. (link)
lnrpc/walletrpc/walletkit_server_test.go (46-47)
Since the loop variable redeclarations have been removed, the comment explaining why they are being redeclared is now obsolete and should be removed.
References
- Comments must explain the intention and context of the code accurately. Obsolete comments explaining removed logic should be cleaned up. (link)
|
maybe check all the switched off linters maybe we can reactivate more ? |
|
also could you separate test files and the other files in different commits ? |
True, I'll check for others linters. |
c82db41 to
0f9c709
Compare
Since Go 1.22 loop variables are scoped per-iteration, so the `x := x` / `a, b := a, b` copies inside range/for loops are no longer needed. This removes the existing redundant copies in test files.
…test files Since Go 1.22 loop variables are scoped per-iteration, so the `x := x` / `a, b := a, b` copies inside range/for loops are no longer needed. This enables the `copyloopvar` linter so these are caught automatically and removes the existing redundant copies in non-test files.
0f9c709 to
90fbe48
Compare
|
Done @ziggie1984 |
90fbe48 to
3f677e6
Compare
This PR enables five previously-disabled linters and fixes the violations they surface. It also corrects a mislabeled section of
.golangci.ymlwhere a block of active linters was wrongly commented as "Deprecated linters".Each linter is enabled in its own commit, with code fixes landing before the linter is switched on, so CI stays green at every commit. Code changes are also split into separate test-file and non-test-file commits.
Linters enabled:
copyloopvar: redundantx := xloop-variable copies, unnecessary since Go 1.22 per-iteration scoping (161 files).wastedassign: dead stores assigned but never read (25 sites). This surfaced a real missing error check inrouting/router_test.go, where err from New() was never asserted.bodyclose: unclosed HTTP response bodies (0 existing issues).rowserrcheck: unchecked sql.Rows.Err() (0 existing issues).sqlclosecheck: unclosed sql.Rows / sql.Stmt (0 existing issues).Linters left disabled, now with accurate comments instead of the wrong "deprecated" label:
contextcheck: requires threadingcontext.Contextthrough many existing signatures, including test harnesses.tparallel: requires addingt.Parallel()to many existing subtests, which can surface shared-state races.unparam: sizeable backlog of unused parameters to clean up first.nilerr: too noisy here; nearly all reports are intentional documented error-swallowing or false positives.noctx: only flags a couple of interface methods and a test helper with no context to thread through.#10858 (comment)