Show warning, when overwriting variable

[guettli]

Is your feature request related to a problem? Please describe.

I wrote that code, and I did not understand why path was empty, although findDev() returns a non-empty path:

func getDeviceFromPath(path string) (*evdev.InputDevice, error) {
	if path == "" {
		path, err := findDev()   // "path" of outer block gets overwritten
		if err != nil {
			return nil, err
		}
		fmt.Printf("Using device %q\n", path)
	}
	sourceDev, err := evdev.Open(path)

Describe the solution you'd like

I would like to see a warning in vscode, when I overwrite a variable from the outer scope.

Describe alternatives you've considered

I could use external linters, but I think that should be available "out of the box".

[h9jiang]

Hi guettli,

Based on the go spec,

An identifier declared in a block may be redeclared in an inner block. While the identifier of the inner declaration is in scope, it denotes the entity declared by the inner declaration.

In this case, path got re-declared so the value returns from findDev() got assigned to path declare in the inner block (the if block).

I would like to see a warning in vscode, when I overwrite a variable from the outer scope.

If I understand your quest, you are looking for a warning when you re-declare a variable in the inner block. In this case, show a warning in line 3 path that tell the user this path is re-declaring in the inner scope (because there is another path in the outer block).

@findleyr to see if this is something worth implementing from the gopls side.

[firelizzard18]

@guettli redeclaring a variable in an inner scope is a pretty normal thing to do in Go. Given the Go spec and the language's approach to this situation I think an external linter is more appropriate than gopls itself issuing a warning for all inner scope redeclarations. That being said I think this particular scenario is well suited for go vet to the point where I'm almost surprised it isn't already a warning. I suggest submitting a proposal in the go repo to add a go vet analyzer that:

1. Detects an operation that returns two values;
2. Where the second value is either error or bool;
3. And the first value redeclares an existing variable.

While I agree with the language designers that redeclaration in general should not be considered a problem, I've been tripped up by this specific scenario many times.

[guettli]

@firelizzard18 thank you for your feedback

I created an issue in the Go repo for that: golang/go#71555

[h9jiang]

Hi guettli

I find this issue very similar to your request here. I wonder if this is something you are looking for. #270

"gopls": {
        "ui.diagnostic.analyses": {
            "shadow": true
        },
},

I give it a try with function below and variable err and path have shadow warnings.

[guettli]

@h9jiang yes, this looks good. Thank you for the link. Nevertheless I think this should be enabled by default.

[findleyr]

@guettli the shadow analyzer is far too noisy to be enabled by default... though we have recently codified our definitions of diagnostic severity, and perhaps this makes sense as a 'hint' diagnostic.

Let's see how folks respond to the new 'modernizer' hints; if folks find those to be too noisy, they will certainly find the shadow analyzer to be noisy. CC @adonovan