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: diagnose set and not used #6072

Open
rsc opened this Issue Aug 7, 2013 · 8 comments

Comments

Projects
None yet
5 participants
@rsc
Contributor

rsc commented Aug 7, 2013

If you have:

x := 1
_ = x
x = 2

the spec allows this program and therefore the compilers accept it without comment.

However, it would be nice if vet could tell you about 'set and not used' on the final
line.
Maybe this falls out of some code Alan already has lying around. Maybe not.
@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 20, 2013

Contributor

Comment 1:

Deferring to 1.3.

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

Contributor

robpike commented Aug 20, 2013

Comment 1:

Deferring to 1.3.

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

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 20, 2013

Contributor

Comment 2:

Labels changed: removed go1.3maybe.

Contributor

robpike commented Aug 20, 2013

Comment 2:

Labels changed: removed go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 27, 2013

Contributor

Comment 3:

Labels changed: added go1.3maybe.

Contributor

rsc commented Nov 27, 2013

Comment 3:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 4:

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

Contributor

rsc commented Dec 4, 2013

Comment 4:

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

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 5:

Labels changed: added repo-tools.

Contributor

rsc commented Dec 4, 2013

Comment 5:

Labels changed: added repo-tools.

@adonovan

This comment has been minimized.

Show comment
Hide comment
@adonovan

adonovan Nov 10, 2014

Comment 6:

(Sorry, didn't notice this till today.)
To detect this situation in general, we need to find all assignments
to non-address-taken local variables (except named return parameters)
on which there is no control flow path leading to a reference to that
var.
Obviously that means we'd need to build a syntax-level control-flow graph.
Updates of named return parameters are exempt because their effect may
be observed via return or a defer/panic/recover.
Address-taken vars are exempted because we cannot tell with only local
analysis whether the update might be observed through a pointer.  
The following expressions e are considered address-taken:
Base cases:
- &e, obviously.
- var e T; _ = func() { ...e... }
  Free variables of a function literal are considered address-taken
  since the closure holds the address, and we cannot know in general
  when the func is called.
- e.Method, where e has type T and the method receiver is *T.
Induction rules:
- from (e) to e
- from e[:] to e  (iff e has array type)
- from e[i] to e  (iff e has array type)
- from e.f to e
This information is easily obtained during SSA construction.
(Although the SSA builder would optimize the dead stores away, this
can be suppressed by skipping the conversion to full SSA, i.e. ssadump
-build=N.)  But vet probably doesn't want to depend on go/ssa...
The lower-hanging fruit might be assignments to non-AT local vars
whose declaration is in the same or an outer lexical block, with no
intervening for-loop blocks (and ignore loops created with goto), with
no references in subsequent statements.  This doesn't need a CFG, and
I can probably factor out a useful little AT pass that would make this
trivial.

Owner changed to @adonovan.

adonovan commented Nov 10, 2014

Comment 6:

(Sorry, didn't notice this till today.)
To detect this situation in general, we need to find all assignments
to non-address-taken local variables (except named return parameters)
on which there is no control flow path leading to a reference to that
var.
Obviously that means we'd need to build a syntax-level control-flow graph.
Updates of named return parameters are exempt because their effect may
be observed via return or a defer/panic/recover.
Address-taken vars are exempted because we cannot tell with only local
analysis whether the update might be observed through a pointer.  
The following expressions e are considered address-taken:
Base cases:
- &e, obviously.
- var e T; _ = func() { ...e... }
  Free variables of a function literal are considered address-taken
  since the closure holds the address, and we cannot know in general
  when the func is called.
- e.Method, where e has type T and the method receiver is *T.
Induction rules:
- from (e) to e
- from e[:] to e  (iff e has array type)
- from e[i] to e  (iff e has array type)
- from e.f to e
This information is easily obtained during SSA construction.
(Although the SSA builder would optimize the dead stores away, this
can be suppressed by skipping the conversion to full SSA, i.e. ssadump
-build=N.)  But vet probably doesn't want to depend on go/ssa...
The lower-hanging fruit might be assignments to non-AT local vars
whose declaration is in the same or an outer lexical block, with no
intervening for-loop blocks (and ignore loops created with goto), with
no references in subsequent statements.  This doesn't need a CFG, and
I can probably factor out a useful little AT pass that would make this
trivial.

Owner changed to @adonovan.

@rsc rsc added accepted labels Nov 10, 2014

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc changed the title from cmd/vet: diagnose set and not used to x/tools/cmd/vet: diagnose set and not used Apr 14, 2015

@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015

@rsc rsc removed the repo-tools label Apr 14, 2015

@dgryski

This comment has been minimized.

Show comment
Hide comment
@dgryski

dgryski Oct 20, 2017

Contributor

The https://github.com/gordonklaus/ineffassign tool performs a similar check and I have found it has a very low false-positive rate.

Contributor

dgryski commented Oct 20, 2017

The https://github.com/gordonklaus/ineffassign tool performs a similar check and I have found it has a very low false-positive rate.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 22, 2017

Member

@dgryski I believe staticcheck also has some relevant checks, see SA4xxx category at https://staticcheck.io/docs/staticcheck#checks. /cc @dominikh

Member

dmitshur commented Oct 22, 2017

@dgryski I believe staticcheck also has some relevant checks, see SA4xxx category at https://staticcheck.io/docs/staticcheck#checks. /cc @dominikh

@mvdan mvdan changed the title from x/tools/cmd/vet: diagnose set and not used to cmd/vet: diagnose set and not used May 31, 2018

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