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/vet: warn about unused struct or array, ignoring assignment to field or element #449

Open
gopherbot opened this issue Dec 22, 2009 · 8 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gopherbot
Copy link

by RyanneDolan:

What steps will reproduce the problem?

v := new(struct{i int})
v.i = 1

What is the expected output? What do you see instead?

Should get "v declared and not used" error.  Instead, compiles fine.

What is your $GOOS?  $GOARCH?

linux/386

Which revision are you using?  (hg identify)

54392bd03d06 tip

Please provide any additional information below.

The dot operator should mark its left operand as "used" only when it 
appears on the RHS.

a = 1    // 'a' is not marked as used (CORRECT)
v.i = 1  // 'v' is marked as used (WRONG)
a = v.i  // 'v' is marked as used (CORRECT)

This bug is important to fix for consistency, and because it confuses new 
programmers who might expect reference semantics:

for _,v := range arr {v.i = 1} // doesn't do anything

Mentioned here:

http://groups.google.com/group/golang-
nuts/browse_thread/thread/342fe61c78916496/fbb57f693f1aca72
@rsc
Copy link
Contributor

rsc commented Dec 22, 2009

Comment 1:

Your first example is definitely too strict.
You suggest disallowing:
v := new(Point)
v.x = 1
but what about
v := NewPoint()
v.x = 1
or
v := OldPoint()  // returns a reference shared with others
v.x = 1
The used and not set rules do not treat "new" specially.
"v" is being used to get to the field v.x in all these
examples.  In particular, when v is a pointer,
    for _, v = range arr { v.i = 1 }
is a perfectly legitimate program.
It is only when v is a struct (not a pointer) that this
matters.  It might be reasonable to treat v.i = 1, when
v is not a pointer, only as a write to v, not a read.
I thought the specific rule was in the spec but I can't
find it at the moment.

Labels changed: added languagechange.

Owner changed to r...@golang.org.

Status changed to Thinking.

@gopherbot
Copy link
Author

Comment 2 by RyanneDolan:

> v := OldPoint()  // returns a reference shared with others
> v.x = 1
Good point.  My example is still logically incorrect I think, but not a compile 
error.  Sorry.
A revised example (as per your comment) for the record:
var v struct{i int}
v.i = 1
Interestingly, I think
  for _, v = range arr { v.i = 1 }
illuminates a problem with adding generics to Go: the same code can have value or 
reference semantics depending on the underlying types.  I guess that is what confused 
me, and as I said above, confuses new Go programmers.
Thanks for considering the issue.

@rsc
Copy link
Contributor

rsc commented Dec 9, 2011

Comment 3:

Labels changed: added priority-later.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 5:

Labels changed: added go2.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 6:

Labels changed: added repo-main.

@rsc
Copy link
Contributor

rsc commented Mar 3, 2014

Comment 7:

Labels changed: added release-none.

@gopherbot gopherbot added Thinking LanguageChange priority-later v2 A language change or incompatible library change labels Mar 3, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/gc: struct field assignment is treated as a read for used and not set errors cmd/compile: struct field assignment is treated as a read for used and not set errors Jun 8, 2015
@rsc rsc changed the title cmd/compile: struct field assignment is treated as a read for used and not set errors proposal: cmd/compile: do not treat struct field assignment use of struct Jun 17, 2017
@rsc rsc changed the title proposal: cmd/compile: do not treat struct field assignment use of struct proposal: cmd/compile: do not treat struct field assignment as use of struct Jun 17, 2017
@rsc
Copy link
Contributor

rsc commented Jun 17, 2017

To be clear, this is now only about:

var j int
j = 1

var v struct { i int }
v.i = 1

var x [2]int
x[0] = 1

The compiler reports that j is declared and not used, but it fails to report that both v and x are declared and not used.

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

I think that spec wording can be read as applying to all of j, v, and x, but it could break existing programs, hence the LanguageChange and Go2 tags.

@rsc rsc changed the title proposal: cmd/compile: do not treat struct field assignment as use of struct proposal: spec: do not treat struct field assignment as use of struct Jun 17, 2017
@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Thinking labels Dec 5, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@gopherbot gopherbot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 3, 2019
@ianlancetaylor ianlancetaylor changed the title proposal: spec: do not treat struct field assignment as use of struct cmd/vet: warn about unused struct or array, ignoring assignment to field or element Mar 17, 2020
@ianlancetaylor
Copy link
Contributor

In the current Go ecosystem, this makes sense as a vet change. Let's repurpose this issue to adding this check to vet, rather than worrying about changing the compiler or language.

@ianlancetaylor ianlancetaylor removed the v2 A language change or incompatible library change label Mar 17, 2020
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed LanguageChange NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 17, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Backlog Mar 17, 2020
@odeke-em odeke-em self-assigned this Mar 10, 2021
@rsc rsc unassigned rsc and odeke-em Jun 22, 2022
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants