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:14Z

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:05Z

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:15: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 Oct 2, 2022

charliermarsh force-pushed the charlie/end-location branch from 3924854 to 6ab8d68 Compare Oct 9, 2022

charliermarsh mentioned this pull request Oct 15, 2022

Enable installation via cargo charliermarsh/ruff#43

Open

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

charliermarsh force-pushed the charlie/end-location branch from f4359ee to 790e557 Compare Oct 15, 2022

youknowone force-pushed the charlie/end-location branch from 790e557 to b132ede Compare Oct 16, 2022

youknowone self-requested a review Oct 16, 2022

youknowone force-pushed the charlie/end-location branch from b132ede to b2148b3 Compare Oct 16, 2022

charliermarsh force-pushed the charlie/end-location branch from b2148b3 to ff8e947 Compare Oct 16, 2022

charliermarsh commented Oct 16, 2022

View changes

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

youknowone approved these changes Oct 17, 2022

View changes

charliermarsh added 2 commits Oct 17, 2022

Add end locations to all nodes

ea6cbf3

Rename to location and end_location

e5e4a5f

charliermarsh added 4 commits Oct 17, 2022

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 Oct 17, 2022

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

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