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

checkbounds doesn't obey API for strings #24840

Closed
StefanKarpinski opened this issue Nov 29, 2017 · 6 comments · Fixed by #25086
Closed

checkbounds doesn't obey API for strings #24840

StefanKarpinski opened this issue Nov 29, 2017 · 6 comments · Fixed by #25086
Assignees
Labels
strings "Strings!"
Milestone

Comments

@StefanKarpinski
Copy link
Member

The documentation for checkbounds says that it returns true if an index (or indices) are in bounds:

checkbounds(Bool, A, I...)

Return true if the specified indices I are in bounds for the given array A. Subtypes of AbstractArray should specialize this method if they need to provide custom bounds checking behaviors; however, in many cases one can rely on A's indices and checkindex.

However, the methods seem to variously return nothing or throw an error, return true or throw an error and maybe somewhere there's a method that actually does what the docs say.

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Nov 29, 2017
@timholy
Copy link
Member

timholy commented Nov 29, 2017

julia> A = rand(2,2)
2×2 Array{Float64,2}:                                                                                                                                                                                                                        
 0.106665   0.653657                                                                                                                                                                                                                         
 0.0492862  0.985517                                                                                                                                                                                                                         
                                                                                                                                                                                                                                             
julia> checkbounds(Bool, A, 1, 1)
true                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                             
julia> checkbounds(Bool, A, 1, 3)
false                                                                                                                                                                                                                                        
                                                                                                                                                                                                                                             
julia> checkbounds(A, 1, 1)
                                                                                                                                                                                                                                                                   
julia> checkbounds(A, 1, 3)
ERROR: BoundsError: attempt to access 2×2 Array{Float64,2} at index [1, 3]                                                                                                                                                                                         
Stacktrace:                                                                                                                                                                                                                                                        
 [1] throw_boundserror(::Array{Float64,2}, ::Tuple{Int64,Int64}) at ./abstractarray.jl:434                                                                                                                                                                         
 [2] checkbounds(::Array{Float64,2}, ::Int64, ::Int64) at ./abstractarray.jl:399                                                                                                                                                                                   
 [3] top-level scope                                                                                                                                                                                                                                               

OK to close?

@rfourquet
Copy link
Member

The checkbounds(A, I...) method is at the end of the help, after an "Examples" section, so it's easy to miss (I think there was a PR to make clearer the separation between the doc of different methods).

@StefanKarpinski
Copy link
Member Author

It's still a mess for strings:

julia> checkbounds("foo", 1)
true

julia> checkbounds("foo", 0)
ERROR: BoundsError: attempt to access "foo"
  at index [0]
Stacktrace:
 [1] checkbounds(::String, ::Int64) at ./strings/basic.jl:391
 [2] top-level scope

julia> checkbounds(Bool, "foo", 0)
ERROR: MethodError: no method matching checkbounds(::Type{Bool}, ::String, ::Int64)
Closest candidates are:
  checkbounds(::Type{Bool}, ::AbstractArray, ::Any) at abstractarray.jl:383
  checkbounds(::Type{Bool}, ::AbstractArray, ::Any...) at abstractarray.jl:378
  checkbounds(::AbstractArray, ::Any...) at abstractarray.jl:398
  ...

@timholy timholy changed the title checkbounds is a mess checkbounds doesn't obey API for strings Nov 29, 2017
@JeffBezanson JeffBezanson added the strings "Strings!" label Nov 29, 2017
@JeffBezanson
Copy link
Member

Seems straightforward enough to fix. If we bring strings in line with other types here do we think it will break anything?

@StefanKarpinski
Copy link
Member Author

Probably not? Hard to say without trying it.

@nalimilan
Copy link
Member

Cc: @bkamins

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Nov 30, 2017
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Nov 30, 2017
@StefanKarpinski StefanKarpinski self-assigned this Nov 30, 2017
StefanKarpinski added a commit that referenced this issue Dec 14, 2017
fix #24840 all the way, already mostly fixed by #24999
StefanKarpinski added a commit that referenced this issue Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings "Strings!"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants