From bd271313321666850ce8d9d8d6093e6f5db530f4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 26 Oct 2018 12:11:19 +0200 Subject: [PATCH 01/10] Add helpful logging statements. This commit adds logging statements to `promote_consts` and `qualify_consts` to make it easier to understand what it is doing. --- src/librustc_mir/transform/promote_consts.rs | 8 +++++++ src/librustc_mir/transform/qualify_consts.rs | 22 +++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index f4efe33da7080..790e6e82723ef 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -53,6 +53,7 @@ pub enum TempState { impl TempState { pub fn is_promotable(&self) -> bool { + debug!("is_promotable: self={:?}", self); if let TempState::Defined { uses, .. } = *self { uses > 0 } else { @@ -88,6 +89,7 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { &index: &Local, context: PlaceContext<'tcx>, location: Location) { + debug!("visit_local: index={:?} context={:?} location={:?}", index, context, location); // We're only interested in temporaries if self.mir.local_kind(index) != LocalKind::Temp { return; @@ -97,10 +99,15 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { // then it's constant and thus drop is noop. // Storage live ranges are also irrelevant. if context.is_drop() || context.is_storage_marker() { + debug!( + "visit_local: context.is_drop={:?} context.is_storage_marker={:?}", + context.is_drop(), context.is_storage_marker(), + ); return; } let temp = &mut self.temps[index]; + debug!("visit_local: temp={:?}", temp); if *temp == TempState::Undefined { match context { PlaceContext::Store | @@ -121,6 +128,7 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { PlaceContext::Borrow {..} => true, _ => context.is_nonmutating_use() }; + debug!("visit_local: allowed_use={:?}", allowed_use); if allowed_use { *uses += 1; return; diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 78547abf9d9d3..29e6ef18d2d30 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -84,7 +84,7 @@ impl<'a, 'tcx> Qualif { } /// What kind of item we are in. -#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] enum Mode { Const, Static, @@ -383,6 +383,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> { // Collect all the temps we need to promote. let mut promoted_temps = BitSet::new_empty(self.temp_promotion_state.len()); + debug!("qualify_const: promotion_candidates={:?}", self.promotion_candidates); for candidate in &self.promotion_candidates { match *candidate { Candidate::Ref(Location { block: bb, statement_index: stmt_idx }) => { @@ -414,6 +415,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { &local: &Local, _: PlaceContext<'tcx>, _: Location) { + debug!("visit_local: local={:?}", local); let kind = self.mir.local_kind(local); match kind { LocalKind::ReturnPointer => { @@ -435,6 +437,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } if !self.temp_promotion_state[local].is_promotable() { + debug!("visit_local: (not promotable) local={:?}", local); self.add(Qualif::NOT_PROMOTABLE); } @@ -451,6 +454,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { place: &Place<'tcx>, context: PlaceContext<'tcx>, location: Location) { + debug!("visit_place: place={:?} context={:?} location={:?}", place, context, location); match *place { Place::Local(ref local) => self.visit_local(local, context, location), Place::Promoted(_) => bug!("promoting already promoted MIR"), @@ -557,6 +561,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { + debug!("visit_operand: operand={:?} location={:?}", operand, location); self.super_operand(operand, location); match *operand { @@ -591,6 +596,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + debug!("visit_rvalue: rvalue={:?} location={:?}", rvalue, location); // Recurse through operands and places. if let Rvalue::Ref(region, kind, ref place) = *rvalue { let mut is_reborrow = false; @@ -696,6 +702,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } } + debug!("visit_rvalue: forbidden_mut={:?}", forbidden_mut); if forbidden_mut { self.add(Qualif::NOT_CONST); } else { @@ -709,15 +716,19 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } place = &proj.base; } + debug!("visit_rvalue: place={:?}", place); if let Place::Local(local) = *place { if self.mir.local_kind(local) == LocalKind::Temp { + debug!("visit_rvalue: local={:?}", local); if let Some(qualif) = self.local_qualif[local] { // `forbidden_mut` is false, so we can safely ignore // `MUTABLE_INTERIOR` from the local's qualifications. // This allows borrowing fields which don't have // `MUTABLE_INTERIOR`, from a type that does, e.g.: // `let _: &'static _ = &(Cell::new(1), 2).1;` + debug!("visit_rvalue: qualif={:?}", qualif); if (qualif - Qualif::MUTABLE_INTERIOR).is_empty() { + debug!("visit_rvalue: candidate={:?}", candidate); self.promotion_candidates.push(candidate); } } @@ -815,6 +826,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { bb: BasicBlock, kind: &TerminatorKind<'tcx>, location: Location) { + debug!("visit_terminator_kind: bb={:?} kind={:?} location={:?}", bb, kind, location); if let TerminatorKind::Call { ref func, ref args, ref destination, .. } = *kind { self.visit_operand(func, location); @@ -967,6 +979,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { let candidate = Candidate::Argument { bb, index: i }; if is_shuffle && i == 2 { if this.qualif.is_empty() { + debug!("visit_terminator_kind: candidate={:?}", candidate); this.promotion_candidates.push(candidate); } else { span_err!(this.tcx.sess, this.span, E0526, @@ -983,6 +996,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { return } if this.qualif.is_empty() { + debug!("visit_terminator_kind: candidate={:?}", candidate); this.promotion_candidates.push(candidate); } else { this.tcx.sess.span_err(this.span, @@ -1056,6 +1070,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { dest: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { + debug!("visit_assign: dest={:?} rvalue={:?} location={:?}", dest, rvalue, location); self.visit_rvalue(rvalue, location); // Check the allowed const fn argument forms. @@ -1104,10 +1119,12 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } fn visit_source_info(&mut self, source_info: &SourceInfo) { + debug!("visit_source_info: source_info={:?}", source_info); self.span = source_info.span; } fn visit_statement(&mut self, bb: BasicBlock, statement: &Statement<'tcx>, location: Location) { + debug!("visit_statement: bb={:?} statement={:?} location={:?}", bb, statement, location); self.nest(|this| { this.visit_source_info(&statement.source_info); match statement.kind { @@ -1131,6 +1148,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { bb: BasicBlock, terminator: &Terminator<'tcx>, location: Location) { + debug!("visit_terminator: bb={:?} terminator={:?} location={:?}", bb, terminator, location); self.nest(|this| this.super_terminator(bb, terminator, location)); } } @@ -1197,6 +1215,7 @@ impl MirPass for QualifyAndPromoteConstants { hir::BodyOwnerKind::Static(hir::MutMutable) => Mode::StaticMut, }; + debug!("run_pass: mode={:?}", mode); if mode == Mode::Fn || mode == Mode::ConstFn { // This is ugly because Qualifier holds onto mir, // which can't be mutated until its scope ends. @@ -1239,6 +1258,7 @@ impl MirPass for QualifyAndPromoteConstants { // In `const` and `static` everything without `StorageDead` // is `'static`, we don't have to create promoted MIR fragments, // just remove `Drop` and `StorageDead` on "promoted" locals. + debug!("run_pass: promoted_temps={:?}", promoted_temps); for block in mir.basic_blocks_mut() { block.statements.retain(|statement| { match statement.kind { From 6c1d71859dc643d18c0043efb41769b298be9634 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 26 Oct 2018 13:21:29 +0200 Subject: [PATCH 02/10] Test for cast causing static promotion failure. This commit adds a test that ensures that a cast in a static doesn't stop const promotion within the static. --- src/test/ui/nll/issue-55288.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 src/test/ui/nll/issue-55288.rs diff --git a/src/test/ui/nll/issue-55288.rs b/src/test/ui/nll/issue-55288.rs new file mode 100644 index 0000000000000..c0f10ee607cf2 --- /dev/null +++ b/src/test/ui/nll/issue-55288.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. + +#![feature(nll)] + +// run-pass + +struct Slice(&'static [&'static [u8]]); + +static MAP: Slice = Slice(&[ + b"CloseEvent" as &'static [u8], +]); + +fn main() {} From 0c3dfe117789078543f5bcece2ad5ee12df64e3b Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 26 Oct 2018 13:22:45 +0200 Subject: [PATCH 03/10] Refactor and add `PlaceContext::AscribeUserTy`. This commit refactors `PlaceContext` to split it into four different smaller enums based on if the context represents a mutating use, non-mutating use, maybe-mutating use or a non-use (this is based on the recommendation from @oli-obk on Zulip[1]). This commit then introduces a `PlaceContext::AscribeUserTy` variant. `StatementKind::AscribeUserTy` is now correctly mapped to `PlaceContext::AscribeUserTy` instead of `PlaceContext::Validate`. `PlaceContext::AscribeUserTy` can also now be correctly categorized as a non-use which fixes an issue with constant promotion in statics after a cast introduces a `AscribeUserTy` statement. [1]: https://rust-lang.zulipchat.com/#narrow/stream/122657-wg-nll/subject/.2355288.20cast.20fails.20to.20promote.20to.20'static/near/136536949 --- src/librustc/mir/visit.rs | 290 ++++++++++++------ src/librustc_codegen_llvm/mir/analyze.rs | 49 ++- src/librustc_mir/borrow_check/borrow_set.rs | 17 +- .../borrow_check/nll/type_check/mod.rs | 8 +- src/librustc_mir/borrow_check/used_muts.rs | 3 +- src/librustc_mir/transform/check_unsafety.rs | 12 +- src/librustc_mir/transform/const_prop.rs | 15 +- src/librustc_mir/transform/promote_consts.rs | 21 +- src/librustc_mir/transform/qualify_consts.rs | 23 +- src/librustc_mir/transform/simplify.rs | 6 +- .../transform/uniform_array_move_out.rs | 6 +- src/librustc_mir/util/collect_writes.rs | 18 +- src/librustc_mir/util/liveness.rs | 35 ++- 13 files changed, 308 insertions(+), 195 deletions(-) diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index bfc03923f6011..caa627441ceb6 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -364,33 +364,54 @@ macro_rules! make_mir_visitor { self.visit_assign(block, place, rvalue, location); } StatementKind::FakeRead(_, ref $($mutability)* place) => { - self.visit_place(place, - PlaceContext::Inspect, - location); + self.visit_place( + place, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect), + location + ); } StatementKind::EndRegion(_) => {} StatementKind::Validate(_, ref $($mutability)* places) => { for operand in places { - self.visit_place(& $($mutability)* operand.place, - PlaceContext::Validate, location); + self.visit_place( + & $($mutability)* operand.place, + PlaceContext::NonUse(NonUseContext::Validate), + location + ); self.visit_ty(& $($mutability)* operand.ty, TyContext::Location(location)); } } StatementKind::SetDiscriminant{ ref $($mutability)* place, .. } => { - self.visit_place(place, PlaceContext::Store, location); + self.visit_place( + place, + PlaceContext::MutatingUse(MutatingUseContext::Store), + location + ); } StatementKind::StorageLive(ref $($mutability)* local) => { - self.visit_local(local, PlaceContext::StorageLive, location); + self.visit_local( + local, + PlaceContext::NonUse(NonUseContext::StorageLive), + location + ); } StatementKind::StorageDead(ref $($mutability)* local) => { - self.visit_local(local, PlaceContext::StorageDead, location); + self.visit_local( + local, + PlaceContext::NonUse(NonUseContext::StorageDead), + location + ); } StatementKind::InlineAsm { ref $($mutability)* outputs, ref $($mutability)* inputs, asm: _ } => { for output in & $($mutability)* outputs[..] { - self.visit_place(output, PlaceContext::AsmOutput, location); + self.visit_place( + output, + PlaceContext::MutatingUse(MutatingUseContext::AsmOutput), + location + ); } for input in & $($mutability)* inputs[..] { self.visit_operand(input, location); @@ -412,7 +433,11 @@ macro_rules! make_mir_visitor { place: &$($mutability)* Place<'tcx>, rvalue: &$($mutability)* Rvalue<'tcx>, location: Location) { - self.visit_place(place, PlaceContext::Store, location); + self.visit_place( + place, + PlaceContext::MutatingUse(MutatingUseContext::Store), + location + ); self.visit_rvalue(rvalue, location); } @@ -459,7 +484,11 @@ macro_rules! make_mir_visitor { TerminatorKind::Drop { ref $($mutability)* location, target, unwind } => { - self.visit_place(location, PlaceContext::Drop, source_location); + self.visit_place( + location, + PlaceContext::MutatingUse(MutatingUseContext::Drop), + source_location + ); self.visit_branch(block, target); unwind.map(|t| self.visit_branch(block, t)); } @@ -468,7 +497,11 @@ macro_rules! make_mir_visitor { ref $($mutability)* value, target, unwind } => { - self.visit_place(location, PlaceContext::Drop, source_location); + self.visit_place( + location, + PlaceContext::MutatingUse(MutatingUseContext::Drop), + source_location + ); self.visit_operand(value, source_location); self.visit_branch(block, target); unwind.map(|t| self.visit_branch(block, t)); @@ -484,7 +517,11 @@ macro_rules! make_mir_visitor { self.visit_operand(arg, source_location); } if let Some((ref $($mutability)* destination, target)) = *destination { - self.visit_place(destination, PlaceContext::Call, source_location); + self.visit_place( + destination, + PlaceContext::MutatingUse(MutatingUseContext::Call), + source_location + ); self.visit_branch(block, target); } cleanup.map(|t| self.visit_branch(block, t)); @@ -552,14 +589,28 @@ macro_rules! make_mir_visitor { Rvalue::Ref(ref $($mutability)* r, bk, ref $($mutability)* path) => { self.visit_region(r, location); - self.visit_place(path, PlaceContext::Borrow { - region: *r, - kind: bk - }, location); + let ctx = match bk { + BorrowKind::Shared => PlaceContext::NonMutatingUse( + NonMutatingUseContext::SharedBorrow(*r) + ), + BorrowKind::Shallow => PlaceContext::NonMutatingUse( + NonMutatingUseContext::ShallowBorrow(*r) + ), + BorrowKind::Unique => PlaceContext::NonMutatingUse( + NonMutatingUseContext::UniqueBorrow(*r) + ), + BorrowKind::Mut { .. } => + PlaceContext::MutatingUse(MutatingUseContext::Borrow(*r)), + }; + self.visit_place(path, ctx, location); } Rvalue::Len(ref $($mutability)* path) => { - self.visit_place(path, PlaceContext::Inspect, location); + self.visit_place( + path, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect), + location + ); } Rvalue::Cast(_cast_kind, @@ -584,7 +635,11 @@ macro_rules! make_mir_visitor { } Rvalue::Discriminant(ref $($mutability)* place) => { - self.visit_place(place, PlaceContext::Inspect, location); + self.visit_place( + place, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect), + location + ); } Rvalue::NullaryOp(_op, ref $($mutability)* ty) => { @@ -632,10 +687,18 @@ macro_rules! make_mir_visitor { location: Location) { match *operand { Operand::Copy(ref $($mutability)* place) => { - self.visit_place(place, PlaceContext::Copy, location); + self.visit_place( + place, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy), + location + ); } Operand::Move(ref $($mutability)* place) => { - self.visit_place(place, PlaceContext::Move, location); + self.visit_place( + place, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Move), + location + ); } Operand::Constant(ref $($mutability)* constant) => { self.visit_constant(constant, location); @@ -648,7 +711,11 @@ macro_rules! make_mir_visitor { _variance: & $($mutability)* ty::Variance, user_ty: & $($mutability)* UserTypeProjection<'tcx>, location: Location) { - self.visit_place(place, PlaceContext::Validate, location); + self.visit_place( + place, + PlaceContext::NonUse(NonUseContext::AscribeUserTy), + location + ); self.visit_user_type_projection(user_ty); } @@ -693,9 +760,9 @@ macro_rules! make_mir_visitor { ref $($mutability)* elem, } = *proj; let context = if context.is_mutating_use() { - PlaceContext::Projection(Mutability::Mut) + PlaceContext::MutatingUse(MutatingUseContext::Projection) } else { - PlaceContext::Projection(Mutability::Not) + PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) }; self.visit_place(base, context, location); self.visit_projection_elem(elem, location); @@ -713,7 +780,11 @@ macro_rules! make_mir_visitor { self.visit_ty(ty, TyContext::Location(location)); } ProjectionElem::Index(ref $($mutability)* local) => { - self.visit_local(local, PlaceContext::Copy, location); + self.visit_local( + local, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy), + location + ); } ProjectionElem::ConstantIndex { offset: _, min_length: _, @@ -896,125 +967,146 @@ pub enum TyContext { } #[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum PlaceContext<'tcx> { - // Appears as LHS of an assignment - Store, +pub enum NonMutatingUseContext<'tcx> { + /// Being inspected in some way, like loading a len. + Inspect, + /// Consumed as part of an operand. + Copy, + /// Consumed as part of an operand. + Move, + /// Shared borrow. + SharedBorrow(Region<'tcx>), + /// Shallow borrow. + ShallowBorrow(Region<'tcx>), + /// Unique borrow. + UniqueBorrow(Region<'tcx>), + /// Used as base for another place, e.g. `x` in `x.y`. Will not mutate the place. + /// For example, the projection `x.y` is not marked as a mutation in these cases: + /// + /// z = x.y; + /// f(&x.y); + /// + Projection, +} - // Can often be treated as a Store, but needs to be separate because - // ASM is allowed to read outputs as well, so a Store-AsmOutput sequence - // cannot be simplified the way a Store-Store can be. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum MutatingUseContext<'tcx> { + /// Appears as LHS of an assignment. + Store, + /// Can often be treated as a `Store`, but needs to be separate because + /// ASM is allowed to read outputs as well, so a `Store`-`AsmOutput` sequence + /// cannot be simplified the way a `Store`-`Store` can be. AsmOutput, - - // Dest of a call + /// Destination of a call. Call, - - // Being dropped + /// Being dropped. Drop, + /// Mutable borrow. + Borrow(Region<'tcx>), + /// Used as base for another place, e.g. `x` in `x.y`. Could potentially mutate the place. + /// For example, the projection `x.y` is marked as a mutation in these cases: + /// + /// x.y = ...; + /// f(&mut x.y); + /// + Projection, +} - // Being inspected in some way, like loading a len - Inspect, - - // Being borrowed - Borrow { region: Region<'tcx>, kind: BorrowKind }, - - // Used as base for another place, e.g. `x` in `x.y`. - // - // The `Mutability` argument specifies whether the projection is being performed in order to - // (potentially) mutate the place. For example, the projection `x.y` is marked as a mutation - // in these cases: - // - // x.y = ...; - // f(&mut x.y); - // - // But not in these cases: - // - // z = x.y; - // f(&x.y); - Projection(Mutability), - - // Consumed as part of an operand - Copy, - Move, - - // Starting and ending a storage live range +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum NonUseContext { + /// Starting a storage live range. StorageLive, + /// Ending a storage live range. StorageDead, - - // Validation command + /// User type annotation assertions for NLL. + AscribeUserTy, + /// Validation command. Validate, } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum PlaceContext<'tcx> { + NonMutatingUse(NonMutatingUseContext<'tcx>), + MutatingUse(MutatingUseContext<'tcx>), + NonUse(NonUseContext), +} + impl<'tcx> PlaceContext<'tcx> { - /// Returns true if this place context represents a drop. + /// Returns `true` if this place context represents a drop. pub fn is_drop(&self) -> bool { match *self { - PlaceContext::Drop => true, + PlaceContext::MutatingUse(MutatingUseContext::Drop) => true, + _ => false, + } + } + + /// Returns `true` if this place context represents a borrow. + pub fn is_borrow(&self) -> bool { + match *self { + PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) | + PlaceContext::MutatingUse(MutatingUseContext::Borrow(..)) => true, _ => false, } } - /// Returns true if this place context represents a storage live or storage dead marker. + /// Returns `true` if this place context represents a storage live or storage dead marker. pub fn is_storage_marker(&self) -> bool { match *self { - PlaceContext::StorageLive | PlaceContext::StorageDead => true, + PlaceContext::NonUse(NonUseContext::StorageLive) | + PlaceContext::NonUse(NonUseContext::StorageDead) => true, _ => false, } } - /// Returns true if this place context represents a storage live marker. + /// Returns `true` if this place context represents a storage live marker. pub fn is_storage_live_marker(&self) -> bool { match *self { - PlaceContext::StorageLive => true, + PlaceContext::NonUse(NonUseContext::StorageLive) => true, _ => false, } } - /// Returns true if this place context represents a storage dead marker. + /// Returns `true` if this place context represents a storage dead marker. pub fn is_storage_dead_marker(&self) -> bool { match *self { - PlaceContext::StorageDead => true, + PlaceContext::NonUse(NonUseContext::StorageDead) => true, _ => false, } } - /// Returns true if this place context represents a use that potentially changes the value. + /// Returns `true` if this place context represents a use that potentially changes the value. pub fn is_mutating_use(&self) -> bool { match *self { - PlaceContext::Store | PlaceContext::AsmOutput | PlaceContext::Call | - PlaceContext::Borrow { kind: BorrowKind::Mut { .. }, .. } | - PlaceContext::Projection(Mutability::Mut) | - PlaceContext::Drop => true, - - PlaceContext::Inspect | - PlaceContext::Borrow { kind: BorrowKind::Shared, .. } | - PlaceContext::Borrow { kind: BorrowKind::Shallow, .. } | - PlaceContext::Borrow { kind: BorrowKind::Unique, .. } | - PlaceContext::Projection(Mutability::Not) | - PlaceContext::Copy | PlaceContext::Move | - PlaceContext::StorageLive | PlaceContext::StorageDead | - PlaceContext::Validate => false, + PlaceContext::MutatingUse(..) => true, + _ => false, } } - /// Returns true if this place context represents a use that does not change the value. + /// Returns `true` if this place context represents a use that does not change the value. pub fn is_nonmutating_use(&self) -> bool { match *self { - PlaceContext::Inspect | - PlaceContext::Borrow { kind: BorrowKind::Shared, .. } | - PlaceContext::Borrow { kind: BorrowKind::Shallow, .. } | - PlaceContext::Borrow { kind: BorrowKind::Unique, .. } | - PlaceContext::Projection(Mutability::Not) | - PlaceContext::Copy | PlaceContext::Move => true, - - PlaceContext::Borrow { kind: BorrowKind::Mut { .. }, .. } | PlaceContext::Store | - PlaceContext::AsmOutput | - PlaceContext::Call | PlaceContext::Projection(Mutability::Mut) | - PlaceContext::Drop | PlaceContext::StorageLive | PlaceContext::StorageDead | - PlaceContext::Validate => false, + PlaceContext::NonMutatingUse(..) => true, + _ => false, } } + /// Returns `true` if this place context represents a use. pub fn is_use(&self) -> bool { - self.is_mutating_use() || self.is_nonmutating_use() + match *self { + PlaceContext::NonUse(..) => false, + _ => true, + } + } + + /// Returns `true` if this place context represents an assignment statement. + pub fn is_place_assignment(&self) -> bool { + match *self { + PlaceContext::MutatingUse(MutatingUseContext::Store) | + PlaceContext::MutatingUse(MutatingUseContext::Call) | + PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) => true, + _ => false, + } } } diff --git a/src/librustc_codegen_llvm/mir/analyze.rs b/src/librustc_codegen_llvm/mir/analyze.rs index 1602ef3c5b795..a63cbe70df611 100644 --- a/src/librustc_codegen_llvm/mir/analyze.rs +++ b/src/librustc_codegen_llvm/mir/analyze.rs @@ -15,7 +15,7 @@ use rustc_data_structures::bit_set::BitSet; use rustc_data_structures::graph::dominators::Dominators; use rustc_data_structures::indexed_vec::{Idx, IndexVec}; use rustc::mir::{self, Location, TerminatorKind}; -use rustc::mir::visit::{Visitor, PlaceContext}; +use rustc::mir::visit::{Visitor, PlaceContext, MutatingUseContext, NonMutatingUseContext}; use rustc::mir::traversal; use rustc::ty; use rustc::ty::layout::LayoutOf; @@ -116,7 +116,11 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { self.not_ssa(index); } } else { - self.visit_place(place, PlaceContext::Store, location); + self.visit_place( + place, + PlaceContext::MutatingUse(MutatingUseContext::Store), + location + ); } self.visit_rvalue(rvalue, location); @@ -142,7 +146,11 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { // is not guaranteed to be statically dominated by the // definition of x, so x must always be in an alloca. if let mir::Operand::Move(ref place) = args[0] { - self.visit_place(place, PlaceContext::Drop, location); + self.visit_place( + place, + PlaceContext::MutatingUse(MutatingUseContext::Drop), + location + ); } } } @@ -160,7 +168,8 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { if let mir::Place::Projection(ref proj) = *place { // Allow uses of projections that are ZSTs or from scalar fields. let is_consume = match context { - PlaceContext::Copy | PlaceContext::Move => true, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => true, _ => false }; if is_consume { @@ -190,7 +199,11 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { // A deref projection only reads the pointer, never needs the place. if let mir::ProjectionElem::Deref = proj.elem { - return self.visit_place(&proj.base, PlaceContext::Copy, location); + return self.visit_place( + &proj.base, + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy), + location + ); } } @@ -202,16 +215,14 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { context: PlaceContext<'tcx>, location: Location) { match context { - PlaceContext::Call => { + PlaceContext::MutatingUse(MutatingUseContext::Call) => { self.assign(local, location); } - PlaceContext::StorageLive | - PlaceContext::StorageDead | - PlaceContext::Validate => {} + PlaceContext::NonUse(_) => {} - PlaceContext::Copy | - PlaceContext::Move => { + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => { // Reads from uninitialized variables (e.g. in dead code, after // optimizations) require locals to be in (uninitialized) memory. // NB: there can be uninitialized reads of a local visited after @@ -227,15 +238,19 @@ impl Visitor<'tcx> for LocalAnalyzer<'mir, 'a, 'll, 'tcx> { } } - PlaceContext::Inspect | - PlaceContext::Store | - PlaceContext::AsmOutput | - PlaceContext::Borrow { .. } | - PlaceContext::Projection(..) => { + PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) | + PlaceContext::MutatingUse(MutatingUseContext::Store) | + PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) | + PlaceContext::MutatingUse(MutatingUseContext::Borrow(..)) | + PlaceContext::MutatingUse(MutatingUseContext::Projection) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) => { self.not_ssa(local); } - PlaceContext::Drop => { + PlaceContext::MutatingUse(MutatingUseContext::Drop) => { let ty = mir::Place::Local(local).ty(self.fx.mir, self.fx.cx.tcx); let ty = self.fx.monomorphize(&ty.to_ty(self.fx.cx.tcx)); diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index ab44ebc052f30..db56ce4627410 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -12,7 +12,9 @@ use borrow_check::place_ext::PlaceExt; use dataflow::indexes::BorrowIndex; use dataflow::move_paths::MoveData; use rustc::mir::traversal; -use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{ + PlaceContext, Visitor, NonUseContext, MutatingUseContext, NonMutatingUseContext +}; use rustc::mir::{self, Location, Mir, Place, Local}; use rustc::ty::{Region, TyCtxt}; use rustc::util::nodemap::{FxHashMap, FxHashSet}; @@ -116,7 +118,7 @@ impl LocalsStateAtExit { impl<'tcx> Visitor<'tcx> for HasStorageDead { fn visit_local(&mut self, local: &Local, ctx: PlaceContext<'tcx>, _: Location) { - if ctx == PlaceContext::StorageDead { + if ctx == PlaceContext::NonUse(NonUseContext::StorageDead) { self.0.insert(*local); } } @@ -266,7 +268,9 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { // Watch out: the use of TMP in the borrow itself // doesn't count as an activation. =) - if borrow_data.reserve_location == location && context == PlaceContext::Store { + if borrow_data.reserve_location == location && + context == PlaceContext::MutatingUse(MutatingUseContext::Store) + { return; } @@ -287,10 +291,9 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { borrow_data.activation_location = match context { // The use of TMP in a shared borrow does not // count as an actual activation. - PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. } - | PlaceContext::Borrow { kind: mir::BorrowKind::Shallow, .. } => { - TwoPhaseActivation::NotActivated - } + PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) => + TwoPhaseActivation::NotActivated, _ => { // Double check: This borrow is indeed a two-phase borrow (that is, // we are 'transitioning' from `NotActivated` to `ActivatedAt`) and diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index 33bd009c8b565..f1ebddfd6d658 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -35,7 +35,7 @@ use rustc::infer::outlives::env::RegionBoundPairs; use rustc::infer::{InferCtxt, InferOk, LateBoundRegionConversionTime}; use rustc::mir::interpret::EvalErrorKind::BoundsCheck; use rustc::mir::tcx::PlaceTy; -use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{PlaceContext, Visitor, MutatingUseContext, NonMutatingUseContext}; use rustc::mir::*; use rustc::traits::query::type_op; use rustc::traits::query::type_op::custom::CustomTypeOp; @@ -472,9 +472,9 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> { } Place::Projection(ref proj) => { let base_context = if context.is_mutating_use() { - PlaceContext::Projection(Mutability::Mut) + PlaceContext::MutatingUse(MutatingUseContext::Projection) } else { - PlaceContext::Projection(Mutability::Not) + PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) }; let base_ty = self.sanitize_place(&proj.base, location, base_context); if let PlaceTy::Ty { ty } = base_ty { @@ -488,7 +488,7 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> { self.sanitize_projection(base_ty, &proj.elem, place, location) } }; - if let PlaceContext::Copy = context { + if let PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) = context { let tcx = self.tcx(); let trait_ref = ty::TraitRef { def_id: tcx.lang_items().copy_trait().unwrap(), diff --git a/src/librustc_mir/borrow_check/used_muts.rs b/src/librustc_mir/borrow_check/used_muts.rs index dbe19bc47859f..dad87ed65a7d4 100644 --- a/src/librustc_mir/borrow_check/used_muts.rs +++ b/src/librustc_mir/borrow_check/used_muts.rs @@ -14,7 +14,6 @@ use rustc::mir::{Local, Location, Place}; use rustc_data_structures::fx::FxHashSet; use borrow_check::MirBorrowckCtxt; -use util::collect_writes::is_place_assignment; impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// Walks the MIR looking for assignments to a set of locals, as part of the unused mutable @@ -46,7 +45,7 @@ impl<'visit, 'cx, 'gcx, 'tcx> Visitor<'tcx> for GatherUsedMutsVisitor<'visit, 'c return; } - if is_place_assignment(&place_context) { + if place_context.is_place_assignment() { // Propagate the Local assigned at this Location as a used mutable local variable for moi in &self.mbcx.move_data.loc_map[location] { let mpi = &self.mbcx.move_data.moves[*moi].path; diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index ae881bad58dcd..edd15c39fed3e 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -19,7 +19,7 @@ use rustc::hir::Node; use rustc::hir::def_id::DefId; use rustc::lint::builtin::{SAFE_EXTERN_STATICS, SAFE_PACKED_BORROWS, UNUSED_UNSAFE}; use rustc::mir::*; -use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{PlaceContext, Visitor, MutatingUseContext}; use syntax::ast; use syntax::symbol::Symbol; @@ -152,7 +152,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { place: &Place<'tcx>, context: PlaceContext<'tcx>, location: Location) { - if let PlaceContext::Borrow { .. } = context { + if context.is_borrow() { if util::is_disaligned(self.tcx, self.mir, self.param_env, place) { let source_info = self.source_info; let lint_root = @@ -193,9 +193,11 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { } ty::Adt(adt, _) => { if adt.is_union() { - if context == PlaceContext::Store || - context == PlaceContext::AsmOutput || - context == PlaceContext::Drop + if context == PlaceContext::MutatingUse(MutatingUseContext::Store) || + context == PlaceContext::MutatingUse(MutatingUseContext::Drop) || + context == PlaceContext::MutatingUse( + MutatingUseContext::AsmOutput + ) { let elem_ty = match elem { &ProjectionElem::Field(_, ty) => ty, diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 626baf207eebc..51644a6bba236 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -16,7 +16,7 @@ use rustc::hir::def::Def; use rustc::mir::{Constant, Location, Place, Mir, Operand, Rvalue, Local}; use rustc::mir::{NullOp, UnOp, StatementKind, Statement, BasicBlock, LocalKind}; use rustc::mir::{TerminatorKind, ClearCrossCrate, SourceInfo, BinOp, ProjectionElem}; -use rustc::mir::visit::{Visitor, PlaceContext}; +use rustc::mir::visit::{Visitor, PlaceContext, MutatingUseContext, NonMutatingUseContext}; use rustc::mir::interpret::{ ConstEvalErr, EvalErrorKind, Scalar, GlobalId, EvalResult }; @@ -529,17 +529,18 @@ impl<'tcx> Visitor<'tcx> for CanConstProp { // Constants must have at most one write // FIXME(oli-obk): we could be more powerful here, if the multiple writes // only occur in independent execution paths - Store => if self.found_assignment[local] { + MutatingUse(MutatingUseContext::Store) => if self.found_assignment[local] { self.can_const_prop[local] = false; } else { self.found_assignment[local] = true }, // Reading constants is allowed an arbitrary number of times - Copy | Move | - StorageDead | StorageLive | - Validate | - Projection(_) | - Inspect => {}, + NonMutatingUse(NonMutatingUseContext::Copy) | + NonMutatingUse(NonMutatingUseContext::Move) | + NonMutatingUse(NonMutatingUseContext::Inspect) | + NonMutatingUse(NonMutatingUseContext::Projection) | + MutatingUse(MutatingUseContext::Projection) | + NonUse(_) => {}, _ => self.can_const_prop[local] = false, } } diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 790e6e82723ef..629211a7b554d 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -23,7 +23,7 @@ //! move analysis runs after promotion on broken MIR. use rustc::mir::*; -use rustc::mir::visit::{PlaceContext, MutVisitor, Visitor}; +use rustc::mir::visit::{PlaceContext, MutatingUseContext, MutVisitor, Visitor}; use rustc::mir::traversal::ReversePostorder; use rustc::ty::TyCtxt; use syntax_pos::Span; @@ -97,11 +97,11 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { // Ignore drops, if the temp gets promoted, // then it's constant and thus drop is noop. - // Storage live ranges are also irrelevant. - if context.is_drop() || context.is_storage_marker() { + // Non-uses are also irrelevent. + if context.is_drop() || !context.is_use() { debug!( - "visit_local: context.is_drop={:?} context.is_storage_marker={:?}", - context.is_drop(), context.is_storage_marker(), + "visit_local: context.is_drop={:?} context.is_use={:?}", + context.is_drop(), context.is_use(), ); return; } @@ -110,9 +110,9 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { debug!("visit_local: temp={:?}", temp); if *temp == TempState::Undefined { match context { - PlaceContext::Store | - PlaceContext::AsmOutput | - PlaceContext::Call => { + PlaceContext::MutatingUse(MutatingUseContext::Store) | + PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) | + PlaceContext::MutatingUse(MutatingUseContext::Call) => { *temp = TempState::Defined { location, uses: 0 @@ -124,10 +124,7 @@ impl<'tcx> Visitor<'tcx> for TempCollector<'tcx> { } else if let TempState::Defined { ref mut uses, .. } = *temp { // We always allow borrows, even mutable ones, as we need // to promote mutable borrows of some ZSTs e.g. `&mut []`. - let allowed_use = match context { - PlaceContext::Borrow {..} => true, - _ => context.is_nonmutating_use() - }; + let allowed_use = context.is_borrow() || context.is_nonmutating_use(); debug!("visit_local: allowed_use={:?}", allowed_use); if allowed_use { *uses += 1; diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 29e6ef18d2d30..c78fb51961819 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -26,7 +26,7 @@ use rustc::ty::cast::CastTy; use rustc::ty::query::Providers; use rustc::mir::*; use rustc::mir::traversal::ReversePostorder; -use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{PlaceContext, Visitor, MutatingUseContext, NonMutatingUseContext}; use rustc::middle::lang_items; use rustc_target::spec::abi::Abi; use syntax::ast::LitKind; @@ -271,7 +271,11 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> { // This must be an explicit assignment. _ => { // Catch more errors in the destination. - self.visit_place(dest, PlaceContext::Store, location); + self.visit_place( + dest, + PlaceContext::MutatingUse(MutatingUseContext::Store), + location + ); self.statement_like(); } } @@ -610,10 +614,17 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } if is_reborrow { - self.super_place(place, PlaceContext::Borrow { - region, - kind - }, location); + let ctx = match kind { + BorrowKind::Shared => + PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(region)), + BorrowKind::Shallow => + PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(region)), + BorrowKind::Unique => + PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(region)), + BorrowKind::Mut { .. } => + PlaceContext::MutatingUse(MutatingUseContext::Borrow(region)), + }; + self.super_place(place, ctx, location); } else { self.super_rvalue(rvalue, location); } diff --git a/src/librustc_mir/transform/simplify.rs b/src/librustc_mir/transform/simplify.rs index a6e0932bf0ace..c20d40af50151 100644 --- a/src/librustc_mir/transform/simplify.rs +++ b/src/librustc_mir/transform/simplify.rs @@ -331,8 +331,10 @@ struct DeclMarker { impl<'tcx> Visitor<'tcx> for DeclMarker { fn visit_local(&mut self, local: &Local, ctx: PlaceContext<'tcx>, _: Location) { - // ignore these altogether, they get removed along with their otherwise unused decls. - if ctx != PlaceContext::StorageLive && ctx != PlaceContext::StorageDead { + // Ignore storage markers altogether, they get removed along with their otherwise unused + // decls. + // FIXME: Extend this to all non-uses. + if !ctx.is_storage_marker() { self.locals.insert(*local); } } diff --git a/src/librustc_mir/transform/uniform_array_move_out.rs b/src/librustc_mir/transform/uniform_array_move_out.rs index b123a846596e5..949b8f74f71e9 100644 --- a/src/librustc_mir/transform/uniform_array_move_out.rs +++ b/src/librustc_mir/transform/uniform_array_move_out.rs @@ -39,7 +39,7 @@ use rustc::ty; use rustc::ty::TyCtxt; use rustc::mir::*; -use rustc::mir::visit::{Visitor, PlaceContext}; +use rustc::mir::visit::{Visitor, PlaceContext, NonUseContext}; use transform::{MirPass, MirSource}; use util::patch::MirPatch; use rustc_data_structures::indexed_vec::{IndexVec}; @@ -316,8 +316,8 @@ impl<'tcx> Visitor<'tcx> for RestoreDataCollector { location: Location) { let local_use = &mut self.locals_use[*local]; match context { - PlaceContext::StorageLive => local_use.alive = Some(location), - PlaceContext::StorageDead => local_use.dead = Some(location), + PlaceContext::NonUse(NonUseContext::StorageLive) => local_use.alive = Some(location), + PlaceContext::NonUse(NonUseContext::StorageDead) => local_use.dead = Some(location), _ => { local_use.use_count += 1; if local_use.first_use.is_none() { diff --git a/src/librustc_mir/util/collect_writes.rs b/src/librustc_mir/util/collect_writes.rs index 23f753f8569bb..3e1d0852deedf 100644 --- a/src/librustc_mir/util/collect_writes.rs +++ b/src/librustc_mir/util/collect_writes.rs @@ -43,24 +43,8 @@ impl<'tcx> Visitor<'tcx> for FindLocalAssignmentVisitor { return; } - if is_place_assignment(&place_context) { + if place_context.is_place_assignment() { self.locations.push(location); } } } - -/// Returns true if this place context represents an assignment statement -crate fn is_place_assignment(place_context: &PlaceContext) -> bool { - match *place_context { - PlaceContext::Store | PlaceContext::Call | PlaceContext::AsmOutput => true, - PlaceContext::Drop - | PlaceContext::Inspect - | PlaceContext::Borrow { .. } - | PlaceContext::Projection(..) - | PlaceContext::Copy - | PlaceContext::Move - | PlaceContext::StorageLive - | PlaceContext::StorageDead - | PlaceContext::Validate => false, - } -} diff --git a/src/librustc_mir/util/liveness.rs b/src/librustc_mir/util/liveness.rs index 420ca4efde37d..d16094e8238de 100644 --- a/src/librustc_mir/util/liveness.rs +++ b/src/librustc_mir/util/liveness.rs @@ -33,7 +33,9 @@ //! generator yield points, all pre-existing references are invalidated, so this //! doesn't matter). -use rustc::mir::visit::{PlaceContext, Visitor}; +use rustc::mir::visit::{ + PlaceContext, Visitor, MutatingUseContext, NonMutatingUseContext, NonUseContext, +}; use rustc::mir::Local; use rustc::mir::*; use rustc::ty::{item_path, TyCtxt}; @@ -161,10 +163,10 @@ pub fn categorize<'tcx>(context: PlaceContext<'tcx>) -> Option { /////////////////////////////////////////////////////////////////////////// // DEFS - PlaceContext::Store | + PlaceContext::MutatingUse(MutatingUseContext::Store) | // This is potentially both a def and a use... - PlaceContext::AsmOutput | + PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) | // We let Call define the result in both the success and // unwind cases. This is not really correct, however it @@ -172,12 +174,12 @@ pub fn categorize<'tcx>(context: PlaceContext<'tcx>) -> Option { // generate MIR. To do things properly, we would apply // the def in call only to the input from the success // path and not the unwind path. -nmatsakis - PlaceContext::Call | + PlaceContext::MutatingUse(MutatingUseContext::Call) | // Storage live and storage dead aren't proper defines, but we can ignore // values that come before them. - PlaceContext::StorageLive | - PlaceContext::StorageDead => Some(DefUse::Def), + PlaceContext::NonUse(NonUseContext::StorageLive) | + PlaceContext::NonUse(NonUseContext::StorageDead) => Some(DefUse::Def), /////////////////////////////////////////////////////////////////////////// // REGULAR USES @@ -186,18 +188,23 @@ pub fn categorize<'tcx>(context: PlaceContext<'tcx>) -> Option { // purposes of NLL, these are special in that **all** the // lifetimes appearing in the variable must be live for each regular use. - PlaceContext::Projection(..) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) | + PlaceContext::MutatingUse(MutatingUseContext::Projection) | // Borrows only consider their local used at the point of the borrow. // This won't affect the results since we use this analysis for generators // and we only care about the result at suspension points. Borrows cannot // cross suspension points so this behavior is unproblematic. - PlaceContext::Borrow { .. } | - - PlaceContext::Inspect | - PlaceContext::Copy | - PlaceContext::Move | - PlaceContext::Validate => + PlaceContext::MutatingUse(MutatingUseContext::Borrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow(..)) | + + PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) | + PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) | + PlaceContext::NonUse(NonUseContext::AscribeUserTy) | + PlaceContext::NonUse(NonUseContext::Validate) => Some(DefUse::Use), /////////////////////////////////////////////////////////////////////////// @@ -208,7 +215,7 @@ pub fn categorize<'tcx>(context: PlaceContext<'tcx>) -> Option { // uses in drop are special because `#[may_dangle]` // attributes can affect whether lifetimes must be live. - PlaceContext::Drop => + PlaceContext::MutatingUse(MutatingUseContext::Drop) => Some(DefUse::Drop), } } From 8a6f4c819bef9e7bc473fbfe14dd2a4f59447f93 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 18 Nov 2018 10:07:25 -0500 Subject: [PATCH 04/10] remove "approx env bounds" if we already know from trait --- src/librustc/infer/outlives/obligations.rs | 30 ++++++++++++++---- src/test/ui/nll/ty-outlives/issue-55756.rs | 37 ++++++++++++++++++++++ 2 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 src/test/ui/nll/ty-outlives/issue-55756.rs diff --git a/src/librustc/infer/outlives/obligations.rs b/src/librustc/infer/outlives/obligations.rs index 5db850f1588b6..0215b0380bdf3 100644 --- a/src/librustc/infer/outlives/obligations.rs +++ b/src/librustc/infer/outlives/obligations.rs @@ -388,22 +388,38 @@ where // rule might not apply (but another rule might). For now, we err // on the side of adding too few edges into the graph. + // Compute the bounds we can derive from the trait definition. + // These are guaranteed to apply, no matter the inference + // results. + let trait_bounds: Vec<_> = self.verify_bound + .projection_declared_bounds_from_trait(projection_ty) + .collect(); + // Compute the bounds we can derive from the environment. This // is an "approximate" match -- in some cases, these bounds // may not apply. - let approx_env_bounds = self.verify_bound + let mut approx_env_bounds = self.verify_bound .projection_approx_declared_bounds_from_env(projection_ty); debug!( "projection_must_outlive: approx_env_bounds={:?}", approx_env_bounds ); - // Compute the bounds we can derive from the trait definition. - // These are guaranteed to apply, no matter the inference - // results. - let trait_bounds: Vec<_> = self.verify_bound - .projection_declared_bounds_from_trait(projection_ty) - .collect(); + // Remove outlives bounds that we get from the environment but + // which are also deducable from the trait. This arises (cc + // #55756) in cases where you have e.g. `>::Item: + // 'a` in the environment but `trait Foo<'b> { type Item: 'b + // }` in the trait definition. + approx_env_bounds.retain(|bound| { + match bound.0.sty { + ty::Projection(projection_ty) => { + self.verify_bound.projection_declared_bounds_from_trait(projection_ty) + .all(|r| r != bound.1) + } + + _ => panic!("expected only projection types from env, not {:?}", bound.0), + } + }); // If declared bounds list is empty, the only applicable rule is // OutlivesProjectionComponent. If there are inference variables, diff --git a/src/test/ui/nll/ty-outlives/issue-55756.rs b/src/test/ui/nll/ty-outlives/issue-55756.rs new file mode 100644 index 0000000000000..cda3915849e2d --- /dev/null +++ b/src/test/ui/nll/ty-outlives/issue-55756.rs @@ -0,0 +1,37 @@ +// Regression test for #55756. +// +// In this test, the result of `self.callee` is a projection `>::Guard`. As it may contain a destructor, the dropck +// rules require that this type outlivess the scope of `state`. Unfortunately, +// our region inference is not smart enough to figure out how to +// translate a requirement like +// +// >::guard: 'r +// +// into a requirement that `'0: 'r` -- in particular, it fails to do +// so because it *also* knows that `>::Guard: 'a` +// from the trait definition. Faced with so many choices, the current +// solver opts to do nothing. +// +// Fixed by tweaking the solver to recognize that the constraint from +// the environment duplicates one from the trait. +// +// compile-pass + +#![crate_type="lib"] + +pub trait Database<'a> { + type Guard: 'a; +} + +pub struct Stateful<'a, D: 'a>(&'a D); + +impl<'b, D: for <'a> Database<'a>> Stateful<'b, D> { + pub fn callee<'a>(&'a self) -> >::Guard { + unimplemented!() + } + pub fn caller<'a>(&'a self) -> >::Guard { + let state = self.callee(); + unimplemented!() + } +} From 410f5207874eab36cbfac080782d00c86642f2f1 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 16 Nov 2018 08:25:46 -0500 Subject: [PATCH 05/10] improve debug output related to bound calculation --- src/librustc/traits/mod.rs | 7 ++++++- src/librustc_typeck/collect.rs | 1 + src/librustc_typeck/outlives/implicit_infer.rs | 17 +++++++++++++---- src/librustc_typeck/outlives/mod.rs | 3 +++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 8bceebb23950c..c7e5d89007418 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -693,7 +693,12 @@ fn do_normalize_predicates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, predicates: Vec>) -> Result>, ErrorReported> { - debug!("do_normalize_predicates({:?})", predicates); + debug!( + "do_normalize_predicates(predicates={:?}, region_context={:?}, cause={:?})", + predicates, + region_context, + cause, + ); let span = cause.span; tcx.infer_ctxt().enter(|infcx| { // FIXME. We should really... do something with these region diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index eb52a013b0566..b33b21478ca4b 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -1591,6 +1591,7 @@ fn predicates_defined_on<'a, 'tcx>( tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId, ) -> ty::GenericPredicates<'tcx> { + debug!("predicates_defined_on({:?})", def_id); let explicit = tcx.explicit_predicates_of(def_id); let span = tcx.def_span(def_id); let predicates = explicit.predicates.into_iter().chain( diff --git a/src/librustc_typeck/outlives/implicit_infer.rs b/src/librustc_typeck/outlives/implicit_infer.rs index 132da8f5cea8d..101edac04c7d4 100644 --- a/src/librustc_typeck/outlives/implicit_infer.rs +++ b/src/librustc_typeck/outlives/implicit_infer.rs @@ -245,6 +245,7 @@ fn insert_required_predicates_to_be_wf<'tcx>( } } +#[derive(Debug)] pub struct IgnoreSelfTy(bool); /// We also have to check the explicit predicates @@ -270,10 +271,18 @@ pub fn check_explicit_predicates<'tcx>( explicit_map: &mut ExplicitPredicatesMap<'tcx>, ignore_self_ty: IgnoreSelfTy, ) { - debug!("def_id = {:?}", &def_id); - debug!("substs = {:?}", &substs); - debug!("explicit_map = {:?}", explicit_map); - debug!("required_predicates = {:?}", required_predicates); + debug!( + "check_explicit_predicates(def_id={:?}, \ + substs={:?}, \ + explicit_map={:?}, \ + required_predicates={:?}, \ + ignore_self_ty={:?})", + def_id, + substs, + explicit_map, + required_predicates, + ignore_self_ty, + ); let explicit_predicates = explicit_map.explicit_predicates_of(tcx, *def_id); for outlives_predicate in explicit_predicates.iter() { diff --git a/src/librustc_typeck/outlives/mod.rs b/src/librustc_typeck/outlives/mod.rs index cca77b20d9b30..1eb53ffc730f6 100644 --- a/src/librustc_typeck/outlives/mod.rs +++ b/src/librustc_typeck/outlives/mod.rs @@ -67,6 +67,9 @@ fn inferred_outlives_of<'a, 'tcx>( } err.emit(); } + + debug!("inferred_outlives_of({:?}) = {:?}", item_def_id, predicates); + predicates } From 84afecccdca8d771e5ae0734bda928304df30dc3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 16 Nov 2018 08:58:55 -0500 Subject: [PATCH 06/10] handle trait objects formed from traits with `Self::Foo: 'a` clauses --- .../outlives/implicit_infer.rs | 21 ++++++++++++++----- .../ui/rfc-2093-infer-outlives/issue-54467.rs | 17 +++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 src/test/ui/rfc-2093-infer-outlives/issue-54467.rs diff --git a/src/librustc_typeck/outlives/implicit_infer.rs b/src/librustc_typeck/outlives/implicit_infer.rs index 101edac04c7d4..30e304375fe0e 100644 --- a/src/librustc_typeck/outlives/implicit_infer.rs +++ b/src/librustc_typeck/outlives/implicit_infer.rs @@ -14,6 +14,7 @@ use rustc::hir::def_id::DefId; use rustc::hir::itemlikevisit::ItemLikeVisitor; use rustc::ty::subst::{Kind, Subst, UnpackedKind}; use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::fold::TypeFoldable; use rustc::util::nodemap::FxHashMap; use super::explicit::ExplicitPredicatesMap; @@ -311,13 +312,23 @@ pub fn check_explicit_predicates<'tcx>( // // Note that we do this check for self **before** applying `substs`. In the // case that `substs` come from a `dyn Trait` type, our caller will have - // included `Self = dyn Trait<'x, X>` as the value for `Self`. If we were + // included `Self = usize` as the value for `Self`. If we were // to apply the substs, and not filter this predicate, we might then falsely // conclude that e.g. `X: 'x` was a reasonable inferred requirement. - if let UnpackedKind::Type(ty) = outlives_predicate.0.unpack() { - if ty.is_self() && ignore_self_ty.0 { - debug!("skipping self ty = {:?}", &ty); - continue; + // + // Another similar case is where we have a inferred + // requirement like `::Foo: 'b`. We presently + // ignore such requirements as well (cc #54467)-- though + // conceivably it might be better if we could extract the `Foo + // = X` binding from the object type (there must be such a + // binding) and thus infer an outlives requirement that `X: + // 'b`. + if ignore_self_ty.0 { + if let UnpackedKind::Type(ty) = outlives_predicate.0.unpack() { + if ty.has_self_ty() { + debug!("skipping self ty = {:?}", &ty); + continue; + } } } diff --git a/src/test/ui/rfc-2093-infer-outlives/issue-54467.rs b/src/test/ui/rfc-2093-infer-outlives/issue-54467.rs new file mode 100644 index 0000000000000..438923e29246c --- /dev/null +++ b/src/test/ui/rfc-2093-infer-outlives/issue-54467.rs @@ -0,0 +1,17 @@ +// Regression test for #54467: +// +// Here, the trait object has an "inferred outlives" requirement that +// `>::Item: 'a`; but since we don't know what +// `Self` is, we were (incorrectly) messing things up, leading to +// strange errors. This test ensures that we do not give compilation +// errors. +// +// compile-pass + +trait MyIterator<'a>: Iterator where Self::Item: 'a { } + +struct MyStruct<'a, A> { + item: Box> +} + +fn main() { } From 0a26e7959aad8b354636862c3847940320307cc9 Mon Sep 17 00:00:00 2001 From: varkor Date: Sat, 10 Nov 2018 18:08:50 +0000 Subject: [PATCH 07/10] Rewrite `...` as `..=` as a MachineApplicable 2018 idiom lint --- src/librustc/lint/context.rs | 7 ++- src/librustc/lint/mod.rs | 2 +- src/librustc_lint/builtin.rs | 44 ++++++++++++++----- src/librustc_lint/unused.rs | 4 +- .../lint/inclusive-range-pattern-syntax.fixed | 6 +++ .../ui/lint/inclusive-range-pattern-syntax.rs | 6 +++ .../inclusive-range-pattern-syntax.stderr | 6 +++ .../range-inclusive-pattern-precedence.stderr | 4 +- 8 files changed, 62 insertions(+), 17 deletions(-) diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 7e2532bb1c4e8..ebb495fdfcee6 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -1020,9 +1020,12 @@ impl<'a> ast_visit::Visitor<'a> for EarlyContext<'a> { } fn visit_pat(&mut self, p: &'a ast::Pat) { - run_lints!(self, check_pat, p); + let mut visit_subpats = true; + run_lints!(self, check_pat, p, &mut visit_subpats); self.check_id(p.id); - ast_visit::walk_pat(self, p); + if visit_subpats { + ast_visit::walk_pat(self, p); + } } fn visit_expr(&mut self, e: &'a ast::Expr) { diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 5ac0c0d32dcdc..c1e439ce607d4 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -341,7 +341,7 @@ pub trait EarlyLintPass: LintPass { fn check_block_post(&mut self, _: &EarlyContext<'_>, _: &ast::Block) { } fn check_stmt(&mut self, _: &EarlyContext<'_>, _: &ast::Stmt) { } fn check_arm(&mut self, _: &EarlyContext<'_>, _: &ast::Arm) { } - fn check_pat(&mut self, _: &EarlyContext<'_>, _: &ast::Pat) { } + fn check_pat(&mut self, _: &EarlyContext<'_>, _: &ast::Pat, _: &mut bool) { } fn check_expr(&mut self, _: &EarlyContext<'_>, _: &ast::Expr) { } fn check_expr_post(&mut self, _: &EarlyContext<'_>, _: &ast::Expr) { } fn check_ty(&mut self, _: &EarlyContext<'_>, _: &ast::Ty) { } diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 19f9168cf0a0e..fec9542680fb0 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -40,6 +40,8 @@ use rustc::util::nodemap::FxHashSet; use syntax::tokenstream::{TokenTree, TokenStream}; use syntax::ast; +use syntax::ptr::P; +use syntax::ast::Expr; use syntax::attr; use syntax::source_map::Spanned; use syntax::edition::Edition; @@ -47,6 +49,7 @@ use syntax::feature_gate::{AttributeGate, AttributeType, Stability, deprecated_a use syntax_pos::{BytePos, Span, SyntaxContext}; use syntax::symbol::keywords; use syntax::errors::{Applicability, DiagnosticBuilder}; +use syntax::print::pprust::expr_to_string; use rustc::hir::{self, GenericParamKind, PatKind}; use rustc::hir::intravisit::FnKind; @@ -1477,21 +1480,42 @@ impl LintPass for EllipsisInclusiveRangePatterns { } impl EarlyLintPass for EllipsisInclusiveRangePatterns { - fn check_pat(&mut self, cx: &EarlyContext, pat: &ast::Pat) { - use self::ast::{PatKind, RangeEnd, RangeSyntax}; + fn check_pat(&mut self, cx: &EarlyContext, pat: &ast::Pat, visit_subpats: &mut bool) { + use self::ast::{PatKind, RangeEnd, RangeSyntax::DotDotDot}; + + /// If `pat` is a `...` pattern, return the start and end of the range, as well as the span + /// corresponding to the ellipsis. + fn matches_ellipsis_pat(pat: &ast::Pat) -> Option<(&P, &P, Span)> { + match &pat.node { + PatKind::Range(a, b, Spanned { span, node: RangeEnd::Included(DotDotDot), .. }) => { + Some((a, b, *span)) + } + _ => None, + } + } + + let (parenthesise, endpoints) = match &pat.node { + PatKind::Ref(subpat, _) => (true, matches_ellipsis_pat(&subpat)), + _ => (false, matches_ellipsis_pat(pat)), + }; - if let PatKind::Range( - _, _, Spanned { span, node: RangeEnd::Included(RangeSyntax::DotDotDot) } - ) = pat.node { + if let Some((start, end, join)) = endpoints { let msg = "`...` range patterns are deprecated"; + let suggestion = "use `..=` for an inclusive range"; + let (span, replacement) = if parenthesise { + *visit_subpats = false; + (pat.span, format!("&({}..={})", expr_to_string(&start), expr_to_string(&end))) + } else { + (join, "..=".to_owned()) + }; let mut err = cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, span, msg); err.span_suggestion_short_with_applicability( - span, "use `..=` for an inclusive range", "..=".to_owned(), - // FIXME: outstanding problem with precedence in ref patterns: - // https://github.com/rust-lang/rust/issues/51043#issuecomment-392252285 - Applicability::MaybeIncorrect + span, + suggestion, + replacement, + Applicability::MachineApplicable, ); - err.emit() + err.emit(); } } } diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 9ccfecd46fdb2..049bcc3010be1 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -397,12 +397,12 @@ impl EarlyLintPass for UnusedParens { self.check_unused_parens_expr(cx, &value, msg, followed_by_block); } - fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { + fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat, _: &mut bool) { use ast::PatKind::{Paren, Range}; // The lint visitor will visit each subpattern of `p`. We do not want to lint any range // pattern no matter where it occurs in the pattern. For something like `&(a..=b)`, there // is a recursive `check_pat` on `a` and `b`, but we will assume that if there are - // unnecessry parens they serve a purpose of readability. + // unnecessary parens they serve a purpose of readability. if let Paren(ref pat) = p.node { match pat.node { Range(..) => {} diff --git a/src/test/ui/lint/inclusive-range-pattern-syntax.fixed b/src/test/ui/lint/inclusive-range-pattern-syntax.fixed index d16859df79e25..f0aee8a51f18b 100644 --- a/src/test/ui/lint/inclusive-range-pattern-syntax.fixed +++ b/src/test/ui/lint/inclusive-range-pattern-syntax.fixed @@ -20,4 +20,10 @@ fn main() { //~^ WARN `...` range patterns are deprecated _ => {} } + + match &despondency { + &(1..=2) => {} + //~^ WARN `...` range patterns are deprecated + _ => {} + } } diff --git a/src/test/ui/lint/inclusive-range-pattern-syntax.rs b/src/test/ui/lint/inclusive-range-pattern-syntax.rs index 9d418aec0858f..97bc04faa774b 100644 --- a/src/test/ui/lint/inclusive-range-pattern-syntax.rs +++ b/src/test/ui/lint/inclusive-range-pattern-syntax.rs @@ -20,4 +20,10 @@ fn main() { //~^ WARN `...` range patterns are deprecated _ => {} } + + match &despondency { + &1...2 => {} + //~^ WARN `...` range patterns are deprecated + _ => {} + } } diff --git a/src/test/ui/lint/inclusive-range-pattern-syntax.stderr b/src/test/ui/lint/inclusive-range-pattern-syntax.stderr index de04fed589b23..9226137f1152f 100644 --- a/src/test/ui/lint/inclusive-range-pattern-syntax.stderr +++ b/src/test/ui/lint/inclusive-range-pattern-syntax.stderr @@ -10,3 +10,9 @@ note: lint level defined here LL | #![warn(ellipsis_inclusive_range_patterns)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +warning: `...` range patterns are deprecated + --> $DIR/inclusive-range-pattern-syntax.rs:25:9 + | +LL | &1...2 => {} + | ^^^^^^ help: use `..=` for an inclusive range + diff --git a/src/test/ui/range/range-inclusive-pattern-precedence.stderr b/src/test/ui/range/range-inclusive-pattern-precedence.stderr index cd5ce3035c683..3ac6be2b8ea6a 100644 --- a/src/test/ui/range/range-inclusive-pattern-precedence.stderr +++ b/src/test/ui/range/range-inclusive-pattern-precedence.stderr @@ -11,10 +11,10 @@ LL | box 10..=15 => {} | ^^^^^^^ help: add parentheses to clarify the precedence: `(10 ..=15)` warning: `...` range patterns are deprecated - --> $DIR/range-inclusive-pattern-precedence.rs:24:11 + --> $DIR/range-inclusive-pattern-precedence.rs:24:9 | LL | &0...9 => {} - | ^^^ help: use `..=` for an inclusive range + | ^^^^^^ help: use `..=` for an inclusive range | note: lint level defined here --> $DIR/range-inclusive-pattern-precedence.rs:19:9 From 9ab3b0fa0032ae539cb1093771c81c6df0b94374 Mon Sep 17 00:00:00 2001 From: varkor Date: Sat, 10 Nov 2018 21:27:40 +0000 Subject: [PATCH 08/10] Use non-short suggestion for parenthesised ..= --- src/librustc_lint/builtin.rs | 28 +++++++++++-------- .../inclusive-range-pattern-syntax.stderr | 2 +- .../range-inclusive-pattern-precedence.stderr | 2 +- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index fec9542680fb0..7291e27048280 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -1502,20 +1502,26 @@ impl EarlyLintPass for EllipsisInclusiveRangePatterns { if let Some((start, end, join)) = endpoints { let msg = "`...` range patterns are deprecated"; let suggestion = "use `..=` for an inclusive range"; - let (span, replacement) = if parenthesise { + if parenthesise { *visit_subpats = false; - (pat.span, format!("&({}..={})", expr_to_string(&start), expr_to_string(&end))) + let mut err = cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, pat.span, msg); + err.span_suggestion_with_applicability( + pat.span, + suggestion, + format!("&({}..={})", expr_to_string(&start), expr_to_string(&end)), + Applicability::MachineApplicable, + ); + err.emit(); } else { - (join, "..=".to_owned()) + let mut err = cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, join, msg); + err.span_suggestion_short_with_applicability( + join, + suggestion, + "..=".to_owned(), + Applicability::MachineApplicable, + ); + err.emit(); }; - let mut err = cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, span, msg); - err.span_suggestion_short_with_applicability( - span, - suggestion, - replacement, - Applicability::MachineApplicable, - ); - err.emit(); } } } diff --git a/src/test/ui/lint/inclusive-range-pattern-syntax.stderr b/src/test/ui/lint/inclusive-range-pattern-syntax.stderr index 9226137f1152f..b13afdbc023d4 100644 --- a/src/test/ui/lint/inclusive-range-pattern-syntax.stderr +++ b/src/test/ui/lint/inclusive-range-pattern-syntax.stderr @@ -14,5 +14,5 @@ warning: `...` range patterns are deprecated --> $DIR/inclusive-range-pattern-syntax.rs:25:9 | LL | &1...2 => {} - | ^^^^^^ help: use `..=` for an inclusive range + | ^^^^^^ help: use `..=` for an inclusive range: `&(1..=2)` diff --git a/src/test/ui/range/range-inclusive-pattern-precedence.stderr b/src/test/ui/range/range-inclusive-pattern-precedence.stderr index 3ac6be2b8ea6a..6fa67a5d4fa30 100644 --- a/src/test/ui/range/range-inclusive-pattern-precedence.stderr +++ b/src/test/ui/range/range-inclusive-pattern-precedence.stderr @@ -14,7 +14,7 @@ warning: `...` range patterns are deprecated --> $DIR/range-inclusive-pattern-precedence.rs:24:9 | LL | &0...9 => {} - | ^^^^^^ help: use `..=` for an inclusive range + | ^^^^^^ help: use `..=` for an inclusive range: `&(0..=9)` | note: lint level defined here --> $DIR/range-inclusive-pattern-precedence.rs:19:9 From 45418275c2ca9633516d1a302e94afec57b79d1c Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Thu, 8 Nov 2018 19:06:16 -0600 Subject: [PATCH 09/10] don't inline `pub use some_crate` unless directly asked to --- src/doc/rustdoc/src/the-doc-attribute.md | 3 ++ src/librustdoc/clean/mod.rs | 15 +++++++- .../inline_cross/auxiliary/use_crate.rs | 15 ++++++++ .../inline_cross/auxiliary/use_crate_2.rs | 11 ++++++ src/test/rustdoc/inline_cross/use_crate.rs | 37 +++++++++++++++++++ src/test/rustdoc/src-links-external.rs | 1 + 6 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 src/test/rustdoc/inline_cross/auxiliary/use_crate.rs create mode 100644 src/test/rustdoc/inline_cross/auxiliary/use_crate_2.rs create mode 100644 src/test/rustdoc/inline_cross/use_crate.rs diff --git a/src/doc/rustdoc/src/the-doc-attribute.md b/src/doc/rustdoc/src/the-doc-attribute.md index 296422744fa40..61e5b3d0133ff 100644 --- a/src/doc/rustdoc/src/the-doc-attribute.md +++ b/src/doc/rustdoc/src/the-doc-attribute.md @@ -186,6 +186,9 @@ mod bar { Now we'll have a `Re-exports` line, and `Bar` will not link to anywhere. +One special case: In Rust 2018 and later, if you `pub use` one of your dependencies, `rustdoc` will +not eagerly inline it as a module unless you add `#[doc(inline)}`. + ## `#[doc(hidden)]` Any item annotated with `#[doc(hidden)]` will not appear in the documentation, unless diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index bed3f7f99ae02..4fe6cec301546 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -3491,13 +3491,16 @@ impl Clean> for doctree::Import { // forcefully don't inline if this is not public or if the // #[doc(no_inline)] attribute is present. // Don't inline doc(hidden) imports so they can be stripped at a later stage. - let denied = !self.vis.node.is_pub() || self.attrs.iter().any(|a| { + let mut denied = !self.vis.node.is_pub() || self.attrs.iter().any(|a| { a.name() == "doc" && match a.meta_item_list() { Some(l) => attr::list_contains_name(&l, "no_inline") || attr::list_contains_name(&l, "hidden"), None => false, } }); + // Also check whether imports were asked to be inlined, in case we're trying to re-export a + // crate in Rust 2018+ + let please_inline = self.attrs.lists("doc").has_word("inline"); let path = self.path.clean(cx); let inner = if self.glob { if !denied { @@ -3510,6 +3513,16 @@ impl Clean> for doctree::Import { Import::Glob(resolve_use_source(cx, path)) } else { let name = self.name; + if !please_inline { + match path.def { + Def::Mod(did) => if !did.is_local() && did.index == CRATE_DEF_INDEX { + // if we're `pub use`ing an extern crate root, don't inline it unless we + // were specifically asked for it + denied = true; + } + _ => {} + } + } if !denied { let mut visited = FxHashSet::default(); if let Some(items) = inline::try_inline(cx, path.def, name, &mut visited) { diff --git a/src/test/rustdoc/inline_cross/auxiliary/use_crate.rs b/src/test/rustdoc/inline_cross/auxiliary/use_crate.rs new file mode 100644 index 0000000000000..55202de1981be --- /dev/null +++ b/src/test/rustdoc/inline_cross/auxiliary/use_crate.rs @@ -0,0 +1,15 @@ +// Copyright 2018 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. + +pub mod asdf { + pub struct SomeStruct; +} + +pub trait SomeTrait {} diff --git a/src/test/rustdoc/inline_cross/auxiliary/use_crate_2.rs b/src/test/rustdoc/inline_cross/auxiliary/use_crate_2.rs new file mode 100644 index 0000000000000..1f11cbc4da718 --- /dev/null +++ b/src/test/rustdoc/inline_cross/auxiliary/use_crate_2.rs @@ -0,0 +1,11 @@ +// Copyright 2018 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. + +pub struct SomethingElse; diff --git a/src/test/rustdoc/inline_cross/use_crate.rs b/src/test/rustdoc/inline_cross/use_crate.rs new file mode 100644 index 0000000000000..a98704446ee76 --- /dev/null +++ b/src/test/rustdoc/inline_cross/use_crate.rs @@ -0,0 +1,37 @@ +// Copyright 2018 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. + +// aux-build:use_crate.rs +// aux-build:use_crate_2.rs +// build-aux-docs +// edition:2018 +// compile-flags:--extern use_crate --extern use_crate_2 -Z unstable-options + +// During the buildup to Rust 2018, rustdoc would eagerly inline `pub use some_crate;` as if it +// were a module, so we changed it to make `pub use`ing crate roots remain as a `pub use` statement +// in docs... unless you added `#[doc(inline)]`. + +#![crate_name = "local"] + +// @!has-dir local/use_crate +// @has local/index.html +// @has - '//code' 'pub use use_crate' +pub use use_crate; + +// @has-dir local/asdf +// @has local/asdf/index.html +// @has local/index.html '//a/@href' 'asdf/index.html' +pub use use_crate::asdf; + +// @has-dir local/use_crate_2 +// @has local/use_crate_2/index.html +// @has local/index.html '//a/@href' 'use_crate_2/index.html' +#[doc(inline)] +pub use use_crate_2; diff --git a/src/test/rustdoc/src-links-external.rs b/src/test/rustdoc/src-links-external.rs index d3307bb4d42c1..6cc7f1743ad07 100644 --- a/src/test/rustdoc/src-links-external.rs +++ b/src/test/rustdoc/src-links-external.rs @@ -18,6 +18,7 @@ extern crate src_links_external; // @has foo/bar/index.html '//a/@href' '../../src/src_links_external/src-links-external.rs.html#11' +#[doc(inline)] pub use src_links_external as bar; // @has foo/bar/struct.Foo.html '//a/@href' '../../src/src_links_external/src-links-external.rs.html#11' From 1d1213f2a9c0836341c4f8a2aa7611e64707b5f2 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sun, 18 Nov 2018 19:08:06 -0800 Subject: [PATCH 10/10] Increase `Duration` approximate equal threshold to 1us Previously this threshold when testing was 100ns, but the Windows documentation states: > which is a high resolution (<1us) time stamp which presumably means that we could have up to 1us resolution, which means that 100ns doesn't capture "equivalent" time intervals due to various bits of rounding here and there. It's hoped that this.. Closes #56034 --- src/libstd/time.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstd/time.rs b/src/libstd/time.rs index 90ab349159915..bb795a33746e2 100644 --- a/src/libstd/time.rs +++ b/src/libstd/time.rs @@ -481,7 +481,8 @@ mod tests { let (a, b) = ($a, $b); if a != b { let (a, b) = if a > b {(a, b)} else {(b, a)}; - assert!(a - Duration::new(0, 100) <= b); + assert!(a - Duration::new(0, 1000) <= b, + "{:?} is not almost equal to {:?}", a, b); } }) }