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

Untyped consts should not be automatically type converted #41160

Closed
rittneje opened this issue Sep 1, 2020 · 16 comments
Closed

Untyped consts should not be automatically type converted #41160

rittneje opened this issue Sep 1, 2020 · 16 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@rittneje
Copy link

rittneje commented Sep 1, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/.cache/go-build"
GOENV="/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build533260346=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/iLPNLfxLl5e

What did you expect to see?

All five calls to foo should have been compiler errors due to an implicit conversion from int to time.Duration.

What did you see instead?

If the variable in question is a const, it is automatically type converted. In our case, this lead to a bug.

I understand that this is as per the Go spec:

A value x is assignable to a variable of type T ("x is assignable to T") if one of the following conditions applies: [...] x is an untyped constant representable by a value of type T.

However, since this can easily lead to a bug, it would be nice if go vet at least warned about this situation.

@davecheney
Copy link
Contributor

All five calls to foo should have been compiler errors due to an implicit conversion from int to time.Duration.

This is not correct, a and c are not ints, they are I typed integer constants.

@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

@davecheney I get that that's the the technicality of the Go spec (that I cited). However, if you ask Go for the type of those consts, it will say int. As a developer, the fact that var and const behave differently on this point is needlessly confusing and error-prone.

@davecheney
Copy link
Contributor

This is because int is the default type for an I typed numeric constant when it is assigned To a variable

Those constants are not int, isn’t is the type of the value that is created when you pass those constants to fmt.Println

@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

Regardless of the technicalities of the Go spec on this point, the fact remains that this led to a real bug. A developer wrote const timeout = 60 instead of const timeout = 60 * time.Second. Consequently, the timeout was actually 60 nanoseconds. Had Go refused the conversion from untyped const to time.Duration, the bug would have been caught in advance.

@randall77
Copy link
Contributor

Unfortunately this is not something we can change at this point. Just forbidding the conversions would break people's code (and break the Go1 compatibility promise). In order to change this behavior, we'd need a full proposal about how to handle existing code without breaking anyone.

That's independent of whether we should make this change.

I understand that the current behavior can cause bugs. But it is also convenient in many situations. Is there any way we can increase type safety without needing casts for code like below?

func max(x, y int) int { ... }
v = max(v, 7)       // currently
v = max(v, int(7)) // with this proposal?

@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

I understand it would be a breaking change, which is why I was hoping for at least go vet warning. (I believe there is some precedent for that.)

With regards to your code sample, my immediate thought is that "conversion" of an untyped const to a compatible built-in type is okay, while anything else is flagged as a potential bug.

const x = 10

type myInt int

func intFunc(y int) { ... }
func uintFunc(y uint) { ... }
func myIntFunc(z myInt) { ... }
func durationFunc(d time.Duration) { ... }

intFunc(x) // ok
uintFunc(x) // ok
myIntFunc(x) // flagged
durationFunc(x) // flagged

@randall77
Copy link
Contributor

Vet is not intended as a side-channel to introduce a language change. We have strict standards as to what is allowed to be added to vet, and I don't think this idea qualifies (because of the false positive problem).

@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

Can you elaborate on the false positive problem? In my experience, the reason people make typedefs like this is because there are semantics associated with the new type. Take time.Duration for example. It's not just a number, but rather has the semantics of being a time duration (in nanoseconds, technically). So in my opinion, allowing these untyped consts to "sneak in" is always problematic, because it violates the intentions of the developer who created the interface you are working with.

@randall77
Copy link
Contributor

Just grepping through the stdlib, there are cases like:

./runtime/race/testdata/chan_test.go:	time.Sleep(1e7)

These uses are ok. Perhaps they are violating the abstraction, but they won't generate incorrect execution.

Speaking of, why is this ok:

const timeout = 60 * time.Second

But presumably, we would want this to not be ok:

const timeout = 60 + time.Second

How do we distinguish the two? Or would you have to write:

const timeout = time.Duration(60) * time.Second

@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

These uses are ok. Perhaps they are violating the abstraction, but they won't generate incorrect execution.

Sure they may be technically correct, but my feeling is they should be "corrected" to time.Sleep(10 * time.Millisecond) so that people who read the code later aren't forced to do mental math.

I'm assuming your second question is about how go vet (or any automated tool) could tell the difference, yes? I don't know enough to say how it would work.

const timeout = 60 * time.Second

I think this is okay, and should continue to be allowed.


const timeout = 60 + time.Second

I agree, this shouldn't be okay. (I think this situation shows that it would be nice if Go supported operator overloading.)


const timeout = time.Duration(60) * time.Second

This is okay, but the typecast is unnecessary.


Also, there is the matter of function typedefs, which in my experience ought to follow the same notions of implicit interface implementation. That is:

type myFunc func()

func foo() { ... }

func bar(f myFunc) { ... }

bar(foo) // should still be okay even without a typecast

So the more I think about, the more I think that if Go had (1) operator overloading and (2) proper enums, most of these type conversion bugs would go away.

@randall77
Copy link
Contributor

Sure they may be technically correct, but my feeling is they should be "corrected" to time.Sleep(10 * time.Millisecond) so that people who read the code later aren't forced to do mental math.

I agree that this code should be fixed to use the proper form. But Go made an explicit choice not to force people to change working code. That's the whole point of the Go 1 compatibility guarantee. We're not going to break user's code because we don't like their style.

I'm assuming your second question is about how go vet (or any automated tool) could tell the difference, yes? I don't know enough to say how it would work.

RIght. If we're going to entertain this idea, we need an unambiguous rule about when an ideal constant can implicitly casted to a named type and when it can't. (For tools to use, certainly, but that's secondary. For humans to understand is primary.)

@bcmills
Copy link
Contributor

bcmills commented Sep 1, 2020

Compare #20757.

@ianlancetaylor
Copy link
Contributor

time.Duration is one particular case. Existing code uses many types that are intended to support conversion from untyped constants, as well as many types that are not intended to support conversion from untyped constants. At this point we can't arbitrarily declare that all such conversions are invalid. That would break working, correct code. And we can't make that change in "go vet" either.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 1, 2020
@rittneje
Copy link
Author

rittneje commented Sep 1, 2020

@ianlancetaylor Do you have an alternative proposal for avoiding these kinds of bugs with typedefs like time.Duration? Fundamentally, I'd like to avoid such a bug happening again.

If go vet cannot enforce this because in some cases it is more stylistic in nature, would golint be acceptable?

@ianlancetaylor
Copy link
Contributor

@rittneje Run staticcheck (https://staticcheck.io/). It has checks for many things, including apparent misuses of time.Duration.

@mvdan
Copy link
Member

mvdan commented Jun 15, 2021

Seems like Ian answered all your points here, and there doesn't seem to be anything else to do, so I'm closing this issue for now.

@mvdan mvdan closed this as completed Jun 15, 2021
@golang golang locked and limited conversation to collaborators Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

7 participants