-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Make trait methods callable in const contexts #3762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
I love it and I’m very excited to get to replace the outlandish const fn + associated const Thank you for all of the hard work that everyone working on the various implementation prototypes and around has put into const traits! |
I didn’t see const implementations in alternatives. Is there a reason they can’t be considered ? Eg. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
fe1331e
to
ff7fabe
Compare
Co-authored-by: Tim Neumann <[email protected]>
text/0000-const-trait-impls.md
Outdated
which we definitely do not support and have historically rejected over and over again. | ||
|
||
|
||
### `~const Destruct` trait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should just be ~const Drop
? Drop
bounds in their present form are completely useless, so repurposing them would make sense. Drop
would be implemented by every currently existing type, and ~const Drop
only by ones that can be dropped in const
contexts.
(Overall, very impressed by this RFC. It addresses essentially all the concerns I thought I might have going in. Thank you @oli-obk and team for all your hard work!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that would be neat. But it needs an edition and giving the ppl that needed T: Drop
bounds the ability to still do whatever they were doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ppl that needed
T: Drop
bounds
Are there any such people at all? Making more types implement a trait should not be breaking or require an edition, no? Unless there is some useful property (for e.g. unsafe code) that only types that are currently Drop
have—and there isn’t, AFAICT. (Plus, removing an explicit Drop
impl from a type is usually not considered breaking.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's very useful conceptually to split Destruct
and Drop
since the former is structural and the latter really isn't -- it's more like an "OnDrop
" handler. If we moved to ~const Drop
, then in order to write a well-formed ~const Drop
impl, you need to write where {all of my fields}: ~const Drop
in the where clause.
That is to say, there's a very good reason we split ~const Destruct
out of ~const Drop
in the first place :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's more like an "
OnDrop
" handler.
Yeah, that’s what it is right now, but we could expand its meaning.
in order to write a well-formed
~const Drop
impl, you need to writewhere {all of my fields}: ~const Drop
in the where clause.
The bound could always be made implicitly inferred. Drop
is extremely magic already, why not a little more?
But actually, I think it’s a good thing that these bounds can be specified explicitly, because it enables library authors to leave room for adding or changing private fields in the future. I could see allowing impl Drop
/impl const Drop
blocks with no fn drop()
method, that serve only to add restrictions on dropping in const
contexts. (In today’s Rust, you could use a ZST field for this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing how random trait bounds of otherwise typical traits are presented, even over an edition, is not useful. It's not Fn, FnMut, FnOnce, or Sized. The mistake isn't that you can write a Drop bound, it's that Drop was handled by a typical trait, despite having atypical needs, and was not given special treatment to begin with. That is something you cannot simply change over an edition. Otherwise, introducing a magical special case too-late to help is not really for the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@workingjubilee Can you elaborate? To be clear, my suggestion is that Drop
should be like a normal trait (at least in terms of its trait bounds). The “magical special case” I suggested would be only for old editions, to preserve compatibility for the small number of people relying on the current not-like-a-normal-trait behavior (where a type that satisfies the Drop
bound is less capable than one that does not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??? Perhaps I misunderstood something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current Drop
: trait bound satisfied only when an explicit impl
exists. Such an impl
must contain an fn drop()
. Adding such an impl
makes the type less capable.
Proposed Drop
: trait bound always satisfied on new editions (like this RFC’s Destruct
). Bare : Drop
bounds retain their current behavior on old editions (with a warning), for compatibility. ~const Drop
bounds behave like this RFC’s ~const Destruct
on all editions. Conceptually: when implementing Drop
manually, you override the default impl (like with an auto trait). An explicit impl
may specify ~const
bounds, or an fn drop()
handler. Adding such a handler implicitly (a) makes the type ineligible for destructuring, and (b) unimplements auto trait TrivialDrop
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone who has spent much time helping people getting their hands on Rust, I'd like to second this.
I feel like having a Destruct
trait which is used only in this case but is different than Drop
(which you should not use in bounds anyway) is really confusing. This is not something that you can understand without knowing about rustc internals or the language history goes backward to the language great consistency.
Most people just want to know "can I drop this at compile time or not", they don't care if it is because of a manual Drop
impl or drop glue. Drop
is already a special trait, I think we can make it mean something else for ~const
bounds (and maybe change it later for normal bounds).
In short, I think that adding ~const Destruct
instead of ~const Drop
would make the language harder to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I am really excited about this; with &mut
out of the door, traits are the next big frontier for const. I fully agree we shouldn't block this on the async/try effects work; const is quite different since there's no monadic type in the language reifying the effect, and I also don't want to wait another 4 years before const fn can finally use basic language features such as traits.
My main concern is the amount of ~const
people will have to add everywhere. I'm not convinced it's such a bad idea to make that the default mode for const fn
. However that would clearly need an edition migration so it doesn't have to be part of the MVP. I just don't agree with the way the RFC dismisses this alternative.
{ | ||
... | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reference-level section, this seems more understandable than the <T as Default>::k#host = Conditionally
thing above, but maybe that's just because I have already through about the "be generic over constness" formulation quite a bit.
text/0000-const-trait-impls.md
Outdated
impl<T: (const) Default> const Default for Thing<T> { | ||
(const) fn default() -> Self { Self(T::default()) } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over in rust-lang/rust#139858 (comment), there was an ask to start a thread here about this (const) fn
syntax on methods in the impl.
Requiring this (const) fn
syntax in cases like the one above, where the method's constness as implemented necessarily depends on the constness of a generic from the impl header -- and consequently on the associated effect in the desugaring -- is where @nikomatsakis and I had landed back at the start of March. We had discussed it both ways, but we liked this one better in our our discussion. Among other things, we liked how this emphasizes the connection to the associated effect, and this syntax allows for copying signatures from the trait def to the impl.
Niko mentioned this in his write-up here, and I elaborated on this earlier in this thread in #3762 (comment) and in #3762 (comment). Please see those comments, particularly the first, for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I'd a copy paste error. I have never intentionally written this syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just more syntax salt solely for the purpose of some language purity. There is no meaningful difference unless we stop marking impls which would be a mess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just more syntax salt solely for the purpose of some language purity.
No, it is using syntax to convey a conceptual difference: is this function's constness tied to the constness of the trait (and therefore dependent on (const)
bounds in the impl header) or not?
At least, that's what I understood it to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but is this difference actually useful? We need to check whether const fn
refers to a (const) fn
in the trait or a const fn
in the trait anyway. This is just a random thing users will need to learn without actually caring. Only from a language purity this is nice, but practically this has neither an effect on the compiler (beyond being annoying to implement) nor on users' ability to understand the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems quite relevant to me to understand which bounds I have in context. A const fn
cannot use (const) Trait
bounds of the impl block, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it was declared as (const) fn
in the trait it can afaict. Or I still don't understand the TC/niko model so maybe someone else should write this RFC because I'm lost honestly and it seems bad to have someone who dislikes a model (the one allowing const
, (const)
and normal fns in one trait) being the one to write the RFC just to walk into mines like these.
see also my confusion in #3762 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the one allowing const, (const) and normal fns in one trait
Yeah fair, on that one I am with you, I don't think we need that for the MVP -- though we should keep it in mind as a future possibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems quite relevant to me to understand which bounds I have in context. A
const fn
cannot use(const) Trait
bounds of the impl block, right?
If there are no bounds on the impl
block, then this is a distinction without a difference. IMO we should require (const) fn
only when there actually are such bounds.
73e98c4
to
fdd11fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: A lot of the discussion around the language design is happening on Zulip, under the t-lang/effects stream. The latest major proposal discussion can be found at https://rust-lang.zulipchat.com/#narrow/channel/328082-t-lang.2Feffects/topic/All.20that's.20old.20is.20new.20again. It would be nice if before commenting on this RFC one could check if this topic has been discussed on the Zulip stream before :) When there appears to be a consensus forming from community members + the lang team, the proposal should then be updated.
Note: the following issue is marked deprecated (and locked) It says the tracking issue for the rewrite is: but that obviously only tracks the removal of the old version. I don't really know what the feature is supposed to be, but it sorta seems like this RFC should have a new tracking issue, and maybe the old one should link to the new one instead of to the removal tracking issue? |
This RFC will get a new tracking issue once it gets accepted. |
@@ -15,21 +15,21 @@ Make trait methods callable in const contexts. This includes the following parts | |||
Fully contained example ([Playground of currently working example](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2ab8d572c63bcf116b93c632705ddc1b)): | |||
|
|||
```rust | |||
const trait Default { | |||
(const) fn default() -> Self; | |||
[const] trait Default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the jury is still out on this one, as similarly to const fn
there is no benefit of choosing [const] trait
over const trait
, as the latter can't mean anything different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's also true
Most of the time you don't want to write out your impls by hand, but instead derive them as the implementation is obvious from your data structure. | ||
|
||
```rust | ||
#[const_derive(PartialEq, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a typo - you talk below (consistently) about derive_const
, but this is const_derive
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to share this comment I made here: rust-lang/rust#90080 (comment)
Specifically, I'd like to point out that rust-lang/rust#67792 is a currently locked issue still being added as a tracking issue for several things depending on this RFC, with the caveat that "a new issue will be opened when the RFC is accepted."
We all know that this RFC is going to be accepted in some form or another, just, not sure what its final form will be. There are currently plenty of discussions happening in the background behind closed doors that have been influencing implementations (for example, rust-lang/rust#139858 mentions that the lang team decided on a new syntax but there is no link to where that discussion happened), and currently, they are all stuck on this old, locked tracking issue where nobody else is allowed to participate except those directly under the rust-lang org.
This should be resolved properly. Either, there needs to be a consensus that new stuff should not be done with the const traits (syntax, making traits const, etc.), or it should be acknowledged that things are being done under the assumption this RFC will be accepted in some form, and either the old tracking issues should be unlocked, or new ones should be made.
EDIT: I tried to follow your "only threadable discussions", "reminder" (it's not a reminder, it's a request, since this is not how the GitHub UI is designed to be used), but apparently leaving a review without linking it directly to a line in the source doesn't accomplish this. Sorry; feel free to create a new "thread," or just resolve this in a single comment so there's no need to have a thread.
When @oli-obk said, over in rust-lang/rust#139858, that,
he was referring to: In that meeting, we didn't decide on the syntax. But, as Oli said, we need hands-on experience here, and along with the new proposal presented in that meeting and the lack of immediate strong objections, that's enough to move forward the experimental implementation work, as Oli did. The doors aren't closed. Our meetings are open, and we keep extensive minutes that you can find linked from there. |
Nevertheless, having proper tracking issues would be helpful. The current situation of reusing either 5-year old locked issues or 5-year-old closed issues as tracking issues is very confusing. |
Agree of course we should properly track things. In my view, rust-lang/rust#67792 is still the proper tracking issue for this work (as with many of our tracking issues, of course, it could use a lot of love to be brought up to date). I've gone ahead and unlocked it. I'd expect the reason we needed to lock it was temporary and hopefully the need for that has passed. If that's not the case, we can always lock it again. |
I should clarify, I wasn't saying that the information wasn't technically available, just that it wasn't immediately obvious where to look. For example, even among lang team meetings, it's not immediately clear which one it took place in, and the more time that passes between when a decision was made and when the decision is noticed by someone else, the harder it is to find which specific discussion occurred if it's not linked. It's entirely plausible that the discussion happened multiple meetings before the PR and it was only at that point where someone got around to implementing it. This is kind of the reason why tracking issues exist: instead of linking every micro-discussion that happened elsewhere when it comes to important changes with respect to a thing, the unified tracking issue tracks any relevant decisions that happened. Precisely so you don't have to look through a bunch of irrelevant discussions in meeting notes, you can just have a bullet point in the tracking issue pointing out a PR that was made, which mentions in its description that it was after a discussion in a lang team meeting. (Thank you for unlocking the issue, by the way; I'm mostly just adding a bit more context since I'm not 100% sure my point was clear.) |
|
||
### Const trait bounds | ||
|
||
Any item that can have trait bounds can also have `const Trait` bounds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it allowed to write const Trait
or [const] Trait
bounds when Trait
is not declared #[const_trait]
/[const]
?
For example:
trait Foo {}
const fn bar<F>()
where
F: [const] Foo, // <-- Is this an error?
{}
I ask because currently the const_trait_impl
feature on nightly does require #[const_trait]
in order to write const
and [const]
bounds. So currently the example would fail to compile on nightly with something like:
error: `[const] can only be applied to `#[const_trait]` traits
I wasn't clear whether this is the intended behavior, or just an unstable feature being unstable. The quoted sentence here makes it sound to me like the bound should be allowed. But maybe it's ambiguous—it could also be interpreted as only defining where the const
/[const]
bound syntax is permitted, without precluding further restrictions. (Sorry if I missed a more relevant part of the RFC.)
I understand that const Trait
impls require Trait
to be declared [const]
, which helps ensure forward compatibility. But I don't think that concern would carry over to const Trait
and [const] Trait
bounds—they would just be trivially unsatisfiable if a const Trait
impl couldn't exist.
I think allowing this would be very useful to library authors, who could write [const] Trait
bounds on Trait
from external crates, while still supporting older versions of the crate where Trait
isn't declared [const]
yet. Adding a new const fn
in crate A isn't a breaking change; nor is adding [const]
/#[const_trait]
to Trait
in crate B (I think?). But if A had to bump the MSV of its dependency on B to add a new const fn
with a [const] Trait
bound, that would be a breaking change for crate A, which would make it more painful to start using the new feature gradually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence applies to the item the trait bound is applied to, not the trait definition. The currently implemented behavior matches the RFC’s intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also simplify life for macro writers, in a similar way to allow_trivial_constraints
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trait Foo {
fn foo<T: Bar>();
}
Cannot be implemented without the trait being marked, as otherwise you are making a decision for the trait author as to whether that bound is not const or conditionally const
There's lots of semver issues around that, I'm gonna add a section, but TLDR is "not possible for traits with generic methods, associated type bounds or super traits other than Sized"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what they were tasking about though? They were asking about bounds, not impls:
trait Foo { ... }
// Forbidden
impl const Foo for ... { ... }
// Could be allowed?
const fn bar<T: [const] Foo>(x: T) { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisbouchard That’s a good example, thanks! I guess I have to amend my statement: [const]
should be forbidden in the where
clauses of these items. In your example, the [const] Input
bound is not in a where
clause, so it doesn’t break the property I listed earlier, and should be allowed. (I had never really thought about the fact that type Assoc: Supertrait;
and type Assoc where Self::Assoc: Supertrait;
mean different things. Would make a nice question for https://github.com/dtolnay/rust-quiz …)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jules-Bertholet Hmmm. Again, I feel like [const] bounds in associated items' where clauses could be useful, but I haven't fully thought it through yet.
Could you express something like
[const] trait Trait<T> {
type Collection: [const] IntoIterator<Item=T>
where
<Self::Collection as IntoIterator>::IntoIter: [const] Iterator<Item=T>;
without a [const] bound in the where clause? (In a future where the iterator traits are [const].)
At least in this case, I think you could replace it with a second associated type
[const] trait Trait<T> {
type Iter: [const] Iterator<Item=T>;
type Collection: [const] IntoIterator<Item=T, IntoIter=Self::Iter>;
at the expense of a second associated type, which forces impls to spell out the iterator type.
(I suppose in this const iterator future IntoIterator::IntoIter
may already be [const] Iterator
, but not necessarily. E.g., you could construct an iterator at compile time that can only be iterated at runtime.)
I do feel like I see the outline of your concern, but I don't think I understand it fully. Could you share an example where a [const] bound in an item where clause makes const Trait
surprisingly weaker than Trait
? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you express something like
[const] trait Trait<T> { type Collection: [const] IntoIterator<Item=T> where <Self::Collection as IntoIterator>::IntoIter: [const] Iterator<Item=T>; }without a [const] bound in the where clause?
Yes. Just move the where
clause up:
[const] trait Trait<T>
where
<Self::Collection as IntoIterator>::IntoIter: [const] Iterator<Item=T>,
{
type Collection: [const] IntoIterator<Item=T>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you express something like
[const] trait Trait<T> { type Collection: [const] IntoIterator<Item=T> where <Self::Collection as IntoIterator>::IntoIter: [const] Iterator<Item=T>;without a [const] bound in the where clause?
or you could use the following, but it's not as general:
[const] trait Trait<T> {
type Collection: [const] IntoIterator<Item = T, IntoIter: [const] Iterator<Item = T>>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The place to look for what would become inexpressible here are associated functions (including e.g. RPITIT ones) with these [const]
WC bounds.
|
||
### Conditionally const traits methods | ||
|
||
Traits need to opt-in to allowing their impls to have const methods. Thus you need to mark the trait as `[const]` and all the methods will become const callable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to understand why this opt-in is needed, so here is a rewording suggestion that will make it more obvious:
Traits need to opt-in to allowing their impls to have const methods. Thus you need to mark the trait as `[const]` and all the methods will become const callable. | |
Making a trait's methods callable in const contexts imposes extra constraints on their default implementations (if any). | |
Therefore, traits need to opt-in to allowing their impls to have const methods. | |
You need to mark the trait as `[const]` and all the methods will become const callable. |
I don't know if this is open for discussion anymore, but I just wanted to say that I find the syntax using square brackets in I much preferred the old |
I was playing around with this today. And I noticed a potential inconsistency. In trait definitions, you have this theoretical syntax: const unsafe trait Foo { ... } But in implementation, you have this theoretical syntax: unsafe impl const Foo ... Perhaps it should be this? const unsafe impl Foo ... I suspect |
@npmccallum There is the precedent of But then there's still the issue that the RFC proposes |
$ rustc --crate-type lib - <<EOF
const unsafe fn const_first() {}
unsafe const fn unsafe_first() {}
EOF
error: expected one of `extern` or `fn`, found keyword `const`
--> <anon>:2:8
|
2 | unsafe const fn unsafe_first() {}
| -------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `const` must come before `unsafe`: `const unsafe`
|
= note: keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern`
error: aborting due to 1 previous error |
Also, rust-lang/rust#143879 puts To be clear, I don't care about the ordering of |
In light of that, from what I can tell the syntax needs to be either:
to be self-consistent. 2 is impossible because the existing syntax is |
|
Regardless, I think it's worth mentioning that there should be parser recovery and proper linting to fix the keyword order here if people mess it up, like we have for ordinary functions. |
Please remember to create inline comments for discussions to keep this RFC manageable and discussion trees resolveable.
Rendered
Related:
impl const Trait for Ty
and[const]
(conditionally const) syntax (const_trait_impl
) rust#67792