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

setproperty!! dynamic dispatch #226

Closed
cscherrer opened this issue Feb 25, 2022 · 2 comments · Fixed by #227
Closed

setproperty!! dynamic dispatch #226

cscherrer opened this issue Feb 25, 2022 · 2 comments · Fixed by #227

Comments

@cscherrer
Copy link

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
Copy link
Author

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
Copy link
Member

tkf commented Feb 26, 2022

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.

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 a pull request may close this issue.

2 participants