juniper icon indicating copy to clipboard operation
juniper copied to clipboard

Remove `DynType` and resculpt `ast::Type`

Open tyranron opened this issue 10 months ago • 3 comments

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.md updated

tyranron avatar May 20 '25 19:05 tyranron

@audunhalland it would be nice if you provide some input of this, if you have time to spare. Thanks!

tyranron avatar May 22 '25 15:05 tyranron

@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 avatar May 25 '25 13:05 audunhalland

@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.

tyranron avatar May 26 '25 12:05 tyranron