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.

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

Suggest returning local on "expected ty, found ()" due to expr-less block #98784

Merged
merged 1 commit into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 4 additions & 40 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ use rustc_middle::ty::{self, DefIdTree, IsSuggestable, Ty};
use rustc_session::Session;
use rustc_span::symbol::Ident;
use rustc_span::{self, Span};
use rustc_trait_selection::traits::{
self, ObligationCauseCode, SelectionContext, StatementAsExpression,
};
use rustc_trait_selection::traits::{self, ObligationCauseCode, SelectionContext};

use std::iter;
use std::slice;
Expand Down Expand Up @@ -1410,7 +1408,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self.misc(sp),
&mut |err| {
if let Some(expected_ty) = expected.only_has_type(self) {
self.consider_hint_about_removing_semicolon(blk, expected_ty, err);
if !self.consider_removing_semicolon(blk, expected_ty, err) {
self.consider_returning_binding(blk, expected_ty, err);
}
if expected_ty == self.tcx.types.bool {
// If this is caused by a missing `let` in a `while let`,
// silence this redundant error, as we already emit E0070.
Expand Down Expand Up @@ -1478,42 +1478,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty
}

/// A common error is to add an extra semicolon:
///
/// ```compile_fail,E0308
/// fn foo() -> usize {
/// 22;
/// }
/// ```
///
/// This routine checks if the final statement in a block is an
/// expression with an explicit semicolon whose type is compatible
/// with `expected_ty`. If so, it suggests removing the semicolon.
fn consider_hint_about_removing_semicolon(
&self,
blk: &'tcx hir::Block<'tcx>,
expected_ty: Ty<'tcx>,
err: &mut Diagnostic,
) {
if let Some((span_semi, boxed)) = self.could_remove_semicolon(blk, expected_ty) {
if let StatementAsExpression::NeedsBoxing = boxed {
err.span_suggestion_verbose(
span_semi,
"consider removing this semicolon and boxing the expression",
"",
Applicability::HasPlaceholders,
);
} else {
err.span_suggestion_short(
span_semi,
"remove this semicolon",
"",
Applicability::MachineApplicable,
);
}
}
}

fn parent_item_span(&self, id: hir::HirId) -> Option<Span> {
let node = self.tcx.hir().get_by_def_id(self.tcx.hir().get_parent_item(id));
match node {
Expand Down
159 changes: 156 additions & 3 deletions compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::astconv::AstConv;
use crate::errors::{AddReturnTypeSuggestion, ExpectedReturnTypeLabel};

use rustc_ast::util::parser::ExprPrecedence;
use rustc_data_structures::stable_set::FxHashSet;
use rustc_errors::{Applicability, Diagnostic, MultiSpan};
use rustc_hir as hir;
use rustc_hir::def::{CtorOf, DefKind};
Expand All @@ -11,12 +12,12 @@ use rustc_hir::{
Expr, ExprKind, GenericBound, Node, Path, QPath, Stmt, StmtKind, TyKind, WherePredicate,
};
use rustc_infer::infer::{self, TyCtxtInferExt};
use rustc_infer::traits;
use rustc_infer::traits::{self, StatementAsExpression};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty};
use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty, TypeVisitable};
use rustc_span::symbol::sym;
use rustc_span::Span;
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub(in super::super) fn suggest_semicolon_at_end(&self, span: Span, err: &mut Diagnostic) {
Expand Down Expand Up @@ -864,4 +865,156 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
}
}

/// A common error is to add an extra semicolon:
///
/// ```compile_fail,E0308
/// fn foo() -> usize {
/// 22;
/// }
/// ```
///
/// This routine checks if the final statement in a block is an
/// expression with an explicit semicolon whose type is compatible
/// with `expected_ty`. If so, it suggests removing the semicolon.
pub(crate) fn consider_removing_semicolon(
&self,
blk: &'tcx hir::Block<'tcx>,
expected_ty: Ty<'tcx>,
err: &mut Diagnostic,
) -> bool {
if let Some((span_semi, boxed)) = self.could_remove_semicolon(blk, expected_ty) {
if let StatementAsExpression::NeedsBoxing = boxed {
err.span_suggestion_verbose(
span_semi,
"consider removing this semicolon and boxing the expression",
"",
Comment on lines +888 to +891
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preexisting: This stands out to me. I guess we didn't have the statement span without the semicolon to suggest Box::new( before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix that -- we should be able to strip the semicolon and provide a full suggestion (MaybeIncorrect, still) to box it

Applicability::HasPlaceholders,
);
} else {
err.span_suggestion_short(
span_semi,
"remove this semicolon",
"",
Applicability::MachineApplicable,
);
}
true
} else {
false
}
}

pub(crate) fn consider_returning_binding(
&self,
blk: &'tcx hir::Block<'tcx>,
expected_ty: Ty<'tcx>,
err: &mut Diagnostic,
) {
let mut shadowed = FxHashSet::default();
let mut candidate_idents = vec![];
let mut find_compatible_candidates = |pat: &hir::Pat<'_>| {
if let hir::PatKind::Binding(_, hir_id, ident, _) = &pat.kind
&& let Some(pat_ty) = self.typeck_results.borrow().node_type_opt(*hir_id)
{
let pat_ty = self.resolve_vars_if_possible(pat_ty);
if self.can_coerce(pat_ty, expected_ty)
&& !(pat_ty, expected_ty).references_error()
&& shadowed.insert(ident.name)
{
candidate_idents.push((*ident, pat_ty));
}
}
true
};

let hir = self.tcx.hir();
for stmt in blk.stmts.iter().rev() {
let StmtKind::Local(local) = &stmt.kind else { continue; };
local.pat.walk(&mut find_compatible_candidates);
}
match hir.find(hir.get_parent_node(blk.hir_id)) {
Some(hir::Node::Expr(hir::Expr { hir_id, .. })) => {
match hir.find(hir.get_parent_node(*hir_id)) {
Some(hir::Node::Arm(hir::Arm { pat, .. })) => {
pat.walk(&mut find_compatible_candidates);
}
Some(
hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(_, _, body), .. })
| hir::Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Fn(_, body),
..
})
| hir::Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(body)),
..
})
| hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::Closure(hir::Closure { body, .. }),
..
}),
) => {
for param in hir.body(*body).params {
param.pat.walk(&mut find_compatible_candidates);
}
}
Some(hir::Node::Expr(hir::Expr {
kind:
hir::ExprKind::If(
hir::Expr { kind: hir::ExprKind::Let(let_), .. },
then_block,
_,
),
..
})) if then_block.hir_id == *hir_id => {
let_.pat.walk(&mut find_compatible_candidates);
}
_ => {}
}
}
_ => {}
}

match &candidate_idents[..] {
[(ident, _ty)] => {
let sm = self.tcx.sess.source_map();
if let Some(stmt) = blk.stmts.last() {
let stmt_span = sm.stmt_span(stmt.span, blk.span);
let sugg = if sm.is_multiline(blk.span)
&& let Some(spacing) = sm.indentation_before(stmt_span)
{
format!("\n{spacing}{ident}")
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
} else {
format!(" {ident}")
};
err.span_suggestion_verbose(
stmt_span.shrink_to_hi(),
format!("consider returning the local binding `{ident}`"),
sugg,
Applicability::MachineApplicable,
);
} else {
let sugg = if sm.is_multiline(blk.span)
&& let Some(spacing) = sm.indentation_before(blk.span.shrink_to_lo())
{
format!("\n{spacing} {ident}\n{spacing}")
} else {
format!(" {ident} ")
};
let left_span = sm.span_through_char(blk.span, '{').shrink_to_hi();
err.span_suggestion_verbose(
sm.span_extend_while(left_span, |c| c.is_whitespace()).unwrap_or(left_span),
format!("consider returning the local binding `{ident}`"),
sugg,
Applicability::MachineApplicable,
);
}
}
values if (1..3).contains(&values.len()) => {
let spans = values.iter().map(|(ident, _)| ident.span).collect::<Vec<_>>();
err.span_note(spans, "consider returning one of these bindings");
}
_ => {}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ LL | | break 0u8;
LL | | };
| |_________- enclosing `async` block

error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
--> $DIR/async-block-control-flow-static-semantics.rs:26:39
|
LL | let _: &dyn Future<Output = ()> = &block;
| ^^^^^^ expected `()`, found `u8`
|
= note: required for the cast from `impl Future<Output = u8>` to the object type `dyn Future<Output = ()>`

error[E0308]: mismatched types
--> $DIR/async-block-control-flow-static-semantics.rs:21:58
|
Expand All @@ -32,7 +40,7 @@ LL | | }
| |_^ expected `u8`, found `()`

error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
--> $DIR/async-block-control-flow-static-semantics.rs:26:39
--> $DIR/async-block-control-flow-static-semantics.rs:17:39
|
LL | let _: &dyn Future<Output = ()> = &block;
| ^^^^^^ expected `()`, found `u8`
Expand All @@ -47,14 +55,6 @@ LL | fn return_targets_async_block_not_fn() -> u8 {
| |
| implicitly returns `()` as its body has no tail or `return` expression

error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
--> $DIR/async-block-control-flow-static-semantics.rs:17:39
|
LL | let _: &dyn Future<Output = ()> = &block;
| ^^^^^^ expected `()`, found `u8`
|
= note: required for the cast from `impl Future<Output = u8>` to the object type `dyn Future<Output = ()>`

error[E0308]: mismatched types
--> $DIR/async-block-control-flow-static-semantics.rs:47:44
|
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/liveness/liveness-forgot-ret.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ LL | fn f(a: isize) -> isize { if god_exists(a) { return 5; }; }
| - ^^^^^ expected `isize`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression
|
help: consider returning the local binding `a`
|
LL | fn f(a: isize) -> isize { if god_exists(a) { return 5; }; a }
| +

error: aborting due to previous error

Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/parser/issues/issue-33413.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ LL | fn f(*, a: u8) -> u8 {}
| - ^^ expected `u8`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression
|
help: consider returning the local binding `a`
|
LL | fn f(*, a: u8) -> u8 { a }
| +

error: aborting due to 2 previous errors

Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/suggestions/return-bindings-multi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
fn a(i: i32) -> i32 {
//~^ ERROR mismatched types
let j = 2i32;
}

fn b(i: i32, j: i32) -> i32 {}
//~^ ERROR mismatched types

fn main() {}
34 changes: 34 additions & 0 deletions src/test/ui/suggestions/return-bindings-multi.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
error[E0308]: mismatched types
--> $DIR/return-bindings-multi.rs:1:17
|
LL | fn a(i: i32) -> i32 {
| - ^^^ expected `i32`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression
|
note: consider returning one of these bindings
--> $DIR/return-bindings-multi.rs:1:6
|
LL | fn a(i: i32) -> i32 {
| ^
LL |
LL | let j = 2i32;
| ^

error[E0308]: mismatched types
--> $DIR/return-bindings-multi.rs:6:25
|
LL | fn b(i: i32, j: i32) -> i32 {}
| - ^^^ expected `i32`, found `()`
| |
| implicitly returns `()` as its body has no tail or `return` expression
|
note: consider returning one of these bindings
--> $DIR/return-bindings-multi.rs:6:6
|
LL | fn b(i: i32, j: i32) -> i32 {}
| ^ ^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
23 changes: 23 additions & 0 deletions src/test/ui/suggestions/return-bindings.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// run-rustfix

#![allow(unused)]

fn a(i: i32) -> i32 { i }
//~^ ERROR mismatched types

fn b(opt_str: Option<String>) {
let s: String = if let Some(s) = opt_str {
s
//~^ ERROR mismatched types
} else {
String::new()
};
}

fn c() -> Option<i32> {
//~^ ERROR mismatched types
let x = Some(1);
x
}

fn main() {}
21 changes: 21 additions & 0 deletions src/test/ui/suggestions/return-bindings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// run-rustfix

#![allow(unused)]

fn a(i: i32) -> i32 {}
//~^ ERROR mismatched types

fn b(opt_str: Option<String>) {
let s: String = if let Some(s) = opt_str {
//~^ ERROR mismatched types
} else {
String::new()
};
}

fn c() -> Option<i32> {
//~^ ERROR mismatched types
let x = Some(1);
}

fn main() {}
Loading