[WIP] Impl Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Default for closures #62815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some changes occurred in diagnostic error codes |
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
luca-barbieri
force-pushed
the
closure_traits
branch
from
517e4ea
to
e2d5489
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Too wide-reaching for me! r? @BurntSushi |
Centril
added
A-closures
|
This is outside my wheelhouse too. r? @alexcrichton cc @rust-lang/libs I wouldn't be surprised if this needs an RFC, since this is adding a lot of impls to a core part of the language. |
|
This seems like a neat way to implement closures and add features for closures, thanks for the work here @luca-barbieri! I think the best way to go about landing this would be to split this into two pieces. One would be updating the compiler and moving the existing clone/copy impls to this new infrastructure, and that would largely require sign-off from the compiler team. For the libs team afterwards we could separately consider adding these new impls. I think the libs team wants to be in the loop on the first part since it'll likely influence the design, but @luca-barbieri how do you feel about that? |
|
New stable guarantees about closures, as provided in this PR, will require language team sign-off on an RFC. This is far too subtle and has interactions with other parts of the language (e.g. rust-lang/rfcs#2229, tuple variadics, tying the fate to tuples, exposing the type of the upvars, using cc @rust-lang/lang
There is no current consensus that it needs to be fixed for tuples (e.g. with variadics). |
|
Thank you for your feedback. I'll look at writing an RFC, although it may take a while until I have time to do so. |
|
Hey! This is a ping from triage, it seems to be a great plan @luca-barbieri. @rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review |
rustbot
added
S-waiting-on-author
|
Just want to chime in on a use-case for this work. I have been writing a WASM web framework in Rust along the principles of Elm. The app maintains a virtual dom (VDOM) which mirrors the DOM. There is also a View function which runs whenever some application state changes. It creates a new VDOM, which is diffed with the previous VDOM, and the diffs are applied to the DOM. This is a very common frontend-framework pattern. Problem - We want to create a closure to act as a listener for e.g. an A niche use-case to be sure - for now, at least! |
|
I also have a combinator library for memoized graph-shaped computations ( |
|
IMO this should be solved properly with VGs ("variadic generics") and some form of (statically typed) closure reflection (could be std/rustc-internal). We can defer the VG requirement by limiting closures like tuples, e.g. max 12 elements for most traits. For example, #[fundamental]
trait Closure {
type Captures/*: Tuple*/;
fn captures_ref(&self) -> &Self::Captures;
}
struct Capture<const NAME: &'static str, T>(T);roughly like this: impl Closure for /*typeof(|| x + y)*/ {
type Captures = (Capture<"x", i32>, Capture<"y", i32>);
fn captures_ref(&self) -> &Self::Captures {
// Note that this requires the closure to be represented as its Captures:
unsafe { &*(self as *const _ as *const _) }
}
}This is what a std impl based on this form of reflection would look like: impl<A, B, const NAME_A: &'static str, const NAME_B: &'static str, C> fmt::Debug for C
where C: Closure<Captures = (Capture<NAME_A, A>, Capture<NAME_B, B>)>,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let (a, b) = self.captures_ref();
f.debug_struct("closure").field(NAME_A, a).field(NAME_B, b).finish()
}
}Of course, there is a lot to bikeshed here, but I think this could work now/soon. |
|
Ping from Triage: Hi @luca-barbieri - any update on this? |
|
I'm probably not going to do any further work on this any time soon, so if anyone wants to write an RFC and do further work (e.g adopting @eddyb 's more featureful proposal) they are welcome to do so. |
Alexendoo
removed
the
S-waiting-on-author
Alexendoo
added
the
S-inactive
|
Thanks for the work @luca-barbieri, I'll close this as inactive to keep things tidy, if there's any change in the future please feel free to reopen it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request implements these traits for closures in addition to the Copy and Clone that the compiler already supports, as well as allowing crates to implement their custom traits for closures.
Motivation
As for motivation, there is the "serde_closure" crate that does this by re-implementing closures with a macro (except it can't automatically detect upvars), and in general Debug is useful for debugging, while the other traits are useful to determine whether instances of a closure are equal, which can allow to avoid re-calling the closure and instead using a memoized result (when the API requires the closure to be a pure function): this is useful for instance for React-like WebAssembly frameworks where a component may take as props closures that generate subcomponents and that should not be called again if props is set to props equal to the old props (which requires an impl of Eq for closures).
Implementation
This code uses a clever technique that allows to add this feature with minimal effort and maximal flexibility:
The advantage of this strategy is that it does not require any code to generate implementations of specific
traits, and allows to implement custom traits on closures without changing the compiler.
Drawbacks
The first drawback is that we inherit the tuple length limit on the impls. However, that needs to be fixed for tuples anyway, and the fix will fix closures as well.
Another apparent drawback that should however not be an issue is that we expose closures as tuples, allowing code to know about the upvar types. Since we use a special layout marker that includes the closure type and that can't be instantiated otherwise, this does not limit the ability to do profile-guided layout or in general to change closure layout in an arbitrary way. Also, closures are semantically a tuple of their upvars so exposing this should not be a problem.
We also expose an ordering of the upvars. However, there is a canonical ordering, which is the order of the source code position of the first mention of the upvar, that I hope the compiler is currently using and if so there seems to be no reason to change it.
Alternatives
Adapting the #[derive] code is not easily possible because it's broken (generates incorrect type bounds) and because it's based on the AST, and in that phase we don't even know what the upvars are.
Generating MIR like for the Clone shim is possible, but it requires ad-hoc code and wouldn't allow to implement for instance the serde traits without adding even more ad-hoc code to the compiler; on the other hand, the approach in this pull request allows to reuse trait implementations for tuples.
Shortcomings
The current implementation seems to work but has:
Conclusion
Let me know if this is considered a viable approach, and whether you would like a feature gate, RFC, or tests.