panic: runtime error: slice bounds out of range

[tali...@gmail.com]

I'd argue that slicing from i to j where j < i should give a more informative message than "panic: runtime error: slice bounds out of range".

Perhaps it should say something like "panic: runtime error: slice has negative length"

Or does this have performance implications?

Otherwise, I'm having a lot of fun with go. 

[Devon H. O'Dell]

2012/5/22 <tali...@gmail.com>:

> I'd argue that slicing from i to j where j < i should give a more
> informative message than "panic: runtime error: slice bounds out of range".
>
> Perhaps it should say something like "panic: runtime error: slice has
> negative length"
>
> Or does this have performance implications?

This test is already currently accounted for in the if condition in
runtime.sliceslice / runtime.slicearray / string.goc where we test lb
> hb, so it shouldn't have any performance implications. The only
pushback I see is having yet another runtime.panicslice-like function,
which might muddy the API. Happy to supply a patch if people
@golang.org have a supportive opinion on approaching this matter.

--dho

[Paul Borman]

In Go the text returned with errors and panics are sometimes used when checking to see what error you had.  Code might exist that says:

defer func() {

    if p = recover(); p != nil {

        if re, ok := p.(error); ok && strings.Contains(re.Error, "slice bounds out of range") {

            // Oh, I can handle this!

        }

    }

}

Changing a well known error/panic string is sort of like creating a new errno.  I am not saying anyone is looking at this string, but someone certainly might be.  These messages should not be changed lightly.

[Taliesin Beynon]

Hi! Thanks for replying.

Here are my counterarguments to your objection:

0. One could avoid the objection entirely (at least for your specific code example) by adding a suffix to the string that describes exactly what went wrong: "slice bounds out of range: end is less than start", "slice bounds out of range: capacity exceeded", and so on.. 

1. The actual breakage to affected developers will be easy for them to notice and fix, because the now uncaught slice message will (presumably) reach the top level and they'll see it. 

2. Doing pure string matching of panic messages shouldn't be expected to be robust in the first place. It's a hack, and probably the language will adapt in future to provide a better way to match panic conditions.

3. I'd imagine that very few developers, if any, would be affected in practice. The fraction of slice index panics that are due to the upper index being below the lower index is probably quite small compared to the usual case, so the small fraction of developers who do panic string matching needs to multiplied by another small fraction to yield the truly affected people. 

So I've argued that very few people will be affected in practice, they probably should be coding more robustly in the first place, and it will be easy for them to work out what happened and fix it.

The reasons I think it is a good idea are:

1. "Out of bounds" is a misleading name for foo[4:3]. It is suggestive in the wrong direction. It caused me to start thinking along the wrong lines when doing debugging. If I had a more informative error message to begin with, I would have taken 1 minute to fix the bug, instead of 5. I bet that over the history of the language this will happen some quite large number of times, and add up to many hours (or more) of wasted developer time that could have been avoided with a better error message.

2. It would be bad to set the precedent that it is too late (at version 1.0.1) to enrich the panic messages. I imagine there will be other similar cases in the future. If ever there was time to establish better messages it would be now, while the language is still undergoing flux.

Sincerely,

Tali

[Paul Borman]

When a string is all you have then a string is all you must compare against.  I do not think Go is going to add constants so we can say:   if err == runtime.NegativeSliceLength.  Further, it does not work for error messages that contain context.

Appending doesn't solve it, either, depending how it is compared against.  I have been told that error strings will not change and they are safe to match against.  I am just raising a point you might not have thought of.  I am not saying a more descriptive panic message is bad, I am just saying there are consequences.  The Go team could very well decide if the risk in this case is minimal and the benefit large enough that they will change it.

    -Paul

[Devon H. O'Dell]

2012/5/22 Paul Borman <bor...@google.com>:

> When a string is all you have then a string is all you must compare against.

It's a good point. It's also worth noting that there are some packages
that behave in this way (e.g. fmt).

--dho

[Eleanor McHugh]

On 22 May 2012, at 23:15, Paul Borman wrote:
> In Go the text returned with errors and panics are sometimes used when checking to see what error you had. Code might exist that says:
>
> defer func() {
> if p = recover(); p != nil {
> if re, ok := p.(error); ok && strings.Contains(re.Error, "slice bounds out of range") {
> // Oh, I can handle this!
> }
> }
> }
>
> Changing a well known error/panic string is sort of like creating a new errno. I am not saying anyone is looking at this string, but someone certainly might be. These messages should not be changed lightly.

I check this a few times in sequences [0] as it's a handy short-circuit in slice iteration. I do however like the idea of the panic providing more information.


Ellie

[0] http://github.com/feyeleanor/sequences

Eleanor McHugh
Games With Brains
http://feyeleanor.tel
----
if _, ok := reality.(Reasonable); !ok { panic(reality) }

[Russ Cox]

On Tue, May 22, 2012 at 6:15 PM, Paul Borman <bor...@google.com> wrote:

> In Go the text returned with errors and panics are sometimes used when
> checking to see what error you had.  Code might exist that says:
>
> defer func() {
>     if p = recover(); p != nil {
>         if re, ok := p.(error); ok && strings.Contains(re.Error, "slice
> bounds out of range") {
>             // Oh, I can handle this!
>         }
>     }
> }
>
> Changing a well known error/panic string is sort of like creating a new
> errno.  I am not saying anyone is looking at this string, but someone
> certainly might be.  These messages should not be changed lightly.

I agree that it is not something to change lightly, but I also want to
point out that we have never made a promise that these will not
change. If you can convince us that there is a compelling reason to
detect a specific kind of runtime error, we'll make a specific type
for it in package runtime. Of course, that's not something that will
be changed lightly either, and I'm skeptical you'll convince us in
this case.

To answer the original request for more information here: I agree that
it would be great to have more information, but at the same time we
are trying to minimize both the performance impact and the size of the
generated code for the bounds checks. The main reason there's no
information is that we don't want to generate the code to record it
for every index or slice operation. Devon pointed out a few runtime
routines that can generate the message, but most of the time they are
bypassed in favor of generated code, and a pending CL will eliminate
them entirely. So every byte we save in the generated code for x[i] is
saved on every x[i] in your program.

If some day we get to the point where the compiler was discarding
bounds checks for, say, 99% of index or slice operations, then a case
might be made that it is cheap enough to record the information for
the remaining operations. But that's not where we are today.

Russ

[Daniel Morsing]

On Sat, 2 Jun 2012 20:53:26 -0400
Russ Cox <r...@golang.org> wrote:
> To answer the original request for more information here: I agree that
> it would be great to have more information, but at the same time we
> are trying to minimize both the performance impact and the size of the
> generated code for the bounds checks. The main reason there's no
> information is that we don't want to generate the code to record it
> for every index or slice operation. Devon pointed out a few runtime
> routines that can generate the message, but most of the time they are
> bypassed in favor of generated code, and a pending CL will eliminate
> them entirely. So every byte we save in the generated code for x[i] is
> saved on every x[i] in your program.
>

I'm not knowledgeable on the subject, but if you're generating the
panic code inline, couldn't you pull the index and length directly out
of the registers when panicing? Although, this approach would mean
generating new panic code for each of the slices that you're indexing in
a function, it wouldn't be in the fast path.

I don't suspect that we care that much about how fast we recover from
the panic.

Regards,
Daniel Morsing

[Daniel Morsing]

On Sat, 2 Jun 2012 20:53:26 -0400
Russ Cox <r...@golang.org> wrote:

> To answer the original request for more information here: I agree that
> it would be great to have more information, but at the same time we
> are trying to minimize both the performance impact and the size of the
> generated code for the bounds checks. The main reason there's no
> information is that we don't want to generate the code to record it
> for every index or slice operation. Devon pointed out a few runtime
> routines that can generate the message, but most of the time they are
> bypassed in favor of generated code, and a pending CL will eliminate
> them entirely. So every byte we save in the generated code for x[i] is
> saved on every x[i] in your program.
>

[Daniel Morsing]

Sorry about the mail spam, mailer went crazy

[bugpowder]

On Wednesday, May 23, 2012 6:15:35 AM UTC+8, Paul Borman wrote:

In Go the text returned with errors and panics are sometimes used when checking to see what error you had.  Code might exist that says:

Changing a well known error/panic string is sort of like creating a new errno.  I am not saying anyone is looking at this string, but someone certainly might be.  These messages should not be changed lightly.

Depending on error message phrasing seems like an EXTREMELY bad practice.

If anybody does that, he deserves his code to be broken.

That said, a better way is needed for the same purpose.

[Thomas Bushnell, BSG]

On Sun, Jun 3, 2012 at 6:20 AM, bugpowder <mit...@gmail.com> wrote:

Depending on error message phrasing seems like an EXTREMELY bad practice.

More to the point, the language specification explicitly says, "The exact error values that represent distinct run-time error conditions are unspecified."

Thomas

[Russ Cox]

On Sun, Jun 3, 2012 at 5:44 AM, Daniel Morsing <daniel....@gmail.com> wrote:
> I'm not knowledgeable on the subject, but if you're generating the
> panic code inline, couldn't you pull the index and length directly out
> of the registers when panicing? Although, this approach would mean
> generating new panic code for each of the slices that you're indexing in
> a function, it wouldn't be in the fast path.

You could. It's just more work and probably a little more code or
data. You would need a mapping from panic location to the relevant
registers.

Russ