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

parser bug: macro calls w/o parens in generator expressions #18650

Closed
StefanKarpinski opened this issue Sep 23, 2016 · 6 comments
Closed

parser bug: macro calls w/o parens in generator expressions #18650

StefanKarpinski opened this issue Sep 23, 2016 · 6 comments
Assignees
Labels
breaking This change will break code bug Indicates an unexpected problem or unintended behavior help wanted Indicates that a maintainer wants help on an issue or pull request needs decision A decision on this change is needed parser Language parsing and surface syntax
Milestone

Comments

@StefanKarpinski
Copy link
Member

julia> maximum([@elapsed sleep(1) for k = 1:3])
1.003554001

julia> maximum(@elapsed(sleep(1)) for k = 1:3)
1.006058621

julia> maximum(@elapsed sleep(1) for k = 1:3)
ERROR: syntax: unexpected ")"
@StefanKarpinski StefanKarpinski added bug Indicates an unexpected problem or unintended behavior parser Language parsing and surface syntax labels Sep 23, 2016
@StefanKarpinski StefanKarpinski added this to the 0.5.x milestone Sep 23, 2016
@StefanKarpinski StefanKarpinski added help wanted Indicates that a maintainer wants help on an issue or pull request and removed help wanted Indicates that a maintainer wants help on an issue or pull request labels Oct 27, 2016
@c42f
Copy link
Member

c42f commented Jan 1, 2017

Hmm. We want

:(@a k for k=1:2)            -> :(@a(k) for k=1:2)              (1)
:(@a k for k=1:2; k ; end)   -> :(@a(k, for k=1:2; k ; end))    (2)

Currently the comprehension version works by setting the inside-vect parser state to true so it can stop parsing the macro when seeing a 'for token in parse-space-separated-exprs. But this simple strategy won't work for generators because case (2) above is already valid syntax.

@JeffBezanson
Copy link
Member

Yes, I'm not sure this can be resolved. We use syntax like @threads x for i = 1:n ... where the for loop is an argument to the macro. The only possibility is to parse it differently inside parentheses, but then we have an expression that can't be parenthesized without changing its parsing, which seems like a bad idea to me.

@c42f
Copy link
Member

c42f commented Jan 2, 2017

Yes I imagined an inside-parens state might make sense and something inside parse-space-separated-exprs would just need to look further ahead to determine whether a given occurrence of for is a prefix of a for loop, or a generator expression.

But I'm not sure that kind of look ahead is compatible with the structure of the parser? It rather looks like it isn't.

@StefanKarpinski StefanKarpinski modified the milestones: 1.0, 0.5.x Jan 5, 2017
@Keno Keno added needs decision A decision on this change is needed breaking This change will break code labels Jul 20, 2017
@JeffBezanson
Copy link
Member

Looked into this a bit. I think there are basically three options:

  1. Do nothing --- only being inside square brackets can stop a macro from grabbing a for expression.
  2. Sacrifice (a; b; c) block syntax, as described in parser bug: macro calls w/o parens in generator expressions #18650 (comment). (@mac for i in x ... ) would always parse as a generator. Real-world example of code that would break:
broadcast!(f, X::AbstractArray) = (@inbounds for I in eachindex(X); X[I] = f(); end; X)

We could make @mac for (when the for loop is the first argument to the macro) special, but I think that's too fiddly.

  1. Change the parsing only inside call syntax, so f(@mac for i = ...) would pass a generator to f.

I think (3) is a pretty good option, as it fixes the example in the OP, and there is no real use for writing a for loop in a function argument list. However, the difference between function arguments and tuple arguments might be confusing.

@StefanKarpinski
Copy link
Member Author

I guess 3 does sound like the best option, followed by 1. I would be sad to lose (a; b; c) syntax.

@JeffBezanson
Copy link
Member

Potential change in #22943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code bug Indicates an unexpected problem or unintended behavior help wanted Indicates that a maintainer wants help on an issue or pull request needs decision A decision on this change is needed parser Language parsing and surface syntax
Projects
None yet
Development

No branches or pull requests

4 participants