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

Experiment: mark derived Clone impls as const #93255

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jan 24, 2022

This is an attempt to lay the foundation for marking #[derive]d trait impls as const. Right now, only trivial Clone impls (*self) are marked as const, but in the future, more complicated impls like PartialEq and PartialOrd could be marked as const as well.

This mostly just modifies the TraitDef struct in rustc_builtin_macros to allow marking traits as const. For now, only the Clone implementations are marked as const since they're the simplest, to prove that the feature works.

This also augments the existing Clone derive test to check that the generated impls can be const.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 24, 2022
@rust-highfive
Copy link
Contributor

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

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

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

Even though it currently builds and the test passes, I'm going to leave this marked as a draft since I think it still could use some feedback in terms of how it's done (this is my first time changing any compiler code) and also just, the way it works in general.

@estebank
Copy link
Contributor

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Jan 26, 2022
#[default_method_body_is_const]
fn clone_from(&mut self, source: &Self)
where
Self: ~const Clone + ~const Drop,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this show up in rustdoc?

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem with this approach is that it is a breaking change for

struct Foo;
impl Clone for Foo {
    fn clone(&self) -> Self { Foo }
}
#[derive(Clone)]
struct Bar(Foo);

Thus the only way we can ever really make derives work, is by opting in, meaning something like #[derive(const Clone)]. This has been discussed in https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/syntax.2Fgrammar.3A.20deriving.20const.20traits

The discussion there isn't yet clear on the syntax, so I'm not sure how to proceed here.

@clarfonthey
Copy link
Contributor Author

That's very fair; I think that the #[derive(const Clone)] syntax would be simple enough to implement that it could be changed later once a proper syntax is decided, but I also would understand if the preference is to not do that.

As you probably noticed in the code itself, what is probably a bigger concern is how to annotate the derived Clone impls in the standard library specifically, since there's no proper way to annotate them with separate rustc_const_unstable features. Not really sure if the current approach is okay, and I don't think it's acceptable to force them all to be manually implemented if a separate attribute is desired.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

(Will properly rebase later; am glad the error messages improved.)

@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Mar 2, 2022
@bors
Copy link
Collaborator

bors commented Mar 10, 2022

☔ The latest upstream changes (presumably #94787) made this pull request unmergeable. Please resolve the merge conflicts.

@clarfonthey
Copy link
Contributor Author

Gonna close since this needs a redesign anyway.

@clarfonthey clarfonthey deleted the derive_const branch March 27, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants