cmd/compile: report error for ineffectual variable assignments #10989

Closed
gordonklaus opened this Issue May 29, 2015 · 13 comments

Comments

Projects
None yet
7 participants
@gordonklaus
Contributor

gordonklaus commented May 29, 2015

This block
{
x := 0
_ = x
x = 1
}
compiles without error, whereas I would expect an error on the "x=1" line. Something like "x assigned and not used".

For precedent we have the existing "x declared and not used" error. This is essentially the same error.

For someone bitten by the lack of this error, see http://www.qureet.com/blog/golang-beartrap/

@gordonklaus

This comment has been minimized.

Show comment
Hide comment
@gordonklaus

gordonklaus May 29, 2015

Contributor

#449 seems related

Contributor

gordonklaus commented May 29, 2015

#449 seems related

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer May 29, 2015

Contributor

This is not a bug. For reference: http://tip.golang.org/ref/spec#Variable_declarations

"Implementation restriction: A compiler may make it illegal to declare a variable inside a function body if the variable is never used."

In this block, the assignment

_ = x

uses the variable x. There's no promise of an error if a variable is assigned to, but not used later.

One might argue that assignment to _ doesn't constitute a "use", but in fact this exact pattern is used all over the place to mark a use for the compiler when we don't have another one (e.g. during development).

Contributor

griesemer commented May 29, 2015

This is not a bug. For reference: http://tip.golang.org/ref/spec#Variable_declarations

"Implementation restriction: A compiler may make it illegal to declare a variable inside a function body if the variable is never used."

In this block, the assignment

_ = x

uses the variable x. There's no promise of an error if a variable is assigned to, but not used later.

One might argue that assignment to _ doesn't constitute a "use", but in fact this exact pattern is used all over the place to mark a use for the compiler when we don't have another one (e.g. during development).

@griesemer griesemer closed this May 29, 2015

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer May 29, 2015

Contributor

PS: Just to be clear; I'm not saying it wouldn't be useful to have such cases flagged as errors. I am just saying that this is not a bug per the current spec.

The problem with flagging such errors is that in general the situation is undecidable; e.g.:

x := f() // f always called for some side effect
if condition {
   y = x
}

In this case, x may or may not get used (no _ involved here!), and the compiler may not be able to deduce this 100% correctly in general.

A conservative approach might be better than what we have now. For instance, one could say that a compiler may make it illegal to declare or assign to a variable inside a function body if there's no possible path that will use the variable. This would permit the above case (x is used in the "then" path), and it would also permit things like

var x int
for cond {
    sum += x
    x = f()
}
// no further use of x below

since it's possible for x to be used in a subsequent iteration. Such decision making would need to be done independent of the condition otherwise it get's very complicated to explain the behavior (e.g., is x used if cond is the constant false?), etc. etc. But I believe it wouldn't be too hard and actually decidable (analogous to the checks we do for missing return statements, but more conservative in this case).

But even if it's possible to tighten the rules here a bit, this would be a backward-incompatible change because it may make currently compiling programs not compile anymore.

Contributor

griesemer commented May 29, 2015

PS: Just to be clear; I'm not saying it wouldn't be useful to have such cases flagged as errors. I am just saying that this is not a bug per the current spec.

The problem with flagging such errors is that in general the situation is undecidable; e.g.:

x := f() // f always called for some side effect
if condition {
   y = x
}

In this case, x may or may not get used (no _ involved here!), and the compiler may not be able to deduce this 100% correctly in general.

A conservative approach might be better than what we have now. For instance, one could say that a compiler may make it illegal to declare or assign to a variable inside a function body if there's no possible path that will use the variable. This would permit the above case (x is used in the "then" path), and it would also permit things like

var x int
for cond {
    sum += x
    x = f()
}
// no further use of x below

since it's possible for x to be used in a subsequent iteration. Such decision making would need to be done independent of the condition otherwise it get's very complicated to explain the behavior (e.g., is x used if cond is the constant false?), etc. etc. But I believe it wouldn't be too hard and actually decidable (analogous to the checks we do for missing return statements, but more conservative in this case).

But even if it's possible to tighten the rules here a bit, this would be a backward-incompatible change because it may make currently compiling programs not compile anymore.

@gordonklaus

This comment has been minimized.

Show comment
Hide comment
@gordonklaus

gordonklaus May 30, 2015

Contributor

Just to be clear, I wasn't suggesting it was a bug, I was asking for a feature (-:

I think it would be really interesting to do an analysis of some large Go corpus to see how often this "assigned and not used" error would crop up, if the compiler implemented it. My suspicion is that it would highlight only buggy code, in which case it would be worthwhile to make the backwards-incompatible change (i.e., the cost of breaking some people's code would be outweighed by the benefit of finding their bugs).

Contributor

gordonklaus commented May 30, 2015

Just to be clear, I wasn't suggesting it was a bug, I was asking for a feature (-:

I think it would be really interesting to do an analysis of some large Go corpus to see how often this "assigned and not used" error would crop up, if the compiler implemented it. My suspicion is that it would highlight only buggy code, in which case it would be worthwhile to make the backwards-incompatible change (i.e., the cost of breaking some people's code would be outweighed by the benefit of finding their bugs).

@gordonklaus

This comment has been minimized.

Show comment
Hide comment
@gordonklaus

gordonklaus May 30, 2015

Contributor

I wrote a tool [0] to do a rough analysis. Running it on the standard library and whatever was in my GOPATH turned up mostly innocuous (but sloppy) code, and also a few (~10%) bugs. Probably not enough to warrant a backwards-incompatible language change.

[0] https://github.com/gordonklaus/ineffassign

Contributor

gordonklaus commented May 30, 2015

I wrote a tool [0] to do a rough analysis. Running it on the standard library and whatever was in my GOPATH turned up mostly innocuous (but sloppy) code, and also a few (~10%) bugs. Probably not enough to warrant a backwards-incompatible language change.

[0] https://github.com/gordonklaus/ineffassign

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Jun 2, 2015

Contributor

The compiler could do this. I believe it is allowed to. There is already flow analysis that complains about some unused variables.

Contributor

robpike commented Jun 2, 2015

The compiler could do this. I believe it is allowed to. There is already flow analysis that complains about some unused variables.

@robpike robpike reopened this Jun 2, 2015

@alandonovan

This comment has been minimized.

Show comment
Hide comment
@alandonovan

alandonovan Jun 2, 2015

Contributor

See also #6072, which requests that vet check for the situation described above, and #8560, which reports two related inconsistencies with the "unused" check in gc.

Contributor

alandonovan commented Jun 2, 2015

See also #6072, which requests that vet check for the situation described above, and #8560, which reports two related inconsistencies with the "unused" check in gc.

@dr2chase

This comment has been minimized.

Show comment
Hide comment
@dr2chase

dr2chase Jun 2, 2015

Contributor

I think as we bring SSA online we will discover more and more places where dubious code could be automatically discovered in programs that are officially specification-conforming. I'm a little unclear on how close the cooperation is between "vet" and the guts of the compiler, but it seems likely to me that there could be a rolling technology upgrade.

Contributor

dr2chase commented Jun 2, 2015

I think as we bring SSA online we will discover more and more places where dubious code could be automatically discovered in programs that are officially specification-conforming. I'm a little unclear on how close the cooperation is between "vet" and the guts of the compiler, but it seems likely to me that there could be a rolling technology upgrade.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Jun 2, 2015

Contributor

The problem is not so much what could be done by a compiler to flag dubious or even clearly erroneous code (even w/o SSA), but what can be specified clearly, and easily in the spec such that all compilers reject the same program. We want the language to be defined by the spec, and not by the implementation.

Contributor

griesemer commented Jun 2, 2015

The problem is not so much what could be done by a compiler to flag dubious or even clearly erroneous code (even w/o SSA), but what can be specified clearly, and easily in the spec such that all compilers reject the same program. We want the language to be defined by the spec, and not by the implementation.

@dr2chase

This comment has been minimized.

Show comment
Hide comment
@dr2chase

dr2chase Jun 2, 2015

Contributor

I understand entirely the constraints on the compiler, but the problem is that the compiler is a likely home for all the newer analyses. What I imagine is that either go vet gains access to SSA-derived information, or else that we add a mode/flag to the compiler that would enable a bunch more warnings.

I get the impression that we prefer doing this in vet. How do we feel about the compiler gaining some flags putting it into "vet-helper" mode, if that turns out to save a lot on implementation effort or lets us add the analysis to vet a little earlier?

Contributor

dr2chase commented Jun 2, 2015

I understand entirely the constraints on the compiler, but the problem is that the compiler is a likely home for all the newer analyses. What I imagine is that either go vet gains access to SSA-derived information, or else that we add a mode/flag to the compiler that would enable a bunch more warnings.

I get the impression that we prefer doing this in vet. How do we feel about the compiler gaining some flags putting it into "vet-helper" mode, if that turns out to save a lot on implementation effort or lets us add the analysis to vet a little earlier?

@alandonovan

This comment has been minimized.

Show comment
Hide comment
@alandonovan

alandonovan Jun 2, 2015

Contributor

The Go compiler has no warnings because the experience from C++ is that each site has their own policy for which warnings are treated as hard errors and which can be ignored. The hard errors create different dialects of the language, and the ignorable warnings create noise.

Two simpler solutions:
(1) don't make vet too clever.
(2) if we really need dataflow analysis or SSA form, use the go.tools/x/go/ssa package, which is designed for source analysis tools that, like vet, want high fidelity to source and don't need to transform the IR.

Contributor

alandonovan commented Jun 2, 2015

The Go compiler has no warnings because the experience from C++ is that each site has their own policy for which warnings are treated as hard errors and which can be ignored. The hard errors create different dialects of the language, and the ignorable warnings create noise.

Two simpler solutions:
(1) don't make vet too clever.
(2) if we really need dataflow analysis or SSA form, use the go.tools/x/go/ssa package, which is designed for source analysis tools that, like vet, want high fidelity to source and don't need to transform the IR.

@ianlancetaylor ianlancetaylor changed the title from cmd/gc: report error for ineffectual variable assignments to cmd/compile: report error for ineffectual variable assignments Jun 3, 2015

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jun 3, 2015

@alandonovan

This comment has been minimized.

Show comment
Hide comment
@alandonovan

alandonovan Oct 5, 2016

Contributor

I think we should close this issue since it cannot be implemented as a compiler error without breaking many legal Go 1.x programs. It could usefully be implemented as a vet check, but that is the topic of issue #6072.

Contributor

alandonovan commented Oct 5, 2016

I think we should close this issue since it cannot be implemented as a compiler error without breaking many legal Go 1.x programs. It could usefully be implemented as a vet check, but that is the topic of issue #6072.

@griesemer griesemer changed the title from cmd/compile: report error for ineffectual variable assignments to cmd/vet: report error for ineffectual variable assignments Oct 5, 2016

@griesemer griesemer changed the title from cmd/vet: report error for ineffectual variable assignments to cmd/compile: report error for ineffectual variable assignments Oct 5, 2016

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Oct 5, 2016

Contributor

@alandonovan Agreed. Closing in favor of #6072.

Contributor

griesemer commented Oct 5, 2016

@alandonovan Agreed. Closing in favor of #6072.

@griesemer griesemer closed this Oct 5, 2016

@golang golang locked and limited conversation to collaborators Oct 5, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.