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

[RFC] Add index hint to BoundsError message #43815

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
34 changes: 32 additions & 2 deletions base/errorshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(Note to self: Strings)

return print(io, '1')
end
print(io, "1:", x.stop)
end
show_index(io::IO, x::Colon) = print(io, ':')

function show_index(io::IO, x::Tuple)
if length(x) == 1
return show_index(io, only(x))
end
print(io, '[')
show_index(io, first(x))
for el in x[2:end]
print(io, ", ")
show_index(io, el)
end
print(io, ']')
end

function showerror(io::IO, ex::BoundsError)
print(io, "BoundsError")
Expand All @@ -55,10 +71,24 @@ function showerror(io::IO, ex::BoundsError)
end
print(io, ']')
end
print(io, ".\nLegal indices are ")
show_legal_indices(io, ex.a)
end
Experimental.show_error_hints(io, ex)
end

"""
show_legal_indices(io, x)

Describe legal ways to index `x` in human-readable form. This should be a continuation of
the sentence "Legal indices are ...". Will be shown to the user upon `BoundsError`.
"""
show_legal_indices(io::IO, x::Any) = print(io, "unknown.");
function show_legal_indices(io::IO, x::AbstractArray{<:Any})
show_index(io, axes(x))
print(io, '.')
end

function showerror(io::IO, ex::TypeError)
print(io, "TypeError: ")
if ex.expected === Bool
Expand Down
3 changes: 3 additions & 0 deletions base/strings/string.jl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ function Base.showerror(io::IO, exc::StringIndexError)
end
end
end
function show_legal_indices(io::IO, s::AbstractString)
print(io, "between ", firstindex(s), " and ", thisind(s, ncodeunits(s)), '.')
end

const ByteArray = Union{CodeUnits{UInt8,String}, Vector{UInt8},Vector{Int8}, FastContiguousSubArray{UInt8,1,CodeUnits{UInt8,String}}, FastContiguousSubArray{UInt8,1,Vector{UInt8}}, FastContiguousSubArray{Int8,1,Vector{Int8}}}

Expand Down
4 changes: 4 additions & 0 deletions base/tuple.jl
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,10 @@ function in(x::Symbol, itr::Tuple{Vararg{Symbol}})
return sym_in(x, itr)
end

function show_legal_indices(io::IO, x::Tuple)
show_index(io, axes(x))
print(io, '.')
end

"""
empty(x::Tuple)
Expand Down
1 change: 1 addition & 0 deletions doc/src/manual/interfaces.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,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) |
| `show_legal_indices(io, A)` | `Base.show_index(io, axes(A)); print(io, '.');` | Human readable continuation of the sentence "Legal indices are ...", for the `BoundsError` message |

If a type is defined as a subtype of `AbstractArray`, it inherits a very large set of rich behaviors
including iteration and multidimensional indexing built on top of single-element access. See
Expand Down
10 changes: 5 additions & 5 deletions test/arrayops.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2900,29 +2900,29 @@ end
b = IOBuffer()
showerror(b, err)
@test String(take!(b)) ==
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, 1:2]"
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, 1:2].\nLegal indices are [1:2, 1:2]."

err = try x[10, trues(2)]; catch err; err; end
b = IOBuffer()
showerror(b, err)
@test String(take!(b)) ==
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, 2-element BitVector]"
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, 2-element BitVector].\nLegal indices are [1:2, 1:2]."

# 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]."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


err = BoundsError(x, "bad index")
showerror(b, err)
@test String(take!(b)) ==
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [\"bad index\"]"
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [\"bad index\"].\nLegal indices are [1:2, 1:2]."

err = BoundsError(x, (10, "bad index"))
showerror(b, err)
@test String(take!(b)) ==
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, \"bad index\"]"
"BoundsError: attempt to access 2×2 Matrix{Float64} at index [10, \"bad index\"].\nLegal indices are [1:2, 1:2]."
end

@testset "inference of Union{T,Nothing} arrays 26771" begin
Expand Down
11 changes: 6 additions & 5 deletions test/errorshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ struct Bounded # not an AbstractArray
end
Base.getindex(b::Bounded, i) = checkindex(Bool, 1:b.bound, i) || throw(BoundsError(b, i))
Base.summary(io::IO, b::Bounded) = print(io, "$(b.bound)-size Bounded")
Base.show_legal_indices(io::IO, b::Bounded) = print(io, "1:$(b.bound).")
let undefvar
err_str = @except_strbt sqrt(-1) DomainError
@test occursin("Try sqrt(Complex(x)).", err_str)
Expand All @@ -297,17 +298,17 @@ let undefvar
@test occursin("DomainError with [0.0 -1.0 …", err_str)

err_str = @except_str (1, 2, 3)[4] BoundsError
@test err_str == "BoundsError: attempt to access Tuple{$Int, $Int, $Int} at index [4]"
@test err_str == "BoundsError: attempt to access Tuple{$Int, $Int, $Int} at index [4].\nLegal indices are 1:3."

err_str = @except_str [5, 4, 3][-2, 1] BoundsError
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [-2, 1]"
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [-2, 1].\nLegal indices are 1:3."
err_str = @except_str [5, 4, 3][1:5] BoundsError
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [1:5]"
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [1:5].\nLegal indices are 1:3."
err_str = @except_str [5, 4, 3][trues(6,7)] BoundsError
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [6×7 BitMatrix]"
@test err_str == "BoundsError: attempt to access 3-element Vector{$Int} at index [6×7 BitMatrix].\nLegal indices are 1:3."

err_str = @except_str Bounded(2)[3] BoundsError
@test err_str == "BoundsError: attempt to access 2-size Bounded at index [3]"
@test err_str == "BoundsError: attempt to access 2-size Bounded at index [3].\nLegal indices are 1:2."

err_str = @except_str 0::Bool TypeError
@test err_str == "TypeError: non-boolean ($Int) used in boolean context"
Expand Down