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

internal: Add preliminary SyntaxEditor functionality #18032

Merged
merged 11 commits into from
Sep 10, 2024

Conversation

DropDemBits
Copy link
Contributor

Related to #15710

Implements a SyntaxEditor interface to abstract over the details of modifying syntax trees, to both simplify creating new code fixes and code actions, as well as start on the path of getting rid of mutable syntax nodes.

SyntaxEditor relies on SyntaxMappingBuilders to feed in the correct information to map AST nodes created by make constructors, as make constructors do not guarantee that node identity is preserved. This is to paper over the fact that make constructors simply re-parse text input instead of building AST nodes from the ground up and re-using the provided syntax nodes.

SyntaxAnnotations are used to find where syntax elements have ended up after edits are applied. This is primarily useful for the add_{placeholder,tabstop} set of methods on SourceChangeBuilder, as that currently relies on the nodes provided being in the final syntax tree.

Eventually, the goal should be to move this into the rowan crate when we move away from mutable syntax nodes, but for now it'll stay in the syntax crate.


Closes #14921 as SyntaxEditor ensures that all replace changes are disjoint
Closes #9649 by implementing SyntaxAnnotations

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2024
@DropDemBits DropDemBits changed the title internal: Add preliminary SyntaxEditor internal: Add preliminary SyntaxEditor functionality Sep 2, 2024
@Veykril
Copy link
Member

Veykril commented Sep 3, 2024

Eventually, the goal should be to move this into the rowan crate when we move away from mutable syntax nodes, but for now it'll stay in the syntax crate.

We should also consider forking rowan in-tree as we will likely start changing things a bunch once we consider dropping mutable syntax trees. (and at some point feed the changes back to main rowan and adopt it into the monorepo)

@Veykril
Copy link
Member

Veykril commented Sep 3, 2024

This is to paper over the fact that make constructors simply re-parse text input instead of building AST nodes from the ground up and re-using the provided syntax nodes.

Presumably, fixing this would simplify things here? Would be good to check if if thats a requirement we want for our next rowan and whether it has certain requirements on the implementation then

@DropDemBits
Copy link
Contributor Author

DropDemBits commented Sep 3, 2024

Presumably, fixing this would simplify things here?

Thinking about it more, I think we'll still need SyntaxMapping especially for when we move to immutable syntax trees, since reusing the syntax nodes to preserve node identity that way would imply that we mutate that syntax node.

useful for `SourceChangeBuilder` so it can still perform a tree diff without having to store the old root separately
@DropDemBits
Copy link
Contributor Author

I believe functionality-wise SyntaxEditor is complete, so marking as ready for review.

@DropDemBits DropDemBits marked this pull request as ready for review September 3, 2024 15:22
Copy link
Contributor

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

Honestly, looks good to me: I'd like to land this and start moving assists over to this.

@davidbarsky
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2024

📌 Commit 12c6266 has been approved by davidbarsky

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 10, 2024

⌛ Testing commit 12c6266 with merge f13c776...

@bors
Copy link
Contributor

bors commented Sep 10, 2024

☀️ Test successful - checks-actions
Approved by: davidbarsky
Pushing f13c776 to master...

@bors bors merged commit f13c776 into rust-lang:master Sep 10, 2024
11 checks passed
@DropDemBits DropDemBits deleted the sed-tree-edits branch September 11, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edits in assists are prone to panic Investigate syntax tree editing via node tracking
5 participants