Skip to content
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

Mark defaulted PartialEq/PartialOrd methods as const #91439

Merged
merged 3 commits into from
Dec 18, 2021

Conversation

ecstatic-morse
Copy link
Contributor

WIthout it, const impls of these traits are unpleasant to write. I think this kind of change is allowed now. although it looks like it might require some Miri tweaks. Let's find out.

r? @fee1-dead

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021
@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 2, 2021
@fee1-dead
Copy link
Member

cc @oli-obk per https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/default_method_body_is_const

Overview of #[default_method_body_is_const]

This attribute is currently a placeholder. When the const_trait_impl feature gets stabilized it should be changed to something else (preferably new syntax). When marked on a trait method with a default body, we perform checks on it as if it were a const function. This allows users to have traits that are easier to write const impls for.

trait PartialEq {
    fn eq(&self, other: &Self) -> bool;

    #[default_method_body_is_const]
    fn ne(&self, other: &Self) -> bool {
        !self.eq(other) // call to self.eq is allowed in const context because `eq` has a const implementation
    }
}

Note that we cannot use const fn as an indicator for default_method_body_is_const, because it can contradict with future extensions such as const trait fns which always require implementations to be const.

@oli-obk oli-obk added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 2, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Dec 2, 2021

Nominating for T-lang discussion.

We want to experimentally and unstably use this never-to-be-stabilized attribute. I'm assuming we're going to add something (e.g. partial or default impls) in the future that allows us to remove the attribute as you could mark such impls as const then. In any case, this is all unstable and reversible until we stabilize the ability for users to call or write const trait impls

@ecstatic-morse
Copy link
Contributor Author

Ah, I didn't think this was novel. My bad. This came up as part of #91386, and it's basically the most obvious use-case for #[default_method_body_is_const], so it's a fine place to start.

I don't know what's causing miri to fail though. I'm guessing it's not correctly resolving the method to the defaulted one?

@Mark-Simulacrum
Copy link
Member

@oli-obk Could you expand a little on the ask from T-lang? " In any case, this is all unstable and reversible until we stabilize the ability for users to call or write const trait impls" makes it sound like there is no risk to this PR, so it's not clear that a discussion is necessary -- if it is, I feel like a concrete question to answer would be helpful to make sure that discussion tries to answer it.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2021

The question is: are we going to add something (e.g. partial or default impls) in the future that allow us to remove the attribute as you could then mark such impls as const?

The reason I ask is that we don't really gate this feature, except for the gate that allows you to call const trait impls. Were we to stabilize any const traits that use this feature, we'd lock ourselves into requiring that we'll have such a feature

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting. This sounds reasonable to us, as long as it's documented in the tracking issue for const trait impls as a gating consideration for stabilization (e.g. in the top post of such a tracking issue).

@Mark-Simulacrum Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 14, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2021

The tracking issue of const trait impls (#67792) tracks this in the main issue comment

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 14, 2021

📌 Commit dc18d50 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2021
…ult-methods, r=oli-obk

Mark defaulted `PartialEq`/`PartialOrd` methods as const

WIthout it, `const` impls of these traits are unpleasant to write. I think this kind of change is allowed now. although it looks like it might require some Miri tweaks. Let's find out.

r? `@fee1-dead`
@ecstatic-morse
Copy link
Contributor Author

@bors r-

This is broken, and I still don't know that the bug is.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2021
@matthiaskrgr
Copy link
Member

Failed in a rollup: #91937 (comment)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2021
Fix default_method_body_is_const when used across crates

r? `@oli-obk`

unblocks rust-lang#91439.
@fee1-dead
Copy link
Member

Consider rebasing on top of master since the fix (#92001) has landed.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Dec 17, 2021

📌 Commit 2049287 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2021
…ult-methods, r=oli-obk

Mark defaulted `PartialEq`/`PartialOrd` methods as const

WIthout it, `const` impls of these traits are unpleasant to write. I think this kind of change is allowed now. although it looks like it might require some Miri tweaks. Let's find out.

r? `@fee1-dead`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2021
…ult-methods, r=oli-obk

Mark defaulted `PartialEq`/`PartialOrd` methods as const

WIthout it, `const` impls of these traits are unpleasant to write. I think this kind of change is allowed now. although it looks like it might require some Miri tweaks. Let's find out.

r? ``@fee1-dead``
This was referenced Dec 18, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91439 (Mark defaulted `PartialEq`/`PartialOrd` methods as const)
 - rust-lang#91516 (Improve suggestion to change struct field to &mut)
 - rust-lang#91896 (Remove `in_band_lifetimes` for `rustc_passes`)
 - rust-lang#91909 (:arrow_up: rust-analyzer)
 - rust-lang#91922 (Remove `in_band_lifetimes` from `rustc_mir_dataflow`)
 - rust-lang#92025 (Revert "socket ancillary data implementation for dragonflybsd.")
 - rust-lang#92030 (Update stdlib to the 2021 edition)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 359c88e into rust-lang:master Dec 18, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants