Add built-in implementations of `Default` for function definition and… by Diggse...

Contributor

rust-highfive commented on Oct 8

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov commented on Oct 8

… zero-sized closure types.

FWIW, C++ also have this starting from C++20, search for "default constructor" in https://en.cppreference.com/w/cpp/language/lambda.
Rust unlike C++ previously never integrated Default at the language level though.

clarfonthey commented on Oct 12

Maybe I'm not seeing this properly but what would a default implementation of a function even mean semantically?

carbotaniuman commented on Oct 12 •

@clarfonthey Each function definition and closure is its own type, so it would just be the function.

clarfonthey commented on Oct 12

Oh, I see, I missed the bit on only accepting closures.

I dunno, semantically this still seems weird. Is there a reason why Clone wouldn't work instead? Could just as easily be implemented but wouldn't be as confusing IMHO.

Diggsey commented on Oct 12

Oh, I see, I missed the bit on only accepting closures.

It doesn't just accept closures. Not only closures have their own types, function definitions also have their own types already too.

I dunno, semantically this still seems weird. Is there a reason why Clone wouldn't work instead? Could just as easily be implemented but wouldn't be as confusing IMHO.

Copy and Clone are already implemented for these types, but that doesn't help if you don't have an instance of the function already.

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 Default implementation there's no way to implement adapt_bar without changing its signature to accept an instance of F, which would then make it unusable in get_adapted_function_ptr where we need to return a fn(Bar).

These function/closure types are all zero-sized types, with the same properties as the unit type (). However, unlike () they do not currently implement Default, which leaves this hole in the language.

Diggsey commented on Oct 22

@rust-lang/compiler any chance I could get a review?

nikomatsakis commented on Oct 23

I'm nominating this for discussion in the @rust-lang/lang meeting, this will require sign-off.

Diggsey commented on Oct 23

Thanks

H2CO3 commented on Oct 29

In addition to making Default into a lang item, this is also very strange. Sure enough, it can be assigned meaning, but I'd scream if I ever actually saw code constructing a "default" value for a closure based on its unique type. After all, 1. this would mean that closures no longer have unique types, and 2. even if it can be made into something that works, it's very confusing to see a function constructed from nothing.

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 on Oct 29

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 on Oct 29

After all, 1. this would mean that closures no longer have unique types, and 2. even if it can be made into something that works, it's very confusing to see a function constructed from nothing.

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();
}

Code that needs this property is designed improperly and should be refactored

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 F as a parameter would change the signature of the function, thereby defeating the point of the thunk in the first place. The compiler itself generates thunks in some cases, yet there is no safe way to do it within the language, despite the type system being perfectly capable of that.

IMO, all publicly constructible unit types should implement Default. Such a type has only a single value, and so there is by definition a default. It's not like we're assigning some arbitrary behaviour: there can only be one possible behaviour. Function types are already unit types, and are trivially constructible. Therefore, they should implement Default: the only question is how to make that happen.

@Mark-Simulacrum

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 lang_items.rs. Many of these lang items are significantly less "fundamental" to the language than the Default trait, which to me is on par with Clone for "fundamentalness".

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:

• Function types
• Closure types
• Generator types

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 on Oct 29

While I also see a FunctionPointer trait to be the "right" fix for this situation, I also don't see a reason not to have Default implemented for capture-less closures and functions

Mark-Simulacrum commented on Oct 29

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 &fn() {foo} is likely to be pretty confusing since that does have a natural default too, like any pointer to ZST). That's in some sense just more broadly true though.

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 on Oct 29

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).

But then why not create a SingletonFunction auto-trait, and then add impl<F: SingletonFunction> Default for F {} in the standard library? Surely if we want to abstract over the functions, the right way to go is to encapsulate their behavior in a new trait specific to them, instead of lang-itemizing every other trait they touch.

And I don't think having this Default impl is so urgent that (ab)using something already-available should be pushed.

Diggsey commented on Oct 30

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"

That's interesting to me. Do you see a distinction between things that should implement the Default trait, and things which one might consider default-constructible ala C++ default constructors?

For me, I've always seen Default as simply Rust's solution for generalising over types with a unique parameter-less constructor. I'm struggling to grasp why Vec::resize is a "legitimate" use of default, whilst wanting to construct an instance of a type is not?

rpjohnst commented on Oct 30

IMO, even if no production code ever uses a fn/closure's Default impl, it's still a net win to add it, as it makes the language's built-in types more uniform. If you can create a value of type fn() {foo} or [[email protected]] with no arguments in a non-generic context, why not in a generic context? And given that that's desirable, why not use the same trait as everything else to do so?

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. Default is probably one of, if not the, most "canonical" trait signatures possible in Rust- a single zero-argument method that returns a value of its only type parameter.

scottmcm commented on Oct 31

Since some of the storm here is my fault, I want to say explicitly that I have no problem with Default being a lang item as the way to implement this. (Without taking a position either way, for the purpose of this post, on whether this should be done.)

bors commented on Oct 31

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:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

joshtriplett commented on Nov 2

I'm in favor of making Default a lang item as well.

nikomatsakis commented on Nov 2

I actually don't think that my FunctionPointer trait would help with this use case. That trait was meant to apply to types like fn(&u32) and so forth -- i.e., function pointers -- and those types cannot implement Default. In my initial version I didn't even include function definitions or closures, though I think there's a case to be made for including them.

For the Default impl, we would instead need a trait that is specific to zero capture closures and function definitions, and that excludes function pointers.

Diggsey commented on Nov 2

I actually don't think that my FunctionPointer trait would help with this use case.

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 Closure trait with an associated Upvars type, and then Default could be implemented by core for all T: Closure<Upvars=()>, thereby reducing the number of lang items to just the number of internal types.

danielhenrymantilla commented on Nov 4

Just for the sake of "the example", I have had similar needs. That being said, Default doesn't seem like the ideal abstraction; it's just the one that is easiest / fastest to implement and get going. I am thus not against it, but not 100% in favour either.

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 const-constructible:

• I've been feeling the lack trait FnItem<Args> : Fn<Args> { const F: Self; } as a clear "hole" in the current Fn design (independent of function pointers involving higher-order lifetimes, which is a separate, although also a bit annoying concern ).

• There has been a very interesting mention in a "similar" direction here, which is to notice that function items / capture-less closures are singletons. That's the whole thing with their unique types. So my suggested FnItem is not that far off the PublicSingleton / ConstSingleton suggestion, which could indeed be the shared trait between fn items and ().

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.

danielhenrymantilla commented on Nov 4

a method/const to convert to a function pointer

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)
}

  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`
  }

Nadrieril commented on Nov 11 •

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.

I also feel that's the source of the ickyness of using Default here: Default (to me) indicates "a reasonable way of constructing a particular value of that type". For the case at hand Default would mean "a reasonable way of constructing a particular function of that type". This suggests that we should also define it for closures with Default upvars, or other cases where there's a "reasonable" choice, like a noop function for fn(_) (or even the identity function for fn(T) -> T x) ).

Singleton would be the idea that a particular type has one and exactly one value. Those two ideas should imply radically different uses: if I want to create a collection generically, Default+Extend makes sense; if I want to smuggle a value through a type, Singleton makes that clear. Singleton::singleton is naturally const; Default::default should not be. Note also the distinction between "construction" and "existence": calling <Box<u8>>::default() twice gives me two different pointers, but calling singleton twice suggests I get the exact same value.

Or any other trait than Default would be fine by me. It's Default that I find confusing here. If find the FnStatic idea appealing too.

As an aside, I think a Singleton (auto-)trait might be useful as a more general-purpose feature? I know that Haskellers love their singleton types when they do type-level magic. I imagine it would be useful for the typenum crate or for crates that make extensive use of type-level struct Token;s.

Diggsey commented on Nov 11

@Nadrieril Were we to add a Singleton trait, do you agree that it should inherit from Default? By definition, any type which implements Singleton is also default constructible, so I would expect that to be the case.

By that logic, closures with no upvars must also implement Default: whether any additional closures implement Default is irrelevant to me: my code is still correct even if closures with upvars implemented Default, because I don't rely on the trait bound for correctness.

Nadrieril commented on Nov 12

@Diggsey Oh hm indeed, it seems silly to not have Singleton inherit from Default. Which makes the PR forward-compatible with Singleton if we ever want that. But also somewhat muddies the rationale for not going with Default, if we're going to get it anyways.
I dunno, I guess it will also boil down to how this feature is explained in the reference/guides so that users don't come to have miscalibrated expectations.

Diggsey commented on Nov 12 •

@Nadrieril
I think this doesn't really affect whether this PR should be merged, but you also questioned why closures shouldn't also implement Default if they have only Default upvars.

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 42, because that's how we define the closure. Therefore, closures should only implement Default when it is possible to do so in way that makes sense.

Right now, the only closures where we can do this are closures which have no upvars. However, if we were to add a Singleton trait, you could imagine that we could extend this to closures whose upvars are themselves singletons.

scottmcm commented on Nov 12

I have yet to see a really convincing argument for why Default is fundamentally different from Clone, that could then be applied when determining which traits other internal types should implement.

That's where the "upvar" conversation came from. Copy and Clone work like running their derive on the "magic struct" generated for the closure. If we decided closures should be Debug, then it'd also work like that. And we'd probably never have Eq on closure types, but if we did it'd work that way too. These are all things that work for the base case but also generalize to non-empty things. Whereas it seemed like Default only made sense to people in the "no captures" case.

I don't know if that's convincing enough, but I think it's a clear difference.

Were we to add a Singleton trait, do you agree that it should inherit from Default? By definition, any type which implements Singleton is also default constructible, so I would expect that to be the case.

In the meeting, someone (boats, maybe?) said that if we were going to have a trait that allows this, it might as well be Default, because telling people "no, you need the special default" doesn't really help anything. And I generally agree with that -- a special case of default would probably also allow the normal default. (All this said without taking a position on whether Singleton would be a good addition.)

Though there might be a version of it that could make sense from a slightly different direction -- I could imagine something like an FnStatic trait for things that can be called without an instance at all as a plausible fourth "ownership mode" for the set.

IMO, all publicly constructible unit types should implement Default.

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 impls; the fn vs pub fn difference exists. And with modules, being pub fn doesn't necessarily mean it's public to everyone, so a rule like "it impls default if it's pub" doesn't really work either (not that that's a rule I would support anyway, but as a thought experiment).

This is reminding me a fair bit of the Here! conversation in safe-transmute. My recollection of that was that part of the unhappiness with that was using the knowledge of a type as a capability, and that the rules weren't necessarily set up for that. Now, the case in this issue is much less severe than the implications of that one, but I think much of the discussion may still apply. I don't think passing someone an iter::Empty<F>() is a license to call it -- especially not if we get specialization in the future which would let people call it without a bound if it happened to be Default.

For closures I think it's even clearer -- especially since a closure is allowed to call unsafe methods if it's in an unsafe block in the parent function. Contrived example to illustrate the problem:

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 Default.

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 Default in their same scope seems clearly fine.)

Thus my inclination here is that this would be better done with some sort of Stateless marker trait with no safe methods -- what form that would take concretely I don't know, though I imagine it would probably be something a type would need to opt-in to with one of safe-transmute's promises, which it seems fine for function and closure types to do.

Diggsey commented on Nov 12

That's where the "upvar" conversation came from. Copy and Clone work like running their derive on the "magic struct" generated for the closure.

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 Upvar<T> never implements Default, and so actually #[derive(Default)] works exactly the same. I don't think we say enough about the magic struct to even say that Default differs from Clone.

I think this is the core question, though I'll emphasize a different place: are functions publicly constructible?

Yeah, I agree this is the important question.

The place where this argument falls down is Clone: I could equally well construct a FnOnce closure whose safety depends on it only being called once, but because Clone is automatically derived, I accidentally leak that to another crate. In fact, that example is a lot easier to come up with than any that might exploit Default, where you have to do some really contrived stuff to actually extract the type.

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 impl Fn() instead.

My recollection of that was that part of the unhappiness with that was using the knowledge of a type as a capability

Rust has always used types as capabilities. Any trait with an associated item, or a method that doesn't take self is a capability that you get from having knowledge of the type.

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.

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 on Nov 12

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 Default applied to a closure with upvars could lead to all manner of mistakes, since it is not clear that the closure is meant to operate on arbitrary values for its upvars.

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 impl Fn + Clone or something to return that sort of value to its callers (and otherwise, the cloning must occur during the function itself, and it seems like the function could do all kinds of buggy things so it's not clear why we should call this one particular pattern out).

I guess I'm softening my resistance through these arguments.

carbotaniuman commented on Nov 14

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 Default is really just a compile-time Clone, and given we already allow Clone, Default is a natural follow-up.

Note that the original motivating example also requires a value of the type, which really means that

there is a real expectation that you get capabilities by having access to a value of the closure

is already fulfilled - the only way to get them is through inference.

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);
}

nikomatsakis commented on Nov 16

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>) { }

joshtriplett commented on Nov 18

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 FnOnce you could call it more than once. We also feel like the broader type-system implications are not sufficiently understood to feel confident that this will not lead to any soundness issue.

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

rfcbot commented on Nov 18 •

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 on Nov 18

That seems like surprising behavior; for instance, if you're passed a FnOnce you could call it more than once.

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.

We aren't opposed to other potential solutions to the underlying problems (e.g. writing function wrappers).

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 on Nov 19

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.)

bors commented on Nov 20

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:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

rfcbot commented 12 days ago

This is now entering its final comment period, as per the review above.

phaylon commented 10 days ago

@nikomatsakis

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 impl Fn + Clone or something to return that sort of value to its callers (and otherwise, the cloning must occur during the function itself, and it seems like the function could do all kinds of buggy things so it's not clear why we should call this one particular pattern out).

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 Clone if it can be derived and it passes through anything requiring it to be Clone. So the Fn and Clone requirements can be quite far apart. For Clone you'd still need access to the value, of course. But for Default a closure this happens to should now potentially be constructable by everyone that can access its concrete type.

Unless I'm missing some restrictions on propagating closure derive requirements.

rfcbot commented 2 days ago

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.