Rework method chaining system of Rust generator

[QuantumSegfault]

Follow up to #1586

After testing, #1586 had some major flaws in practice

• In contrary to Rust idioms, the method chaining was done using &Self, which makes things like passing a method chain into a parameter expecting an own more cumbersome than necessary.
• It was a blunt instrument, being at best minor pollution, and at worst, a hindrance. Especially once the first point is addressed, forcing all void returns to be owning self (and by extension take owning self) was not practical.

This replaces the --enable-method-chaining switch with a --chainable-methods parameter, that works similar to the current async filter. You can choose to opt-in to it for all methods, for all methods within a particular resource, or just for a particular method (or any combination of those).

Additional, methods with method chaining enabled now take owning self instead of the default borrowing &self, and are -> Self rather than -> &Self. The ABI remains unaffected.

[QuantumSegfault]

@alexcrichton

Requesting review.

[alexcrichton]

For crates/core/src/chainable_method.rs it looks like that's more-or-less a duplicate of crates/core/src/async_.rs I think? Could those be deduplicated? Overall though I do think it's nice to have more fine-grained control over the configuration here.

For the implementation, personally I think returning &Self would be more suitable here because that still allows chaining and is still an idiomatic way of doing chaining in Rust. It's true that it doesn't allow returning something, so therading it all the way through would require a temporary, but personally I think that's reasonable (and is already the case for other builder-like patterns such as Command in the standard library).

Additionally for the implementation, would it make sense to give the FunctionBindgen structure access to the underlying Function? That way the return_self: bool parameter should largely go away since the function itself could be stored and then the predicate could be called on-demand.

[QuantumSegfault]

It is indeed a duplicate, but with a couple of changes (filter by resource; but no import vs export). They probably could be dedupped to an extent...not sure how easily though.

As for owning vs borrowing, in our use case its the difference between doing:

let command = Command::new(&["fly".to_string()], "Gives you the ability to fly.");

let with_speed = CommandNode::argument(
    "speed",
    &pumpkin_plugin_api::command_wit::ArgumentType::Float((Some(0.0), Some(1.0))),
);
with_speed.execute(WithSpeedExecutor);

let no_speed = CommandNode::argument(
    "target",
    &pumpkin_plugin_api::command_wit::ArgumentType::Players,
);
no_speed.execute(NoSpeedExecutor).then(with_speed);

command.then(no_speed).execute(BaseExecutor);

vs:

let mut command = Command::new(&["fly".to_string()], "Gives you the ability to fly.")
.then(
    CommandNode::argument(
        "target",
        &pumpkin_plugin_api::command_wit::ArgumentType::Players,
    )
    .execute(NoSpeedExecutor)
    .then(
        CommandNode::argument(
            "speed",
            &pumpkin_plugin_api::command_wit::ArgumentType::Float((
                Some(0.0),
                Some(1.0),
            )),
        )
        .execute(WithSpeedExecutor),
    ),
)
.execute(BaseExecutor);

Have to break up the chain in reverse order instead of being able to have one atomic builder.

EDIT: I cleaned up the "before" example to be a little more fair.

---

As for exposing Function directly...that might work. All depends if FunctionBindgen has everything else that is needed to give to the query.

[alexcrichton]

For deduplication, given how highly similar the two parsing modules are that's why I'd want to make sure they can be kept in sync. I think it makes sense to necessarily make the overall implementation a little more complicated to handle the two needs, but I'd prefer to not copy/paste the entire file, then edit small bits, and in the end have more-or-less twice as much code to maintain.

I'm wary to try to pick "the one true Rust idiom" here personally w.r.t. moving ownership around. There's always the possibility of wrapping the generated types and/or extending them with methods, so while it's a blurry line I do think there should be a line between what's expected to be auto-generated vs hand-written. Given the interaction with the rest of the system, is it reasonable to say that your use case here might be best served by hand-written bindings? For example an sdk-style crate would have auto-generated bindings + handwritten ones layered on top too.

[QuantumSegfault]

I think that's accurate, especially since we already do have a very a small per-language SDK as it stands.

The question then becomes whether the automatic method chaining provides any real value at all then? If when it comes down to it, we're going to be wrapping all the methods of interest in owning-chain form anyway (well, we may not, and just settle for the borrowing chains; but if we were to insist on onwing chains for our particular use case).

[alexcrichton]

That's true yeah. I don't think it's unreasonable to have something built-in, but I agree it'll have limited utility due to there not being a one-size-fits-all answer.

[QuantumSegfault]

Wait, aren't we missing an obvious solution? We are already introducing a fine-grained filter...what if we allow the user to choose between owning vs borrowing as they select methods to chain? Also a switch to control the default (marking as chainable, without specifying owning: vs. borrowing:); which itself defaults to borrowing?

[alexcrichton]

It's possible, yeah, and that sort of comes down to when it comes to codegen/bindgen you can always do whatever you want with a sufficiently complicated code generator. In other words I think it boils down to complexity. If it's a huge amount of code to maintain to implement this, it may not be worth it, but if it's easy enough to implement, is well tested and solid, etc, then it's probably fine.

[QuantumSegfault]

Shouldn't be too much more complexity than is already present. Just taking those should_return_self and making them a ternary instead, and modifying the command-line option/filter-set.

Which I still need to de-dup.