Skip to content

Explain why a closure is FnOnce in closure errors. #42196

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

Merged
merged 5 commits into from
May 31, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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
6 changes: 5 additions & 1 deletion src/librustc/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1682,7 +1682,11 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
{
if let InferTables::InProgress(tables) = self.tables {
if let Some(id) = self.tcx.hir.as_local_node_id(def_id) {
return tables.borrow().closure_kinds.get(&id).cloned();
return tables.borrow()
.closure_kinds
.get(&id)
.cloned()
.map(|(kind, _)| kind);
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ use syntax::abi;
use syntax::ast::{self, Name, NodeId};
use syntax::attr;
use syntax::symbol::{Symbol, keywords};
use syntax_pos::Span;

use hir;

Expand Down Expand Up @@ -229,8 +230,9 @@ pub struct TypeckTables<'tcx> {
/// Records the type of each closure.
pub closure_tys: NodeMap<ty::PolyFnSig<'tcx>>,

/// Records the kind of each closure.
pub closure_kinds: NodeMap<ty::ClosureKind>,
/// Records the kind of each closure and the span of the variable that
/// cause the closure to be this kind.
pub closure_kinds: NodeMap<(ty::ClosureKind, Option<Span>)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think what we want is to include the span of a variable, but perhaps more than that we would prefer to include the name as well, for later use in the error message (including just the node-id of the reference could suffice, but I'm a bit wary of including a node-id in this value, since it will make life harder later as we handle incremental etc).


/// For each fn, records the "liberated" types of its arguments
/// and return type. Liberated means that all bound regions
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,9 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
if let Ok(ty::ClosureKind::FnOnce) =
ty::queries::closure_kind::try_get(self.tcx, DUMMY_SP, id) {
err.help("closure was moved because it only implements `FnOnce`");
if let Some(&(_kind, Some(span))) = self.tables.closure_kinds.get( ) {
Copy link
Contributor Author

@tommyip tommyip May 24, 2017

Choose a reason for hiding this comment

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

How do you find the node_id here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be the result of self.tcx.hir.as_local_node_id(id), I think -- we're basically looking things up based on the id of the closure. However, what we might want to do is just to include this information in the result of closure_kind::try_get() above, or else make a separate query.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also, @eddyb, why is this try_get() anyhow? That seems...dubious to me?)

err.span_label(span, "move occured here");
}
false
} else {
true
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
self.tables.borrow_mut().closure_tys.insert(expr.id, sig);
match opt_kind {
Some(kind) => {
self.tables.borrow_mut().closure_kinds.insert(expr.id, kind);
self.tables.borrow_mut().closure_kinds.insert(expr.id, (kind, None));
}
None => {}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ impl<'a, 'gcx, 'tcx> ProbeContext<'a, 'gcx, 'tcx> {

let closure_kinds = &self.tables.borrow().closure_kinds;
let closure_kind = match closure_kinds.get(&closure_id) {
Some(&k) => k,
Some(&(k, _)) => k,
None => {
return Err(MethodError::ClosureAmbiguity(trait_def_id));
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ fn closure_kind<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId)
-> ty::ClosureKind {
let node_id = tcx.hir.as_local_node_id(def_id).unwrap();
tcx.typeck_tables_of(def_id).closure_kinds[&node_id]
tcx.typeck_tables_of(def_id).closure_kinds[&node_id].0
}

fn adt_destructor<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
Expand Down
36 changes: 23 additions & 13 deletions src/librustc_typeck/check/upvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

struct SeedBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
temp_closure_kinds: NodeMap<ty::ClosureKind>,
temp_closure_kinds: NodeMap<(ty::ClosureKind, Option<Span>)>,
}

impl<'a, 'gcx, 'tcx> Visitor<'gcx> for SeedBorrowKind<'a, 'gcx, 'tcx> {
Expand Down Expand Up @@ -107,7 +107,7 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> {
capture_clause: hir::CaptureClause)
{
if !self.fcx.tables.borrow().closure_kinds.contains_key(&expr.id) {
self.temp_closure_kinds.insert(expr.id, ty::ClosureKind::Fn);
self.temp_closure_kinds.insert(expr.id, (ty::ClosureKind::Fn, None));
debug!("check_closure: adding closure {:?} as Fn", expr.id);
}

Expand Down Expand Up @@ -143,12 +143,12 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> {

struct AdjustBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
temp_closure_kinds: NodeMap<ty::ClosureKind>,
temp_closure_kinds: NodeMap<(ty::ClosureKind, Option<Span>)>,
}

impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
fn new(fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
temp_closure_kinds: NodeMap<ty::ClosureKind>)
temp_closure_kinds: NodeMap<(ty::ClosureKind, Option<Span>)>)
-> AdjustBorrowKind<'a, 'gcx, 'tcx> {
AdjustBorrowKind { fcx: fcx, temp_closure_kinds: temp_closure_kinds }
}
Expand Down Expand Up @@ -211,8 +211,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {

// If we are also inferred the closure kind here, update the
// main table and process any deferred resolutions.
if let Some(&kind) = self.temp_closure_kinds.get(&id) {
self.fcx.tables.borrow_mut().closure_kinds.insert(id, kind);
if let Some(&(kind, span)) = self.temp_closure_kinds.get(&id) {
self.fcx.tables.borrow_mut().closure_kinds.insert(id, (kind, span));
let closure_def_id = self.fcx.tcx.hir.local_def_id(id);
debug!("closure_kind({:?}) = {:?}", closure_def_id, kind);

Expand Down Expand Up @@ -276,6 +276,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
// for that to be legal, the upvar would have to be borrowed
// by value instead
let guarantor = cmt.guarantor();
let tcx = self.fcx.tcx;
debug!("adjust_upvar_borrow_kind_for_consume: guarantor={:?}",
guarantor);
match guarantor.cat {
Expand All @@ -289,7 +290,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {

// to move out of an upvar, this must be a FnOnce closure
self.adjust_closure_kind(upvar_id.closure_expr_id,
ty::ClosureKind::FnOnce);
ty::ClosureKind::FnOnce,
tcx.hir.span(upvar_id.var_id));
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 I would just use guarantor.span here.


let upvar_capture_map =
&mut self.fcx.tables.borrow_mut().upvar_capture_map;
Expand All @@ -303,7 +305,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
// to be a FnOnce closure to permit moves out
// of the environment.
self.adjust_closure_kind(upvar_id.closure_expr_id,
ty::ClosureKind::FnOnce);
ty::ClosureKind::FnOnce,
tcx.hir.span(upvar_id.var_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, guarantor.span seems good.

}
mc::NoteNone => {
}
Expand Down Expand Up @@ -394,6 +397,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
ty::ImmBorrow => false,
});

let tcx = self.fcx.tcx;

match *note {
mc::NoteUpvarRef(upvar_id) => {
// if this is an implicit deref of an
Expand All @@ -407,15 +412,19 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
}

// also need to be in an FnMut closure since this is not an ImmBorrow
self.adjust_closure_kind(upvar_id.closure_expr_id, ty::ClosureKind::FnMut);
self.adjust_closure_kind(upvar_id.closure_expr_id,
ty::ClosureKind::FnMut,
tcx.hir.span(upvar_id.var_id));

true
}
mc::NoteClosureEnv(upvar_id) => {
// this kind of deref occurs in a `move` closure, or
// for a by-value upvar; in either case, to mutate an
// upvar, we need to be an FnMut closure
self.adjust_closure_kind(upvar_id.closure_expr_id, ty::ClosureKind::FnMut);
self.adjust_closure_kind(upvar_id.closure_expr_id,
ty::ClosureKind::FnMut,
tcx.hir.span(upvar_id.var_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

here, in addition to passing in the &Note, we can pass in the cmt that the note originated from, and use that span. That should be the span of the variable reference.


true
}
Expand Down Expand Up @@ -462,11 +471,12 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {

fn adjust_closure_kind(&mut self,
closure_id: ast::NodeId,
new_kind: ty::ClosureKind) {
new_kind: ty::ClosureKind,
upvar_span: Span) {
debug!("adjust_closure_kind(closure_id={}, new_kind={:?})",
closure_id, new_kind);

if let Some(&existing_kind) = self.temp_closure_kinds.get(&closure_id) {
if let Some(&(existing_kind, _)) = self.temp_closure_kinds.get(&closure_id) {
debug!("adjust_closure_kind: closure_id={}, existing_kind={:?}, new_kind={:?}",
closure_id, existing_kind, new_kind);

Expand All @@ -482,7 +492,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
(ty::ClosureKind::Fn, ty::ClosureKind::FnOnce) |
(ty::ClosureKind::FnMut, ty::ClosureKind::FnOnce) => {
// new kind is stronger than the old kind
self.temp_closure_kinds.insert(closure_id, new_kind);
self.temp_closure_kinds.insert(closure_id, (new_kind, Some(upvar_span)));
}
}
}
Expand Down