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

When needing type annotations in local bindings, account for impl Trait and closures #63507

Merged
merged 7 commits into from
Aug 15, 2019
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
32 changes: 27 additions & 5 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -649,12 +649,34 @@ impl<'hir> Map<'hir> {
}
}

pub fn is_const_scope(&self, hir_id: HirId) -> bool {
self.walk_parent_nodes(hir_id, |node| match *node {
Node::Item(Item { node: ItemKind::Const(_, _), .. }) => true,
Node::Item(Item { node: ItemKind::Fn(_, header, _, _), .. }) => header.is_const(),
/// Whether the expression pointed at by `hir_id` belongs to a `const` evaluation context.
/// Used exclusively for diagnostics, to avoid suggestion function calls.
pub fn is_const_context(&self, hir_id: HirId) -> bool {
let parent_id = self.get_parent_item(hir_id);
match self.get(parent_id) {
Node::Item(&Item {
node: ItemKind::Const(..),
..
})
| Node::TraitItem(&TraitItem {
node: TraitItemKind::Const(..),
..
})
| Node::ImplItem(&ImplItem {
node: ImplItemKind::Const(..),
..
})
| Node::AnonConst(_)
| Node::Item(&Item {
node: ItemKind::Static(..),
..
}) => true,
Node::Item(&Item {
node: ItemKind::Fn(_, header, ..),
..
}) => header.constness == Constness::Const,
_ => false,
}, |_| false).map(|id| id != CRATE_HIR_ID).unwrap_or(false)
}
}

/// If there is some error when walking the parents (e.g., a node does not
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,7 @@ pub enum ExprKind {
Match(P<Expr>, HirVec<Arm>, MatchSource),
/// A closure (e.g., `move |a, b, c| {a + b + c}`).
///
/// The final span is the span of the argument block `|...|`.
/// The `Span` is the argument block `|...|`.
///
/// This may also be a generator literal or an `async block` as indicated by the
/// `Option<GeneratorMovability>`.
Expand Down
212 changes: 161 additions & 51 deletions src/librustc/infer/error_reporting/need_type_info.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use crate::hir::def::Namespace;
use crate::hir::{self, Local, Pat, Body, HirId};
use crate::hir::{self, Body, FunctionRetTy, Expr, ExprKind, HirId, Local, Pat};
use crate::hir::intravisit::{self, Visitor, NestedVisitorMap};
use crate::infer::InferCtxt;
use crate::infer::type_variable::TypeVariableOriginKind;
use crate::ty::{self, Ty, Infer, TyVar};
use crate::ty::print::Print;
use syntax::source_map::DesugaringKind;
use syntax_pos::Span;
use errors::DiagnosticBuilder;
use errors::{Applicability, DiagnosticBuilder};

struct FindLocalByTypeVisitor<'a, 'tcx> {
infcx: &'a InferCtxt<'a, 'tcx>,
Expand All @@ -16,9 +16,26 @@ struct FindLocalByTypeVisitor<'a, 'tcx> {
found_local_pattern: Option<&'tcx Pat>,
found_arg_pattern: Option<&'tcx Pat>,
found_ty: Option<Ty<'tcx>>,
found_closure: Option<&'tcx ExprKind>,
}

impl<'a, 'tcx> FindLocalByTypeVisitor<'a, 'tcx> {
fn new(
infcx: &'a InferCtxt<'a, 'tcx>,
target_ty: Ty<'tcx>,
hir_map: &'a hir::map::Map<'tcx>,
) -> Self {
Self {
infcx,
target_ty,
hir_map,
found_local_pattern: None,
found_arg_pattern: None,
found_ty: None,
found_closure: None,
}
}

fn node_matches_type(&mut self, hir_id: HirId) -> Option<Ty<'tcx>> {
let ty_opt = self.infcx.in_progress_tables.and_then(|tables| {
tables.borrow().node_type_opt(hir_id)
Expand Down Expand Up @@ -72,6 +89,60 @@ impl<'a, 'tcx> Visitor<'tcx> for FindLocalByTypeVisitor<'a, 'tcx> {
}
intravisit::walk_body(self, body);
}

fn visit_expr(&mut self, expr: &'tcx Expr) {
if let (ExprKind::Closure(_, _fn_decl, _id, _sp, _), Some(_)) = (
&expr.node,
self.node_matches_type(expr.hir_id),
) {
self.found_closure = Some(&expr.node);
}
intravisit::walk_expr(self, expr);
}
}

/// Suggest giving an appropriate return type to a closure expression.
fn closure_return_type_suggestion(
span: Span,
err: &mut DiagnosticBuilder<'_>,
output: &FunctionRetTy,
body: &Body,
name: &str,
ret: &str,
) {
let (arrow, post) = match output {
FunctionRetTy::DefaultReturn(_) => ("-> ", " "),
_ => ("", ""),
};
let suggestion = match body.value.node {
ExprKind::Block(..) => {
vec![(output.span(), format!("{}{}{}", arrow, ret, post))]
}
_ => {
vec![
(output.span(), format!("{}{}{}{{ ", arrow, ret, post)),
(body.value.span.shrink_to_hi(), " }".to_string()),
]
}
};
err.multipart_suggestion(
"give this closure an explicit return type without `_` placeholders",
suggestion,
Applicability::HasPlaceholders,
);
err.span_label(span, InferCtxt::missing_type_msg(&name));
}

/// Given a closure signature, return a `String` containing a list of all its argument types.
fn closure_args(fn_sig: &ty::PolyFnSig<'_>) -> String {
fn_sig.inputs()
.skip_binder()
.iter()
.next()
.map(|args| args.tuple_fields()
.map(|arg| arg.to_string())
.collect::<Vec<_>>().join(", "))
.unwrap_or_default()
}

impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
Expand Down Expand Up @@ -106,16 +177,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let ty = self.resolve_vars_if_possible(&ty);
let name = self.extract_type_name(&ty, None);

let mut err_span = span;

let mut local_visitor = FindLocalByTypeVisitor {
infcx: &self,
target_ty: ty,
hir_map: &self.tcx.hir(),
found_local_pattern: None,
found_arg_pattern: None,
found_ty: None,
};
let mut local_visitor = FindLocalByTypeVisitor::new(&self, ty, &self.tcx.hir());
let ty_to_string = |ty: Ty<'tcx>| -> String {
let mut s = String::new();
let mut printer = ty::print::FmtPrinter::new(self.tcx, &mut s, Namespace::TypeNS);
Expand All @@ -136,6 +198,31 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let expr = self.tcx.hir().expect_expr(body_id.hir_id);
local_visitor.visit_expr(expr);
}
let err_span = if let Some(pattern) = local_visitor.found_arg_pattern {
estebank marked this conversation as resolved.
Show resolved Hide resolved
pattern.span
} else {
span
};

let is_named_and_not_impl_trait = |ty: Ty<'_>| {
&ty.to_string() != "_" &&
// FIXME: Remove this check after `impl_trait_in_bindings` is stabilized. #63527
(!ty.is_impl_trait() || self.tcx.features().impl_trait_in_bindings)
};

let ty_msg = match local_visitor.found_ty {
estebank marked this conversation as resolved.
Show resolved Hide resolved
Some(ty::TyS { sty: ty::Closure(def_id, substs), .. }) => {
let fn_sig = substs.closure_sig(*def_id, self.tcx);
let args = closure_args(&fn_sig);
let ret = fn_sig.output().skip_binder().to_string();
format!(" for the closure `fn({}) -> {}`", args, ret)
}
Some(ty) if is_named_and_not_impl_trait(ty) => {
let ty = ty_to_string(ty);
format!(" for `{}`", ty)
}
_ => String::new(),
};

// When `name` corresponds to a type argument, show the path of the full type we're
// trying to infer. In the following example, `ty_msg` contains
Expand All @@ -150,27 +237,58 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// | consider giving `b` the explicit type `std::result::Result<i32, E>`, where
// | the type parameter `E` is specified
// ```
let (ty_msg, suffix) = match &local_visitor.found_ty {
Some(ty) if &ty.to_string() != "_" && name == "_" => {
let mut err = struct_span_err!(
self.tcx.sess,
err_span,
E0282,
"type annotations needed{}",
ty_msg,
);

let suffix = match local_visitor.found_ty {
Some(ty::TyS { sty: ty::Closure(def_id, substs), .. }) => {
let fn_sig = substs.closure_sig(*def_id, self.tcx);
let ret = fn_sig.output().skip_binder().to_string();

if let Some(ExprKind::Closure(_, decl, body_id, ..)) = local_visitor.found_closure {
if let Some(body) = self.tcx.hir().krate().bodies.get(body_id) {
closure_return_type_suggestion(
span,
&mut err,
&decl.output,
&body,
&name,
&ret,
);
// We don't want to give the other suggestions when the problem is the
// closure return type.
return err;
}
}

// This shouldn't be reachable, but just in case we leave a reasonable fallback.
estebank marked this conversation as resolved.
Show resolved Hide resolved
let args = closure_args(&fn_sig);
// This suggestion is incomplete, as the user will get further type inference
// errors due to the `_` placeholders and the introduction of `Box`, but it does
// nudge them in the right direction.
format!("a boxed closure type like `Box<dyn Fn({}) -> {}>`", args, ret)
}
Some(ty) if is_named_and_not_impl_trait(ty) && name == "_" => {
let ty = ty_to_string(ty);
(format!(" for `{}`", ty),
format!("the explicit type `{}`, with the type parameters specified", ty))
format!("the explicit type `{}`, with the type parameters specified", ty)
}
Some(ty) if &ty.to_string() != "_" && ty.to_string() != name => {
Some(ty) if is_named_and_not_impl_trait(ty) && ty.to_string() != name => {
let ty = ty_to_string(ty);
(format!(" for `{}`", ty),
format!(
"the explicit type `{}`, where the type parameter `{}` is specified",
format!(
"the explicit type `{}`, where the type parameter `{}` is specified",
ty,
name,
))
)
}
_ => (String::new(), "a type".to_owned()),
_ => "a type".to_string(),
};
let mut labels = vec![(span, InferCtxt::missing_type_msg(&name))];

if let Some(pattern) = local_visitor.found_arg_pattern {
err_span = pattern.span;
// We don't want to show the default label for closures.
//
// So, before clearing, the output would look something like this:
Expand All @@ -187,39 +305,31 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// ^ consider giving this closure parameter the type `[_; 0]`
// with the type parameter `_` specified
// ```
labels.clear();
labels.push((
err.span_label(
pattern.span,
format!("consider giving this closure parameter {}", suffix),
));
);
} else if let Some(pattern) = local_visitor.found_local_pattern {
if let Some(simple_ident) = pattern.simple_ident() {
let msg = if let Some(simple_ident) = pattern.simple_ident() {
match pattern.span.desugaring_kind() {
None => labels.push((
pattern.span,
format!("consider giving `{}` {}", simple_ident, suffix),
)),
Some(DesugaringKind::ForLoop) => labels.push((
pattern.span,
"the element type for this iterator is not specified".to_owned(),
)),
_ => {}
None => {
format!("consider giving `{}` {}", simple_ident, suffix)
}
Some(DesugaringKind::ForLoop) => {
"the element type for this iterator is not specified".to_string()
}
_ => format!("this needs {}", suffix),
}
} else {
labels.push((pattern.span, format!("consider giving this pattern {}", suffix)));
}
};

let mut err = struct_span_err!(
self.tcx.sess,
err_span,
E0282,
"type annotations needed{}",
ty_msg,
);

for (target_span, label_message) in labels {
err.span_label(target_span, label_message);
format!("consider giving this pattern {}", suffix)
};
err.span_label(pattern.span, msg);
}
if !err.span.span_labels().iter().any(|span_label| {
span_label.label.is_some() && span_label.span == span
}) && local_visitor.found_arg_pattern.is_none()
{ // Avoid multiple labels pointing at `span`.
err.span_label(span, InferCtxt::missing_type_msg(&name));
estebank marked this conversation as resolved.
Show resolved Hide resolved
}

err
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2068,6 +2068,9 @@ impl<'tcx> TyS<'tcx> {
Error => { // ignore errors (#54954)
ty::Binder::dummy(FnSig::fake())
}
Closure(..) => bug!(
"to get the signature of a closure, use `closure_sig()` not `fn_sig()`",
),
_ => bug!("Ty::fn_sig() called on non-fn type: {:?}", self)
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_typeck/check/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

self.suggest_compatible_variants(&mut err, expr, expected, expr_ty);
self.suggest_ref_or_into(&mut err, expr, expected, expr_ty);
self.suggest_boxing_when_appropriate(&mut err, expr, expected, expr_ty);
self.suggest_missing_await(&mut err, expr, expected, expr_ty);

(expected, Some(err))
Expand Down Expand Up @@ -548,7 +549,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
checked_ty: Ty<'tcx>,
expected_ty: Ty<'tcx>,
) -> bool {
if self.tcx.hir().is_const_scope(expr.hir_id) {
if self.tcx.hir().is_const_context(expr.hir_id) {
// Shouldn't suggest `.into()` on `const`s.
// FIXME(estebank): modify once we decide to suggest `as` casts
return false;
Expand Down
Loading