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] disable orphan check for marker traits #96766

Closed
wants to merge 1 commit into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 6, 2022

closes #67919, either by landing this or by officially deciding that we don't want this.
I very much prefer to not land this change.

The reasoning for this was given by @Centril in #67919

In this specific case the user has marked the trait as #[marker] (and therefore it has no computational content and overlap is vacuously sound), so it seems to me that their intent was to allow this. Indeed, the reason I found this problem was that I wanted to move rustc::hir::ArenaAllocatable to rustc_arena, but couldn't because of the orphan rule.

The original RFC from 2015 seems to imply that the orphan rules should be relaxed, but it was generally light on details. The impact this has on coherence was not mentioned there.

As downstream crates can now implement marker traits for arbitrary types, all MyType: MarkerTrait bounds have to either hold or are ambiguous. We can never prove that T: MarkerTrait does not hold. This prevents the snippet in the forbidden-fruit test.

While I am completely forbidding negative impls for marker traits here, we could allow them in the crate defining the marker trait. They still would be forbidden for impls of foreign marker traits for local types though.

cc #68004 (comment)

Mostly opened this to get a general consensus from @rust-lang/wg-traits that this is something we do not want ^^

r? @ghost

@lcnr lcnr added the F-marker_trait_attr `#![feature(marker_trait_attr)]` label May 6, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 6, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 6, 2022

This seems very wrong to me from a user perspective. I would certainly not expect anyone but my crate to be able to implement a trait for a type of my crate, unless they defined they trait. I could have functions exposed in my crate that specifically require the marker trait to exist and then not implemented it for some local types, expecting them to never get passed to said functions.

Of course I can emulate that with regular trait hierarchies, but that's annoying

@joshtriplett
Copy link
Member

I do think this needs some further evaluation.

In @rust-lang/lang we've talked about possibilities like allowing duplicate implementations of a third-party trait for a third-party type, as long as the implementations were identical (e.g. created by a standalone derive, or implemented using only default method implementations, etc).

Marker traits do create some cases where I could imagine not wanting that.

@scottmcm
Copy link
Member

scottmcm commented May 7, 2022

I'm strongly against disabling the orphan check here, because it mixes poorly with negative impls.

If any random crate can have impl Marker for MyType, that means that it's breaking for me to impl !Marker for MyType, which I don't think is a restriction that we should force on crate authors, not even for #[marker] traits. That's especially true with rust-lang/negative-impls-initiative#1, where the crate author might need to add the negative impl to be able to do some other blanket impls.

(That's particularly true for impl foo_crate::Marker for foo_crate::Type. If the marker trait is in the same crate as the type, I see absolutely no reason that a different crate should be allowed to impl that trait for that type. I can understand the desire when neither the trait nor the type is a parent crate of the other, and thus there's no obvious place to just write it, but it just seems fundamentally wrong to me to allow something that should be a PR to the crate that defines both.)

While I am completely forbidding negative impls for marker traits here

I think this is all the more reason to not do this. I think impl !Copy for Blah is a great thing to be able to do -- we could even make it implied by impl Drop for Blah -- and I'd like to be able to make Copy into a #[marker] trait.

@joshtriplett
Copy link
Member

@scottmcm I agree with you that the semantics of marker traits mean we likely shouldn't do this.

How would you reconcile that with the general idea of being able to merge duplicate trait impls from multiple crates, which does seem like a positive for non-marker traits?

@scottmcm
Copy link
Member

scottmcm commented May 7, 2022

@joshtriplett As I said in the meeting where I first heard about it, I'm skeptical about that idea in general, since it means that every impl addition ever is a potentially-breaking change.

If it's an extra opt-in on the trait to allow that, then #[marker] traits could orthogonally also choose to opt-in to that behaviour. (And if it's going to work at all, I think it'd be good for that opt-in to, say, also point to the One True structural implementation generator that's allowed to generate such impls, so that they actually have a hope of coming out matching, rather than just conflicting wildly everywhere.)

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 7, 2022
@scottmcm
Copy link
Member

scottmcm commented May 7, 2022

You know what, I feel strongly enough about this that I'm going to put forward a motion.

@rfcbot fcp close

@lcnr mentions in the OP that they'd prefer not to make this change, and I agree. #[marker] is about traits where it's ok to disable the overlap rules. If people want an orthogonal feature to disable the orphan rules, that's great, but I see no reason it has to be coupled to #[marker] traits specifically. (And if, hypothetically, said feature had the "it's ok if they have the same implementation" rule, that sounds to me like it specifically wants to work on non-marker traits too.)

So let's take the opportunity here to, as the OP said, "officially decid[e] that we don't want this".

(I have this as a T-lang FCP because I don't think the bot can do a traits team pFCP, but if anyone on @rust-lang/wg-traits wants to put a concern on here, ping me and I'll add one on your behalf.)

@rfcbot
Copy link

rfcbot commented May 7, 2022

Team member @scottmcm has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels May 7, 2022
@joshtriplett
Copy link
Member

joshtriplett commented May 8, 2022

@scottmcm First of all: completely valid approach.

Second, seeking some clarification: it sounds like you're making the case that, if we do have a mechanism to disable the orphan check, it should be opt-in, rather than opt-out or universal? You're not seeking, at this time, to say that we should never have such a mechanism for this, but you are seeking consensus that we should never have an opt-out or universal mechanism for this?

(Also, I personally would want such an opt-in on the type, not just on the trait.)

@scottmcm
Copy link
Member

scottmcm commented May 8, 2022

I think all I'm saying for the purpose of this issue is that whatever we do shouldn't be tied to the #[marker] attribute specifically.

That's compatible with doing it for everything, with doing it for nothing, with having an opt-out, or with having an opt-in. It's just not saying that #[marker] is an opt-in to the behaviour.

@joshtriplett
Copy link
Member

@scottmcm If we're just aiming for consensus on "this behavior shouldn't be conditional on #[marker]", then I agree.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp reviewed

I wrote up some thoughts here that seem relevant. In short, if we were to remove orphan rules from marker traits, you also couldn't have negative impls for them (or at least, you can't add those negative impls except for a fresh trait or fresh type), because you can't know whether downstream crates may have implemented them. @scottmcm also notes that it may be useful to denote Copy things.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 10, 2022
@rfcbot
Copy link

rfcbot commented May 10, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 10, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 20, 2022
@rfcbot
Copy link

rfcbot commented May 20, 2022

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@lcnr
Copy link
Contributor Author

lcnr commented May 21, 2022

thanks!

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. F-marker_trait_attr `#![feature(marker_trait_attr)]` finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

rustc::traits::orphan_check_trait_ref does not consider TraitDef::is_marker
8 participants