jw3126 / Setfield.jl

Update deeply nested immutable structs.
Other
167 stars 17 forks source link

Interoperability #21

Closed tkf closed 6 years ago

tkf commented 6 years ago

Not sure we want to actually merge this. It's just for start discussing interoperability with other packages, especially QuickTypes.jl.

tkf commented 6 years ago

Since QuickTypes generates a struct and a bunch of functions, we need to support blocks in @settable and ignore non-struct statements, if we are to support it. Also, using QuickTypes (especially @qstruct_fp) with Setfield is not straight forward since QuickTypes generates only "type-parameter -full" inner constructor. That is to say,

@qstruct_fp Plane(nwheels, weight::Number; brand=:zoomba)

generates an inner constructor

function (::Type{Plane{T1, T2, T3}}){T1 <: Any, T2 <: Number, T3 <: Any}(
        nwheels=nothing, weight=nothing; brand=:zoomba)
    return new{T1, T2, T3}(nwheels, weight, brand)
end

and an outer constructor

Plane{T1 <: Any, T2 <: Number, T3 <: Any}(
        nwheels::T1=nothing, weight::T2=nothing; brand::T3=:zoomba) =
    Plane{T1, T2, T3}(nwheels, weight; brand=brand)

If QuickTypes generated a type parameter-less inner constructor, i.e.,

function Plane(nwheels::T1 = nothing,
               weight::T2 = nothing;
               brand::T3 = :zoomba) where {T1 <: Any, T2 <: Number, T3 <: Any}
    return new{T1, T2, T3}(nwheels, weight, brand)
end

in addition to (or instead of) the above inner constructor, then @settable would have worked seamlessly. Not sure there is anything else we can do in Setfield.jl side.

This is the actual test code:

https://github.com/tkf/Setfield.jl/blob/9bcb9f8e639709b4b494f131c2b2c788d48541ac/test/test_quicktypes.jl#L94-L122

ping @cstjean

cstjean commented 6 years ago

If QuickTypes generated a type parameter-less inner constructor, i.e.,

Could you please show me which constructor call the @set macro makes, that should work but doesn't? I.e. what is @macroexpand @set x0.brand = 31 on this branch?

I haven't updated QuickTypes.jl for 0.7 yet...

jw3126 commented 6 years ago

Okay say x0::X{T1,T2}. Then @set x0.brand=31 will at some point call X(31, x0.other_field). Note that the parameters T1, T2 were stripped. (We choose to do this, so that @set can change the type, see here for an example where this is useful.)

Now this PR is about interoperability with customized types, which may not have positional constructors. The idea is to add a macro

@settable struct X{T1,T2}
    ...
end

that makes sure, that there is a X(brand, other_field) positional constructor. For this macro to work, we rely on the existence of an inner constructor of the form X(...), but QuickTypes only has one of the form X{T1,T2}(...).

cstjean commented 6 years ago

Would it help if I made the outer constructor an inner constructor?

jw3126 commented 6 years ago

Yes, that would help!

cstjean commented 6 years ago

I just moved the outer constructor inside the type definition, on the current master. It wasn't entirely trivial, so please report any issue you find, and let me know if it allows interoperability with this PR.

jw3126 commented 6 years ago

@cstjean cool, this is very much appreciated! Thanks a lot! @tkf are you still working on this? If not are you okay, if I take over this branch?

jw3126 commented 6 years ago

@cstjean Mhh I checked out the latest master of QuickTypes, but the outer constructor is still outside, it seems. We would need

@macroexpand @qstruct_fp Plane3(nwheels, weight::Number; brand=:zoomba)

expand to

   ...
    struct Plane3{T1 <: Any, T2 <: Number, T3 <: Any} <: Any
        nwheels::T1
        weight::T2
        brand::T3
        begin  # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 200:
            function (::Type{Plane3{T1, T2, T3}}){T1 <: Any, T2 <: Number, T3 <: Any}(nwheels, weight; brand=:zoomba) # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 202:
                nothing # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 203:
                return new{T1, T2, T3}(nwheels, weight, brand)
            end
            function Plane3(nwheels::T1 = nothing,
               weight::T2 = nothing;
               brand::T3 = :zoomba) where {T1 <: Any, T2 <: Number, T3 <: Any}
               return new{T1, T2, T3}(nwheels, weight, brand)
            end
        end
    end
    ...

The important points are

Instead current behavior is the following:

:($(Expr(:toplevel, quote 
    $(Expr(:meta, :doc))
    struct Plane3{T1 <: Any, T2 <: Number, T3 <: Any} <: Any
        nwheels::T1
        weight::T2
        brand::T3
        begin  # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 200:
            function (::Type{Plane3{T1, T2, T3}}){T1 <: Any, T2 <: Number, T3 <: Any}(nwheels, weight; brand=:zoomba) # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 202:
                nothing # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 203:
                return new{T1, T2, T3}(nwheels, weight, brand)
            end
        end
    end
end, :(Plane3{T1 <: Any, T2 <: Number, T3 <: Any}(nwheels::T1, weight::T2; brand::T3=:zoomba) = Plane3{T1, T2, T3}(nwheels, weight; brand=brand)), :(function QuickTypes.construct(::Type{Plane3}, nwheels, weight, brand) # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 218:
        nothing # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 219:
        Plane3(nwheels, weight; brand=brand)
    end), :(function Base.show(io::IO, obj::Plane3) # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 80:
        print(io, typeof(obj)) # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 81:
        write(io, "(") # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 83:
        begin 
            show(io, obj.nwheels)
            write(io, ", ")
        end
        begin 
            show(io, obj.weight)
            nothing
        end # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 87:
        write(io, "; ") # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 90:
        begin 
            write(io, "brand")
            write(io, "=")
            show(io, obj.brand)
            nothing
        end # /home/jan/.julia/v0.6/QuickTypes/src/QuickTypes.jl, line 94:
        write(io, ")")
    end), nothing)))
cstjean commented 6 years ago

Ahhh, very sorry about that, it seems that I forgot to merge my branch into master. Will do when I get home tonight.

jw3126 commented 6 years ago

No problem. Thanks again, I really like QuickTypes, looking forward to use it in conjunction with this package!

cstjean commented 6 years ago

Merged and pushed.

jw3126 commented 6 years ago

@cstjean It works now! At least the Plane3 example does. I will test more thoroughly after easter and tell you if I find any issues. Thanks again for making this possible!

codecov-io commented 6 years ago

Codecov Report

Merging #21 into master will increase coverage by 0.98%. The diff coverage is 88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   85.84%   86.82%   +0.98%     
==========================================
  Files           3        5       +2     
  Lines         106      205      +99     
==========================================
+ Hits           91      178      +87     
- Misses         15       27      +12
Impacted Files Coverage Δ
src/Setfield.jl 50% <ø> (ø) :arrow_up:
src/lens.jl 83.33% <50%> (-1.58%) :arrow_down:
src/macrotools.jl 84.31% <84.31%> (ø)
src/settable.jl 93.61% <93.61%> (ø)

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 42933d9...9bcb9f8. Read the comment docs.

jw3126 commented 6 years ago

I merged this PR, it works fine with the latest QuickTypes master.

tkf commented 6 years ago

@jw3126 Ooohhh... Thanks for finishing up this branch! (and sorry for absence)

@cstjean Thank you very much for reorganizing your package for Setfield.jl!

jw3126 commented 6 years ago

@cstjean I think it works fine! Can you tag a new release?

cstjean commented 6 years ago

Already did a few days ago.