at-set does not work on instance contained within other module

mauro3 commented 5 years ago

This does not work:

julia> using Setfield

julia> module A
       struct B
       c
       end
       b = B(4)
       end
Main.A

julia> @set A.b.c = 1
ERROR: ArgumentError: Module has no field b
Stacktrace:
 [1] assert_hasfields(::Type, ::Tuple{Symbol}) at /home/mauro/.julia/packages/Setfield/qFM8U/src/lens.jl:113
 [2] #s13#3(::Any, ::Any, ::Any) at /home/mauro/.julia/packages/Setfield/qFM8U/src/lens.jl:119
 [3] (::Core.GeneratedFunctionStub)(::Any, ::Vararg{Any,N} where N) at ./boot.jl:522
 [4] macro expansion at /home/mauro/.julia/packages/Setfield/qFM8U/src/lens.jl:100 [inlined]
 [5] set(::Module, ::Setfield.PropertyLens{:b}, ::Main.A.B) at /home/mauro/.julia/packages/Setfield/qFM8U/src/lens.jl:100
 [6] set(::Module, ::Setfield.ComposedLens{Setfield.PropertyLens{:b},Setfield.PropertyLens{:c}}, ::Int64) at /home/mauro/.julia/packages/Setfield/qFM8U/src/lens.jl:192
 [7] top-level scope at /home/mauro/.julia/packages/Setfield/qFM8U/src/sugar.jl:113

Work around:

julia> b_ = A.b
Main.A.B(4)

julia> @set b_.c = 1
Main.A.B(1)

No big deal, but I though I report.

jw3126 commented 5 years ago

Thanks for reporting this! Not sure, what the correct behavior would be. Return a copy of module A with b replaced by B(1)? This is probably not possible in Julia. If you just want a modified copy of b then I think your work around is actually the cleaner way to express this anyway. However I agree, the error message is not ideal.

mauro3 commented 5 years ago

If you just want a modified copy of b

Yes, that's what I want. Modules ~can~ cannot be copied, so that option is not possible, thus it would probably be ok to just the modified copy of b as there is no ambiguity.

jw3126 commented 5 years ago

Right, I agree it would be unambiguous. However I like the simplicity of @set obj... returning a modified copy of obj instead of doing something clever that is not syntactically obvious. If you like, you are very welcome to improve the error message. I think adding a method to setproperties here would be enough:

function setproperties(m::Module, patch)
    throw(ArgumentError("Cannot set properties $patch of module $m."))
end

tkf commented 5 years ago

Is the problem here that @set a.b.c = x returns a modified copy of a, not b? I wonder if it makes sense to support "scoped assignment" syntax like @set a.b.(c = x) to return a modified copy of b. We can also extend this to a "batch" update syntax

a_modified = @set a.(
    b = x;
    c = y;
    d = z;
)

jw3126 commented 5 years ago

Personally I sometimes do

b = a.b
@set b.c = x

but it is rare that I wish I could do this in one line. Does the desire for scoped assignment come up frequently for you? As for batch updates, this is a need that comes up very often for me. One case I run into frequently is inner constructors that enforce invariants. These are sometimes violated, if not all updates are carried out at the same time. E.g.

v = MyUnitVector(1,0)
@set! v.x = 0  # ouch this does not have norm 1!
@set! v.y = 1  # this again has norm 1

tkf commented 5 years ago

Right, it's not a big pain (especially now that there's @set!). I'm mentioning it because I thought it might be what @mauro3 is asking. My motivation was the batch update, too. But I just realized that mixsing the notation for b = a.b; @set b.c = x and the batch update was a bad idea because it then make it impossible to do the batch update for the nested locations.

mauro3 commented 5 years ago

I'd be interested in batch update too but not related to this. Thanks for all the inputs, I'll close now.

jw3126 / Setfield.jl

at-set does not work on instance contained within other module #61