RustPython / Parser

MIT License
67 stars 24 forks source link

Move `range` from `Attributed` to `Node`s #22

Closed MichaReiser closed 1 year ago

MichaReiser commented 1 year ago

This PR attaches a TextRange to every node (except unions) and moves the TextRange from Attributed to the Nodes itself.

The reason for moving the range to the Node is that Ruff always needs the location information, but there's no easy way to refine an Attributed<Stmt> to an Attributed<MatchStmt> without creating a new Attributed<&MatchStmt> and passing that instead.

Moving the information to the node also ensures all nodes have location information. We need this for Ruff's autoformatter because we need to associate comments with their nodes and we can only do this if we have location information for all nodes.

Downsides

One downside is that an Attributed<ExprKind> now always stores the byte-offset, even in the case of RustPython that uses the custom to store the row and column information (we probably want to migrate away from this because looking up all row/columns is rather expensive).

Another is that this change can increase the size of nodes. The solution here is to Box all nodes in the union instead (which should also help with overall memory consumption)

youknowone commented 1 year ago

To use rustpython_ast::located as a top root ast module, it must contains all of aliases to generics. Located is correctly generated but types without Located is missing.

MichaReiser commented 1 year ago

To use rustpython_ast::located as a top root ast module, it must contains all of aliases to generics.

Located is correctly generated but types without Located is missing.

Oh yeah good point. They also don't need the feature gating

MichaReiser commented 1 year ago

To use rustpython_ast::located as a top root ast module, it must contains all of aliases to generics. Located is correctly generated but types without Located is missing.

I think this should now be resolved. I also excluded gated the problematic tests to only run for one feature. Let me know if you want me to change anything else.

youknowone commented 1 year ago

RustPython port: https://github.com/RustPython/RustPython/pull/4969 using #23

MichaReiser commented 1 year ago

@MichaReiser ls this ready to merge? I need to attach a few more edits. Usually adding commits to opened PR is possible, but this one is not. Maybe because the PR originated branch is an organization one (astral-sh)

Yes, this is ready to merge. Hmm. Not sure. I also don't see the option to allow edits from maintainers.

For greater collaboration, you can allow commits on branches you've created from forks owned by your personal account.

Yup, the fact that this PR is from an organisational account must be the issue.

MichaReiser commented 1 year ago

It could be the easiest to merge the PR and then create a follow up PR right after that includes your changes. You can merge that quickly with your maintainer permissions 😊

youknowone commented 1 year ago

Thank you!

MichaReiser commented 1 year ago

Thank you!

Thank you for all your support and feedback, to make this PR better than its initial version and keeping Ruff and RustPython in sync.

youknowone commented 1 year ago

I am so glad that we can co-work for shared future 😄