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

Allow for proposed new parsing of array[end] in Accessors #196

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

mlechu
Copy link
Contributor

@mlechu mlechu commented Feb 18, 2025

To fix JuliaLang/julia#57269, a small-but-breaking AST change is needed (PR JuliaLang/julia#57368). The set macro in Setfield and Accessors breaks, and enough packages depend on this macro that it needs to be updated before deciding whether to merge the julialang PR.

This PR only recognizes the new way of parsing array[end], and should not change any behaviour when used with the current parser.

@aplavin
Copy link
Member

aplavin commented Feb 19, 2025

It's not too common to include explicit support for something that's not even in nightly yet and may easily not go into any Julia version in the end... I'm not fundamentally against this, just want to clarify:
I assume that if the linked Julia PR isn't accepted in the end, we should remove this code? Also, not forget to modify it if that PR evolves on the way.

src/sugar.jl Outdated
# New syntax for begin/end in indexing expression, see https://github.com/JuliaLang/julia/pull/57368
index = MacroTools.replace(
index, Expr(:end),
dim === nothing ? :($(Base.lastindex)($collection)) : :($(Base.lastindex)($collection, $dim))
Copy link
Member

Choose a reason for hiding this comment

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

This is the same replacement as for :end above, right? Then – for clarity – it should probably be defined once and assigned to a variable, with two short replace() calls following it.

@mlechu
Copy link
Contributor Author

mlechu commented Feb 19, 2025

Thanks!

I assume that if the linked Julia PR isn't accepted in the end, we should remove this code? Also, not forget to modify it if that PR evolves on the way.

Yes, I'll update/undo these changes if they become obsolete.

@jw3126
Copy link
Member

jw3126 commented Feb 19, 2025

Just for reference it is very likely that this AST change will happen in julia and @Keno recommends merging this PR: JuliaLang/julia#57368 (comment)

@jw3126 jw3126 merged commit 133fd94 into JuliaObjects:master Feb 19, 2025
11 checks passed
@mlechu
Copy link
Contributor Author

mlechu commented Feb 24, 2025

@jw3126 Thanks! Whenever you have time, could you add this change to a release for both packages so that I can check how the parsing change affects the packages depending on setfield and accessors?

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

Successfully merging this pull request may close these issues.

arr[var"end"] is the same as arr[end]
3 participants