Arbitrary self types v2: no deshadow pre feature.

[adetaylor]

The arbitrary self types v2 work introduces a check for shadowed methods, whereby a method in some "outer" smart pointer type may called in preference to a method in the inner referent. This is bad if the outer pointer adds a method later, as it may change behavior, so we ensure we error in this circumstance.

It was intended that this new shadowing detection system only comes into play for users who enable the arbitrary_self_types feature (or of course everyone later if it's stabilized). It was believed that the new deshadowing code couldn't be reached without building the custom smart pointers that arbitrary_self_types enables, and therefore there was no risk of this code impacting existing users.

However, it turns out that cunning use of Pin::get_ref can cause this type of shadowing error to be emitted now. This commit adds a test for this case.

As we want this test to pass without arbitrary_self_types, but fail with it, I've split it into two files (one with run-pass and one without). If there's a better way I can amend it.

Part of #44874

r? @wesleywiser

[wesleywiser]

I don't want to hold up this PR because adding the feature gate check is absolutely necessary as the other test case shows, but what is the plan for stabilizing the feature with regards to this check?

We can't just report this error when the feature is stabilized because that could break existing code. If we need to log an unresolved question for this, that's fine!

[adetaylor]

Good question.

Existing code which could break must fit these criteria:

• It uses an existing standard library smart pointer type - Box, Pin, Rc or Arc.
• The smart pointer type must have an inherent method (not just an associated function)
• Case A:
  • That standard library method must take self by value
  • The referent must have an identically named method which takes self: &SmartPointerType<Self> or self: &mut SmartPointerType<Self>
• Case B:
  • That standard library method must take &self
  • The referent must have an identically named method which takes self: &mut SmartPointerType<Self>

Standard library methods which might cause these problems:

• Case B: Pin::as_ref
• Case A: Pin::map_unchecked
• Case A: Pin::get_ref
• Case A: Pin::into_ref
• Case A: Pin::get_unchecked_mut
• Case A: Pin::map_unchecked_mut
• There are no cases in Rc, Box or Arc

So folks will get broken if, and only if, they have types with any of these methods:

fn as_ref(self: &Pin<Self>)
fn as_ref(self: &mut Pin<Self>)
fn map_unchecked(self: &mut Pin<Self>)
fn get_ref(self: &mut Pin<Self>)
fn into_ref(self: &mut Pin<Self>)
fn get_unchecked_mut(self: &mut Pin<Self>)
fn map_unchecked_mut(self: &mut Pin<Self>)

But here's the thing. As of now, I don't believe there would be any way to call those methods. Because method resolution would always pick the method on Pin rather than the inner method. You could call these methods through UFCS, but in practice I don't think anyone will have added these methods to any types, because aside from calling them using UFCS, it's completely pointless to add these methods.

Therefore - I think in practice we won't break anyone.

But, I did want us to consider this fully and decide if it's OK to take this theoretical risk during stabilization instead of now, hence raising this PR.

[compiler-errors]

I think it's worthwhile to note this in the stabilization report as something that will begin to be triggered once this is stabilized, though. I'll link it.

[wesleywiser]

Thanks for the detailed explanation @adetaylor! I agree it seems somewhat unlikely that we would break anyone but as Michael said, we should include this in the stabilization report when it comes time for that.

[adetaylor]

@traviscross points out that this problem is even smaller than the discussion above suggests, and probably non-existent.

He pointed out that Pin<Self> does not implement Deref unless Self is further constrained, and thus these method signatures would normally have been invalid:

fn as_ref(self: &Pin<Self>)
fn as_ref(self: &mut Pin<Self>)
fn map_unchecked(self: &mut Pin<Self>)
fn get_ref(self: &mut Pin<Self>)
fn into_ref(self: &mut Pin<Self>)
fn get_unchecked_mut(self: &mut Pin<Self>)
fn map_unchecked_mut(self: &mut Pin<Self>)

It's possible that somebody might have found a way to declare these if:

• they're already using the arbitrary_self_types feature gate; and
• Self: core::ops::Deref<Target=Self>. Only in this case would &mut Pin<Self> be a valid receiver type, otherwise these method declarations are rejected with invalid self parameter type. Such a Deref would be recursive, which would likely result in an error, and in any case I think it would only come into force if the self type were &mut Pin<&Self> rather than &mut Pin<Self>.

In short, I can't find any way to add one of the above methods to a type, even if the arbitrary_self_types gate is enabled, and if such a possibility exists it's some corner case of a corner case.

[compiler-errors]

Otherwise LGTM.

@bors r+ rollup

[bors]

📌 Commit fae7207 has been approved by compiler-errors

It is now in the queue for this repository.