Latest pre-commit hook is broken when used with --all-files or --files flag using multiple packages

[jasonphi]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc.).
• Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

Since the v1.52.0 release, the included pre-commit hook no longer works with the pre-commit --all-files flag or by passing multiple .go files living in different packages with the --files flag, which severely limits its usefulness.

Using v1.51.2:

> pre-commit run --all-files
golangci-lint............................................................Passed

> pre-commit run --files cmd/foo/* bar/*
golangci-lint............................................................Passed

After v1.52.0:

> pre-commit run --all-files
golangci-lint............................................................Failed
- hook id: golangci-lint
- exit code: 7

ERRO [linters_context] typechecking error: named files must all be in one directory; have cmd/foo and bar

> pre-commit run --files cmd/foo/* bar/*
golangci-lint............................................................Failed
- hook id: golangci-lint
- exit code: 7

ERRO [linters_context] typechecking error: named files must all be in one directory; have cmd/foo and bar

Note: this issue was pointed out in the MR after it was merged.

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version 1.52.1 built with go1.20.2 from d92b38cc on 2023-03-21T19:48:38Z

Configuration file

$ cat .golangci.yml
run:
  # Timeout for analysis, e.g. 30s, 5m.
  # Default: 1m
  timeout: 3m
  allow-parallel-runners: true


# This file contains only configs which differ from defaults.
# All possible options can be found here https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml
linters-settings:
  cyclop:
    # The maximal code complexity to report.
    # Default: 10
    max-complexity: 30
    # The maximal average package complexity.
    # If it's higher than 0.0 (float) the check is enabled
    # Default: 0.0
    package-average: 10.0

  exhaustive:
    # Program elements to check for exhaustiveness.
    # Default: [ switch ]
    check:
      - switch
      - map

  gci:
    sections:
      - standard
      - default
      - blank
      - dot
      - prefix(source.foo.com)

  gocognit:
    # Minimal code complexity to report
    # Default: 30 (but we recommend 10-20)
    min-complexity: 20

  gocritic:
    # Settings passed to gocritic.
    # The settings key is the name of a supported gocritic checker.
    # The list of supported checkers can be find in https://go-critic.github.io/overview.
    settings:
      captLocal:
        # Whether to restrict checker to params only.
        # Default: true
        paramsOnly: false
      underef:
        # Whether to skip (*x).method() calls where x is a pointer receiver.
        # Default: true
        skipRecvDeref: false

  gomnd:
    # List of function patterns to exclude from analysis.
    # Values always ignored: `time.Date`,
    # `strconv.FormatInt`, `strconv.FormatUint`, `strconv.FormatFloat`,
    # `strconv.ParseInt`, `strconv.ParseUint`, `strconv.ParseFloat`.
    # Default: []
    ignored-functions:
      - os.Chmod
      - os.Mkdir
      - os.MkdirAll
      - os.OpenFile
      - os.WriteFile
      - prometheus.ExponentialBuckets
      - prometheus.ExponentialBucketsRange
      - prometheus.LinearBuckets

  gomodguard:
    blocked:
      # List of blocked modules.
      # Default: []
      modules:
        - github.com/golang/protobuf:
            recommendations:
              - google.golang.org/protobuf
            reason: "see https://developers.google.com/protocol-buffers/docs/reference/go/faq#modules"
        - github.com/satori/go.uuid:
            recommendations:
              - github.com/google/uuid
            reason: "satori's package is not maintained"
        - github.com/gofrs/uuid:
            recommendations:
              - github.com/google/uuid
            reason: "see recommendation from dev-infra team: https://confluence.gtforge.com/x/gQI6Aw"

  govet:
    # Enable all analyzers.
    # Default: false
    enable-all: true
    # Disable analyzers by name.
    # Run `go tool vet help` to see all analyzers.
    # Default: []
    disable:
      - fieldalignment # too strict
    # Settings per analyzer.
    settings:
      # shadow:
        # Whether to be strict about shadowing; can be noisy.
        # Default: false
        # strict: true

  lll:
    # Default: 120.
    line-length: 150

  nakedret:
    # Make an issue if func has more lines of code than this setting, and it has naked returns.
    # Default: 30
    max-func-lines: 0

  nolintlint:
    # Exclude following linters from requiring an explanation.
    # Default: []
    allow-no-explanation: [ funlen, gocognit, lll ]
    # Enable to require an explanation of nonzero length after each nolint directive.
    # Default: false
    require-explanation: true
    # Enable to require nolint directives to mention the specific linter being suppressed.
    # Default: false
    require-specific: true

  rowserrcheck:
    # database/sql is always checked
    # Default: []
    packages:
      - github.com/jmoiron/sqlx

  tenv:
    # The option `all` will run against whole test files (`_test.go`) regardless of method/function signatures.
    # Otherwise, only methods that take `*testing.T`, `*testing.B`, and `testing.TB` as arguments are checked.
    # Default: false
    all: true


linters:
  disable-all: true
  enable:
    ## enabled by default
    - errcheck # checking for unchecked errors, these unchecked errors can be critical bugs in some cases
    - gosimple # specializes in simplifying a code
    - govet # reports suspicious constructs, such as Printf calls whose arguments do not align with the format string
    - ineffassign # detects when assignments to existing variables are not used
    # Disabling staticcheck and unused until https://github.com/dominikh/go-tools/issues/1361 is fixed
    #- staticcheck # is a go vet on steroids, applying a ton of static analysis checks
    #- unused # checks for unused constants, variables, functions and types
    - typecheck # like the front-end of a Go compiler, parses and type-checks Go code
    ## disabled by default
    - asasalint # checks for pass []any as any in variadic func(...any)
    - asciicheck # checks that your code does not contain non-ASCII identifiers
    - bidichk # checks for dangerous unicode character sequences
    - bodyclose # checks whether HTTP response body is closed successfully
    # - cyclop # checks function and package cyclomatic complexity, no longer maintained
    - dupl # tool for code clone detection
    - durationcheck # checks for two durations multiplied together
    # - errname # checks that sentinel errors are prefixed with the Err and error types are suffixed with the Error
    - errorlint # finds code that will cause problems with the error wrapping scheme introduced in Go 1.13
    - execinquery # checks query string in Query function which reads your Go src files and warning it finds
    # - exhaustive # checks exhaustiveness of enum switch statements
    - exportloopref # checks for pointers to enclosing loop variables
    - forbidigo # forbids identifiers
    - funlen # tool for detection of long functions
    - gci
    # - gochecknoglobals # checks that no global variables exist
    - gochecknoinits # checks that no init functions are present in Go code
    # - gocognit # computes and checks the cognitive complexity of functions
    - goconst # finds repeated strings that could be replaced by a constant
    - gocritic # provides diagnostics that check for bugs, performance and style issues
    - gocyclo # computes and checks the cyclomatic complexity of functions
    - godot # checks if comments end in a period
    - goimports # in addition to fixing imports, goimports also formats your code in the same style as gofmt
    - gomnd # detects magic numbers
    - gomoddirectives # manages the use of 'replace', 'retract', and 'excludes' directives in go.mod
    - gomodguard # allow and block lists linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations
    - goprintffuncname # checks that printf-like functions are named with f at the end
    - gosec # inspects source code for security problems
    - lll # reports long lines
    - loggercheck # checks key value pairs for common logger libraries (kitlog,klog,logr,zap)
    - makezero # finds slice declarations with non-zero initial length
    - nakedret # finds naked returns in functions greater than a specified function length
    - nestif # reports deeply nested if statements
    - nilerr # finds the code that returns nil even if it checks that the error is not nil
    - nilnil # checks that there is no simultaneous return of nil error and an invalid value
    - noctx # finds sending http request without context.Context
    - nolintlint
    # - nonamedreturns # reports all named returns
    - nosprintfhostport # checks for misuse of Sprintf to construct a host with port in a URL
    # - predeclared # finds code that shadows one of Go's predeclared identifiers
    - promlinter # checks Prometheus metrics naming via promlint
    - reassign # checks that package variables are not reassigned
    - revive # fast, configurable, extensible, flexible, and beautiful linter for Go, drop-in replacement of golint
    - rowserrcheck # checks whether Err of rows is checked successfully
    - sqlclosecheck # checks that sql.Rows and sql.Stmt are closed
    # - stylecheck # is a replacement for golint; we're using revive instead
    - tenv # detects using os.Setenv instead of t.Setenv since Go1.17
    - testableexamples # checks if examples are testable (have an expected output)
    # - testpackage # makes you use a separate _test package
    - tparallel # detects inappropriate usage of t.Parallel() method in your Go test codes
    - unconvert # removes unnecessary type conversions
    # - unparam # reports unused function parameters
    - usestdlibvars # detects the possibility to use variables/constants from the Go standard library
    - wastedassign # finds wasted assignment statements
    - whitespace # detects leading and trailing whitespace

issues:
  # Maximum count of issues with the same text.
  # Set to 0 to disable.
  # Default: 3
  max-same-issues: 50

  exclude-rules:
    - source: "^//\\s*go:generate\\s"
      linters: [ lll ]
    - source: "(noinspection|TODO)"
      linters: [ godot ]
    - source: "//noinspection"
      linters: [ gocritic ]
    - source: "^\\s+if _, ok := err\\.\\([^.]+\\.InternalError\\); ok {"
      linters: [ errorlint ]
    - path: "_test\\.go"
      linters:
        - bodyclose
        - dupl
        - funlen
        - goconst
        - gosec
        - noctx
        - wrapcheck
        - nilnil

Go environment

$ go version && go env
go version go1.20.1 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jphillips/.cache/go-build"
GOENV="/home/jphillips/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/jphillips/go/pkg/mod"
GONOPROXY="source.foo.com,gitlab.com/foo"
GONOSUMDB="source.foo.com,gitlab.com/foo"
GOOS="linux"
GOPATH="/home/jphillips/go"
GOPRIVATE="source.foo.com,gitlab.com/foo"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jphillips/devel/foo/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2925188725=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

golangci-lint runs as expected without the --new-from-rev HEAD flag specified

Code example or link to a public repository

n/a

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[ldez]

Related to #3713

[crockeo]

I worked through building a minimal reproduction with a coworker of mine (attached below in the details block). Notably because of pre-commit semantics it won't trigger unless you've at least staged files for commit. For the repro below you have to:

git init
git add .
# continue testing...

Some exploration w/ pre-commit config on my side showed:

• Adding require_serial: true to the hook config doesn't fix the typechecking error but it does fix the parallel golangci-lint is running error.
• Removing --new-from-rev HEAD from entry doesn't seem to change anything.
• Removing pass_filenames: true seems to fix the issue in all cases that I've seen.
  • Note: You can also keep pass_filenames: true and disable the typecheck linter to fix the issue.

I hope this is helpful, and thank you for your work on golangci-lint! 🙏

Repro set

• .golangci.yml

  linters:
    disable-all: true
    enable:
      - typecheck

• .pre-commit-config.yaml

  repos:
  -   repo: https://github.com/golangci/golangci-lint
      rev: v1.52.1
      hooks:
      -   id: golangci-lint

• go.mod

  module github.com/crockeo/minimal-repro
  
  go 1.20
  

• main.go

  package main
  
  import (
  	"github.com/crockeo/minimal-repro/folder1"
  	"github.com/crockeo/minimal-repro/folder2"
  )
  
  func main() {
  	folder1.Something()
  	folder2.Something()
  }

• folder1/pkg.go

  package folder1
  
  import "github.com/crockeo/minimal-repro/folder2"
  
  func Something() {
  	folder2.Something()
  }

• folder2/pkg.go

  package folder2
  
  func Something() {}

[ldez]

I updated the PR to add pass_filenames: true

Note: typecheck is not a real linter it's just a way to parse/display "compilation" and linters errors (linter reports are not errors).
It cannot be disabled because of that.

[crockeo]

Thank you for the clarification, and thank you even more for the fix!