Fix IndexLens for mutable arrays

[tkf]

This is alternative to #39. If we want to handle mutable arrays (such as Array) by always copying them, this PR provides an implementation for it. This is consistent with how nomut branch handles mutable struct.

[codecov-io]

Codecov Report

Merging #40 into nomut will increase coverage by 0.11%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           nomut      #40      +/-   ##
=========================================
+ Coverage   97.5%   97.61%   +0.11%     
=========================================
  Files          4        4              
  Lines        160      168       +8     
=========================================
+ Hits         156      164       +8     
  Misses         4        4

Impacted Files	Coverage Δ
src/lens.jl	100% <100%> (ø)	:arrow_up:

---

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 d8a8730...05179c8. Read the comment docs.

[tkf]

So my benchmark shows the static version is ~20 times faster.

@generated function setindex_dynamic(obj, val, indices...)
    quote
        if hasmethod(Base.setindex, $(Tuple{obj, val, indices...}))
            setter = Base.setindex
        else
            setter = setindex_on_copy
        end
        setter(obj, val, indices...)
    end
end

@generated function setindex_static(obj, val, indices...)
    if hasmethod(Base.setindex, Tuple{obj, val, indices...})
        setter = Base.setindex
    else
        setter = setindex_on_copy
    end
    quote
        $setter(obj, val, indices...)
    end
end

function setindex_on_copy(obj, val, indices...)
    clone = similar(obj, promote_type(eltype(obj), typeof(val)))
    copyto!(clone, obj)
    setindex!(clone, val, indices...)
    return clone
end

using BenchmarkTools
a = [1, 2, 3]
println("\n", "*** setindex_dynamic")
display(@benchmark setindex_dynamic($a, 1, 1))
println("\n", "*** setindex_static")
display(@benchmark setindex_static($a, 1, 1))

using InteractiveUtils
versioninfo()

Output:

*** setindex_dynamic
BenchmarkTools.Trial:
  memory estimate:  144 bytes
  allocs estimate:  2
  --------------
  minimum time:     473.071 ns (0.00% GC)
  median time:      476.987 ns (0.00% GC)
  mean time:        529.616 ns (5.03% GC)
  maximum time:     177.103 μs (99.55% GC)
  --------------
  samples:          10000
  evals/sample:     196
*** setindex_static
BenchmarkTools.Trial:
  memory estimate:  112 bytes
  allocs estimate:  1
  --------------
  minimum time:     25.980 ns (0.00% GC)
  median time:      28.185 ns (0.00% GC)
  mean time:        38.229 ns (17.35% GC)
  maximum time:     38.632 μs (99.82% GC)
  --------------
  samples:          10000
  evals/sample:     992Julia Version 1.0.0
Commit 5d4eaca0c9 (2018-08-08 20:58 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

[tkf]

I tried to invoke the bug in the hasmethod hack but the only way I can invoke the bug is to write somewhat unrealistic code (see struct B below). Here is what I tried.

module TestHasMethodHack

using Test

function setindex_on_copy(obj, val, indices...)
    clone = similar(obj, promote_type(eltype(obj), typeof(val)))
    copyto!(clone, obj)
    setindex!(clone, val, indices...)
    return clone
end

struct A
    x
end

Base.setindex(::A, x, ::Integer) = A(x)

@generated function setindex_static(obj, val, indices...)
    if hasmethod(Base.setindex, Tuple{obj, val, indices...})
        setter = Base.setindex
    else
        setter = setindex_on_copy
    end
    quote
        $setter(obj, val, indices...)
    end
end

@assert setindex_static([0], 1, 1) == [1]  # Having it here (or commenting it out) does not change anything.

struct B
    x
end

try
    setindex_static(B(1), 2, 1)
catch err
    @info "Ignoring" exception=err
end

Base.setindex(::B, x, ::Integer) = B(x)

struct C
    x
end

Base.setindex(::C, x, ::Integer) = C(x)

@testset begin
    @test setindex_static(A(0), 1, 1) == A(1)
    @test_broken setindex_static(B(0), 1, 1) == B(1)
    @test setindex_static(C(0), 1, 1) == C(1)
end

end  # module

Output:

┌ Info: Ignoring
│   exception =
│    MethodError: no method matching similar(::Main.TestHasMethodHack.B, ::Type{Any})
│    Closest candidates are:
│      similar(::Array{T,1}, ::Type) where T at array.jl:329
│      similar(::Array{T,2}, ::Type) where T at array.jl:330
│      similar(::Array, ::Type, ::Tuple{Vararg{Int64,N}}) where N at array.jl:332
└      ...
Test Summary: | Pass  Broken  Total
test set      |    2       1      3

[jw3126]

Thanks for the example, I agree it is somewhat exotic. But this kind of thing comes up in REPL sessions. Is setindex on Vector important to you? I would prefer removing it over having a slow / slightly buggy version. But if it is important to you, we can support it.

[tkf]

Is setindex on Vector important to you?

Nope :) It is just me trying to make test_quicktypes.jl pass.

I would prefer removing it over having a slow / slightly buggy version.

Totally agree. Let's pull #39 then!

[tkf]

I asked the question about @generated here https://discourse.julialang.org/t/can-i-use-hasmethod-inside-generated-function/13645?u=tkf