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.

Sign up for GitHub

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

[python_ast] Make the iter_mut functions public #13542

Merged
merged 1 commit into from
Oct 19, 2024
Merged

[python_ast] Make the iter_mut functions public #13542

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/ruff_python_ast/src/nodes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,7 +1296,7 @@ impl FStringValue {

/// Returns an iterator over all the [`FStringPart`]s contained in this value
/// that allows modification.
pub(crate) fn iter_mut(&mut self) -> IterMut<FStringPart> {
pub fn iter_mut(&mut self) -> IterMut<FStringPart> {
self.as_mut_slice().iter_mut()
}

Expand Down Expand Up @@ -1749,7 +1749,7 @@ impl StringLiteralValue {

/// Returns an iterator over all the [`StringLiteral`] parts contained in this value
/// that allows modification.
pub(crate) fn iter_mut(&mut self) -> IterMut<StringLiteral> {
pub fn iter_mut(&mut self) -> IterMut<StringLiteral> {
Copy link
Member

Choose a reason for hiding this comment

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

The fact that this method isn't public seems intentional to me because mutating can result in an inconsistent to_str result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my particular usage, I actually only want to mutate the ranges. Thoughts on how to do that in a safe way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, if I had Parser::new_starts_at exposed that would suit my needs even better. I have a string, but I know it starts halfway through a file, and what to parse that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I guess this only applies to the StringLiteral one - since the others don't have a cached to_str, they could be made public?

Copy link
Member

Choose a reason for hiding this comment

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

The parser exposes a parse_expression_range method. I rather not expose the parser struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(in all cases we'd be delighted to supply them as a PR, once we figure out the design)

Copy link
Member

Choose a reason for hiding this comment

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

For (1), we do use parse_expression_range, you might be interested in going through https://github.com/astral-sh/ruff/blob/5118166d21784e7c78e38ea42919ba50bb2a5142/crates/ruff_python_parser/src/typing.rs which parses string type annotations. Ruff still has a bug for complex type annotations (like implicitly concatenated strings) #10586.

For (2), we have the Transformer trait which will visit the nodes to allow mutation.

Is this useful?

Copy link
Member

Choose a reason for hiding this comment

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

We are also looking into how to solve 1 for our type checker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhruvmanila - yep, we have the identical problem, but haven't added the "simple literal" special case. We probably should. But then still end up with the same issue. If you had the parse_expression_at, then the special case becomes non-interesting and you can simplify the code.

The Transformer trait is super interesting. It's a shame that the self isn't mut - that makes accumulating in the visitor much harder. It's a shame there is no lifetime constraint, e.g. if you are walking an &'a Stmt, you have the guarantee that all the statements you bump into have the same lifetime 'a (I note SourceOrderVisitor has this). Can't you use this to screw with the literals in a StringLiteral, thus breaking the to_str cache? With those adjustments, I think we'd implement our visitors on top of your Transformer trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, if you added parse_expression_in_string_literal(x: &StringLiteral) -> Expr then that would be exactly the primitive we both need, we could delete the slightly dubious parse_expression_range (is there another good reason to have this?) and you could do very clever things to deal with things like escapes only once.

self.as_mut_slice().iter_mut()
}

Expand Down Expand Up @@ -2138,7 +2138,7 @@ impl BytesLiteralValue {

/// Returns an iterator over all the [`BytesLiteral`] parts contained in this value
/// that allows modification.
pub(crate) fn iter_mut(&mut self) -> IterMut<BytesLiteral> {
pub fn iter_mut(&mut self) -> IterMut<BytesLiteral> {
self.as_mut_slice().iter_mut()
}

Expand Down
Loading