Skip to content

Move E0101 and E0102 logic into new E0282 mechanism #40013 #41236

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

Merged
bors merged 3 commits into from
Apr 19, 2017
Merged

Move E0101 and E0102 logic into new E0282 mechanism #40013 #41236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6383de1
fixes #40013
Apr 11, 2017
8b45a21
comment out removed error codes
Apr 18, 2017
3092ac4
change note for patterns
Apr 18, 2017
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
2 changes: 2 additions & 0 deletions src/librustc/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1812,6 +1812,8 @@ makes a difference in practice.)

register_diagnostics! {
// E0006 // merged with E0005
// E0101, // replaced with E0282
// E0102, // replaced with E0282
// E0134,
// E0135,
E0278, // requirement is not satisfied
Expand Down
125 changes: 84 additions & 41 deletions src/librustc/traits/error_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>,
Copy link
Contributor

@arielb1 arielb1 Apr 17, 2017 •
edited
Loading

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 and found_arg_pattern? I'll have a single found_local enum.

Copy link
Contributor Author

@cengiz-io cengiz-io Apr 17, 2017

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?

Copy link
Contributor

@arielb1 arielb1 Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

enum TypeVariableHolder<'gcx> {
    // later, TypeParam(...),
    Argument(&'gcx Pat),
    Local(&'gcx Pat),
}

struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { 
    ...,
    found_variable_holder: Option<TypeVariableHolder<'gcx>>
}

And find the maximum relevant TypeVariableHolder.

Copy link
Contributor

@nikomatsakis nikomatsakis Apr 17, 2017

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.

Copy link
Contributor

@arielb1 arielb1 Apr 18, 2017

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.

Copy link
Contributor

@nikomatsakis nikomatsakis Apr 18, 2017

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.

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);
}
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}

Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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>) {
Copy link
Contributor

@nikomatsakis nikomatsakis Apr 12, 2017

Choose a reason for hiding this comment

The 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 error_reporting/need_type_info.rs

Copy link
Contributor Author

@cengiz-io cengiz-io Apr 13, 2017

Choose a reason for hiding this comment

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

Sounds good Will do as soon as this lands

Copy link
Contributor Author

@cengiz-io cengiz-io Apr 13, 2017

Choose a reason for hiding this comment

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

#41284

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))];
Copy link
Contributor

@arielb1 arielb1 Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@cengiz-io cengiz-io Apr 17, 2017

Choose a reason for hiding this comment

The 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.
#38812

Copy link
Contributor

@arielb1 arielb1 Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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,

E101: unable to infer the type of the local `x`
let x = vec![];
//   ^ `x` can only be resolved to `Vec<_>`

Copy link
Contributor

@nikomatsakis nikomatsakis Apr 17, 2017

Choose a reason for hiding this comment

The 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();
}

Expand Down
Loading