typeck: silence lint for "collision safe" items

[davidtwco]

Requested in #97483 (comment) and on Zulip.

---

When an unstable method exists on a type and the user is invoking a method that would no longer have priority when the unstable method is stabilized, an "unstable name collision" lint is emitted. In the vast majority of circumstances, this is desirable.

However, when adding a new inherent method to the standard library, which deliberately shadows the name of a method in the Deref target, then this lint can be triggered, affecting users on stable who don't even know about the new unstable inherent method. As the new method is being added to the standard library, by the library team, it can be known that the lint isn't necessary, as the library team can ensure that the new inherent method has the same behaviour and won't cause any problems for users when it is stabilized.

pub struct Foo;
pub struct Bar;

impl std::ops::Deref for Foo {
    type Target = Bar;
    fn deref(&self) -> &Self::Target { &Bar }
}

impl Foo {
    #[unstable(feature = "new_feature", issue = "none", collision_safe)]
    pub fn example(&self) -> u32 { 4 }
}

impl Bar {
    #[stable(feature = "old_feature", since = "1.0.0")]
    pub fn example(&self) -> u32 { 3 }
}

// ..in another crate..
fn main() {
    let foo = Foo;
    assert_eq!(foo.example(), 3);
    // still invokes `Bar`'s `example`, as the `new_feature` isn't enabled, but doesn't
    // trigger a name collision lint (in practice, both `example` functions should
    // have identical behaviour)
}

Without this addition, the new inherent method would need to be insta-stable in order to avoid breaking stable users.

---

There wasn't a explicit solution described by the members of the library team (like in #99212) in any of the places that this was discussed, so I've done what seemed sensible to me. collision_safe needs to be used carefully, but I believe it solves the problem described without having any undesirable consequences. If there are alternative solutions then I'm happy to implement those instead.

cc @yaahc @rust-lang/libs-api

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

@bors r+

[bors]

📌 Commit ca5a3a4a723cfb49156cec2282e3512fdb0267bf has been approved by petrochenkov

It is now in the queue for this repository.

[davidtwco]

Thanks @petrochenkov, I'd like to wait until someone from t-libs has a chance to sign-off on this being a desirable approach for them before we approve it :)

@bors r-

[davidtwco]

Most recent update from t-libs appears to be (cc @m-ou-se):

• rust.tf/99898 typeck: silence lint for “collision safe” items
  Could cause subtle issues when someone forgets to enable #![feature(..)] and they silently get the stable version, which might be more strict w.r.t. soundness.
  Mara to comment.
  source

I think that there are always going to be potential risks with silencing this lint, so it might not be desirable. I've tried to allievate the potential downsides as much as possible by limiting this to only when the library team has annotated the new unstable item (doing so when knowing which items it would shadow in std once stabilized) and when the item that is selected without the feature flag is from std (so user code that would be shadowed will always get a lint just like without this). That's still risky but hopefully it could be used sparingly and carefully, as in #97483. If there's another solution that the library team would like me to explore then I'm happy to do so.

[m-ou-se]

Apologies, I should've replied sooner.

As mentioned in the meeting notes you quoted, I'm a bit worried about users silently getting a different version of a method if they enable/disable a #![feature].

It seems to me that if it really doesn't matter which one they get, it'd be fine to insta-stabilize the new one, which removes the need for this PR. If it does matter which one of the two equally named methods is called, for example because one more quickly results in UB than the other (like in #97483) then we probably want to be careful and not make it easy to get the wrong one by forgetting to enable a feature.

[davidtwco]

If it does matter which one of the two equally named methods is called, for example because one more quickly results in UB than the other (like in #97483) then we probably want to be careful and not make it easy to get the wrong one by forgetting to enable a feature.

This makes sense to me. I guess I'm not entirely sure what the next steps are here. #97483 is an example where it does matter which method is called (as per the quoted comment), but also where you want to be able to introduce the new methods unstably (as per #97483 (comment)).

This pull request makes introducing the new methods unstably possible - but also introduces the possibility of enabling a feature flag and more quickly having UB or some other such difference in behaviour because a different method will be called. Given the nature of the lint being silenced, while there may be other ways this could be implemented which change how t-libs-api indicate the lint should be silenced, I can't see a way to enable unstably merging functions like these that doesn't also come with the associated risk that this approach does (though it's worth noting that the risk for any given pair of functions should be entirely predictable for t-libs-api).

Regardless, feel free to close this if you don't want the change or r=petrochenkov if you do, it's only here to enable merging PRs like #97483 if you want it :)

[rustbot]

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

[bors]

☔ The latest upstream changes (presumably #103042) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

Not sure what is the status here.

This change complicates method resolution logic and there's no obvious enthusiasm towards landing this, from what I see in this thread, so maybe it should be closed.
@rustbot author

[davidtwco]

I'm happy to close this, doesn't seem like there was interest in it.