jw3126 / Setfield.jl

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

Define constructor_of for singleton lenses #84

Closed tkf closed 5 years ago

tkf commented 5 years ago

While I'm using constructor_of inside some meta-programming code, I noticed that it didn't work with some lenses. It's not necessary for Setfield.jl to work so I'm not sure if this is a good change. But it'd make sense for Setfield.jl to have its own type constructor_of-compatible?

A more magic way to do this is

@generated constructor_of(::Type{T}) where T =
    if fieldcount(T) == 0
        T
    else
        getfield(parentmodule(T), nameof(T))
    end

Not sure if I like it though...

jw3126 commented 5 years ago

Any trouble you anticipate with the more magic implementation?

tkf commented 5 years ago

I find it uneasy that the behavior is very different when fieldcount(T) == 0.

Actually, the magic way can be implemented by the user side. That is to say, you can simple do

@generated my_constructor_of(::Type{T}) where T =
    if fieldcount(T) == 0
        T
    else
        :(Setfield.constructor_of(T))
    end

So, I think it is better to keep constructor_of in Setfield as simple as possible.

jw3126 commented 5 years ago

Okay, I guess then the question is what the semantics of constructor_of should be. I thought

T = typeof(x)
constructor_of(T)(fieldvalues(x)...) == x

is a property that we would like to have.

Your interpretation seems to be that constructor_of just strips all type parameters? That is a useful function as well, but maybe should have a different name?

tkf commented 5 years ago

Your interpretation seems to be that constructor_of just strips all type parameters? That is a useful function as well, but maybe should have a different name?

We can always do typeof(x)(fieldvalues(x)...). So, if you don't need to change type parameters, I think that's the way to go.

then the question is what the semantics of constructor_of should be.

I'd say "constructor_of(typeof(T))(fieldvalues(x)...) == x holds and constructor_of(T)(args...) works as many types of args as possible" (though I guess that's somewhat vague). The problem is that in some cases type parameters cannot be determined from the arguments and you need to define constructor_of for your types. So, I thought it'd be better to keep the default implementation simple so that it is clear when you need to define it.

jw3126 commented 5 years ago

I'd say "constructor_of(typeof(T))(fieldvalues(x)...) == x holds and constructor_of(T)(args...) works as many types of args as possible" (though I guess that's somewhat vague).

I like that specification.

So, I thought it'd be better to keep the default implementation simple so that it is clear when you need to define it.

Thats a sane approach, I am ok with it. If we are really pedantic about it, we should merge this PR. But I think its okay to only merge things like this PR if there is practical need.

jw3126 commented 5 years ago

BTW I think we should we create a ConstructionBase package now like discussed in https://github.com/jw3126/Setfield.jl/issues/68 I think you would be a great owner for that package. If you don't want to, I can create it instead.