Skip to content
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

cmd/compile: diagnose unused variable even in closure #3059

Open
rogpeppe opened this issue Feb 17, 2012 · 32 comments
Open

cmd/compile: diagnose unused variable even in closure #3059

rogpeppe opened this issue Feb 17, 2012 · 32 comments

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Feb 17, 2012

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
Copy link
Contributor

@rsc rsc commented Feb 17, 2012

Comment 1:

Labels changed: added priority-later, removed priority-triage.

Owner changed to builder@golang.org.

Status changed to Accepted.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 12, 2012

Comment 2:

Labels changed: added go1.1maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2013

Comment 3:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 4:

Labels changed: added go1.2maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 5:

Labels changed: added feature.

@robpike
Copy link
Contributor

@robpike robpike commented Aug 16, 2013

Comment 6:

Not likely to happen in 1.2.

Labels changed: added go1.3maybe, removed go1.2maybe.

@robpike
Copy link
Contributor

@robpike robpike commented Aug 20, 2013

Comment 7:

Labels changed: removed go1.3maybe.

@remyoudompheng
Copy link
Contributor

@remyoudompheng remyoudompheng commented Sep 29, 2013

Comment 8:

Issue #6414 has been merged into this issue.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 9:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 10:

Labels changed: removed feature.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 11:

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

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 12:

Labels changed: added repo-main.

@zeebo
Copy link
Contributor

@zeebo zeebo commented Dec 19, 2014

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.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/gc: diagnose unused variable even in closure cmd/compile: diagnose unused variable even in closure Jun 8, 2015
mwhudson added a commit to mwhudson/juju that referenced this issue Dec 14, 2015
…w gccgo

gccgo (in xenial at least) is stricter than gc when it comes to a certain
class of declared but not used variable, see:

    golang/go#6415
    golang/go#3059
jujubot added a commit to juju/juju that referenced this issue Dec 15, 2015
fix api/metricsadder & apiserver/environment test compilation with ne…

…w gccgo

gccgo (in xenial at least) is stricter than gc when it comes to a certain
class of declared but not used variable, see:

    golang/go#6415
    golang/go#3059
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 21, 2018

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

gopherbot pushed a commit that referenced this issue Mar 21, 2018
Unused variables in closures are currently not diagnosed by the
compiler (this is Issue #3059), while go/types catches them.

One unused variable in the cmd/trace tests is causing the go/types
test that typechecks the whole standard library to fail:

  FAIL: TestStdlib (8.05s)
    stdlib_test.go:223: cmd/trace/annotations_test.go:241:6: gcTime
    declared but not used
  FAIL

Remove it.

Updates #24464

Change-Id: I0f1b9db6ae1f0130616ee649bdbfdc91e38d2184
Reviewed-on: https://go-review.googlesource.com/101815
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
@bradfitz bradfitz removed this from the Go1.11 milestone May 18, 2018
@bradfitz bradfitz added this to the Go1.12 milestone May 18, 2018
@gopherbot gopherbot removed this from the Go1.12 milestone May 23, 2018
@gopherbot gopherbot added this to the Unplanned milestone May 23, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 28, 2018

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

gopherbot pushed a commit that referenced this issue Jul 17, 2018
Since Go1.10, go test runs vet on the tests before executing them.

Moreover, the vet tool typechecks the package under analysis with
go/types before running. In Go1.10, a typechecking failure just caused
a warning to be printed. In Go1.11, a typechecking failure will cause
vet to exit with a fatal error (see Issue #21287).

This means that starting with Go1.11, tests that don't typecheck will
fail immediately. This would not normally be an issue, since a test
that doesn't typecheck shouldn't even compile, and it should already
be broken.

Unfortunately, there's a bug in gc that makes it accept programs with
unused variables inside a closure (Issue #3059). This means that a
test with an unused variable inside a closure, that compiled and
passed in Go1.10, will fail in the typechecking step of vet starting
with Go1.11.

Explain this in the 1.11 release notes.

Fixes #26109

Change-Id: I970c1033ab6bc985d8c64bd24f56e854af155f96
Reviewed-on: https://go-review.googlesource.com/121455
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 28, 2018

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
Copy link
Contributor

@randall77 randall77 commented Oct 29, 2018

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
Copy link
Contributor

@griesemer griesemer commented Oct 29, 2018

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2018

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
Copy link

@sigxcpu76 sigxcpu76 commented Jun 16, 2020

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
}

@mdempsky mdempsky self-assigned this Jun 16, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 17, 2020

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

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 18, 2020

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
Copy link
Member

@mdempsky mdempsky commented Aug 14, 2020

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 15, 2020

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
Copy link
Member

@mdempsky mdempsky commented Aug 15, 2020

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 15, 2020

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

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 16, 2020

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.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 21, 2021

We're running into this on dev.typeparams, because the race detector has some test code that only compiles with cmd/compile because of this bug. (The tests are obviously trivially fixable with just three lines.)

I'm still of the opinion that 0 real-world programs depend on this today, because of the go test behavior I pointed out above; and that if any code does depend on it, that it's not guaranteed by the Go 1 compat guarantee anyway, which says "Unspecified behavior. The Go specification tries to be explicit about most properties of the language, but there are some aspects that are undefined. Programs that depend on such unspecified behavior may break in future releases."

We routinely fix compiler bugs without guaranteeing backwards compatibility. For example, this program builds with Go 1.16 and prints "1", due to a compiler bug that fails to detect overflow from the unary ^ operator: https://play.golang.org/p/elofakyGeMh. I've fixed this bug on dev.regabi and the program is now rejected with a "constant bitwise complement overflow" error.

If we need a language flag to control taking away the "ability to only set a variable and never use it as long as one of the sets is in a closure" feature, do we need one to control taking away the "ability to construct 513-bit integers by using bitwise complement to bypass overflow detection" feature too?

If #23116 hadn't been fixed until after the introduction of -lang, would it too have required backwards compatibility for the "ability to declare guarded type switches without any cases" feature?

What about #28085? If gccgo starts rejecting case 3 to match cmd/compile and go/types, would it continue supporting the "ability to use duplicate constant values in interface-typed switch statements" feature when compiled with -lang=go1.16?

Or order of evaluation? This program prints 0 with cmd/compile. If we were to modify SSA construction so it instead prints 1 like gccgo, will we continue supporting the "slice expressions are handled like function calls for order-of-evaluation purposes" feature for backwards compatibility?

Where does it end?

I don't think we should be bound to maintain particular, unspecified behavior. I think we specify expected and allowable language behavior for a reason.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 22, 2021

The program below relies on cmd/compile's historical "ability to assume pointers to zero-size objects allocated in and returned by functions that contain for loops will always compare equal" feature, which has been supported since at least Go 1.1. However, with Go 1.16 beta 1, even with -gcflags=all=-lang=go1.15, it now silently compiles to a program that panics instead:

package main

func main() {
	if f() != f() {
		panic("FAIL")
	}
}

func f() *[0]byte {
	for {
		return new([0]byte)
	}
}

Should we file a release-blocker issue because Go 1.16 will break programs that depend on this behavior?

I argue no. The Go spec says "Pointers to distinct zero-size variables may or may not be equal." and consistent with that, we've allowed whether any particular such comparison compares equal or not to change over time. And notably, this change is silent, arbitrary, and largely unpredictable. It's also a recurring source of user issue reports (e.g., #43755 was filed just 4 days ago; see https://github.com/golang/go/issues?q=is%3Aissue+%22zero-size+variables%22 for past instances). There's not even a vet warning that these comparisons are unspecified.

Meanwhile, the Go spec also says "A compiler may make it illegal to declare a variable inside a function body if the variable is never used." And to contrast with the above, presumably acceptable change in pointer-comparison behavior, the change in question for this issue would align cmd/compile's behavior with gccgo and go/types' existing, long-term behavior (i.e., wholly predictable and understandable), and programs that are impacted (whose existence I still doubt) will fail loudly rather than silently.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 4, 2021

Change https://golang.org/cl/325030 mentions this issue: [dev.typeparams] runtime/race: make test compatible with types2

gopherbot pushed a commit that referenced this issue Jun 5, 2021
types2 correctly distinguishes variable assignment from use even
within function literals. Whatever the outcome of #3059, the test
cases in runtime/race need to be fixed to accomodate that.

Change-Id: Ibe3547f07b681ff41225caabaf050872a48c98d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/325030
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet