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

Allow marker traits as additional traits on trait objects. #89061

Closed
wants to merge 1 commit into from

Conversation

beepster4096
Copy link
Contributor

@beepster4096 beepster4096 commented Sep 18, 2021

Currently, only auto traits are allowed to be extra traits on trait objects because they are guaranteed to have no functions. However, there is another type of trait that has this property: marker traits. It seems like an oversight that this is not allowed.

This PR fixes this.

Work left to do:

  • See if this change is enough to make it work
  • Change error message

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Sep 18, 2021
@beepster4096 beepster4096 marked this pull request as draft September 18, 2021 06:01
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@beepster4096 beepster4096 marked this pull request as ready for review September 18, 2021 18:27
@beepster4096 beepster4096 changed the title [WIP] Allow marker traits as additional traits on trait objects. Allow marker traits as additional traits on trait objects. Sep 18, 2021
@beepster4096
Copy link
Contributor Author

Tests are passing, so this is ready for review

@oli-obk
Copy link
Contributor

oli-obk commented Sep 20, 2021

@bors r+

Makes sense to me and impl looks good.

@bors
Copy link
Contributor

bors commented Sep 20, 2021

📌 Commit 7f03b70f471cd51466b6c6fc45bab58158970ffb 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 Sep 20, 2021
@jackh726
Copy link
Member

Is this something we want to feature gate? Maybe not because marker traits are unstable anyways...

@beepster4096
Copy link
Contributor Author

Technically, a crate could use this without feature flags by depending on a crate with a marker trait in it. I doubt this will cause problems however.

@Mark-Simulacrum
Copy link
Member

I think at least we should cc @rust-lang/lang for awareness. I was going to suggest noting this on the tracking issue for marker traits, but that doesn't seem to exist -- I guess the other thing would be the tracking issue for "multi trait trait objects"? I think @nikomatsakis you were working with someone on that, maybe in wg-traits context?

@Mark-Simulacrum
Copy link
Member

@bors r-

Can we squash the commits here, please? I was going to do it for you, but it looks like that may be disabled on your fork/this PR?

@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 Sep 21, 2021
@nikomatsakis
Copy link
Contributor

I agree with cc'ing lang on this =)

@cramertj
Copy link
Member

What does is_marker currently mean here? Is it only specifically tagged traits, or is it all traits that have no methods? I remember discussing similar proposals before, and at the time I was concerned that "marker trait" wasn't a clear pre-existing user-visible term. I also am worried about the idea of making it a breaking change to add a single defaulted method to a trait that previously had no methods.

@scottmcm
Copy link
Member

Assuming this is based on https://doc.rust-lang.org/nightly/unstable-book/language-features/marker-trait-attr.html (not just the trait being empty) then it seems plausible to me. The big question I'd have is around marker traits with supertraits -- it might need to be "#[marker] traits where all their supertraits are also #[marker]" or something.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2021
@beepster4096
Copy link
Contributor Author

That's a good point I didn't consider. This code doesn't check super traits because auto traits cannot have any.

@nikomatsakis
Copy link
Contributor

What does is_marker currently mean here?

I believe @cramertj that marker traits currently require a #[marker] annotation (@scottmcm made this change in #53693).

@nikomatsakis
Copy link
Contributor

I'm comfortable overall with this change, but I'm a bit nervous about exposing the word "marker trait" to end users in diagnostics.

@oli-obk oli-obk 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon
Copy link
Member

I'm comfortable overall with this change, but I'm a bit nervous about exposing the word "marker trait" to end users in diagnostics.

ping from triage:
@drmeepster can you address this comment?
Please adjust the label with @rustbot ready when you're ready for another review

@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2021

Error: Parsing shortcut command in comment failed: ...'tbot ready' | error: expected end of command at >| ' when you''...

Please let @rust-lang/release know if you're having trouble with this bot.

@JohnCSimon
Copy link
Member

ping again from triage:
@drmeepster can you address this comment?

@beepster4096
Copy link
Contributor Author

Can't seem to figure out how to do this right, and not interested in spending any more time trying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.