Skip to content

Commit

Permalink
Auto merge of rust-lang#123125 - gurry:122561-bad-note-non-zero-loop-…
Browse files Browse the repository at this point in the history 
…iters-2, r=estebank

Remove suggestion about iteration count in coerce

Fixes rust-lang#122561

The iteration count-centric suggestion was implemented in PR rust-lang#100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix rust-lang#100285 by simply making it redundant.
  • Loading branch information
@bors
bors committed May 5, 2024
2 parents 02f7806 + 6289ed8 commit 06e88c3
Show file tree
Hide file tree
Showing 13 changed files with 545 additions and 205 deletions.
155 changes: 17 additions & 138 deletions compiler/rustc_hir_typeck/src/coercion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,9 @@
use crate::errors::SuggestBoxingForReturnImplTrait;
use crate::FnCtxt;
use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag, MultiSpan};
use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::Expr;
use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
use rustc_infer::infer::type_variable::TypeVariableOrigin;
use rustc_infer::infer::{Coercion, DefineOpaqueTypes, InferOk, InferResult};
Expand Down Expand Up @@ -93,22 +91,6 @@ impl<'a, 'tcx> Deref for Coerce<'a, 'tcx> {

type CoerceResult<'tcx> = InferResult<'tcx, (Vec<Adjustment<'tcx>>, Ty<'tcx>)>;

struct CollectRetsVisitor<'tcx> {
ret_exprs: Vec<&'tcx hir::Expr<'tcx>>,
}

impl<'tcx> Visitor<'tcx> for CollectRetsVisitor<'tcx> {
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
match expr.kind {
hir::ExprKind::Ret(_) => self.ret_exprs.push(expr),
// `return` in closures does not return from the outer function
hir::ExprKind::Closure(_) => return,
_ => {}
}
intravisit::walk_expr(self, expr);
}
}

/// Coercing a mutable reference to an immutable works, while
/// coercing `&T` to `&mut T` should be forbidden.
fn coerce_mutbls<'tcx>(
Expand Down Expand Up @@ -1597,7 +1579,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {

let mut err;
let mut unsized_return = false;
let mut visitor = CollectRetsVisitor { ret_exprs: vec![] };
match *cause.code() {
ObligationCauseCode::ReturnNoExpression => {
err = struct_span_code_err!(
Expand Down Expand Up @@ -1632,11 +1613,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
if !fcx.tcx.features().unsized_locals {
unsized_return = self.is_return_ty_definitely_unsized(fcx);
}
if let Some(expression) = expression
&& let hir::ExprKind::Loop(loop_blk, ..) = expression.kind
{
intravisit::walk_block(&mut visitor, loop_blk);
}
}
ObligationCauseCode::ReturnValue(id) => {
err = self.report_return_mismatched_types(
Expand Down Expand Up @@ -1737,6 +1713,22 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
augment_error(&mut err);

if let Some(expr) = expression {
if let hir::ExprKind::Loop(
_,
_,
loop_src @ (hir::LoopSource::While | hir::LoopSource::ForLoop),
_,
) = expr.kind
{
let loop_type = if loop_src == hir::LoopSource::While {
"`while` loops"
} else {
"`for` loops"
};

err.note(format!("{loop_type} evaluate to unit type `()`"));
}

fcx.emit_coerce_suggestions(
&mut err,
expr,
Expand All @@ -1745,15 +1737,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
None,
Some(coercion_error),
);
if visitor.ret_exprs.len() > 0 {
self.note_unreachable_loop_return(
&mut err,
fcx.tcx,
&expr,
&visitor.ret_exprs,
expected,
);
}
}

let reported = err.emit_unless(unsized_return);
Expand Down Expand Up @@ -1827,110 +1810,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
);
}

fn note_unreachable_loop_return(
&self,
err: &mut Diag<'_>,
tcx: TyCtxt<'tcx>,
expr: &hir::Expr<'tcx>,
ret_exprs: &Vec<&'tcx hir::Expr<'tcx>>,
ty: Ty<'tcx>,
) {
let hir::ExprKind::Loop(_, _, _, loop_span) = expr.kind else {
return;
};
let mut span: MultiSpan = vec![loop_span].into();
span.push_span_label(loop_span, "this might have zero elements to iterate on");
const MAXITER: usize = 3;
let iter = ret_exprs.iter().take(MAXITER);
for ret_expr in iter {
span.push_span_label(
ret_expr.span,
"if the loop doesn't execute, this value would never get returned",
);
}
err.span_note(
span,
"the function expects a value to always be returned, but loops might run zero times",
);
if MAXITER < ret_exprs.len() {
err.note(format!(
"if the loop doesn't execute, {} other values would never get returned",
ret_exprs.len() - MAXITER
));
}
let hir = tcx.hir();
let item = hir.get_parent_item(expr.hir_id);
let ret_msg = "return a value for the case when the loop has zero elements to iterate on";
let ret_ty_msg =
"otherwise consider changing the return type to account for that possibility";
let node = tcx.hir_node(item.into());
if let Some(body_id) = node.body_id()
&& let Some(sig) = node.fn_sig()
&& let hir::ExprKind::Block(block, _) = hir.body(body_id).value.kind
&& !ty.is_never()
{
let indentation = if let None = block.expr
&& let [.., last] = &block.stmts
{
tcx.sess.source_map().indentation_before(last.span).unwrap_or_else(String::new)
} else if let Some(expr) = block.expr {
tcx.sess.source_map().indentation_before(expr.span).unwrap_or_else(String::new)
} else {
String::new()
};
if let None = block.expr
&& let [.., last] = &block.stmts
{
err.span_suggestion_verbose(
last.span.shrink_to_hi(),
ret_msg,
format!("\n{indentation}/* `{ty}` value */"),
Applicability::MaybeIncorrect,
);
} else if let Some(expr) = block.expr {
err.span_suggestion_verbose(
expr.span.shrink_to_hi(),
ret_msg,
format!("\n{indentation}/* `{ty}` value */"),
Applicability::MaybeIncorrect,
);
}
let mut sugg = match sig.decl.output {
hir::FnRetTy::DefaultReturn(span) => {
vec![(span, " -> Option<()>".to_string())]
}
hir::FnRetTy::Return(ty) => {
vec![
(ty.span.shrink_to_lo(), "Option<".to_string()),
(ty.span.shrink_to_hi(), ">".to_string()),
]
}
};
for ret_expr in ret_exprs {
match ret_expr.kind {
hir::ExprKind::Ret(Some(expr)) => {
sugg.push((expr.span.shrink_to_lo(), "Some(".to_string()));
sugg.push((expr.span.shrink_to_hi(), ")".to_string()));
}
hir::ExprKind::Ret(None) => {
sugg.push((ret_expr.span.shrink_to_hi(), " Some(())".to_string()));
}
_ => {}
}
}
if let None = block.expr
&& let [.., last] = &block.stmts
{
sugg.push((last.span.shrink_to_hi(), format!("\n{indentation}None")));
} else if let Some(expr) = block.expr {
sugg.push((expr.span.shrink_to_hi(), format!("\n{indentation}None")));
}
err.multipart_suggestion(ret_ty_msg, sugg, Applicability::MaybeIncorrect);
} else {
err.help(format!("{ret_msg}, {ret_ty_msg}"));
}
}

fn report_return_mismatched_types<'a>(
&self,
cause: &ObligationCause<'tcx>,
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_hir_typeck/src/demand.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
|| self.suggest_into(err, expr, expr_ty, expected)
|| self.suggest_floating_point_literal(err, expr, expected)
|| self.suggest_null_ptr_for_literal_zero_given_to_ptr_arg(err, expr, expected)
|| self.suggest_coercing_result_via_try_operator(err, expr, expected, expr_ty);
|| self.suggest_coercing_result_via_try_operator(err, expr, expected, expr_ty)
|| self.suggest_returning_value_after_loop(err, expr, expected);

if !suggested {
self.note_source_of_type_mismatch_constraint(
Expand Down
89 changes: 87 additions & 2 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ use rustc_hir::def::Res;
use rustc_hir::def::{CtorKind, CtorOf, DefKind};
use rustc_hir::lang_items::LangItem;
use rustc_hir::{
CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, GenericBound, HirId, Node,
Path, QPath, Stmt, StmtKind, TyKind, WherePredicate,
Arm, CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, GenericBound, HirId,
Node, Path, QPath, Stmt, StmtKind, TyKind, WherePredicate,
};
use rustc_hir_analysis::collect::suggest_impl_trait;
use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
Expand Down Expand Up @@ -1942,6 +1942,91 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
false
}

// If the expr is a while or for loop and is the tail expr of its
// enclosing body suggest returning a value right after it
pub fn suggest_returning_value_after_loop(
&self,
err: &mut Diag<'_>,
expr: &hir::Expr<'tcx>,
expected: Ty<'tcx>,
) -> bool {
let hir = self.tcx.hir();
let enclosing_scope =
hir.get_enclosing_scope(expr.hir_id).map(|hir_id| self.tcx.hir_node(hir_id));

// Get tail expr of the enclosing block or body
let tail_expr = if let Some(Node::Block(hir::Block { expr, .. })) = enclosing_scope
&& expr.is_some()
{
*expr
} else {
let body_def_id = hir.enclosing_body_owner(expr.hir_id);
let body_id = hir.body_owned_by(body_def_id);
let body = hir.body(body_id);

// Get tail expr of the body
match body.value.kind {
// Regular function body etc.
hir::ExprKind::Block(block, _) => block.expr,
// Anon const body (there's no block in this case)
hir::ExprKind::DropTemps(expr) => Some(expr),
_ => None,
}
};

let Some(tail_expr) = tail_expr else {
return false; // Body doesn't have a tail expr we can compare with
};

// Get the loop expr within the tail expr
let loop_expr_in_tail = match expr.kind {
hir::ExprKind::Loop(_, _, hir::LoopSource::While, _) => tail_expr,
hir::ExprKind::Loop(_, _, hir::LoopSource::ForLoop, _) => {
match tail_expr.peel_drop_temps() {
Expr { kind: ExprKind::Match(_, [Arm { body, .. }], _), .. } => body,
_ => return false, // Not really a for loop
}
}
_ => return false, // Not a while or a for loop
};

// If the expr is the loop expr in the tail
// then make the suggestion
if expr.hir_id == loop_expr_in_tail.hir_id {
let span = expr.span;

let (msg, suggestion) = if expected.is_never() {
(
"consider adding a diverging expression here",
"`loop {}` or `panic!(\"...\")`".to_string(),
)
} else {
("consider returning a value here", format!("`{expected}` value"))
};

let src_map = self.tcx.sess.source_map();
let suggestion = if src_map.is_multiline(expr.span) {
let indentation = src_map.indentation_before(span).unwrap_or_else(String::new);
format!("\n{indentation}/* {suggestion} */")
} else {
// If the entire expr is on a single line
// put the suggestion also on the same line
format!(" /* {suggestion} */")
};

err.span_suggestion_verbose(
span.shrink_to_hi(),
msg,
suggestion,
Applicability::MaybeIncorrect,
);

true
} else {
false
}
}

/// If the expected type is an enum (Issue #55250) with any variants whose
/// sole field is of the found type, suggest such variants. (Issue #42764)
pub(crate) fn suggest_compatible_variants(
Expand Down
Loading

0 comments on commit 06e88c3

Please sign in to comment.