Skip to content

Rework method chaining system of Rust generator#1602

Open
QuantumSegfault wants to merge 1 commit intobytecodealliance:mainfrom
QuantumSegfault:gen-rust-owning-method-chains
Open

Rework method chaining system of Rust generator#1602
QuantumSegfault wants to merge 1 commit intobytecodealliance:mainfrom
QuantumSegfault:gen-rust-owning-method-chains

Conversation

@QuantumSegfault
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

@alexcrichton

Requesting review.

@alexcrichton
Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants