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

Consider newlines as spaces for space-sensitive parsing #20265

Merged
merged 10 commits into from
Jul 24, 2018

Conversation

TotalVerb
Copy link
Contributor

@TotalVerb TotalVerb commented Jan 26, 2017

Fix #16594 by treating newlines, tabs, etc. as spaces that make binary parsing take precedence over unary parsing in space-sensitive situations.

cc @stevengj

@kshyatt kshyatt added the parser Language parsing and surface syntax label Jan 27, 2017
@stevengj
Copy link
Member

Looks good to me, but @JeffBezanson should probably take a look, too.

@stevengj
Copy link
Member

Note that #16594 occurred only for the special case of operators that are parsed as both unary and binary, so a more conservative version of this PR would just change the unary-and-binary-ops case.

@TotalVerb
Copy link
Contributor Author

This issue manifested in a different way for colon. It's minor, but the new behaviour is better.

Before:

julia> [1 :
       2]
1×2 Array{Int64,2}:
 1  2

Now:

julia> [1 :
       2]
ERROR: syntax: line break in ":" expression

(this case is tested)

As for ~, it is both a unary and binary operator, just a strange one, so #16594 does indeed apply.

Old

julia> :(@test 1 ~
               2)
ERROR: syntax: missing comma or ) in argument list

New

julia> :(@test 1 ~
               2)
:(@test @~(1,2))

@TotalVerb
Copy link
Contributor Author

The initial version of this PR was incorrect, leading to test failures. I have corrected this PR.

@StefanKarpinski
Copy link
Member

Failure is macOS-only. Probably unrelated.

@StefanKarpinski
Copy link
Member

bump @JeffBezanson

@tkelman
Copy link
Contributor

tkelman commented Apr 28, 2017

Jeff, please review, do we want this one or not?

@stevengj
Copy link
Member

stevengj commented Mar 3, 2018

Bump.

@stevengj stevengj added the triage This should be discussed on a triage call label Jul 19, 2018
@JeffBezanson
Copy link
Member

This example from the linked issue seems like the biggest problem:

julia> [1 + 
       2 0]
1×2 Array{Int64,2}:
 3  0

(where the trailing space on the first line makes a difference). I wonder if it's possible to fix just that?
@stevengj Do you want to take a look at this? Otherwise I feel we're running out of time and we can't really deal with this.

@stevengj
Copy link
Member

I fixed the merge conflict.

@stevengj
Copy link
Member

stevengj commented Jul 19, 2018

The change seems pretty reasonable to me. Breaking, but unlikely to affect working real-world code. It is 11th hour but this one has been hanging around for a decision for a while…

@StefanKarpinski
Copy link
Member

Failure is real—does not pass parsing tests.

@TotalVerb
Copy link
Contributor Author

I think the issue is that the introduced space-before-next-token? consumes the whitespace unlike the existing peek-char test. It can probably be resolved by testing the next character against the spaces without skipping them? I forget why I introduced the skip-ws now. I can take a quick look.

@TotalVerb
Copy link
Contributor Author

Hold on, it looks like all the test failures are precisely the added tests (which are testing the issue that this fixes) which have bitrotted over time. The argument count for each of them is precisely off by one because the macro syntax has changed. The string one must have had a few extra quotes once upon a time. Let's see if the new commit fixes it.

@stevengj
Copy link
Member

The :([x .+ y]) parsing had changed to a vect node, so I fixed that test (I hope) as well.

@TotalVerb
Copy link
Contributor Author

One last test it looks like... This one because of ParseError binding deprecation.

@TotalVerb
Copy link
Contributor Author

@StefanKarpinski good to go now?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jul 22, 2018

macOS failure is #26725. This isn't my call—@JeffBezanson will have to take a look and decide.

test/parse.jl Outdated
1).args) == 3
@test length(:(@x 1 +
1 +
1).args) == 3
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer these tests to check more than just the number of arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, just check Expr equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have improved the tests using @stevengj's suggestion.

@JeffBezanson JeffBezanson merged commit bb443fe into JuliaLang:master Jul 24, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 24, 2018
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.

6 participants