Uh oh!
There was an error while loading. Please reload this page.
Add built-in implementations of Default for function definition and… - #77688
Add built-in implementations of Default for function definition and…#77688Diggsey wants to merge 1 commit into
Default for function definition and…#77688Conversation
rust-highfive
commented
Oct 8, 2020
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
petrochenkov
commented
Oct 8, 2020
FWIW, C++ also have this starting from C++20, search for "default constructor" in https://en.cppreference.com/w/cpp/language/lambda. |
petrochenkov
commented
Oct 8, 2020
clarfonthey
commented
Oct 12, 2020
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. |
clarfonthey
commented
Oct 12, 2020
Oh, I see, I missed the bit on only accepting closures. I dunno, semantically this still seems weird. Is there a reason why |
Diggsey
commented
Oct 12, 2020
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: fnuse_foo(x:Foo){
...}fnadapt_bar<F:FnOnce(Foo) + Default>(bar:Bar){let f = F::default();f(bar.into_foo());}fnget_adapted_function_ptr<F:FnOnce(Foo) + Default>(inner:F) -> fn(Bar){adapt_bar::<F>}fnmain(){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
549412bCompareDiggsey
commented
Oct 21, 2020
@rust-lang/compiler any chance I could get a review? |
nikomatsakis
commented
Oct 22, 2020
I'm nominating this for discussion in the @rust-lang/lang meeting, this will require sign-off. |
Diggsey
commented
Oct 22, 2020
Thanks 👍 |
H2CO3
commented
Oct 28, 2020
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. |
Mark-Simulacrum
commented
Oct 28, 2020
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. |
Diggsey
commented
Oct 29, 2020
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: fnfoo(){/* do something */}fnpure_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: fnadapt_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. |
oli-obk
commented
Oct 29, 2020
While I also see a |
Mark-Simulacrum
commented
Oct 29, 2020
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. |
H2CO3
commented
Oct 29, 2020
But then why not create a And I don't think having this |
Diggsey
commented
Oct 29, 2020
That's interesting to me. Do you see a distinction between things that should implement the For me, I've always seen |
rpjohnst
commented
Oct 29, 2020
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. |
scottmcm
commented
Oct 30, 2020
Since some of the storm here is my fault, I want to say explicitly that I have no problem with |
bors
commented
Oct 31, 2020
☔ The latest upstream changes (presumably #78182) made this pull request unmergeable. Please resolve the merge conflicts. 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: |
joshtriplett
commented
Nov 1, 2020
I'm in favor of making |
nikomatsakis
commented
Nov 2, 2020
I actually don't think that my For the |
Diggsey
commented
Nov 2, 2020
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 |
Nadrieril
commented
Nov 11, 2020
@Diggsey Oh hm indeed, it seems silly to not have |
@Nadrieril I think this example explains that: fnexample() -> implFnOnce() -> 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 |
scottmcm
commented
Nov 11, 2020
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 letmut 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 |
Diggsey
commented
Nov 11, 2020
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: structMagicClosure{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). |
nikomatsakis
commented
Nov 11, 2020
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. |
carbotaniuman
commented
Nov 14, 2020
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. |
nikomatsakis
commented
Nov 16, 2020
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. fnfoo(){letmut x = None;iffalse{
x = Some(|| 22);}bar(x);// leaks the type of the closure above, even though it was never constructed}fnbar<T>(x:Option<T>){} |
joshtriplett
commented
Nov 17, 2020
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. |
Diggsey
commented
Nov 18, 2020
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. |
nikomatsakis
commented
Nov 19, 2020
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.) |
Diggsey
commented
Nov 20, 2020
bors
commented
Nov 20, 2020
☔ The latest upstream changes (presumably #79220) made this pull request unmergeable. Please resolve the merge conflicts. 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: |
rfcbot
commented
Dec 8, 2020
🔔 This is now entering its final comment period, as per the review above. 🔔 |
phaylon
commented
Dec 10, 2020
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. |
rfcbot
commented
Dec 18, 2020
The final comment period, with a disposition to close, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
oli-obk
commented
Dec 28, 2020
Closing as per the FCP |
… 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.