From 8ffc04b0325b0efc749121d03f92538daef37a11 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Jul 2016 16:52:11 -0400 Subject: [PATCH 1/2] Avoid writing a temporary closure kind We used to write a temporary closure kind into the inference table, but this could lead to obligations being incorrectled resolved before inference had completed. This result could then be cached, leading to further trouble. This patch avoids writing any closure kind until the computation is complete. Fixes #34349. --- src/librustc/infer/mod.rs | 16 +++++ src/librustc/middle/mem_categorization.rs | 22 ++++-- src/librustc_typeck/check/upvar.rs | 85 +++++++++++------------ src/test/compile-fail/issue-34349.rs | 32 +++++++++ 4 files changed, 105 insertions(+), 50 deletions(-) create mode 100644 src/test/compile-fail/issue-34349.rs diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index 2ea2978b2940d..dfca7b924b129 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -175,6 +175,12 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { // any obligations set during the current snapshot. In that case, the // snapshot can't be rolled back. pub obligations_in_snapshot: Cell, + + // This is false except during closure kind inference. It is used + // by the mem-categorization code to be able to have stricter + // assertions (which are always true except during upvar + // inference). + during_closure_kind_inference: Cell, } /// A map returned by `skolemize_late_bound_regions()` indicating the skolemized @@ -491,6 +497,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'gcx> { tainted_by_errors_flag: Cell::new(false), err_count_on_creation: self.sess.err_count(), obligations_in_snapshot: Cell::new(false), + during_closure_kind_inference: Cell::new(false), } } } @@ -532,6 +539,7 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> { tainted_by_errors_flag: Cell::new(false), err_count_on_creation: tcx.sess.err_count(), obligations_in_snapshot: Cell::new(false), + during_closure_kind_inference: Cell::new(false), })) } } @@ -1294,6 +1302,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { .map(|method| resolve_ty(method.ty))) } + pub fn set_during_closure_kind_inference(&self, value: bool) { + self.during_closure_kind_inference.set(value); + } + + pub fn during_closure_kind_inference(&self) -> bool { + self.during_closure_kind_inference.get() + } + /// True if errors have been reported since this infcx was /// created. This is sometimes used as a heuristic to skip /// reporting errors that often occur as a result of earlier diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 28bfb460a14fa..e4308aabf5f75 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -362,7 +362,9 @@ impl MutabilityCategory { impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> { pub fn new(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>) -> MemCategorizationContext<'a, 'gcx, 'tcx> { - MemCategorizationContext { infcx: infcx } + MemCategorizationContext { + infcx: infcx, + } } fn tcx(&self) -> TyCtxt<'a, 'gcx, 'tcx> { @@ -584,10 +586,20 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> { self.cat_upvar(id, span, var_id, fn_node_id, kind) } None => { - span_bug!( - span, - "No closure kind for {:?}", - closure_id); + if !self.infcx.during_closure_kind_inference() { + span_bug!( + span, + "No closure kind for {:?}", + closure_id); + } + + // during closure kind inference, we + // don't know the closure kind yet, but + // it's ok because we detect that we are + // accessing an upvar and handle that + // case specially anyhow. Use Fn + // arbitrarily. + self.cat_upvar(id, span, var_id, fn_node_id, ty::ClosureKind::Fn) } } } diff --git a/src/librustc_typeck/check/upvar.rs b/src/librustc_typeck/check/upvar.rs index 702dd5f8de58a..c1ffc668bc261 100644 --- a/src/librustc_typeck/check/upvar.rs +++ b/src/librustc_typeck/check/upvar.rs @@ -47,11 +47,11 @@ use middle::mem_categorization as mc; use middle::mem_categorization::Categorization; use rustc::ty::{self, Ty}; use rustc::infer::UpvarRegion; -use std::collections::HashSet; use syntax::ast; use syntax_pos::Span; use rustc::hir; use rustc::hir::intravisit::{self, Visitor}; +use rustc::util::nodemap::NodeMap; /////////////////////////////////////////////////////////////////////////// // PUBLIC ENTRY POINTS @@ -60,9 +60,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn closure_analyze_fn(&self, body: &hir::Block) { let mut seed = SeedBorrowKind::new(self); seed.visit_block(body); - let closures_with_inferred_kinds = seed.closures_with_inferred_kinds; - let mut adjust = AdjustBorrowKind::new(self, &closures_with_inferred_kinds); + let mut adjust = AdjustBorrowKind::new(self, seed.temp_closure_kinds); adjust.visit_block(body); // it's our job to process these. @@ -72,9 +71,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn closure_analyze_const(&self, body: &hir::Expr) { let mut seed = SeedBorrowKind::new(self); seed.visit_expr(body); - let closures_with_inferred_kinds = seed.closures_with_inferred_kinds; - let mut adjust = AdjustBorrowKind::new(self, &closures_with_inferred_kinds); + let mut adjust = AdjustBorrowKind::new(self, seed.temp_closure_kinds); adjust.visit_expr(body); // it's our job to process these. @@ -87,7 +85,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { struct SeedBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { fcx: &'a FnCtxt<'a, 'gcx, 'tcx>, - closures_with_inferred_kinds: HashSet, + temp_closure_kinds: NodeMap, } impl<'a, 'gcx, 'tcx, 'v> Visitor<'v> for SeedBorrowKind<'a, 'gcx, 'tcx> { @@ -106,7 +104,7 @@ impl<'a, 'gcx, 'tcx, 'v> Visitor<'v> for SeedBorrowKind<'a, 'gcx, 'tcx> { impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> { fn new(fcx: &'a FnCtxt<'a, 'gcx, 'tcx>) -> SeedBorrowKind<'a, 'gcx, 'tcx> { - SeedBorrowKind { fcx: fcx, closures_with_inferred_kinds: HashSet::new() } + SeedBorrowKind { fcx: fcx, temp_closure_kinds: NodeMap() } } fn check_closure(&mut self, @@ -116,11 +114,8 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> { { let closure_def_id = self.fcx.tcx.map.local_def_id(expr.id); if !self.fcx.tables.borrow().closure_kinds.contains_key(&closure_def_id) { - self.closures_with_inferred_kinds.insert(expr.id); - self.fcx.tables.borrow_mut().closure_kinds - .insert(closure_def_id, ty::ClosureKind::Fn); - debug!("check_closure: adding closure_id={:?} to closures_with_inferred_kinds", - closure_def_id); + self.temp_closure_kinds.insert(expr.id, ty::ClosureKind::Fn); + debug!("check_closure: adding closure {:?} as Fn", expr.id); } self.fcx.tcx.with_freevars(expr.id, |freevars| { @@ -154,14 +149,14 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> { struct AdjustBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { fcx: &'a FnCtxt<'a, 'gcx, 'tcx>, - closures_with_inferred_kinds: &'a HashSet, + temp_closure_kinds: NodeMap, } impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { fn new(fcx: &'a FnCtxt<'a, 'gcx, 'tcx>, - closures_with_inferred_kinds: &'a HashSet) + temp_closure_kinds: NodeMap) -> AdjustBorrowKind<'a, 'gcx, 'tcx> { - AdjustBorrowKind { fcx: fcx, closures_with_inferred_kinds: closures_with_inferred_kinds } + AdjustBorrowKind { fcx: fcx, temp_closure_kinds: temp_closure_kinds } } fn analyze_closure(&mut self, @@ -176,8 +171,10 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { debug!("analyze_closure(id={:?}, body.id={:?})", id, body.id); { + self.fcx.set_during_closure_kind_inference(true); let mut euv = euv::ExprUseVisitor::new(self, self.fcx); euv.walk_fn(decl, body); + self.fcx.set_during_closure_kind_inference(false); } // Now that we've analyzed the closure, we know how each @@ -211,10 +208,14 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { self.fcx.demand_eqtype(span, final_upvar_ty, upvar_ty); } - // Now we must process and remove any deferred resolutions, - // since we have a concrete closure kind. + // If we are also inferred the closure kind here, update the + // main table and process any deferred resolutions. let closure_def_id = self.fcx.tcx.map.local_def_id(id); - if self.closures_with_inferred_kinds.contains(&id) { + if let Some(&kind) = self.temp_closure_kinds.get(&id) { + self.fcx.tables.borrow_mut().closure_kinds + .insert(closure_def_id, kind); + debug!("closure_kind({:?}) = {:?}", closure_def_id, kind); + let mut deferred_call_resolutions = self.fcx.remove_deferred_call_resolutions(closure_def_id); for deferred_call_resolution in &mut deferred_call_resolutions { @@ -259,7 +260,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { }) } - fn adjust_upvar_borrow_kind_for_consume(&self, + fn adjust_upvar_borrow_kind_for_consume(&mut self, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { @@ -350,7 +351,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { } } - fn adjust_upvar_borrow_kind_for_unique(&self, cmt: mc::cmt<'tcx>) { + fn adjust_upvar_borrow_kind_for_unique(&mut self, cmt: mc::cmt<'tcx>) { debug!("adjust_upvar_borrow_kind_for_unique(cmt={:?})", cmt); @@ -381,7 +382,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { } } - fn try_adjust_upvar_deref(&self, + fn try_adjust_upvar_deref(&mut self, note: &mc::Note, borrow_kind: ty::BorrowKind) -> bool @@ -430,7 +431,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { /// moving from left to right as needed (but never right to left). /// Here the argument `mutbl` is the borrow_kind that is required by /// some particular use. - fn adjust_upvar_borrow_kind(&self, + fn adjust_upvar_borrow_kind(&mut self, upvar_id: ty::UpvarId, upvar_capture: &mut ty::UpvarCapture, kind: ty::BorrowKind) { @@ -460,36 +461,30 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { } } - fn adjust_closure_kind(&self, + fn adjust_closure_kind(&mut self, closure_id: ast::NodeId, new_kind: ty::ClosureKind) { debug!("adjust_closure_kind(closure_id={}, new_kind={:?})", closure_id, new_kind); - if !self.closures_with_inferred_kinds.contains(&closure_id) { - return; - } - - let closure_def_id = self.fcx.tcx.map.local_def_id(closure_id); - let closure_kinds = &mut self.fcx.tables.borrow_mut().closure_kinds; - let existing_kind = *closure_kinds.get(&closure_def_id).unwrap(); + 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); - debug!("adjust_closure_kind: closure_id={}, existing_kind={:?}, new_kind={:?}", - closure_id, existing_kind, new_kind); - - match (existing_kind, new_kind) { - (ty::ClosureKind::Fn, ty::ClosureKind::Fn) | - (ty::ClosureKind::FnMut, ty::ClosureKind::Fn) | - (ty::ClosureKind::FnMut, ty::ClosureKind::FnMut) | - (ty::ClosureKind::FnOnce, _) => { - // no change needed - } + match (existing_kind, new_kind) { + (ty::ClosureKind::Fn, ty::ClosureKind::Fn) | + (ty::ClosureKind::FnMut, ty::ClosureKind::Fn) | + (ty::ClosureKind::FnMut, ty::ClosureKind::FnMut) | + (ty::ClosureKind::FnOnce, _) => { + // no change needed + } - (ty::ClosureKind::Fn, ty::ClosureKind::FnMut) | - (ty::ClosureKind::Fn, ty::ClosureKind::FnOnce) | - (ty::ClosureKind::FnMut, ty::ClosureKind::FnOnce) => { - // new kind is stronger than the old kind - closure_kinds.insert(closure_def_id, new_kind); + (ty::ClosureKind::Fn, ty::ClosureKind::FnMut) | + (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); + } } } } diff --git a/src/test/compile-fail/issue-34349.rs b/src/test/compile-fail/issue-34349.rs new file mode 100644 index 0000000000000..591753181db12 --- /dev/null +++ b/src/test/compile-fail/issue-34349.rs @@ -0,0 +1,32 @@ +// Copyright 2016 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. + +// This is a regression test for a problem encountered around upvar +// inference and trait caching: in particular, we were entering a +// temporary closure kind during inference, and then caching results +// based on that temporary kind, which led to no error being reported +// in this particular test. + +fn main() { + let inc = || {}; + inc(); + + fn apply(f: F) where F: Fn() { + f() + } + + let mut farewell = "goodbye".to_owned(); + let diary = || { //~ ERROR E0525 + farewell.push_str("!!!"); + println!("Then I screamed {}.", farewell); + }; + + apply(diary); +} From 63eb4d9114b05695bca2639628019ca61bae7366 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 25 Jul 2016 10:18:16 -0400 Subject: [PATCH 2/2] move `during_closure_kind_inference` flag to mc We used to put the flag on the `InferCtxt`. --- src/librustc/infer/mod.rs | 16 ---------------- src/librustc/middle/expr_use_visitor.rs | 13 +++++++++++-- src/librustc/middle/mem_categorization.rs | 21 ++++++++++++++++++++- src/librustc_typeck/check/upvar.rs | 9 ++++++--- 4 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index dfca7b924b129..2ea2978b2940d 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -175,12 +175,6 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { // any obligations set during the current snapshot. In that case, the // snapshot can't be rolled back. pub obligations_in_snapshot: Cell, - - // This is false except during closure kind inference. It is used - // by the mem-categorization code to be able to have stricter - // assertions (which are always true except during upvar - // inference). - during_closure_kind_inference: Cell, } /// A map returned by `skolemize_late_bound_regions()` indicating the skolemized @@ -497,7 +491,6 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'gcx> { tainted_by_errors_flag: Cell::new(false), err_count_on_creation: self.sess.err_count(), obligations_in_snapshot: Cell::new(false), - during_closure_kind_inference: Cell::new(false), } } } @@ -539,7 +532,6 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> { tainted_by_errors_flag: Cell::new(false), err_count_on_creation: tcx.sess.err_count(), obligations_in_snapshot: Cell::new(false), - during_closure_kind_inference: Cell::new(false), })) } } @@ -1302,14 +1294,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { .map(|method| resolve_ty(method.ty))) } - pub fn set_during_closure_kind_inference(&self, value: bool) { - self.during_closure_kind_inference.set(value); - } - - pub fn during_closure_kind_inference(&self) -> bool { - self.during_closure_kind_inference.get() - } - /// True if errors have been reported since this infcx was /// created. This is sometimes used as a heuristic to skip /// reporting errors that often occur as a result of earlier diff --git a/src/librustc/middle/expr_use_visitor.rs b/src/librustc/middle/expr_use_visitor.rs index 6551e0129f884..18b80a9636b45 100644 --- a/src/librustc/middle/expr_use_visitor.rs +++ b/src/librustc/middle/expr_use_visitor.rs @@ -271,10 +271,19 @@ enum PassArgs { impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> { pub fn new(delegate: &'a mut (Delegate<'tcx>+'a), - infcx: &'a InferCtxt<'a, 'gcx, 'tcx>) -> Self + infcx: &'a InferCtxt<'a, 'gcx, 'tcx>) + -> Self + { + ExprUseVisitor::with_options(delegate, infcx, mc::MemCategorizationOptions::default()) + } + + pub fn with_options(delegate: &'a mut (Delegate<'tcx>+'a), + infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, + options: mc::MemCategorizationOptions) + -> Self { ExprUseVisitor { - mc: mc::MemCategorizationContext::new(infcx), + mc: mc::MemCategorizationContext::with_options(infcx, options), delegate: delegate } } diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index e4308aabf5f75..0bc3c1ae899dd 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -259,6 +259,18 @@ impl ast_node for hir::Pat { #[derive(Copy, Clone)] pub struct MemCategorizationContext<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { pub infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, + options: MemCategorizationOptions, +} + +#[derive(Copy, Clone, Default)] +pub struct MemCategorizationOptions { + // If true, then when analyzing a closure upvar, if the closure + // has a missing kind, we treat it like a Fn closure. When false, + // we ICE if the closure has a missing kind. Should be false + // except during closure kind inference. It is used by the + // mem-categorization code to be able to have stricter assertions + // (which are always true except during upvar inference). + pub during_closure_kind_inference: bool, } pub type McResult = Result; @@ -362,8 +374,15 @@ impl MutabilityCategory { impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> { pub fn new(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>) -> MemCategorizationContext<'a, 'gcx, 'tcx> { + MemCategorizationContext::with_options(infcx, MemCategorizationOptions::default()) + } + + pub fn with_options(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, + options: MemCategorizationOptions) + -> MemCategorizationContext<'a, 'gcx, 'tcx> { MemCategorizationContext { infcx: infcx, + options: options, } } @@ -586,7 +605,7 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> { self.cat_upvar(id, span, var_id, fn_node_id, kind) } None => { - if !self.infcx.during_closure_kind_inference() { + if !self.options.during_closure_kind_inference { span_bug!( span, "No closure kind for {:?}", diff --git a/src/librustc_typeck/check/upvar.rs b/src/librustc_typeck/check/upvar.rs index c1ffc668bc261..71490423f73f2 100644 --- a/src/librustc_typeck/check/upvar.rs +++ b/src/librustc_typeck/check/upvar.rs @@ -171,10 +171,13 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> { debug!("analyze_closure(id={:?}, body.id={:?})", id, body.id); { - self.fcx.set_during_closure_kind_inference(true); - let mut euv = euv::ExprUseVisitor::new(self, self.fcx); + let mut euv = + euv::ExprUseVisitor::with_options(self, + self.fcx, + mc::MemCategorizationOptions { + during_closure_kind_inference: true + }); euv.walk_fn(decl, body); - self.fcx.set_during_closure_kind_inference(false); } // Now that we've analyzed the closure, we know how each