-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
08ee85b
to
d3f2583
Compare
Tests are passing, so this is ready for review |
@bors r+ Makes sense to me and impl looks good. |
📌 Commit 7f03b70f471cd51466b6c6fc45bab58158970ffb has been approved by |
Is this something we want to feature gate? Maybe not because marker traits are unstable anyways... |
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. |
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? |
@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? |
7f03b70
to
74d3d70
Compare
I agree with cc'ing lang on this =) |
What does |
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 " |
That's a good point I didn't consider. This code doesn't check super traits because auto traits cannot have any. |
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: |
Error: Parsing shortcut command in comment failed: ...'tbot ready' | error: expected end of command at >| ' when you''... Please let |
ping again from triage: |
ebd1ba0
to
333be81
Compare
Can't seem to figure out how to do this right, and not interested in spending any more time trying. |
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: