From bf02c57b16264ce332c799237c58eb8bf1c299c0 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Jan 2018 06:03:51 -0500 Subject: [PATCH 1/6] simplify `UniversalRegions::to_region_vid` to just consult the map This doesn't actually fix the bug, but seems better. --- src/librustc_mir/borrow_check/nll/universal_regions.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/borrow_check/nll/universal_regions.rs b/src/librustc_mir/borrow_check/nll/universal_regions.rs index 45604d52958c1..0e329266ea111 100644 --- a/src/librustc_mir/borrow_check/nll/universal_regions.rs +++ b/src/librustc_mir/borrow_check/nll/universal_regions.rs @@ -799,10 +799,12 @@ impl<'tcx> UniversalRegionIndices<'tcx> { /// during initialization. Relies on the `indices` map having been /// fully initialized. pub fn to_region_vid(&self, r: ty::Region<'tcx>) -> RegionVid { - match r { - ty::ReEarlyBound(..) | ty::ReStatic => *self.indices.get(&r).unwrap(), - ty::ReVar(..) => r.to_region_vid(), - _ => bug!("cannot convert `{:?}` to a region vid", r), + if let ty::ReVar(..) = r { + r.to_region_vid() + } else { + *self.indices.get(&r).unwrap_or_else(|| { + bug!("cannot convert `{:?}` to a region vid", r) + }) } } From d201e83f710065ded8f27ef8d072319574ac75c8 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Jan 2018 06:04:18 -0500 Subject: [PATCH 2/6] renumber regions in the generator interior Fixes #47189. --- src/librustc_mir/borrow_check/nll/renumber.rs | 17 ++++++++- .../ui/nll/generator-distinct-lifetime.rs | 38 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/nll/generator-distinct-lifetime.rs diff --git a/src/librustc_mir/borrow_check/nll/renumber.rs b/src/librustc_mir/borrow_check/nll/renumber.rs index 4aee48b979d61..c54acda8a6252 100644 --- a/src/librustc_mir/borrow_check/nll/renumber.rs +++ b/src/librustc_mir/borrow_check/nll/renumber.rs @@ -9,7 +9,7 @@ // except according to those terms. use rustc::ty::subst::Substs; -use rustc::ty::{self, ClosureSubsts, Ty, TypeFoldable}; +use rustc::ty::{self, ClosureSubsts, GeneratorInterior, Ty, TypeFoldable}; use rustc::mir::{BasicBlock, Location, Mir, Statement, StatementKind}; use rustc::mir::visit::{MutVisitor, TyContext}; use rustc::infer::{InferCtxt, NLLRegionVariableOrigin}; @@ -90,6 +90,21 @@ impl<'a, 'gcx, 'tcx> MutVisitor<'tcx> for NLLVisitor<'a, 'gcx, 'tcx> { *constant = self.renumber_regions(ty_context, &*constant); } + fn visit_generator_interior(&mut self, + interior: &mut GeneratorInterior<'tcx>, + location: Location) { + debug!( + "visit_generator_interior(interior={:?}, location={:?})", + interior, + location, + ); + + let ty_context = TyContext::Location(location); + *interior = self.renumber_regions(ty_context, interior); + + debug!("visit_generator_interior: interior={:?}", interior); + } + fn visit_closure_substs(&mut self, substs: &mut ClosureSubsts<'tcx>, location: Location) { debug!( "visit_closure_substs(substs={:?}, location={:?})", diff --git a/src/test/ui/nll/generator-distinct-lifetime.rs b/src/test/ui/nll/generator-distinct-lifetime.rs new file mode 100644 index 0000000000000..261d1bf85b157 --- /dev/null +++ b/src/test/ui/nll/generator-distinct-lifetime.rs @@ -0,0 +1,38 @@ +// 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. + +#![feature(generators, nll)] + +// Test for issue #47189. Here, both `s` and `t` are live for the +// generator's lifetime, but within the generator they have distinct +// lifetimes. +// +// Currently, we accept this code (with NLL enabled) since `x` is only +// borrowed once at a time -- though whether we should is not entirely +// obvious to me (the borrows are live over a yield, but then they are +// re-borrowing borrowed content, etc). Maybe I just haven't had +// enough coffee today, but I'm not entirely sure at this moment what +// effect a `suspend` should have on existing borrows. -nmatsakis + +fn foo(x: &mut u32) { + move || { + let s = &mut *x; + yield; + *s += 1; + + let t = &mut *x; + yield; + *t += 1; + }; +} + +fn main() { + foo(&mut 0); +} From ea742a4e5528ba937e4a2e9a15f74ac99287ef88 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Jan 2018 17:36:53 -0500 Subject: [PATCH 3/6] update test case --- src/test/ui/nll/generator-distinct-lifetime.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/test/ui/nll/generator-distinct-lifetime.rs b/src/test/ui/nll/generator-distinct-lifetime.rs index 261d1bf85b157..60f67b1766c2c 100644 --- a/src/test/ui/nll/generator-distinct-lifetime.rs +++ b/src/test/ui/nll/generator-distinct-lifetime.rs @@ -12,14 +12,11 @@ // Test for issue #47189. Here, both `s` and `t` are live for the // generator's lifetime, but within the generator they have distinct -// lifetimes. -// -// Currently, we accept this code (with NLL enabled) since `x` is only -// borrowed once at a time -- though whether we should is not entirely -// obvious to me (the borrows are live over a yield, but then they are -// re-borrowing borrowed content, etc). Maybe I just haven't had -// enough coffee today, but I'm not entirely sure at this moment what -// effect a `suspend` should have on existing borrows. -nmatsakis +// lifetimes. We accept this code -- even though the borrow extends +// over a yield -- because the data that is borrowed (`*x`) is not +// stored on the stack. + +// must-compile-successfully fn foo(x: &mut u32) { move || { From 5d259b23a4d69dad34f6ad1041faed84ba304fc5 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 19 Jan 2018 13:56:48 -0500 Subject: [PATCH 4/6] change MIR dump format to include yield type --- src/librustc_mir/util/pretty.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/util/pretty.rs b/src/librustc_mir/util/pretty.rs index 37f59773cd6f0..6ed797f70513d 100644 --- a/src/librustc_mir/util/pretty.rs +++ b/src/librustc_mir/util/pretty.rs @@ -522,7 +522,7 @@ pub fn write_mir_intro<'a, 'gcx, 'tcx>( w: &mut Write, ) -> io::Result<()> { write_mir_sig(tcx, src, mir, w)?; - writeln!(w, " {{")?; + writeln!(w, "{{")?; // construct a scope tree and write it out let mut scope_tree: FxHashMap> = FxHashMap(); @@ -585,13 +585,20 @@ fn write_mir_sig(tcx: TyCtxt, src: MirSource, mir: &Mir, w: &mut Write) -> io::R write!(w, "{:?}: {}", Place::Local(arg), mir.local_decls[arg].ty)?; } - write!(w, ") -> {}", mir.return_ty()) + write!(w, ") -> {}", mir.return_ty())?; } (hir::BodyOwnerKind::Const, _) | (hir::BodyOwnerKind::Static(_), _) | (_, Some(_)) => { assert_eq!(mir.arg_count, 0); - write!(w, ": {} =", mir.return_ty()) + write!(w, ": {} =", mir.return_ty())?; } } + + if let Some(yield_ty) = mir.yield_ty { + writeln!(w)?; + writeln!(w, "yields {}", yield_ty)?; + } + + Ok(()) } fn write_temp_decls(mir: &Mir, w: &mut Write) -> io::Result<()> { From fcb9e928b7104b68e63359cf237610e3ee50f968 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 19 Jan 2018 19:18:02 -0300 Subject: [PATCH 5/6] Integrate generators to universal region setup --- src/librustc/mir/visit.rs | 9 +++++++++ .../borrow_check/nll/constraint_generation.rs | 1 + .../borrow_check/nll/type_check/input_output.rs | 9 +++++++++ src/librustc_mir/borrow_check/nll/universal_regions.rs | 10 ++++++++++ 4 files changed, 29 insertions(+) diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index 58067931d562e..57ed41f2f06e6 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -277,6 +277,13 @@ macro_rules! make_mir_visitor { fn super_mir(&mut self, mir: & $($mutability)* Mir<'tcx>) { + if let Some(yield_ty) = &$($mutability)* mir.yield_ty { + self.visit_ty(yield_ty, TyContext::YieldTy(SourceInfo { + span: mir.span, + scope: ARGUMENT_VISIBILITY_SCOPE, + })); + } + // for best performance, we want to use an iterator rather // than a for-loop, to avoid calling Mir::invalidate for // each basic block. @@ -852,6 +859,8 @@ pub enum TyContext { /// The return type of the function. ReturnTy(SourceInfo), + YieldTy(SourceInfo), + /// A type found at some location. Location(Location), } diff --git a/src/librustc_mir/borrow_check/nll/constraint_generation.rs b/src/librustc_mir/borrow_check/nll/constraint_generation.rs index 7ca4ebd1cb296..3a39eb5c908de 100644 --- a/src/librustc_mir/borrow_check/nll/constraint_generation.rs +++ b/src/librustc_mir/borrow_check/nll/constraint_generation.rs @@ -69,6 +69,7 @@ impl<'cg, 'cx, 'gcx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'gcx fn visit_ty(&mut self, ty: &ty::Ty<'tcx>, ty_context: TyContext) { match ty_context { TyContext::ReturnTy(source_info) | + TyContext::YieldTy(source_info) | TyContext::LocalDecl { source_info, .. } => { span_bug!(source_info.span, "should not be visiting outside of the CFG: {:?}", diff --git a/src/librustc_mir/borrow_check/nll/type_check/input_output.rs b/src/librustc_mir/borrow_check/nll/type_check/input_output.rs index 9e88a632f5c3d..b1aeae0b76bb1 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/input_output.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/input_output.rs @@ -60,6 +60,15 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { self.equate_normalized_input_or_output(start_position, input_ty, mir_input_ty); } + assert!( + mir.yield_ty.is_some() && universal_regions.yield_ty.is_some() || + mir.yield_ty.is_none() && universal_regions.yield_ty.is_none() + ); + if let Some(mir_yield_ty) = mir.yield_ty { + let ur_yield_ty = universal_regions.yield_ty.unwrap(); + self.equate_normalized_input_or_output(start_position, ur_yield_ty, mir_yield_ty); + } + // Return types are a bit more complex. They may contain existential `impl Trait` // types. debug!( diff --git a/src/librustc_mir/borrow_check/nll/universal_regions.rs b/src/librustc_mir/borrow_check/nll/universal_regions.rs index 0e329266ea111..e426457a3493d 100644 --- a/src/librustc_mir/borrow_check/nll/universal_regions.rs +++ b/src/librustc_mir/borrow_check/nll/universal_regions.rs @@ -96,6 +96,8 @@ pub struct UniversalRegions<'tcx> { /// our special inference variable there, we would mess that up. pub region_bound_pairs: Vec<(ty::Region<'tcx>, GenericKind<'tcx>)>, + pub yield_ty: Option>, + relations: UniversalRegionRelations, } @@ -505,6 +507,13 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { num_universals ); + let yield_ty = match defining_ty { + DefiningTy::Generator(def_id, substs, _) => { + Some(substs.generator_yield_ty(def_id, self.infcx.tcx)) + } + _ => None, + }; + UniversalRegions { indices, fr_static, @@ -516,6 +525,7 @@ impl<'cx, 'gcx, 'tcx> UniversalRegionsBuilder<'cx, 'gcx, 'tcx> { unnormalized_output_ty, unnormalized_input_tys, region_bound_pairs: self.region_bound_pairs, + yield_ty: yield_ty, relations: self.relations, } } From 8f2cc02bc347aca311e6a5ce5548a62c7a896047 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 19 Jan 2018 20:02:32 -0300 Subject: [PATCH 6/6] Run yield-subtype test on nll mode too as a regression check --- src/test/run-pass/generator/yield-subtype.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/run-pass/generator/yield-subtype.rs b/src/test/run-pass/generator/yield-subtype.rs index 5ea4b1fe93c80..7e8a0d1e2b925 100644 --- a/src/test/run-pass/generator/yield-subtype.rs +++ b/src/test/run-pass/generator/yield-subtype.rs @@ -8,6 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// revisions:lexical nll +#![cfg_attr(nll, feature(nll))] + #![feature(generators)] fn bar<'a>() {