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.

Sign up for GitHub

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/vet: flag atomic.Value usages with interface types #22550

Open
dsnet opened this issue Nov 2, 2017 · 4 comments
Open

cmd/vet: flag atomic.Value usages with interface types #22550

dsnet opened this issue Nov 2, 2017 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented Nov 2, 2017

Consider the following:

var v atomic.Value
var err error

err = &http.ProtocolError{}
v.Store(err)
err = io.EOF
v.Store(err)

The intention to have a atomic value store for errors. However, running this code panics:

panic: sync/atomic: store of inconsistently typed value into Value

This is because atomic.Value requires that the underlying concrete type be the same (which is a reasonable expectation for its implementation). When going through the atomic.Value.Store method call, the fact that both these are of the error interface is lost.

Perhaps we should add a vet check that flags usages of atomic.Value where the argument passed in is an interface type?

Vet criterions:

  • frequency: not sure, I haven't done an analysis through all Go corpus.
  • correctness: this is almost always wrong. Any "correct" usages should type assert to the concrete value first.
  • accuracy: if the type information available can conclusively show an argument is an interface type, then very accurate.

\cc @robpike
\cc @dominikh for staticcheck
oshimaya pushed a commit to oshimaya/go that referenced this issue Nov 3, 2017
@rsc
go/importer: support lookup in importer.For
9aa6f80 
The support in this CL assumes that something at a higher level than
the toolchain-specific importers is taking care of converting imports
in source code into canonical import paths before invoking the
toolchain-specific importers. That kind of "what does an import mean"
as opposed to "find me the import data for this specific path"
should be provided by higher-level layers.

That's a different layering than the default behavior but matches the
current layering in the compiler and linker and works with the metadata
planned for generation by the go command for package management.
It should also eventually allow the importer code to stop concerning
itself with source directories and vendor import translation and maybe
deprecate ImporterFrom in favor of Importer once again. But that's all
in the future. For now, just make non-nil lookups work, and test that.

Fixes golang#13847.
Adds golang#22550.

Change-Id: I048c6a384492e634988a7317942667689ae680ff
Reviewed-on: https://go-review.googlesource.com/74354
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang deleted a comment from gopherbot Nov 3, 2017
@robpike
Copy link
Contributor

robpike commented Nov 3, 2017

Deleted an incorrect reference from a CL to help others avoid the confusion it caused me.

I suspect the frequency is very low, and since the runtime catches the error anyway, and is likely to do so the first time the program is run, some convincing is required.

@dsnet dsnet added this to the Unplanned milestone Nov 3, 2017
@dominikh dominikh mentioned this issue Sep 18, 2018
Closed
27 tasks
@agnivade
Copy link
Contributor

/cc @mvdan @alandonovan

@alandonovan
Copy link
Contributor

Seems very obscure. atomic.Value is not an everyday or (everyyear) thing.

@mvdan
Copy link
Member

mvdan commented Feb 17, 2019

Here's a self-contained reproducer: https://play.golang.org/p/mM_n7IQFJ0p

I am also unsure that this passes the frequency requirement. I've only used atomic.Value a handful of times, and I've never encountered this bug.

I've also quickly put together a gogrep query to find these, and to verify that std cmd golang.org/x/tools/... has zero occurrences. I realise tha the query is a bit silly; I need to refine the tool.

$ gogrep -x '$v.Store($x)' -x '$v' -a 'type(atomic.Value)' -p 2 -x '$x' -a 'is(interface)' -p 2 f.go
/home/mvdan/f.go:17:2: v.Store(err)
/home/mvdan/f.go:19:2: v.Store(err)

Perhaps the query can be used on a large corpus of Go code, to check if any matches would be found.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 3, 2019
@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
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@agnivade @mvdan @ALTree @robpike @adonovan @dsnet @alandonovan