Support indexing involving end

[tkf]

(Edit: it now does not depend on GroundEffects.jl)

The idea of this PR is to factor out the common logic of lowering AST to an external package: https://github.com/tkf/GroundEffects.jl (still not registered yet)

ref: tkf/GroundEffects.jl#1

close #15

[jw3126]

Do you suggest to use GroundEffects just for e.g @lens _[end] or are there more situations where it could simplify lens parsing?

[tkf]

I think it's useful only for indexing with end like @lens _[end ÷ 2]. Most of the time, you don't want lowered AST; e.g., lowering a.b to getproperty(a, :b) is not useful for Setfield.jl. Lowering @lens _' to @lens adjoint(_) is another example where you need desugaring but it's probably easier to do it manually.

Since the part relevant to indexing with end is only less than 20 lines of code, it may be better to not rely on an external library. The code for lowering end in GroundEffects.jl relies on other facility but I guess you can do it less than 40 lines (it's a bit hairy when you support broadcasting; e.g., x[x.^2 .< end]).

[jw3126]

Yeah, I was wondering about that. GroundEffects.jl could be somewhat maintenance-hungry, as it must track base lowering? I think we can depend on GroundEffects for now and if GroundEffects becomes a problematic dependency copy paste the relevant lines.

[tkf]

somewhat maintenance-hungry, as it must track base lowering

Exactly. I am not sure if I want to keep maintaining it 😄. That's why it's not registered yet.

OTHO, Julia in 1.x may not have drastic changes in AST. It's supposed to be a public API. Also, I need something similar in LazyArrays.jl and probably in BangBang.jl. I don't want to keep (helping) maintaining three different code bases with overlapping functionality. The important question is how overlapping they would be; hence the PRs.

[tkf]

Actually, how about pulling relevant code from GroundEffects.jl for now? This is a useful feature and I want to use it now that I implemented it :)

Whether or not it makes sense to use GroundEffects.jl can be discussed/experimented later.

[jw3126]

Can you add some more complicated lenses? Composed and multi index? Also can you add show tests?

[tkf]

OK. I added a few more tests. I completed forgot about multi-index version so that's implemented/tested as well.

[jw3126]

e.g. @lens _[end, end]

[tkf]

Regarding show, I don't think there is much we can do or test (lambda is not parsable). As we know the original expr, it is in principle possible to do something similar to:

struct DynamicIndexLens{F, expr}
    f::F
end

DynamicIndexLens(f, expr) = DynamicIndexLens{typeof(f), Symbol(string(expr))}(f)

print_application(io::IO, ::DynamicIndexLens{<:Any, expr}) where {exprr} =
    print(io, "[", string(expr), "]")

But I don't think it's a good idea to waste compilation time just to get a nice show.

[jw3126]

Mhh you expect show would increase compile time by a noticeable amount? Implementation wise I would prefer not to make expr part of the type, but instead expand e.g. @lens _[end] into something like

let f = obj -> obj[end]
    lens = DynamicIndexLens(f)
    Setfield.print_application(io::IO, ::typeof(lens)) = "[end]"
    lens
end

[tkf]

That's an interesting idea. But I guess it doesn't work inside functions? Also, I think it's problematic when the indexing function is a closure

shift = 1
lens = @lens _[end - shift]

[tkf]

I guess it's doable if we handle the lowering of closure:

• Analyze all the free variables and create a callable type C for capturing them during macro evaluation (i.e., manually eval).
• Define Setfield.print_application(io::IO, ::DynamicIndexLens{<:C}) during macro evaluation (this needs manual eval too).
• Generate an expression :(DynamicIndexLens(C($(free_variables...)))).

This feels like the right way to do it but it's rather a non-trivial project for a somewhat auxiliary feature. So, I prefer to do this later (or maybe never unless there is a better idea) if you are OK with it.

[jw3126]

Yes you are right my show approach does not work and eval is probably needed. I agree its probably not worth the trouble so lets do this later/never.
Can you maybe add tests that not every indexing is dynamic? E.g. like @test @lens(_[1]) isa IndexLens? LGTM, feel free to merge.