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

use Vec for region constraints instead of BTreeMap #118824

Merged
merged 2 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,30 +408,35 @@ fn try_extract_error_from_region_constraints<'tcx>(
mut region_var_origin: impl FnMut(RegionVid) -> RegionVariableOrigin,
mut universe_of_region: impl FnMut(RegionVid) -> UniverseIndex,
) -> Option<DiagnosticBuilder<'tcx, ErrorGuaranteed>> {
let placeholder_universe = match placeholder_region.kind() {
ty::RePlaceholder(p) => p.universe,
ty::ReVar(vid) => universe_of_region(vid),
_ => ty::UniverseIndex::ROOT,
};
let matches =
|a_region: Region<'tcx>, b_region: Region<'tcx>| match (a_region.kind(), b_region.kind()) {
(RePlaceholder(a_p), RePlaceholder(b_p)) => a_p.bound == b_p.bound,
_ => a_region == b_region,
};
let check = |constraint: &Constraint<'tcx>, cause: &SubregionOrigin<'tcx>, exact| {
match *constraint {
let mut check =
|constraint: &Constraint<'tcx>, cause: &SubregionOrigin<'tcx>, exact| match *constraint {
Constraint::RegSubReg(sub, sup)
if ((exact && sup == placeholder_region)
|| (!exact && matches(sup, placeholder_region)))
&& sup != sub =>
{
Some((sub, cause.clone()))
}
// FIXME: Should this check the universe of the var?
Constraint::VarSubReg(vid, sup)
if ((exact && sup == placeholder_region)
|| (!exact && matches(sup, placeholder_region))) =>
if (exact
&& sup == placeholder_region
&& !universe_of_region(vid).can_name(placeholder_universe))
|| (!exact && matches(sup, placeholder_region)) =>
{
Some((ty::Region::new_var(infcx.tcx, vid), cause.clone()))
}
_ => None,
}
};
};
let mut info = region_constraints
.constraints
.iter()
Expand Down
12 changes: 9 additions & 3 deletions compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
) -> LexicalRegionResolutions<'tcx> {
let mut var_data = self.construct_var_data();

// Deduplicating constraints is shown to have a positive perf impact.
self.data.constraints.sort_by_key(|(constraint, _)| *constraint);
self.data.constraints.dedup_by_key(|(constraint, _)| *constraint);
the8472 marked this conversation as resolved.
Show resolved Hide resolved

if cfg!(debug_assertions) {
self.dump_constraints();
}
Expand Down Expand Up @@ -183,7 +187,7 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
let mut constraints = IndexVec::from_elem(Vec::new(), &var_values.values);
// Tracks the changed region vids.
let mut changes = Vec::new();
for constraint in self.data.constraints.keys() {
for (constraint, _) in &self.data.constraints {
match *constraint {
Constraint::RegSubVar(a_region, b_vid) => {
let b_data = var_values.value_mut(b_vid);
Expand Down Expand Up @@ -678,7 +682,7 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
let dummy_source = graph.add_node(());
let dummy_sink = graph.add_node(());

for constraint in self.data.constraints.keys() {
for (constraint, _) in &self.data.constraints {
match *constraint {
Constraint::VarSubVar(a_id, b_id) => {
graph.add_edge(NodeIndex(a_id.index()), NodeIndex(b_id.index()), *constraint);
Expand Down Expand Up @@ -885,9 +889,11 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> {
}

Constraint::RegSubVar(region, _) | Constraint::VarSubReg(_, region) => {
let constraint_idx =
this.constraints.binary_search_by(|(c, _)| c.cmp(&edge.data)).unwrap();
state.result.push(RegionAndOrigin {
region,
origin: this.constraints.get(&edge.data).unwrap().clone(),
origin: this.constraints[constraint_idx].1.clone(),
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,8 @@ impl<'tcx> MiniGraph<'tcx> {
region_constraints.undo_log.region_constraints_in_snapshot(&snapshot.undo_snapshot)
{
match undo_entry {
AddConstraint(constraint) => {
each_constraint(constraint);
&AddConstraint(i) => {
each_constraint(&region_constraints.data().constraints[i].0);
}
&AddVerify(i) => span_bug!(
region_constraints.data().verifys[i].origin.span(),
Expand All @@ -430,8 +430,8 @@ impl<'tcx> MiniGraph<'tcx> {
region_constraints
.data()
.constraints
.keys()
.for_each(|constraint| each_constraint(constraint));
.iter()
.for_each(|(constraint, _)| each_constraint(constraint));
}
}

Expand Down
21 changes: 8 additions & 13 deletions compiler/rustc_infer/src/infer/region_constraints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use rustc_middle::ty::{ReBound, ReVar};
use rustc_middle::ty::{Region, RegionVid};
use rustc_span::Span;

use std::collections::BTreeMap;
use std::ops::Range;
use std::{cmp, fmt, mem};

Expand Down Expand Up @@ -90,7 +89,7 @@ pub type VarInfos = IndexVec<RegionVid, RegionVariableInfo>;
pub struct RegionConstraintData<'tcx> {
/// Constraints of the form `A <= B`, where either `A` or `B` can
/// be a region variable (or neither, as it happens).
pub constraints: BTreeMap<Constraint<'tcx>, SubregionOrigin<'tcx>>,
pub constraints: Vec<(Constraint<'tcx>, SubregionOrigin<'tcx>)>,

/// Constraints of the form `R0 member of [R1, ..., Rn]`, meaning that
/// `R0` must be equal to one of the regions `R1..Rn`. These occur
Expand Down Expand Up @@ -273,7 +272,7 @@ pub(crate) enum UndoLog<'tcx> {
AddVar(RegionVid),

/// We added the given `constraint`.
AddConstraint(Constraint<'tcx>),
AddConstraint(usize),
Copy link
Member

Choose a reason for hiding this comment

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

Could be an IndexVec :)

Copy link
Member Author

Choose a reason for hiding this comment

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

or simply remove the index from AddConstraint? 🤷‍♂️ It is useless and I only added it for consistency with AddVerify.

Copy link
Member Author

@aliemjay aliemjay Dec 15, 2023

Choose a reason for hiding this comment

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

It is useless and I only added it for consistency with AddVerify.

This is wrong. The index was actually needed in in leak_check code, but now I have removed the UndoLog altogether so it doesn't matter anymore (*edit: I reverted this commit).

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding a newtype index seems incompatible with sorting and deduplicating the IdexVec IMO.


/// We added the given `verify`.
AddVerify(usize),
Expand Down Expand Up @@ -319,8 +318,9 @@ impl<'tcx> RegionConstraintStorage<'tcx> {
self.var_infos.pop().unwrap();
assert_eq!(self.var_infos.len(), vid.index());
}
AddConstraint(ref constraint) => {
self.data.constraints.remove(constraint);
AddConstraint(index) => {
self.data.constraints.pop().unwrap();
assert_eq!(self.data.constraints.len(), index);
}
AddVerify(index) => {
self.data.verifys.pop();
Expand Down Expand Up @@ -443,14 +443,9 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
// cannot add constraints once regions are resolved
debug!("RegionConstraintCollector: add_constraint({:?})", constraint);

// never overwrite an existing (constraint, origin) - only insert one if it isn't
// present in the map yet. This prevents origins from outside the snapshot being
// replaced with "less informative" origins e.g., during calls to `can_eq`
let undo_log = &mut self.undo_log;
self.storage.data.constraints.entry(constraint).or_insert_with(|| {
undo_log.push(AddConstraint(constraint));
origin
});
let index = self.storage.data.constraints.len();
self.storage.data.constraints.push((constraint, origin));
self.undo_log.push(AddConstraint(index));
}

fn add_verify(&mut self, verify: Verify<'tcx>) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
let mut vid_map: FxHashMap<RegionTarget<'cx>, RegionDeps<'cx>> = FxHashMap::default();
let mut finished_map = FxHashMap::default();

for constraint in regions.constraints.keys() {
for (constraint, _) in &regions.constraints {
match constraint {
&Constraint::VarSubVar(r1, r2) => {
{
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ where
// into a map. Each RegionTarget (either a RegionVid or a Region) maps
// to its smaller and larger regions. Note that 'larger' regions correspond
// to sub-regions in Rust code (e.g., in 'a: 'b, 'a is the larger region).
for constraint in regions.constraints.keys() {
for (constraint, _) in &regions.constraints {
match *constraint {
Constraint::VarSubVar(r1, r2) => {
{
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/higher-ranked/higher-ranked-lifetime-error.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ error[E0308]: mismatched types
LL | assert_all::<_, &String>(id);
| ^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected reference `&String`
found reference `&String`
= note: expected trait `for<'a> <for<'a> fn(&'a String) -> &'a String {id} as FnMut<(&'a String,)>>`
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's the second commit.

found trait `for<'a> <for<'a> fn(&'a String) -> &'a String {id} as FnMut<(&'a String,)>>`

error: aborting due to 1 previous error

Expand Down
Loading