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

Point at method call when type annotations are needed #65951

Merged
merged 12 commits into from
Dec 14, 2019
Merged

Point at method call when type annotations are needed #65951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
12af256
Point at method call when type annotations are needed
estebank Oct 29, 2019
252773a
Deduplicate logic
estebank Oct 30, 2019
cca4b6d
Fix rebase
estebank Nov 16, 2019
33b0636
Use the appropriate number of type arguments in suggestion
estebank Dec 10, 2019
6b76d82
Move suggestion code to its own method
estebank Dec 10, 2019
5d1adbb
Use `generics_of` instead of incorrectly inspecting `FnSig` arguments
estebank Dec 10, 2019
2924d38
Add tests
estebank Dec 10, 2019
cc1ab3d
Suggest setting type param on function call
estebank Dec 10, 2019
860c7e4
missing test change, bad suggestion
estebank Dec 10, 2019
94ab9ec
Avoid invalid suggestion by checking the snippet in const fn call
estebank Dec 10, 2019
45550ef
Reuse existing HirId → DefId table
estebank Dec 11, 2019
da023c0
Add more context for type parameters
estebank Dec 12, 2019
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
1 change: 1 addition & 0 deletions src/librustc/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ use std::{cmp, fmt};
mod note;

mod need_type_info;
pub use need_type_info::TypeAnnotationNeeded;

pub mod nice_region_error;

Expand Down
177 changes: 152 additions & 25 deletions src/librustc/infer/error_reporting/need_type_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
use crate::hir::def::Namespace;
use crate::hir::def::{DefKind, Namespace};
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::symbol::kw;
use syntax_pos::Span;
use errors::{Applicability, DiagnosticBuilder};
use std::borrow::Cow;

use rustc_error_codes::*;

Expand All @@ -19,6 +21,7 @@ struct FindLocalByTypeVisitor<'a, 'tcx> {
found_arg_pattern: Option<&'tcx Pat>,
found_ty: Option<Ty<'tcx>>,
found_closure: Option<&'tcx ExprKind>,
found_method_call: Option<&'tcx Expr>,
}

impl<'a, 'tcx> FindLocalByTypeVisitor<'a, 'tcx> {
Expand All @@ -35,6 +38,7 @@ impl<'a, 'tcx> FindLocalByTypeVisitor<'a, 'tcx> {
found_arg_pattern: None,
found_ty: None,
found_closure: None,
found_method_call: None,
}
}

Expand Down Expand Up @@ -93,11 +97,12 @@ impl<'a, 'tcx> Visitor<'tcx> for FindLocalByTypeVisitor<'a, 'tcx> {
}

fn visit_expr(&mut self, expr: &'tcx Expr) {
if let (ExprKind::Closure(_, _fn_decl, _id, _sp, _), Some(_)) = (
&expr.kind,
self.node_matches_type(expr.hir_id),
) {
self.found_closure = Some(&expr.kind);
if self.node_matches_type(expr.hir_id).is_some() {
estebank marked this conversation as resolved.
Show resolved Hide resolved
match expr.kind {
ExprKind::Closure(..) => self.found_closure = Some(&expr.kind),
ExprKind::MethodCall(..) => self.found_method_call = Some(&expr),
_ => {}
}
}
intravisit::walk_expr(self, expr);
}
Expand All @@ -109,6 +114,7 @@ fn closure_return_type_suggestion(
err: &mut DiagnosticBuilder<'_>,
output: &FunctionRetTy,
body: &Body,
descr: &str,
name: &str,
ret: &str,
) {
Expand All @@ -132,7 +138,7 @@ fn closure_return_type_suggestion(
suggestion,
Applicability::HasPlaceholders,
);
err.span_label(span, InferCtxt::missing_type_msg(&name));
err.span_label(span, InferCtxt::missing_type_msg(&name, &descr));
}

/// Given a closure signature, return a `String` containing a list of all its argument types.
Expand All @@ -147,17 +153,42 @@ fn closure_args(fn_sig: &ty::PolyFnSig<'_>) -> String {
.unwrap_or_default()
}

pub enum TypeAnnotationNeeded {
E0282,
E0283,
E0284,
}

impl Into<errors::DiagnosticId> for TypeAnnotationNeeded {
fn into(self) -> errors::DiagnosticId {
syntax::diagnostic_used!(E0282);
syntax::diagnostic_used!(E0283);
syntax::diagnostic_used!(E0284);
errors::DiagnosticId::Error(match self {
Self::E0282 => "E0282".to_string(),
Self::E0283 => "E0283".to_string(),
Self::E0284 => "E0284".to_string(),
})
}
}

impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
pub fn extract_type_name(
&self,
ty: Ty<'tcx>,
highlight: Option<ty::print::RegionHighlightMode>,
) -> (String, Option<Span>) {
) -> (String, Option<Span>, Cow<'static, str>) {
if let ty::Infer(ty::TyVar(ty_vid)) = ty.kind {
let ty_vars = self.type_variables.borrow();
let var_origin = ty_vars.var_origin(ty_vid);
if let TypeVariableOriginKind::TypeParameterDefinition(name) = var_origin.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @estebank if we wanted to say more about where the type parameter came from, we would augment this enum with the DefId of the parameter instead of its name (I imagine we can extract the name -- as well as the context -- from that DefId)

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #67277 to track this

return (name.to_string(), Some(var_origin.span));
if name != kw::SelfUpper {
return (
name.to_string(),
Some(var_origin.span),
"type parameter".into(),
);
}
}
}

Expand All @@ -167,26 +198,27 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
printer.region_highlight_mode = highlight;
}
let _ = ty.print(printer);
(s, None)
(s, None, ty.prefix_string())
}

pub fn need_type_info_err(
&self,
body_id: Option<hir::BodyId>,
span: Span,
ty: Ty<'tcx>,
error_code: TypeAnnotationNeeded,
) -> DiagnosticBuilder<'tcx> {
let ty = self.resolve_vars_if_possible(&ty);
let (name, name_sp) = self.extract_type_name(&ty, None);
let (name, name_sp, descr) = self.extract_type_name(&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);
let ty_vars = self.type_variables.borrow();
let getter = move |ty_vid| {
if let TypeVariableOriginKind::TypeParameterDefinition(name) =
ty_vars.var_origin(ty_vid).kind {
let var_origin = ty_vars.var_origin(ty_vid);
if let TypeVariableOriginKind::TypeParameterDefinition(name) = var_origin.kind {
return Some(name.to_string());
}
None
Expand All @@ -210,6 +242,22 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// 3 | let _ = x.sum() as f64;
// | ^^^ cannot infer type for `S`
span
} else if let Some(
ExprKind::MethodCall(_, call_span, _),
) = local_visitor.found_method_call.map(|e| &e.kind) {
// Point at the call instead of the whole expression:
// error[E0284]: type annotations needed
// --> file.rs:2:5
// |
// 2 | vec![Ok(2)].into_iter().collect()?;
// | ^^^^^^^ cannot infer type
// |
// = note: cannot resolve `<_ as std::ops::Try>::Ok == _`
if span.contains(*call_span) {
*call_span
} else {
span
}
} else {
span
};
Expand Down Expand Up @@ -247,12 +295,11 @@ 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 mut err = struct_span_err!(
self.tcx.sess,
let error_code = error_code.into();
let mut err = self.tcx.sess.struct_span_err_with_code(
err_span,
E0282,
"type annotations needed{}",
ty_msg,
&format!("type annotations needed{}", ty_msg),
error_code,
);

let suffix = match local_visitor.found_ty {
Expand All @@ -267,6 +314,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
&mut err,
&decl.output,
&body,
&descr,
&name,
&ret,
);
Expand Down Expand Up @@ -334,6 +382,36 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
format!("consider giving this pattern {}", suffix)
};
err.span_label(pattern.span, msg);
} else if let Some(e) = local_visitor.found_method_call {
if let ExprKind::MethodCall(segment, ..) = &e.kind {
// Suggest specifiying type params or point out the return type of the call:
//
// error[E0282]: type annotations needed
// --> $DIR/type-annotations-needed-expr.rs:2:39
// |
// LL | let _ = x.into_iter().sum() as f64;
// | ^^^
// | |
// | cannot infer type for `S`
// | help: consider specifying the type argument in
// | the method call: `sum::<S>`
// |
// = note: type must be known at this point
//
// or
//
// error[E0282]: type annotations needed
// --> $DIR/issue-65611.rs:59:20
// |
// LL | let x = buffer.last().unwrap().0.clone();
// | -------^^^^--
// | | |
// | | cannot infer type for `T`
// | this method call resolves to `std::option::Option<&T>`
// |
// = note: type must be known at this point
self.annotate_method_call(segment, e, &mut err);
}
estebank marked this conversation as resolved.
Show resolved Hide resolved
}
// Instead of the following:
// error[E0282]: type annotations needed
Expand All @@ -351,37 +429,86 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// | ^^^ cannot infer type for `S`
// |
// = note: type must be known at this point
let span = name_sp.unwrap_or(span);
let span = name_sp.unwrap_or(err_span);
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));
err.span_label(span, InferCtxt::missing_type_msg(&name, &descr));
}

err
}

/// If the `FnSig` for the method call can be found and type arguments are identified as
/// needed, suggest annotating the call, otherwise point out the resulting type of the call.
fn annotate_method_call(
&self,
segment: &hir::ptr::P<hir::PathSegment>,
e: &Expr,
err: &mut DiagnosticBuilder<'_>,
) {
if let (Ok(snippet), Some(tables), None) = (
self.tcx.sess.source_map().span_to_snippet(segment.ident.span),
self.in_progress_tables,
&segment.args,
) {
let borrow = tables.borrow();
if let Some((DefKind::Method, did)) = borrow.type_dependent_def(e.hir_id) {
let generics = self.tcx.generics_of(did);
if !generics.params.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I was debating if this condition is sufficient. I decided it probably is, though we could also choose to be more selective.

One thing is that there won't always be a direct connection between the type parameters and the return type. e.g. it might be

fn foo<T: Iterator>() -> T::Item

but, then again, it's still true that specifying T would help to figure out the Item type.

Another is that there could be many parameters.

I could imagine searching through the substitutions for the call to detect cases (like collect) where the return type is directly linked to a parameter and limiting to that case, or highlighting which parameter specifies the return type in the case that there are many (e.g., maybe writing "try writing out the return type X, as in foo.bar::<_, _, X>()"

But it's probably "good enough" the way it is, tbh, those seem like "potential improvements". (And the best phrasing is sort of unclear anyway.)

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I did wonder is if we want to be careful around "defaults", though they are currently being phased out on functions:

fn foo<T, U = u64>()...

you could imagine not showing U in our suggestions. But nobody quite knows what the semantics of said defaults should be so it's probably "ok as is".

Copy link
Contributor Author

@estebank estebank Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions.
 --> file12.rs:1:11
  |
1 | fn foo<T, U = u64>() -> T {
  |           ^
  |
  = note: `#[deny(invalid_type_param_default)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #36887 <https://github.com/rust-lang/rust/issues/36887>

with #[allow(invalid_type_param_default)] we do something more reasonable already IMO:

error[E0282]: type annotations needed
 --> file12.rs:8:13
  |
8 |     let _ = foo();
  |         -   ^^^ cannot infer type for `T`
  |         |
  |         consider giving this pattern a type

For the Iterator::Item case, the output is not ideal, but not any worse than it already is:

error[E0282]: type annotations needed
 --> file12.rs:8:13
  |
8 |     let _ = foo();
  |             ^^^ cannot infer type for `T`

error[E0284]: type annotations needed
  --> file12.rs:13:15
   |
13 |     let _ = S.foo();
   |               ^^^ cannot infer type for `T`
   |
   = note: cannot resolve `<_ as std::iter::Iterator>::Item == _`

I'm inclined at leaving things as they are for now.

The last case is because we have a visitor looking at the method calls for something that resolves to T, but what we should be looking is a method call that resolves to <T as std::iter::Iterator>::Item instead (for this case).

Copy link
Contributor Author

@estebank estebank Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Another interesting case:

trait T {
    type A;
    fn foo(&self) -> Self::A {
        panic!()
    }
}
struct S<X>(std::marker::PhantomData<X>);

impl<X> T for S<X> {
   type A = X;
}

fn main() {
    S(std::marker::PhantomData).foo();
}
error[E0282]: type annotations needed
  --> file12.rs:14:5
   |
2  |     type A;
   |     ------- `<Self as T>::A` defined here
...
14 |     S(std::marker::PhantomData).foo();
   |     ^--------------------------------
   |     |
   |     this method call resolves to `<Self as T>::A`
   |     cannot infer type for type parameter `X`

err.span_suggestion(
segment.ident.span,
&format!(
"consider specifying the type argument{} in the method call",
if generics.params.len() > 1 {
"s"
} else {
""
},
),
format!("{}::<{}>", snippet, generics.params.iter()
.map(|p| p.name.to_string())
.collect::<Vec<String>>()
.join(", ")),
Applicability::HasPlaceholders,
);
} else {
let sig = self.tcx.fn_sig(did);
let bound_output = sig.output();
let output = bound_output.skip_binder();
err.span_label(e.span, &format!("this method call resolves to `{:?}`", output));
let kind = &output.kind;
if let ty::Projection(proj) | ty::UnnormalizedProjection(proj) = kind {
if let Some(span) = self.tcx.hir().span_if_local(proj.item_def_id) {
err.span_label(span, &format!("`{:?}` defined here", output));
}
}
}
}
}
}

pub fn need_type_info_err_in_generator(
&self,
kind: hir::GeneratorKind,
span: Span,
ty: Ty<'tcx>,
) -> DiagnosticBuilder<'tcx> {
let ty = self.resolve_vars_if_possible(&ty);
let name = self.extract_type_name(&ty, None).0;
let (name, _, descr) = self.extract_type_name(&ty, None);
let mut err = struct_span_err!(
self.tcx.sess, span, E0698, "type inside {} must be known in this context", kind,
);
err.span_label(span, InferCtxt::missing_type_msg(&name));
err.span_label(span, InferCtxt::missing_type_msg(&name, &descr));
err
}

fn missing_type_msg(type_name: &str) -> String {
fn missing_type_msg(type_name: &str, descr: &str) -> Cow<'static, str>{
if type_name == "_" {
"cannot infer type".to_owned()
"cannot infer type".into()
} else {
format!("cannot infer type for `{}`", type_name)
format!("cannot infer type for {} `{}`", descr, type_name).into()
}
}
}
Loading