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

Add: orphan rule rationale. #1755

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TmSalviano
Copy link

@TmSalviano TmSalviano commented Mar 11, 2025

  1. I thought it would be helpful to introduce the reader to the problem the orphan rule solves before being exposed to the rule itself. Understanding the orphan rule for the first time can be challenging, and knowing why it exists beforehand can make it easier to grasp.
  2. Also, including an explanation of the rationale behind the orphan rule in the orphan rule section of the page just makes sense.

@rustbot rustbot added the S-waiting-on-review Status: The marked PR is awaiting review from a maintainer label Mar 11, 2025
Comment on lines +183 to +184
implementation is local to your crate, ensuring only one crate defines the implementation and
thereby maintaining coherence.

Choose a reason for hiding this comment

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

I would personally put a “that” here:

Suggested change
implementation is local to your crate, ensuring only one crate defines the implementation and
thereby maintaining coherence.
implementation is local to your crate, ensuring that only one crate defines the implementation
and thereby maintaining coherence.

Comment on lines +177 to +184
r[items.impl.trait.orphan-rule.rationale]
The orphan rule helps ensure that other people's code can't break your code, and vice versa.
If an external crate implements an external trait for an external type, and your crate also
implements the same trait for the same type, the compiler wouldn't know which implementation
to use.\
The orphan rule prevents this by requiring that either the trait or some type in the
implementation is local to your crate, ensuring only one crate defines the implementation and
thereby maintaining coherence.
Copy link
Contributor

@traviscross traviscross Mar 20, 2025

Choose a reason for hiding this comment

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

Probably we'll want to move this into a note section, but it's generally going in the right direction. The one caveat I'd add is that the problem being prevented here isn't really about the compiler not knowing what to pick. The compiler knows what to do about that. It'd throw an ambiguity error. But what that means is that adding an impl for your own types or of your own traits would be breaking (in many more situations). So it's mostly about saving space for such authors, and that's the real rationale. Of course, whenever you talk about removing the rule, you do find out all the ways that we are leaning on the rule more technically, and those reasons are a bit more subtle. E.g.:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: The marked PR is awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants