Skip to content

govet requires go install to work better #87

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
alecthomas opened this issue Jun 12, 2018 · 15 comments
Closed

govet requires go install to work better #87

alecthomas opened this issue Jun 12, 2018 · 15 comments
Labels
bug Something isn't working

Comments

@alecthomas
Copy link
Contributor

I know this report won't be that helpful, but I don't have much information.

I have this configuration:

# Exhaustive list of options here:
# https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml
run:
  deadline: 5m
  tests: true
  skip-dirs:
    - mock # Remove once https://github.com/golangci/golangci-lint/issues/86 is fixed

output:
  print-issued-lines: false

linters:
  enable-all: true
  disable:
    - maligned

linters-settings:
  govet:
    check-shadowing: true
  golint:
    min-confidence: 0
  gocyclo:
    min-complexity: 10
  dupl:
    threshold: 100
  goconst:
    min-len: 2
    min-occurrences: 2
  gocyclo:
    min-complexity: 20

issues:
  max-per-linter: 0
  max-same: 0
  exclude-use-default: false
  exclude:
    # Captured by errcheck.
    - '^(G104|G204):'
    # Very commonly not checked.
    - 'Error return value of .(.*\.Help|.*\.MarkFlagRequired|(os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked'
    # TODO(aat): remove this once docs are added.
    - '.*should have.*comment.*'
    # Weird error only seen on CI...
    - 'internal error: no range for'

Running golangci-lint run locally on my laptop results in the following error:

pkg/provision/iam/policies.go:133:5: declaration of "err" shadows declaration at pkg/provision/iam/policies.go:115 (govet)

But when this runs under CI (on Kochiku), no error is reported. I've run it on CI with -v and it gives the following:

time="2018-06-12T00:27:06Z" level=info msg="Config search paths: [./ /data/app/kochiku-worker/shared/build-partition/cash/squaremason/src/stash.corp.squareup.com/cash/squaremason /data/app/kochiku-worker/shared/build-partition/cash/squaremason/src/stash.corp.squareup.com/cash /data/app/kochiku-worker/shared/build-partition/cash/squaremason/src/stash.corp.squareup.com /data/app/kochiku-worker/shared/build-partition/cash/squaremason/src /data/app/kochiku-worker/shared/build-partition/cash/squaremason /data/app/kochiku-worker/shared/build-partition/cash /data/app/kochiku-worker/shared/build-partition /data/app/kochiku-worker/shared /data/app/kochiku-worker /data/app /data /]"
time="2018-06-12T00:27:06Z" level=info msg="Used config file .golangci.yml"
time="2018-06-12T00:27:06Z" level=info msg="Active 18 linters: [deadcode depguard dupl errcheck gas goconst gocyclo gofmt goimports golint govet ineffassign interfacer megacheck structcheck typecheck unconvert varcheck]"
time="2018-06-12T00:27:06Z" level=info msg="Skipped dirs: [mocks pkg/awsenv/testdata vendor]"
time="2018-06-12T00:27:06Z" level=info msg="Paths resolving took 3.812451ms: [cmd/sqm cmd/sqm/env cmd/sqm/service internal/registry pkg/awsenv pkg/awsenv/iampolicy pkg/k8senv pkg/provision pkg/provision/database pkg/provision/iam pkg/provision/service server]"
time="2018-06-12T00:27:13Z" level=info msg="Program loading took 7.027793144s"
time="2018-06-12T00:27:17Z" level=info msg="SSA repr building took 3.545941115s"
time="2018-06-12T00:27:17Z" level=info msg="worker.3 took 358.10083ms with stages: goimports: 239.360565ms, dupl: 98.248495ms, ineffassign: 16.93739ms, gocyclo: 1.830568ms, unconvert: 1.651006ms"
time="2018-06-12T00:27:17Z" level=info msg="worker.4 took 360.277981ms with stages: golint: 237.566721ms, gofmt: 57.642512ms, gas: 45.870413ms, varcheck: 7.755287ms, deadcode: 3.634114ms, structcheck: 3.598876ms, errcheck: 2.459229ms, goconst: 1.650107ms, typecheck: 948ns"
time="2018-06-12T00:27:18Z" level=info msg="worker.1 took 928.704012ms with stages: interfacer: 790.557348ms, govet: 138.112655ms, depguard: 7.709µs"
time="2018-06-12T00:28:08Z" level=info msg="worker.2 took 51.523991372s with stages: megacheck: 51.523955449s"
time="2018-06-12T00:28:08Z" level=info msg="Workers idle times: #1: 50.595252839s, #3: 51.165207269s, #4: 51.163674384s"
time="2018-06-12T00:28:08Z" level=info msg="processing took 1.665043ms with stages: nolint: 824.966µs, exclude: 571.104µs, path_prettifier: 149.593µs, autogenerated_exclude: 78.38µs, cgo: 13.559µs, max_same_issues: 4.953µs, uniq_by_line: 4.94µs, diff: 4.718µs, max_per_file_from_linter: 4.331µs, skip_files: 4.306µs, max_from_linter: 4.193µs"
Congrats! No issues were found.

The CI gets the latest golangci-lint with:

go get -u github.com/golangci/golangci-lint/cmd/golangci-lint

And I'm using the same version locally.

Sorry I can't be of more help, but I can't get any more information out of golangci-lint or the CI.

@alecthomas
Copy link
Contributor Author

I have a bit more information. I added a manual run of go vet -shadow ./... to the CI, and also ran it locally.

Locally:

$ go vet -shadow ./...
$

CI:

+ go vet -shadow ./...
+ golangci-lint run
client/protocol.go:102:2: declaration of "entity" shadows declaration at client/protocol.go:11 (govet)
store/db.go:242:2: declaration of "entity" shadows declaration at store/db.go:14 (govet)
store/db.go:267:3: declaration of "entity" shadows declaration at store/db.go:14 (govet)
store/fake.go:117:2: declaration of "entity" shadows declaration at store/fake.go:9 (govet)

I'm not sure what's going on. These reports are around a variable shadowing a package import, so they are correct.

I'm not sure why go vet isn't reporting them either :\

@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

hi!

  1. try go install -i ./... (depends on project, maybe go install -i ./cmd/...) first in CI, does it help?
  2. give a feedback please: why don't you consider golangci.com instead of running golangci-lint in custom CI?

@alecthomas
Copy link
Contributor Author

  1. You gave me an idea; I did the opposite. I removed my local pkg dir and got the following:
$ rm -rf $GOPATH/pkg
$ golangci-lint run --fast
cmd/sotw/main.go:13:2: declaration of "entity" shadows declaration at cmd/sotw/main.go:5 (govet)
$ go install ./...
$ golangci-lint run --fast
Congrats! No issues were found.

I don't know why that would be the case...

  1. We can't use any SAAS, including GitHub, Circle, golangci.com, etc.

@jirfag jirfag added the bug Something isn't working label Jun 12, 2018
@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

which version of go do you use?

@alecthomas
Copy link
Contributor Author

I've tried 1.9, 1.9.2, 1.10 and 1.10.2

@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

ok, thank you.
I will try to reproduce, it's a weird bug.

@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

  1. On your local machine is golangci-lint built locally (binary made by go get) or you use prebuilt binary (curl ... | bash -s ...)? And how it's built/installed on CI and which version of go do you use in CI machine?
  2. run locally, please
go version
head -5 cmd/sotw/main.go | tail -1
head -13 cmd/sotw/main.go | tail -1
rm -rf $GOPATH/pkg
golangci-lint run --no-config --disable-all -Egovet cmd/sotw/main.go
go vet -shadow ./...
go install -i ./cmd/sotw/
golangci-lint run --no-config --disable-all -Egovet cmd/sotw/main.go
go vet -shadow ./...

@alecthomas
Copy link
Contributor Author

It's built by go get.

$ go version
go version go1.10.2 darwin/amd64
$ head -5 cmd/sotw/main.go | tail -1
	"stash.corp.squareup.com/cash/squaresotw/entity"
$ head -13 cmd/sotw/main.go | tail -1
	entity, err := poller.Poll(entity.ID("arn:aws:eks:us-east-1:934409966526:cluster/eks-us-east-1"))
$ rm -rf $GOPATH/pkg
$ golangci-lint run --no-config --disable-all -Egovet cmd/sotw/main.go
Congrats! No issues were found.
$ golangci-lint run --no-config --disable-all -Egovet
Congrats! No issues were found.
$ go vet -shadow ./...
$ go install -i ./cmd/sotw
$ golangci-lint run --no-config --disable-all -Egovet cmd/sotw/main.go
Congrats! No issues were found.
$ go vet -shadow ./...

So it looks like I can only reproduce the warnings when I am using the configuration file AND have cleared the pkg directory...that is truly baffling.

@alecthomas
Copy link
Contributor Author

I just tried it with the curled version 1.6.1 and I can reproduce it:

$ rm -rf $GOPATH/pkg
$ ./bin/golangci-lint run --fast
cmd/sotw/main.go:13:2: declaration of "entity" shadows declaration at cmd/sotw/main.go:5 (govet)

@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

in environment where you've reproduced it run, please:

go vet -x -v -shadow ./cmd/sotw/main.go
go tool vet -v -shadow ./cmd/sotw/main.go

@jirfag
Copy link
Contributor

jirfag commented Jun 12, 2018

Got it. For go version < 1.10 we need to manually go install project to go vet work properly, it's a known issue. In go 1.10 it was fixed: go vet now rebuilds project on each invocation using build cache. But golangci-lint doesn't do it.

I have 3 choices how to fix it:

  1. Run go install from golangci-lint if we are in fast mode and govet is enabled. It won't slow down go 1.10 builds and can slow down go < 1.10 builds, but will make them correct. The disadvantage is in need of go binary, but it looks ok.
  • In slow mode govet must consume source-loaded data, not from an object files.
  1. Make govet a "slow" linter: it starts to consume source-loaded (not object files loaded) information about types.
  2. Make type information loading incremental: use separate cache for it. It will deprecate --fast and all linters can consume type information from source code.

I can't guarantee that fix will be in a near future. The fastest solution now is to run go install in CI.

@jirfag jirfag changed the title govet reports issues locally but not under CI govet requires go install to work better Jun 12, 2018
@alecthomas
Copy link
Contributor Author

Are you sure that analysis is correct? The example below shows that it reports correctly when there are no compiled artifacts. That seems like the opposite of what you've determined?

Or perhaps it's that it only analyses while it's building, and it's already built it skips it?

$ rm -rf $GOPATH/pkg
$ golangci-lint run --fast
cmd/sotw/main.go:13:2: declaration of "entity" shadows declaration at cmd/sotw/main.go:5 (govet)
$ go install ./...
$ golangci-lint run --fast
Congrats! No issues were found.

@alecthomas
Copy link
Contributor Author

It's also very odd that go vet doesn't work, so maybe it's an upstream bug.

@jirfag
Copy link
Contributor

jirfag commented Jun 13, 2018

analysis by golangci-lint isn't correct in terms of go vet. Govet has the following code:

	// Don't complain if the types differ: that implies the programmer really wants two different things.
	if types.Identical(obj.Type(), shadowed.Type()) {
		f.Badf(ident.Pos(), "declaration of %q shadows declaration at %s", obj.Name(), f.loc(shadowed.Pos()))
	}

In your case entity as the import name has one type, and entity as the variable another type.
But golangci-lint couldn't load types information => types got nil values => this comparison was true.
Also, there is a bug in golangci-lint: original go vet exits and warns if it can't load type data. I will fix it first, and then try to pass type data to go vet.

golangci pushed a commit that referenced this issue Jun 17, 2018
1. Allow govet to work in 2 modes: fast and slow. Default is slow.
In fast mode golangci-lint runs `go install -i` and `go test -i`
for analyzed packages. But it's fast only when:
  - go >= 1.10
  - it's repeated run or $GOPATH/pkg or `go env GOCACHE` is cached
  between CI builds
In slow mode we load program from source code like for another linters
and do it only once for all linters.

3. Patch govet code to warn about any troubles with the type
information. Default behaviour of govet was to hide such warnings.
Fail analysis if there are any troubles with type loading: it will
prevent false-positives and false-negatives from govet.

4. Describe almost all options in .golangci.example.yml and
include it into README. Describe when to use slow or fast mode of govet.

5. Speed up govet: reuse AST parsing: it's already parsed once by
golangci-lint.
For "slow" runs (when we run at least one slow linter) speedup by
not loading type information second time.

6. Improve logging, debug logging
golangci pushed a commit that referenced this issue Jun 18, 2018
1. Allow govet to work in 2 modes: fast and slow. Default is slow.
In fast mode golangci-lint runs `go install -i` and `go test -i`
for analyzed packages. But it's fast only when:
  - go >= 1.10
  - it's repeated run or $GOPATH/pkg or `go env GOCACHE` is cached
  between CI builds
In slow mode we load program from source code like for another linters
and do it only once for all linters.

3. Patch govet code to warn about any troubles with the type
information. Default behaviour of govet was to hide such warnings.
Fail analysis if there are any troubles with type loading: it will
prevent false-positives and false-negatives from govet.

4. Describe almost all options in .golangci.example.yml and
include it into README. Describe when to use slow or fast mode of govet.

5. Speed up govet: reuse AST parsing: it's already parsed once by
golangci-lint.
For "slow" runs (when we run at least one slow linter) speedup by
not loading type information second time.

6. Improve logging, debug logging

7. Fix crash in logging of AST cache warnings (#118)
jirfag added a commit that referenced this issue Jun 18, 2018
1. Allow govet to work in 2 modes: fast and slow. Default is slow.
In fast mode golangci-lint runs `go install -i` and `go test -i`
for analyzed packages. But it's fast only when:
  - go >= 1.10
  - it's repeated run or $GOPATH/pkg or `go env GOCACHE` is cached
  between CI builds
In slow mode we load program from source code like for another linters
and do it only once for all linters.

3. Patch govet code to warn about any troubles with the type
information. Default behaviour of govet was to hide such warnings.
Fail analysis if there are any troubles with type loading: it will
prevent false-positives and false-negatives from govet.

4. Describe almost all options in .golangci.example.yml and
include it into README. Describe when to use slow or fast mode of govet.

5. Speed up govet: reuse AST parsing: it's already parsed once by
golangci-lint.
For "slow" runs (when we run at least one slow linter) speedup by
not loading type information second time.

6. Improve logging, debug logging

7. Fix crash in logging of AST cache warnings (#118)
@alecthomas
Copy link
Contributor Author

This is fixed now I think? I'll close 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

2 participants