Tracking issue for incoherent_fundamental_impls compatibility lint

[arielb1]

This is the summary issue for the incoherent_fundamental_impls
future-compatibility warning 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?

What is coherence

Rust relies coherence to ensure that for every use of a trait item, there is a single impl that is used to provide it. This is important for general understandability, and also for ensuring soundness in the presence of associated types.

While coherence is a relation between 2 impls of a single trait, in more complicated cases, coherence can rely on the non-existence of impls for a different trait.

For example, in one long-present case in libstd, std::str::pattern::Pattern is implemented for both char and all types implementing FnMut(char) -> bool, allowing searching strings using str::contains with both characters (my_str.contains('a')) and predicates on characters (my_str.contains(|c| c.is_uppercase())).

However, in order to be coherent, that relies on char not implementing FnMut(char) -> bool! If char behaved like a function, it would be not obvious which impl would be used in the case of my_string.contains('a').

Therefore, when making sure the impls for Pattern are coherent, the compiler has to check for impls in FnMut.

Dependent Crate Coherence

Coherence checking is sometimes more subtle. For example, in this wrong case, found in the crate rusqlite:

// This code is WRONG

pub enum ValueRef<'a> {
    // ...
}
pub enum Value {
    // ...
}
pub trait ToSqlOutput<'a> {
    // ...
}

impl<'a> From<&'a str> for ValueRef<'a> {
    // ...
}

impl<'a> From<String> for Value {
    // ...
}

impl<'a, T: ?Sized> From<&'a T> for dyn ToSqlOutput<'a>
where
    &'a T: Into<ValueRef<'a>>
{
    // ...
}
impl<'a, T: Into<Value>> From<T> for dyn ToSqlOutput<'a> {
//~^ ERROR conflicting implementations
    // ...
}

This code would violate coherence if there exists a type &'a T where both impls can be used. That would happen if both &'a T: Into<Value> and &'a T: Into<ValueRef<'a>>.

We can see in the code that the only types that are Into<Value> are Value (using the identity impl for From) and String (using the explicit impl for From), so no reference can be Into<Value>, and it would seem that coherence holds.

In that case, why is that code wrong? As compiling that crate will tell you, "downstream crates may implement trait std::convert::From<&_> for type Value". Even through there is no impl for Value in this crate, a dependent crate could implement both From<&MyReference> for Value and From<&'a MyReference> for ValueRef<'a> without breaking the orphan rules, causing a coherence conflict for From<&'a MyReference> for ToSqlOutput<'a>.

While all versions of rustc try to catch this sort of trick, older versions had a subtle bug that would miss it in more complicated cases (such as the above) which means that the code that appeared to work in the wild (as long as nobody actually did the tricky impls) is now not compiling.

How to fix this?

Are you seeing the warning in another crate or in your dependencies? We've made a big effort to ensure that there are newer versions available for crates that avoid this error. Upgrading will likely fix your problem:

• gtk-rs: upgrade to at least 0.4
• rusqlite: upgrade to at least 0.14
• nalgebra: upgrade to at least 0.15
• spade: upgrade or refresh the Cargo.lock file to use version 1.7
• imageproc: upgrade to at least 0.16

If however you are getting the warning for your own code, then read on: As this warning indicates a deep problem in the way the impls in the crate appear, there is no fix that works in all cases. However, it is often possible to change the impls a bit to get rid of the problem.

For example, rusqlite changed the second generic impl (the one where T: Into<Value>) to instead match all concrete cases:

impl<'a> From<String> for ToSqlOutput<'a> {
    fn from(t: String) -> Self { /* ... */ }
}
// <other cases>

Now, we already know that String isn't Into<ValueRef<'a>>, and no dependent crate can add such an impl, as the orphan rule would stop them in their tracks, which means coherence is restored!

When will this warning become a hard error?

At the beginning of each 6-week release cycle, the Rust compiler team
will review the set of outstanding future compatibility warnings and
nominate some of them for Final Comment Period. Toward the end of
the cycle, we will review any comments and make a final determination
whether to convert the warning into a hard error or remove it
entirely.

[nikomatsakis]

This is a "forwards compatibility" lint. We used to accept potentially overlapping impls due to a bug. We fixed the bug but only gave warnings so people had time to adapt. As we move forward on overhauling the trait system, though, I predict this is going to be an annoyance -- I think we should close this bug.

In order to do that, we need to prepare a PR that removes this lint and just makes the situation a hard error. You can find the code related to this lint by ripgrep'ing the code for INCOHERENT_FUNDAMENTAL_IMPLS (or use github search). Mostly it's here:

rust/src/librustc_typeck/coherence/inherent_impls_overlap.rs

Lines 50 to 63 in 56733bc

	let mut err = if used_to_be_allowed && node_id.is_some() {
	self.tcx.struct_span_lint_node(
	lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,
	node_id.unwrap(),
	self.tcx.span_of_impl(item1).unwrap(),
	&format!("duplicate definitions with name `{}` (E0592)", name)
	)
	} else {
	struct_span_err!(self.tcx.sess,
	self.tcx.span_of_impl(item1).unwrap(),
	E0592,
	"duplicate definitions with name `{}`",
	name)
	};

and here:

rust/src/librustc/traits/specialize/mod.rs

Lines 349 to 361 in dca1470

	let mut err = if used_to_be_allowed {
	tcx.struct_span_lint_node(
	lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,
	tcx.hir.as_local_node_id(impl_def_id).unwrap(),
	impl_span,
	&msg)
	} else {
	struct_span_err!(tcx.sess,
	impl_span,
	E0119,
	"{}",
	msg)
	};

We would remove the used_to_be_allowed_flag, which should allow us to simplify some of the code around it. We would also remove the code that declares the lint:

rust/src/librustc/lint/builtin.rs

Lines 209 to 213 in a0b0f5f

	declare_lint! {
	pub INCOHERENT_FUNDAMENTAL_IMPLS,
	Warn,
	"potentially-conflicting impls were erroneously allowed"
	}

And add a call to register_removed, sorta like this one:

rust/src/librustc_lint/lib.rs

Lines 317 to 318 in a0b0f5f

	store.register_removed("resolve_trait_on_defaulted_unit",
	"converted into hard error, see https://github.com/rust-lang/rust/issues/48950");

cc @rust-lang/wg-traits -- good trait-related cleanup!

[hdhoang]

I would like to tackle this, thanks for the instructions! AIUI, I can start with addressing these usages:

src/librustc_lint/lib.rs
268:            id: LintId::of(INCOHERENT_FUNDAMENTAL_IMPLS),

src/librustc_typeck/coherence/inherent_impls_overlap.rs
52:                            lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,

src/librustc/lint/builtin.rs
303:            INCOHERENT_FUNDAMENTAL_IMPLS,

src/test/compile-fail/issue-43355.rs
11:#![deny(incoherent_fundamental_impls)]

src/librustc/traits/specialize/mod.rs
351:                        lint::builtin::INCOHERENT_FUNDAMENTAL_IMPLS,

That could start out with replacing usage of used_to_be_allowed_flag with false, then trim down the resulting branches, then removing used_to_be_allowed_flag itself.

Then I will remove the lint declaration, and add the removal notification with link to this issue, and the run-pass compile test.

[nikomatsakis]

@hdhoang thanks!

[RReverser]

This new lint makes it very inconvenient to create generic wrappers.

For example, on one hand, I can't do

struct CustomWrapper<T>(T);

impl<T> From<CustomWrapper<T>> for T {
    fn from(w: CustomWrapper<T>) -> T { w.0 }
}

because that violates

error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g. `MyStruct<T>`)

 --> <source>:3:1

  |

3 | impl<T> From<CustomWrapper<T>> for T {

  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `T` must be used as the type parameter for some local type

  |

  = note: only traits defined in the current crate can be implemented for a type parameter

and, on the other hand, I now can't do more restricted version either

struct CustomWrapper<T>(T);

impl<T> Into<T> for CustomWrapper<T> {
    fn into(self) -> T { self.0 }
}

because of

rror[E0119]: conflicting implementations of trait `std::convert::Into<_>` for type `CustomWrapper<_>`:

 --> <source>:3:1

  |

3 | impl<T> Into<T> for CustomWrapper<T> {

  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  |

  = note: conflicting implementation in crate `core`:

          - impl<T, U> std::convert::Into<U> for T

            where U: std::convert::From<T>;

Is now the only way to define custom method each time I want to provide conversion? It doesn't feel very idiomatic if so, given the existence of specialised traits for that...

[Centril]

@arielb1 Could you update the issue description to fix "TBD: write understandable version."? Thanks.

[arielb1]

@Centril

Sure enough. That was TBD for quite some time :-).

[Centril]

Fixed in #49799.

[jonas-schievink]

There's still code referring to this lint, can this be removed now?

rust/src/librustc/traits/mod.rs

Lines 79 to 84 in 442ae7f

	/// Whether to enable bug compatibility with issue #43355.
	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
	pub enum IntercrateMode {
	Issue43355,
	Fixed,
	}