Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add built-in implementations of `Default` for function definition and… #77688
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
FWIW, C++ also have this starting from C++20, search for "default constructor" in https://en.cppreference.com/w/cpp/language/lambda. |
|
Maybe I'm not seeing this properly but what would a default implementation of a function even mean semantically? |
|
@clarfonthey Each function definition and closure is its own type, so it would just be the function. |
|
Oh, I see, I missed the bit on only accepting closures. I dunno, semantically this still seems weird. Is there a reason why |
It doesn't just accept closures. Not only closures have their own types, function definitions also have their own types already too.
Imagine code like this: fn use_foo(x: Foo) {
...
}
fn adapt_bar<F: FnOnce(Foo) + Default>(bar: Bar) {
let f = F::default();
f(bar.into_foo());
}
fn get_adapted_function_ptr<F: FnOnce(Foo) + Default>(inner: F) -> fn(Bar) {
adapt_bar::<F>
}
fn main() {
let ... = get_adapted_function_ptr(use_foo);
}Without the These function/closure types are all zero-sized types, with the same properties as the unit type |
1914669
to
549412b
|
@rust-lang/compiler any chance I could get a review? |
|
I'm nominating this for discussion in the @rust-lang/lang meeting, this will require sign-off. |
|
Thanks |
|
In addition to making Code that needs this property is designed improperly and should be refactored, instead of being encouraged by introducing such weirdness to the core language itself. |
|
I feel like it would make more sense to me to introduce something like @nikomatsakis's "FunctionPointer" trait (rust-lang/lang-team#23) which presumably would permit synthesis/calling zero-sized closures much more naturally. That's somewhat harder, of course, but I'm not sure that this fits any of the current language priorities enough to merit what feels like a somewhat rushed solution. |
This doesn't make much sense to me. Closures still have unique types, this PR doesn't change that. If we're talking about instances of a closure type, I can already create multiple instances of closures in safe rust (one obvious way is that closures and function types already implement the Copy and Clone traits...). Also, functions are constructed from nothing all the time, I don't see why this is special. For example: fn foo() { /* do something */ }
fn pure_function() {
// Look, I constructed an instance of `foo` from nothing, just its name.
let my_fn_instance = foo;
my_fn_instance();
}
That's very presumptious of you. I welcome you to suggest an alternative to write "thunks" in safe rust. A thunk being a plain function that adapts another function in some way, for example: fn adapt_foo_to_bar<F: Fn(Foo) + Default>(bar: Bar) {
let f = F::default();
f(bar.into());
}Thunks are required when you need a plain function pointer, and cannot use a closure. Passing IMO, all publicly constructible unit types should implement A FunctionPtr trait would be nice. However, as you say it's more difficult and likely not a priority: given that, I don't feel this is a particularly rushed solution, it's simply the best solution for the foreseeable future, and one that is already in use elsewhere. For reference, prior to this PR, there are 126 lang items defined in Adding one more is not going to be the straw that breaks the camels back. Even if using lang items in this way is considered tech debt, I don't believe this is adding any tech debt beyond what already exists, since it's not using lang items in a new way, it's just following the existing patterns laid down in the compiler's code. Furthermore, I think that even if a FunctionPtr trait were to be added, the compiler is still the correct place for these implementations to exist: there are a lot of "special" types in the compiler:
The compiler generates these types, and the traits they implement are decided based on the internal structure generated by the compiler (eg. for closures, that would be its arguments, its upvars, etc.). Short of exposing every detail of these types within the type system (such as via an associated Upvars type on a closure trait) and extending the type system to be more powerful (two things which are so far off as to not even be worth considering), the logical thing is to have the compiler decide when to implement such types... And that requires lang items. |
|
While I also see a |
|
I fully agree that preserving a "small number" of lang items is not an issue here; I don't think it really matters whether Default is a lang item or not. Realistically in this case it's not default being special cased anyway, it's the function definition/closure types (which currently you cannot abstract over, due to lack of a FunctionPointer or similar trait). Yeah, I think the main reason I'm not a fan of adding a Default impl here is because you (probably) would never actually use it really as a "default"; e.g. Vec::resize'ing with it is super unlikely. It's also not really a Default but more just "the only value." Certainly the error message telling me that Default is not implemented for That said I agree that this concern is somewhat superficial; it is unlikely to cause problems in practice. It may be that the right answer is not Default (I think we agree here?) but that in practice Default is not really harmful and is simple to add today. |
But then why not create a And I don't think having this |
That's interesting to me. Do you see a distinction between things that should implement the For me, I've always seen |
|
IMO, even if no production code ever uses a This kind of uniformity makes refactoring and experimentation easier, even if (as above) it doesn't make it to production. You can move code around or reuse it, across functions or even modules, without as much contortion. |
|
Since some of the storm here is my fault, I want to say explicitly that I have no problem with |
|
Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
|
I'm in favor of making |
|
I actually don't think that my For the |
Yeah, I think that was a bit of a red herring: I took the suggestion to mean something that more directly generalizes the internal types. For example, all closures might implement a |
|
Looking more at the example @Diggsey brings up as illustrative of the expected use case, I think I've better grasped why Default feels a little wrong here. As @scottmcm points out, we're not trying to create new instances here, really, we're just smuggling an existing instance through the type rather than a value. In particular, this code would (very likely) break if state-carrying closures started conditionally implementing Default and creating Default instances of their captures. Since auto-deriving those impls seems at least plausible (like we do for Copy/Clone) I would be hesitant to make a guarantee here that we would never do so. It could be said that at that point we'd add a NonCapturingFn trait, but that seems like the wrong approach -- it would still create a runtime incompatibility with older libraries relying on the Default bound meaning NonCapturing. I think this is definitely an interesting pattern to solve. The types for which this implements Default are convertible to It seems like this solution is "heavier" in some sense; after all it would mean another fn-like trait. I think it's a better fit, though; it would match the requirements here better and avoid the problem of future expansion of Default impls. |
I think I see what people have been getting at around this: the objection is that for my use-case the In my case, this is a non-issue: since I am generating the closure in the first place, I know it doesn't have any upvars. The key point is that I am not relying on the If I allowed users of my library to supply their own type, and I used |
|
I feel like a lot of focus has been put on whether my particular motivation is best served by this feature. However, I'm not sure how relevant that actually is? Even if we ignore my motivation for this. It still makes sense, given the precedents that have been set, for voldemort types to implement core traits like |
|
+1 for adding |
|
Just for the sake of "the example", I have had similar needs. That being said, I see two main alternatives here: these function items / capture-less closures (these are the same; but they are not the same as a zero-sized closure which could, for instance, be capturing a ZST singleton, or even an uninhabited type) ought to be
Whatever the choice may be, I do agree with @Diggsey that this is something that is "missing" from the language, and so, ideally, it shouldn't take too long to palliate it |
|
If we decide to add a new trait, I think it should include a method/const to convert to a function pointer, since that could be handy to use in a generic context. |
Either: // Aesthetic nit
trait FnItem<Args> : Fn<Args> {
- const F: Self;
+ const CALL: Self;
}fn to_fn_pointer_example<F : FnItem(u8) -> u16> ()
-> fn(u8) -> u16
{
|arg: u8| (F::CALL)(arg)
}Or: trait FnItem<Args> : Fn<Args> {
- const F: Self;
+ const PTR: extern "rust-call" fn(Args) -> Self::Output; // can be coerced to `fn(Args…) -> Self::Output`
} |
| /// Function definitions and closures without upvars have a compiler-generated `Default` | ||
| /// implementation. | ||
| /// | ||
| /// The `DefId` is for `Default::default`, the `Ty` is the type `T` with the builtin `Default` impl. |
o752d
Nov 8, 2020
•
Contributor
Suggested change
/// The `DefId` is for `Default::default`, the `Ty` is the type `T` with the builtin `Default` impl.
/// The `DefId` is for `Default::default`, the `Ty` is the type `T` with the built-in `Default` impl.
there is inconsistency thoughout in the hyphenation of the(se) word(s) in this PR (and others), the surface area is limited to documentation so it maybe worth a simple rg and replace for consistency?
| /// The `DefId` is for `Default::default`, the `Ty` is the type `T` with the builtin `Default` impl. | |
| /// The `DefId` is for `Default::default`, the `Ty` is the type `T` with the built-in `Default` impl. |
there is inconsistency thoughout in the hyphenation of the(se) word(s) in this PR (and others), the surface area is limited to documentation so it maybe worth a simple rg and replace for consistency?
Diggsey
Nov 8, 2020
Author
Contributor
Yeah this is just from rebasing on top of other PRs that changed these things, will address that before this get merged.
Yeah this is just from rebasing on top of other PRs that changed these things, will address that before this get merged.
I also feel that's the source of the ickyness of using
Or any other trait than As an aside, I think a |
|
@Nadrieril Were we to add a By that logic, closures with no upvars must also implement |
|
@Diggsey Oh hm indeed, it seems silly to not have |
|
@Nadrieril I think this example explains that: fn example() -> impl FnOnce() -> i32 {
let x = 42;
|| x
}If this closure has a default, the default should clearly be an instance that returns Right now, the only closures where we can do this are closures which have no upvars. However, if we were to add a |
That's where the "upvar" conversation came from. I don't know if that's convincing enough, but I think it's a clear difference.
In the meeting, someone (boats, maybe?) said that if we were going to have a trait that allows this, it might as well be Though there might be a version of it that could make sense from a slightly different direction -- I could imagine something like an
I think this is the core question, though I'll emphasize a different place: are functions publicly constructible? Syntactically it's pretty clearly no. They're not This is reminding me a fair bit of the For closures I think it's even clearer -- especially since a closure is allowed to call let mut it = unsafe { iter::once(|| hint::unreachable_unchecked()) };
it.next(); // the closure instance is gone
some_other_crate::foo(it); // <-- is this necessarily sound? As far as I know that's sound today, since no matter how much it misbehaves there's nothing for safe code to call. And that would change if that closure -- which has no upvars -- were to implement I think what I've reached here is just the usual safety vs validity. Like other promised-to-be-a-ZST-but-not-publicly-constructible types, it seems to me that it's valid to summon one of these types from the æther, but that it shouldn't be safe. (Because we only have universal trait impls -- if we one day grew scoped impls, as is also desired for safe transmute, then Thus my inclination here is that this would be better done with some sort of |
Ah OK - still the "magic struct" is an implementation detail, and we don't specify what it looks like. For example, it could look like this: struct MagicClosure {
upvar1: Upvar<T1>,
upvar2: Upvar<T2>,
}Where
Yeah, I agree this is the important question. The place where this argument falls down is If we're going to say that closures should not "leak" any extra traits they implement in case there's a safety consideration, then automatically deriving Clone is also a mistake. But, not being able to have my own closures implement a trait just because there's a really obscure pattern, never seen in the wild, which might lead to unsafety if you have a particularly malicious upstream crate? Especially when there's a really easy way to avoid that danger, by just returning
Rust has always used types as capabilities. Any trait with an associated item, or a method that doesn't take
Right, but we've already set the precedent with closures that they do implement additional traits (that might go against their safety requirement). |
|
It feels like we're approaching the point where we need a kind of summary comment / write-up to capture these considerations. I found both @scottmcm's latest comment -- as well as @Diggsey's comment -- quite insightful. It's true for example that cloning a closure is not necessarily valid, there might be program logic that relies on the closure not being cloned. It seems also obviously true that I think for me there is a real expectation that you get capabilities by having access to a value of the closure, which is why I prefer the const-generics-based solution here, but I'm trying to find good ways to validate that intuition or justify it. It's also true that the risk of "accidentally applying default" or clone to a closure feels relatively low, given that the function must explicitly use I guess I'm softening my resistance through these arguments. |
|
I think the main intuition for this is that because one can't name the type of a function definition or closure without already having an instance of said type, that Note that the original motivating example also requires a value of the type, which really means that
is already fulfilled - the only way to get them is through inference.
|
|
You can definitely "leak" the type without ever constructing an instance, but the only examples I can think of are in a "downward" direction right now (i.e., you can only leak it to code that you yourself invoked). I guess with named impl Trait this would not be true. fn foo() {
let mut x = None;
if false {
x = Some(|| 22);
}
bar(x); // leaks the type of the closure above, even though it was never constructed
}
fn bar<T>(x: Option<T>) { } |
|
We discussed this again in today's lang team meeting. The general consensus was that we don't think we should make this change. This would add a new capability, allowing you to materialize and call a function, knowing only the type of that function. That seems like surprising behavior; for instance, if you're passed a We aren't opposed to other potential solutions to the underlying problems (e.g. writing function wrappers). But we'd like to see that start with a problem statement (in an MCP), not a proposed solution. @rfcbot close |
|
Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Well actually that was an example of something that's already the case with Clone The triage discussion was informative. I feel like the cat's already out of the bag re: types as capabilities, and that we'll inevitably end up with something like this in the long run. That said, I understand the desire for caution and that this isn't a huge priority.
Other than const-eval, I don't know how the "thunk" example is possible without giving function types this capability, so if the primary objection is giving "function call" capability to types rather than values, then I'm not sure what could be proposed. The "thunk" use-case is where you specifically want types to have the "call function" capability. |
|
One thing that @joshtriplett didn't capture in his write-up is that I for one would very much like to see a write-up as a design note. I think we uncovered a lot of interesting space in this write-up. I'm still a bit reluctant to go forward with this PR at this moment, though. @Diggsey would you be interested in trying to produce such a write-up? (I should note that I'm not as concerned about soundness in particular.) |
|
Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
|
|
I'm a bit late to the discussion, but I think that's only sort-of true? A quick test on the playground seems to tell me that a closure becomes Unless I'm missing some restrictions on propagating closure derive requirements. |
… non-capturing closure types.
Without these implementations, there's currently no safe way to construct these types from nothing. Furthermore, there's no way to safely constrain an unsafe implementation because the necessary bounds are not expressible.