Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing ! to setindex in setting with a lens #71

Closed
wants to merge 3 commits into from

Conversation

programmeroftheeve
Copy link

No description provided.

@jw3126
Copy link
Owner

jw3126 commented Jul 22, 2019

Thanks for the PR. setindex! is missing on purpose, however. All lens operations should be pure. This means that they never should mutate an object, which setindex! does. Do you have a use case for this PR?

@programmeroftheeve
Copy link
Author

programmeroftheeve commented Jul 22, 2019

My use case was modifing the value in a ref or array.

struct Foo
    a::Int
end

foo = Ref(Foo(1))

refLens = @lens _[].a

set(foo, refLens, 2)

It is for the modification of immutable objects inside of containers.

Equivalent code for current code base.

nonRefLens = @lens _.a
foo[] = set(foo[], nonRefLens, 2)

@programmeroftheeve
Copy link
Author

But the pure-ness makes sense, perhaps a set! function is needed?

@tkf
Copy link
Collaborator

tkf commented Jul 22, 2019

I've been thinking about how to support in-place operations in Setfield API. There are two possible semantics:

  1. Mutate-if-mutable: Always use in-place operation for mutable object. This throws when the value cannot be convert'ed.
  2. Try-mutate: Try to use in-place operation first; if the type cannot hold the new field value, re-construct the new container. (I implemented this semantics in BangBang.setindex!!. It should be easy to wrap it as a lens.)

An interesting consequence is that, when a.b :: Int, a.b = 1.0 would mutate a in-place with the mutate-if-mutable semantics while a would be re-constructed with the try-mutate semantics.

In terms of the (low-level) API, there are two possibilities

  1. Per-call policy: Use one of the three "mutation policies" (i.e., current "always re-construct" plus above two).
  2. Per-lens policy: Define (say) IndexLensMutateIfMutable and/or IndexLensTryMutate.

The surface syntax for the per-call policy would be relatively easy to implement as we just need to implement entry points that work like @set. Unfortunately, we already have @set! which is the most obvious syntax to use. But adding support after a deprecation period is not so bad.

The surface syntax for the per-lens policy would be a bit challenging. One possibility is:

@set a.(!b).c = value
@set a.(![1]).c = value

@jw3126
Copy link
Owner

jw3126 commented Jul 23, 2019

In very early version of Setfield there was a per call option for mutation. See also #32.
I did not like it in practice however. I only rarely used it and it made defining ad hoc lenses more annoying. But as more practical need arises, I am open for impure lenses.

I think to get this started I would prefer the per lens approach. The advantage is that it does not interfer with the current implementation. The disadvantage is that it is probably less dry, as you need to define multiple versions of each lens.

As for syntax I think we should start with something simple stupid. We could have @lens, @lens! @lens!! and @set, @set!, @set!!:

  • The @set macros would be equivalent to a lens definition and a set. For instance:
@set!! o.a.b.c.d = x 

would be equivalent to

let l = @lens!!(_.a.b.c.d)
     set(o, l, x)
end
  • If you want fine grained control of where to use which policy in a long chain a.b.c.d you would need composition: @lens(_.a.b) ∘ @lens!!(_.c.d).

@colinxs
Copy link

colinxs commented Oct 7, 2019

Any update on this? This functionality would be quite useful for wrapping deeply-nested C structs where the top level struct may or may not be mutable. Happy to help out as well!

@jw3126
Copy link
Owner

jw3126 commented Oct 7, 2019

No, I think nothing happened here in this direction. If you want to contribute, that is awesome. I think the hardest decisions are where the mutation is controlled. Quoting @tkf:

In terms of the (low-level) API, there are two possibilities

1. Per-call policy: Use one of the three "mutation policies" (i.e., current "always re-construct" plus above two).

2. Per-lens policy: Define (say) `IndexLensMutateIfMutable` and/or `IndexLensTryMutate`.

I lean towards 2. Maybe @tkf has some opinions on 1. vs 2. or entirely different ideas?

@colinxs
Copy link

colinxs commented Oct 7, 2019 via email

@tkf
Copy link
Collaborator

tkf commented Oct 7, 2019

@jw3126 Yeah, I think per-lens mutation policy is the way to go. Also, using BangBang.jl for a while, I think try-mutate semantics is more useful than mutate-if-mutable or always-mutate.

@colinxs
Copy link

colinxs commented Oct 8, 2019

I pushed my quick-and-dirty implementation of the per-lens mutation policy. Hopefully this is along the lines of what you all were thinking! I had to merge #91 because of the circular dependency between BangBangv0.3.1 and Setfield.

Initial thoughts?

@tkf
Copy link
Collaborator

tkf commented Oct 15, 2019

Closing, as #92 has more discussion.

@tkf tkf closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants