Bug in `functor(::Type{NamedTuple}, Any)`

[mcabbott]

I think this is a bug -- functor(typeof(x), y) should always use x's field names on y:

julia> struct Foo; x; y; end

julia> @functor Foo

julia> struct AnotherFoo; x; y; end

julia> x = Foo([1, 2], 3);

julia> y = AnotherFoo([4, 5], 6);

julia> z = (x = [7, 8], y = 9);

julia> functor(x)
((x = [1, 2], y = 3), var"#31#32"())

julia> functor(typeof(x), y)
((x = [4, 5], y = 6), var"#31#32"())

julia> functor(typeof(z), y)  # this is wrong?
(AnotherFoo([4, 5], 6), Functors.var"#5#6"())

[ToucheSir]

https://github.com/FluxML/Functors.jl/blob/v0.2.7/src/functor.jl#L5. Suffice it to say I don't think this case was considered when the base functor definitions were written...

[ToucheSir]

(the Tuple and AbstractArray defs are likewise "fun")

[mcabbott]

I didn't think about those. You are suggesting that

julia> t = ([10,11], 12);

julia> functor(typeof(t), z)
((x = [7, 8], y = 9), Functors.var"#3#4"())

should return a tuple? No struct can match propertynames(t) I believe, but z[1] == z[:x] != getproperty(z, 1)... where's the line between using getproperty and using getindex?

[ToucheSir]

I'm not sure. This is one of those areas where the original codebase played fast and loose with what behaviour is "in spec", so it's not clear to me what the intent was. I do think that it shouldn't pass x through like it does now if the NamedTuple case is changed.

[mcabbott]

One attempt is:

functor(::Type{<:Tuple{Vararg{Any,N}}}, x) where N = NTuple{N}(x), y -> NTuple{N}(y)
functor(::Type{<:NamedTuple{L}}, x) where L = NamedTuple{L}(map(s -> getproperty(x, s), L)), y -> NamedTuple{L}(map(s -> getproperty(y, s), L))

I'm not sure the y -> ... half is necessary at all. Nor whether the Tuple case is desirable.

The NTuple case showed up in tests here: https://github.com/FluxML/Functors.jl/pull/37/files/4adbcf1614af31478bb56665c1fd8f66353e0b35..7ab2efdbc0e2b030397b6cd5e8a0ec6071fbcef1#diff-ff6ed95b5e91003416c0e6ea6e89f3cd7672e0b34da92293fa8db6632622c8ebR118