Skip to content

Parse arr[end] differently from arr[var"end"] #537

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mlechu
Copy link
Member

@mlechu mlechu commented Feb 11, 2025

Fixes a small issue present in both parsers (see discussion at JuliaLang/julia#57269) due to treating begin/end in indexing expressions as normal identifiers.

This would be a breaking change that goes with PR JuliaLang/julia#57368, which makes the same change to the lisp parser, and changes lowering to expect Expr(:end) instead of a symbol.

src/parser.jl Outdated
@@ -3627,6 +3627,14 @@ function parse_atom(ps::ParseState, check_identifiers=true, has_unary_prefix=fal
elseif check_identifiers && is_closing_token(ps, leading_kind)
# :(end) ==> (quote-: (error end))
bump(ps, error="invalid identifier")
elseif ps.end_symbol && leading_kind in KSet"begin end"
Copy link
Member

Choose a reason for hiding this comment

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

Needs a version check on the parse syntax, since JuliaSyntax itself is not tied to a Julia version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Assuming 1.12?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do 1.13 - we can change it later if we decide to backport.

@c42f
Copy link
Member

c42f commented Feb 12, 2025

I think the changes to SyntaxNode are good here and we should do them even if it's too breaking to change Expr in Base. JuliaSyntax already contains many such AST differences - some of which are much more significant than changing begin/end. (As an example, consider K"dotcall".)

So if you like we could merge this right away if you make sure the Expr form remains unchanged. We just need:

  • Tests added to parsestmt_with_kind_tests in test/parser.jl to assert the correct kind for the begin/end leaf nodes in the tree
  • Currently-failing tests fixed
  • Reuse K"begin" / K"end" rather than make new kinds. Keeping close to the source strings makes it super easy to remember the right kind.

Later you can make a separate PR if PkgEval deems the Expr changes not to be breaking.

@c42f c42f added the AST label Feb 12, 2025
@mlechu mlechu marked this pull request as draft February 12, 2025 17:22
@mlechu
Copy link
Member Author

mlechu commented Feb 12, 2025

Thanks for the help!

  • Tests added to parsestmt_with_kind_tests in test/parser.jl to assert the correct kind for the begin/end leaf nodes in the tree

Am I correct in thinking I would need to update parsestmt_with_kind_tests to take the version into account? I think not.

Waiting on pkgeval before deciding whether to keep the Expr change.

Copy link

codecov bot commented Apr 28, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.08%. Comparing base (30109d4) to head (17be4e3).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/expr.jl 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   96.18%   96.08%   -0.10%     
==========================================
  Files          13       13              
  Lines        4032     4113      +81     
==========================================
+ Hits         3878     3952      +74     
- Misses        154      161       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants