search

rust-analyzer / rowan

Apache License 2.0
677 stars 57 forks source link

Is Ord strictly required for Lang Kind? #131

Open DirectXMan12 opened 2 years ago

DirectXMan12 commented 2 years ago

Hi!

I've been poking around at using rowan for a project, and I was wondering what the logic for Lang & Kind being Ord was. It's not a massive burden (like... #[derive(PartialOrd, Ord)] is pretty easy :-P) but it feels a bit strange for me to make my SyntaxKind Ord when it's shouldn't really be compared like that (like, what does Identifier > Operator mean). Tried cargo build --examples and cargo test w/o it and nothing seemed to fail to compile (also tried cargo build on rust-analyzer itself with the dep pointed to the local copy, and it seemed to build fine), so that got me curious as to what the original logic was.

matklad commented 2 years ago

The thinking here is that Kind is pretty much an integer, so we can : Everything. I think some of those bounds could be removed (we don't use Hash either IIRC), but that'll make using this less ergonomic at the call site, as, should you need this extra bound, you'd have to specify it manually.

The Ord there is just in case someone wants to put the thing into a BTreeMap, or wants to sort (TextRange, SyntaxKind) tuples, where ordering on the second field is irrelevant, but without it one is forced to reach out for sort_by_key and lambdas.

Though, I'd say Ord for kinds makes some intrinsic sense. The kind is a enumeration, and the enumeration has a natural order. For example, in rust-analyzer, kinds for leaf tokens go before kinds for interior nodes.

TL;DR: no real reason, but I think there are marginal ergonomics wins here, so it doesn't hurt.

DirectXMan12 commented 2 years ago

Ah, that makes some amount of sense.

Something bugs me semantically about the actually implying that the natural ordering is meaningful (in my project at least, I don't think it actually is). If y'all are amenable to loosening those requirements I'm happy to fire off a PR, but I kinda got the impression from that response of "I'd rather keep them", so I can also just live with an ordering defined on my SyntaxKind & just ignore it or something I suppose and close this. Not a huge deal to me either way :-)

matklad commented 2 years ago

My gut feeling is that I’d rather keep, yeah. But. Can you maybe try to do this change and check if rust-analyzer needs any changes? If that’s not the case and it just works, than let’s remove the bound.

ra has some setup for substituting rowan already:

https://github.com/rust-analyzer/rust-analyzer/blob/e691ae0ab287a77dcf42842d7c564362b45460eb/Cargo.toml#L25