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

Closed
griesemer opened this issue Mar 23, 2017 · 15 comments
Closed

cmd/vet: report uses of -0 in float32/64 context #19675

griesemer opened this issue Mar 23, 2017 · 15 comments

Comments

@griesemer
Copy link
Contributor

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
Copy link
Contributor

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

@griesemer
Copy link
Contributor Author

@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
Copy link
Contributor

cc @robpike

How often does this really happen?

@griesemer
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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 cmd/vet: report uses of -0 in float32/64 context proposal: cmd/vet: report uses of -0 in float32/64 context Mar 23, 2017
@robpike
Copy link
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
Copy link
Contributor

rsc commented Apr 3, 2017

Since the check is cheap, proposal accepted.

@rsc rsc changed the title proposal: cmd/vet: report uses of -0 in float32/64 context 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
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
Copy link

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

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017
@rsc
Copy link
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
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

@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
Copy link
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
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
@smasher164
Copy link
Member

Seems like this is a subset of #30951. Further discussion on the -0 float check should continue there.

@golang golang locked and limited conversation to collaborators Sep 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests