jw3126 / Setfield.jl

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

constructor_of() #68

Closed rafaqz closed 5 years ago

rafaqz commented 5 years ago

I'm updating and abstracting Flatten.jl to allow arbitrary flattening and modifications of nested structs using method and type 'queries'.

It has some similarities to Setfield.jl in that it needs to call the constructor of immutable types, so I've just copied your constructor_of() to make this more apparent. But I was thinking it might be nice to abstract this to a base package one day so that structs with custom type parameters can just define a specific constructor once for anything that needs it.

jw3126 commented 5 years ago

I am open to moving things out of Setfield.jl that other packages want to share. Are you only thinking about constructor_of or do you have other stuff that should move to the base package?

@generated constructor_of(::Type{T}) where T =
getfield(T.name.module, Symbol(T.name.name))

While this function is non trivial, it is also very short and I hesitate to add a dependency just for such a short function.

rafaqz commented 5 years ago

Yeah it does seem a bit short to add that dependency for.

I mostly wanted to flag that I'm using the exact same function for the same purpose, and it seems to be a general requirement for any tool that works with immutables like this. Maybe when more work is done in base we can have something generalised.

tkf commented 5 years ago

While the default constructor_of is super short, I think it makes sense to extract it out to a base package because it is an overloadable interface. For example, maybe QuickTypes.jl can overload constructor_of by default. We then don't need to use Setfield.@settable. I also occasionally overload constructor_of in my code. It'd be nice if such definition of constructor_of can be reused in other packages.

arbitrary flattening and modifications of nested structs

Just FYI I recently created a similar package https://github.com/tkf/Kaleido.jl based on Setfield.jl

rafaqz commented 5 years ago

Yeah I was thinking that, but I'm also not strongly opinionated about it because in practice I only have one or two types that I'm actually overloading constructor_of for. I imagine some coding styles that use a lot of custom type parameters would need it a lot more. But I would depend on ConstructorBase (or whatever) in Flatten if we split it out.

I'll take a look at Kaleido. The core difference between Flatten and Setfield/Kaleido approach is that Flatten is more about abstracting the process than specifying it - for cases where you don't know or care what the field names are. Its like a query: get all the e.g Real fields in some user-defined nested struct, do something to them and reconstruct the object. It mostly compiles down to just modifying those fields directly.

tkf commented 5 years ago

The core difference between Flatten and Setfield/Kaleido approach is that Flatten is more about abstracting the process than specifying it

I think lens is powerful and abstract enough to handle such cases.

get all the e.g Real fields in some user-defined nested struct, do something to them and reconstruct the object

For example, this can be done by

using Setfield

struct SubtypeFieldsLens{T} <: Lens
end

Setfield.get(obj, ::SubtypeFieldsLens{T}) where T =
    foldl(fieldnames(typeof(obj)), init=NamedTuple()) do xs, n
        v = getfield(obj, n)
        if v isa T
            return (; xs..., (n => v,)...)
        else
            return xs
        end
    end

Setfield.set(obj, ::SubtypeFieldsLens{T}, xs) where T =
    foldl(fieldnames(typeof(obj)), init=(obj, 1)) do (obj, i), n
        v = getfield(obj, n)
        if v isa T
            return (set(obj, Setfield.PropertyLens{n}(), xs[i]), i + 1)
        else
            return (obj, i)
        end
    end[1]

@assert get((a = 1, b = "2", c = 3.0), SubtypeFieldsLens{Real}()) ==
    (a = 1, c = 3.0)
@assert set((a = 1, b = "2", c = 3.0), SubtypeFieldsLens{Real}(), (2, 6)) ==
    (a = 2, b = "2", c = 6)

# Parse all String fields into Int:
@assert modify(
    xs -> parse.(Int, Tuple(xs)),
    (a = 1, b = "2", c = "3"),
    SubtypeFieldsLens{String}(),
) == (a = 1, b = 2, c = 3)

# Get the second String field of the object inside property `a`
@assert get(
    (a = (b = 1, c = "2", d = 3, e = "4"),),
    (@lens _.a) ∘ SubtypeFieldsLens{String}() ∘ (@lens _[2])
) == "4"

(This is probably not super efficient but I think you can improve it to make things more inferrable.)

It also should be possible to recurse into all sub-objects and flatten fields.

In the last example I tried to show that lenses compose with other lenses. I think it would be nice if you express what you need using Lens. It would then compose well with other Lenses from other libraries.

Also, you can simply use Setfield.constructor_of this way :)

rafaqz commented 5 years ago

That's interesting. The composability would be great.

It would need to handle field level trait functions like flattenable to ignore specific fields. I guess they could be a parameter of SubtypeFieldLens.

Also Flatten is very efficient - ie flatten and reconstruct compile together to just change the fields that are altered. Basically everything is compiled away.

And it's very simple to use and a tiny amount of code that I understand really well. I doubt I will have time to rewrite it as a Lens anytime soon as I have so many other packages to work on but I'll definately think about it in future.

tkf commented 5 years ago

Of course, it is good to keep your library simple. I just thought it might be worth considering Lens API (it's just getter and setter actually) as you were mentioning rewrite and abstraction. Anyway, I guess I should try not to derail the discussion. Sorry about that!

rafaqz commented 5 years ago

No that was really interesting! The only problem is I have already done the rewrite otherwise I would be looking at using Lens :)

jw3126 commented 5 years ago

@tkf https://github.com/tkf/Kaleido.jl looks really cool! Getting rid of @settable would be nice. @cstjean are you open to add a tiny dependency to QuickTypes and call constructor_of in your package? This would make working with your types easier for other packages e.g. Setfield.jl and Flatten.jl.

So what should be the scope of the base package? Just constructor_of? Or what else should live there?

cstjean commented 5 years ago

FWIW, I feel like SetField should put on its pants, and use whatever unexported function Julia uses to implement new. I know that's heresy, but relying on Foo(a,b) being the default constructor seems both unnecessary and dangerous, when the assumption is false.

cstjean commented 5 years ago

re you open to add a tiny dependency to QuickTypes and call constructor_of in your package?

Sure!

jw3126 commented 5 years ago

FWIW, I feel like SetField should put on its pants, and use whatever unexported function Julia uses to implement new. I know that's heresy, but relying on Foo(a,b) being the default constructor seems both unnecessary and dangerous, when the assumption is false.

Interesting idea. I never tried to understand what new is precisely or if it can somehow be called outside of a struct definition.

jw3126 commented 5 years ago

Yeah ok I am in favor of a base package. Lets talk about name and scope.

tkf commented 5 years ago

I think other people may be interested in this. For example, when using Flux.jl, I repeatedly write code like this

using Setfield: @settable
using QuickTypes: @qstruct_fp
using Flux: @treelike

@settable @qstruct_fp Foo(...)
@treelike Foo

which is not really sane. Looking at Flux.@treelike, it's also doing something similar (especially look at Flux.mapchildren(f, x::$T) = $T(f.($children(x))...)): https://github.com/FluxML/Flux.jl/blob/1902c0e7c568a1bdb0cda7dca4d69f3896c023c7/src/treelike.jl#L4-L23

@MikeInnes Are you interested in a base package for constructors? Hope you don't mind the ping :)

  • I think setproperties should go there a well.

I agree.

Also, it may be useful to add setindices which can have implementation for Tuple and Namedtuple and overloaded by StaticArrays. I'm not quite sure if this is the best idea, but I think it's better to think about the package name which can cover the concepts like setindices.

  • As for the name ConstructionBase? ImmutablesBase?

I prefer ConstructionBase as you might want to re-construct mutable struct (which may change type parameters).

use whatever unexported function Julia uses to implement new

I think deserialize and deepcopy are good starting point. It looks like you need to ccall C API like jl_new_struct and jl_new_struct_uninit?

(But I'm not quite sure relying on internal/C API is a great idea. Using positional constructor sounds OK to me.)

jw3126 commented 5 years ago

I agree, the package should cover mutables and things like setindices. As for setindices specifically, how does it differ from Base.setindex? Here are more packages that could profit from the base package:

tkf commented 5 years ago

how does it differ from Base.setindex

A somewhat contrived example would be a user-defined (named) tuple-like type with some kind of invariant; e.g., the sum of all the numerical fields is less than one.

tkf commented 5 years ago

Ah, I guess you mean that it's possible to define something like

Base.setindex(collection::Tuple, values::Tuple, indices::Tuple)

to do the batch update (in principle; this example as-is a type piracy against Base so not really nice). Yeah, I missed that point.

jw3126 commented 5 years ago

Yeah you are right, setindices seems useful.

cstjean commented 5 years ago

How to construct an object without calling the constructors: https://github.com/MikeInnes/BSON.jl/blob/95dc736ec99dcd6bbf1abb59fdefece85a9bc3c7/src/extensions.jl#L104-L120

rafaqz commented 5 years ago

Interesting. But isn't that doing type conversion to match the existing field type? In Flatten and Setfield you can change the field type.

cstjean commented 5 years ago

Ah, good observation. It does look like a messy operation.

tkf commented 5 years ago

Re https://github.com/jw3126/Setfield.jl/pull/84#issuecomment-535816747. Can we create ConstructionBase in JuliaCollections? I think @jw3126 and I can co-maintain the repository? Maybe it's a bit stretch to say this is about collections but struct is in some sense a collection. Also, I don't know other Julia* organization that fits with the idea.

@ararslan @kmsquire @oxinabox @scls19fr @simonster You guys are listed in https://github.com/orgs/JuliaCollections/people. Is it possible to set up a repository JuliaCollections/ConstructionBase? The idea is to (mainly) have a function ConstructionBase.constructor_of such that constructor_of(typeof(x))(fieldvalues(x)...) reconstruct the value x. It is useful for various tools dealing with immutable structs.

jw3126 commented 5 years ago

Just to confirm, I am willing to maintain ConstructionBase.

rafaqz commented 5 years ago

+1. I've been thinking about this as well the last few days, while implementing it in yet another package.

Can also co-maintain.

ararslan commented 5 years ago

IMO it doesn't make sense to have it in JuliaCollections. It could easily live in a personal account with multiple people having commit access.

tkf commented 5 years ago

@ararslan Thanks for the response. I guess it was too much of a stretch then...

tkf commented 5 years ago

Alternatively, how about creating a new organization? I think it'd be nice to put Setfield in the same organization as well (maybe after renaming #54). Of course, provided that @jw3126 likes that option. I believe the getter/setter API formulated in Setfield should be used across Julia ecosystem. I think putting it in an organization would communicate it better.

...but the problem is it's hard to come up with a good name. Just to start brainstorming:

jw3126 commented 5 years ago

I am okay with moving Setfield to an organization. (And maybe also rename the Setfield package itself in that process). Finding a good org name is really hard. So the scope of the organization should be "updating (mutable and immutable) objects"? Also creating objects (like e.g. QuickTypes?) Here is a list of packages that come to my mind that should be included:

These names came to my mind first:

Out of the names suggested so far, I like JuliaObjects most I think.

tkf commented 5 years ago
  • JuliaConstruction

I thought about it a bit but then it felt like a team developing tooling for designing physical buildings or something. ...although I don't trust my common sense for English phrase :) Besides, I don't think it fits super well with Setfield. Anyway, I think other options are better.

jw3126 commented 5 years ago

@rafaqz @tkf shall we go with JuliaObjects?

tkf commented 5 years ago

Sounds good to me. But maybe @rafaqz have other ideas?

rafaqz commented 5 years ago

I like JuliaObjects. Flatten.jl would fit there as well.