Arbitrary self types v2: shadowing semver break.

[adetaylor]

This is a pre-existing risk of semver breakage for types implementing Deref<Target=T>. It's made more apparent by the arbitrary self types v2 change, and will now generate errors under some circumstances. This PR documents the existing risk and the new ways it can be triggered.

If it's seen as desirable to document the existing risk before Arbitrary Self Types v2 is stabilized, we can split this commit up into two.

Part of rust-lang/rust#44874 Stabilization PR rust-lang/rust#135881

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[adetaylor]

This was requested by @obi1kenobi

[adetaylor]

I'm working a little bit on the code examples here.

@rustbot label -S-waiting-on-review +S-waiting-on-author

[adetaylor]

@rustbot label +S-waiting-on-review -S-waiting-on-author

[workingjubilee]

"must not" is too strong.

This is intentionally done and relied on in a number of cases.

[adetaylor]

Would you agree with the statement if I say:

If you have a type which implements Deref<Target=T> for an arbitrary user-controlled T, you must not add methods which may "shadow" methods in T.

I'm quite keen to give a precise hard rule because it will make it easier for the cargo semver folks, if nothing else.

If you still think that's not a true statement, could you give examples of where this is intentionally done and is OK? Some of these cases will turn into hard errors when/if rust-lang/rust#135881 lands.

[obi1kenobi]

I appreciate the precision in the rule — indeed very helpful.

For cargo-semver-checks purposes, the use of "must not" vs a different verb doesn't matter too much. We'll implement the same lint either way, since the goal of the tool is to notify maintainers that something risky / worth a look has happened and not to tie their hands about it. There are legitimate situations where users may ship known-breaking changes in non-major releases, and that's fine. We look to prevent unknown-breaking changes from happening.

[adetaylor]

@obi1kenobi @workingjubilee I'd appreciate a little more advice here about how to resolve this concern.

[obi1kenobi]

I'm fine with whatever @adetaylor and @workingjubilee decide on. I thoroughly trust your judgment.

[obi1kenobi]

As a possible path forward, in an effort to save everyone else's valuable time:

Suggested change

	If you have a type which implements `Deref<Target=T>`, you must not add methods
	which may "shadow" methods in `T`. This can lead to unexpected changes in
	program behavior.
	If you have a type which implements `Deref<Target=T>`, adding methods to that type
	which may "shadow" methods in `T` can cause breaking changes in program behavior.

This uses factual tone ("the change is breaking") and defers "must/should" level decisions to maintainers' judgment.

[adetaylor]

@obi1kenobi In this comment you say that this example is educational. Just to be explicit, I'm not planning to make changes to this PR based on that example, since I think it's independent of the "arbitrary self types v2" work.