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.

Sign up for GitHub

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

Provide suggestion to convert numeric op LHS rather than unwrapping RHS #73195

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Provide suggestion to convert numeric op LHS rather than unwrapping RHS #73195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e243f62
Provide suggestion to convert numeric op LHS rather than unwrapping RHS
ayazhafiz Jun 9, 2020
0c02f8a
fixup! Provide suggestion to convert numeric op LHS rather than unwra…
ayazhafiz Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/coercion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1377,7 +1377,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
}

if let Some(expr) = expression {
fcx.emit_coerce_suggestions(&mut err, expr, found, expected);
fcx.emit_coerce_suggestions(&mut err, expr, found, expected, None);
}

// Error possibly reported in `check_assign` so avoid emitting error again.
Expand Down
109 changes: 79 additions & 30 deletions src/librustc_typeck/check/demand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
expr_ty: Ty<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
) {
self.annotate_expected_due_to_let_ty(err, expr);
self.suggest_compatible_variants(err, expr, expected, expr_ty);
self.suggest_deref_ref_or_into(err, expr, expected, expr_ty);
self.suggest_deref_ref_or_into(err, expr, expected, expr_ty, expected_ty_expr);
if self.suggest_calling_boxed_future_when_appropriate(err, expr, expected, expr_ty) {
return;
}
Expand Down Expand Up @@ -102,9 +103,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
checked_ty: Ty<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
allow_two_phase: AllowTwoPhase,
) -> Ty<'tcx> {
let (ty, err) = self.demand_coerce_diag(expr, checked_ty, expected, allow_two_phase);
let (ty, err) =
self.demand_coerce_diag(expr, checked_ty, expected, expected_ty_expr, allow_two_phase);
if let Some(mut err) = err {
err.emit();
}
Expand All @@ -121,6 +124,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
checked_ty: Ty<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
allow_two_phase: AllowTwoPhase,
) -> (Ty<'tcx>, Option<DiagnosticBuilder<'tcx>>) {
let expected = self.resolve_vars_with_obligations(expected);
Expand All @@ -141,7 +145,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return (expected, None);
}

self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected);
self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected, expected_ty_expr);

(expected, Some(err))
}
Expand Down Expand Up @@ -671,6 +675,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
checked_ty: Ty<'tcx>,
expected_ty: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
) -> bool {
if self.tcx.sess.source_map().is_imported(expr.span) {
// Ignore if span is from within a macro.
Expand Down Expand Up @@ -747,7 +752,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let msg = format!("you can convert an `{}` to `{}`", checked_ty, expected_ty);
let cast_msg = format!("you can cast an `{} to `{}`", checked_ty, expected_ty);
let try_msg = format!("{} and panic if the converted value wouldn't fit", msg);
let lit_msg = format!(
"change the type of the numeric literal from `{}` to `{}`",
checked_ty, expected_ty,
Expand All @@ -761,7 +765,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};

let cast_suggestion = format!("{}{} as {}", prefix, with_opt_paren(&src), expected_ty);
let try_into_suggestion = format!("{}{}.try_into().unwrap()", prefix, with_opt_paren(&src));
let into_suggestion = format!("{}{}.into()", prefix, with_opt_paren(&src));
let suffix_suggestion = with_opt_paren(&format_args!(
"{}{}",
Expand All @@ -782,22 +785,55 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};

let in_const_context = self.tcx.hir().is_inside_const_context(expr.hir_id);

let suggest_fallible_into_or_lhs_from =
|err: &mut DiagnosticBuilder<'_>, exp_to_found_is_fallible: bool| {
// If we know the expression the expected type is derived from, we might be able
// to suggest a widening conversion rather than a narrowing one (which may
// panic). For example, given x: u8 and y: u32, if we know the span of "x",
// x > y
// can be given the suggestion "u32::from(x) > y" rather than
// "x > y.try_into().unwrap()".
let lhs_expr_and_src = expected_ty_expr.and_then(|expr| {
match self.tcx.sess.source_map().span_to_snippet(expr.span).ok() {
Some(src) => Some((expr, src)),
None => None,
}
});
let (span, msg, suggestion) = if let (Some((lhs_expr, lhs_src)), false) =
(lhs_expr_and_src, exp_to_found_is_fallible)
{
let msg = format!(
"you can convert `{}` from `{}` to `{}`, matching the type of `{}`",
lhs_src, expected_ty, checked_ty, src
);
let suggestion = format!("{}::from({})", checked_ty, lhs_src,);
(lhs_expr.span, msg, suggestion)
} else {
let msg = format!("{} and panic if the converted value wouldn't fit", msg);
let suggestion =
format!("{}{}.try_into().unwrap()", prefix, with_opt_paren(&src));
(expr.span, msg, suggestion)
};
err.span_suggestion(span, &msg, suggestion, Applicability::MachineApplicable);
};

let suggest_to_change_suffix_or_into =
|err: &mut DiagnosticBuilder<'_>, is_fallible: bool| {
|err: &mut DiagnosticBuilder<'_>,
found_to_exp_is_fallible: bool,
exp_to_found_is_fallible: bool| {
let msg = if literal_is_ty_suffixed(expr) {
&lit_msg
} else if in_const_context {
// Do not recommend `into` or `try_into` in const contexts.
return;
} else if is_fallible {
&try_msg
} else if found_to_exp_is_fallible {
return suggest_fallible_into_or_lhs_from(err, exp_to_found_is_fallible);
} else {
&msg
};
let suggestion = if literal_is_ty_suffixed(expr) {
suffix_suggestion.clone()
} else if is_fallible {
try_into_suggestion
} else {
into_suggestion.clone()
};
Expand All @@ -806,41 +842,54 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

match (&expected_ty.kind, &checked_ty.kind) {
(&ty::Int(ref exp), &ty::Int(ref found)) => {
let is_fallible = match (exp.bit_width(), found.bit_width()) {
(Some(exp), Some(found)) if exp < found => true,
(None, Some(8 | 16)) => false,
(None, _) | (_, None) => true,
_ => false,
let (f2e_is_fallible, e2f_is_fallible) = match (exp.bit_width(), found.bit_width())
{
(Some(exp), Some(found)) if exp < found => (true, false),
(Some(exp), Some(found)) if exp > found => (false, true),
(None, Some(8 | 16)) => (false, true),
(Some(8 | 16), None) => (true, false),
(None, _) | (_, None) => (true, true),
_ => (false, false),
};
suggest_to_change_suffix_or_into(err, is_fallible);
suggest_to_change_suffix_or_into(err, f2e_is_fallible, e2f_is_fallible);
true
}
(&ty::Uint(ref exp), &ty::Uint(ref found)) => {
let is_fallible = match (exp.bit_width(), found.bit_width()) {
(Some(exp), Some(found)) if exp < found => true,
(None, Some(8 | 16)) => false,
(None, _) | (_, None) => true,
_ => false,
let (f2e_is_fallible, e2f_is_fallible) = match (exp.bit_width(), found.bit_width())
{
(Some(exp), Some(found)) if exp < found => (true, false),
(Some(exp), Some(found)) if exp > found => (false, true),
(None, Some(8 | 16)) => (false, true),
(Some(8 | 16), None) => (true, false),
(None, _) | (_, None) => (true, true),
_ => (false, false),
};
suggest_to_change_suffix_or_into(err, is_fallible);
suggest_to_change_suffix_or_into(err, f2e_is_fallible, e2f_is_fallible);
true
}
(&ty::Int(exp), &ty::Uint(found)) => {
let is_fallible = match (exp.bit_width(), found.bit_width()) {
(Some(exp), Some(found)) if found < exp => false,
(None, Some(8)) => false,
_ => true,
let (f2e_is_fallible, e2f_is_fallible) = match (exp.bit_width(), found.bit_width())
{
(Some(exp), Some(found)) if found < exp => (false, true),
(None, Some(8)) => (false, true),
_ => (true, true),
};
suggest_to_change_suffix_or_into(err, is_fallible);
suggest_to_change_suffix_or_into(err, f2e_is_fallible, e2f_is_fallible);
true
}
(&ty::Uint(_), &ty::Int(_)) => {
suggest_to_change_suffix_or_into(err, true);
(&ty::Uint(exp), &ty::Int(found)) => {
let (f2e_is_fallible, e2f_is_fallible) = match (exp.bit_width(), found.bit_width())
{
(Some(exp), Some(found)) if found > exp => (true, false),
(Some(8), None) => (true, false),
_ => (true, true),
};
suggest_to_change_suffix_or_into(err, f2e_is_fallible, e2f_is_fallible);
true
}
(&ty::Float(ref exp), &ty::Float(ref found)) => {
if found.bit_width() < exp.bit_width() {
suggest_to_change_suffix_or_into(err, false);
suggest_to_change_suffix_or_into(err, false, true);
} else if literal_is_ty_suffixed(expr) {
err.span_suggestion(
expr.span,
Expand Down
23 changes: 12 additions & 11 deletions src/librustc_typeck/check/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

if let Some(mut err) = self.demand_suptype_diag(expr.span, expected_ty, ty) {
let expr = expr.peel_drop_temps();
self.suggest_deref_ref_or_into(&mut err, expr, expected_ty, ty);
self.suggest_deref_ref_or_into(&mut err, expr, expected_ty, ty, None);
extend_err(&mut err);
// Error possibly reported in `check_assign` so avoid emitting error again.
err.emit_unless(self.is_assign_to_bool(expr, expected_ty));
Expand All @@ -98,10 +98,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
expr: &'tcx hir::Expr<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
) -> Ty<'tcx> {
let ty = self.check_expr_with_hint(expr, expected);
// checks don't need two phase
self.demand_coerce(expr, ty, expected, AllowTwoPhase::No)
self.demand_coerce(expr, ty, expected, expected_ty_expr, AllowTwoPhase::No)
}

pub(super) fn check_expr_with_hint(
Expand Down Expand Up @@ -776,7 +777,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
span: &Span,
) -> Ty<'tcx> {
let lhs_ty = self.check_expr_with_needs(&lhs, Needs::MutPlace);
let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty);
let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty, Some(lhs));

let expected_ty = expected.coercion_target_type(self, expr.span);
if expected_ty == self.tcx.types.bool {
Expand Down Expand Up @@ -1026,7 +1027,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let (element_ty, t) = match uty {
Some(uty) => {
self.check_expr_coercable_to_type(&element, uty);
self.check_expr_coercable_to_type(&element, uty, None);
(uty, uty)
}
None => {
Expand Down Expand Up @@ -1063,7 +1064,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let elt_ts_iter = elts.iter().enumerate().map(|(i, e)| match flds {
Some(ref fs) if i < fs.len() => {
let ety = fs[i].expect_ty();
self.check_expr_coercable_to_type(&e, ety);
self.check_expr_coercable_to_type(&e, ety, None);
ety
}
_ => self.check_expr_with_expectation(&e, NoExpectation),
Expand Down Expand Up @@ -1237,7 +1238,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// Make sure to give a type to the field even if there's
// an error, so we can continue type-checking.
self.check_expr_coercable_to_type(&field.expr, field_type);
self.check_expr_coercable_to_type(&field.expr, field_type, None);
}

// Make sure the programmer specified correct number of fields.
Expand Down Expand Up @@ -1735,7 +1736,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
match self.lookup_indexing(expr, base, base_t, idx_t, needs) {
Some((index_ty, element_ty)) => {
// two-phase not needed because index_ty is never mutable
self.demand_coerce(idx, idx_t, index_ty, AllowTwoPhase::No);
self.demand_coerce(idx, idx_t, index_ty, None, AllowTwoPhase::No);
element_ty
}
None => {
Expand Down Expand Up @@ -1788,7 +1789,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
) -> Ty<'tcx> {
match self.resume_yield_tys {
Some((resume_ty, yield_ty)) => {
self.check_expr_coercable_to_type(&value, yield_ty);
self.check_expr_coercable_to_type(&value, yield_ty, None);

resume_ty
}
Expand All @@ -1797,7 +1798,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// information. Hence, we check the source of the yield expression here and check its
// value's type against `()` (this check should always hold).
None if src.is_await() => {
self.check_expr_coercable_to_type(&value, self.tcx.mk_unit());
self.check_expr_coercable_to_type(&value, self.tcx.mk_unit(), None);
self.tcx.mk_unit()
}
_ => {
Expand Down Expand Up @@ -1836,11 +1837,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
match ty.kind {
ty::FnDef(..) => {
let fnptr_ty = self.tcx.mk_fn_ptr(ty.fn_sig(self.tcx));
self.demand_coerce(expr, ty, fnptr_ty, AllowTwoPhase::No);
self.demand_coerce(expr, ty, fnptr_ty, None, AllowTwoPhase::No);
}
ty::Ref(_, base_ty, mutbl) => {
let ptr_ty = self.tcx.mk_ptr(ty::TypeAndMut { ty: base_ty, mutbl });
self.demand_coerce(expr, ty, ptr_ty, AllowTwoPhase::No);
self.demand_coerce(expr, ty, ptr_ty, None, AllowTwoPhase::No);
}
_ => {}
}
Expand Down
9 changes: 5 additions & 4 deletions src/librustc_typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ fn typeck_tables_of_with_fallback<'tcx>(
// Gather locals in statics (because of block expressions).
GatherLocalsVisitor { fcx: &fcx, parent_id: id }.visit_body(body);

fcx.check_expr_coercable_to_type(&body.value, revealed_ty);
fcx.check_expr_coercable_to_type(&body.value, revealed_ty, None);

fcx.write_ty(id, revealed_ty);

Expand Down Expand Up @@ -4123,7 +4123,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let coerce_ty = expected.only_has_type(self).unwrap_or(formal_ty);
// We're processing function arguments so we definitely want to use
// two-phase borrows.
self.demand_coerce(&arg, checked_ty, coerce_ty, AllowTwoPhase::Yes);
self.demand_coerce(&arg, checked_ty, coerce_ty, None, AllowTwoPhase::Yes);
final_arg_types.push((i, checked_ty, coerce_ty));

// 3. Relate the expected type and the formal one,
Expand Down Expand Up @@ -4541,7 +4541,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.demand_eqtype(init.span, local_ty, init_ty);
init_ty
} else {
self.check_expr_coercable_to_type(init, local_ty)
self.check_expr_coercable_to_type(init, local_ty, None)
}
}

Expand Down Expand Up @@ -5027,6 +5027,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr: &hir::Expr<'_>,
expected: Ty<'tcx>,
found: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
) {
if let Some((sp, msg, suggestion, applicability)) = self.check_ref(expr, found, expected) {
err.span_suggestion(sp, msg, suggestion, applicability);
Expand All @@ -5037,7 +5038,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let sp = self.sess().source_map().guess_head_span(sp);
err.span_label(sp, &format!("{} defined here", found));
}
} else if !self.check_for_cast(err, expr, found, expected) {
} else if !self.check_for_cast(err, expr, found, expected, expected_ty_expr) {
let is_struct_pat_shorthand_field =
self.is_hir_id_from_struct_pattern_shorthand_field(expr.hir_id, expr.span);
let methods = self.get_conversion_methods(expr.span, expected, found, expr.hir_id);
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_typeck/check/op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
match BinOpCategory::from(op) {
BinOpCategory::Shortcircuit => {
// && and || are a simple case.
self.check_expr_coercable_to_type(lhs_expr, tcx.types.bool);
self.check_expr_coercable_to_type(lhs_expr, tcx.types.bool, None);
let lhs_diverges = self.diverges.get();
self.check_expr_coercable_to_type(rhs_expr, tcx.types.bool);
self.check_expr_coercable_to_type(rhs_expr, tcx.types.bool, None);

// Depending on the LHS' value, the RHS can never execute.
self.diverges.set(lhs_diverges);
Expand Down Expand Up @@ -170,7 +170,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
kind: TypeVariableOriginKind::MiscVariable,
span: lhs_expr.span,
});
self.demand_coerce(lhs_expr, lhs_ty, fresh_var, AllowTwoPhase::No)
self.demand_coerce(lhs_expr, lhs_ty, fresh_var, Some(rhs_expr), AllowTwoPhase::No)
}
IsAssign::Yes => {
// rust-lang/rust#52126: We have to use strict
Expand All @@ -196,7 +196,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let result = self.lookup_op_method(lhs_ty, &[rhs_ty_var], Op::Binary(op, is_assign));

// see `NB` above
let rhs_ty = self.check_expr_coercable_to_type(rhs_expr, rhs_ty_var);
let rhs_ty = self.check_expr_coercable_to_type(rhs_expr, rhs_ty_var, Some(lhs_expr));
let rhs_ty = self.resolve_vars_with_obligations(rhs_ty);

let return_ty = match result {
Expand Down
Loading