-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Move E0101 and E0102 logic into new E0282 mechanism #40013 #41236
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,16 +25,16 @@ use super::{ | |
|
||
use errors::DiagnosticBuilder; | ||
use fmt_macros::{Parser, Piece, Position}; | ||
use hir::{intravisit, Local, Pat}; | ||
use hir::{self, intravisit, Local, Pat, Body}; | ||
use hir::intravisit::{Visitor, NestedVisitorMap}; | ||
use hir::map::NodeExpr; | ||
use hir::def_id::DefId; | ||
use infer::{self, InferCtxt}; | ||
use infer::type_variable::TypeVariableOrigin; | ||
use rustc::lint::builtin::EXTRA_REQUIREMENT_IN_IMPL; | ||
use std::fmt; | ||
use syntax::ast; | ||
use ty::{self, AdtKind, ToPredicate, ToPolyTraitRef, Ty, TyCtxt, TypeFoldable}; | ||
use syntax::ast::{self, NodeId}; | ||
use ty::{self, AdtKind, ToPredicate, ToPolyTraitRef, Ty, TyCtxt, TypeFoldable, TyInfer, TyVar}; | ||
use ty::error::ExpectedFound; | ||
use ty::fast_reject; | ||
use ty::fold::TypeFolder; | ||
|
@@ -66,37 +66,52 @@ impl<'a, 'gcx, 'tcx> TraitErrorKey<'tcx> { | |
struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { | ||
infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, | ||
target_ty: &'a Ty<'tcx>, | ||
found_pattern: Option<&'a Pat>, | ||
hir_map: &'a hir::map::Map<'gcx>, | ||
found_local_pattern: Option<&'gcx Pat>, | ||
found_arg_pattern: Option<&'gcx Pat>, | ||
} | ||
|
||
impl<'a, 'gcx, 'tcx> FindLocalByTypeVisitor<'a, 'gcx, 'tcx> { | ||
fn is_match(&self, ty: Ty<'tcx>) -> bool { | ||
ty == *self.target_ty || match (&ty.sty, &self.target_ty.sty) { | ||
(&ty::TyInfer(ty::TyVar(a_vid)), &ty::TyInfer(ty::TyVar(b_vid))) => | ||
self.infcx.type_variables | ||
.borrow_mut() | ||
.sub_unified(a_vid, b_vid), | ||
|
||
fn node_matches_type(&mut self, node_id: &'gcx NodeId) -> bool { | ||
match self.infcx.tables.borrow().node_types.get(node_id) { | ||
Some(&ty) => { | ||
let ty = self.infcx.resolve_type_vars_if_possible(&ty); | ||
ty.walk().any(|inner_ty| { | ||
inner_ty == *self.target_ty || match (&inner_ty.sty, &self.target_ty.sty) { | ||
(&TyInfer(TyVar(a_vid)), &TyInfer(TyVar(b_vid))) => { | ||
self.infcx | ||
.type_variables | ||
.borrow_mut() | ||
.sub_unified(a_vid, b_vid) | ||
} | ||
_ => false, | ||
} | ||
}) | ||
} | ||
_ => false, | ||
} | ||
} | ||
} | ||
|
||
impl<'a, 'gcx, 'tcx> Visitor<'a> for FindLocalByTypeVisitor<'a, 'gcx, 'tcx> { | ||
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'a> { | ||
NestedVisitorMap::None | ||
impl<'a, 'gcx, 'tcx> Visitor<'gcx> for FindLocalByTypeVisitor<'a, 'gcx, 'tcx> { | ||
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'gcx> { | ||
NestedVisitorMap::OnlyBodies(&self.hir_map) | ||
} | ||
|
||
fn visit_local(&mut self, local: &'a Local) { | ||
if let Some(&ty) = self.infcx.tables.borrow().node_types.get(&local.id) { | ||
let ty = self.infcx.resolve_type_vars_if_possible(&ty); | ||
let is_match = ty.walk().any(|t| self.is_match(t)); | ||
fn visit_local(&mut self, local: &'gcx Local) { | ||
if self.found_local_pattern.is_none() && self.node_matches_type(&local.id) { | ||
self.found_local_pattern = Some(&*local.pat); | ||
} | ||
intravisit::walk_local(self, local); | ||
} | ||
|
||
if is_match && self.found_pattern.is_none() { | ||
self.found_pattern = Some(&*local.pat); | ||
fn visit_body(&mut self, body: &'gcx Body) { | ||
for argument in &body.arguments { | ||
if self.found_arg_pattern.is_none() && self.node_matches_type(&argument.id) { | ||
self.found_arg_pattern = Some(&*argument.pat); | ||
} | ||
} | ||
intravisit::walk_local(self, local); | ||
intravisit::walk_body(self, body); | ||
} | ||
} | ||
|
||
|
@@ -721,6 +736,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
// coherence violation, so we don't report it here. | ||
|
||
let predicate = self.resolve_type_vars_if_possible(&obligation.predicate); | ||
let body_id = hir::BodyId { node_id: obligation.cause.body_id }; | ||
let span = obligation.cause.span; | ||
|
||
debug!("maybe_report_ambiguity(predicate={:?}, obligation={:?})", | ||
predicate, | ||
|
@@ -768,10 +785,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
self.tcx.lang_items.sized_trait() | ||
.map_or(false, |sized_id| sized_id == trait_ref.def_id()) | ||
{ | ||
self.need_type_info(obligation, self_ty); | ||
self.need_type_info(body_id, span, self_ty); | ||
} else { | ||
let mut err = struct_span_err!(self.tcx.sess, | ||
obligation.cause.span, E0283, | ||
span, E0283, | ||
"type annotations required: \ | ||
cannot resolve `{}`", | ||
predicate); | ||
|
@@ -785,7 +802,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
// Same hacky approach as above to avoid deluging user | ||
// with error messages. | ||
if !ty.references_error() && !self.tcx.sess.has_errors() { | ||
self.need_type_info(obligation, ty); | ||
self.need_type_info(body_id, span, ty); | ||
} | ||
} | ||
|
||
|
@@ -796,7 +813,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
let &SubtypePredicate { a_is_expected: _, a, b } = data.skip_binder(); | ||
// both must be type variables, or the other would've been instantiated | ||
assert!(a.is_ty_var() && b.is_ty_var()); | ||
self.need_type_info(obligation, a); | ||
self.need_type_info(hir::BodyId { node_id: obligation.cause.body_id }, | ||
obligation.cause.span, | ||
a); | ||
} | ||
} | ||
|
||
|
@@ -874,42 +893,66 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
} | ||
} | ||
|
||
fn need_type_info(&self, obligation: &PredicateObligation<'tcx>, ty: Ty<'tcx>) { | ||
pub fn need_type_info(&self, body_id: hir::BodyId, span: Span, ty: Ty<'tcx>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit for future PR: feels like this function (and its associated visitor etc) would be ripe for promotion into a sub-module There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good Will do as soon as this lands There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
let ty = self.resolve_type_vars_if_possible(&ty); | ||
let name = self.extract_type_name(&ty); | ||
let ref cause = obligation.cause; | ||
|
||
let mut err = struct_span_err!(self.tcx.sess, | ||
cause.span, | ||
E0282, | ||
"type annotations needed"); | ||
|
||
err.span_label(cause.span, &format!("cannot infer type for `{}`", name)); | ||
let mut err_span = span; | ||
let mut labels = vec![(span, format!("cannot infer type for `{}`", name))]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we ever need this message? I think a local with inference types in it is always an error at this stage. I think that if we ever find a local/argument with inference variables in it we should have E0101 with only that local spanned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we do. That was decided long time ago in a different issue + PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing that would be nice is to always use the type of the local,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I feel like it'd be nice to say what we do now of the type, but we never did settle on the precise format we wanted for it. (The discussion was in #39361) |
||
|
||
let mut local_visitor = FindLocalByTypeVisitor { | ||
infcx: &self, | ||
target_ty: &ty, | ||
found_pattern: None, | ||
hir_map: &self.tcx.hir, | ||
found_local_pattern: None, | ||
found_arg_pattern: None, | ||
}; | ||
|
||
// #40294: cause.body_id can also be a fn declaration. | ||
// Currently, if it's anything other than NodeExpr, we just ignore it | ||
match self.tcx.hir.find(cause.body_id) { | ||
match self.tcx.hir.find(body_id.node_id) { | ||
Some(NodeExpr(expr)) => local_visitor.visit_expr(expr), | ||
_ => () | ||
} | ||
|
||
if let Some(pattern) = local_visitor.found_pattern { | ||
let pattern_span = pattern.span; | ||
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: | ||
// ``` | ||
// let x = |_| { }; | ||
// - ^^^^ cannot infer type for `[_; 0]` | ||
// | | ||
// consider giving this closure parameter a type | ||
// ``` | ||
// | ||
// After clearing, it looks something like this: | ||
// ``` | ||
// let x = |_| { }; | ||
// ^ consider giving this closure parameter a type | ||
// ``` | ||
labels.clear(); | ||
labels.push((pattern.span, format!("consider giving this closure parameter a type"))); | ||
} | ||
|
||
if let Some(pattern) = local_visitor.found_local_pattern { | ||
if let Some(simple_name) = pattern.simple_name() { | ||
err.span_label(pattern_span, | ||
&format!("consider giving `{}` a type", | ||
simple_name)); | ||
labels.push((pattern.span, format!("consider giving `{}` a type", simple_name))); | ||
} else { | ||
err.span_label(pattern_span, &format!("consider giving a type to pattern")); | ||
labels.push((pattern.span, format!("consider giving the pattern a type"))); | ||
} | ||
} | ||
|
||
let mut err = struct_span_err!(self.tcx.sess, | ||
err_span, | ||
E0282, | ||
"type annotations needed"); | ||
|
||
for (target_span, label_message) in labels { | ||
err.span_label(target_span, &label_message); | ||
} | ||
|
||
err.emit(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have separate options for
found_local_pattern
andfound_arg_pattern
? I'll have a singlefound_local
enum.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not only holding target patterns, but also changing the flow of the result.
Can you share your suggestion with a concrete example please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And find the maximum relevant
TypeVariableHolder
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like it makes much difference either way to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's mainly something I want to build on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I would be fine landing as is, but also fine with adopting the single enum approach -- I imagine as we add more cases, we'll probably want a single enum, as you suggest. (Though I was wondering if it may be the case that we can't detect until the end whether certain patterns are met, and hence we wouldn't be able to decide the final state until the end.) Either way, easy to evolve as needed.