WIP: Implement mutating versions of @set and @lens (#71)

[colinxs]

No description provided.

[tkf]

Here is the diff wrt #91 (for easy review): constbase...colinxs:bangbang

[tkf]

BangBang is not a super small library and it probably will accumulate more code over time, as it has to re-implement some Base/stdlib functions. I'm not sure if it is a good approach for Setfield.jl to depend on it. Maybe we should factor out setproperty!! and setindex!! to ConstructionBase.jl or some small library under JuliaObjects? (Also, we probably should create a new API setproperties!! instead.)

[colinxs]

Makes sense! What's your timeline on registering ConstructionBase.jl?

[tkf]

We are just waiting for it JuliaRegistries/General#4063

[colinxs]

Oh, awesome!! Glad to hear.

[colinxs]

If we do refactor, need that block this PR? I'm happy to make the changes, just looking to get this merged sooner-than-later so I can start developing around it :).

Also, is #91 just waiting on JuliaRegistries/General#4063?

[colinxs]

Just pushed the changes to use MutationPolicy.

In principal, I like the idea of making Setfield extensible. At one point I was considering manipulating deeply nested C structs using unsafe_store! for cases where (1) the compiler can't optimize away object construction and (2) thread-safe field manipulation. I recently found mchristianl/MemoryMutate.jl which does just this and could be re-implemented in terms of Setfield.jl.

The separation of BangBang is obviously up to you all.

My selfish opinion would be if you want to refactor BangBang, then make Setfield extensible, otherwise either is fine :)

[jw3126]

• I am fine with Setfield depending on BangBang temporarily if we have a plan how to fix that in near future.
• If we move the required things from BangBang to ConstructionBase, how much code/dependencies would that add to ConstructionBase @tkf?
• I like the idea to split @set more cleanly in a "parsing" step and an "interpretation" step that can be overloaded by other macros. But I feel developing this is quite some effort and should be done independently of this PR.

[tkf]

• I am fine with Setfield depending on BangBang temporarily if we have a plan how to fix that in near future.

I think fixing it (= separating out the base part of BangBang) is hard.

• If we move the required things from BangBang to ConstructionBase, how much code/dependencies would that add to ConstructionBase

Reviewing my code in BangBang.jl, I realized that there are quite a few interface methods in BangBang.jl (may, possible, pure, ismutable, ismutablestruct, and _asbb). I think it would be too much to add those methods as-is in ConstructionBase.jl. (Although I think it makes sense to have ismutablestruct(::Type{<:StructType}) and even ismutable(::Type{<:CollectionType}) there.) What ConstructionBase.jl has at the moment are a few functions that are definitely the basic things you want. However, I feel what BangBang.jl has are rather opinionated set of functions (even though I think this is a nice direction to explore).

• But I feel developing this is quite some effort and should be done independently of this PR.

I do agree this is a hard task. But I think it's the most reasonable option for implementing @set!! and @set! (If I were forced choose refactoring BangBang.jl or making Setfield.jl extensible, I'll definitely choose the latter).

[jw3126]

I did a POC here.

[jw3126]

I responed JuliaObjects/ConstructionBase.jl#25 (comment)

[tkf]

I would implement it using something like this:

abstract type MutationPolicy end
struct Immutable <: MutationPolicy end  # no bang
struct AlwaysMutate <: MutationPolicy end  # !
struct TryMutate <: MutationPolicy end  # !!

struct PropertyLens{fieldname, TP <: MutationPolicy} <: Lens
    policy::TP
end

PropertyLens(fieldname, policy = Immutable()) =
    PropertyLens{fieldname, typeof(policy)}(policy)

It would also be handy define accessor functions

MutationPolicy(::Lens) = Immutable()  # should it be this?
MutationPolicy(l::PropertyLens) = l.policy

[colinxs]

Yeah that seems more concise (and also replaces the fairly redundant BangStyle trait). To maintain PropertyLens being a Singleton, what about:

struct PropertyLens{fieldname, TP} end
PropertyLens(fieldname, policy::MutationPolicy = Immutable()) = PropertyLens{fieldname, policy}()
MutationPolicy(::PropertyLens{<:Any, TP}) = TP

Also, just curious, what does TP stand for?

[tkf]

To maintain PropertyLens being a Singleton

It would be a singleton if plicy is a singleton:

struct Demo1 end
struct Demo2{T}
    demo::T
end

julia> sizeof(Demo2(Demo1()))

julia> Base.issingletontype(typeof(Demo2(Demo1())))
true

If we want to enforce the policy to be a singleton, I think your approach would be better.

what does TP stand for?

I was thinking Type-of-Policy. It can have a better name, too :)

[colinxs]

Ahh, I didn't know that! I was operating off of the definition in the docs:

Immutable composite types with no fields are singletons

which I took to imply "singleton if-and-only-if immutable composite type with no fields". Thanks for the explanation!

Perhaps MP for Mutation Policy?

[tkf]

Maybe? We can also call it simply Policy, as done in e.g. Broadcasted: https://github.com/JuliaLang/julia/blob/d5d5718b85d829aff6c61ab23ad2d91a50f8688b/base/broadcast.jl#L169-L173

[colinxs]

I like the verbosity.

[jw3126]

• I would rename Immutable -> NeverMutate. It is more consistent with the other variants and we might want the name Immutable for another trait.
• I think MutationPolixy(l::Lens) = NeverMutate() is the right default. I think Setfield still is mainly about manipulation of immutable structs.
• The design should be so that an ad hoc defined lens only needs to implement get(obj, lens) and set(obj, lens, val) and still works if it is not interested in mutating things.

[jw3126]

I think the part where it gets really challenging is to implement set on a composition. A composition that involves !! or ! lens applied to a nested struct should only mutate the innermost possible mutable in a nested struct update and not reconstruct any other objects.

[tkf]

I was assuming that the answer was that "users should be careful." Why not just say that you'd loose the pureness property if there is at least one mutating-lens? That is to say, isn't it OK to have

x = (a=[1],)
y = set(x, (@lens _.a) ∘ (@lens! _[1]), 2)
@assert x.a === y.a
@assert x.a[1] == 2

[jw3126]

Yes you example is the only reasonable behavior I can imagine. What I was talking about is

struct A; a end
x = A([1])
y = set(x, @lens!! _.a[1], 2)
@assert x.a === y.a
@assert x.a[1] == 2

Does this do a constructor call to A? I would prefer it if that was optimized away. But if this makes compose implementation too involved, I can live with it not being optimized out.

Edit: About composition of @lens! I am fine with if that just mutates the innermost (and errors if the innermost is immutable).

[jw3126]

We had some half baked per set call mutation support in the beginning. The implementation is probably not interesting, but maybe you can salvage some of the tests.
1ac9153

[tkf]

Does this do a constructor call to A?

Ah, that's a good point. Maybe something like this would work?

function set(obj, l::ComposedLens, val)
    inner_obj = get(obj, l.outer)
    if MutationPolicy(l.inner) isa AlwaysMutate
        set(inner_obj, l.inner, val)
        obj
    else
        # Do what we are doing:
        inner_val = set(inner_obj, l.inner, val)
        set(obj, l.outer, inner_val)
    end
end

But, if it made the code better, it would mean that there might be some non-trivial invariant checking in the constructor. Maybe we shouldn't be skipping it? If you know that it is safe to skip those checks then you probably know the type of the outer object reasonably well so that you can directly modify mutable inner object?

[jw3126]

But, if it made the code better, it would mean that there might be some non-trivial invariant checking in the constructor.

A plain a.b.c = d does no invariant checking, so I think it would be a sane choice if we allow ourselfs to optimize that away as well.

If you know that it is safe to skip those checks then you probably know the type of the outer object reasonably well so that you can directly modify mutable inner object?

That is an interesting point. Maybe we should examine some performance/IR and check if in the case of plain structs without inner constructor julia manages to elide the reconstruction and just mutates the innermost. If julia manages to optimize that well, that would be great. If this is the case, then I agree the invariant checking is a nice feature.

[tkf]

Actually, I know from experience that re-constructing structs sometimes helps improving code-gen. There is a function (adequately) called darkritual in Transducers.jl which just re-constructs all nested immutable structs. It is useful when one of the struct has an Array as a member and IIRC it was impossible to generate SIMD'ed loop without it.

[colinxs]

So I'm almost done incorporating the above feedback, but ran across the following which I thought was a bug. Using your example:

struct A; a end
x = A([1])
y = set(x, @lens!! _.a[1], 2)
@assert x.a === y.a
@assert x.a[1] == 2

You also have (without making the above modification to set(obj, l::ComposedLens, val)):
@assert x === y

Edit: This behavior stems from setproperty!!:

x = A([1])
a = x.a
y = setproperty!!(x, :a, a)
@assert x === y

[tkf]

Isn't it expected? That's equivalent to

julia> a = [1];

julia> x = (a=a,);

julia> y = (a=a,);

julia> x === y
true

Right?

[colinxs]

Ahh I guess so! Nevermind :)

[jw3126]

Can you please use four spaces for indentation? Looks like you are using two tabs?

[colinxs]

Definitely! Unsure how that happened as I usually use 4 spaces.

[jw3126]

I think we should also deprecate the current @set! macro into something else. Maybe @reset?
The new @set! macro can live in Setfield.Future for a while.

[tkf]

A bit crazy plan is to rename the whole package :)

[jw3126]

Since you pointed out that my favorite name is already taken in I am somewhat depressed about it. But yeah the package needs to be renamed, we can discuss that further in #54.

[tkf]

As of 5035991, we have

Setfield.jl/src/lens.jl

Lines 142 to 143 in 5035991

	@inline set(obj, l::PropertyLens{name, AlwaysMutate()}, val) where {name} =
	setproperty!(obj, name, val)

and

Setfield.jl/src/lens.jl

Lines 212 to 213 in 5035991

	Base.@propagate_inbounds set(obj, l::IndexLens{<:Tuple, AlwaysMutate()}, val) =
	setindex!(obj, val, l.indices...)

I should have noticed this earlier, but it means that @set! x.a[1] = 2 is equivalent to x.a[1] = 2, right? It seems like a redundant syntax? I think @lens! x.a[1] may be useful though.

FYI what I call mutate-if-mutable in #71 (comment) would be something like this

if ismutablestruct(obj)
    setproperty!(obj, name, val)
    obj
else
    set(obj, PropertyLens(name, NeverMutate()), val)
end

We could use this for @set! but it is only slightly different from @set!! so it may be confusing.

[tkf]

I was thinking how to make @set!! implementable outside Setfield.jl. I think it is somewhat possible already. @colinxs Maybe you can use it if you don't like waiting for this PR to be merged?

Here is an implementation that "replaces" PropertyLens with !!-variant

using Setfield
using Setfield: ComposedLens, PropertyLens, parse_obj_lens
using BangBang

struct Lens!!{T <: Lens} <: Lens
    lens::T
end

Setfield.get(obj, lens::Lens!!) = get(obj, lens.lens)

# Default to immutable:
Setfield.set(obj, lens::Lens!!, value) =
    set(obj, lens.lens, value)

Setfield.set(obj, lens::Lens!!{<:ComposedLens}, value) =
    set(obj, Lens!!(lens.lens.outer) ∘ Lens!!(lens.lens.inner), value)

Setfield.set(obj, ::Lens!!{<:PropertyLens{fieldname}}, value) where fieldname =
    setproperty!!(obj, fieldname, value)

macro set!!(ex)
    ref, val = ex.args
    obj, lens = parse_obj_lens(ref)
    val = esc(val)
    quote
        $obj = let compose = $Setfield.compose,
            IndexLens = $Setfield.IndexLens
            $Setfield.set($obj, $Lens!!($lens), $val)
        end
    end
end

mutable struct Mutable
    a
    b
end

x = orig = Mutable(1, 2)
@set!! x.a = 10
@assert orig.a == 10

x = orig = Mutable((1, 2), 3)
@set!! x.a[1] = 10
@assert orig.a == (10, 2)

There is a bit of hack needed (let in @set!!) but this can actually be easily solved by embedding function/type objects in the expression returned by parse_obj_lens. Also, it indicates that making @set "overloadable" is as easy as

diff --git a/src/sugar.jl b/src/sugar.jl
index d735047..8a5b4f2 100644
--- a/src/sugar.jl
+++ b/src/sugar.jl
@@ -133,7 +133,7 @@ struct _UpdateOp{OP,V}
 end
 (u::_UpdateOp)(x) = u.op(x, u.val)

-function atset_impl(ex::Expr; overwrite::Bool)
+function atset_impl(ex::Expr; overwrite::Bool, preprocess=identity)
     @assert ex.head isa Symbol
     @assert length(ex.args) == 2
     ref, val = ex.args
@@ -142,14 +142,14 @@ function atset_impl(ex::Expr; overwrite::Bool)
     val = esc(val)
     ret = if ex.head == :(=)
         quote
-            lens = $lens
+            lens = $preprocess($lens)
             $dst = set($obj, lens, $val)
         end
     else
         op = get_update_op(ex.head)
         f = :(_UpdateOp($op,$val))
         quote
-            $dst = modify($f, $obj, $lens)
+            $dst = modify($f, $obj, $preprocess($lens))
         end
     end
     ret

We can then do this:

macro set!!(ex)
    atset_impl(ex; overwrite=true, preprocess=Lens!!)
end

[jw3126]

Nice @tkf I actually had very much the same idea, coming from a different angle. I thought about splitting @set in a "parse" step and an "interpretation" step. Now the output of the parse step would be some kind of "Setfield.AST", with objects like:

AST.ComposedLens(AST.IndexLens(:myindex), AST.PropertyLens(:myproperty))

But this structure is (almost) isomorphic to its standard interpretation (AST.ComposedLens -> Setfield.ComposedLens). So it feels somewhat redundant to have it in the first place.

So the outcome is very much the same as yours. The only thing I would have done differently is that instead of $preprocess($lens) there would be a call to interpret($interpreter, $lens)

[tkf]

Is interpret something like a map over "lens/AST tree?" And the reasoning was that users don't have to write their recursive transformation? But maybe it makes sense to allow users to transform ComposedLens to a completely different representation? Like fusing some combinations of lenses? Not that I have a good example ATM, though.

But this structure is (almost) isomorphic to its standard interpretation (AST.ComposedLens -> Setfield.ComposedLens).

I'm just curious. What will be lost when going from AST to the standard interpretation?

[tkf]

It's not for @set macro but I have one example of lens tree transformation I'm using for some private codebase. What I'm doing is, given a lens like

(@lens _.a.b.c) ∘ settingas𝕀

insert "non-constant annotation" for Zygote (see this uncut function) like this:

(@lens _.a.b.c) ∘ converting(fromfield=unwrap, tofield=uncut) ∘ settingas𝕀

That is to say, insert converting lens before the left-most "non-field" lens. This is an example lens transformation that has to look at entire tree.

The question is if it makes sense to do this kind of transformation for @set macro. I don't know if this is the case, but it seems harmless to allow arbitrary transformation as it does not complicate the API.

[jw3126]

Is interpret something like a map over "lens/AST tree?" And the reasoning was that users don't
have to write their recursive transformation?

We would have preprocess == lens -> interpret(interpreter, lens) so there is no technical advantage of either. Just a spelling to be bike shedded. I think I like passing around a single function more then an interpreter object. Though I don't like the name preprocess much.

But maybe it makes sense to allow users to transform ComposedLens to a completely different representation? Like fusing some combinations of lenses? Not that I have a good example ATM, though.

Yes I think giving a lot of freedom makes sense, even if I also have no application in mind.

But this structure is (almost) isomorphic to its standard interpretation (AST.ComposedLens -> Setfield.ComposedLens).

I'm just curious. What will be lost when going from AST to the standard interpretation?

I am also curious. This is a bit fuzzy in my head. I cannot define in full rigor, what I mean by "this structure", "isomorphic" or "standard interpretation". But one difference is that AST would be a compile time object that would be interpreted at compile time. E.g. in the code we would have $(preprocess(lens_ast)) instead of $preprocess($lens).

[tkf]

E.g. in the code we would have $(preprocess(lens_ast)) instead of $preprocess($lens).

Oh, I see. I was thinking that what you meant was $preprocess($lens_ast); i.e., the evaluation would happen at run-time, rather than at macro expansion time. So I suppose the difference is that $preprocess($lens) approach would have access to the values of lenses (e.g., the index value of IndexLens) while $(preprocess(lens_ast)) approach would have access to lens expressions (e.g., function expression of the anonymous function wrapped by DynamicIndexLens). So, both seem to gain and lose information compared to the alternative.

Though I don't like the name preprocess much.

Me too :) It can be better. transform? convert? It also can have lens prefix/suffix, like lenstransform. Or maybe something related to physical lenses... like polish? (OK that's maybe too much of a pun.)

[jw3126]

Oh, I see. I was thinking that what you meant was $preprocess($lens_ast); i.e., the evaluation would happen at run-time, rather than at macro expansion time.
...
So, both seem to gain and lose information compared to the alternative.

Yeah. Though of course a compile time transform can just insert a runtime transform, but not the other way round. But I think we should stick with the runtime version, it is more convenient. And only switch to the compile time version if we have a good reason why we need that.

Me too :) It can be better. transform? convert? It also can have lens prefix/suffix, like lenstransform. Or maybe something related to physical lenses... like polish? (OK that's maybe too much of a pun.)

I like lenstransform. Since this is a rare operation, a long descriptive name seems the way to go. Pushing the lens analogy e.g. polish is a fun thing to do, but I think it hurts understanding.
If none of you guys is faster, I will make a PR next week.

[colinxs]

Hey sorry all! I was pretty swamped these last two days. Catching up with your comments now.

[tkf]

Though of course a compile time transform can just insert a runtime transform, but not the other way round.

@jw3126 Oh, that's right. I missed that.

These C structs are nested 3-4 structs in, and the inner most field may be an StaticArray/NTuple.

@colinxs Re JuliaObjects/ConstructionBase.jl#25 (comment), I think my approach above #92 (comment) can already be used for this.

[colinxs]

@tkf I'll test that out later today! Thanks for taking the time to put that together. If it works then all my needs are satisfied for now. I'll keep an eye on this to see what cool implementation you come up with.

[colinxs]

Thanks for all the hard work! With #95 I think my use-case is satisfied.

[colinxs]

@tkf @jw3126 in reference to JuliaLang/julia#21912, Setfield.jl implements the @setfield macro described in that PR which creates a copy of an immutable with the respective field changed. My concern is the same as yuyichao, however, in that I may have concurrent threads modifying the same immutable and thus would prefer the RefField implementation that does the unsafe_store! (rather than relying on the compiler optimizing out the copy).

Using #95, this would be pretty straightforward to implement using Lens. I was going to take a stab at that in the next day or so, but wanted to check with you to see if that's something you may have already considered/started implemented to avoid duplicated work.

[tkf]

I may have concurrent threads modifying the same immutable and thus would prefer the RefField implementation that does the unsafe_store!

I'm a bit confused here. If you have multiple threads, don't you want to keep objects immutable so that you don't need to worry about data races?

BTW, here is a PR that implements @set!!: JuliaFolds/BangBang.jl#25

[colinxs]

@tkf If I have a struct with a StaticArray field (which has to be immutable to get the C equivalent inline memory layout) and want threads to be operating on different parts of the data, then @set as currently implemented would create a data race (dependent on whether the compiler optimizes out the creation of the new struct or does an unsafe_store!.

I'll checkout the PR! Thanks again for all your help.

[tkf]

Hmm... Is it OK to call it a data race though? I thought data race needs one or more thread(s) writing to some memory location. There is no "write" in set. Isn't it just using invalid update mechanism?

By the way, did you check https://github.com/RelationalAI-oss/Blobs.jl? It seems to be more appropriate for mutating heap-allocated structs, if your main goal is interop with C?

[colinxs]

Oh Blobs.jl looks awesome! That might just help out quite a bit. Although some of these fields are pointers so it's not just a single flat memory region. I'll take a look :)

Yeah it's not quite a "race condition" but the outcome is similar in that it depends on the order of execution.