Skip to content

Shadowed Variable Checking is Broken with Default Settings #1008

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
3 tasks done
Zamiell opened this issue Mar 30, 2020 · 7 comments
Closed
3 tasks done

Shadowed Variable Checking is Broken with Default Settings #1008

Zamiell opened this issue Mar 30, 2020 · 7 comments
Labels
question Further information is requested

Comments

@Zamiell
Copy link
Contributor

Zamiell commented Mar 30, 2020

Thank you for creating the issue!

  • 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).

Please include the following information:

Version of golangci-lint
$ golangci-lint --version
golangci-lint has version 1.24.0 built from 6fd4383 on 2020-03-15T11:38:01Z
Config file
$ cat .golangci.yml
# none; using the default
Go environment
$ go version && go env
go version go1.14 windows/amd64                                                                                                                                              set GO111MODULE=                                                                                                                                                             set GOARCH=amd64                                                                                                                                                             set GOBIN=                                                                                                                                                                   set GOCACHE=C:\Users\james\AppData\Local\go-build                                                                                                                            set GOENV=C:\Users\james\AppData\Roaming\go\env                                                                                                                              set GOEXE=.exe                                                                                                                                                               set GOFLAGS=                                                                                                                                                                 set GOHOSTARCH=amd64                                                                                                                                                         set GOHOSTOS=windows                                                                                                                                                         set GOINSECURE=                                                                                                                                                              set GONOPROXY=                                                                                                                                                               set GONOSUMDB=                                                                                                                                                               set GOOS=windows                                                                                                                                                             set GOPATH=C:\Users\james\go                                                                                                                                                 set GOPRIVATE=                                                                                                                                                               set GOPROXY=https://proxy.golang.org,direct                                                                                                                                  set GOROOT=c:\go                                                                                                                                                             set GOSUMDB=sum.golang.org                                                                                                                                                   set GOTMPDIR=                                                                                                                                                                set GOTOOLDIR=c:\go\pkg\tool\windows_amd64                                                                                                                                   set GCCGO=gccgo                                                                                                                                                              set AR=ar                                                                                                                                                                    set CC=gcc                                                                                                                                                                   set CXX=g++                                                                                                                                                                  set CGO_ENABLED=1                                                                                                                                                            set GOMOD=                                                                                                                                                                   set CGO_CFLAGS=-g -O2                                                                                                                                                        set CGO_CPPFLAGS=                                                                                                                                                            set CGO_CXXFLAGS=-g -O2                                                                                                                                                      set CGO_FFLAGS=-g -O2                                                                                                                                                        set CGO_LDFLAGS=-g -O2                                                                                                                                                       set PKG_CONFIG=pkg-config                                                                                                                                                    set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\james\AppData\Local\Temp\go-build017672494=/tmp/go-build -gno-record-gcc-switches
--


Verbose output of running
$ golangci-lint run -v
level=info msg="[config_reader] Config search paths: [./ C:\\Users\\james\\test C:\\Users\\james C:\\Users C:\\]"                                                            level=info msg="[lintersdb] Active 10 linters: [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]"                             level=info msg="[lintersdb] Active 10 linters: [deadcode errcheck gosimple govet ineffassign staticcheck structcheck typecheck unused varcheck]"                             level=info msg="[loader] Go packages loading at mode 575 (compiled_files\|deps\|exports_file\|imports\|name\|files\|types_sizes) took 280.3909ms"                                  level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 0s"                                                                                                  level=info msg="[runner/goanalysis_metalinter/goanalysis] analyzers took 0s with no stages"                                                                                  level=info msg="[runner/unused/goanalysis] analyzers took 0s with no stages"                                                                                                 level=info msg="[runner] processing took 0s with stages: autogenerated_exclude: 0s, max_per_file_from_linter: 0s, skip_files: 0s, exclude: 0s, path_shortener: 0s, diff: 0s,  max_same_issues: 0s, max_from_linter: 0s, cgo: 0s, filename_unadjuster: 0s, identifier_marker: 0s, uniq_by_line: 0s, source_code: 0s, path_prettifier: 0s, skip_dirs: 0s, e xclude-rules: 0s, nolint: 0s"                                                                                                                                                level=info msg="[runner] linters took 26.9292ms with stages: goanalysis_metalinter: 25.9318ms, unused: 997.4µs"                                                              level=info msg="File cache stats: 0 entries of total size 0B"                                                                                                                level=info msg="Memory: 5 samples, avg is 22.2MB, max is 25.7MB"                                                                                                             level=info msg="Execution took 351.1888ms"
--


Description of the Problem

I'm using the following "main.go" file:

package main

import "fmt"

func main() {
	a := 1
	fmt.Println(a)
	if true {
		a := 2
		fmt.Println(a)
	}
}

Question 1 - Obviously, I am shadowing a variable in the above code snippet. If I am not mistaken, this should be caught by govet. So something seems broken, because according to the documentation in the README.md file, the "vetshadow" linter is supposed to be enabled by default. What gives?

Question 2 - Poking around in the default options, I noticed that it has the following specification for govet:

  govet:
    # report about shadowed variables
    check-shadowing: true

    # settings per analyzer
    settings:
      printf: # analyzer name, run `go tool vet help` to see all analyzers
        funcs: # run `go tool vet help printf` to see available settings for `printf` analyzer
          - (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

    # enable or disable analyzers by name
    enable:
      - atomicalign
    enable-all: false
    disable:
      - shadow
    disable-all: false

Can someone explain why does it simultaneously says check-shadowing: true and disable: -shadow? Is that a bug? If it is not a bug, can we add a comment or something and explain what the heck is going on there?

@Zamiell
Copy link
Contributor Author

Zamiell commented Apr 3, 2020

Specifically, it looks like this line is lying to the userbase:
https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml#L153
Because the default value appears to be false.

@torbjornvatn
Copy link

I have the same issue, and it doesn't work to enable it explicitly in my own .golangci.yml file either.

But from what I can see @Zamiell the -shadow flag has been removed from go vet and moved into a separate tool that has to be installed. golang/go#29260 (comment)
So I guess golangci-lint has to install that tool to make this work

@ldez
Copy link
Member

ldez commented Jan 9, 2021

Can someone explain why does it simultaneously says check-shadowing: true and disable: -shadow? Is that a bug? If it is not a bug, can we add a comment or something and explain what the heck is going on there?

@Zamiel The file https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml does not contain the default options, it's just an example of all options, so there is no meaning for this configuration.


@torbjornvatn the shadow linter is already supported: #697


The linter seems to work as expected:

no problem
package main

import "fmt"

func main() {
	a := 1
	fmt.Println(a)
	if true {
		a := 2
		fmt.Println(a)
	}
}
problem
package main

import "fmt"

func main() {
	a := 1
	if true {
		a := 2
		fmt.Println(a)
	}
	fmt.Println(a)
}
cmd/foo.go:8:3: shadow: declaration of "a" shadows declaration at line 6 (govet)
                a := 2
                ^
exit status 1

@ldez ldez added the question Further information is requested label Jan 9, 2021
@butuzov
Copy link
Member

butuzov commented Jan 12, 2021

@ldez I believe it would be beneficial to remove "check-shadowing:" setting as misleading for newcomers.

I just run into it, while checking why with enabled check-shadowing we have this issue in code. I can pick it (just in case).

found my logic error., I was expecting next code to trigger shadowing warning.

package testdataabstract

import (
	"context"
	ctxpkg "context"
	"fmt"
)

func ReadFromContext(context context.Context) context.Context {
	return ctxpkg.WithValue(context, "key", "val")
}

@ldez ldez closed this as completed Jan 15, 2021
@Zamiell
Copy link
Contributor Author

Zamiell commented Jan 15, 2021

@ldez

  1. Why was this closed? The issue does not seem to be resolved.
  2. More specifically, can you answer: is this line a bug or not?
    https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml#L270
  3. If it is not a bug, then why does enabling it not do anything?
  4. Why isn't shadow variable checking enabled by default?

@ldez
Copy link
Member

ldez commented Jan 15, 2021

I was thinking that I already answered your questions #1008 (comment)

  1. Why was this closed? The issue does not seem to be resolved.

because there is no bug, the behavior is expected.

  1. More specifically, can you answer: is this line a bug or not?
    https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml#L270

It's not a bug, .golangci.example.yml does not contain the default options, it's just an example of all options, so there is no meaning for this configuration.

  1. If it is not a bug, then why does enabling it not do anything?

because in your example there is no shadowing.

no shadow
package main

import "fmt"

func main() {
	a := 1
	fmt.Println(a)
	if true {
		a := 2
		fmt.Println(a)
	}
}
shadow
package main

import "fmt"

func main() {
	a := 1
	if true {
		a := 2
		fmt.Println(a)
	}
	fmt.Println(a)
}
cmd/foo.go:8:3: shadow: declaration of "a" shadows declaration at line 6 (govet)
                a := 2
                ^
exit status 1
  1. Why isn't shadow variable checking enabled by default?

Because it's not enabled by default in govet.

@Zamiell
Copy link
Contributor Author

Zamiell commented Jan 15, 2021

Ah, ok, I see. Thanks for the quick reply Idez.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants