From 9d188e680e5558af2842231e6c32a676afc0086b Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Mon, 5 Jun 2017 11:14:48 +0100 Subject: [PATCH 1/6] Better closure error message Use tracked data introduced in #42196 to provide a better closure error message by showing why a closure implements `FnOnce`. ``` error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` --> $DIR/issue_26046.rs:4:19 | 4 | let closure = move || { | ___________________^ 5 | | vec 6 | | }; | |_____^ | note: closure is `FnOnce` because it moves the variable `vec` out of its environment --> $DIR/issue_26046.rs:5:9 | 5 | vec | ^^^ error: aborting due to previous error(s) ``` Fixes #26046 --- src/librustc/traits/error_reporting.rs | 32 ++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index 049d5e488c9..e29e4786671 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -29,7 +29,7 @@ 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::{self, InferCtxt, InferTables, InferTablesRef}; use infer::type_variable::TypeVariableOrigin; use rustc::lint::builtin::EXTRA_REQUIREMENT_IN_IMPL; use std::fmt; @@ -640,16 +640,38 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { ty::Predicate::ClosureKind(closure_def_id, kind) => { let found_kind = self.closure_kind(closure_def_id).unwrap(); let closure_span = self.tcx.hir.span_if_local(closure_def_id).unwrap(); + let node_id = self.tcx.hir.as_local_node_id(closure_def_id).unwrap(); let mut err = struct_span_err!( self.tcx.sess, closure_span, E0525, "expected a closure that implements the `{}` trait, \ but this closure only implements `{}`", kind, found_kind); - err.span_note( - obligation.cause.span, - &format!("the requirement to implement \ - `{}` derives from here", kind)); + + let infer_tables = match self.tables { + InferTables::Interned(tables) => + Some(InferTablesRef::Interned(tables)), + InferTables::InProgress(tables) => + Some(InferTablesRef::InProgress(tables.borrow())), + InferTables::Missing => None, + }; + + if let Some(tables) = infer_tables { + if let Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) = + tables.closure_kinds.get(&node_id) + { + err.span_note( + span, + &format!("closure is `FnOnce` because it moves the \ + variable `{}` out of its environment", name)); + } + } else { + err.span_note( + obligation.cause.span, + &format!("the requirement to implement `{}` \ + derives from here", kind)); + } + err.emit(); return; } From 9cbb5f9a245f859c94d90acebef02adbc4a807e3 Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Mon, 5 Jun 2017 11:35:17 +0100 Subject: [PATCH 2/6] Add ui tests for issue 26046 --- src/test/ui/issue-26046.rs | 21 +++++++++++++++++++++ src/test/ui/issue-26046.stderr | 17 +++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 src/test/ui/issue-26046.rs create mode 100644 src/test/ui/issue-26046.stderr diff --git a/src/test/ui/issue-26046.rs b/src/test/ui/issue-26046.rs new file mode 100644 index 00000000000..de06de530c6 --- /dev/null +++ b/src/test/ui/issue-26046.rs @@ -0,0 +1,21 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn get_closure() -> Box Vec> { + let vec = vec![1u8, 2u8]; + + let closure = move || { + vec + }; + + Box::new(closure) +} + +fn main() {} diff --git a/src/test/ui/issue-26046.stderr b/src/test/ui/issue-26046.stderr new file mode 100644 index 00000000000..fa3681dc01f --- /dev/null +++ b/src/test/ui/issue-26046.stderr @@ -0,0 +1,17 @@ +error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` + --> $DIR/issue-26046.rs:14:19 + | +14 | let closure = move || { + | ___________________^ +15 | | vec +16 | | }; + | |_____^ + | +note: closure is `FnOnce` because it moves the variable `vec` out of its environment + --> $DIR/issue-26046.rs:15:9 + | +15 | vec + | ^^^ + +error: aborting due to previous error(s) + From 94c808c1d3fbf6f73b2bfdb8d6fbcad1d260fe64 Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Tue, 6 Jun 2017 09:06:56 +0100 Subject: [PATCH 3/6] Update closure errors to emit context for FnMut It now handles both FnMut and FnOnce case. --- src/librustc/traits/error_reporting.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index e29e4786671..73753382a7c 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -657,13 +657,18 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { }; if let Some(tables) = infer_tables { - if let Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) = - tables.closure_kinds.get(&node_id) - { - err.span_note( - span, - &format!("closure is `FnOnce` because it moves the \ - variable `{}` out of its environment", name)); + match tables.closure_kinds.get(&node_id) { + Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) => { + err.span_note(span, &format!( + "closure is `FnOnce` because it moves the \ + variable `{}` out of its environment", name)); + }, + Some(&(ty::ClosureKind::FnMut, Some((span, name)))) => { + err.span_note(span, &format!( + "closure is `FnMut` because it mutates the \ + variable `{}` here", name)); + }, + _ => {} } } else { err.span_note( From 2c282b8e5d0b592f72e9fd3c6038c8e2e80d6055 Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Tue, 6 Jun 2017 09:17:06 +0100 Subject: [PATCH 4/6] Add additional ui tests for issue 26046 This tests the FnMut case. --- src/test/ui/issue-26046-fn-mut.rs | 21 +++++++++++++++++++++ src/test/ui/issue-26046-fn-mut.stderr | 17 +++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 src/test/ui/issue-26046-fn-mut.rs create mode 100644 src/test/ui/issue-26046-fn-mut.stderr diff --git a/src/test/ui/issue-26046-fn-mut.rs b/src/test/ui/issue-26046-fn-mut.rs new file mode 100644 index 00000000000..5ed7ace5437 --- /dev/null +++ b/src/test/ui/issue-26046-fn-mut.rs @@ -0,0 +1,21 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn foo() -> Box { + let num = 5; + + let closure = || { + num += 1; + }; + + Box::new(closure) +} + +fn main() {} diff --git a/src/test/ui/issue-26046-fn-mut.stderr b/src/test/ui/issue-26046-fn-mut.stderr new file mode 100644 index 00000000000..4dd33ef8a0e --- /dev/null +++ b/src/test/ui/issue-26046-fn-mut.stderr @@ -0,0 +1,17 @@ +error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnMut` + --> $DIR/issue-26046-fn-mut.rs:14:19 + | +14 | let closure = || { + | ___________________^ +15 | | num += 1; +16 | | }; + | |_____^ + | +note: closure is `FnMut` because it mutates the variable `num` here + --> $DIR/issue-26046-fn-mut.rs:15:9 + | +15 | num += 1; + | ^^^ + +error: aborting due to previous error(s) + From b1b6490c5d60b0c5d0166407db9b57e39fc3e3a4 Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Tue, 6 Jun 2017 09:29:06 +0100 Subject: [PATCH 5/6] Group closure context ui tests --- src/test/ui/{ => closure_context}/issue-26046-fn-mut.rs | 0 src/test/ui/{ => closure_context}/issue-26046-fn-mut.stderr | 0 .../issue-26046-fn-once.rs} | 0 .../issue-26046-fn-once.stderr} | 4 ++-- .../ui/{fn_once-moved.rs => closure_context/issue-42065.rs} | 0 .../issue-42065.stderr} | 4 ++-- 6 files changed, 4 insertions(+), 4 deletions(-) rename src/test/ui/{ => closure_context}/issue-26046-fn-mut.rs (100%) rename src/test/ui/{ => closure_context}/issue-26046-fn-mut.stderr (100%) rename src/test/ui/{issue-26046.rs => closure_context/issue-26046-fn-once.rs} (100%) rename src/test/ui/{issue-26046.stderr => closure_context/issue-26046-fn-once.stderr} (83%) rename src/test/ui/{fn_once-moved.rs => closure_context/issue-42065.rs} (100%) rename src/test/ui/{fn_once-moved.stderr => closure_context/issue-42065.stderr} (86%) diff --git a/src/test/ui/issue-26046-fn-mut.rs b/src/test/ui/closure_context/issue-26046-fn-mut.rs similarity index 100% rename from src/test/ui/issue-26046-fn-mut.rs rename to src/test/ui/closure_context/issue-26046-fn-mut.rs diff --git a/src/test/ui/issue-26046-fn-mut.stderr b/src/test/ui/closure_context/issue-26046-fn-mut.stderr similarity index 100% rename from src/test/ui/issue-26046-fn-mut.stderr rename to src/test/ui/closure_context/issue-26046-fn-mut.stderr diff --git a/src/test/ui/issue-26046.rs b/src/test/ui/closure_context/issue-26046-fn-once.rs similarity index 100% rename from src/test/ui/issue-26046.rs rename to src/test/ui/closure_context/issue-26046-fn-once.rs diff --git a/src/test/ui/issue-26046.stderr b/src/test/ui/closure_context/issue-26046-fn-once.stderr similarity index 83% rename from src/test/ui/issue-26046.stderr rename to src/test/ui/closure_context/issue-26046-fn-once.stderr index fa3681dc01f..4a7aae87439 100644 --- a/src/test/ui/issue-26046.stderr +++ b/src/test/ui/closure_context/issue-26046-fn-once.stderr @@ -1,5 +1,5 @@ error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` - --> $DIR/issue-26046.rs:14:19 + --> $DIR/issue-26046-fn-once.rs:14:19 | 14 | let closure = move || { | ___________________^ @@ -8,7 +8,7 @@ error[E0525]: expected a closure that implements the `Fn` trait, but this closur | |_____^ | note: closure is `FnOnce` because it moves the variable `vec` out of its environment - --> $DIR/issue-26046.rs:15:9 + --> $DIR/issue-26046-fn-once.rs:15:9 | 15 | vec | ^^^ diff --git a/src/test/ui/fn_once-moved.rs b/src/test/ui/closure_context/issue-42065.rs similarity index 100% rename from src/test/ui/fn_once-moved.rs rename to src/test/ui/closure_context/issue-42065.rs diff --git a/src/test/ui/fn_once-moved.stderr b/src/test/ui/closure_context/issue-42065.stderr similarity index 86% rename from src/test/ui/fn_once-moved.stderr rename to src/test/ui/closure_context/issue-42065.stderr index 27b7d91d1d4..5bbd372adb6 100644 --- a/src/test/ui/fn_once-moved.stderr +++ b/src/test/ui/closure_context/issue-42065.stderr @@ -1,5 +1,5 @@ error[E0382]: use of moved value: `debug_dump_dict` - --> $DIR/fn_once-moved.rs:21:5 + --> $DIR/issue-42065.rs:21:5 | 20 | debug_dump_dict(); | --------------- value moved here @@ -7,7 +7,7 @@ error[E0382]: use of moved value: `debug_dump_dict` | ^^^^^^^^^^^^^^^ value used here after move | note: closure cannot be invoked more than once because it moves the variable `dict` out of its environment - --> $DIR/fn_once-moved.rs:16:29 + --> $DIR/issue-42065.rs:16:29 | 16 | for (key, value) in dict { | ^^^^ From 345b8332bde78dca7664b1b1b4f4a7284bd70a6d Mon Sep 17 00:00:00 2001 From: Tommy Ip Date: Wed, 7 Jun 2017 21:26:28 +0100 Subject: [PATCH 6/6] Cover all cases in closure errors --- src/librustc/traits/error_reporting.rs | 11 ++++++----- src/test/ui/closure_context/issue-26046-fn-mut.stderr | 3 +++ .../ui/closure_context/issue-26046-fn-once.stderr | 3 +++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index 73753382a7c..c8e99c0354a 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -648,6 +648,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { kind, found_kind); + err.span_label( + obligation.cause.span, + format!("the requirement to implement `{}` derives from here", kind)); + let infer_tables = match self.tables { InferTables::Interned(tables) => Some(InferTablesRef::Interned(tables)), @@ -656,6 +660,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { InferTables::Missing => None, }; + // Additional context information explaining why the closure only implements + // a particular trait. if let Some(tables) = infer_tables { match tables.closure_kinds.get(&node_id) { Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) => { @@ -670,11 +676,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { }, _ => {} } - } else { - err.span_note( - obligation.cause.span, - &format!("the requirement to implement `{}` \ - derives from here", kind)); } err.emit(); diff --git a/src/test/ui/closure_context/issue-26046-fn-mut.stderr b/src/test/ui/closure_context/issue-26046-fn-mut.stderr index 4dd33ef8a0e..dbf702e4503 100644 --- a/src/test/ui/closure_context/issue-26046-fn-mut.stderr +++ b/src/test/ui/closure_context/issue-26046-fn-mut.stderr @@ -6,6 +6,9 @@ error[E0525]: expected a closure that implements the `Fn` trait, but this closur 15 | | num += 1; 16 | | }; | |_____^ +17 | +18 | Box::new(closure) + | ----------------- the requirement to implement `Fn` derives from here | note: closure is `FnMut` because it mutates the variable `num` here --> $DIR/issue-26046-fn-mut.rs:15:9 diff --git a/src/test/ui/closure_context/issue-26046-fn-once.stderr b/src/test/ui/closure_context/issue-26046-fn-once.stderr index 4a7aae87439..3ec3f0cc9aa 100644 --- a/src/test/ui/closure_context/issue-26046-fn-once.stderr +++ b/src/test/ui/closure_context/issue-26046-fn-once.stderr @@ -6,6 +6,9 @@ error[E0525]: expected a closure that implements the `Fn` trait, but this closur 15 | | vec 16 | | }; | |_____^ +17 | +18 | Box::new(closure) + | ----------------- the requirement to implement `Fn` derives from here | note: closure is `FnOnce` because it moves the variable `vec` out of its environment --> $DIR/issue-26046-fn-once.rs:15:9