Incorrect coherence diagnostic

[gnzlbg]

struct MyError;

impl From<i32> for Result<(), MyError> {
    fn from(x: i32) -> Self {
        if x == 0 { Ok(()) } else { Err(MyError) }
    }
}

fn main() {}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:3:1
  |
3 | impl From<i32> for Result<(), MyError> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
  |
  = note: the impl does not reference any types defined in this crate
  = note: define and implement a trait or new type instead

error: aborting due to previous error

For more information about this error, try `rustc --explain E0117`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.

This is incorrect because the error message states:

the impl does not reference any types defined in this crate

but the impl does reference one type defined in this crate: MyError.

[estebank]

This could be fixed by changing the wording to "the impl does not reference only types defined in this crate" either only for cases like this if easy, or always.

[lbrande]

I would like to give this a try. I haven't worked on this repo before so if possible I would appreciate some guidance.

[estebank]

@lbrande the code to change is the note at

rust/src/librustc_typeck/coherence/orphan.rs

Lines 47 to 55 in 25a42b2

	struct_span_err!(self.tcx.sess,
	sp,
	E0117,
	"only traits defined in the current crate can be \
	implemented for arbitrary types")
	.span_label(sp, "impl doesn't use types inside crate")
	.note("the impl does not reference any types defined in this crate")
	.note("define and implement a trait or new type instead")
	.emit();

The simplest change will be to just change the wording unconditionally to "the impl does not reference only types defined in this crate". The other option is to look at the following method

rust/src/librustc/traits/coherence.rs

Lines 254 to 390 in 25a42b2

	/// Check whether a trait-ref is potentially implementable by a crate.
	///
	/// The current rule is that a trait-ref orphan checks in a crate C:
	///
	/// 1. Order the parameters in the trait-ref in subst order - Self first,
	/// others linearly (e.g. `<U as Foo<V, W>>` is U < V < W).
	/// 2. Of these type parameters, there is at least one type parameter
	/// in which, walking the type as a tree, you can reach a type local
	/// to C where all types in-between are fundamental types. Call the
	/// first such parameter the "local key parameter".
	/// - e.g. `Box<LocalType>` is OK, because you can visit LocalType
	/// going through `Box`, which is fundamental.
	/// - similarly, `FundamentalPair<Vec<()>, Box<LocalType>>` is OK for
	/// the same reason.
	/// - but (knowing that `Vec<T>` is non-fundamental, and assuming it's
	/// not local), `Vec<LocalType>` is bad, because `Vec<->` is between
	/// the local type and the type parameter.
	/// 3. Every type parameter before the local key parameter is fully known in C.
	/// - e.g. `impl<T> T: Trait<LocalType>` is bad, because `T` might be
	/// an unknown type.
	/// - but `impl<T> LocalType: Trait<T>` is OK, because `LocalType`
	/// occurs before `T`.
	/// 4. Every type in the local key parameter not known in C, going
	/// through the parameter's type tree, must appear only as a subtree of
	/// a type local to C, with only fundamental types between the type
	/// local to C and the local key parameter.
	/// - e.g. `Vec<LocalType<T>>>` (or equivalently `Box<Vec<LocalType<T>>>`)
	/// is bad, because the only local type with `T` as a subtree is
	/// `LocalType<T>`, and `Vec<->` is between it and the type parameter.
	/// - similarly, `FundamentalPair<LocalType<T>, T>` is bad, because
	/// the second occurrence of `T` is not a subtree of *any* local type.
	/// - however, `LocalType<Vec<T>>` is OK, because `T` is a subtree of
	/// `LocalType<Vec<T>>`, which is local and has no types between it and
	/// the type parameter.
	///
	/// The orphan rules actually serve several different purposes:
	///
	/// 1. They enable link-safety - i.e. 2 mutually-unknowing crates (where
	/// every type local to one crate is unknown in the other) can't implement
	/// the same trait-ref. This follows because it can be seen that no such
	/// type can orphan-check in 2 such crates.
	///
	/// To check that a local impl follows the orphan rules, we check it in
	/// InCrate::Local mode, using type parameters for the "generic" types.
	///
	/// 2. They ground negative reasoning for coherence. If a user wants to
	/// write both a conditional blanket impl and a specific impl, we need to
	/// make sure they do not overlap. For example, if we write
	/// ```
	/// impl<T> IntoIterator for Vec<T>
	/// impl<T: Iterator> IntoIterator for T
	/// ```
	/// We need to be able to prove that `Vec<$0>: !Iterator` for every type $0.
	/// We can observe that this holds in the current crate, but we need to make
	/// sure this will also hold in all unknown crates (both "independent" crates,
	/// which we need for link-safety, and also child crates, because we don't want
	/// child crates to get error for impl conflicts in a *dependency*).
	///
	/// For that, we only allow negative reasoning if, for every assignment to the
	/// inference variables, every unknown crate would get an orphan error if they
	/// try to implement this trait-ref. To check for this, we use InCrate::Remote
	/// mode. That is sound because we already know all the impls from known crates.
	///
	/// 3. For non-#[fundamental] traits, they guarantee that parent crates can
	/// add "non-blanket" impls without breaking negative reasoning in dependent
	/// crates. This is the "rebalancing coherence" (RFC 1023) restriction.
	///
	/// For that, we only a allow crate to perform negative reasoning on
	/// non-local-non-#[fundamental] only if there's a local key parameter as per (2).
	///
	/// Because we never perform negative reasoning generically (coherence does
	/// not involve type parameters), this can be interpreted as doing the full
	/// orphan check (using InCrate::Local mode), substituting non-local known
	/// types for all inference variables.
	///
	/// This allows for crates to future-compatibly add impls as long as they
	/// can't apply to types with a key parameter in a child crate - applying
	/// the rules, this basically means that every type parameter in the impl
	/// must appear behind a non-fundamental type (because this is not a
	/// type-system requirement, crate owners might also go for "semantic
	/// future-compatibility" involving things such as sealed traits, but
	/// the above requirement is sufficient, and is necessary in "open world"
	/// cases).
	///
	/// Note that this function is never called for types that have both type
	/// parameters and inference variables.
	fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt<'_, '_, '_>,
	trait_ref: ty::TraitRef<'tcx>,
	in_crate: InCrate)
	-> Result<(), OrphanCheckErr<'tcx>>
	{
	debug!("orphan_check_trait_ref(trait_ref={:?}, in_crate={:?})",
	trait_ref, in_crate);
	if trait_ref.needs_infer() && trait_ref.needs_subst() {
	bug!("can't orphan check a trait ref with both params and inference variables {:?}",
	trait_ref);
	}
	// First, create an ordered iterator over all the type parameters to the trait, with the self
	// type appearing first.
	// Find the first input type that either references a type parameter OR
	// some local type.
	for input_ty in trait_ref.input_types() {
	if ty_is_local(tcx, input_ty, in_crate) {
	debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);
	// First local input type. Check that there are no
	// uncovered type parameters.
	let uncovered_tys = uncovered_tys(tcx, input_ty, in_crate);
	for uncovered_ty in uncovered_tys {
	if let Some(param) = uncovered_ty.walk()
	.find(|t| is_possibly_remote_type(t, in_crate))
	{
	debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
	return Err(OrphanCheckErr::UncoveredTy(param));
	}
	}
	// OK, found local type, all prior types upheld invariant.
	return Ok(());
	}
	// Otherwise, enforce invariant that there are no type
	// parameters reachable.
	if let Some(param) = input_ty.walk()
	.find(|t| is_possibly_remote_type(t, in_crate))
	{
	debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
	return Err(OrphanCheckErr::UncoveredTy(param));
	}
	}
	// If we exit above loop, never found a local type.
	debug!("orphan_check_trait_ref: no local type");
	return Err(OrphanCheckErr::NoLocalInputType);
	}

and come up with a further check for this case and new variant on OrphanCheckErr to encode it.

Ideally we could produce output like the following, by keeping the Spans for the local types found

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:3:1
  |
3 | impl From<i32> for Result<(), MyError> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------^ impl doesn't use types inside crate
  |                               |
  |                               even though this type is defined here, `Result` isn't
  |
  = note: the impl does not reference only types defined in this crate
  = note: define and implement a trait or new type instead

but I feel the added complexity in the implementation isn't as useful, and we might be better served with just improving the wording. I'm looking at the second edition of the book, and this is the explanation there:

This restriction is part of a property of programs called coherence, and more specifically the orphan rule, so named because the parent type is not present. This rule ensures that other people’s code can’t break your code and vice versa. Without the rule, two crates could implement the same trait for the same type, and Rust wouldn’t know which implementation to use.

---

For information on how to build and debug rustc, look at the forge, particularly "Building the compiler".

[lbrande]

Thanks, will try to get things set up later today.

[estebank]

Another possible output (which might be easier to accomplish) could be:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:3:1
  |
3 | impl From<i32> for Result<(), MyError> {
  | ^^^^^----^^^^^^^^^^------^^^^^^^^^^^^^ impl doesn't use types inside crate
  |      |             |
  |      |             this type is isn't defined in this crate
  |      this trait isn't defined in this crate
  |
  = note: the impl does not reference only types defined in this crate
  = note: define and implement a trait or new type instead