Consolidate Ty variants into a new variant Ty::Apply #988
Conversation
There was a problem hiding this comment.
Looks nice from the impl point of view!
However, the interface which requires deep matching feels awkward. I also notice that, when we do matching, we almost always have an _ branch anyway...
Perhaps adding a slew of conversions to Ty, like Ty::as_adt, Ty::is_primitive, etc, would make this nicer? This might also be a good place to check invariant like "primitive types have empty subst"
crates/ra_hir/src/ty.rs
Outdated
| Ty::Adt { def_id, .. } => Ty::Adt { def_id, substs }, | ||
| Ty::FnDef { def, .. } => Ty::FnDef { def, substs }, | ||
| Ty::Apply(ApplicationTy { name, .. }) => { | ||
| Ty::Apply(ApplicationTy { name, parameters: substs }) |
There was a problem hiding this comment.
should we check that before/after subst lengths are the same?
There was a problem hiding this comment.
Yep, and I found a bug when I did it ;)
crates/ra_hir/src/ty.rs
Outdated
| #[derive(Clone, PartialEq, Eq, Debug)] | ||
| pub enum Ty { | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] | ||
| pub enum TypeName { |
There was a problem hiding this comment.
Isn't a TypeCtor a better name here? I imagine name would be a pretty overloaded term, so it's better to avoid it. I guess TypeName is what's used in chalk? I wonder if it was intentionally chosen over TypeCtor.
There was a problem hiding this comment.
Renamed it to TypeCtor now.
Yeah, that's what I was thinking... |
... and fix a small bug revealed by that.
|
bors r+ |
988: Consolidate Ty variants into a new variant Ty::Apply r=matklad a=flodiebold This gets us a lot closer to Chalk. It also introduces a lot of boilerplate, though, especially when matching :/ A lot of this can probably be refactored to be nicer, though. Co-authored-by: Florian Diebold <[email protected]>
Build succeeded |
This gets us a lot closer to Chalk. It also introduces a lot of boilerplate, though, especially when matching :/ A lot of this can probably be refactored to be nicer, though.