Make trait methods callable in const contexts, take III

[fee1-dead]

View all comments

Rendered.

Continuation of #3762

Important

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.

[Jules-Bertholet]

First of all: Thank you @fee1-dead for your work on pushing forward this feature over so long, working to get something shipped despite people's widely conflicting views.

That being said, I have some concerns with this approach:

View changes since this review

[Jules-Bertholet]

"You can't use custom marker traits with const traits" is not just an annoying restriction, but also one which it will be very hard for users to build a mental model of. And worst of all, it will encourage ecosystem libraries to jump the gun and make all their marker traits unnecessarily const trait—which they can then (IIUC) never take back without breaking changes.

More generally, I think the pursuit of syntax minimalism/opt-out at all costs is counterproductive, and will come at the cost of clarity, user understanding, and future language evolution. I understand the appeal of "no syntax" as a way to avoid syntax bikesheds. But in this case, I think we would regret it, especially as it would just lead to even more contentious bikesheds down the road, once we realize we have painted ourselves into a corner.

[fee1-dead]

I think I would need to hear out a more significant drawback than this to justify proposing syntax. I strongly dislike T: do() Trait as a way to opt out, and I am meh on T: ?const Trait.

The question is whether people really want the opt out right now, and how many users of const traits use a lot of marker traits. And even then, I feel like marker traits would not lose a lot, even if they turned into const traits.

[Jules-Bertholet]

I strongly dislike T: do() Trait as a way to opt out, and I am meh on T: ?const Trait.

I agree with you on that! I think opt-out is entirely the wrong approach, and would much prefer opt-in. But I might be willing to change my mind on that if there was an opt-out proposal with less TODOs than this one. That way, we can be confident that there is some reasonable path forward for the opt-out design—and we are not just painting ourselves into a corner where we will never have something complete/free of "temporary" hacks.

[fee1-dead]

I like to think of this proposal as not an opt-out proposal, or at least not entirely. We could, in a future edition, decide that T: Trait in these contexts really ought to mean non-const, and force everyone to use T: [const] Trait or a different syntax. Although the proposal does make opt-out seem like the natural extension (as opt-out would not require a new edition).

I think it is a fair reaction to think that this proposal leaves out too much to be designed and refined upon. It took me quite a long time, but I think I've learned to like this. Having a single meaning of the bound possible in these contexts makes edition changes less bumpy.

Marker traits, and many other ways people use a trait bound without using methods, such as only using associated types and constants, are valid use cases. But they are rare. And they still work under the proposal. Users just need to make these traits const. I agree that it seems technically wrong, and if we actually have an opt-out available, they should be using the opt out.

But still, why? Why should we introduce the idea of non-const bounds at all in the first place? Why teach it? Why make it possible? In an ideal world, non-const trait implementations are the exception and const traits and impls are the default. Even though we already have the default in const fn as trait bounds not requiring const, it's still not very useful. I don't feel like it unblocks any real use cases. Most it does is makes it a bit less annoying to use (I don't have to migrate my impls to be const to use it).

On the other hand, not teaching this thing or having this concept at all sounds nice too: to allow something to use const traits, slap const in front of your trait and impls. I don't think it would be that annoying if marker traits and impls for them have to be const too. And I can't think of ways it would be really harmful to the users.

I have a feeling that any proposals with syntax more than const trait and const impl would be very hard to get through (we've been doing this for a long time now!), but I think it would be worth getting T-lang's input on this first too.

[Jules-Bertholet]

Users just need to make these traits const. I agree that it seems technically wrong, and if we actually have an opt-out available, they should be using the opt out.

But still, why? Why should we introduce the idea of non-const bounds at all in the first place? Why teach it? Why make it possible?

This RFC's proposal forces marker traits which shouldn't need to care about const, to care. It forces maintainers of libraries defining such traits to deal with change requests from their users, when they shouldn't need to make any changes. It places the burden of const trait design flaws on everyone, not just people and parts of the codebase which choose to use the feature.

(Stabilizing a version of #[rustc_never_const_trait] wouldn't address this problem. It's inherent to the opt-out approach)

We could, in a future edition, decide that T: Trait in these contexts really ought to mean non-const, and force everyone to use T: [const] Trait or a different syntax.

We theoretically could, but we shouldn't. This would be particularly ugly as edition changes go, in terms of user confusion. We should do our best to get it right the first time.

[fee1-dead]

I take your point, as an example, maybe one wants to use Pod within such contexts, and we probably shouldn't tell bytemuck to change anything in their crate just to add such support.

This would be particularly ugly as edition changes go, in terms of user confusion. We should do our best to get it right the first time.

I agree with you here. But we didn't get far the many previous times we've talked about the syntax for this. I personally do wish that we have some syntax we can all agree on, but it doesn't look very likely. This is why I intend to keep opt-out syntax out of this proposal until T-lang can form consensus around it. And I am happy to incorporate whatever we decide on into the proposal if that is the case. Until then, all I can say is it sucks but that's the best I can do :)

[clarfonthey]

I guess maybe a better question is: would #[hypothetical_never_const] attribute be possible to add to existing const trait definitions without breaking anything? With strictly the stable syntax, I don't think so.

[traviscross]

Looks great. Thanks to @fee1-dead for putting together this draft.

View changes since this review

[traviscross]

How would we interpret the following under these rules?

struct NonConstDestruct;
impl Drop for NonConstDestruct { fn drop(&mut self) { println!(); } }
const fn make() -> NonConstDestruct { NonConstDestruct }
struct S;
const impl S {
    fn call_and_drop<F: FnOnce() -> R, R>(f: F) {
        //                             ^
        //                 Under the rules, we assume this parameter
        //                 type to be const destructible, right?
        let _x: R = f();
    }
}
const C: () = S::call_and_drop(make);
//            ^^^^^^^^^^^^^^^^^^^^^^
//      When do we catch the problem here?

[fee1-dead]

This is a good catch, I think that's a serious limitation of the current approach, and it might just be simpler to use a desugaring approach to infer [const] Destruct on every type.

If we stuck with this approach, it would mean that const fn cannot satisfy FnOnce (we would only provide built in impls for const impl methods)

[traviscross]

Makes sense; thanks.

If we stuck with this approach, it would mean that const fn cannot satisfy FnOnce (we would only provide built in impls for const impl methods)

Another variant:

use core::mem::ManuallyDrop;
struct NonConstDestruct;
impl Drop for NonConstDestruct { fn drop(&mut self) { println!(); } }
const M: ManuallyDrop<NonConstDestruct> = ManuallyDrop::new(NonConstDestruct);
//       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
//     This type implements `const Destruct`.
struct S;
const impl S {
    fn unwrap_and_drop<T>(x: ManuallyDrop<T>) {
        //             ^
        //     Under the rules, we assume this parameter type
        //     is const destructible.
        let _x: T = ManuallyDrop::into_inner(x);
        //core::mem::forget(_x);
    }
}
const C: () = S::unwrap_and_drop(M);
//            ^^^^^^^^^^^^^^^^^^^^^
//           What do we make of this?

[tmandry]

it might just be simpler to use a desugaring approach to infer [const] Destruct on every type.

I understand this RFC's rules as attempting to over-approximate this approach, but it seems like more of an over-approximation than necessary, because I don't know what else we would end up with. [const] Destruct seems like the "obviously correct" way to model it; we just aren't exposing that until we decide how it's spelled.

[fee1-dead]

I think I have been convinced that this part of the proposal is more trouble than it is worth. I'll try to come up with a redesign that is simpler and easier to reason with today or tomorrow, and it will most likely just be desugaring [const] Destruct for all parameter types & projection types in scope.

[fee1-dead]

I've switched to the desugaring approach. Therefore this should be resolved, though definitely worth keeping in mind for the footguns of the original approach.

[tmandry]

After some discussion with @traviscross, I think that if we do this and remove the MIR body check, it should remove the need for #[const_bounds], and const fn can call into const impl again. Is that right? That seems preferable if so.

(Modulo the current generic bounds for const fn not matching up, but those are not useful for const fn today anyway.)

[RalfJung]

The need for const_bounds is still there, to be able to define free const fn with [const] Trait bounds.