Untyped consts should not be automatically type converted

[rittneje]

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]

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]

@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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

Compare #20757.

[ianlancetaylor]

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.

[rittneje]

@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]

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

[mvdan]

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.