Skip to content

pkg/golinters: maybe do not exclude govet internal analysers #915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
carnott-snap opened this issue Jan 10, 2020 · 5 comments
Closed

pkg/golinters: maybe do not exclude govet internal analysers #915

carnott-snap opened this issue Jan 10, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@carnott-snap
Copy link

It looks like excluding internal analysers, as was done in #763, is causing my config to fail, because linters-settings.govet.settings.printf no longer exists.

Version of golangci-lint
$ golangci-lint --version
golangci-lint has version v1.22.2 built from (unknown, mod sum: "h1:iaihss3Tf6NvZVjun3lHimKSgofPV1+FqE/cbehoiRQ=") on (unknown)
Config file
$ cat .golangci.toml
[linters]
disable-all = true
enable = [ "govet" ]

[linters-settings.govet.settings.printf]
funcs = [
  "warnf",
  "infof",
  "errorf",
  "logf",
  "printf",
  "warningf",
  "fatalf",
  "sprintf",
  "fprintf",
  "panicf",
]
Go environment
$ go version && go env
go version go1.13.6 linux/amd64
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/user/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/.local/share/umake/go/go-lang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/local/share/umake/go/go-lang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build142817576=/tmp/go-build -gno-record-gcc-switches"
Verbose output of running
$ golangci-lint run -v
INFO [config_reader] Used config file .golangci.toml 
INFO [lintersdb] Active 1 linters: [govet]        
INFO [lintersdb] Active 1 linters: [govet]        
INFO [loader] Go packages loading at mode 575 (files|types_sizes|compiled_files|deps|name|exports_file|imports) took 1.819537546s 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 163.287254ms 
WARN [runner] Can't run linter govet: failed to configure analyzers: settings key "printf" must be valid analyzer name, valid analyzers: [asmdecl assign atomic bools buildtag cgocall composites copylocks errorsas httpresponse loopclosure lostcancel nilfunc shift stdmethods structtag tests unmarshal unreachable unsafeptr unusedresult] 
INFO [runner] processing took 4.181µs with stages: max_same_issues: 1.344µs, filename_unadjuster: 435ns, nolint: 349ns, skip_dirs: 341ns, max_from_linter: 247ns, cgo: 168ns, path_prettifier: 168ns, identifier_marker: 149ns, path_shortener: 145ns, source_code: 139ns, diff: 134ns, skip_files: 133ns, autogenerated_exclude: 129ns, exclude: 81ns, uniq_by_line: 79ns, max_per_file_from_linter: 72ns, exclude-rules: 68ns 
INFO [runner] linters took 245.841µs with stages: govet: 161.417µs 
ERRO Running error: failed to configure analyzers: settings key "printf" must be valid analyzer name, valid analyzers: [asmdecl assign atomic bools buildtag cgocall composites copylocks errorsas httpresponse loopclosure lostcancel nilfunc shift stdmethods structtag tests unmarshal unreachable unsafeptr unusedresult] 
INFO Memory: 21 samples, avg is 68.9MB, max is 69.3MB 
INFO Execution took 1.994757397s
@tpounds tpounds added the bug Something isn't working label Jan 12, 2020
@tpounds
Copy link
Contributor

tpounds commented Jan 12, 2020

@ernado Any thoughts since you added the internal analyzer exclusions in #763?

@ernado
Copy link
Member

ernado commented Jan 13, 2020

It is very strange since printf is enabled by default and was not disabled in #763.

func getDefaultAnalyzers() []*analysis.Analyzer {
	return []*analysis.Analyzer{
		asmdecl.Analyzer,
		assign.Analyzer,
		atomic.Analyzer,
		bools.Analyzer,
		buildtag.Analyzer,
		cgocall.Analyzer,
		composite.Analyzer,
		copylock.Analyzer,
		errorsas.Analyzer,
		httpresponse.Analyzer,
		loopclosure.Analyzer,
		lostcancel.Analyzer,
		nilfunc.Analyzer,
		printf.Analyzer,
		shift.Analyzer,
		stdmethods.Analyzer,
		structtag.Analyzer,
		tests.Analyzer,
		unmarshal.Analyzer,
		unreachable.Analyzer,
		unsafeptr.Analyzer,
		unusedresult.Analyzer,
	}
}

It is explicitly checked by TestGovet. Also, we have following configuration in .golangci.yml:

linters-settings:
  govet:
    check-shadowing: true
    settings:
      printf:
        funcs:
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf

So linters-settings.govet.settings.printf should fail our CI, but it doesn't.

@ernado
Copy link
Member

ernado commented Jan 13, 2020

$ golangci-lint run -c .golangci.toml --verbose
INFO [config_reader] Used config file .golangci.toml 
INFO [lintersdb] Active 1 linters: [govet]        
INFO [lintersdb] Active 1 linters: [govet]        
INFO [loader] Go packages loading at mode 575 (files|name|types_sizes|deps|exports_file|imports|compiled_files) took 77.405363ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 8.487417ms 
INFO [runner/govet/goanalysis] analyzers took 0s with no stages 
INFO [runner] processing took 3.634µs with stages: max_same_issues: 1.286µs, autogenerated_exclude: 470ns, nolint: 293ns, skip_dirs: 289ns, max_from_linter: 271ns, max_per_file_from_linter: 123ns, cgo: 120ns, skip_files: 118ns, diff: 116ns, identifier_marker: 115ns, path_prettifier: 112ns, filename_unadjuster: 111ns, exclude: 43ns, source_code: 42ns, exclude-rules: 42ns, uniq_by_line: 42ns, path_shortener: 41ns 
INFO [runner] linters took 25.087378ms with stages: govet: 25.055116ms 
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 3 samples, avg is 69.5MB, max is 69.7MB 
INFO Execution took 115.101269ms

$ cat .golangci.toml 
[linters]
disable-all = true
enable = [ "govet" ]

[linters-settings.govet.settings.printf]
funcs = [
  "warnf",
  "infof",
  "errorf",
  "logf",
  "printf",
  "warningf",
  "fatalf",
  "sprintf",
  "fprintf",
  "panicf",
]

$ golangci-lint --version
golangci-lint has version 1.22.2 built from cb2f8ba on 2019-12-30T19:26:28Z

Actually I can't even reproduce :(

Are you sure that you are using correctly built version of golangci-lint?

@mec07
Copy link

mec07 commented Jan 16, 2020

I had the same issue as @carnott-snap. When I was having the issue I noticed this:

$ golangci-lint --version
golangci-lint has version v1.22.2 built from (unknown, mod sum: "h1:iaihss3Tf6NvZVjun3lHimKSgofPV1+FqE/cbehoiRQ=") on (unknown)

Thanks to @ernado it became apparent that my install (from go get) was not quite right. I've fixed it by following the install instructions (https://github.com/golangci/golangci-lint#install) which have now changed (no more go get...). Now that I am installing it using homebrew I get the following:

$ golangci-lint --version
golangci-lint has version 1.22.2 built from cb2f8ba on 2019-12-30T19:26:28Z

@zliuva
Copy link

zliuva commented Jan 18, 2020

@mec07 thanks to your comment I figured out that the replace directive is what's different from the official build vs. our build (from go.mod).

after adding the same line replace golang.org/x/tools => github.com/golangci/tools v0.0.0-20190915081525-6aa350649b1c to our go.mod golangci-lint starts to work again.

however, it seems to me that the real issue is golantci-lint hasn't been tested with newer versions of the upstream x/tools but rather is pinned to an older forked commit. (if the divergence is intentional, should we consider depending on the forked golangci/tools package directly instead of using x/tools then overrides it?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants