runtime: inconsistent behaviour of make with negative lengths

[ainar-g]

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

$ go version
go version go1.17beta1 linux/amd64

Does this issue reproduce with the latest release?

Yes, and also on Go 1.15.11 and 1.16.4.

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ainar/.cache/go-build"
GOENV="/home/ainar/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ainar/go/pkg/mod"
GONOPROXY="[removed]"
GONOSUMDB="[removed["
GOOS="linux"
GOPATH="/home/ainar/go"
GOPRIVATE="[removed]"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ainar/go/go1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ainar/go/go1.17/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17beta1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1226297306=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

What did you expect to see?

runtime.errorString	runtime error: makemap: len out of range
runtime.errorString	runtime error: makeslice: len out of range
runtime.errorString	runtime error: makechan: len out of range

What did you see instead?

(no panic)
runtime.errorString	runtime error: makeslice: len out of range
runtime.plainError	makechan: size out of range

The map make doesn't panic at all, while the other two panic with inconsistent messages. The documentation doesn't specify any constraints on these parameters, besides cap >= len, and neither does the Spec, as far as I can see. At the very least the behaviour w.r.t. invalid lengths should be documented, I think.

[randall77]

The map value is just a hint, so we don't panic in that case. In other words, there's something reasonable we can do and the program can continue (unlike the slice or chan situations). More discussion at #24308
I agree that the chan failure should have "runtime error:" prefixed, and probably be an errorString. I think we're allowed to change runtime errors without breaking go1 compatibility.

[ainar-g]

I see, thanks. Perhaps the fact that this is a hint as opposed to an actual size should also be documented in package builtin. The docs currently say (emphasis added):

Map: An empty map is allocated with enough space to hold the
specified number of elements. The size may be omitted, in which case
a small starting size is allocated.

The Spec calls the parameter “hint”, which is correct, I assume.

[randall77]

I think we decided in #14965 that we weren't going to add the "runtime error:" prefix to errors we were already generating, just to avoid breaking people that depended on the text. So I don't think there's anything to do here. Closing as unfortunate, but not worth fixing.

[ainar-g]

@randall77, should the documentation in package builtin mention that invalid hints are set to zero, or would that be unnecessary? If it should, I am ready to send a CL about that. If not, oh well.

[randall77]

I don't think it is necessary.

Map: An empty map is allocated with enough space to hold the specified number of elements.

That seems correct and complete. If you specify -3, then the map is allocated with enough space for -3 elements.

The "size" part in the next sentence (which you quoted above) is just the formal parameter name in the declaration.

If anything, we could document the panic behavior for the other kinds (slice, chan). But we generally don't get that formal (for instance, close of a nil channel panics, but we don't mention that in the builtin docs).