You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
cmd/compile: diagnose unused variable even in closure #3059
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.
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)?
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.
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 vetorgo testand 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
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.
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.
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.
Reacted by Zik Aeroh, Sean Liao, Jeff Wendling, Cuong Manh Le, Elias Naur, Tom Thorogood and Oleg Kovalov
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.
Since switching to the types2 type checker in Go 1.18, cmd/compile unconditionally rejects the sample code with "i declared but not used". I think this issue is fixed.
Activity
rsc commentedon Feb 17, 2012
Comment 1:
Labels changed: added priority-later, removed priority-triage.
Owner changed to builder@golang.org.
Status changed to Accepted.
rsc commentedon Sep 12, 2012
Comment 2:
Labels changed: added go1.1maybe.
rsc commentedon Mar 12, 2013
Comment 3:
Labels changed: removed go1.1maybe.
rsc commentedon Jul 30, 2013
Comment 4:
Labels changed: added go1.2maybe.
rsc commentedon Jul 30, 2013
Comment 5:
Labels changed: added feature.
robpike commentedon Aug 16, 2013
Comment 6:
Labels changed: added go1.3maybe, removed go1.2maybe.
robpike commentedon Aug 20, 2013
Comment 7:
Labels changed: removed go1.3maybe.
remyoudompheng commentedon Sep 29, 2013
Comment 8:
Issue #6414 has been merged into this issue.
rsc commentedon Nov 27, 2013
Comment 9:
Labels changed: added go1.3maybe.
rsc commentedon Nov 27, 2013
Comment 10:
Labels changed: removed feature.
rsc commentedon Dec 4, 2013
Comment 11:
Labels changed: added release-none, removed go1.3maybe.
rsc commentedon Dec 4, 2013
Comment 12:
Labels changed: added repo-main.
52 remaining items
mdempsky commentedon Aug 14, 2020
Any comments on whether the stricter behavior should be decided by
-lang=go1.16or just enabled by default (i.e., consistent with gccgo, go/types, and cmd/vet)?ianlancetaylor commentedon 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 commentedon Aug 15, 2020
I believe it's even more restrictive than that in practice.
go testautomatically runsgo vet, andgo vetmust 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 usego vetorgo testand don't have any users that use them either.Test setup w/ package
example.com/athat uses this anti-feature, andexample.com/bthat depends on it:Here
go vetandgo testreport failures (exit code 2) for packageexample.com/bbecausego/typesrejectsexample.com/a: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 assumedintwas 32 bits and stopped working in Go 1.1, but the proposal seems to conclude this was relatively harmless, nonetheless.ianlancetaylor commentedon Aug 15, 2020
Thanks for looking into it. I'll let someone else adjudicate.
randall77 commentedon 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.SliceHeaderissue, not using a variable isn't going to create silent bad behavior.mdempsky commentedon 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 testbehavior 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 commentedon 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: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 commentedon Jun 4, 2021
Change https://golang.org/cl/325030 mentions this issue:
[dev.typeparams] runtime/race: make test compatible with types2[dev.typeparams] runtime/race: make test compatible with types2
Remove unused err
mdempsky commentedon Feb 2, 2023
Since switching to the types2 type checker in Go 1.18, cmd/compile unconditionally rejects the sample code with "i declared but not used". I think this issue is fixed.