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

BangBang._setindex! is broken on Julia 1.7 for undef entries #21

Closed
torfjelde opened this issue Apr 20, 2024 · 2 comments
Closed

BangBang._setindex! is broken on Julia 1.7 for undef entries #21

torfjelde opened this issue Apr 20, 2024 · 2 comments

Comments

@torfjelde
Copy link
Contributor

MWE on Julia 1.7.3

julia> using BangBang

julia> xs = Vector{Vector{Float64}}(undef, 2)
2-element Vector{Vector{Float64}}:
 #undef
 #undef

julia> BangBang.@set!! xs[1] = [1.0]
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex
   @ ./array.jl:861 [inlined]
 [2] copy!(dst::Vector{Vector{Float64}}, src::Vector{Vector{Float64}})
   @ Base ./abstractarray.jl:874
 [3] _setindex
   @ ~/.julia/packages/BangBang/g5v4f/src/NoBang/base.jl:133 [inlined]
 [4] may
   @ ~/.julia/packages/BangBang/g5v4f/src/core.jl:11 [inlined]
 [5] setindex!!
   @ ~/.julia/packages/BangBang/g5v4f/src/base.jl:478 [inlined]
 [6] set(obj::Vector{Vector{Float64}}, lens::BangBang.AccessorsImpl.Lens!!{Accessors.IndexLens{Tuple{Int64}}}, value::Vector{Float64})
   @ BangBang.AccessorsImpl ~/.julia/packages/BangBang/g5v4f/src/accessors.jl:35 [7] top-level scope
   @ REPL[42]:1

while on Julia 1.10 it works without issues.

It comes down to the usage of copy!(ys, xs) here

function _setindex(xs::AbstractArray, v, I...)
T = promote_type(eltype(xs), typeof(v))
ys = similar(xs, T)
if eltype(xs) !== Union{}
copy!(ys, xs)
end
ys[I...] = v
return ys
end

which, on Julia 1.7 requires getindex:

function copy!(dst::AbstractVector, src::AbstractVector)
    if length(dst) != length(src)
        resize!(dst, length(src))
    end
    for i in eachindex(dst, src)
        @inbounds dst[i] = src[i]
    end
    dst
end

while on Julia 1.10 we hit

function copy!(dst::AbstractVector, src::AbstractVector)
    firstindex(dst) == firstindex(src) || throw(ArgumentError(
        "vectors must have the same offset for copy! (consider using `copyto!`)"))
    if length(dst) != length(src)
        resize!(dst, length(src))
    end
    copyto!(dst, src)
end

and eventually

function _copyto_impl!(dest::Array, doffs::Integer, src::Array, soffs::Integer, n::Integer)
    n == 0 && return dest
    n > 0 || _throw_argerror("Number of elements to copy must be nonnegative.")
    @boundscheck checkbounds(dest, doffs:doffs+n-1)
    @boundscheck checkbounds(src, soffs:soffs+n-1)
    unsafe_copyto!(dest, doffs, src, soffs, n)
    return dest
end

where the unsafe_copyto! saves us.

I'm not really certain if this is a "bug" on the end of BangBang.jl or if it's a bug in copy! on Julia 1.7 😕

Manifest.toml
  [7d9f7c33] Accessors v0.1.36
  [198e06fe] BangBang v0.4.1
  [34da2185] Compat v4.14.0
  [a33af91c] CompositionsBase v0.1.2
  [187b0558] ConstructionBase v1.5.5
  [22cec73e] InitialValues v0.3.1
  [3587e190] InverseFunctions v0.1.13
  [1914dd2f] MacroTools v0.5.13
  [ae029012] Requires v1.3.0
  [56f22d72] Artifacts
  [2a0f44e3] Base64
  [ade2ca70] Dates
  [b77e0a4c] InteractiveUtils
  [8f399da3] Libdl
  [37e2e46d] LinearAlgebra
  [56ddb016] Logging
  [d6f4376e] Markdown
  [de0858da] Printf
  [9a3f8284] Random
  [ea8e919c] SHA
  [9e88b42a] Serialization
  [fa267f1f] TOML
  [8dfed614] Test
  [cf7118a7] UUIDs
  [4ec0a83e] Unicode
  [e66e0078] CompilerSupportLibraries_jll
  [4536629a] OpenBLAS_jll
  [8e850b90] libblastrampoline_jll
@torfjelde
Copy link
Contributor Author

torfjelde commented Apr 20, 2024

Note that this example shouldn't really hit the NoBang implementation in the first place 😬

But it does because some knobhead (me) introduced this check:

BangBang.jl/src/base.jl

Lines 520 to 525 in cc65483

# This will still return `false` for scenarios such as
#
# setindex!!([1, 2, 3], [4, 5, 6], :, 1)
#
# which are in fact valid. However, this cases are rare.
(_index_dimension(indices) == M || _index_dimension(indices) == 1)

which returns false in this scenario because M = 1 but _index_dimension(indices) = 0.

EDIT: Though it seems this was a concern in the original PR (though that was just based on a hunch) #16 (comment)

@torfjelde
Copy link
Contributor Author

Fixed by #22

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

No branches or pull requests

1 participant