-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
JuliaSyntax: fix anonymous function parsing #60221
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
base: master
Are you sure you want to change the base?
Conversation
|
Just for reference, @KristofferC just touched this code in JuliaLang/JuliaSyntax.jl#580 |
e5769e5 to
4b253d6
Compare
|
The fix here has become quite complicated.. @c42f @KristofferC |
|
Would it be possible to add a unit test for this? (incl. for the previous change that affected this part of the code) Did we actually find out where the error comes from (by bisecting starting from Julia-1.12)? |
|
There are a couple of tests added already. I believe the issue was introduced by JuliaLang/JuliaSyntax.jl#580 |
4b253d6 to
2f3972c
Compare
2f3972c to
28bf44b
Compare
| # Check if there's a newline between `)` and the next `(` or `.`. | ||
| # We need to find where `)` is and check what immediately follows it. | ||
| # If peek(1, skip_newlines=false) is `)`, we're directly before it. | ||
| # Otherwise there's whitespace/newline before `)`. | ||
| next_token_pos = if peek(ps, 1, skip_newlines=false) == K")" | ||
| # Directly before ), token after ) is at 2 | ||
| 2 | ||
| else | ||
| # There's whitespace before ), so ) is at 2 | ||
| # and what follows ) is at 3 | ||
| 3 | ||
| end | ||
| token_after_paren = peek(ps, next_token_pos, skip_newlines=false) | ||
| # If token_after_paren is a newline, this is an anonymous function | ||
| has_newline_after_paren = _maybe_grouping_parens && token_after_paren == K"NewlineWs" | ||
| # Get the next significant token to determine if we need to parse a call | ||
| next_kind = peek(ps, 2, skip_newlines=_maybe_grouping_parens && !has_newline_after_paren) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make a lot of sense to split the logic with a case distinction on _maybe_grouping_parens:
| # Check if there's a newline between `)` and the next `(` or `.`. | |
| # We need to find where `)` is and check what immediately follows it. | |
| # If peek(1, skip_newlines=false) is `)`, we're directly before it. | |
| # Otherwise there's whitespace/newline before `)`. | |
| next_token_pos = if peek(ps, 1, skip_newlines=false) == K")" | |
| # Directly before ), token after ) is at 2 | |
| 2 | |
| else | |
| # There's whitespace before ), so ) is at 2 | |
| # and what follows ) is at 3 | |
| 3 | |
| end | |
| token_after_paren = peek(ps, next_token_pos, skip_newlines=false) | |
| # If token_after_paren is a newline, this is an anonymous function | |
| has_newline_after_paren = _maybe_grouping_parens && token_after_paren == K"NewlineWs" | |
| # Get the next significant token to determine if we need to parse a call | |
| next_kind = peek(ps, 2, skip_newlines=_maybe_grouping_parens && !has_newline_after_paren) | |
| next_kind = if _maybe_grouping_parens | |
| # Check if there's a newline between `)` and the next `(` or `.`. | |
| # We need to find where `)` is and check what immediately follows it. | |
| # If peek(1, skip_newlines=false) is `)`, we're directly before it. | |
| # Otherwise there's whitespace/newline before `)`. | |
| next_token_pos = if peek(ps, 1, skip_newlines=false) == K")" | |
| # Directly before ), token after ) is at 2 | |
| 2 | |
| else | |
| # There's whitespace before ), so ) is at 2 | |
| # and what follows ) is at 3 | |
| 3 | |
| end | |
| token_after_paren = peek(ps, next_token_pos, skip_newlines=false) | |
| # If token_after_paren is a newline, this is an anonymous function | |
| # Get the next significant token to determine if we need to parse a call | |
| peek(ps, 2, skip_newlines= token_after_paren != K"NewlineWs") | |
| else | |
| # Get the next significant token to determine if we need to parse a call | |
| peek(ps, 2, skip_newlines=false) | |
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsequently, it makes sense to inspect more closely what happens if _maybe_grouping_parens is true: If the next tokens are:
K")", K"NewlineWs", then we callpeek(ps, 2, skip_newlines=false) = token_after_parenK")", !K"NewlineWs", then we callpeek(ps, 2, skip_newlines=true) = token_after_paren!K")", x/*we expect K")" here, but don't check that*/, K"NewlineWs", then we callpeek(ps, 2, skip_newlines=false)=token_after_paren(I think (hope I interpret it correctly) that__lookahead_indextreats the case withskip_newlines=truesuch that it also skips newlines entirely, including in the count up ton=2)!K")", x, !K"NewlineWs", then we callpeek(ps, 2, skip_newlines=false)=token_after_paren`
So, I think we can omit the peek(ps, 2, skip_newlines= token_after_paren != K"NewlineWs") entirely.
| # Check if there's a newline between `)` and the next `(` or `.`. | |
| # We need to find where `)` is and check what immediately follows it. | |
| # If peek(1, skip_newlines=false) is `)`, we're directly before it. | |
| # Otherwise there's whitespace/newline before `)`. | |
| next_token_pos = if peek(ps, 1, skip_newlines=false) == K")" | |
| # Directly before ), token after ) is at 2 | |
| 2 | |
| else | |
| # There's whitespace before ), so ) is at 2 | |
| # and what follows ) is at 3 | |
| 3 | |
| end | |
| token_after_paren = peek(ps, next_token_pos, skip_newlines=false) | |
| # If token_after_paren is a newline, this is an anonymous function | |
| has_newline_after_paren = _maybe_grouping_parens && token_after_paren == K"NewlineWs" | |
| # Get the next significant token to determine if we need to parse a call | |
| next_kind = peek(ps, 2, skip_newlines=_maybe_grouping_parens && !has_newline_after_paren) | |
| next_token_pos = if _maybe_grouping_paren | |
| # Check if there's a newline between `)` and the next `(` or `.`. | |
| # We need to find where `)` is and check what immediately follows it. | |
| # If peek(1, skip_newlines=false) is `)`, we're directly before it. | |
| # Otherwise there's whitespace/newline before `)`. | |
| if peek(ps, 1, skip_newlines=false) == K")" | |
| # Directly before ), token after ) is at 2 | |
| 2 | |
| else | |
| # There's whitespace before ), so ) is at 2 | |
| # and what follows ) is at 3 | |
| 3 | |
| end | |
| else | |
| 2 | |
| end | |
| # Get the next significant token to determine if we need to parse a call | |
| next_kind = peek(ps, next_token_pos, skip_newlines=false) |
Fixes #60202
Written by Claude