Make use of ConstructionBase

[jw3126]

Various packages implement variants of Parameters.reconstruct. ConstructionBase.jl is an effort to unify these. Benefits are:

• Better performance: ConstructionBase.setproperties is very fast
• More dry
• Better interoperability: For instance if a user wants a type with customized getproperty to interact well with both Setfield.jl and Parameters.jl, she only needs to overload one function.

See also JuliaObjects/ConstructionBase.jl#6
ConstructionBase is not yet released, so CI will fail and this PR should not yet be merged.

[tkf]

@mauro3 Just as a fair warning, ConstructionBase.setproperties uses a lot of @generated. I'm just bringing it up as this concern was mentioned in #50. I opened an issue JuliaObjects/ConstructionBase.jl#21 to discuss how we can make the situation better.

[mauro3]

@jw3126 thanks for the PR! Yes, making things more DRY and faster is certainly appreciated.

@tkf that is a good point.

Just to let you know, I will be on holidays the next two weeks. Don't expect to hear from me.

[jw3126]

@mauro3 Thanks for telling us, I wish you happy holidays!
@tkf thanks for mentioning. It did not occur to me, that this could be a concern.

[jw3126]

@mauro3 what do you think about this PR?

[tkf]

How about adding ConstructionBase = "0.1, 1.0"? (I think CompatHelper can figure out 1.0 but I'm not sure if it adds 0.1)

[mauro3]

Aright, sorry for the long radio-silence. I answer, just as I'm about to go on holidays again...

I'm a bit hesitant to take on the dependency of ConstructionBase.jl. Is my understanding right, that if someone wants good performance, then there is nothing in the way of them just using ConstructionBase.setproperties with Parameters.jl. If someone doesn't care, then they would be just as happy with the current situation and using reconstruct (and with one dependency less).

• If this is the case, then I would prefer to just do a doc-string update, to give an example of how to use ConstructionBase.setproperties.
• If this is not the case, do you have an example where this is not the case? (Note that Remove problematic "reconstruct"-constructor #116 would have been such a case but should be removed.)

[jw3126]

I guess that is correct. Are you open to have some tests that test that setproperties actually works on types created by your macro?

[mauro3]

Yes, I have no reservations to have ConstructionBase as a test-dependency.

[tkf]

I think of this PR more as for package composition rather than performance. For example, I think we need this PR to make this scenario work:

• Package A has TypeA overloading ConstructionBase.setproperties
• Package B has function_b using reconstruct
• Package C uses function_b with TypeA

[mauro3]

True, but note that

• package C works as is, albeit slower
• this can also be solved by package B using ConstructionBase.setproperties

My hunch is that this is fairly rare (but I might be wrong) and has an easy fix, and thus imposing the extra dependency is not worth it.

[tkf]

I should've set up the scenario more accurately. What I meant to say was that TypeA might be overloading Base.getproperty (and Base.propertynames). Then, if we have function_b that uses Base.getproperty, e.g.,

function_b(x) = reconstruct(x, alpha = x.alpha + 1)

package C cannot safely call function_b(::TypeA) because field alpha for TypeA may not exist.

[mauro3]

Ok, I don't quite understand ConstructionBase. How do properties even feature in ConstructionBase? setproperties uses fieldnames https://github.com/JuliaObjects/ConstructionBase.jl/blob/2bb8db024876adb9372f7dd86d197b101999fe4f/src/ConstructionBase.jl#L48, just like reconstruct.

[tkf]

ConstructionBase.setproperties is meant to be a canonical customization point for "out-of-place property setter." See also: "Implementation" section in the manual https://juliaobjects.github.io/ConstructionBase.jl/dev/#ConstructionBase.setproperties

Here is a concrete toy example:

module A
using ConstructionBase

struct TypeA{x,y,z} end
TypeA(x, y, z) = TypeA{x,y,z}()

Base.getproperty(::TypeA{x,y,z}, name::Symbol) where {x,y,z} =
    if name === :x
        x
    elseif name === :y
        y
    elseif name === :z
        z
    else
        throw(ArgumentError("Unknown property $name"))
    end

Base.propertynames(::TypeA) = (:x, :y, :z)

function ConstructionBase.setproperties(::TypeA{x0,y0,z0}, nt::NamedTuple) where {x0,y0,z0}
    props = merge((x = x0, y = y0, z = z0), nt)
    if keys(props) !== (:x, :y, :z)
        throw(ArgumentError("Unknown properties in: $nt"))
    end
    return TypeA(props.x, props.y, props.z)
end
end  # module A


# I'm pretending that package `B` uses `Parameters.reconstruct`:
module B
# using Parameters

using ConstructionBase
const reconstruct = setproperties

function_b(obj) = reconstruct(obj, x = obj.x + 1)
end  # module B


# Package `C` uses packages `A` and `B` without knowing they are using
# `ConstructionBase`/`Parameters`.
module C
using ..A: TypeA
using ..B: function_b

demo() = @assert function_b(TypeA(0, 2, 3)) === TypeA(1, 2, 3)
end  # module C


C.demo()

[mauro3]

Thanks for the detailed example! (Maybe something like this could go into your docs, as they are a bit on the theoretical side so far.) But in the example, package B could change from it using reconstruct to setproperties. That way, still all @with_kw types would work and also TypeA.

I guess, I kinda see reconstruct as something to be used mostly with @with_kw-types and not as a comprehensive solution. Thus my hesitance to make it into that.

OT: what is setproperties supposed to do when a unsettable property is attempted to be set, error presumably?

[tkf]

I guess, I kinda see reconstruct as something to be used mostly with @with_kw-types and not as a comprehensive solution.

I see, I think that makes sense. Indeed, package B just has to use setproperties.

Maybe something like this could go into your docs, as they are a bit on the theoretical side so far.

Thanks for the suggestion. That's a good idea.

what is setproperties supposed to do when a unsettable property is attempted to be set, error presumably?

Yeah, I think throwing an error makes sense.

[mauro3]

Another possibility would be to remove reconstruct from Parameters.jl and just advice to use ConstructionBase or Setfield. Although, as reconstruct is pretty essential, that would be almost identical to just including ConstructionBase.

Or, we go down the route of defining custom methods for constructorof and setproperties within the @with_kw, to alleviate my generated-function anxiety (but leave the dependency on ConstructionBase).

[tkf]

Yeah, you can overload constructorof and setproperties via @with_kw. As this macro knows the full spec of the struct, you can define them without using generated functions. I think it'd be nice, as it helps people using ConstructionBase even if they do not "like" generated functions.

leave the dependency on ConstructionBase

But Parameters.jl still needs to depend on ConstructionBase.jl if you want to overload constructorof and setproperties. Do you mean Parameters.jl users don't have to know that Parameters.jl depends on ConstructionBase.jl?

[mauro3]

Yes, I wasn't clear: the last option would require the dependency on ConstructionBase. Which is ok, but it's also ok to strive for few dependencies.