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

Add Located::start, Located::end and impl Deref #4929

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Apr 26, 2023

This is a follow-up to #4192 and is motivated by the discussions in astral-sh#4

The PR adds thestart and end methods to Located that return a node's start or end location. The end method falls back to returning location if end_location is None.

The PR further implements Deref for Located so that the node's fields and methods can be accessed directly: You can write located.field instead of located.node.field

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you!

@youknowone youknowone merged commit 23fee27 into RustPython:main Apr 26, 2023
@MichaReiser
Copy link
Contributor Author

Thank you!

Thank you for chiming in on the PR, suggesting to upstream these changes and merging them so quickly. It's great fun contributing to RustPython thanks to you :)

@DimitrisJim
Copy link
Member

I'd slightly push back on Deref here, docs boldly state it should only be implemented on smart pointer types but we might already be doing it in other places so 😄

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Apr 26, 2023

I'd slightly push back on Deref here, docs boldly state it should only be implemented on smart pointer types but we might already be doing it in other places so 😄

We can undo the change if you feel strongly about.

The term smart-pointer isn't well defined and there's ongoing discussion to re-phrase or even remove the section rust-lang/rust#91004.

The way I think about it is that Located is a thin-wrapper around Node that adds source locations to it. You could probably even argue that it's, to some extend, similar to an Rc because it stores the inner data and some metadata about it (reference count / source locations)

@DimitrisJim
Copy link
Member

I definitely do not feel that strongly!

@youknowone
Copy link
Member

Maybe it can be anti-pattern, but we already depends on a lot for Deref for thin wrappers. And it is abused a lot in many projects 😂 . Replacing them to traits will be possible but will be painful. I hope rust have a similar features allows method call for wrappers but disallow * operator to prevent confusion between reference and wrapper.

@MichaReiser Thank you so much for paying attention to keep unified code base. You are most welcome!

@jmaargh
Copy link

jmaargh commented Oct 21, 2023

FYI: Some better guidance on when and when not to Deref is currently in review: rust-lang/rust#110340

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.

4 participants