Skip to content

Misleading diagnostic span using trait projections in arguments #60980

Closed
@alexcrichton

Description

@alexcrichton
Member

I came across an interesting case today where the span for a diagnostic isn't quite leading to the best location. When compiling the example code below

struct A;

trait IntoWasmAbi {
    type Abi;
    fn into_abi(self) -> Self::Abi;
}

type T = <A as IntoWasmAbi>::Abi;

fn foo(a: T) {}

you get:

error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
  --> src/lib.rs:10:1
   |
10 | fn foo(a: T) {}
   | ^^^^^^^^^^^^^^^ the trait `IntoWasmAbi` is not implemented for `A`

but it's sort of misleading because neither A nor IntoWasmAbi is mentioned in the highlight. It'd be ideal if the error would point to the type definition to indicat that's where the source of the error comes from.

Activity

added
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
C-bugCategory: This is a bug.
on May 20, 2019
added
D-confusingDiagnostics: Confusing error or lint that should be reworked.
on Oct 8, 2019
estebank

estebank commented on Oct 8, 2019

@estebank
Contributor

The following is where the obligation could be changed from MiscObligation to something more specific:

rust/src/librustc/infer/mod.rs

Lines 1528 to 1550 in c20654e

/// Normalizes associated types in `value`, potentially returning
/// new obligations that must further be processed.
pub fn partially_normalize_associated_types_in<T>(
&self,
span: Span,
body_id: hir::HirId,
param_env: ty::ParamEnv<'tcx>,
value: &T,
) -> InferOk<'tcx, T>
where
T: TypeFoldable<'tcx>,
{
debug!("partially_normalize_associated_types_in(value={:?})", value);
let mut selcx = traits::SelectionContext::new(self);
let cause = ObligationCause::misc(span, body_id);
let traits::Normalized { value, obligations } =
traits::normalize(&mut selcx, param_env, cause, value);
debug!(
"partially_normalize_associated_types_in: result={:?} predicates={:?}",
value, obligations
);
InferOk { value, obligations }
}

And here is where we could peek at that new obligation and extend it with further information.

/// The guts of `normalize`: normalize a specific projection like `<T
/// as Trait>::Item`. The result is always a type (and possibly
/// additional obligations). If ambiguity arises, which implies that
/// there are unresolved type variables in the projection, we will
/// substitute a fresh type variable `$X` and generate a new
/// obligation `<T as Trait>::Item == $X` for later.
pub fn normalize_projection_type<'a, 'b, 'tcx>(
selcx: &'a mut SelectionContext<'b, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
projection_ty: ty::ProjectionTy<'tcx>,
cause: ObligationCause<'tcx>,
depth: usize,
obligations: &mut Vec<PredicateObligation<'tcx>>,
) -> Ty<'tcx> {
opt_normalize_projection_type(selcx, param_env, projection_ty.clone(), cause.clone(), depth,
obligations)
.unwrap_or_else(move || {
// if we bottom out in ambiguity, create a type variable
// and a deferred predicate to resolve this when more type
// information is available.
let tcx = selcx.infcx().tcx;
let def_id = projection_ty.item_def_id;
let ty_var = selcx.infcx().next_ty_var(
TypeVariableOrigin {
kind: TypeVariableOriginKind::NormalizeProjectionType,
span: tcx.def_span(def_id),
},
);
let projection = ty::Binder::dummy(ty::ProjectionPredicate {
projection_ty,
ty: ty_var
});
let obligation = Obligation::with_depth(
cause, depth + 1, param_env, projection.to_predicate());
obligations.push(obligation);
ty_var
})
}

Doing the naïve change cause.span = tcx.def_span(tcx.associated_item(projection_ty.item_def_id).container.id()) would cause this error to output:

error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
 --> file.rs:3:1
  |
3 | / trait IntoWasmAbi {
4 | |     type Abi;
5 | |     fn into_abi(self) -> Self::Abi;
6 | | }
  | |_^ the trait `IntoWasmAbi` is not implemented for `A`

The proper solution would require to track types (which would probably need #21934 to be fixed), but would ideally look like this:

error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
 --> file.rs:9:1
  |
1 |   struct A;
  |   -------- this struct doesn't implement `IntoWasmAbi`
2 | 
3 | / trait IntoWasmAbi {
4 | |     type Abi;
5 | |     fn into_abi(self) -> Self::Abi;
6 | | }
  | |_^ this trait is not implemented for `A`
7 | 
8 |   type T = <A as IntoWasmAbi>::Abi;
  |   -------------------------------- `IntoWasmAbi` expected to be implemented for `A` because of this
  | 
9 |   fn foo(a: T) {}
  |             - `IntoWasmAbi` is not implemented for `A`
estebank

estebank commented on Mar 5, 2020

@estebank
Contributor

@rust-lang/lang: we could assert well-formedness of hir::ItemKind::TyAlias early in check_item_well_formed (we don't today, we just skip them 👀), which would turn this case into the following more understandable output:

error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
 --> file2.rs:8:10
  |
8 | type T = <A as IntoWasmAbi>::Abi;
  |          ^^^^^^^^^^^^^^^^^^^^^^^ the trait `IntoWasmAbi` is not implemented for `A`

error[E0277]: the trait bound `A: IntoWasmAbi` is not satisfied
  --> file2.rs:10:1
   |
10 | fn foo(a: T) {}
   | ^^^^^^^^^^^^^^^ the trait `IntoWasmAbi` is not implemented for `A`

Is this something we would consider doing, even if it is as a warning? I believe this would be effectively a breaking change (code that compiles today would stop compiling) but not "in effect" because the previously compiling code could never be used. This seems to me like an accident to me.

For example, there's a test type Y where i32: Foo = (); // OK - bound is ignored which would fail with "the trait bound i32: Foo is not satisfied" with my proposed change, which is the correct behavior of all other items.


Edit: I see now that because we explicitily do not accept trait bounds on types arguments, we cannot really perform the wf check always, otherwise we would need to allow trait bounds (and either propagate them or continue ignoring them but now without a warning) so that type T<X: Trait> = <X as Trait>::Foo; is allowed, or only do a wf check when there are no generic types involved (which feels like a hack, but a valid one that will expand to cover other invalid cases in the future). Thoughts?


Edit: #69741 has the simplest change that should be safe to introduce based on my current understanding of the situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

A-diagnosticsArea: Messages for errors, warnings, and lintsC-bugCategory: This is a bug.D-confusingDiagnostics: Confusing error or lint that should be reworked.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Participants

    @alexcrichton@Centril@estebank@compiler-errors

    Issue actions

      Misleading diagnostic span using trait projections in arguments · Issue #60980 · rust-lang/rust