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

Interest in a FieldLens for bypassing inner constructors? #179

Open
MasonProtter opened this issue Dec 2, 2024 · 12 comments
Open

Interest in a FieldLens for bypassing inner constructors? #179

MasonProtter opened this issue Dec 2, 2024 · 12 comments

Comments

@MasonProtter
Copy link
Contributor

So it makes sense that by default, Accessors.jl should be going through ConstructionBase and work in terms of properties, but I think there are times where it might make sense for a user to want to work in terms of fields instead of properties.

Furthermore, I think there are cases where you might want to make this choice at the call-site, rather than overloading constructorof and making that choice apply to all instances of a type. I made a demo of an optic for this on Discourse, and it's quite simple:

using Accessors: IndexLens, PropertyLens, ComposedOptic, setmacro, opticmacro, modifymacro

struct FieldLens{L}
    inner::L
end

(l::FieldLens)(o) = l.inner(o)
function Accessors.set(o, l::FieldLens{<:ComposedOptic}, val)
    o_inner = l.inner.inner(o)
    set(o_inner, FieldLens(l.inner.outer), val)
end
function Accessors.set(o, l::FieldLens{PropertyLens{prop}}, val) where {prop}
    setfield(o, val, Val(prop))
end

@generated function setfield(obj::T, val, ::Val{name}) where {T, name}
    fields = fieldnames(T)
    name  fields || error("$(repr(name)) is not a field of $T, expected one of ", fields)
    Expr(:new, T, (name == field ? :val : :(getfield(obj, $(QuoteNode(field)))) for field  fields)...)
end

macro setfield(ex)
    setmacro(FieldLens, ex, overwrite=false)
end
macro resetfield(ex)
    setmacro(FieldLens, ex, overwrite=true)
end

and then here's how you could use it on some struct:

struct EvenInt
    x::Int
    function EvenInt(x::Int)
        iseven(x) || throw(ArgumentError("Must be even."))
        sleep(1) # pretend this is useful processing
        new(x)
    end
end
julia> @resetfield y.x = 5 # bypassing the inner constructor!
EvenInt(5)

julia> @time @resetfield y.x = 4
  0.000005 seconds (1 allocation: 16 bytes)
EvenInt(4)

vs normal usage:

julia> @time y = EvenInt(2)
  1.002596 seconds (4 allocations: 112 bytes)
EvenInt(2)

julia> @reset y.x = 5
ERROR: ArgumentError: Must be even.

julia> @time @reset y.x = 4
  1.002588 seconds (5 allocations: 128 bytes)
EvenInt(4)

Is this something we might want to put into Accessors.jl? I just thought I should ask before I bother making a PR

@jw3126
Copy link
Member

jw3126 commented Dec 2, 2024

Yes, this looks useful and we can add it. I think I would prefer a more ugly name to make clear that this is dangerous stuff that can make your numbers odd.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Dec 2, 2024

Yeah. Maybe something like @unsafe_setfield / @unsafe_resetfield?

@jw3126
Copy link
Member

jw3126 commented Dec 2, 2024

Yes these sound fine.

@jw3126
Copy link
Member

jw3126 commented Dec 2, 2024

About implementation, I wonder if it would be better to do

struct FieldLens{field} end
function prop_to_field_lens(optic)
    ... # replace all occurences of PropertyLens by FieldLens
end

macro unsafe_setfield(ex)
    setmacro(prop_to_field_lens, ex, overwrite=false)
end

@MasonProtter
Copy link
Contributor Author

yeah that would make sense.

@aplavin
Copy link
Member

aplavin commented Dec 2, 2024

A relevant old PR with some discussion: #57. I agree such a functionality would be useful, and if others are fine with including the Expr(:new) internals hack, I also support it! :)

Also, it doesn't really need new macros IMO. Following the linked discussion, there are at least two ways to do it:

  1. Just add FieldLens, then the syntax is @set y |> FieldLens(:x) = newx. This allows adding a FieldLens to a set call that uses properties otherwise, like @set y.x[1] |> FieldLens(:a) = newval.
  2. No new functions/objects at all, use getfield(_, :a) (see support setting fields #57 implementation). It can either be type-unstable, or add a special getfield handling so that @set getfield($y, :x) = newx doesn't go to Base.Fix2.

@aplavin
Copy link
Member

aplavin commented Dec 2, 2024

Also, maybe setfield() from your first post should go to ConstructionBase.jl? It has setproperties() for example, that are used here.

@MasonProtter
Copy link
Contributor Author

Those alternatives sound kinda annoying to use. I'd rather scare users off by using a scary name rather than more awkward syntax.

@aplavin
Copy link
Member

aplavin commented Dec 2, 2024

Ofc these preferences are subjective, I just believe that "define a new macro" needs a much higher barrier than "define a new function/struct". Macros can do many nontrivial code transformations, while functions are much easier to reason about.
In this case, for a rare usage scenario, it just seems natural to reuse existing syntax IMO – rather than make everyone reading the code wonder what's this new macro (as it will be used much more rarely than @set).

@aplavin
Copy link
Member

aplavin commented Dec 2, 2024

Oh, and if adding field support in any capacity, then @set getfields(x) = (... newfields ...) should also be added and not forgotten :)
(getfields from ConstructionBase)

@jw3126
Copy link
Member

jw3126 commented Dec 6, 2024

I agree with @aplavin's argument. How about just adding

struct UnsafeFieldLens{field} end

and the way to use it is:

@set obj[i].f |> UnsafeFieldLens(:myfield) = val

About getfield and getfields happy to have them if they respect the inner constructor. If we want bypassing versions of them, these also need scary names.

@aplavin
Copy link
Member

aplavin commented Dec 6, 2024

I tend to agree with you regarding the desire for "scary names" for such a functionality :) Is the Expr(:new) trick even officially documented anywhere?
If yes, if it was the official way to reliably set field values disregarding the constructor, then it could make sense do use as the @set getfield() backend IMO. But I don't think Julia documents it at all, meaning we should be very careful when exposing it to users.

Regarding the macro/struct syntax, I think I understand now what subconsciously worried me in the suggested macro. In Accessors, y = @set f(x) = v is designed to result in f(y) == v. Ideally, for any callable f(x): @set x.a.b.c, @set first(x.a).b, @set x |> PropertyLens(:a) – in all of these cases, the invariant holds. It also holds in the custom optics example in https://juliaobjects.github.io/Accessors.jl/dev/examples/custom_macros/.
@resetfield y.x = 5 reads like y.x == 5 afterwards, which isn't the case (it sets fields, not properties). Meanwhile, both @reset y |> FieldLens(:x) and @reset y |> getfield(_, :x) would follow that invariant.

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

No branches or pull requests

3 participants