-
-
Notifications
You must be signed in to change notification settings - Fork 43
Fix multiline function signature parsing #580
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: main
Are you sure you want to change the base?
Conversation
491f3f7 to
ed5d06a
Compare
src/julia/parser.jl
Outdated
| _needs_parse_call = peek(ps, 2) ∈ KSet"( ." | ||
| # Check if we should skip newlines - only for specific cases | ||
| # where we have a single type annotation like (::T) | ||
| _skip_newlines = !had_commas && num_subexprs == 1 && |
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'll be honest in that I don't really know what is going on here...
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.
This is the same condition as the maybe_grouping_parens below - should be refactored if that's intended.
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.
So the odd thing about this is that it needs to ignore whitespace before the first peek token, but not the second one. I don't know that these conditions here make any sense. We don't have a peek api like that, but I think that's what's needed.
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 the correct solution is to move the _needs_parse_call outside the do block - it seems weird that it would affect needs_parameters, because if there are parameters, the peek will never actually see the parenthesis.
ed5d06a to
2cca490
Compare
Multiline function signatures with type annotations were incorrectly
parsed as tuples instead of calls when newlines appeared between
parentheses. For example:
```julia
function (
::A
)()
end
```
was parsed as `(function (tuple ...) (block))` instead of the correct
`(function (call (parens ...)) (block))`, inconsistent with the
single-line version `function (::A)() end`.
The issue was in parse_function_signature where `peek(ps, 2)` was used
to detect if a call pattern follows the closing parenthesis, but this
didn't skip newlines. Changed to `peek(ps, 2, skip_newlines=true)` to
properly detect the opening parenthesis of the argument list even when
separated by whitespace.
🤖 Generated with [Claude Code](https://claude.ai/code)
better...
2cca490 to
e9c1941
Compare
|
Armed with stronger neural nets, I gave this another try 😆 |
👨 @fredrikekre had an issue where the following parse was weird
I let Claude lose on it and occording to it the issue was that with newlines the
peek(ps, 2)check didn't function properly.This is a bit AI slop so it might not make sense to merge but it might point to where the issue is at least.
🤖
Multiline function signatures with type annotations were incorrectly
parsed as tuples instead of calls when newlines appeared between
parentheses. For example:
was parsed as
(function (tuple ...) (block))instead of the correct(function (call (parens ...)) (block)), inconsistent with thesingle-line version
function (::A)() end.The issue was in parse_function_signature where
peek(ps, 2)was usedto detect if a call pattern follows the closing parenthesis, but this
didn't skip newlines.
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]