Redesign #[graphql_scalar] macro

[ilslv]

Part of #1010

Synopsis

Main goal of this PR is to support generic scalars.

Solution

In addition to this PR tries to unify #[graphql_scalar] and #[derive(GraphQLScalar)] macros. This is done by introducing ability to use custom resolvers:

#[derive(GraphQLScalar)]
#[graphql(to_output_with = Self::to_output)]
struct Scalar(i32);

impl Scalar {
    fn to_output(&self) -> Value {
        Value::scalar(self.0 + 1)
    }
}

But we can't remove #[graphql_scalar] macro entirely because of uncovered use-case of derive macro: types from other crate

#[graphql_scalar(
    with = object_id,
    parse_token = String,
    scalar = LocalScalarValue,
)]
type ObjectId = bson::oid::ObjectId;

mod object_id {
    use super::*;

    pub(super) type Error = String;

    pub(super) fn to_output(v: &ObjectId) -> Value<LocalScalarValue> {
        Value::scalar(v.to_hex())
    }

    pub(super) fn from_input(v: &InputValue<LocalScalarValue>) -> Result<ObjectId, Error> {
        v.as_string_value()
            .ok_or_else(|| format!("Expected `String`, found: {}", v))
            .and_then(|s| {
                ObjectId::parse_str(s).map_err(|e| format!("Failed to parse `ObjectId`: {}", e))
            })
    }
}

Checklist

• Created PR:
  • In draft mode
  • Name contains Draft: prefix
  • Name contains issue reference
  • Has assignee
• Documentation is updated (if required)
• Tests are updated (if required)
• Changes conform code style
• CHANGELOG entry is added (if required)
• FCM (final commit message) is posted
  • and approved
• Review is completed and changes are approved
• Before merge:
  • Milestone is set
  • PR's name and description are correct and up-to-date
  • Draft: prefix is removed
  • All temporary labels are removed

[tyranron]

@ilslv why do we need trait bounds at type alias definition at all? I think we may simply deny them.

[ilslv]

@tyranron in case an original struct lacks them and we need to enforce them. Imagine, that chrone::DateTime<Tz> wouldn't have : TimeZone bound on a struct level (in most cases this considered to be the good practice), but we need them in scalar impl. In that case this is especially critical, as this approach is intended to be used with foreign types.

[tyranron]

@ilslv so, we need them on generated impls, not the type alias itself.

Then let's allow them as far as Rust allows them, but also let's provide alternative way via additional attribute argument allowing to specify any desired additional bounds like:

#[graphql_scalar(where(Tz: chrono::TimeZone + Debug, chrono::DateTime<Tz>: fmt::Debug))]
type DateTime<Tz> = chrono::DateTime<Tz>;

[tyranron]

@ilslv anything else seems well enough 👍

The part with from_input_error = String still quite bothers me, but I don't see how to remove it. The with = module pattern is quite neat. 👍

[tyranron]

Let's allow only parse_token(i32) argument form. parse_token = i32 is kinda confusing.

[tyranron]

Let's mark such places with // TODO comment to not forget restore it to derive variant in the next PR.

[ilslv]

@tyranron I don't know, why I didn't thought of this earlier, but we can expand into something like

impl<S: ScalarValue> FromInputValue<S> for Scalar {
    type Error = FieldError<S>;

    fn from_input_value(v: &InputValue<S>) -> {
        forwarded(v).map_err(IntoFieldError::<S>::into_field_error)
    }
}

Just transform provided error into FieldError by the way.

[tyranron]

@ilslv with necessity of type Error being removed it's much more pleasant now 👍

Regarding this pattern, I wonder whether we can push it a little bit further in terms of ergonomics and have this:

#[graphql_scalar(
    name = "TestComplexScalar",
    with = Self, // or `with = ComplexScalar`, so it acts like module
    parse_token(String),
)]
type ComplexScalar = TestComplexScalar;

impl ComplexScalar {
    fn to_output<S: ScalarValue>(&self) -> Value<S> {
        graphql_value!("SerializedValue")
    }

    fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Self, String> {
        v.as_string_value()
            .filter(|s| *s == "SerializedValue")
            .map(|_| Self)
            .ok_or_else(|| format!(r#"Expected "SerializedValue" string, found: {}"#, v))
    }
}

We may even dare to not require specifying with = attribute argument, implying this case as a default one. And leave with arguments only for overriding things.

See how nice it is:

#[graphql_scalar(
    name = "TestComplexScalar",
    parse_token(String),
)]
type ComplexScalar = TestComplexScalar;

impl TestComplexScalar {
    fn to_output<S: ScalarValue>(&self) -> Value<S> {
        graphql_value!("SerializedValue")
    }

    fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Self, String> {
        v.as_string_value()
            .filter(|s| *s == "SerializedValue")
            .map(|_| Self)
            .ok_or_else(|| format!(r#"Expected "SerializedValue" string, found: {}"#, v))
    }
}

Seems that nothing forbids this, while having it finaly feels "quite right" as reduces boilerplate amap.

[ilslv]

@tyranron with = Self is a great idea and it even works on current branch, but we should definitely add tests for it.

I'm not sure about not specifying with =.

First of all this makes errors very hard to interpret, as the is no way to check whether method exists with pretty error. Especially here I think it's important, as this is a most basic form of a macro, where we should guide users with well-formed error messages on how to achieve their goal, rather then yell at them with hard to understand compiler error.

Also let's remember the use-case for #[graphql_scalar] macro: implement scalar on foreign types. If you can impl on a type, why not use #[derive(GraphQLScalar)]? It now has all the capabilities of #[graphql_scalar].
Also this syntax will be unique for the #[graphql_scalar], as with #[derive(GraphQLScalar)] it will introduce ambiguity:

// Should this use field or Self?
#[derive(GraphQLScalar)]
struct ComplexScalar {
    field: String,
}

impl ComplexScalar {
    fn to_output<S: ScalarValue>(&self) -> Value<S> {
        graphql_value!("SerializedValue")
    }

    fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Self, String> {
        v.as_string_value()
            .filter(|s| *s == "SerializedValue")
            .map(|_| Self)
            .ok_or_else(|| format!(r#"Expected "SerializedValue" string, found: {}"#, v))
    }
}

// And what to use now?
#[derive(GraphQLScalar)]
struct EvenMoreComplexScalar {
    field: String,
    another_field: i32,
}

impl EvenMoreComplexScalar {
    fn to_output<S: ScalarValue>(&self) -> Value<S> {
        graphql_value!("SerializedValue")
    }

    fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Self, String> {
        v.as_string_value()
            .filter(|s| *s == "SerializedValue")
            .map(|_| Self)
            .ok_or_else(|| format!(r#"Expected "SerializedValue" string, found: {}"#, v))
    }
}

I think this is the case where we should be explicit rather then implicit and preserve unified syntax across #[graphql_scalar] and #[derive(GraphQLScalar)]. Realistically #[graphql_scalar] should be the last resort, because of it's goal and usage of it requiring local ScalarValue implementation.

[tyranron]

@ilslv there is no misconception here. You're looking onto #[derive(GraphQLScalar)] and #[graphql_scalar] macros as to something different. I dare to treat them as the same thing, so, eventually, there will be no difference if we write:

#[derive(GraphQLScalar)]
struct MyScalar;

or

#[graphql_scalar]
struct MyScalar;

The semantics will be preserved, and both macro should act like a singe one.

So, keeping in mind the described above...

// Should this use field or Self?
#[derive(GraphQLScalar)]
struct ComplexScalar {
    field: String,
}

It should use Self.

// And what to use now?
#[derive(GraphQLScalar)]
struct EvenMoreComplexScalar {
    field: String,
    another_field: i32,
}

Again, Self.

If we'd like to provide some sort of delegation to the inner scalar here, we should do that with additional attributes like that:

#[derive(GraphQLScalar)]
#[graphql(transparent)] // delegates everything to the inner field
struct ComplexScalar {  // only single-field structs are allowed
    field: String,
}

or even:

#[derive(GraphQLScalar)]
struct EvenMoreComplexScalar {
    #[graphql(delegate)] // delegates everything to this inner field
    field: String,
    another_field: i32,
}

But the default macro semantics shouldn't change depending on what it's applied to.

First of all this makes errors very hard to interpret, as the is no way to check whether method exists with pretty error. Especially here I think it's important, as this is a most basic form of a macro, where we should guide users with well-formed error messages on how to achieve their goal, rather then yell at them with hard to understand compiler error.

Correct me if I'm wrong, but it seems that the default compiler error here won't be that bad, as it only will say about "No method to_output found for type MyScalarValue" or something similar. Actually, could you provide an example of such error?

Also let's remember the use-case for #[graphql_scalar] macro: implement scalar on foreign types.

And this point seems quite right here, as foreign types will unlikely have those methods defined. So I propose making with = (or similar) arguments mandatory only when the macro is placed onto a type alias, and implying with = Self automatically for structs (despite what macro is used exactly).

[ilslv]

@tyranron

Actually, could you provide an example of such error?

struct CustomCounter(i32);

#[graphql_scalar]
type Counter = CustomCounter;

impl Counter {
    // missing impl
    // fn to_output<S: ScalarValue>(v: &Counter) -> Value<S> {
    //     Value::scalar(v.0)
    // }

    fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Counter, String> {
        v.as_int_value()
            .ok_or_else(|| format!("Expected `String`, found: {}", v))
            .map(CustomCounter)
    }

    fn parse_token<S: ScalarValue>(value: ScalarToken<'_>) -> ParseScalarResult<'_, S> {
        <i32 as ParseScalarValue<S>>::from_str(value)
    }
}

error[E0599]: no function or associated item named `to_output` found for struct `with_self::CustomCounter` in the current scope
   --> integration_tests/juniper_tests/src/codegen/impl_scalar.rs:424:5
    |
422 |     struct CustomCounter(i32);
    |     -------------------------- function or associated item `to_output` not found for this
423 | 
424 |     #[graphql_scalar]
    |     ^^^^^^^^^^^^^^^ function or associated item not found in `with_self::CustomCounter`
    |
    = note: this error originates in the attribute macro `graphql_scalar` (in Nightly builds, run with -Z macro-backtrace for more info)

#[derive(GraphQLScalar)]
struct EvenMoreComplexScalar {
    #[graphql(delegate)] // delegates everything to this inner field
    field: String,
    another_field: i32,
}

Ok, with introduction of #[graphql(delegate)]/#[graphql(transparent)] things are much clearer now. I do like this approach more, as usually just newtyping a struct isn't enough to call it a new scalar, you need to check some additional invariants.

So I propose making with = (or similar) arguments mandatory only when the macro is placed onto a type alias, and implying with = Self automatically for structs (despite what macro is used exactly).

I do quite like this approach 👍

[ilslv]

I do have 1 question left though:

so, eventually, there will be no difference if we write:

#[derive(GraphQLScalar)]
struct MyScalar;

or

#[graphql_scalar]
struct MyScalar;

The semantics will be preserved, and both macro should act like a singe one.

Should this PR also implement #[graphql_scalar] on structs or only type aliases? Should we tread this PR like "implementing #[graphql_scalar] entirely" or rather "implementing #[graphql_scalar] for type aliases" and leave #[graphql_scalar] and #[derive(GraphQLScalar)] on structs and enums for #1017?

[tyranron]

@ilslv

Should this PR also implement #[graphql_scalar] on structs or only type aliases?

I'd opt for yes. And not only structs, but enums/unions too. Any type supported by proc_macro_derive. Fortunately, the inner representation is totally user-defined.

Should we tread this PR like "implementing #[graphql_scalar] entirely" or rather implementing #[graphql_scalar] for type aliases and leave #[graphql_scalar] and #[derive(GraphQLScalar)] on structs and enums for #1017?

The implementation doesn't differ much for type aliases and structs. Though we may leave transparent implementation part for #1017, and impl here only basics which are similar for both type aliases and structs.