Add end locations to all nodes #4192

charliermarsh · 2022-10-02T01:41:18Z

Summary

I'm finding myself needing end locations for AST nodes (similar to in the Python AST). Is there interest in enabling this? If so, happy to clean up and test further.

youknowone · 2022-10-15T05:32:03Z

Please feel free to let me know if you need any help

charliermarsh · 2022-10-15T14:49:27Z

@youknowone - Just rebased. I've been using this code successfully in Ruff for a while now, would love to get it merged if you're open to it! Let me know what needs fixing / addressing :)

youknowone · 2022-10-15T15:18:18Z

@charliermarsh I didn't check the details yet, but could you check the failing tests?

charliermarsh · 2022-10-15T15:19:09Z

@youknowone - Absolutely, will do, thanks.

charliermarsh · 2022-10-15T19:01:09Z

@youknowone - Tests passing!

youknowone · 2022-10-16T14:54:43Z

Because we are trying to leave our own footprints to CPython codes as less as possible, I reverted the last commit but tried to change end_location to be optional.

charliermarsh · 2022-10-16T14:56:31Z

I can change it to Optional (but populated everywhere, as it is now) if you'd prefer?

charliermarsh · 2022-10-16T14:58:15Z

Or are you suggesting something else? Using Optional is less convenient for me as a "client" of the parser but I know the spec marks it as optional :)

charliermarsh · 2022-10-16T15:02:54Z

Specifically, I could change the struct to:

#[derive(Debug, PartialEq)]
pub struct Located<T, U = ()> {
    pub location: Location,
    pub end_location: Option<Location>,
    pub custom: U,
    pub node: T,
}

...then propagate that throughout the codebase.

youknowone · 2022-10-16T15:03:03Z

I am ok with either way unless it works well with CPython test codes.
If you mean end_location: Option<Location>, that's also a good way because leaving fake value as I did is a sort of anti-pattern.

charliermarsh · 2022-10-16T15:04:45Z

I can try to make that change today.

charliermarsh · 2022-10-16T15:06:03Z

In practice, does CPython ever omit end_lineno and end_col_offset when you go through ast.parse? Or are they just optional when creating AST nodes "manually"?

youknowone · 2022-10-16T15:09:05Z

@charliermarsh because I made incomplete commit last push, you can work on the new one as a base.

I am sorry, I don't know well about CPython ast that much.
cc @fanninpm @coolreader18

charliermarsh · 2022-10-16T15:46:40Z

@youknowone - Hoping I didn't override your work. This is ready for review.

charliermarsh · 2022-10-16T15:55:13Z

Lib/test/test_ast.py

@@ -2307,8 +2307,6 @@ class C:
        cdef = ast.parse(s).body[0]
        self.assertEqual(ast.get_source_segment(s, cdef.body[0], padded=True), s_method)

-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure


@youknowone - All other test changes were now able to be reverted.

charliermarsh · 2022-10-16T15:56:04Z

compiler/ast/src/ast_gen.rs

    pub custom: U,
    pub node: T,
 }

 impl<T> Located<T> {
-    pub fn new(location: Location, node: T) -> Self {
+    pub fn new(location: Location, end_location: Location, node: T) -> Self {


I kept Location as a required field in ::new for now. (We can omit it in special cases by going through the Struct directly, as in ast.rs.)

I could see arguments either way here. On the one hand, it's odd that the field you pass in differs from what's stored on the struct. On the other hand, this encourages good behavior and reduces the number of code changes required.

I agree this approach and the idea encouraging good behavior.

youknowone · 2022-10-17T03:14:27Z

compiler/parser/src/context.rs

@@ -0,0 +1,173 @@
+use rustpython_ast::{Expr, ExprContext, ExprKind};
+
+pub fn set_context(expr: Expr, ctx: ExprContext) -> Expr {


is this necessary part of end_location?

Oh no, sorry, I need to revert this. Not sure how it got included.

youknowone · 2022-10-17T04:18:46Z

Awesome! Thank you!

charliermarsh force-pushed the charlie/end-location branch 2 times, most recently from 430985f to 3a18132 Compare October 2, 2022 02:03

charliermarsh force-pushed the charlie/end-location branch from 3924854 to 6ab8d68 Compare October 9, 2022 02:03

charliermarsh mentioned this pull request Oct 15, 2022

Enable installation via cargo astral-sh/ruff#43

Open

charliermarsh force-pushed the charlie/end-location branch from 6ab8d68 to 0f070bc Compare October 15, 2022 14:48

charliermarsh force-pushed the charlie/end-location branch from f4359ee to 790e557 Compare October 15, 2022 18:21

youknowone force-pushed the charlie/end-location branch from 790e557 to b132ede Compare October 16, 2022 14:48

youknowone self-requested a review October 16, 2022 15:01

youknowone force-pushed the charlie/end-location branch from b132ede to b2148b3 Compare October 16, 2022 15:07

charliermarsh force-pushed the charlie/end-location branch from b2148b3 to ff8e947 Compare October 16, 2022 15:46

charliermarsh commented Oct 16, 2022

View reviewed changes

charliermarsh force-pushed the charlie/end-location branch from 00238ba to f2a7ae6 Compare October 16, 2022 16:07

youknowone approved these changes Oct 17, 2022

View reviewed changes

charliermarsh added 2 commits October 16, 2022 23:29

Add end locations to all nodes

ea6cbf3

Rename to location and end_location

e5e4a5f

charliermarsh added 4 commits October 16, 2022 23:29

Fix tests

af7a8a8

Make end_location Option<Location> on Located

8f3fd27

Make optional in ast.rs

32dd3ca

Rebase; remove context.rs

4082b17

charliermarsh force-pushed the charlie/end-location branch from f2a7ae6 to 4082b17 Compare October 17, 2022 03:31

youknowone merged commit 610d408 into RustPython:main Oct 17, 2022

JonathanPlasse mentioned this pull request Mar 14, 2023

Fix PYI011 and add auto-fix astral-sh/ruff#3492

Merged

MichaReiser mentioned this pull request Apr 26, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add end locations to all nodes #4192

Add end locations to all nodes #4192

charliermarsh commented Oct 2, 2022 •

edited

youknowone commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

youknowone commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022 •

edited

charliermarsh Oct 16, 2022

charliermarsh Oct 16, 2022

charliermarsh Oct 16, 2022

youknowone Oct 17, 2022

youknowone Oct 17, 2022

charliermarsh Oct 17, 2022

charliermarsh Oct 17, 2022

youknowone commented Oct 17, 2022

		@@ -0,0 +1,173 @@
		use rustpython_ast::{Expr, ExprContext, ExprKind};

		pub fn set_context(expr: Expr, ctx: ExprContext) -> Expr {

Add end locations to all nodes #4192

Add end locations to all nodes #4192

Conversation

charliermarsh commented Oct 2, 2022 • edited

Summary

youknowone commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

youknowone commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

charliermarsh commented Oct 15, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

charliermarsh commented Oct 16, 2022

youknowone commented Oct 16, 2022

charliermarsh commented Oct 16, 2022 • edited

charliermarsh Oct 16, 2022

Choose a reason for hiding this comment

charliermarsh Oct 16, 2022

Choose a reason for hiding this comment

charliermarsh Oct 16, 2022

Choose a reason for hiding this comment

youknowone Oct 17, 2022

Choose a reason for hiding this comment

youknowone Oct 17, 2022

Choose a reason for hiding this comment

charliermarsh Oct 17, 2022

Choose a reason for hiding this comment

charliermarsh Oct 17, 2022

Choose a reason for hiding this comment

youknowone commented Oct 17, 2022

charliermarsh commented Oct 2, 2022 •

edited

charliermarsh commented Oct 16, 2022 •

edited