Remove `DynType` and resculpt `ast::Type`
Follows #1247
Synopsis
In #1247 the ast::Type was made generic over the string type used for name.
pub enum Type<N = ArcStr> {
Named(N),
List(Box<Type<N>>, Option<usize>),
NonNullNamed(N),
NonNullList(Box<Type<N>>, Option<usize>),
}
In the codebase, there are numerous places where it's required to convert both ast::Type<ArcStr> and ast::Type<&str> to some singular representation stored in ValidatorContext or similar places.
However, the current ast::Type enum representation using Boxes introduces a problem that it's not possible to cheaply receive a borrowed version of ast::Type<ArcStr> as ast::Type<&str> without re-allocating all those Boxes. Furthermore, using Boxes involves redundant re-allocations on Cloneing even when &str is used.
Thus, @audunhalland came up with a temporary DynType solution to solve this problem:
pub enum DynType<'a> {
Named(&'a str),
List(&'a dyn AsDynType, Option<usize>),
NonNullNamed(&'a str),
NonNullList(&'a dyn AsDynType, Option<usize>),
}
pub trait AsDynType: fmt::Debug {
fn as_dyn_type(&self) -> DynType<'_>;
}
impl AsDynType for ast::Type<ArcStr> { }
impl AsDynType for ast::Type<&str> {}
The problem with this solution, that it duplicates ast::Type inner logic and pollutes some potentially public APIs.
Solution
As @audunhalland kindly suggested in TODO comment:
// TODO: Ideally this type should not exist, but the reason it currently does is that `Type` has a
// recursive design to allow arbitrary number of list wrappings.
// The list layout could instead be modelled as a modifier so that type becomes a tuple of
// (name, modifier).
// If `Type` is modelled like this it becomes easier to project it as a borrowed version of
// itself, i.e. [Type<ArcStr>] vs [Type<&str>].
This PR resculpts ast::Type in this manner:
pub enum TypeModifier {
NonNull,
List(Option<usize>),
}
pub(crate) type BorrowedType<'a> = Type<&'a str, &'a [TypeModifier]>;
#[derive(Clone, Debug)]
pub struct Type<N = ArcStr, M = Box<[TypeModifier]>> {
name: N,
modifiers: M,
}
Checklist
- [x] Docs are updated
- [x] Tests are updated
- [x]
CHANGELOG.mdupdated
@audunhalland it would be nice if you provide some input of this, if you have time to spare. Thanks!
@tyranron I had a look, it's something like this I imagined.
A variation of the smallvec trick is a type that instead uses statics for commonly used modifier patterns:
https://github.com/audunhalland/juniper/commit/afcf8a706a5c0db9a00e9ffe0c203eb0476d2cf4
The advantage of this is that the TypeModifiers is 24 bytes while SmallVec<[TypeModifier; 2]> is 48 bytes. The TypeModifiers trick can be easily extended to cover more cases.
@audunhalland the reason I've used SmallVec is not that of small-sized optimization in the first place, but rather because it's needed to be mutable to allow wrapping. Initially, I was using Box<[TypeModifier]> and realization that it would require re-allocation on every wrapping lead me to Vec (and thus SmallVec for cutting of allocations on most common types). Though, in your example, it's amortized with some a little more complicated logic for wrapping.
Thanks for the tip! It's very helpful. I'll think on it more and will try to do something of it.