Add default exclude for shadowing of err/ctx

[sudo-suhas]

Version of golangci-lint: e17b954

Go environment:

go version && go env

go version go1.10.1 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/suhaskaranth/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/suhaskaranth/installs/go-tools"
GORACE=""
GOROOT="/usr/lib/go-1.10"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.10/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build692454894=/tmp/go-build   -gno-record-gcc-switches"

It is considered idiomatic in Go to use the variable name err for error which could be returned from function invocations in a block of code which could also have shadowing like this:

err := funcA()
if err != nil {
	// ...
}

val, err := funcB()
if err != nil {
	// ...
}

Along the same lines, derived contexts are also named ctx and shadowing is common:

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

{
	ctx, cancel := context.WithCancel(ctx)

	db, err := mysql.NewClient(ctx)
	if err != nil {
		// ...
	}

	db.DoSomething()

	cancel() // close the DB conn of our imaginary pkg
}

This is not an issue if the default settings are used. It only shows up if check-shadowing is enabled for govet. So how about adding a default exclusion for this with declaration of "(err|ctx)" shadows declaration at?

[jirfag]

hi, thank you for the issue!
why do you enable check-shadowing? I got some cases where err shadowing was a bug, I need to think about it

[sudo-suhas]

Not that I have any specific examples now but I have had instances where a bug was hidden behind shadowing.

[jirfag]

in my opinion you should choose: fully disable shadowing checking or don't ignore err/ctx shadowing

[piersy]

@sudo-suhas You can achieve this with the --exclude flag, which exclude certain results based on a regex matching the result text. So for instance to remove notifications about err shadowing you could use.

golangci-lint run --config ci.yml --exclude "shadow: declaration of .err. shadows declaration"

I used .err. here because I had difficulty getting my shell to represent nested double quotes in the exclude declaration.

I'm sure you could do the same for ctx variables as well.

[ernado]

I'm using exclude-rules for this.

[sudo-suhas]

Yes @piersy I had used the config file to achieve this:

[issues]
  # List of regexps of issue texts to exclude, empty list by default.
  # But independently from this option we use default exclude patterns,
  # it can be disabled by `exclude-use-default: false`. To list all
  # excluded by default patterns execute `golangci-lint run --help`
  exclude=[
    'declaration of "(err|ctx)" shadows declaration at',
  ]

[piersy]

@sudo-suhas Thanks for that snippet, its an improvement on using the flag since we can represent the surrounding double quotes easily.

[Garagator3000]

issues:

  exclude-rules:
    - text: 'shadow: declaration of "(err|ctx)" shadows declaration at'
      linters: [ govet ]

relevant in 2022

[fresonn]

@Garagator3000 thanks a lot!

[s3rj1k]

@jirfag Please reconsider your decision, forcing usage of exclude rule seems sub-optimal solution as types data are lost.

[ldez]

The shadowing of errors or context can be a real problem, and it's usually easy to avoid shadowing them.

Quick example:

package main

import (
	"context"
	"fmt"
)

type contextKey string

const (
	importantKey contextKey = "important"
	secretKey    contextKey = "secret"
)

func main() {
	ctx, msg := foo()
	if msg == "a" {
		ctx := context.WithValue(ctx, importantKey, "important")
		noop(ctx)
	}

	fmt.Println(ctx.Value(secretKey)) // got "secret", expected "secret"

	// BUG: importantKey doesn't exist here because of the shadowing
	fmt.Println(ctx.Value(importantKey)) // got <nil>, expected "important"
}

func foo() (context.Context, string) {
	return context.WithValue(context.Background(), secretKey, "secret"), "a"
}

func noop(_ context.Context) {}

https://go.dev/play/p/-BA0n1B2Cpn

So enabling check-shadowing and excluding the shadowing of errors or context is not a good practice, we can neither add this as default nor recommend it.

FYI, Denis will not answer because he is no longer active on the project.

[s3rj1k]

@ldez I fully agree that this can be a problem but for huge codebases it is nearly impossible to fix all the errors, hence a need to somehow configure excludes.

I am fine with this being disabled by default still having the ability to enable this filter in config.