[RFC] Add index hint to BoundsError message #43815

gustaphe · 2022-01-14T22:11:37Z

I suggested a more human friendly error message in this thread on discourse. Thought I'd try it out -- does this implementation make sense to you?

julia> [1,2,3][4]
ERROR: BoundsError: attempt to access 3-element Vector{Int64} at index [4].
Legal indices are 1:3.

julia> "åäö"[7]
ERROR: BoundsError: attempt to access 6-codeunit String at index [7].
Legal indices are between 1 and 5.

julia> rand(5,5)[1,3:6]
ERROR: BoundsError: attempt to access 5×5 Matrix{Float64} at index [1, 3:6].
Legal indices are [1:5, 1:5].

N5N3 · 2022-01-15T07:54:50Z

test/arrayops.jl


    # Also test : directly for custom types for which it may appear as-is
    err = BoundsError(x, (10, :))
    showerror(b, err)
    @test String(take!(b)) ==
-        "BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :]"
+        "BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :].\nLegal indices are [1:3, 1:3]."


Suggested change

"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :].\nLegal indices are [1:3, 1:3]."

"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :].\nLegal indices are [1:2, 1:2]."

@gustaphe Thank you for your contribution and welcome to the community. This does make error messege clearer!
Beside the the above typo. I guess you also need to fix the test in Test and resolve the mismatches in doc examples.

Sorry, I have a hard time running the tests, they end up running my laptop's memory into the ground and causing a reboot. I thought I found a subset of the tests that would catch everything but sadly some slipped through. I don't want it to seem like I'm brute force debugging this by spamming commits, but I don't have any other method of knowing it works so I kind of am.

It is okay to brute force it with CI time. We usually have a little bit of excess. This seems like a good idea. It might conflict eventually with #43738, but let's cross the bridge later, and hopefully merge this sooner.

jishnub · 2022-01-21T12:28:08Z

English isn't my first language, but 'Legal indices' sounds too strong an admonition to me, perhaps 'Valid indices' would sound more pleasant while conveying the same message?

gustaphe · 2022-01-23T06:15:03Z

Mine either, but you're probably right.

I was informed of the existing error hint system, not sure if this is much better but it's maybe a little cleaner.

I also added some more documentation under "interfaces".

The remaining failing build I think has nothing to do with this PR, right? I don't know how to fix it either way.

KristofferC · 2022-05-12T06:08:01Z

I think this is a good idea but it feels a bit "spammy" to print it for e.g. normal arrays. Maybe it should only be printed when the axis is not the standard 1:length(x)?

gustaphe · 2022-05-12T12:21:25Z

Good shout. I've left the hint in for tuples, because BoundsError: attempt to access Tuple{Int64, Int64, Int64} at index [4] is not quite as obvious to me as BoundsError: attempt to access 3-element Vector{Int64} at index [4] (especially if we imagine the tuple getting longer).

oscardssmith · 2022-05-12T13:44:26Z

IMO this isn't too spammy (especially because Julia is 1 indexed). experienced users are unlikely to see many bounds errors (cause they will write good code), so this is a feature that will mostly be seen by users new to Julia (or programming in general). Getting a good error message the first time you try to take [1,2,3][0] will be very nice.

gustaphe · 2022-05-13T15:21:49Z

Come to think of it I agree. Went back and read the motivation for doing this to begin with, and the latest commit would kind of ruin the main advantage.

But I'm deferring the decision, happy to implement whichever version is favored by maintainers. But I'll hold off on sorting out the currently failing tests until then.

oscardssmith · 2022-09-19T20:08:39Z

adding a triage label to decide if we want to do this.

gbaraldi · 2023-01-05T21:33:07Z

Triage likes this :)

This reverts commit d2c850c.

KristofferC · 2023-01-07T17:31:08Z

base/abstractarray.jl

@@ -744,7 +744,7 @@ checkbounds_indices(::Type{Bool}, ::Tuple{}, ::Tuple{}) = true
 throw_boundserror(A, I) = (@noinline; throw(BoundsError(A, I)))

 function describe_valid_indices(io::IO, a::AbstractArray{<:Any}, i=nothing)
-    print(io, "Valid indices are ")
+    print(io, "\nValid indices are ")


Imo, there should not be a new line here, cf

julia> "fαb"[3] ERROR: StringIndexError: invalid index [3], valid nearby indices [2]=>'α', [4]=>'b'

fredrikekre · 2023-01-07T21:40:30Z

base/errorshow.jl

@@ -32,9 +32,25 @@ showerror(io::IO, ex) = show(io, ex)
 show_index(io::IO, x::Any) = show(io, x)
 show_index(io::IO, x::Slice) = show_index(io, x.indices)
 show_index(io::IO, x::LogicalIndex) = summary(io, x.mask)
-show_index(io::IO, x::OneTo) = print(io, "1:", x.stop)
+function show_index(io::IO, x::OneTo)
+    if x.stop == 1


Why not consistently print as start:stop no matter then length like before? Also for Tuple below.

Maybe. Just a style choice but it didn't really matter to me. I'll fix.

(Note to self: Strings)

fredrikekre · 2023-01-07T21:41:00Z

base/errorshow.jl

@@ -59,6 +75,27 @@ function showerror(io::IO, ex::BoundsError)
    Experimental.show_error_hints(io, ex)
 end

+Experimental.register_error_hint(BoundsError) do io, ex
+    if ~isdefined(ex, :a)
+        return print(io, "\nNo description of valid indices available.")


IMO it is not useful to be told that there is no information to be told.

My thinking was that this would prompt package authors to extend this function, but you're right, this is useless.

fredrikekre · 2023-01-07T21:45:37Z

doc/src/manual/interfaces.md

@@ -236,6 +236,7 @@ ourselves, we can officially define it as a subtype of an [`AbstractArray`](@ref
 | `axes(A)`                                    | `map(OneTo, size(A))`                  | Return a tuple of `AbstractUnitRange{<:Integer}` of valid indices                    |
 | `similar(A, ::Type{S}, inds)`              | `similar(A, S, Base.to_shape(inds))`   | Return a mutable array with the specified indices `inds` (see below)                  |
 | `similar(T::Union{Type,Function}, inds)`   | `T(Base.to_shape(inds))`               | Return an array similar to `T` with the specified indices `inds` (see below)          |
+| `describe_valid_indices(io, A, i)` |  | Human readable version of the sentence "Valid indices are ...", for the `BoundsError` message |


For AbstractArrays a default fallback which shows axes should be enough, no? Feels pretty strange that with the basic methods above you get everything from sorting to matrix multiplication working, but you have to define this extremely specific thing.

There is such a fallback, this is in the "optional" section.

fredrikekre · 2023-01-07T21:47:00Z

base/array.jl

 ERROR: BoundsError: attempt to access 3-element Vector{Int64} at index [4]
+Valid indices are 1:3.


I agree with the suggestion to only do something extra when indices are non-standard. But if something more is added, perhaps the default BoundsError message should instead be

Suggested change

ERROR: BoundsError: attempt to access 3-element Vector{Int64} at index [4]

Valid indices are 1:3.

ERROR: BoundsError: attempt to access 3-element Vector{Int64} with axes $(axes(...)) at index [4]

+1 for adding things into the running text, just like the existing index hints for strings:

julia> "fαb"[3] ERROR: StringIndexError: invalid index [3], valid nearby indices [2]=>'α', [4]=>'b'

KristofferC · 2023-01-07T22:03:48Z

IMO this isn't too spammy (especially because Julia is 1 indexed).

I don't see how Julia being 1 indexed has anything to do with the level of spamminess this has.

experienced users are unlikely to see many bounds errors (cause they will write good code), so this is a feature that will mostly be seen by users new to Julia (or programming in general).

I don't think this is true and the same can be said for any error message so it isn't a good criteria to use for evaluating error messages.

Getting a good error message the first time you try to take [1,2,3][0] will be very nice.

In that case it can be special cased to only show something when the index is 0.

Again, in probably 99% of the cases the indices will be 1:length(x) where this gives zero extra information over the existing printing. Therefore, it makes sense to show this only in the special case where it actually adds some extra information.

gustaphe · 2023-01-08T20:30:29Z

Here's a version that causes fewer messages, basically it currently only hints when

You use 0 to index a OneTo axis AbstractArray or a String
You index out-of-bounds for an AbstractArrray with non-OneTo axes
You index out-of-bounds for a container type with a defined describe_valid_indices-method

Is that better? I don't know which parts of this triage liked and didn't. This will at least help whenever you do a[0] - and opens an interface for non-standard indexed arrays to provide the hint.

Add index hint to BoundsError message

2b4bc40

N5N3 added arrays [a, r, r, a, y, s] error messages Better, more actionable error messages labels Jan 15, 2022

N5N3 reviewed Jan 15, 2022

View reviewed changes

gustaphe added 3 commits January 16, 2022 17:08

Update tests and docs

fb986ec

Fix doctest

6a2a0e4

Fix another test typo

93bc6b7

gustaphe added 2 commits January 22, 2022 18:23

Legal->Valid, use register_error_hint, expand docs

06f7525

Doc typo

30e07e2

Remove error hint for one-to arrays

d2c850c

oscardssmith added the triage This should be discussed on a triage call label Sep 19, 2022

Merge branch 'master' into boundshint

a660b27

gbaraldi removed the triage This should be discussed on a triage call label Jan 5, 2023

gustaphe added 4 commits January 7, 2023 15:10

Revert "Remove error hint for one-to arrays"

c1eb7e9

This reverts commit d2c850c.

More newlines

cb939a3

Remove "between 1 and 1"

3a161bf

Docstring typo

2c880d6

KristofferC reviewed Jan 7, 2023

View reviewed changes

fredrikekre reviewed Jan 7, 2023

View reviewed changes

Fewer index hints, special-case 0

61b7d15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add index hint to BoundsError message #43815

[RFC] Add index hint to BoundsError message #43815

gustaphe commented Jan 14, 2022

N5N3 Jan 15, 2022

N5N3 Jan 15, 2022

gustaphe Jan 19, 2022

vtjnash Jan 19, 2022

jishnub commented Jan 21, 2022 •

edited

Loading

gustaphe commented Jan 23, 2022

KristofferC commented May 12, 2022 •

edited

Loading

gustaphe commented May 12, 2022

oscardssmith commented May 12, 2022

gustaphe commented May 13, 2022

oscardssmith commented Sep 19, 2022

gbaraldi commented Jan 5, 2023

KristofferC Jan 7, 2023

fredrikekre Jan 7, 2023

gustaphe Jan 7, 2023

fredrikekre Jan 7, 2023

gustaphe Jan 7, 2023

fredrikekre Jan 7, 2023

gustaphe Jan 7, 2023

fredrikekre Jan 7, 2023

KristofferC Jan 7, 2023

KristofferC commented Jan 7, 2023

gustaphe commented Jan 8, 2023

	"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :].\nLegal indices are [1:3, 1:3]."
	"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, :].\nLegal indices are [1:2, 1:2]."

		ERROR: BoundsError: attempt to access 3-element Vector{Int64} at index [4]
		Valid indices are 1:3.

[RFC] Add index hint to BoundsError message #43815

Are you sure you want to change the base?

[RFC] Add index hint to BoundsError message #43815

Conversation

gustaphe commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jishnub commented Jan 21, 2022 • edited Loading

gustaphe commented Jan 23, 2022

KristofferC commented May 12, 2022 • edited Loading

gustaphe commented May 12, 2022

oscardssmith commented May 12, 2022

gustaphe commented May 13, 2022

oscardssmith commented Sep 19, 2022

gbaraldi commented Jan 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Jan 7, 2023

gustaphe commented Jan 8, 2023

jishnub commented Jan 21, 2022 •

edited

Loading

KristofferC commented May 12, 2022 •

edited

Loading