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

Behavior of _ is confusing with infix operators #6

Closed
Arkoniak opened this issue Apr 22, 2020 · 15 comments
Closed

Behavior of _ is confusing with infix operators #6

Arkoniak opened this issue Apr 22, 2020 · 15 comments
Labels
documentation Improvements or additions to documentation

Comments

@Arkoniak
Copy link

Not sure whether it's a bug or wrong usage of Underscores.jl, but following example fails

v = [(x = rand(), y = rand()) for _ in 1:10]
@_ v |> sum(_.x, __) / length(__)

ERROR: MethodError: no method matching /(::var"#56#58"{Array{NamedTuple{(:x, :y),Tuple{Float64,Float64}},1}}, ::Int64)
Closest candidates are:
  /(::Missing, ::Number) at missing.jl:115
  /(::BigInt, ::Union{Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}) at gmp.jl:537
  /(::BigFloat, ::Union{Int16, Int32, Int64, Int8}) at mpfr.jl:441
@haberdashPI
Copy link

Just happened to see this; I can answer your question (though I'm not the author of this package).

The error indicates that / cannot accept an anonymous function (::var" bla bla...) as its first argument. This is not parsing the way you expect.

The problem is that _ always consumes all but the last function call: / is a function call, so your code actually parses to the equivalent of the following.

v |> a -> ((b -> sum(b.x, a)) / length(a))

Presumably you wanted this.

v |> a -> (sum(b -> b.x, a)) / length(a))

@Arkoniak
Copy link
Author

Yes, you are right.
It's not even related to the __, as you correctly said the problem is in the function /:

v = [(x = rand(), y = rand()) for _ in 1:10]
@_ v |> sum(_.x, __) / 10

produce the same error. So the question is, is it possible to do what I want with the current syntax or modify it somehow to solve this issue. Both answers are perfectly fine.

@c42f
Copy link
Owner

c42f commented Apr 23, 2020

I can see why this led to confusion: the / doesn't "look like" a function call, so the scope of where the _ expands to is surprising. Removing the infix notation perhaps makes things slightly clearer.

@_ v |> division(sum(_.x, __), length(__))

Unfortunately macros cannot see whether something "looks like a function call". I guess we could prohibit use of _ for operator-named functions and at least the errors would be clearer?

Regardless of all that, the following is vastly more readable to my eyes:

@_(sum(_.x, v)) / length(v)

@c42f
Copy link
Owner

c42f commented Apr 23, 2020

So... I think we can take this as either a documentation issue, or a code cleanup where we deprecate the use of _ when an operator-named Expr(:call) is the outermost call?

It's fiddly though: are there some operators which are higher order functions? is certainly one but it already has special treatment.

@c42f c42f changed the title Can't use __ twice? Behavior of _ is confusing with infix operators Apr 23, 2020
@c42f c42f added the documentation Improvements or additions to documentation label Apr 23, 2020
@c42f
Copy link
Owner

c42f commented Apr 23, 2020

I feel like maybe a pure documentation-based approach is the best for now. In the future macros may well gain the ability to "see the parentheses" with a more detailed introspection of the source code in some compiler work I'm doing.

So a much longer-term plan might be to disallow use of _ in syntax which doesn't have some kind of parentheses in the sourcecode.

@Arkoniak
Copy link
Author

To clarify, it was a part of a longer chain (sorry, I am spoiled by R). Expanding on MWE

v = [(x = rand(), y = rand()) for _ in 1:10]
@_ v |> filter(_.x > 0.5, __) |> sum(_.y, __)/length(__)

Of course it's not a problem, to use mean from Statistics, or to pause chain and do something like this

@_ v1 = v |> filter(_.x > 0.5, __)
@_ (sum(_.x, v1)) / length(v1)

but it felt rather natural(?) to use underscore this way.

Is it possible to do a rule like "any __ should be changed to a new variable, no matter where they appear"? Something like @_ Expr(..., __, ...) changes to newvar -> Expr(..., newvar, ....) or it clashes with some other usage of __?

@c42f
Copy link
Owner

c42f commented Apr 23, 2020

Is it possible to do a rule like "any __ should be changed to a new variable, no matter where they appear"?

I think the __ is already working just like you expect. It's the single _ which is confusing you:

Double underscore:

julia> @macroexpand @_ df |> a(__)/b(__)
:(df |> ((__1,)->begin
              a(__1) / b(__1)
          end))

Single underscore:

julia> @macroexpand @_ df |> sum(_, xs)/len
:(df |> ((_1,)->begin
                  sum(_1, xs)
              end) / len)

see what's happened? It's that @_( sum(_,xs)/Y ) expands to (y->sum(y,xs) ) / Y rather than (sum(y->y,xs) ) / Y.

So the problem is the "outermost-but-one" rule for expanding the _. The division is treated as the outermost call, so the expansion thinks it should be passing an anonymous function to the division rather than the sum.

Of course, as programmers we can guess this is complete nonsense! But from a syntax tree point of view it's the consistent rule the whole expansion is based around.

As I said above, it could be hacked around by not treating operator function names as valid calls which can accept an anonymous function.

But this would be heuristic, because a/b can equivalently be written /(a,b)and the macro can't tell which one was used!

@Arkoniak
Copy link
Author

Thank you for the explanation!

@c42f
Copy link
Owner

c42f commented Apr 27, 2020

So... deciding what to do about this is really tricky and I didn't anticipate encountering this problem with an explicit @_ available to mark the boundary.

Perhaps I should have — almost exactly the same problem has been discussed at length in JuliaLang/julia#24990 (though for tight-binding rather than loose binding).

@mcabbott
Copy link
Collaborator

While infix operators are ordinary function calls, Julia does seem to regard the infix form as the standard one:

julia> :( +(2, 3, *(4, 5)) )
:(2 + 3 + 4 * 5)

Perhaps that supports the idea of always treating them this way, and thus letting the macro work inwards until it finds an honest prefix function call.

Are there any infix operators which would accept a function? Besides and |> which are already handled. (I mean operators in common use, if not in Base. Clearly you could define e.g. ⊞ = sum; sqrt ⊞ 2ones(10) if you wanted to.)

@c42f
Copy link
Owner

c42f commented May 26, 2020

Perhaps that supports the idea of always treating them this way

Yes, agreed this is the right thing (as mentioned in #8 (comment)). BTW we have Base.isoperator() so actually detecting operator names is easy.

Note that and |> are handled differently because they're barriers for __.

@Arkoniak
Copy link
Author

Arkoniak commented Jun 7, 2020

I do not know, whether it would be useful, but I've got another similar (I suppose) issue.

x = collect(1:3:20)
findfirst(z -> z > 15, x)
@_ x |> __[findfirst(_ > 15, __)]

# ERROR: ArgumentError: invalid index: var"#156#158"{Array{Int64,1}}([1, 4, 7, 10, 13, 16, 1
9]) of type var"#156#158"{Array{Int64,1}}

It looks similar to #8 and I hope #11 solve this case as well.

@mcabbott
Copy link
Collaborator

mcabbott commented Jun 7, 2020

That case isn't currently handled by #10, although I think it would make sense to add.
(Edit: now done.)

@c42f
Copy link
Owner

c42f commented Jun 8, 2020

I do not know, whether it would be useful, but I've got another similar (I suppose) issue.

Yes I think that's the same as #8 basically: we should recurse into both the A and I parts of an indexing expression A[I]

@c42f
Copy link
Owner

c42f commented Jul 8, 2020

Fixed in #10 thanks to @mcabbott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants