[RFC] add setproperties!!

[jw3126]

Over at Setfield we have a PR that wants to add lenses that work on mutables and immutables.
This requires setproperties!!, which currently lives in BangBang (or more precisely setproperty!!).
This PR investigates adding setproperties!! here instead. See also this comment

[codecov-io]

Codecov Report

Merging #25 into master will decrease coverage by 6.25%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage     100%   93.75%   -6.25%     
==========================================
  Files           1        1              
  Lines          23       32       +9     
==========================================
+ Hits           23       30       +7     
- Misses          0        2       +2

Impacted Files	Coverage Δ
src/ConstructionBase.jl	93.75% <77.77%> (-6.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3f1f79...dda8446. Read the comment docs.

[jw3126]

I would like to respond to the @tkf comment

• 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).

I think ConstructionBase should be the minimal package, that contains all functionality needed to enable Setfield and co on custom types.
This implies that setproperties!! should be part of ConstructionBase.
About the other functions may, possible, pure, ismutable, ismutablestruct, and _asbb I am not sure. We could have only the very few methods here that are used to implement setproperties!!. The rest is defined in BangBang. While that is technically type piracy, I think thats not a big issue for packages owned by the same org.

Also I think neither ismutablestruct nor setproperties!! is opinionated. ismutable is, but we don't need it here (yet).

[tkf]

OK, so setproperties!! I had in BangBang.jl haven't implemented the full functionalities I wanted to add. I'm trying to do it in JuliaFolds/BangBang.jl#23

Translating to setproperties!!, it would be something like this:

function setproperties!!(obj, patch::NamedTuple{pnames}) where pnames
    if ismutablestruct(obj) && all(n -> patch[n] isa fieldtype(typeof(obj), n), pnames)
        setproperties!(obj, patch)
    else
        setproperties(obj, patch)
    end
end

[tkf]

I think my point was rather setindex!!. It's kind of out of place here. Also, I really want it to be based on ismutable which is also used from functions like push!! and append!!.

[jw3126]

Okay I agree, setindex!! is a tough call. My understanding is that @colinxs is mainly interested in updating structs, not in setindex and wants to merge rather sooner then later?
If so how about moving setproperties!! to ConstructionBase and leaving the IndexLens!! stuff for future?

[tkf]

Hmm... I think I get it but it's rather unsatisfactory that we have @lens!! only usable for properties.
Also, if it were only for properties, I think it's quite easy to implement outside Setfield. See jw3126/Setfield.jl#92 (comment)

[colinxs]

Unfortunately I need indexing as well :(. These C structs are nested 3-4 structs in, and the inner most field may be an StaticArray/NTuple.

[jw3126]

Do we want setproperties!! here? Depending on that I would put more effort into this PR or not.

[tkf]

After jw3126/Setfield.jl#95, we have enough machinery to solve jw3126/Setfield.jl#92 and this PR is not the only way to do implement jw3126/Setfield.jl#92 anymore. In this case, I prefer to let this API evolve outside the core construction API. I think it even makes sense to add @set!! etc. in BangBang.jl so that @set!! and setproperties!! can be developed together there. This way, we can be more flexible about changes in API (e.g., if #25 (comment) is the desired behavior or not). I haven't used setproperty!! a lot so I'm not super confident about the API.

[jw3126]

Okay, then I close this PR for now.