Unify binop errors wording

[estebank]

The following two errors are actually the same problem but have different output because they are handled in different parts of the compiler. We should unify the output to have similar, if not the same, text.

error[E0277]: cannot add `std::option::Option<i8>` to `i8`
 --> src/main.rs:5:13
  |
5 | let sum = x + y;
  |             ^ no implementation for `i8 + std::option::Option<i8>`
  |
  = help: the trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`

error[E0369]: binary operation `+` cannot be applied to type `std::option::Option<i8>`
 --> src/main.rs:5:13
  |
5 | let sum = y + x;
  |           - ^ - i8
  |           |
  |           std::option::Option<i8>
  |
  = note: an implementation of `std::ops::Add` might be missing for `std::option::Option<i8>`

This issue has been assigned to @LeSeulArtichaut via this comment.

[Lonami]

How should one go about implementing this? Somehow sorting the operator order to pick consistently? Or better to always show the two hints for both orderings?

[estebank]

We could do it by picking and matching wording from both errors to make them consistent with each other. The E0277 one can be updated by changing the rustc_on_unimplemented text for the trait, while E0369 can be updated by changing the following code:

rust/src/librustc_typeck/check/op.rs

Lines 329 to 422 in ef9a876

	IsAssign::No => {
	let mut err = struct_span_err!(self.tcx.sess, op.span, E0369,
	"binary operation `{}` cannot be applied to type `{}`",
	op.node.as_str(),
	lhs_ty);
	let mut involves_fn = false;
	if !lhs_expr.span.eq(&rhs_expr.span) {
	involves_fn |= self.add_type_neq_err_label(
	&mut err,
	lhs_expr.span,
	lhs_ty,
	rhs_ty,
	op,
	is_assign
	);
	involves_fn |= self.add_type_neq_err_label(
	&mut err,
	rhs_expr.span,
	rhs_ty,
	lhs_ty,
	op,
	is_assign
	);
	}
	let mut suggested_deref = false;
	if let Ref(_, mut rty, _) = lhs_ty.sty {
	if {
	self.infcx.type_is_copy_modulo_regions(self.param_env,
	rty,
	lhs_expr.span) &&
	self.lookup_op_method(rty,
	&[rhs_ty],
	Op::Binary(op, is_assign))
	.is_ok()
	} {
	if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) {
	while let Ref(_, rty_inner, _) = rty.sty {
	rty = rty_inner;
	}
	let msg = &format!(
	"`{}` can be used on '{}', you can \
	dereference `{2}`: `*{2}`",
	op.node.as_str(),
	rty,
	lstring
	);
	err.help(msg);
	suggested_deref = true;
	}
	}
	}
	let missing_trait = match op.node {
	hir::BinOpKind::Add => Some("std::ops::Add"),
	hir::BinOpKind::Sub => Some("std::ops::Sub"),
	hir::BinOpKind::Mul => Some("std::ops::Mul"),
	hir::BinOpKind::Div => Some("std::ops::Div"),
	hir::BinOpKind::Rem => Some("std::ops::Rem"),
	hir::BinOpKind::BitAnd => Some("std::ops::BitAnd"),
	hir::BinOpKind::BitXor => Some("std::ops::BitXor"),
	hir::BinOpKind::BitOr => Some("std::ops::BitOr"),
	hir::BinOpKind::Shl => Some("std::ops::Shl"),
	hir::BinOpKind::Shr => Some("std::ops::Shr"),
	hir::BinOpKind::Eq |
	hir::BinOpKind::Ne => Some("std::cmp::PartialEq"),
	hir::BinOpKind::Lt |
	hir::BinOpKind::Le |
	hir::BinOpKind::Gt |
	hir::BinOpKind::Ge => Some("std::cmp::PartialOrd"),
	_ => None
	};
	if let Some(missing_trait) = missing_trait {
	if op.node == hir::BinOpKind::Add &&
	self.check_str_addition(expr, lhs_expr, rhs_expr, lhs_ty,
	rhs_ty, &mut err, false, op) {
	// This has nothing here because it means we did string
	// concatenation (e.g., "Hello " + "World!"). This means
	// we don't want the note in the else clause to be emitted
	} else if let ty::Param(_) = lhs_ty.sty {
	// FIXME: point to span of param
	err.note(&format!(
	"`{}` might need a bound for `{}`",
	lhs_ty, missing_trait
	));
	} else if !suggested_deref && !involves_fn {
	err.note(&format!(
	"an implementation of `{}` might \
	be missing for `{}`",
	missing_trait, lhs_ty
	));
	}
	}
	err.emit();

I think in the end E0369 should mimic E0277 and not worry about adding the type labels to E0277.

[JohnTitor]

I'll work on this but I don't catch better wording. One of options is to change E0369 cannot be applied to type {} to cannot be applied to type {} and {}, is this reasonable? And should we also unify note/help wording?

[LeSeulArtichaut]

@rustbot claim

[estebank]

@JohnTitor I believe that doing the following should be enough to close this ticket:

error[E0277]: cannot add `std::option::Option<i8>` to `i8`
 --> src/main.rs:5:13
  |
5 | let sum = x + y;
  |             ^ no implementation for `i8 + std::option::Option<i8>`
  |
  = help: the trait `std::ops::Add<std::option::Option<i8>>` is not implemented for `i8`

error[E0369]: cannot add `i8` to `std::option::Option<i8>`
 --> src/main.rs:5:13
  |
5 | let sum = y + x;
  |           - ^ - i8
  |           | |
  |           | no implementation for `std::option::Option<i8> + i8`
  |           std::option::Option<i8>
  |
  = note: an implementation of `std::ops::Add<i8>` might be missing for `std::option::Option<i8>`

The note suggesting implementing Add<i8> should only be emitted if Option<i8> were local to the current crate, which it isn't, so in this case it shouldn't be emitted.

@LeSeulArtichaut feel free to reach out if you need help with this.

[LeSeulArtichaut]

@estebank I don't know what to do with the comparaison operations. For example, this gives me E0308, and I haven't figured out a case with comparaison operators where E0369 is emitted. Am I missing something?

[estebank]

@LeSeulArtichaut it is ok if you only tackle a subset of all the binops, we can get back to == and other bool binops friends later.

These go through different parts of typeck, which is why they produce different results.