Go: cmd/compile: diagnose unused variable even in closure

[rogpeppe]

package main

func main() {
    i := 0
    go func() {
        i = 1
    }()
}

This compiles without errors.
I would expect this to give an "i declared and not used" error.

[rsc]

Comment 1:

_Labels changed: added priority-later, removed priority-triage._

_Owner changed to [email protected]._

_Status changed to Accepted._

[rsc]

Comment 2:

_Labels changed: added go1.1maybe._

[rsc]

Comment 3:

[The time for maybe has passed.]

_Labels changed: removed go1.1maybe._

[rsc]

Comment 4:

_Labels changed: added go1.2maybe._

[rsc]

Comment 5:

_Labels changed: added feature._

[robpike]

Comment 6:

Not likely to happen in 1.2.

_Labels changed: added go1.3maybe, removed go1.2maybe._

[robpike]

Comment 7:

_Labels changed: removed go1.3maybe._

[remyoudompheng]

Comment 8:

_Issue #6414 has been merged into this issue._

[rsc]

Comment 9:

_Labels changed: added go1.3maybe._

[rsc]

Comment 10:

_Labels changed: removed feature._

[rsc]

Comment 11:

_Labels changed: added release-none, removed go1.3maybe._

[rsc]

Comment 12:

_Labels changed: added repo-main._

[zeebo]

I ran into this problem today when code that was compiling fine under cmd/gc was rejected during godoc's pointer analysis by go/types. It would be nice to have this fixed so that I didn't have to worry and find all the spots we do this in our code base and keep track of if it's happening during code review.

[gopherbot]

Change https://golang.org/cl/101815 mentions this issue: cmd/trace: remove unused variable in tests

[gopherbot]

Change https://golang.org/cl/121455 mentions this issue: doc: document new vet behaviour for typechecking failures

[odeke-em]

Without having fully dug into this I am hypothesizing this might just be a consequence of us taking the address of higher scope variables to make a pointer then technically "assigned" to it
so it perhaps became the equivalent of

var i int
*(&i) = 1

thus when used in the closure got rewritten as

var i int
go func(i *int) {
    *i = 1
}()

which if true would then mean that a fix for this would involve specially marking as used scoped-by-manually-address-of'd variables that were previously just values in their original scope
so in pseudo code

in closure or inner scope, for &prefixedVariables from an outer scope {
    on assignment, markAsUsed iff:
        Originally a pointer in outer scope
}

Kindly paging @griesemer @randall77 @mdempsky for discussion and perhaps to help me check my hypothesis and then I can mail out a fix if plausible.

[randall77]

Yes, we're effectively doing &i when building the closure, which defeats the unused variable analysis.
I'm not sure what the fix would be. We'd need to keep track of, for every variable whose address is taken, whether that address was used in a read or a write (or unknown). It sounds tricky.

[griesemer]

It needs to be done independently of (the implementation detail) whether an address was taken. go/types does this right, but it also does it earlier, during type-checking.

Not urgent.

[ianlancetaylor]

It's not urgent, but it is an ongoing pain point, because different compilers behave differently. That causes people to write code that works with one compiler and complain when it fails with a different one. So it really ought to be fixed.

[sigxcpu76]

vet detects this but compiler is happy. internal_response is never used.

func (s *AnaServer) VisualizationEvent(ctx context.Context, in *pb.VisualizationEventReq) (*pb.VisualizationEventRes, error) {
...
    internal_response := &ana.VisualizationEventRes{}

...
    if err := customcopier.CopyRecommendedDashPageReq(request.DashPage, in.GetDashPage()); err != nil {
        return nil, err
    }

    if err := s.AnaRpc(func(client ana.GateClient) error {
        internal_response, err = client.VisualizationEvent(ctx, request)
        return err
    }); err != nil {
        return nil, err
    }

    return &pb.VisualizationEventRes{}, nil
}

[gopherbot]

Change https://golang.org/cl/238317 mentions this issue: cmd/compile: recognize unused variables even when captured

[gopherbot]

Change https://golang.org/cl/238538 mentions this issue: cmd/compile: maintain legacy "declared but not used" behavior

[gopherbot]

Change https://golang.org/cl/238537 mentions this issue: cmd/compile: refactor "declared but not used" diagnostic

[mdempsky]

I have CLs uploaded to fix this for Go 1.16 (CL 238537, CL 238317).

@ianlancetaylor points out this might break programs that currently build, and so maybe we should only apply impose the new, stricter behavior when -lang=go1.16 is used (e.g., go 1.16 in a go.mod file). I've prepared CL 238538 to implement this conditional behavior.

gccgo, go/types, and cmd/vet all diagnose the original test case as an error. I'm hopeful that we can avoid introducing conditional behavior in cmd/compile. But if we need to make it conditional, I think that's reasonable.

Relatedly, if all major Go compilers/type-checkers now require all declared variables to be used, we should consider changing the "implementation restriction" into a plain restriction (like how directly imported packages have to be used).

[mdempsky]

Any comments on whether the stricter behavior should be decided by -lang=go1.16 or just enabled by default (i.e., consistent with gccgo, go/types, and cmd/vet)?

[ianlancetaylor]

If my program pulls in third party packages as many programs do, and those third party packages don't use vet, then it's possible that my program will suddenly stop building if we make this change, and that the only way for me to fix the build will be to edit third party packages. That seems undesirable. Therefore, unfortunately, I think we do have to condition this on a language flag.

In effect I think we are removing a language feature as described at https://go.googlesource.com/proposal/+/refs/heads/master/design/28221-go2-transitions.md#language-removals, the language feature in this case being something like "the ability to only set a variable and never use it as long as one of the sets is in a closure." Since we are removing a feature, we should use a language flag.

Sorry.

Just my opinion, of course, happy to hear other opinions.

[mdempsky]

If my program pulls in third party packages as many programs do, and those third party packages don't use vet,

I believe it's even more restrictive than that in practice. go test automatically runs go vet, and go vet must be able to process all recursive dependencies. (It appears to have essentially its own parallel build process, with intermediate analysis artifacts instead of compiled code.) So the risk of breakage here is limited to packages that don't use go vet or go test and don't have any users that use them either.

Test setup w/ package example.com/a that uses this anti-feature, and example.com/b that depends on it:

$ cat a/go.mod
module example.com/a
go 1.16

$ cat a/a.go
package a
func f() {
    x := 0
    func() {
        x = 1
    }()
}

$ cat b/go.mod
module example.com/b
go 1.16
replace example.com/a => ../a
require example.com/a v0.0.0-00010101000000-000000000000

$ cat b/b.go
package b
import _ "example.com/a"

$ cat b/b_test.go
package b_test
import "testing"
func TestFoo(t *testing.T) {}

Here go vet and go test report failures (exit code 2) for package example.com/b because go/types rejects example.com/a:

$ ( cd b; go vet; echo $? )
# example.com/a
vet: ../a/a.go:4:2: x declared but not used
2

$ ( cd b; go test; echo $? )
# example.com/a
vet: ../a/a.go:4:2: x declared but not used
PASS
ok      example.com/b   0.008s
2

In effect I think we are removing a language feature as described at https://go.googlesource.com/proposal/+/refs/heads/master/design/28221-go2-transitions.md#language-removals, the language feature in this case being something like "the ability to only set a variable and never use it as long as one of the sets is in a closure." Since we are removing a feature, we should use a language flag.

Pedantically, the general language feature ("ability to only set a variable and never use it") is already optional, and not supported by most implementations (including cmd/compile, most of the time). I think that distinguishes it from integer-to-string conversions like discussed in that section, as those conversions are a mandatory feature.

I would argue the example from https://go.googlesource.com/proposal/+/refs/heads/master/design/28221-go2-transitions.md#language-redefinitions is more applicable: "For example, in Go 1.1 the size of the type int on 64-bit hosts changed from 32 bits to 64 bits. This change was relatively harmless, as the language does not specify the exact size of int. Potentially, though, some Go 1.0 programs continued to compile with Go 1.1 but stopped working."

Similar to not specifying the size of int, the language does not specify whether the feature "the ability to only set a variable and never use it as long as one of the sets is in a closure" is available. I'm sure it would have been frustrating to have a third-party dependency that assumed int was 32 bits and stopped working in Go 1.1, but the proposal seems to conclude this was relatively harmless, nonetheless.

[ianlancetaylor]

Thanks for looking into it. I'll let someone else adjudicate.

[randall77]

I think I'd rather use the go1.16 flag. There's no point in breaking people's programs unnecessarily. Unlike, say, the reflect.SliceHeader issue, not using a variable isn't going to create silent bad behavior.