graphql-rust / juniper

GraphQL server library for Rust
Other
5.72k stars 425 forks source link

Rework core traits #1072

Open tyranron opened 2 years ago

tyranron commented 2 years ago

Requires #1028, #1047, #1052
Required for #975
Related to #1091
Eliminates #1097

This PR represents an attempt to make a fundamental rework of Juniper resolving machinery (GraphQLType, GraphQLValue, etc).

Motivation

At the moment, the core traits like GraphQLType and GraphQLValue impose the following problems to codebase:

Solution

This PR tries to remodel the core traits machinery in the following ways:

Example

Meet a shiny new Juniper:

struct Human(u32);

#[graphql::object]
impl Human {
    fn id(&self) -> u32 {
        self.0
    }

   // Yes, we now have some degree of borrowing in arguments.
   fn has_name(name: &str) -> bool { 
       name == "John"
   }

   fn avatar(&self, context: &FileStorage) -> Option<String> {
       Some(context.lookup_avatar(self.0)?.to_base64_str())
   }

   fn profile(&self, context: &dyn UserRepository) -> Profile {
       context.get_profile(self.0).expect("profile should always exist")
   }
}

struct Context {
    repo: Box<dyn UserRepository>,
    storage: FileStorage,
}

impl Borrow<dyn UserRepository> for Context {
    fn borrow(&self) -> &dyn UserRepository {
        &*self.repo
    }
}

impl Borrow<FileStorage> for Context {
    fn borrow(&self) -> &FileStorage {
        &self.storage
    }
}

let context = Context::init();
juniper::execute(query, None, &schema, &graphql_vars! {}, &context).await;
// Code won't compile if the type, passed as context, doesn't implement all
// the `Borrow`s required by schema fields.
// For a single context type it works naturally due to
// existing `impl<T> Borrow<T> for T` blanket impl.

Further investigation

The solution for the following additional capabilities is not figured out yet and requires further investigation:

The main problem with polymorphism in both questions is that we cannot express forall quantifier for types in Rust:

trait Foo<T> {}
impl<A, B, T> Foo<A> for Box<T> where T: Foo<B> {}

// error[E0207]: the type parameter `B` is not constrained by the impl trait, self type, or predicates
//  --> src/lib.rs:2:9
//   |
// 2 | impl<A, B, T> Foo<A> for Box<T> where T: Foo<B> {}
//   |         ^ unconstrained type parameter

Solved: See https://github.com/graphql-rust/juniper/pull/1072#issuecomment-1147604603

Progress

This is a huge work, as requires an accurate re-implementation of the whole Juniper resolving and type registration machinery.

tyranron commented 2 years ago

Regarding "Further investigation" section...

Polymorphism over ScalarValue.

Well, this is simple. Once we decouple the orphan rules omitting responsibility from ScalarValue type param, it worths nothing to act with the ScalarValue the same way as with Context/TypeInfo: keep it as generic everywhere and require Into<MyCustomScalarValue> in concrete impl blocks. This way the requirement will bubble-up naturally up to the place where root node is executeed, without messing up any intermediate types.

Separate type parameter for omitting orphan rules.

The main problem with polymorphism in both questions is that we cannot express forall quantifier for types in Rust:

trait Foo<T> {}
impl<A, B, T> Foo<A> for Box<T> where T: Foo<B> {}

// error[E0207]: the type parameter `B` is not constrained by the impl trait, self type, or predicates
//  --> src/lib.rs:2:9
//   |
// 2 | impl<A, B, T> Foo<A> for Box<T> where T: Foo<B> {}
//   |         ^ unconstrained type parameter

This one is trickier as hits something quite fundamental regarding the Rust type system:

  1. To omit orphan rules we need to specify our type in a type parameter. Specifying it as an associated type doesn't allow to omit orphan rules, as doesn't give a dimension for parametrization. Associated type - is a concrete type, not a parameter.
  2. To be able to write the impl above, we should have B as a associated type parameter, so it be a concrete type, not a "Rust, please, guess it out of nowhere" type parameter.

We cannot have 1 and 2 at the same time, as the requirements are clear contradiction.

To have them both, we need to introduce some transparent wrapper type holding our type parameter B:

struct Owned<X, By>(X, PhatomData<By>);

Now our B is constrained and we have the desired "polymorphism":

impl<A, B, T> Foo<A> for Owned<Box<T>, B> where T: Foo<B> {}

Hooray? Not yet! The problem has shifted to the place where we should or should not wrap the type. Unfortunately, we cannot do that seamlessly.

trait IntoOwned {
    type Owned;
}

struct Mine;
impl juniper::IntoOwned for u8 {  // oops! orphan rules again
    type Owned = juniper::Owned<Self, Mine>;
}

Without seamless conversion, we cannot use this implementation just like that:

#[derive(graphql::Object)]
struct Human {
    id: u8,
}

This shows very well our fundamental limitation of unconstrained type parameter, described above. Who should guess which implementation should be used here? Different crates may provide different impls for u8. Well, so we may conclude that the user code must specify the desired implementation to use.

Ok, then:

#[derive(graphql::Object)]
struct Human {
    id: Owned<u8, Mine>,
}

Hmm, not quite ergonomic. We don't want to mess with wrappers in the user code.

#[derive(graphql::Object)]
struct Human {
    #[graphql(owner = Mine)]
    id: u8,  // and we do wrapping under-the-hood in the macro expansion only
}

Naming still requires bikeshedding to be better, though.

Conclusions?

With this small ergonomic hit (in a form of additional attribute argument in places where foreign types and our types are connected) we solve the problem of "polymorphism" for a type parameter for omitting orphan rules. This way a custom type won't pollute all the schema types above it, just a field which refers it directly.

And now we're able to use ScalarValue only where it's really needed.

tyranron commented 2 years ago

@ilslv thoughts? ☝️

ilslv commented 2 years ago

@tyranron this sounds a lot like #[serde(with = "module")] attribute and looks like a big ergonomic improvement 👍

From what i can tell, there is no way we can exactly replicate #[serde(with = "module")] by providing mod with a couple of functions (maybe until we can use fn pointers inside const generics?), but I think we can require users also add a type alias for a local struct:

#[derive(graphql::Object)]
struct Human {
    #[graphql(with = "custom")]
    id: u8,  // and we do wrapping under-the-hood in the macro expansion only
}

mod custom {
    pub(super) enum Custom {}

    // functions ...
}
tyranron commented 2 years ago

@ilslv they look similar, but the logic seems different there. with is more like "use intermediate transformation", while owner (renamed to behavior in sources) is like "use the concrete implementation for this type".

(maybe until we can use fn pointers inside const generics?)

Nope, as const values are values, not types, so don't interfere with orphan rules.

tyranron commented 2 years ago

Nota bene

At the moment we have a problem:

error[E0637]: `&` without an explicit lifetime name cannot be used here
   --> juniper/src/schema/schema.rs:140:37
    |
140 |     fn description(&self) -> Option<&str> {
    |                                     ^ explicit lifetime name needed here

Meaning that Option<&str>: ::juniper::resolve::Resolve bound at impl level is problematic.

Seems to be resolvable by quantifying all the type lifetimes into HTRB: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1f8b36c78e61a771540b7a3324f9dd8b

LegNeato commented 2 years ago

Do GATs change any of the design thinking here?

tyranron commented 2 years ago

@LegNeato at the moment I don't see any need of them here, but it may appear eventually to solve some problems.

elementary-particle commented 8 months ago

Are we going to complete this rework eventually? This is a lot of work indeed, we should not leave them around.