@functor is confused about properties vs fields

[mcabbott]

It was noticed in FluxML/Flux.jl#2107 that Functors.jl + ProtoStruct.jl doesn't work, as Functors uses fieldnames + getproperty, which is overloaded by ProtoStruct.jl.

https://github.com/FluxML/Functors.jl/blob/v0.3.0/src/functor.jl#L11-L16

This is a bug, Functors should use getfield to be consistent. (Possibly the code was written before getproperty existed?)

[ToucheSir]

With the reversion of #48, I guess our second attempt would be to try to use all properties? It won't be type stable for most types that define custom getproperty overloads, but playing around with https://github.com/JuliaObjects/ConstructionBase.jl/blob/40800e17ef953c5cccaeef2d1d651346cacc5014/src/ConstructionBase.jl#L75-L78 suggests it should be for those which don't.

[mcabbott]

So the problem in #48 is Tangent, which Optimisers.jl wants to access via getproperty. Not sure that's a great design but it won't change soon.

For Functors to use properties-not-fields everywhere, I think it would have to be functor(guide, target) rather than functor(::Type{Guide}, target). I haven't thought through what else this would change.

[darsnack]

For Functors to use properties-not-fields everywhere, I think it would have to be functor(guide, target) rather than functor(::Type{Guide}, target). I haven't thought through what else this would change.

In my refactor with ConstructionBase, I tried making children and rebuilder (thing that returns re) the foundation instead of functor. In this situation you don't have access to target and guide together, so you need access to the guide itself to close over the leaf nodes.

[mcabbott]

One barrier to properties-everywhere is that Zygote uses fields:

julia> using ProtoStructs, Zygote

julia> @proto struct Mine
         num::Float64
       end

julia> gradient(x -> x.num^2, Mine(3.0))[1]
(properties = (num = 6.0,),)

julia> propertynames(Mine(3.0))
(:num,)

[ToucheSir]

And because we return plain NamedTuples instead of e.g. Tangents, we don't even know whether that was meant to be a ProtoStruct. Even ChainRules assumes fields are being used in a few places—canonicalize comes to mind.