Generic AST

[theduke]

Refactor all the AST types to use Cow<'a, str> instead of String.

This allows using the Ast both in a borrowed style,
and a owned style with Cow<'static, str>

This alleviates performance concerns of juniper in switching to
this parser instead of the custom one.

[theduke]

Note: I also want to add ::into_owned() methods that convert everything from AstType<'a> to AstType<'static>.

[tailhook]

Hi! I'm okay with the changes. The question is: are you sure that using Cow is perfect?

The other options are:

1. Use some kind of Rc<str>/Arc<str>
2. Use some special string type like smallstr or tendril
3. Make it generic over a type string

My doubts are because of:

• I think that working with the ast will require into_owned anyway, especially in asynchronous code. Which makes using Cow a burden not improvement.
• Size of Cow is 32 bytes even if bytes are borrowed, most of the strings after parsing are < 32 bytes
• It may be beneficial to intern (i.e. deduplicate) parsed strings, especially field names (this might be a point for (3) or maybe (2) of the above)

Have you considered other options?

[theduke]

@tailhook I initially wanted to implement a generic solution but then thought it was too cumbersome.

But I revisited the code and I think I've found a reasonably decent structure that enables the AST to be used pretty seamlessly with any type (that implements the new Text trait, right now just &str, Cow, and String).

It does unfortunately produce a bit of noise, because a new wrapper type is required. (Called Str) with takes both a lifetime (carried in a PhantomData marker), and a generic type T: Text<'a>.

The important changes are all on top of common.rs.

This would give users the ability to tailor the AST to their use case, and it doesn't introduce too much awkwardness in the codebase.
Most of the changes are just adding the extra generic T parameter and a trait bound where required.

ps: The implementation does require one unsafe call when using String see here. This just transmutes the unused input lifetime into 'static and is safe.

The PR would need some cleanup and documentation changes, just want to get your view first.

[theduke]

Also, the Str wrapper is theoretically just a internal helper that would not need to be exposed, but that would require duplicating all struct definitions and then transmuting.

[tailhook]

At a glance looks promising. Just quick question so far: why Text trait has a lifetime? Usually, generic types like Document<T: StrLike> allow to use borrowed type as a type parameter, i.e. Document<Cow<'a, str>. I'm probably missing something obvious, but can't find out off the top of my head.

[theduke]

Without it we can't introduce the constraint that Text can be constructed from &'a str and is still bound to the lifetime 'a. (I think)

eg: trait Text: From<&str> {} doesn't work, it requires an explicit lifetime.

[tailhook]

Okay. Makes sense. I'm okay with merging it when you are ready.

(Frankly, I'm still not sure if generics are good in the long run, but I think this is the best to make it to the next release and see how it will be used)

[theduke]

@davidpdrsn @tomhoule since this will affect both of you, do you see any problems with these changes?

[theduke]

I'm still not sure if generics are good in the long run

I don't see any real downsides since it just increases the flexibility.

I think users can just use a defined type like type Document = parser::Document<'static, String> and avoid having to deal with the constraints everywhere.

I don't see any hurry in merging though, I will try to port juniper over to this branch and see if I notice any stumbling blocks.

Also I'll submit some PRs for the spanning infrastructure, since the juniper parser currently has quite a bit more spanning going on and we'll need that for error reporting.
That will potentially also be a breaking change.

[theduke]

I might have found a solution that does not require a wrapper type.

Still fighting some type checker issues, but looks promising:

trait Text<'a> {
    type Value: 'a + AsRef<str> + From<&'a str> + PartialEq + Eq;
}

impl<'a> Text<'a> for &'a str {
    type Value = Self;
}
impl Text<'static> for String {
    type Value = Self;
}

struct Node<'a, T: Text<'a>> {
    name: T::Value,
}

fn compare<'a, T: Text<'a>>(a: T::Out, b: T::Out) -> bool {
    a == b
}

fn main() {
    let a = Node::<&str>{ value: "blub" };
    let b = Node::<&str>{ value: "blub" };
    dbg!(a.value == b.value);
}

[davidpdrsn]

Looks like a good solution to me 👍

Am I correct that users could write the following

// Will use owned `String`s
parse_schema::<String>(&schema)

// Will borrow things from `schema`
parse_schema::<str>(&schema)

For the last one, wouldn't that require parse_schema to be define as

fn parse_schema<'a, T: ?Sized>(_: &'a str) {}
//                     ^^^^^^ To allow for `str` to be used

I might also be totally misunderstanding things 😜

[theduke]

@davidpdrsn I already added a &str example to the doc tests for testing, it just looks like this:

let ast = parse_query::<&str>("query MyQuery { field1, field2 }")?;

The definition of parse query just had to change to (with the first implementation with a wrapper type):

pub fn parse_query<'a, S>(s: &'a str) -> Result<Document<'a, S>, ParseError> 
    where S: Text<'a>,

[davidpdrsn]

Great 👍 I'll implement this change in juniper-from-schema as soon as it is merged.

[theduke]

Not ready for merge yet, but I've pushed the new generic version without a wrapper type.

Tests are passing so looks good.

[tailhook]

Looks good for me.

[LegNeato]

How are we doing here?

[LegNeato]

This is blocking graphql-rust/juniper#324, anything I can do to help push it forward?

[archseer]

Hey @theduke, is there anything we can do to help getting this merged?

[tomhoule]

Sorry, I hadn't seen the mention in a previous comment. I think this change is good for graphql-client, we already try to do zero-copy processing of the schema when parsing from JSON. So no worry from my part.

[LegNeato]

@theduke @tailhook What more do we need here?

[theduke]

If I remember correctly it's ready.

I'll browse over the code to be sure.

[theduke]

@tailhook I fixed some deprecation warnings and checked the code.

I think this should be good to merge if you are agreed.

[tailhook]

Merged. Thanks!

[theduke]

I will probably do a follow-up PR that adds one or two submodules with type aliases for all types, so you don't have to specify the generic argument everywhere.