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

Correct bounds checking for AbstractVectors with no indices specified (fixes #194) #195

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Jan 24, 2021

After this PR

julia> OffsetArray([0], 1)[]
ERROR: BoundsError: attempt to access 1-element OffsetArray(::Array{Int64,1}, 2:2) with eltype Int64 with indices 2:2 at index []

julia> OffsetArray([6], 1:1)[]
6

@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #195 (873cc30) into master (b20b4db) will decrease coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   99.28%   98.93%   -0.35%     
==========================================
  Files           5        5              
  Lines         280      283       +3     
==========================================
+ Hits          278      280       +2     
- Misses          2        3       +1     
Impacted Files Coverage Δ
src/axes.jl 100.00% <100.00%> (ø)
src/precompile.jl 92.85% <0.00%> (-7.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b20b4db...873cc30. Read the comment docs.

@yurivish
Copy link

Thank you for the quick fix!

Is it odd that coverage is reporting that the added Base.checkbounds_indices indices method is reported as not being covered by tests, even though the tests you've added presumably hit that method (otherwise they would not pass)?

@jishnub
Copy link
Member Author

jishnub commented Jan 24, 2021

It's perhaps some inlining quirk. I've added an explicit test, hopefully coverage will be happy now.

@johnnychen94 johnnychen94 changed the title Fix for issue 194 Correct indexing bounds checking (fixes #194) Jan 25, 2021
@jishnub jishnub changed the title Correct indexing bounds checking (fixes #194) Correct bounds checking for AbstractVectors with no indices specified (fixes #194) Jan 25, 2021
Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Nice detective work. No objection in merging this.

src/axes.jl Outdated Show resolved Hide resolved
@jishnub jishnub merged commit 1263513 into JuliaArrays:master Jan 25, 2021
@jishnub jishnub deleted the boundscheckfix branch January 25, 2021 07:01
@mbauman
Copy link
Member

mbauman commented Jan 25, 2021

The problem here isn't the bounds check. Indeed, the expression OffsetArray([6], 2:2)[] should work and return a 6. Where we are hardcoding a 1 is in the linear indexing transform:

julia> Base._to_linear_index(OffsetArray([6], 2:2))
1

This is where the problem is:

https://github.com/JuliaLang/julia/blob/69d24532980bda6db510b6d4b71f66e7ffff2e81/base/abstractarray.jl#L1196

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OffsetArray accesses out-of-bounds memory and returns an incorrect value when indexing with []
4 participants