Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔬 Make interfaces great again! #682

Merged
merged 89 commits into from
Oct 6, 2020
Merged

🔬 Make interfaces great again! #682

merged 89 commits into from
Oct 6, 2020

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Jun 15, 2020

Part of #295
Fixes #544, #605, #658, #748
Related to #421, #2

...or not so great? 🤔

Motivation

We want GraphQL interfaces which just feel right like usual ones in many PLs.

They should be able to work with an open set of types without requiring a user to specify all the concrete types an interface can downcasts into on the interface side.
We're unable to do that, at the moment 😕

But we still could have a nice proc macros as convenient as they can be 🙃

New design

Traits

#[graphql_interface(enum = CharacterValue)]  // <-- use enum for dispatching
#[graphql_interface(for = [Human, Droid, Jedi])]  // <-- mandatory =( and should enumerate all implementors
#[graphql_interface(name = "Character", desc = "Traited character")]
#[graphql_interface(context = MyContext)]  // <-- if not specified, then auto-inferred from methods, if possible, otherwise is `()`
#[graphql_interface(scalar = juniper::DefaultScalarValue)] // <-- if not specified, then generated code is parametrized by `__S: ScalarValue`
#[graphql_interface(on Jedi = custom_downcast_as_jedi)]  // <-- custom external downcaster
trait Character {
    fn id(&self) -> &str;

    // Optional custom downcaster.
    #[graphql_interface(downcast)]
    fn as_droid(&self) -> Option<&Droid> { None }
}

#[graphql_interface(dyn = DynCharacter2, for = Human)]  // use trait object for dispatching
trait Character2 {
    fn id(&self) -> &str;
}

#[derive(GraphQLObject)]
#[graphql(implements = [CharacterValue, DynCharacter2], Context = MyContext)]
struct Human {
    id: String,
    home_planet: String,
}
#[graphql_interface]
impl Character for Human {
    fn id(&self) -> &str {
        &self.id
    }
}
#[graphql_interface(dyn)]  // <-- this `dyn` is inevitable when dispatching via trait objects =(
impl Character2 for Human {
    fn id(&self) -> &str {
        &self.id
    }
}

#[derive(GraphQLObject)]
#[graphql(implements = CharacterValue, Context = MyContext)]
struct Droid {
    id: String,
    primary_function: String,
}
#[graphql_interface]
impl Character for Droid {
    fn id(&self) -> &str {
        &self.id
    }

    fn as_droid(&self) -> Option<&Droid> {
        Some(self)
    }
}


#[derive(GraphQLObject)]
#[graphql(implements = CharacterValue, Context = MyContext)]
struct Jedi {
    id: String,
    rank: String,
}
#[graphql_interface]
impl Character for Jedi {
    fn id(&self) -> &str {
        &self.id
    }
}
fn custom_downcast_as_jedi(ch: &CharacterValue, ctx: &MyContext) -> Option<&Jedi> {
     ctx.can_i_haz_the_jedi_please(ch)
}

The nearest analogue of GraphQL interfaces are, definitely, traits. However, they are a little different beasts than may seem at first glance. The main difference goes that in GraphQL interface type serves both as an abstraction and a value dispatching to concrete implementers, while in Rust, a trait is an abstraction only and you need a separate type to dispatch to a concrete implemeter, like enum or trait object, because trait is not even a type itself. This difference imposes a lot of unobvious corner cases when we try to express GraphQL interfaces in Rust. This PR tries to implement a reasonable middleground.

So, poking with GraphQL interfaces will require two parts:

  • declare the abstraction;
  • operate ingerface's values.

Declaring an abstraction is almost as simple as declaring trait:

#[graphql_interface(enum = CharacterValue)]  // <-- specify type ident to represent value of interface
#[graphql_interface(for = [Human, Droid, Jedi])]  // <-- specify implementers
trait Character {
    fn id(&self) -> &str;
}

And then, you need to operate in a code with a Rust implementing this GraphQL interface's value:

#[derive(GraphQLObject)]
#[graphql(implements = CharacterValue)]  // <-- we refer here to Rust type implementing GraphQL interface, not trait
struct Human {
    id: String,
}
#[graphql_interface]
impl Character for Human {  <-- and here we're implementing GraphQL interface for Human Graphql object as a regulat trait impl
    fn id(&self) -> &str {
        &self.id
    }
}

Trait objects

By default, an enum dispatch is used, because we should enumerate all the implementers anyway.

However, a dyn dispatch via trait is also possible to be used. As this requires a llitle bit more code transformation and aiding during macros expansion, we shoud specify it explicitly both on the interface and the implementers:

#[graphql_interface(dyn = DynCharacter)]  // <-- generates handy type alias for the trait object
#[graphql_interface(for = [Human, Droid, Jedi])]  // <-- specify implementers
trait Character {
    fn id(&self) -> &str;
}

#[derive(GraphQLObject)]
#[graphql(implements = dyn CharacterValue)]  // <-- `dyn` part is vital here, as an underlying trait is transformed
struct Human {
    id: String,
}
#[graphql_interface(dyn)]  // <-- `dyn` part is vital here, as an underlying trait is transformed
impl Character for Human {
    fn id(&self) -> &str {
        &self.id
    }
}

Checklist

  • derive(GraphQLObject)/graphql_object macros:
    • support implements attribute's argument to enumerate all traits this object implements.
  • Traits (#[graphql_interface(dyn)]):
    • trait transformation:
      • support multiple attributes and remove duplicated attributes on first expansion;
      • transform trait to extend AsGraphQLValue in a object-safe manner;
        • ensure trait is object safe;
      • enumerate implementors with #[graphql_interface(for = [Type1, Type2])] attribute;
        • ensure types are unique on type level;
        • ensure types are GraphQL objects;
      • support custom GraphQL name and description with #[graphql_interface(name = "Type")] and #[graphql_interface(description = "A Type desc.")] attributes;
      • support custom GraphQL field deprecation with #[graphql_interface(deprecated = "Text")] attributes;
      • support custom context type with #[graphql_interface(context = Type)] attribute;
        • support custom names for Context argument and allow to mark it with #[graphq_interface(context)] attribute;
      • support accepting executor in method arguments, optionally marked with #[graphq_interface(executor)] attribute;
      • support custom ScalarValue type #[graphql_interface(scalar = Type)] attribute;
        • allow explicit custom ScalarValue type patameter;
      • ignore trait methods with #[graphql_interface(ignore)] attribute;
      • support downcasting methods with #[graphql_interface(downcast)] attribute;
        • allow omitting Context in methods;
        • work correctly with parenthesized types;
        • allow full import paths as types;
        • validate signature for good error messages.
      • support external downcasters on trait itself with #[graphql_interface(on Type = downcast_fn)] attribute;
      • support trivial deriving via #[graphql_interface(derive(Type))] attribute. (will be implemented in a separate PR)
    • impl transformation:
      • support custom context type with #[graphql_interface(context = Type)] attribute; (not required)
      • support custom ScalarValue type #[graphql_interface(scalar = Type)] attribute;
        • allow explicit custom ScalarValue type patameter.
  • Enums (#[graphql_interface(enum)]):
    • generate enum type of all implementers;
      • generate From impls to this enum for each implementer;
      • impl original trait by this enum.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::documentation Related to project documentation semver::breaking Breaking change in terms of SemVer k::api Related to API (application interface) blocked labels Jun 15, 2020
@tyranron tyranron self-assigned this Jun 15, 2020
@tyranron
Copy link
Member Author

@LegNeato @jmpunkt this, at the moment, isn't even a WIP, but rather a searching for a suitable design and a proof-of-concept manual GraphQLType implementation.

I'm feeling being gone too far with it, that's why I just need to share with you my results and discuss the directions...

Interfaces appeared to be much more tough for implementation than previous work. Really, unions were quite easy. The main pain point about interfaces (applicable to traits only), is that we don't know all the downstream types we want to resolve into at the point when we're implementing GraphQLType. This paint point raises 2 main challenges:

  1. GraphQLType shoud be able to resolve_field for an unknown (erased) type. The only possibility to do this in Rust today is trait objects. So, GraphQLType, or some its part, should be object safe to have that ability.
  2. GraphQLType::meta implementation for interfaces should be able to register information in Registry about all its downstream types. We cannot use trait objects here, as GraphQLType::meta calls happen statically.

I've implemented a PoC implementation here, but it works only a half-way (you can use cargo test -p juniper_tests to see the result).

I has turned out, that challenge 1 is not too hard. I've decoupled GraphQLType into two traits: one is object safe, and the other contains all the static stuff. This allows to resolve GraphQLType values dynamically, and resolving an interface fields in tests "just works".

But, I've kinda stuck on the challenge 2.
First, I've tried to use the inventory for meta registration, as I do it for GraphQLIsObjectType::mark() calls: 1, 2.
But... the problem is that S: ScalarValue is a generic and Registry is abstracted over it. So, our function, registered in inventory, should be able to work with an arbitrary ScalarValue, but we cannot simply put the generic into an inventory: all types should be known to do it.
And starting from here I have 2 strategies to investigate:

  1. Try to invent some any+downcast+lazy_init magic to be able to call GraphQLType::meta for all downstream types registered somewhere separately in an inventory.
    Still, haven't found a way to it, though... 🤔 And poking with Any and downcasting doesn't give much hope with that.

  2. Try to remove S: ScalarValue type parameter from Registry.
    This, well, is a way huge refactoring. I've investigated where this S comes from, and it turned out that it's only required by a default_value field of meta::Argument. Once we remove it, the whole Registry can drop an S type parameter from its type. This does mean, that instead of abstracting like Registry<S>::get_type<T>() we will be able to abstract like Registry::get_type<T, S>(), and so remain generic over S without polluting the whole Registry and dependent types' signatures with it. This should allow to register in an inventory a single function and call it, like we do it with GraphQLIsObjectType::mark(). In theory, this also will reduce the codebase complexity and compliation times.
    Another question is: how to do that properly? I'll share thought about that a little bit later after more investigations.

@LegNeato
Copy link
Member

Thanks for doing this! I've been thinking about this a lot and landed on something very similar. I think we need some sort of side channel to pass information at compile time to not have a central registry. I believe a combination of naming convention, trait bounds, dynamic dispatch, and static functions could be put together in some way to build that side channel, but I have yet to come up with it.

@tyranron
Copy link
Member Author

@LegNeato @jmpunkt meh... after playing/poking with it much more, I can conclude that challenge 2 is impossible to be resolved at the current state of Rust and its ecosystem. Even if we've found a way to deal with that generic ScalarValue, the main problem is that we just cannot use inventory, because it doesn't work on WASM target at all, nor linkme does, nor ctor does, which is blocked by wasm-bindgen, which, in turn, blocked by rustc and "who knows when and whether it will be resolved at all". The price of dropping the whole WASM target is too high for having this feature. And without such things we cannot do any distributed implicit stuff registration.

So, the library user will have to enumerate all the interface's implementors on the interface declaration. I'll try to do it as much ergonomic as possible. I'll update the design in description shortly.
If somebody will come up with a resonable and working solution for distributed implicit plugins registration, or ctor/linkme will become usable on WASM and less hacky/unstable, we will be able to drop this requirement in future.

Regarding the object safety, I'll decouple those changes into a separate PR, to not pollute this one with a quite big side-refactoring.

@LegNeato
Copy link
Member

https://github.com/dtolnay/inventory looks pretty sweet. How about we do one method for wasm (without ctor stuff and needing a central listing) and one for non-wasm?

@LegNeato
Copy link
Member

Just thinking out loud, I wonder...is there some way we can store similar info to inventory? The limitation with it is it requires runtime support for ctors (which wasm doesn't have). But we don't need runtime, we need to aggregate all implementors at compile-time and output code that is than compiled and run (and can be simple enums). Hmmm

@LegNeato
Copy link
Member

I know it was suggested before and it is gross, but reading/writing data to CARGO_TARGET_DIR via the proc macro might be an acceptable stopgap. It's gross but it is at compile-time and would be an implementation detail we later swap out.

@LegNeato
Copy link
Member

LegNeato commented Jun 22, 2020

But then we are back to incremental compilation not working. Lame.

@LegNeato
Copy link
Member

More post-beer musings. What about using naming convention as a pre-determined sidechannel? Something like (massively simplified):

// #[juniper::graphql_interface]
trait Rideable {
    fn num_wheels() -> u8;
}


// Generated
trait crate::GraphQLInterfaceRideable: Rideable {
    fn as_graphql_rideable(&self) -> &Self {
        &self
    }
}

struct Bicycle;

// #[juniper::graphql_interface]
impl Rideable for Bicycle {
    fn num_wheels() -> u8 {
        2
    }
}

// Generated
impl crate::GraphQLInterfaceRideable for Bicycle {
    fn as_graphql_rideable(&self) -> &Self {
        &self
    }
}

// Juniper code assumes `crate::GraphQLInterfaceRideable` is implemented for any trait named "Rideable" outside of the type system.

So basically inverting control...the user's code would define a trait like GraphQLInterface[X] via proc macro codegen and juniper would just expect it is there using Any. While still runtime and not static checks, the contract would be between juniper and juniper_codegen's output...both sides of which we control and could test for.

Anyway, pretty sure we run in some of the same problems above...but if we control both sides I feel like we should be able to do this is some way.

@tyranron tyranron removed the blocked label Jun 30, 2020
@tyranron
Copy link
Member Author

tyranron commented Jun 30, 2020

@LegNeato returning to this...

How about we do one method for wasm (without ctor stuff and needing a central listing) and one for non-wasm?

Yeap, I guess we can hide such thing behind a feature flag. But, I'd rather do it as a separate PR on top of this one, which is going to contain only the base "works everywhere" implementation.

But we don't need runtime, we need to aggregate all implementors at compile-time and output code that is than compiled and run (and can be simple enums).

However, ctor/invetory/linkme works in runtime only ("life before main").

reading/writing data to CARGO_TARGET_DIR via the proc macro might be an acceptable stopgap. It's gross but it is at compile-time and would be an implementation detail we later swap out.

But then we are back to incremental compilation not working.

That requires further investigation. Maybe there will be some way with invalidation. Can't say for sure atm.

What about using naming convention as a pre-determined sidechannel?

Maybe I haven't understood your idea completely, but it seems to me that this only shifts the problem to another point (another interface declaration), rather than solves it.

The initial problem is that we should enumerate all the trait implementors statically, without specifying all of them manually in the definition. Those implementors potentially may live even in separate crates. I don't know the way to register such distributed implementors except the "distributed plugin registration" provided by inventory/ctor/linkme. Having conventionally named sub-trait is OK, but I don't see how it allows to enumerate its implementors.

@tyranron
Copy link
Member Author

Update!

Code polishing and code documentation is done!

Only left:

  • codegen failure tests;
  • documentation in book.

@LegNeato
Copy link
Member

LegNeato commented Oct 6, 2020

Let's land this and play around with it? I'm worried about having to rebase while we iterate and this is materially better than what we currently have.

Thanks again for all your work!

@LegNeato LegNeato marked this pull request as ready for review October 6, 2020 07:20
@LegNeato LegNeato merged commit cbf16c5 into master Oct 6, 2020
@tyranron
Copy link
Member Author

tyranron commented Oct 6, 2020

@LegNeato that hasn't been finished yet! 😱
The book is still weird and incomplete, and I've planned to add some more tests. It was a matter of 1-2 days, or so.

@tyranron tyranron changed the title WIP: 🔬 Make interfaces great again! 🔬 Make interfaces great again! Oct 6, 2020
@tyranron tyranron deleted the async-interfaces branch October 6, 2020 08:37
tyranron added a commit that referenced this pull request Oct 6, 2020
@tyranron
Copy link
Member Author

tyranron commented Oct 6, 2020

Fixed Book in b1a0366

@davidpdrsn
Copy link
Contributor

Amazing work @tyranron! 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::documentation Related to project documentation semver::breaking Breaking change in terms of SemVer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable interface tests in the book
3 participants