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

get/set queries #23

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Future = "9fa8497b-333b-5362-9e8d-4d0656e87820"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09"
Requires = "ae029012-a4dd-5104-9daa-d747884805df"
Static = "aedffcd0-7271-4cad-89d0-dc628f76c6d3"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"

[compat]
Expand All @@ -19,6 +20,7 @@ CompositionsBase = "0.1"
ConstructionBase = "1.2"
MacroTools = "0.4.4, 0.5"
Requires = "0.5, 1.0"
Static = "0.3"
StaticNumbers = "0.3"
julia = "1.3"

Expand Down
186 changes: 185 additions & 1 deletion src/optics.jl
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
export @optic
export set, modify
export ∘, opcompose, var"⨟"
export Elements, Recursive, If, Properties
export Elements, Recursive, Query, If, Properties
export setproperties
export constructorof
using ConstructionBase
using CompositionsBase
using Static
using Base: getproperty
using Base

Expand Down Expand Up @@ -134,6 +135,10 @@ else
using Base: Returns
end

struct Changed end
jw3126 marked this conversation as resolved.
Show resolved Hide resolved
struct Unchanged end


@inline function _set(obj, optic, val, ::ModifyBased)
modify(Returns(val), obj, optic)
end
Expand Down Expand Up @@ -318,6 +323,185 @@ function _modify(f, obj, r::Recursive, ::ModifyBased)
end
end

"""
modify_stateful(f, (obj,state), optic) => Tuple{NewValue,NewState}

Here `f` has signature `f(::Value, ::State) => Tuple{NewValue,NewState}`.
"""
function modify_stateful end

@generated function modify_stateful(f::F, (obj, state)::T, optic::Properties) where {T,F}
_modify_stateful_inner(T)
end

# Separated for testing object/state combinations without restarts
function _modify_stateful_inner(::Type{<:Tuple{O,S}}) where {O,S}
modifications = []
vals = Expr(:tuple)
fns = fieldnames(O)
for (i, fn) in enumerate(fns)
v = Symbol("val$i")
st = if S <: ContextState
if O <: Tuple
:(ContextState(state.vals, obj, StaticInt{$(QuoteNode(fn))}()))
else
:(ContextState(state.vals, obj, StaticSymbol{$(QuoteNode(fn))}()))
end
else
:state
end
ms = :(($v, state) = f(getfield(props, $(QuoteNode(fn))), $st))
push!(modifications, ms)
push!(vals.args, v)
end
patch = O <: Tuple ? vals : :(NamedTuple{$fns}($vals))
start = :(props = getproperties(obj))
rest = MacroTools.@q begin
jw3126 marked this conversation as resolved.
Show resolved Hide resolved
patch = $patch
new_obj = maybesetproperties(state, obj, patch)
return (new_obj, state)
end
Expr(:block, start, modifications..., rest)
end

maybesetproperties(state, obj, patch) = setproperties(obj, patch)
maybesetstate(state, obj, patch) = state

abstract type AbstractQuery end

"""
Query(select, descend, optic)
Query(; select=Any, descend=x -> true, optic=Properties())

Query an object recursively, choosing fields where `select`
returns `true`, and descending when `descend` returns `true`.

```jldoctest
julia> using Accessors

julia> q = Query(; select=x -> x isa Int, descend=x -> x isa Tuple)
Query{var"#5#7", var"#6#8", Properties}(var"#5#7"(), var"#6#8"(), Properties())

julia> obj = (7, (a=17.0, b=2.0f0), ("3", 4, 5.0), ((x=19, a=6.0,)), [1])
(7, (a = 17.0, b = 2.0f0), ("3", 4, 5.0), (x = 19, a = 6.0), [1])

julia> q(obj)
(7, 4)
```
$EXPERIMENTAL
"""
struct Query{Select,Descend,Optic<:Union{ComposedOptic,Properties}} <: AbstractQuery
select_condition::Select
descent_condition::Descend
optic::Optic
end
Query(select, descend=x -> true) = Query(select, descend, Properties())
Query(; select=Any, descend=x -> true, optic=Properties()) = Query(select, descend, optic)

OpticStyle(::Type{<:AbstractQuery}) = SetBased()

struct ContextState{V,O,FN}
vals::V
obj::O
fn::FN
end
struct GetAllState{V}
vals::V
end
struct SetAllState{C,V,I}
change::C
vals::V
itr::I
end

const GetStates = Union{GetAllState,ContextState}

@inline pop(x) = first(x), Base.tail(x)
@inline push(x, val) = (x..., val)
@inline push(x::GetAllState, val) = GetAllState(push(x.vals, val))
@inline push(x::ContextState, val) = ContextState(push(x.vals, val), nothing, nothing)

(q::Query)(obj) = _getall(obj, q)

function _getall(obj, q::Q) where Q<:Query
initial_state = GetAllState(())
_, final_state = let q=q
jw3126 marked this conversation as resolved.
Show resolved Hide resolved
modify_stateful((obj, initial_state), q) do o, s
new_state = push(s, outer(q.optic, o, s))
o, new_state
end
end
final_state.vals
end

function set(obj, q::Q, vals) where Q<:Query
initial_state = SetAllState(Unchanged(), vals, 1)
final_obj, _ = let obj=obj, q=q, initial_state=initial_state
jw3126 marked this conversation as resolved.
Show resolved Hide resolved
modify_stateful((obj, initial_state), q) do o, s
new_output = outer(q.optic, o, s)
new_state = SetAllState(Changed(), s.vals, s.itr + 1)
new_output, new_state
end
end
return final_obj
end

function context(f::F, obj, q::Q) where {F,Q<:Query}
initial_state = ContextState((), nothing, nothing)
_, final_state = let f=f
modify_stateful((obj, initial_state), q) do o, s
new_state = push(s, f(s.obj, known(s.fn)))
o, new_state
end
end
return final_state.vals
end

modify(f, obj, q::Query) = set(obj, q, map(f, q(obj)))

@inline function modify_stateful(f::F, (obj, state), q::Q) where {F,Q<:Query}
let f=f, q=q
modify_stateful((obj, state), inner(q.optic)) do o, s
if (q::Q).select_condition(o)
(f::F)(o, s)
elseif (q::Q).descent_condition(o)
ds = descent_state(s)
o, ns = modify_stateful(f::F, (o, ds), q::Q)
o, merge_state(ds, ns)
else
o, s
end
Comment on lines +464 to +473
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for pushing this further. I also played around with implementing this, see this branch:
https://github.com/JuliaObjects/Accessors.jl/tree/modify_stateful
Like with your implementation I encountered inference issues. I do not fully understand them, but it seems the compiler dislikes this switch. To explore what's going on I replaced elseif (q::Q).descent_condition(o) into elseif true but the thing would still not infer. OTOH manually deleting the elseif branch did help the compiler to infer.

Note I did this experimentation with my branch, which is somewhat different then yours, but I guess has the same underlying issues. Worth checking if things are better with the new ConstructionBase version and nightly julia.

If not I think it would be good to produce an MWE and report it upstream. I don't know if it is considered a bug, but maybe we'll get an explanation of why it is this way and how to work around it.

A quick and dirty workaround would be to introduce a more narrow query where you can only select and descent based on type. Then the if elseif ... could be replaced by three methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok interesting we both hit the same problem.

I think the differences are mostly just my code to handle context and optional construction in setall. But those aren't whats causing the instability...

It seems like the problem is calling modify_stateful recursively? It still seems broken even without the elseif, and the elseif was fine in the previous version. I don't understand exactly what is different between these implementations and my previous one or Flatten.jl, but it could be the additional complexity of the return objects? Or there is a little more happening at runtime in modify_stateful than I had in mapobject or Flatten.jl, and the compiler is giving up on something?

I also saw no difference between ConstructionBase versions.

Copy link
Member Author

@rafaqz rafaqz Jun 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems its the layers of anonymous functions. Flatten.jl has the shared code inside the generated functions, and simple separate generated functions for getall/setall (flatten/reconstruct).

I much prefer the clarity of the current solution here. But it seems like if we want this to compile away we cant use anonymous functions inside the recursion.

I can't get this version to compile away even if you don't return any of the updated objects - so Julia can't even tell that the anonymous function is not modifying external state somehow and runs it even if it's not used.

Copy link
Member

@jw3126 jw3126 Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not seem to be related to anonymous functions per see. I played with eliminating all anonymous functions.
I replaced code like

modify_stateful(...) do os
    do_stuff(os, captured, variables)
end

with code like

struct DoStuff{C,V}
    captured::C
    variables::V
end
(f::DoStruff)(os) = do_stuff(os, f.captured, f.variables)
modify_stateful(DoStuff(captured, variables), ...)

It helped in very small examples, small examples still fail. Also, @inline everything did not help. Also eliminating all @generated functions by manually defining all ConstructionBase.f(::MyObject, ...) did not help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, that's exhaustive.

It feels like the compiler is using some heuristic to give up on solving the returned state of the recursion instead of just trying. A single non-recursive modify_stateful compiles away completely, but it wont if called recursively.

Copy link
Member Author

@rafaqz rafaqz Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, think i found the issue. If you define the select and descent conditions like this it's type stable:

select_condition(x::NamedTuple) = true
select_condition(x) = false

descent_condition(x::Tuple) = true
descent_condition(x) = false

So the compiler optimises these methods differently to methods that are passed in in any way. Even with the function in the type instead of fields, or just passing a Type to a 2 arg version of descend_condition still doesn't work. It's just giving up and returning Any. If you actually include the types to pass to 2 arg descend_condition(T, x) in the code, it's type-stable.

I had assumed previously that using a type from a type parameters was practically the same as writing the type in the code manually, but it seems like it isn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome that you found the issue! This looks like a bug to me, can you maybe produce a MWE and report it upstream?

Copy link
Member Author

@rafaqz rafaqz Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm cutting it down to size, but its probably 1.6 inference problems like JuliaLang/julia#35800.

It's all type stable on 1.4 - that's why this feels weird for performance intuition. These things used to just work.

Actually no its still all #35800, even on 1.4!! if you compile if with the types hard coded once then it knows when you use revise with the types as parameters, and is type stable. Then it's not on restart.

end
end
end

maybesetproperties(state::GetStates, obj, patch) = obj
maybesetproperties(state::SetAllState, obj, patch) =
maybesetproperties(state.change, state, obj, patch)
maybesetproperties(::Changed, state::SetAllState, obj, patch) = setproperties(obj, patch)
maybesetproperties(::Unchanged, state::SetAllState, obj, patch) = obj

descent_state(state::SetAllState) = SetAllState(Unchanged(), state.vals, state.itr)
descent_state(state) = state

merge_state(s1::SetAllState, s2) = SetAllState(anychanged(s1, s2), s2.vals, s2.itr)
merge_state(s1, s2) = s2

anychanged(s1, s2) = anychanged(s1.change, s2.change)
anychanged(::Unchanged, ::Unchanged) = Unchanged()
anychanged(::Unchanged, ::Changed) = Changed()
anychanged(::Changed, ::Unchanged) = Changed()
anychanged(::Changed, ::Changed) = Changed()

inner(optic) = optic
inner(optic::ComposedOptic) = optic.inner

outer(optic, o, state::GetStates) = o
outer(optic::ComposedOptic, o, state::GetStates) = optic.outer(o)
outer(optic::ComposedOptic, o, state::SetAllState) = set(o, optic.outer, state.vals[state.itr])
outer(optic, o, state::SetAllState) = state.vals[state.itr]


################################################################################
##### Lenses
################################################################################
Expand Down
4 changes: 4 additions & 0 deletions src/setindex.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ Base.@propagate_inbounds function setindex(args...)
Base.setindex(args...)
end

Base.@propagate_inbounds function setindex(xs::NamedTuple{K}, v, i::Int) where K
Base.setindex(xs, v, K[i])
end

@inline setindex(::Base.RefValue, val) = Ref(val)

Base.@propagate_inbounds function setindex(xs::AbstractArray, v, I...)
Expand Down
98 changes: 95 additions & 3 deletions src/sugar.jl
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export @set, @optic, @reset, @modify
export @set, @optic, @reset, @modify, @getall, @setall
using MacroTools

"""
Expand Down Expand Up @@ -84,13 +84,106 @@ end
This function can be used to create a customized variant of [`@modify`](@ref).
See also [`opticmacro`](@ref), [`setmacro`](@ref).
"""

function modifymacro(optictransform, f, obj_optic)
f = esc(f)
obj, optic = parse_obj_optic(obj_optic)
:(($modify)($f, $obj, $(optictransform)($optic)))
end

"""
@getall [x for x in obs if f(x)]

Get each `x` in `obj` that matches the condition `f`.

This can be combined with other optics, e.g.

```julia
julia> using Accessors

julia> obj = ("1", 2, 3, (a=4, b="5"))
("1", 2, 3, (a = 4, b = "5"))

julia> @getall (x for x in obj if x isa Number && iseven(x))
(2, 4)
```
"""
macro getall(ex)
getallmacro(ex)
end
macro getall(ex, descend)
getallmacro(ex; descend=descend)
end

function getallmacro(ex; descend=true)
# Wrap descend in an anonoymous function
descend = :(descend -> $descend)
if @capture(ex, (lens_ for var_ in obj_ if select_))
select = _select(select, var)
optic =_optics(lens)
:(Query($select, $descend, $optic)($(esc(obj))))
elseif @capture(ex, [lens_ for var_ in obj_ if select_])
select = _select(select, var)
optic =_optics(lens)
:([Query($select, $descend, $optic)($(esc(obj)))...])
elseif @capture(ex, (lens_ for var_ in obj_))
select = _ -> false
optic = _optics(lens)
:(Query($select, $descend, $optic)($(esc(obj))))
elseif @capture(ex, [lens_ for var_ in obj_])
select = _ -> false
optic = _optics(lens)
:([Query($select, $descend, $optic)($(esc(obj)))...])
else
error("@getall must be passed a generator or array comprehension")
end
end

# Turn this into an anonoymous function so it
# doesn't matter which argument val is in
_select(select, val) = :($(esc(val)) -> $(esc(select)))
function _optics(ex)
obj, optic = parse_obj_optic(ex)
:($optic ∘ Properties())
end

"""
@setall [x for x in obs if f(x)] = values

Set each `x` in `obj` matching the condition `f`
to values from the `Tuple` or vector `values`.

# Example

Used combination with lenses to set the `b` field of the
second item of all `Tuple`:

```jldoctest
julia> using Accessors

julia> obj = ("x", (1, (a = missing, b = :y), (2, (a = missing, b = :b))))
("x", (1, (a = missing, b = :y), (2, (a = missing, b = :b))))

julia> @setall (x[2].b for x in obj if x isa Tuple) = (:x, :a)
("x", (1, (a = missing, b = :x), (2, (a = missing, b = :b))))
```
"""
macro setall(ex)
setallmacro(ex)
end

function setallmacro(ex)
if @capture(ex, ((lens_ for var_ in obj_ if select_) = vals_))
select = _select(select, var)
optic =_optics(lens)
:(set($(esc(obj)), Query(; select=$select, optic=$optic), $(esc(vals))))
elseif @capture(ex, ((lens_ for var_ in obj_) = vals_))
optic = _optics(lens)
:(set($(esc(obj)), Query(; optic=$optic), $(esc(vals))))
else
error("@setall must be passed a generator")
end
end

foldtree(op, init, x) = op(init, x)
foldtree(op, init, ex::Expr) =
op(foldl((acc, x) -> foldtree(op, acc, x), ex.args; init=init), ex)
Expand Down Expand Up @@ -310,4 +403,3 @@ function show_composition_order(io::IO, optic::ComposedOptic)
show_composition_order(io, optic.inner)
print(io, ")")
end

Loading