Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utilize inband lifetimes across librustc_mir #52736

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -45,7 +45,7 @@ crate struct BorrowSet<'tcx> {
crate local_map: FxHashMap<mir::Local, FxHashSet<BorrowIndex>>,
}

impl<'tcx> Index<BorrowIndex> for BorrowSet<'tcx> {
impl Index<BorrowIndex> for BorrowSet<'tcx> {
type Output = BorrowData<'tcx>;

fn index(&self, index: BorrowIndex) -> &BorrowData<'tcx> {
@@ -79,7 +79,7 @@ crate struct BorrowData<'tcx> {
crate assigned_place: mir::Place<'tcx>,
}

impl<'tcx> fmt::Display for BorrowData<'tcx> {
impl fmt::Display for BorrowData<'tcx> {
fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
let kind = match self.kind {
mir::BorrowKind::Shared => "",
@@ -96,7 +96,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
}
}

impl<'tcx> BorrowSet<'tcx> {
impl BorrowSet<'tcx> {
pub fn build(tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> Self {
let mut visitor = GatherBorrows {
tcx,
@@ -150,7 +150,7 @@ struct GatherBorrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
pending_activations: FxHashMap<mir::Local, BorrowIndex>,
}

impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
impl Visitor<'tcx> for GatherBorrows<'_, 'gcx, 'tcx> {
fn visit_assign(
&mut self,
block: mir::BasicBlock,
@@ -184,7 +184,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {

return self.super_assign(block, assigned_place, rvalue, location);

fn insert<'a, K, V>(map: &'a mut FxHashMap<K, FxHashSet<V>>, k: &K, v: V)
fn insert<K, V>(map: &mut FxHashMap<K, FxHashSet<V>>, k: &K, v: V)
where
K: Clone + Eq + Hash,
V: Eq + Hash,
@@ -285,7 +285,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
}
}

impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> {
impl GatherBorrows<'_, 'gcx, 'tcx> {
/// Returns true if the borrow represented by `kind` is
/// allowed to be split into separate Reservation and
/// Activation phases.
@@ -27,7 +27,7 @@ use dataflow::move_paths::MovePathIndex;
use dataflow::{FlowAtLocation, MovingOutStatements};
use util::borrowck_errors::{BorrowckErrors, Origin};

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
pub(super) fn report_use_of_moved_or_uninitialized(
&mut self,
_context: Context,
@@ -684,7 +684,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

pub(super) struct IncludingDowncast(bool);

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// End-user visible description of `place` if one can be found. If the
// place is a temporary for instance, None will be returned.
pub(super) fn describe_place(&self, place: &Place<'tcx>) -> Option<String> {
@@ -42,7 +42,7 @@ crate struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> {
pub polonius_output: Option<Rc<Output<RegionVid, BorrowIndex, LocationIndex>>>,
}

impl<'b, 'gcx, 'tcx> Flows<'b, 'gcx, 'tcx> {
impl Flows<'b, 'gcx, 'tcx> {
crate fn new(
borrows: FlowAtLocation<Borrows<'b, 'gcx, 'tcx>>,
uninits: FlowAtLocation<MaybeUninitializedPlaces<'b, 'gcx, 'tcx>>,
@@ -84,7 +84,7 @@ macro_rules! each_flow {
};
}

impl<'b, 'gcx, 'tcx> FlowsAtLocation for Flows<'b, 'gcx, 'tcx> {
impl FlowsAtLocation for Flows<'b, 'gcx, 'tcx> {
fn reset_to_entry_of(&mut self, bb: BasicBlock) {
each_flow!(self, reset_to_entry_of(bb));
}
@@ -102,7 +102,7 @@ impl<'b, 'gcx, 'tcx> FlowsAtLocation for Flows<'b, 'gcx, 'tcx> {
}
}

impl<'b, 'gcx, 'tcx> fmt::Display for Flows<'b, 'gcx, 'tcx> {
impl fmt::Display for Flows<'b, 'gcx, 'tcx> {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
let mut s = String::new();

@@ -75,7 +75,7 @@ pub fn provide(providers: &mut Providers) {
};
}

fn mir_borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> BorrowCheckResult<'tcx> {
fn mir_borrowck(tcx: TyCtxt<'_, 'tcx, 'tcx>, def_id: DefId) -> BorrowCheckResult<'tcx> {
let input_mir = tcx.mir_validated(def_id);
debug!("run query mir_borrowck: {}", tcx.item_path_str(def_id));

@@ -125,8 +125,8 @@ fn mir_borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> BorrowC
opt_closure_req
}

fn do_mir_borrowck<'a, 'gcx, 'tcx>(
infcx: &InferCtxt<'a, 'gcx, 'tcx>,
fn do_mir_borrowck(
infcx: &InferCtxt<'_, 'gcx, 'tcx>,
input_mir: &Mir<'gcx>,
def_id: DefId,
) -> BorrowCheckResult<'gcx> {
@@ -404,7 +404,7 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
// 2. loans made in overlapping scopes do not conflict
// 3. assignments do not affect things loaned out as immutable
// 4. moves do not affect things loaned out in any way
impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
type FlowState = Flows<'cx, 'gcx, 'tcx>;

fn mir(&self) -> &'cx Mir<'tcx> {
@@ -803,7 +803,7 @@ impl InitializationRequiringAction {
}
}

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
/// Invokes `access_place` as appropriate for dropping the value
/// at `drop_place`. Note that the *actual* `Drop` in the MIR is
/// always for a variable (e.g., `Drop(x)`) -- but we recursively
@@ -1421,7 +1421,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}
}

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
fn check_if_reassignment_to_immutable_state(
&mut self,
context: Context,
@@ -1789,9 +1789,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}

/// Adds the place into the used mutable variables set
fn add_used_mut<'d>(
fn add_used_mut(
&mut self,
root_place: RootPlace<'d, 'tcx>,
root_place: RootPlace<'_, 'tcx>,
flow_state: &Flows<'cx, 'gcx, 'tcx>,
) {
match root_place {
@@ -1838,11 +1838,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

/// Whether this value be written or borrowed mutably.
/// Returns the root place if the place passed in is a projection.
fn is_mutable<'d>(
fn is_mutable(
&self,
place: &'d Place<'tcx>,
place: &'place Place<'tcx>,
is_local_mutation_allowed: LocalMutationIsAllowed,
) -> Result<RootPlace<'d, 'tcx>, &'d Place<'tcx>> {
) -> Result<RootPlace<'place, 'tcx>, &'place Place<'tcx>> {
match *place {
Place::Local(local) => {
let local = &self.mir.local_decls[local];
@@ -2017,13 +2017,13 @@ enum Overlap {
Disjoint,
}

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// FIXME (#16118): function intended to allow the borrow checker
// to be less precise in its handling of Box while still allowing
// moves out of a Box. They should be removed when/if we stop
// treating Box specially (e.g. when/if DerefMove is added...)

fn base_path<'d>(&self, place: &'d Place<'tcx>) -> &'d Place<'tcx> {
fn base_path(&self, place: &'place Place<'tcx>) -> &'place Place<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason for the rename here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the policy I tried to follow is to avoid single character lifetimes when they're used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, ok. This is exactly the case where I would prefer a single-character lifetime -- if the point is just to connect a variable and a return value, then a single-character lifetime expresses that (versus trying to tie to some "outer" lifetime).

I think the way I think of it is this:

  • longer lifetime names are for lifetimes that cross scopes.
  • single-character lifetimes are for things local to a particular fn
  • the names gcx and tcx are conceptually global; and within a given context, there may be other such names
    • this means in particular that standalone fn items might use longer names to refer to lifetimes from their callers

Not sure if this is the best policy or not but it's the one I was contemplating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how that makes sense; I can switch this case back.

//! Returns the base of the leftmost (deepest) dereference of an
//! Box in `place`. If there is no dereference of an Box
//! in `place`, then it just returns `place` itself.
@@ -58,7 +58,7 @@ enum GroupedMoveError<'tcx> {
},
}

impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'a, 'gcx, 'tcx> {
pub(crate) fn report_move_errors(&mut self, move_errors: Vec<MoveError<'tcx>>) {
let grouped_errors = self.group_move_errors(move_errors);
for error in grouped_errors {
@@ -293,7 +293,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
fn add_move_hints(
&self,
error: GroupedMoveError<'tcx>,
err: &mut DiagnosticBuilder<'a>,
err: &mut DiagnosticBuilder<'_>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an actual change since 'a was introduced in impl MirBorrowckCtxt<'a, 'gcx, 'tcx> { above, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, yes -- however, arguably, it's a "correct" change as this branch compiles. If we don't need to constrain the lifetime, it's easier to understand the code if we're not doing so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet, if it compiles, seems fine.

span: Span,
) {
match error {
@@ -22,7 +22,7 @@ use rustc::ty::fold::TypeFoldable;
use rustc::ty::subst::Substs;
use rustc::ty::{self, CanonicalTy, ClosureSubsts, GeneratorSubsts};

pub(super) fn generate_constraints<'cx, 'gcx, 'tcx>(
pub(super) fn generate_constraints(
infcx: &InferCtxt<'cx, 'gcx, 'tcx>,
regioncx: &mut RegionInferenceContext<'tcx>,
all_facts: &mut Option<AllFacts>,
@@ -52,7 +52,7 @@ struct ConstraintGeneration<'cg, 'cx: 'cg, 'gcx: 'tcx, 'tcx: 'cx> {
borrow_set: &'cg BorrowSet<'tcx>,
}

impl<'cg, 'cx, 'gcx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'gcx, 'tcx> {
impl Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'gcx, 'tcx> {
fn visit_basic_block_data(&mut self, bb: BasicBlock, data: &BasicBlockData<'tcx>) {
self.super_basic_block_data(bb, data);
}
@@ -184,7 +184,7 @@ impl<'cg, 'cx, 'gcx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'gcx
}
}

impl<'cx, 'cg, 'gcx, 'tcx> ConstraintGeneration<'cx, 'cg, 'gcx, 'tcx> {
impl ConstraintGeneration<'cx, 'cg, 'gcx, 'tcx> {
/// Some variable with type `live_ty` is "regular live" at
/// `location` -- i.e., it may be used later. This means that all
/// regions appearing in the type `live_ty` must be live at
@@ -51,12 +51,12 @@ impl ConstraintGraph {
}
}

crate struct Edges<'s> {
graph: &'s ConstraintGraph,
crate struct Edges<'graph> {
graph: &'graph ConstraintGraph,
pointer: Option<ConstraintIndex>,
}

impl<'s> Iterator for Edges<'s> {
impl Iterator for Edges<'graph> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to use a named lifetime here? I don't see the lifetime used in the impl, so it seems like you should be able to use '_.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Part of me wants to use '_ but I somewhat think this is an unanswered question; see https://mark-simulacrum.github.io/2018/07/19/inband-lifetimes.html#unused-lifetimes-in-impl-blocks as well.

type Item = ConstraintIndex;

fn next(&mut self) -> Option<Self::Item> {
@@ -74,7 +74,7 @@ crate struct RegionGraph<'s> {
constraint_graph: &'s ConstraintGraph,
}

impl<'s> RegionGraph<'s> {
impl RegionGraph<'s> {
/// Create a "dependency graph" where each region constraint `R1:
/// R2` is treated as an edge `R1 -> R2`. We use this graph to
/// construct SCCs for region inference but also for error
@@ -101,34 +101,34 @@ crate struct Successors<'s> {
edges: Edges<'s>,
}

impl<'s> Iterator for Successors<'s> {
impl Iterator for Successors<'s> {
type Item = RegionVid;

fn next(&mut self) -> Option<Self::Item> {
self.edges.next().map(|c| self.set[c].sub)
}
}

impl<'s> graph::DirectedGraph for RegionGraph<'s> {
impl graph::DirectedGraph for RegionGraph<'_> {
type Node = RegionVid;
}

impl<'s> graph::WithNumNodes for RegionGraph<'s> {
impl graph::WithNumNodes for RegionGraph<'_> {
fn num_nodes(&self) -> usize {
self.constraint_graph.first_constraints.len()
}
}

impl<'s> graph::WithSuccessors for RegionGraph<'s> {
fn successors<'graph>(
impl graph::WithSuccessors for RegionGraph<'_> {
fn successors(
&'graph self,
node: Self::Node,
) -> <Self as graph::GraphSuccessors<'graph>>::Iter {
self.sub_regions(node)
}
}

impl<'s, 'graph> graph::GraphSuccessors<'graph> for RegionGraph<'s> {
impl graph::GraphSuccessors<'graph> for RegionGraph<'s> {
type Item = RegionVid;
type Iter = Successors<'graph>;
}
@@ -19,7 +19,7 @@ use rustc::ty::{RegionVid, TyCtxt};
use rustc_data_structures::fx::FxHashSet;
use util::liveness::{self, DefUse, LivenessMode};

crate fn find<'tcx>(
crate fn find(
mir: &Mir<'tcx>,
regioncx: &Rc<RegionInferenceContext<'tcx>>,
tcx: TyCtxt<'_, '_, 'tcx>,
@@ -50,7 +50,7 @@ struct UseFinder<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
liveness_mode: LivenessMode,
}

impl<'cx, 'gcx, 'tcx> UseFinder<'cx, 'gcx, 'tcx> {
impl UseFinder<'cx, 'gcx, 'tcx> {
fn find(&mut self) -> Option<Cause> {
let mut queue = VecDeque::new();
let mut visited = FxHashSet();
@@ -134,7 +134,7 @@ enum DefUseResult {
UseDrop { local: Local },
}

impl<'cx, 'gcx, 'tcx> Visitor<'tcx> for DefUseVisitor<'cx, 'gcx, 'tcx> {
impl Visitor<'tcx> for DefUseVisitor<'cx, 'gcx, 'tcx> {
fn visit_local(&mut self, &local: &Local, context: PlaceContext<'tcx>, _: Location) {
let local_ty = self.mir.local_decls[local].ty;

@@ -16,7 +16,7 @@ use rustc_errors::DiagnosticBuilder;

mod find_use;

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
impl MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
/// Adds annotations to `err` explaining *why* the borrow contains the
/// point from `context`. This is key for the "3-point errors"
/// [described in the NLL RFC][d].
@@ -117,7 +117,7 @@ struct FactWriter<'w> {
dir: &'w Path,
}

impl<'w> FactWriter<'w> {
impl FactWriter<'_> {
fn write_facts_to_path<T>(
&self,
rows: &Vec<T>,
@@ -31,7 +31,7 @@ use rustc::ty::{self, ParamEnv};
use rustc_data_structures::indexed_vec::Idx;
use rustc_data_structures::graph::dominators::Dominators;

pub(super) fn generate_invalidates<'cx, 'gcx, 'tcx>(
pub(super) fn generate_invalidates(
infcx: &InferCtxt<'cx, 'gcx, 'tcx>,
all_facts: &mut Option<AllFacts>,
location_table: &LocationTable,
@@ -74,7 +74,7 @@ struct InvalidationGenerator<'cg, 'cx: 'cg, 'tcx: 'cx, 'gcx: 'tcx> {

/// Visits the whole MIR and generates invalidates() facts
/// Most of the code implementing this was stolen from borrow_check/mod.rs
impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
impl Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
fn visit_statement(&mut self,
block: BasicBlock,
statement: &Statement<'tcx>,
@@ -285,7 +285,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
}
}

impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
impl InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
/// Simulates dropping of a variable
fn visit_terminator_drop(
&mut self,