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

Limiting upstream crates may add new impl of trait in future versions #2758

Open
p-avital opened this issue Sep 5, 2019 · 13 comments
Open
Labels
A-trait-coherence Proposals relating to coherence or orphans. T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@p-avital
Copy link

p-avital commented Sep 5, 2019

Hello,

I've run into this one a few times now when working with generics, and I haven't really found a way to bypass it in most cases.

I completely get why this is here, "be careful, your behaviour may change if you update your crates, it may even stop compiling altogether" seems like a fair thing to warn against.

BUT: most of the time, when this error appears, it's for types that have been stabilised a while ago, and are way more likely never to add new implementations.

Wouldn't it be a good thing to be able to tell Rust "Yes, I know, this might conflict in the future, but with the current version, it's fine, so let me build until there ARE conflicts" ?

Rust's generics through traits and monomorphisation is one of my favourite features, but sometimes, in situations like this, Rust is TOO smart for its own good, and I think there should be ways to temper it.

This would also help with the problems that have been mentioned in the context of specialization and negative reasoning (maybe this could even enable negative reasoning which has the advantage of being more flexible and expressive than the currently selected "default" oriented specialisation), since breaking changes due to new implementations have always been limiting factors for them.

@jonas-schievink jonas-schievink added A-trait-coherence Proposals relating to coherence or orphans. T-lang Relevant to the language team, which will review and decide on the RFC. labels Nov 17, 2019
@s977120
Copy link

s977120 commented Nov 27, 2019

This restriction is really strict, sometimes backward compatibility is not possible, let alone forward compatibility.

@flying-sheep
Copy link

flying-sheep commented Dec 26, 2019

In unstable, one can circumvent this by telling the compiler that they won’t add new blanket impls for the type using #[fundamental]: rust-lang/rust#29635

So if fundamental is stabilized, this can be an error. Until then this needs to be a warning.

@snuk182
Copy link

snuk182 commented Jan 27, 2020

I actually hardly understand this. If I want to build something that fits right away, why should I bother about something theoretically possible in the future? I'm fine with dealing the impl conflicts once they actually appear.
Also I was asking regarding the usage of #[fundamental] in a non-compiler code and got a strict disallow.

@cztomsik
Copy link

cztomsik commented Feb 13, 2021

You can work around this with macro (impl_xxx!(TypeA, TypeB, ...)) - that way you're not using trait constraints because you are explicit.

But this is something which definitely should be fixed in language. I've seen it so many times already it's obvious that this was bad idea.

BTW: my recent case was using Pod from bytemuck - I wanted to impl something for all types which are "binary-safe" and then add extra impl for String and boom, impossible. So I can duplicate the trait myself or add a macro. Again. I'm writing macros all the time and it feels wrong.

@cmpute
Copy link

cmpute commented Mar 7, 2022

I guess it's worth pointing to this tracking issue about negative impls: rust-lang/rust#68318

@Aandreba
Copy link

Aandreba commented Apr 22, 2022

I'll leave here an example i've found if this "feature" causing problems

pub trait Responseable {
    fn into_stream (self) -> Box<dyn Stream<Item = u8>>;
}

impl<S: 'static + Stream<Item = u8>> Responseable for S {
    #[inline(always)]
    fn into_stream (self) -> Box<dyn Stream<Item = u8>> {
        Box::new(self)
    }
}

// conflicting implementations of trait `server::response::Responseable` for type `()`.
// upstream crates may add a new impl of trait `futures::Stream` for type `()` in future versions
impl Responseable for () {
 #[inline(always)]
    fn into_stream (self) -> Box<dyn Stream<Item = u8>> {
        Box::new(futures::stream::empty())
    }
}

As you see, rust is basically saying me "well, maybe someone sometime tries to do the same as you, so you can't do it".
This causes problems very often for no reason, and it should probably be a warning, not an error

@gcollombet
Copy link

recent case was using Pod from bytemuck

^^ Exact same thing here, I try to implement a trait to use bytemuck::cast_slice for all Pod + Zeroable type. And the problem appear when I add for Vec

@SalsaGal
Copy link

This is bizarre, why is this still a thing? Literally anything can be changed and broken in a future version of a crate why pick this in particular???

@Tortoaster
Copy link

I guess it's because new trait implementations are not commonly considered breaking changes (even though they can be).

Suppose your crate b depends on crate a at version 1.0. You implement a trait for a type within a that is not conflicting in the current version. Later on, a releases a breaking change in minor version 1.1 by adding a trait implementation that conflicts with your implementation. If you specified a's version as 1.0 and not ~1.0 or =1.0, a user that depends on your crate would use version 1.1 by default, in which case your crate no longer compiles.

It still feels unnecessarily strict to disallow all implementations that might conflict in the future, though. My use case is that I want my API to work with anything that implements FromStr, but also with time::OffsetDateTime which doesn't implement FromStr, among others. I thought I could solve this with a new trait similar to FromStr, with a blanket implementation for anything that already implements FromStr, along with separate implementations for OffsetDateTime and other types. But that is considered conflicting. As a workaround, I have to make a newtype for OffsetDateTime and implement FromStr for it, which is much less convenient for users of the API.

IMO, a warning that an implementation might conflict in the future would be enough. The warning can be silenced with an annotation, and that annotation would immediately make clear what the problem is if a conflicting implementation is ever added. The warning could also remind you to use a stricter version range for the dependency.

focustense added a commit to focustense/mina that referenced this issue Oct 19, 2023
Adds `Lerp` implementations for Glam's math types (`Vec2` et al).
Requires replacing the blanket primitive trait implementation for `Lerp`
with a macro, thanks to some controversial Rust behavior:

rust-lang/rfcs#2758

It's very silly, as types like `Vec2` are literally never going to
implement Primitive traits, but there's no way to opt out.

`num_traits` is still used for its convenient range-checking of integer
conversions.
@ARitz-Cracker
Copy link

ARitz-Cracker commented Mar 21, 2024

impl<E> From<E> for MyErrorType where E: Into<BaseLibErrorType> {
    fn from(value: E) -> Self {
        Self::BaseError(value.into())
    }
}
impl From<ErrorTypeFrom3rdPartyLib> for MyErrorType {
    // 🚫 ⚠️ 🛑 STOP 🛑 ⚠️ 🚫
    // BaseLibErrorType might implement From<AnyTypeThatCouldEverExistUntilTheEndOfTime>
    // This may result in conflicting traits in the future.
    // Shame on you for not considering this possibility
}

smh, guess I have to write a macro which calls a bunch of other macros, then.

@BaxHugh
Copy link

BaxHugh commented Sep 20, 2024

+1 here, would love for this to just be a warning. I'm having this issue warning me about stuff that as the OP said is stable:
I want to implement my error context trait for Result<T, E: std::error::Error> and E: std::error::Error generically,
but theoretically they might decide to impl std::error::Error for Result...

@pcone
Copy link

pcone commented Sep 25, 2024

+1 this feels very arbitrary. As @SalsaGal said - there's basically an unlimited number of things a library author could do to break compatibility in consuming code, and explicitly preventing this isn't a stated goal of the language (nor should it be - at most maybe force a major version bump in the Cargo.toml?). Why is this thing in particular disallowed, but not any of the hundreds of other arbitrary ways someone could break their code for consumers.

I ran into this trying to implement a trait defined in my code on various collection types - was all fine until I tried to add an impl on a <T: ExactSizeIterator> trait bound rather than a concrete type (because there's too many bespoke iterator impls out there for impl-ing on concrete types to be feasible).

pub trait Foo {
    fn foo(&self) -> &'static str;
}

impl<Item> Foo for Vec<Item> {
    fn foo(&self) -> &'static str {
        "I'm a Vec!"
    }
}

impl<K, V> Foo for HashMap<K, V> {
    fn foo(&self) -> &'static str {
        "I'm a HashMap!"
    }
}

impl<Item, T: ExactSizeIterator<Item = Item>> Foo for T {
    fn foo(&self) -> &'static str {
        "I don't compile :("
    }
}

It's valid code without the concrete impls above it, but fails with 'conflicting implementations of trait Foo for type std::vec::Vec<_> upstream crates may add a new impl of trait std::iter::Iterator for type std::vec::Vec<_> in future versions', even though theres not actually any conflicting implementations.

It sounds like the specialization feature might make this problem go away for this specific use case? Although currently enabling it doesn't change the error.

@Caellian
Copy link

Caellian commented Oct 2, 2024

I agree that this error make little sense in some cases (e.g. Result might implement Error), but I'm betting it's due to some type solver limitation. Besides specialization, chalk might affect this behavior as well.

I'd prefer being able to specify the impls are unsafe and/or have the compiler "bail" when the type with multiple implementations already exists instead of always doing so proactively. By bail I mean just ignore all traits/implementations when multiple apply when the user is trying to use some trait item and note conflict in the error - currently the compiler reports when some function/type/trait exists but isn't imported, so something along those lines (with unsafe) would work much better here.

Alternatively, there's #[stable(since = "1.0.0")] annotations on std trait implementations so the compiler can pick the earliest one for std types - which is the most important case IMO because stdlib can't be vendored as easily as other crates, this would preserve backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-coherence Proposals relating to coherence or orphans. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests