-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Get rid of mutable syntax node usage #15710
Comments
cc @matklad feels like an issue you'd have an opinion about or two :) |
Yeah, I wouldn’t say that mutable syntax API is a success :-) That being said, I think it barks at the right tree: the fundamental capability there is “tracking nodes across edits”, and I think it would be important to have that in one way or another. But I am not too confident here: maybe we don’t need cursor API at all? One curious solution here is Roslyn’s SyntaxAnnotations: my understanding is that this mechanism powers all “mutable” editing APIs, which allow you to incrementally mutate the tree. I looked at how that’s implemented under the hood, and I think it actually is pretty horrible —- there’s a global static weak map of annotated nodes (no link to source, so I might be misremembering) I think another related issue is how our semantics API can panic if you feed it with a “wrong” Syntax Node. I was thinking that maybe the solution to both problems is biting the bullet and ensuring that every single bit of syntax has a globally unique id? Definitely need to change something here, though I still am not 100% positive about the ideal end state :P |
Another thing I just realized, this API makes it near impossible to map up edits back into macro input token trees |
I feel like using both approaches of using Since a A caveat of letting
One interesting situation to handle is when trying to change both a parent node and its children's nodes, e.g. SomeTrait<Item = [u8; SOME_CONSTANT]>; into m1::SomeTrait<Item = [u8; m2::SOME_CONSTANT]>; (based on #17321) Roslyn's approach is to have a Other idle thoughts:
|
That seems fine to me, make functions could be exposed as methods on a
This sounds a lot worse usability wise than option 1 to me.
I don't think anything needs to be said here 😬
I feel like we don't? At least I can't tell an immediate use case (we have one place where we kind of do this right now which is in adjustment inlay hints, but that is really just a hack we need to replace generally)
Feels like something that should be naturally supported.
I'd expect us to only touch real file trees here. That is if we are inside a macro call we ought to first upmap that out and then attempt any edits. Anything else gets hairy for no benefit (in my eyes at least). |
For assists like |
Ah right, yes that should be in theory possible |
Somewhat meant as a means of discussion (though ultimately just expressing my opinion):
After having used and reviewed a bunch of PRs using the mutable syntax node API I am personally not too happy about it. It can be awkward to use at times and has some pitfalls that one has to be aware of. A major problem with that API is that it introduces iterator invalidation (you can iterate while mutating the tree), giving way to easily end up panicking if one does not delay the edits after iteration via collecting into a vec or similar. Likewise it makes it harder to replace our version of rowan to one that uses proper slots which is something I am in favor of, as the slot API is unlikely to have the same feature (unless we implement it back into rome's fork of rowan which would be the current slot based rowan solution).
The current benefit of it is that we have the
ted
api that allows automagically fixing up whitespace on edits, but that's really not necessary in the end game when we have our own syntax formatter which would allow us to format edited nodes on the fly. It is also a rather powerful api in that you can patch up syntax trees on the fly.The main question then arises, how do we do edits if not via mutable syntax nodes? The current approach was using the handwritten
make
module to stitch nodes together (which is still somewhat used). But that approach is really annoying to use, inefficient and looks horrid as well. One nice looking approach would be having a quasi quote api as described in #2227, that could also do some static checks ideally (like checking well typedness of the tree at comp time maybe). With quoting one could just reconstruct the patched tree / construct a the biggest tree encompassing the changes of interested and then ask the assist building api to replace the node pointed to by a specific node ptr with the constructed tree. And alternative to mutation is #9649 though I haven't looked too much into it.In general, assists are all over the place currently, some do text based editing, some do make api + stringified text edits and finally we have some that use the mutable node api. Obviously not ideal if we mix and match however we like to.
(Issue spawned of me really not enjoying reviewing assist PRs nowadays anymore, as they tend to be messy in general, though also because I really want us to switch to the trailing trivia model)
The text was updated successfully, but these errors were encountered: