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

Allow ... outside call #48750

Closed
wants to merge 2 commits into from
Closed

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Feb 21, 2023

Closes #48738

What

  • Before:
julia> +((1, 2)...)
3

julia> 0, (1, 2)... # if this works...
(0, 1, 2)

julia>  (1, 2)... # Then I think this should also work
ERROR: syntax: "..." expression outside call around REPL[3]:1
  • After:
julia> +((1, 2)...)
3

julia> 0, (1, 2)...
(0, 1, 2)

julia> (1, 2)... # <------ The new behaviour.
(1, 2)

Why?

I want this because SimpleUnderscores.jl provides a macro @-> which is meant to act like an anonymous function, but macros have different precedence than -> so we get this:

julia> dump(:(x -> x, 1))
Expr
  head: Symbol tuple
  args: Array{Any}((2,))
    1: Expr
      head: Symbol ->
      args: Array{Any}((2,))
        1: Symbol x
        2: Expr
          head: Symbol block
          args: Array{Any}((2,))
            1: LineNumberNode
              line: Int64 1
              file: Symbol REPL[6]
            2: Symbol x
    2: Int64 1

julia> dump(:(@-> x, 1))
Expr
  head: Symbol macrocall
  args: Array{Any}((3,))
    1: Symbol @->
    2: LineNumberNode
      line: Int64 1
      file: Symbol REPL[7]
    3: Expr
      head: Symbol tuple
      args: Array{Any}((2,))
        1: Symbol x
        2: Int64 1

This means that if @-> wants to mimic the behaviour of ->, it needs to splat out multiple arguments but that currently causes an error if done outside of a :call expression.

  • Before this PR:
julia> using SimpleUnderscores

julia> map(@-> _, [1,])
1-element Vector{Int64}:
 1

julia> (@-> _, [1,])
ERROR: syntax: "..." expression outside call around REPL[6]:1
Stacktrace:
 [1] top-level scope
   @ REPL[6]:1
  • After this PR:
julia> using SimpleUnderscores

julia> map(@-> _, [1,])
1-element Vector{Int64}:
 1

julia> (@-> _, [1,])
(var"#9#10"(), [1])

@MasonProtter
Copy link
Contributor Author

This seems to cause problems with these tests for REPL completions: https://github.com/JuliaLang/julia/blob/master/stdlib/REPL/test/replcompletions.jl#L734-L749

Does anyone know how to diagnose what's going on here that would cause that?

@JeffBezanson
Copy link
Member

This may be a good idea anyway, but I think the macro precedence issue is not fixable. The problem is @-> (x, 1) parses the same as @-> x, 1.

@@ -2766,7 +2767,8 @@
'.>>>= lower-update-op

'|...|
(lambda (e) (error "\"...\" expression outside call"))
(lambda (e)
(expand-tuple (cadr e)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(expand-tuple (cadr e)))
(expand-tuple (cons 'tuple (cdr e))))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I tied at first, but when I do that I get

julia> (1,2)...
((1, 2),)

instead of the desired (1, 2).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe (list 'tuple e)?

@MasonProtter
Copy link
Contributor Author

This may be a good idea anyway, but I think the macro precedence issue is not fixable. The problem is @-> (x, 1) parses the same as @-> x, 1.

Yeah, I also came upon that unfortunate realization earlier today. Chris may have a way out here though: #36547 (comment)

@Liozou
Copy link
Member

Liozou commented Mar 9, 2023

This seems to cause problems with these tests for REPL completions: https://github.com/JuliaLang/julia/blob/master/stdlib/REPL/test/replcompletions.jl#L734-L749

Does anyone know how to diagnose what's going on here that would cause that?

I don't read femtolisp unfortunately, but REPL completions diagnostic is right up my alley! Unfortunately, that code is quite brittle when handling varargs...

What happens in general is that, at some point, the type of something like x... needs to be evaluated. This ends up hitting the code at

newsym = try
Meta.lower(fn, sym)
catch e
# user code failed in lowering (ignore it)
return Any, false
where it tries to lower (x...), but fails since it's a splat outside a call. So the catch part is executed and the whole get_type procedure returns a cautious Any as the expected type of the expression. In summary, a splat is interpreted as a value of type Any.
But your PR makes it not break! So it happily returns a definite Tuple{DataType} (in the case of the failing test where x is Integer[]).

The root issue is that both answers are wrong: what should be returned is Vararg{Integer} (in this case), or at least Vararg{Any}.

A definite fix is included in #44434, but that PR is complicated and it's larger than your issue. Some fix specific to varargs completion could be extracted from 6a1bfbf I believe.

Anyway, as a quick-fix for this PR, I suggest adding the following, which simply forces any vararg expression to be interpreted as a value of type Any in REPL completion (the current somewhat broken behaviour):

diff --git a/stdlib/REPL/src/REPLCompletions.jl b/stdlib/REPL/src/REPLCompletions.jl
index 34ce7ad992..ec95723550 100644
--- a/stdlib/REPL/src/REPLCompletions.jl
+++ b/stdlib/REPL/src/REPLCompletions.jl
@@ -481,6 +481,8 @@ function try_get_type(sym::Expr, fn::Module)
         return try_get_type(sym.args[end], fn)
     elseif sym.head === :escape || sym.head === :var"hygienic-scope"
         return try_get_type(sym.args[1], fn)
+    elseif sym.head === :...
+        return (Any, true)
     end
     return (Any, false)
 end

@brenhinkeller brenhinkeller added the parser Language parsing and surface syntax label Aug 6, 2023
@MasonProtter MasonProtter deleted the splat branch October 30, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider letting x... mean (x...,)
4 participants