Merge confusing World and WorldInit derives

[tyranron]

@ilslv @theredfish what about instead

#[derive(Debug, WorldInit)]
struct World {
    user: Option<String>,
    capacity: usize,
}

#[async_trait(?Send)]
impl cucumber::World for World {
    type Error = Infallible;

    async fn new() -> Result<Self, Self::Error> {
        Ok(Self { user: None, capacity: 0 })
    }
}

to have

#[derive(Debug, World)]
#[world(init = Self::new)]  // optional, and uses `Self::default()` if not specified
struct World {
    user: Option<String>,
    capacity: usize,
}

impl World {
    fn new() -> Self {  // allows also `async` and `Result`
        Self { user: None, capacity: 0 }
    }
}

?

[theredfish]

The second code block is much easier to read, but #[world(init = Self::new)] might be a bit confusing since it's optional. The user will need to carefully read the doc to know about it. But should be fine if we emphasize it.

However I'm not sure about this : allows also async. Isn't it necessary since the framework is "async naturally"? We use @serial for sync tests. What happens exactly if we don't use the macro #[async_trait(?Send)]?

[tyranron]

@theredfish

However I'm not sure about this : allows also async.

This means that we don't force the library user to make initialization function only sync or async only. It will accept both whether user requires async initialization or not. The point is that user writes exactly the signature he needs, and the generated code takes care itself about any type squating.

What happens exactly if we don't use the macro #[async_trait(?Send)]?

The library user won't be able to initialize his World having .awaits inside. Not quite good.

But this approach allows us to derive the World trait implementation for the user, so he won't need to specify #[async_trait(?Send)] attribute ever.

[ilslv]

@tyranron separation of World and WorldInit bothers me too, but I'm sure about proposed solution.

1. I don't think we'll be able to accept #[world(init = Self::new)], as we don't know return type (async or not and Result or Self). We've encountered similar issue in juniper with Results.
2. I see macros as a last resort that should be used only if there is no sound way to do it in plain Rust or it will require too much boilerplate. I don't think this is the case.

But I see a solution here without using macros. First thing that is bothering me, that World reads like the main trait to implement and WorldInit may be forgotten (in that case I think there are pretty messy error messages). This way the case in transitional period, where we've also widely supported old declarations without auto-wiring and a new one. Now this isn't the case and we should switch World and WorldInit.

#[derive(Debug, cucumber::World)]
struct World {
    user: Option<String>,
    capacity: usize,
}

#[async_trait(?Send)]
impl cucumber::WorldInit for World {
    type Error = Infallible;

    async fn new() -> Result<Self, Self::Error> {
        Ok(Self { user: None, capacity: 0 })
    }
}

And finally we can provide blanket impl for Default, so example would become

#[derive(Debug, Default, cucumber::World)]
struct World {
    user: Option<String>,
    capacity: usize,
}

This should cover majority of the cases, as all async initialisations should happen on Given steps, not in the World constructor.

[tyranron]

@ilslv you've misunderstood the idea slightly. We don't touch WorldInit and World traits. They remain the same, and their separation is reasonable enough (WorldInit is required for macros feature only and one could use World trait amnually without bothering about WorldInit at all).

The idea is just to replace the WorldInit derive macro with a little bit more clever World derive macro, removing for the library user the necessity of writing full-fleged World impl (and so importing #[async_trait], and so spell both WorldInit and World at the same time) and in some trivial cases allowing it to skip at all. The macro will write this impl itself in addition to WorldInit impls.

I don't think we'll be able to accept #[world(init = Self::new)], as we don't know return type (async or not and Result or Self). We've encountered similar issue in juniper with Results.

The Result polymorphism is easy part (we have that in juniper already). The harder part is polymorphism over asyncness. I'd try to squat around impls for fn pointers. But if nothing works we always may end up with a slightly different attribute argument (like clap does).

This should cover majority of the cases, as all async initialisations should happen on Given steps, not in the World constructor.

This is too restrictive assumption. I'd like not to make it as a decision base.

[ilslv]

@tyranron I forgot, that we have macros as an optional feature. In that case switching World and WorldInit isn't really an option.

The Result polymorphism is easy part (we have that in juniper already). The harder part is polymorphism over asyncness. I'd try to squat around impls for fn pointers. But if nothing works we always may end up with a slightly different attribute argument (like clap does).

I've looked it up and in juniper we were struggling to extract the Error type from the Result and ended up delegating that to a different part of the code (using .map_err(Into::into) basically, as we were sure, that this trait bound is satisfied). But I'll try to generalise over async and Result. The knowledge of exact World type we are trying to convert into should be helpful.

This is too restrictive assumption. I'd like not to make it as a decision base.

Sure, I wasn't proposing to remove async initialisation entirely, there are suitable use cases for it. I was just making the argument that #[derive(Default, World)] should be enough for most cases.

[tyranron]

@ilslv well, that was easier than I thougth it would be 🙃

[ilslv]

Looking at this issue again, I'm not sure whether introducing breaking change like that worth it. I see 2 alternative options:

1. Just don't assume, that missing #[world(init = )] means that Default should be used. This won't break any code, while still allowing users to opt into this feature.
2. Same as option 1, but emit diagnostic-like waring (tracing::instrument does that), that soon missing #[world(init = )] will use Default impl.

[tyranron]

@ilslv it's OK to break here. We won't have WorldInit derive macro anymore, so users should pay attention to this anyway on upgrade.