rust-analyzer / rowan

Apache License 2.0
677 stars 57 forks source link

Add AstPtr and AstNode #122

Closed azdavis closed 2 years ago

azdavis commented 2 years ago

This basically lifts from

I've found myself wanting AstPtr and AstNode in my own side projects and ended up basically copy-pasting them from rust-analyzer internals. Since these seem generic enough (not rust specific) maybe they could be here in rowan.

If this seems ok to merge I can follow up to migrate rust-analyzer to this.

lnicola commented 2 years ago

Seems reasonable, but please bump the version number too. Is the 2021 change required? It's arguable a breaking change so you should probably use 0.15 in that case.

r? @matklad

matklad commented 2 years ago

Can we actually use these in rust-analyzer? As in, would it run in some coherence problems, or is it usable as is?

azdavis commented 2 years ago

Let me first see if I can migrate rust analyzer to this locally before we merge this.

azdavis commented 2 years ago

The 2021 version isn't required but I thought it'd be good to do anyway.

azdavis commented 2 years ago

I tried migrating rust-analyzer to this version of rowan, see above.

azdavis commented 2 years ago

The only downside I see to this is that it necessitates using AstNode<Language = RustLanguage> instead of the previous AstNode in rust-analyzer (and probably any similar crate that would use AstNode).

When migrating, I tried something like

pub trait AstNode: rowan::ast::AstNode<Language = RustLanguage> {}

impl<N: rowan::ast::AstNode<Language = RustLanguage>> AstNode for N {}

to avoid having to repeat the <Language = RustLanguage> everywhere. But then the methods on rowan::ast::AstNode are only in scope if you explicitly use it (not the rust-specific AstNode), which is a little confusing.

azdavis commented 2 years ago

Thoughts on this? As I see it it’s a mild ergonomics hit (as opposed to not having to specify the language) but in return downstream Rowan users don’t have to invent their own versions of these (what i believe to be) fairly common things.

Veykril commented 2 years ago

The only downside I see to this is that it necessitates using AstNode

Makes you wish we had trait aliases already

azdavis commented 2 years ago

Is this ok to merge?

lnicola commented 2 years ago

¯\(ツ)\

r? @matklad again

matklad commented 2 years ago

Ok, so I think it seems useful for building smaller things on rowan, as you get more of the scaffolding out of the box.

For rust-analyzer though, I think we shouldn't use AstNode from rowan, and instead should define our own AstNode.

    where
        Def::Ast: AstNode<Language = RustLanguage>,

That Language = RustLanguage is accidental complexity which doesn't bring any benefit to rust-analyzer. As ast is the core vocabulary type, it should be as simple as possible, even if that means we need to duplicate some of rowan's code.

Re-using SyntaxNodePtr should be fine I think, as that can be just an alias.

bors r+

bors[bot] commented 2 years ago

Build succeeded: