rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.86k stars 12.67k forks source link

Tracking issue for future-incompatibility lint `unstable_name_collisions` #48919

Open kennytm opened 6 years ago

kennytm commented 6 years ago

This is the summary issue for the unstable_name_collisions future-incompatibility lint and other related errors. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is the warning for?

Rust's standard library is evolving, and will sometimes add new functions we found useful, e.g. Ord::clamp or Iterator::flatten. Unfortunately, there will often be popular third-party packages already implemented the same function with the same name. The compiler, when seeing the name flatten, cannot decide whether you want the one from the standard library or the third-party library, and gives up with the multiple applicable items in scope error.

As an inference breakage canary, the compiler will emit a warning whenever it detected a name conflict with an unstable standard library function.

extern crate itertools;
use itertools::Itertools;

fn main() {
    println!(
        "{:?}",
        [[1, 2], [3, 4]].iter().flatten().collect::<Vec<_>>()
        //~^ WARN a method with this name will be added to the standard library in the future
    );
}

To stop this warning and the future hard error, you could use fully-qualified name to explicitly tell the compiler to use the third-party function:

extern crate itertools;
use itertools::Itertools;

fn main() {
    println!(
        "{:?}",
        Itertools::flatten([[1, 2], [3, 4]].iter()).collect::<Vec<_>>()
//      ^~~~~~~~~~~~~~~~~~~.......................~
//      explicitly use the `flatten` method from the `Itertools` trait.
    );
}

When will this warning become a hard error?

This warning will be emitted for every unstable library feature having name conflicts. When a particular library feature is stabilized, this name conflict will cause a hard error.

Please follow the tracking issue for each library feature for their stabilization progress. Some current unstable method which is known to cause conflict in the wild are:

infinity0 commented 6 years ago

In the case of Itertools::flatten would it not be better to just ignore the warning and wait for it to be added to the standard library and then removed from Itertools? Presumably the two functions do the same thing, so eventually everyone does want to migrate to the std version, to reduce cruft.

kennytm commented 6 years ago

@infinity0 .flatten has already been stabilized and will be available in 1.29...

bstrie commented 4 years ago

Given that the lint definition itself says:

        // Note: this item represents future incompatibility of all unstable functions in the
        //       standard library, and thus should never be removed or changed to an error.

it sort of feels like having a tracking issue is somewhat misleading... Should this issue be closed, or is it expected that this issue will instead be permanently open?

davepacheco commented 3 years ago

This warning will be emitted for every unstable library feature having name conflicts. When a particular library feature is stabilized, this name conflict will cause a hard error.

Based on this, I expected that if I create a trait with a method whose name conflicts with a feature-flagged method in std, and I enable that feature, then I would also get a hard error. That doesn't seem to be the case: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018

@steveklabnik says this might be because the example in this issue text above involves methods with the same name in two different traits, while mine involves a trait method name and an inherent method name. It might be useful to mention this case in the text above, since the compiler points to this issue in both cases and the text here doesn't say there are some cases where you will never get a hard error. (The compiler message does say that you might get a change in behavior or an error.)

Edit 1: Forgot to add: thanks (as always) for the detailed, thoughtful message here in the first place! Edit 2: I also found that the warning is now called unstable_name_collisions, not unstable_name_collision. The compiler tells you this if you use the wrong one, but it might be useful to update this issue's synopsis.

joshtriplett commented 2 years ago

I'd like to propose that we reconsider our current approach to this warning.

I just committed the following code to one of my projects:

// Ignore the warnings about div_ceil and next_multiple_of, to avoid rewriting code twice, once
// to use num_integer explicitly and a second time to use the methods from std.
#![allow(unstable_name_collisions)]

I hope that https://github.com/rust-lang/rfcs/pull/3240 will provide a better solution to this problem. Once that RFC is merged, I'd like to propose that we drop this lint.

your-diary commented 1 year ago

Is it possible to suppress this warning only for some specified method?

Pseudo code:

#![allow(unstable_name_collisions(target = num::Integer::div_ceil))]

If not, I think this warning is too broad.

VorpalBlade commented 1 year ago

The lint suggests calling this with the full method name. It isn't clear to me how to do that in a chained call while keeping the chained syntax. E.g.

myIter.filter(...).intersperse(...).map(...).collect(...)

Here the warning is about intersperse (Itertools vs unstable standard library). How would I go about calling it as itertools::Itertools::intersperse(...) (which the warning suggests) without adding an arbitrary let temp = step? I.e. keeping it as a single chain?

If that is not possible, this seems like a major deficiency in the recommendation from this lint.

your-diary commented 1 year ago

@VorpalBlade

Try

itertools::Itertools::intersperse(myIter.filter(...), &item).map(...).collect(...)

This is known as fully qualified syntax.

VorpalBlade commented 1 year ago

@VorpalBlade

Try

itertools::Itertools::intersperse(myIter.filter(...), &item).map(...).collect(...)

This is known as fully qualified syntax.

Thanks, this works but reverses the reading order in the middle of the expression, which is suboptimal. While the solution with a temporary variable keeps the order, it introduces clutter. Neither approach is optimal.

tv42 commented 1 year ago

I would really like to be able to opt-out of this per colliding method.

I want to use Result::inspect_err without needing a nightly build, and thus ended up with https://github.com/matthiasbeyer/result-inspect and now I'm getting repeat warnings because of that. When Result::inspect_err is stable, I'll happily throw away the dependency, but until that time it seems I can't use #[warn(future_incompatible)] without huge pain.

ChrisJefferson commented 1 year ago

I came here from a warning in my code. I was told to replace div_floor with num_integer::Integer::div_floor.

Except, if I do that, I then get a compile error. Looking at higher-up issues I can see I'm "doing it wrong", but I do think if possible it would be better to improve the error message to link somewhere how to actually fix your code, as the error doing a simple replace isn't helpful at all:

error: expected one of `(`, `.`, `;`, `?`, `}`, or an operator, found `::`
   --> src/constraint_def.rs:498:44
    |
498 |                         v[0][0].num_integer::Integer::div_floor(&v[1][0])
    |                                            ^^ expected one of `(`, `.`, `;`, `?`, `}`, or an operator
ChrisJefferson commented 1 year ago

Suggestion: Could clippy's --fix framework be used to automatically fix code with this issue? I'm not sure of the right way to do this, could the same issue also be added to clippy, or clippy automatically pick up rewrite the compiler wants?

withoutboats commented 7 months ago

(NOT A CONTRIBUTION)

I was shown this lint for calling Range::is_empty on &mut Range<usize>. I'm guessing this is a conflict with an API that may be added to Iterator (which would take a mutable reference and therefore shadow the shared reference inherent method? I'm not 100% sure the method resolution rules in a case like that).

I'm not sure if this is the case because nothing links me to the unstable API that I am in a collision with.

Also, the lint suggested I change my code to std::ops::Range<Idx>, even though Range was in scope and the type was Range<usize>.

I found this to be a pretty bad user experience. I was given a lint that I have no realistic choice but to suppress somehow, which the lint provides wrong advice for how to do. This resulted from calling a method in std, not even using a third party crate. There was no way to reference to the change of behavior that may come, though I assume it will not be relevant to me, because I assume in the case of Range either implementation of is_empty will behave the same.

ijackson commented 2 months ago

IMO this lint ought to be withdrawn, and instead the Rust project should commit to finding a better way to deal with this situation. There have been RFCs proposed, including https://github.com/rust-lang/rfcs/pull/3240

The current compiler behaviour is to make complaints with no good solution. (Using the fully qualified syntax is not a good solution.) This keeps coming up in projects I work on and I'm finding myself adding #[allow] for it. I'm sure this is the case for others too.

Ie, this lint is not meeting its objective, of allowing std to implement these methods without breaking downstream code.

kennytm commented 2 months ago

@ijackson That's the point of rust-lang/rfcs#3624