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

Jump to bottom

cmd/compile: gc inconsistent about reporting "unused variable" errors #8560

Closed
adonovan opened this issue Aug 20, 2014 · 12 comments
Closed

cmd/compile: gc inconsistent about reporting "unused variable" errors #8560

adonovan opened this issue Aug 20, 2014 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@adonovan
Copy link
Member 

For this program:
  http://play.golang.org/p/BzMO7Vt3uy
  switch x := interface{}(nil).(type) {}
go/types reports an error but gc does not.

I realize the spec does not say what should happen here, but the fact of this
implementation-specific behaviour is that we have two dialects of Go whose tools reject
each others' programs.

We should probably make the tools consistent one way or another.
I would also argue in favour of removing this hole in the spec.
@griesemer
Copy link
Contributor

Comment 1:

gccgo also complains about this as an unused variable:
$ gccgo x.go
x.go:4:9: error: 'x' declared and not used
  switch x := interface{}(nil).(type) {
         ^
The spec makes this an implementation restriction, but go/types and gccgo agree with how
the implementation restriction should be applied in this case. Making this a cmd/gc bug.

Labels changed: added release-none, repo-main.

Status changed to Accepted.

@adg
Copy link
Contributor

adg commented Nov 6, 2014

Comment 2:

This program should generate the error "p declared but not used":
package main
func main() {
        p := true
        func() {
                p = true
        }()
}
If you remove the closure, the compiler raises the error.

@ianlancetaylor ianlancetaylor mentioned this issue Apr 6, 2015
Closed
@glycerine glycerine mentioned this issue Apr 6, 2015
Closed
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@alandonovan alandonovan mentioned this issue Jun 2, 2015
Closed
@gopherbot
Copy link

CL https://golang.org/cl/10757 mentions this issue.

alandonovan pushed a commit that referenced this issue Jun 5, 2015
@adonovan
runtime: add blank assignment to defeat "declared but not used" error…
232331f 
… from go/types

gc should ideally consider this an error too; see #8560.

Change-Id: Ieee71c4ecaff493d7f83e15ba8c8a04ee90a4cf1
Reviewed-on: https://go-review.googlesource.com/10757
Reviewed-by: Robert Griesemer <gri@golang.org>
@rsc rsc changed the title cmd/gc: gc inconsistent about reporting "unused variable" errors cmd/compile: gc inconsistent about reporting "unused variable" errors Jun 8, 2015
@alandonovan alandonovan mentioned this issue Mar 7, 2016
Closed
@rsc
Copy link
Contributor

rsc commented Mar 7, 2016

This is in milestone Unplanned. By my assignment should I infer you are requesting we reprioritize it? Normally Unplanned things have no assignee.

@alandonovan
Copy link
Contributor

Yes, that's what I meant. (Sorry if that was unclear or seemed passive-aggressive.)

@rsc rsc removed their assignment Mar 7, 2016
@rsc rsc modified the milestones: Go1.7, Unplanned Mar 7, 2016
@rsc
Copy link
Contributor

rsc commented Mar 7, 2016

OK, reprioritized to Go 1.7, but that's still no guarantee.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 17, 2016
@gopherbot
Copy link

CL https://golang.org/cl/23528 mentions this issue.

gopherbot pushed a commit that referenced this issue Jun 1, 2016
@griesemer
crypto/tls: remove unused variable in benchmark code
fe62a9e 
This fixes `go test go/types`.

https://golang.org/cl/23487/ introduced this code which contains
two unused variables (declared and assigned to, but never read).
cmd/compile doesn't report the error due open issue #8560 (the
variables are assigned to in a closure), but go/types does. The
build bot only runs go/types tests in -short mode (which doesn't
typecheck the std lib), hence this doesn't show up on the dashboard
either.

We cannot call b.Fatal and friends in the goroutine. Communicating
the error to the invoking function requires a channel or a mutex.
Unless the channel/sycnhronized variable is tested in each iteration
that follows, the iteration blocks if there's a failure. Testing in
each iteration may affect benchmark times.

One could use a time-out but that time depends on the underlying system.
Panicking seems good enough in this unlikely case; better than hanging
or affecting benchmark times.

Change-Id: Idce1172da8058e580fa3b3e398825b0eb4316325
Reviewed-on: https://go-review.googlesource.com/23528
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@odeke-em
Copy link
Member

odeke-em commented Aug 5, 2016

@adg your report in #8560 (comment) was a bug that was already reported at #3059, in case you are interested in tracking that case.

@jim-minter
Copy link

Out of interest, are there any known workarounds for the gorename issue #14596 (short of not using closures in this way across a codebase) that can be used to enable use of gorename in codebases such as that before this issue is resolved?

rmmh pushed a commit to rmmh/kubernetes that referenced this issue Feb 27, 2018
Remove unused variables (only assigned to) from test code.
e04b91f 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560
k8s-publishing-bot pushed a commit to kubernetes/apiserver that referenced this issue Feb 28, 2018
@k8s-publishing-bot
Remove unused variables (only assigned to) from test code.
bbfe695 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560

Kubernetes-commit: e04b91facf180c17557a44e8e462858ea2936301
k8s-publishing-bot pushed a commit to kubernetes/apiserver that referenced this issue Feb 28, 2018
@k8s-publishing-bot
Remove unused variables (only assigned to) from test code.
fea31b6 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560

Kubernetes-commit: e04b91facf180c17557a44e8e462858ea2936301
sttts pushed a commit to sttts/apiserver that referenced this issue Mar 1, 2018
@k8s-publishing-bot
Remove unused variables (only assigned to) from test code.
3940efa 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560

Kubernetes-commit: e04b91facf180c17557a44e8e462858ea2936301
sttts pushed a commit to sttts/apiserver that referenced this issue Mar 1, 2018
@k8s-publishing-bot
Remove unused variables (only assigned to) from test code.
8e47dd0 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560

Kubernetes-commit: e04b91facf180c17557a44e8e462858ea2936301
jingxu97 pushed a commit to jingxu97/kubernetes that referenced this issue Mar 13, 2018
@jingxu97
Remove unused variables (only assigned to) from test code.
5cde217 
This is revealed by the go/types package, which is stricter than
the Go compiler about unused variables. See also: golang/go#8560
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 18, 2018
@gopherbot gopherbot modified the milestones: Go1.12, Unplanned May 23, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@griesemer
crypto/tls: remove unused variable in benchmark code
9e5a083 
This fixes `go test go/types`.

https://golang.org/cl/23487/ introduced this code which contains
two unused variables (declared and assigned to, but never read).
cmd/compile doesn't report the error due open issue golang#8560 (the
variables are assigned to in a closure), but go/types does. The
build bot only runs go/types tests in -short mode (which doesn't
typecheck the std lib), hence this doesn't show up on the dashboard
either.

We cannot call b.Fatal and friends in the goroutine. Communicating
the error to the invoking function requires a channel or a mutex.
Unless the channel/sycnhronized variable is tested in each iteration
that follows, the iteration blocks if there's a failure. Testing in
each iteration may affect benchmark times.

One could use a time-out but that time depends on the underlying system.
Panicking seems good enough in this unlikely case; better than hanging
or affecting benchmark times.

Change-Id: Idce1172da8058e580fa3b3e398825b0eb4316325
Reviewed-on: https://go-review.googlesource.com/23528
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@griesemer
crypto/tls: remove unused variable in benchmark code
a709e2d 
This fixes `go test go/types`.

https://golang.org/cl/23487/ introduced this code which contains
two unused variables (declared and assigned to, but never read).
cmd/compile doesn't report the error due open issue golang#8560 (the
variables are assigned to in a closure), but go/types does. The
build bot only runs go/types tests in -short mode (which doesn't
typecheck the std lib), hence this doesn't show up on the dashboard
either.

We cannot call b.Fatal and friends in the goroutine. Communicating
the error to the invoking function requires a channel or a mutex.
Unless the channel/sycnhronized variable is tested in each iteration
that follows, the iteration blocks if there's a failure. Testing in
each iteration may affect benchmark times.

One could use a time-out but that time depends on the underlying system.
Panicking seems good enough in this unlikely case; better than hanging
or affecting benchmark times.

Change-Id: Idce1172da8058e580fa3b3e398825b0eb4316325
Reviewed-on: https://go-review.googlesource.com/23528
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@griesemer griesemer mentioned this issue May 6, 2021
Closed
@griesemer griesemer modified the milestones: Unplanned, Go1.18 Oct 28, 2021
@griesemer griesemer self-assigned this Oct 28, 2021
@griesemer griesemer added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. labels Oct 28, 2021
@griesemer
Copy link
Contributor

Marked NeedsInvestigation since we need to re-establish the current facts.

@griesemer griesemer mentioned this issue Oct 28, 2021
Closed
@griesemer
Copy link
Contributor

The Go 1.18 compiler correctly reports errors in these cases.
Per #49214 we will accept the correct behavior for 1.18 and document it in the release notes.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 14, 2021
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests