JuliaFolds / BangBang.jl

Immutables as mutables, mutables as immutables.
MIT License
108 stars 11 forks source link

`setproperty!!` dynamic dispatch #226

Closed cscherrer closed 2 years ago

cscherrer commented 2 years ago

I haven't worked through the details, but I'd think this can be made to dispatch statically:

julia> using JET, BangBang

julia> nt = (a=1, b=2)
(a = 1, b = 2)

julia> setproperty!!(nt, :a, 3)
(a = 3, b = 2)

julia> @test_opt setproperty!!(nt, :a, 3)
JET-test failed at REPL[143]:1
  Expression: #= REPL[143]:1 =# JET.@test_call analyzer = JET.OptAnalyzer setproperty!!(nt, :a, 3)
  ═════ 2 possible errors found ═════
  ┌ @ /home/chad/.julia/packages/BangBang/C0c8y/src/base.jl:571 %2(%3)
  │ runtime dispatch detected: %2::Type{NamedTuple{_A}} where _A(%3::Tuple{Int64})
  └─────────────────────────────────────────────────────────────
  ┌ @ /home/chad/.julia/packages/BangBang/C0c8y/src/core.jl:11 %5(value, %4)
  │ runtime dispatch detected: %5::typeof(setproperties)(value::NamedTuple{(:a, :b), Tuple{Int64, Int64}}, %4::NamedTuple{_A, Tuple{Int64}} where _A)
  └────────────────────────────────────────────────────────────

ERROR: There was an error during testing
cscherrer commented 2 years ago

For context, I'm mainly using Accessors.jl, and wanted a Lens!! to make things work similarly to BangBang. Using BangBang directly gives the above error, but it seems to fix things if I instead do


@inline function Accessors.set(o, l::Lens!!{PropertyLens{prop}}, val) where {prop}
    if ismutable(o)
        setproperty!(o, prop, val)
    else
        setproperties(o, NamedTuple{(prop,)}((val,)))
    end
end

Maybe a similar change can help BangBang?

tkf commented 2 years ago

Uh, I totally forgot that I haven't still made BangBang compatible with Accessors

For the problem in the OP, I think we need to help inference somewhere to constprop :a.

BTW, for testing API like setproperty!! that requries constprop, you'd need to create a function like f!!(nt) = setproperty!!(nt, :a, 3) and pass it to the inference. IIUC it's the same for JET.