From 8ffc04b0325b0efc749121d03f92538daef37a11 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Jul 2016 16:52:11 -0400 Subject: [PATCH] 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 2ea2978b2940..dfca7b924b12 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 28bfb460a14f..e4308aabf5f7 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 702dd5f8de58..c1ffc668bc26 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; - } + 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); - 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(); + 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 + } - 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 - } - - (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 000000000000..591753181db1 --- /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); +}