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: report uses of -0 in float32/64 context #19675

Open
griesemer opened this Issue Mar 23, 2017 · 14 comments

Comments

Projects
None yet
10 participants
@griesemer
Contributor

griesemer commented Mar 23, 2017

There are no -0 Go constants, -0 is the same as 0. Consequently, using -0 (or -0.0) to set a float32 or float64 variable will not produce the desired effect.

cmd/vet should report the use of a -0 in float32/64 context; i.e., if the type implied by context for a -0 constant is float32 or float64, the code is likely incorrect.

@griesemer griesemer added this to the Go1.9Maybe milestone Mar 23, 2017

@randall77

This comment has been minimized.

Contributor

randall77 commented Mar 23, 2017

-0 is also useless in an integer context. Is it ever useful?

@griesemer

This comment has been minimized.

Contributor

griesemer commented Mar 23, 2017

@randall77 Good point. -0 is just 0 in any numeric context. It's just that with integer values we understand that -0 and 0 are the same, and so there's not the same expectation as for floats.

I suppose one could report it as well.

@josharian

This comment has been minimized.

Contributor

josharian commented Mar 23, 2017

cc @robpike

How often does this really happen?

@griesemer

This comment has been minimized.

Contributor

griesemer commented Mar 23, 2017

@josharian Probably rather rare, but when it happens and it's not caught, it can be annoying. I believe it should be rather trivial to test for: go/types provides the actual machine-type for such constants, and if it's a float32/64, we just look to see if the constant expression has value 0 and starts with a negation (we may want to exclude complex expressions which be designed to also produce non-0 values if constant values change).

@robpike

This comment has been minimized.

Contributor

robpike commented Mar 23, 2017

Please demonstrate the three criteria listed in cmd/vet/README are satisfied. I believe it's hard to satisfy any of them.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Mar 23, 2017

Correctness: An untyped -0 used in floating-point context will always be interpreted as 0 rather than -0, which is probably not what's intended (or otherwise there wouldn't be a minus sign). Missing this detail has caused confusion at best, and bugs at worst. Most recently #19673 exposed the incorrect use of -0 in a test which in turn didn't test all it was supposed to test (and two test cases were incorrect).

Precision: This test can be done precisely and cheaply. We know if an expression is constant, we know its value, and we know its type. A constant expression with 0 value used in float context shouldn't have the form -x.

Frequency: This is the only critera which may perhaps not be satisfied. The main reason for that is that most people don't do floating point. However, in code dealing with floating-point arithmetic primarily, the use of -0 is not that uncommon.

@robpike robpike changed the title from cmd/vet: report uses of -0 in float32/64 context to proposal: cmd/vet: report uses of -0 in float32/64 context Mar 23, 2017

@robpike robpike added Proposal and removed FeatureRequest labels Mar 23, 2017

@robpike

This comment has been minimized.

Contributor

robpike commented Mar 23, 2017

I have trouble believing it's a worthwhile addition, but I've made it a proposal so it can be discussed in that category.

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 3, 2017

Since the check is cheap, proposal accepted.

@rsc rsc changed the title from proposal: cmd/vet: report uses of -0 in float32/64 context to cmd/vet: report uses of -0 in float32/64 context Apr 3, 2017

josharian added a commit to josharian/go that referenced this issue Apr 11, 2017

cmd/vet: add check for -0.0
DO NOT REVIEW

This is a quick hack for golang#19732.
It needs more thought, tests, and docs.

Updates golang#19675

Change-Id: Id1a323ba7ec001b2f1a88f30497defc6b823d409
@gopherbot

This comment has been minimized.

gopherbot commented Apr 21, 2017

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

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 22, 2017

Now that vet runs by default I'd only want to do this if we think it should be run automatically. Leaving for Go 1.11.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@nightlyone

This comment has been minimized.

Contributor

nightlyone commented Feb 3, 2018

interesting test case (which should not complain): https://play.golang.org/p/uGsLWDfHvqR

package main

import (
	"fmt"
)

const (
	a float64 = -1 * iota
	b
	c
	d
)

func main() {
	fmt.Println(a, b, c, d)
}
@cznic

This comment has been minimized.

Contributor

cznic commented Feb 3, 2018

dtto simple

const (
        a = -iota
        b
        c
)

must remain valid even when there's an untyped `-0' in it.

@josharian

This comment has been minimized.

Contributor

josharian commented Mar 30, 2018

@nightlyone @cznic the implementation in CL 38779 should handle both of those cases correctly (i.e. not complain).

I'm marking this as help wanted. CL 38779 is (I think) a good initial implementation. It mainly needs docs and tests. I hope someone interested will use CL 38779 as a base and build from there.

cc @mvdan @dominikh

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 30, 2018

This seems like a good starter issue for vet, especially given how there's a sample solution by Josh already. I'll give a shout out about it at the next London meetup, as I think there were a few people eager to contribute to Go last time.

Now that vet runs by default I'd only want to do this if we think it should be run automatically.

go help test only says that the criteria is "high-confidence", which I assume to be correctness. I assume that this would depend on the actual implementation, as some potential false positives have been posted here. I imagine it's very possible to write such a high-confidence check though, similar to how Josh's implementation is rather conservative.

Quasilyte added a commit to Quasilyte/go-contributing-ru that referenced this issue Apr 1, 2018

tasks: add several new tracked tasks
New tasks include:
golang/go#19675 cmd/vet: report uses of -0 in float32/64 context
golang/go#19683 cmd/compile: eliminate usages of global lineno
golang/go#19670 x/tools/go/ssa: make opaqueType less annoying to use
golang/go#19636 encoding/base64: decoding is slow
golang/go#23471 x/perf/cmd/benchstat: tips or quickstart for newcomers
golang/go#19577 test: errorcheck support for intraline errors
golang/go#19490 cmd/vet: reduce the amount of false positives for -shadow mode
golang/go#19042 cmd/internal/obj: optimize wrapper method prologue for branch prediction
golang/go#19013 cmd/compile: add tool for understanding/debugging SSA rules

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

@dominikh dominikh referenced this issue Sep 18, 2018

Open

staticcheck: incorporate go vet checks #149

4 of 22 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment