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

slices: better panic message for out of range Insert #64152

Open
sedyh opened this issue Nov 15, 2023 · 14 comments
Open

slices: better panic message for out of range Insert #64152

sedyh opened this issue Nov 15, 2023 · 14 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@sedyh
Copy link

sedyh commented Nov 15, 2023

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

$ go version 
go version go1.21 darwin/arm64

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="arm64"
GOBIN=""
GOCACHE="/Users/sedyh/Library/Caches/go-build"
GOENV="/Users/sedyh/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/sedyh/Desktop/share/go/path/go1.21.1/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sedyh/Desktop/share/go/path/go1.21.1"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/sedyh/Desktop/share/go/root/go1.21.1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/sedyh/Desktop/share/go/root/go1.21.1/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.21.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/x5/kntr8jy137vg9cjlm83chwmh0000gn/T/go-build3952104386=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

For some reason slices.Insert causes out of bounds panic if there no values at if !overlaps(v, s[i+m:]) {}.

As far as I can see in https://go.dev/cl/540155, this is intended, but it seems wrong to me, because:

  • I don't see any point in this. I think this is the only case why I wanted to use it.
  • This case is not processed separately, it is a standard out of bounds panic.
  • This is not stated in the function comments.

What did you expect to see?

s := []float64{}
s = slices.Grow(s, 5)
s = slices.Insert(s, 4, 1.0)

The function works the same regardless of how full the slice is.

What did you see instead?

panic: runtime error: slice bounds out of range [5:1]

This is not a specially caused panic, it looks like a missed case.

@randall77
Copy link
Contributor

What would end up in s[0] through s[3]?
In other words, what is the slice that you expect to result from the example you gave?

@sedyh
Copy link
Author

sedyh commented Nov 15, 2023

What would end up in s[0] through s[3]? In other words, what is the slice that you expect to result from the example you gave?

I hoped that slices.Insert would fill them with default values for me (.0) if index is bigger than cap or replace the existing values if index is lower than cap.

@gophun
Copy link

gophun commented Nov 15, 2023

  • This is not stated in the function comments.

It is stated in the doc comment: "Insert panics if i is out of range."

This case is not processed separately, it is a standard out of bounds panic.

Because it is a standard out of bounds panic. slices.Grow does not grow the length of a slice, it only grows the capacity. You would get the same panic here:

s := []float64{}
s = slices.Grow(s, 5)
s[4] = 1.0

@sedyh
Copy link
Author

sedyh commented Nov 15, 2023

I would really like some of the functions in this package to be able to do make/append and fill the passed slice up to requested index as needed.

Is there any workaround or slice trick that combines insert and growing the length?

@sedyh
Copy link
Author

sedyh commented Nov 15, 2023

Is there any workaround or slice trick that combines insert and growing the length?

Slice tricks' Expand look like what I need, I wish that func was in the slices package.

a = append(a[:i], append(make([]T, j), a[i:]...)...)

@go101
Copy link

go101 commented Nov 15, 2023

The one-line trick is quite inefficient for some cases.

For OP's specified case:

s := []float64{}
s = slices.Grow(s, 5)
s = slices.Insert(s, 4, 1.0)

just do:

s := append(make([]float64, 4, 5), 1.0)

@go101
Copy link

go101 commented Nov 15, 2023

If looks it is essential to put the following line at the start of the slices.Insert funciton:

_ = s[i:]

Otherwise, the panic message is misleading.

The line should be helpful for BCE.

This fix for #63913 is not a perfect one.

@sedyh
Copy link
Author

sedyh commented Nov 15, 2023

Just do:
s := append(make([]float64, 4, 5), 1.0)

Thanks for suggestion, but I also need to be able to insert elements at any index location other than extending the slice, so the previous one is more suitable.

It is stated in the doc comment: "Insert panics if i is out of range."

I feel like it would be better to state "or slice is empty" separately, even if the first option implies this.

Otherwise, the panic message is misleading.

+1

@gopherbot
Copy link

Change https://go.dev/cl/542455 mentions this issue: slices: imporve Insert panic message for index out of range

@gazerro
Copy link
Contributor

gazerro commented Nov 15, 2023

I think it could help if the term "out of range" is not used in the documentation:

... Insert panics if i is out of range.. ...

because it might not be clear that "out of range" refers to the slicing s[i:]. It could be interpreted as referring to the slicing s[:i]. With the same value of i, s[i:] can be out of range, and s[:i] may not be.

To remove this ambiguity, I would write:

... Insert panics if i > len(s). ...

@gopherbot
Copy link

Change https://go.dev/cl/542535 mentions this issue: slices: add explanation of 'out of range' for Insert

@callthingsoff
Copy link
Contributor

... Insert panics if i > len(s). ...

@gazerro Also panics when i < 0.

@gazerro
Copy link
Contributor

gazerro commented Nov 15, 2023

@callthingsoff You're right. We can use a similar sentence used for Delete and Replace (see recent commit 7a1fce8), so it would be

... Insert panics if i > len(s) or s[:i] is not a valid slice of s. ...

@seankhliao seankhliao changed the title slices.Insert causes out of bounds if there are no values slices: better panic message for out of range Insert Nov 15, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 15, 2023
@callthingsoff
Copy link
Contributor

We can use a similar sentence used for Delete and Replace (see recent commit 7a1fce8), so it would be

... Insert panics if i > len(s) or s[:i] is not a valid slice of s. ...

cc @ianlancetaylor @randall77

gopherbot pushed a commit that referenced this issue Nov 17, 2023
For #64152

Change-Id: I32531aa8d147f4f10f6498f5ea1474555e93b6de
GitHub-Last-Rev: 48bb3bb
GitHub-Pull-Request: #64180
Reviewed-on: https://go-review.googlesource.com/c/go/+/542535
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jes Cok <xigua67damn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Nov 19, 2023
The panic message of the current implementation for index out of range is not ideal.
This PR tries to improve it.

Fixes #63913 and #64152

Change-Id: Ibcf6c9c0f555c8b8bf46b7d6f20f0ccc5698acd4
GitHub-Last-Rev: 1bbec23
GitHub-Pull-Request: #64163
Reviewed-on: https://go-review.googlesource.com/c/go/+/542455
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants