Skip to content

feat: support if err = doSomething(); err != nil#8

Open
fooree wants to merge 1 commit into
AlwxSin:masterfrom
fooree:feat/inline-err-assign
Open

feat: support if err = doSomething(); err != nil#8
fooree wants to merge 1 commit into
AlwxSin:masterfrom
fooree:feat/inline-err-assign

Conversation

@fooree

@fooree fooree commented Jul 10, 2026

Copy link
Copy Markdown
  • Description

    Extend the noinlineerr analyzer to detect and autofix if err = ...; err != nil patterns (reassignment), not just := declarations.

  • Motivation

    The analyzer previously only flagged if err := f(); err != nil { but missed the equivalent reassignment form if err = f(); err != nil {, which violates the same readability principle — inline
    error handling should be split into a standalone assignment followed by a separate if check. This closes the gap for reassignments that reuse an already-declared err variable.
    I

  • Implementation Details

    • Changed errMessage from a const to a function parameterized by token.Token so the diagnostic message uses := or = depending on the assignment token.
    • Fixed the shadow-check guard condition at :71 — the original if len(Lhs) != 1 || shadowVarsExists(...) matched the = case (which has no shadow concern) but the token.DEFINE condition wasn't
      checked, causing the = case to fall through correctly. Added assignStmt.Tok == token.DEFINE && to the second branch so it only gates := with shadow.
    • Updated all errMessage call sites to pass assignStmt.Tok.
    • Added test function inlineErrReassign in testdata with a // want comment and corresponding golden file entry verifying the autofix output.
  • Test Coverage

    • Added inlineErrReassign test case covering if err = ...; err != nil pattern.
    • Golden file updated to verify the autofix transforms it to err = ... + standalone if.

@AlwxSin AlwxSin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much.

  1. The condition works correctly (&& binds tighter than ||), but mixed operators without parens are hard to read. Please wrap it:

    if len(assignStmt.Lhs) != 1 || (assignStmt.Tok == token.DEFINE && shadowVarsExists(ident.Name, pass.TypesInfo.Scopes[ifStmt])) {
  2. Could you add a test for the multi-LHS reassignment path, e.g.:

    if _, err = doSomething3(); err != nil { // want "avoid inline error handling ..."

    It hits the report-only branch (len(assignStmt.Lhs) != 1) with token.ASSIGN, which isn't covered right now.

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.

3 participants