Skip to content

Commit 7c55e56

Browse files
committed
Removed unsound notion of unremovable_vars, prioritized removal of newer constraints over older
1 parent 021310a commit 7c55e56

File tree

6 files changed

+1620
-107
lines changed

6 files changed

+1620
-107
lines changed

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,11 @@ impl<'a, 'tcx> Deduper<'a, 'tcx> {
6262
let constraint_vars = std::mem::take(&mut self.constraint_vars);
6363
let constraint_cliques =
6464
std::mem::take(&mut self.constraint_cliques).into_iter().map(|x| x.1).collect();
65-
let unremovable_vars =
66-
self.var_indexer.unremovable_vars.iter().map(|x| VarIndex::from(*x)).collect();
67-
let removed = DedupSolver::dedup(constraint_vars, constraint_cliques, unremovable_vars)
65+
let var_universes = std::mem::take(&mut self.var_indexer.var_universes)
66+
.into_iter()
67+
.map(|(var, uni)| (VarIndex::from(var), uni.index()))
68+
.collect();
69+
let removed = DedupSolver::dedup(constraint_vars, constraint_cliques, var_universes)
6870
.removed_constraints;
6971

7072
let mut removed_outlives =

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver/constraint_walker.rs

+26-22
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_data_structures::fx::FxIndexSet;
1+
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
22
use rustc_infer::infer::InferCtxt;
33
use rustc_middle::ty;
44
use rustc_middle::ty::{
@@ -14,7 +14,7 @@ pub struct DedupWalker<'me, 'tcx> {
1414
}
1515
pub struct DedupableIndexer<'tcx> {
1616
vars: FxIndexSet<GenericArg<'tcx>>,
17-
pub unremovable_vars: FxIndexSet<usize>,
17+
pub var_universes: FxIndexMap<usize, ty::UniverseIndex>,
1818
}
1919

2020
impl<'me, 'tcx> DedupWalker<'me, 'tcx> {
@@ -32,13 +32,12 @@ impl<'me, 'tcx> DedupWalker<'me, 'tcx> {
3232
}
3333
impl<'tcx> DedupableIndexer<'tcx> {
3434
pub fn new() -> Self {
35-
Self { vars: FxIndexSet::default(), unremovable_vars: FxIndexSet::default() }
35+
Self { vars: FxIndexSet::default(), var_universes: FxIndexMap::default() }
3636
}
37-
fn lookup(&mut self, var: GenericArg<'tcx>) -> usize {
38-
self.vars.get_index_of(&var).unwrap_or_else(|| self.vars.insert_full(var).0)
39-
}
40-
fn add_unremovable_var(&mut self, var: usize) {
41-
self.unremovable_vars.insert(var);
37+
fn lookup(&mut self, var: GenericArg<'tcx>, universe: ty::UniverseIndex) -> usize {
38+
let var_indx = self.vars.get_index_of(&var).unwrap_or_else(|| self.vars.insert_full(var).0);
39+
self.var_universes.insert(var_indx, universe);
40+
var_indx
4241
}
4342
}
4443

@@ -59,11 +58,11 @@ impl<'tcx> TypeFolder<TyCtxt<'tcx>> for DedupWalker<'_, 'tcx> {
5958
ty::ReVar(..) | ty::RePlaceholder(..) => self.infcx.universe_of_region(region),
6059
_ => return region,
6160
};
62-
let var_id = self.var_indexer.lookup(GenericArg::from(region));
63-
self.vars_present.push(var_id);
6461
if self.max_nameable_universe.can_name(universe) {
65-
self.var_indexer.add_unremovable_var(var_id);
62+
return region;
6663
}
64+
let var_id = self.var_indexer.lookup(GenericArg::from(region), universe);
65+
self.vars_present.push(var_id);
6766
// dummy value
6867
self.interner().mk_re_placeholder(ty::Placeholder {
6968
universe: ty::UniverseIndex::from(self.max_nameable_universe.index() + 1),
@@ -77,20 +76,22 @@ impl<'tcx> TypeFolder<TyCtxt<'tcx>> for DedupWalker<'_, 'tcx> {
7776
fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> {
7877
let universe = match *ty.kind() {
7978
ty::Placeholder(p) => p.universe,
79+
/*
8080
ty::Infer(ty::InferTy::TyVar(vid)) => {
8181
if let Err(uni) = self.infcx.probe_ty_var(vid) {
8282
uni
8383
} else {
8484
return ty;
8585
}
8686
}
87+
*/
8788
_ => return ty,
8889
};
89-
let var_id = self.var_indexer.lookup(GenericArg::from(ty));
90-
self.vars_present.push(var_id);
9190
if self.max_nameable_universe.can_name(universe) {
92-
self.var_indexer.add_unremovable_var(var_id);
91+
return ty;
9392
}
93+
let var_id = self.var_indexer.lookup(GenericArg::from(ty), universe);
94+
self.vars_present.push(var_id);
9495
// dummy value
9596
self.interner().mk_ty_from_kind(ty::Placeholder(ty::Placeholder {
9697
universe: ty::UniverseIndex::from(self.max_nameable_universe.index() + 1),
@@ -101,23 +102,26 @@ impl<'tcx> TypeFolder<TyCtxt<'tcx>> for DedupWalker<'_, 'tcx> {
101102
fn fold_const(&mut self, ct: Const<'tcx>) -> Const<'tcx> {
102103
let new_ty = self.fold_ty(ct.ty());
103104
let universe = match ct.kind() {
105+
/*
104106
ty::ConstKind::Infer(ty::InferConst::Var(vid)) => {
105107
if let Err(uni) = self.infcx.probe_const_var(vid) { Some(uni) } else { None }
106108
}
109+
*/
107110
ty::ConstKind::Placeholder(p) => Some(p.universe),
108111
_ => None,
109112
};
110113
let new_const_kind = if let Some(uni) = universe {
111-
let var_id = self.var_indexer.lookup(GenericArg::from(ct));
112-
self.vars_present.push(var_id);
113114
if self.max_nameable_universe.can_name(uni) {
114-
self.var_indexer.add_unremovable_var(var_id);
115+
ct.kind()
116+
} else {
117+
let var_id = self.var_indexer.lookup(GenericArg::from(ct), uni);
118+
self.vars_present.push(var_id);
119+
// dummy value
120+
ty::ConstKind::Placeholder(ty::Placeholder {
121+
universe: ty::UniverseIndex::from(self.max_nameable_universe.index() + 1),
122+
bound: ty::BoundVar::from_usize(0),
123+
})
115124
}
116-
// dummy value
117-
ty::ConstKind::Placeholder(ty::Placeholder {
118-
universe: ty::UniverseIndex::from(self.max_nameable_universe.index() + 1),
119-
bound: ty::BoundVar::from_usize(0),
120-
})
121125
} else {
122126
ct.kind()
123127
};

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver/solver.rs

+28-36
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
1+
use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
22
use rustc_index::{Idx, IndexVec};
33
use std::cell::RefCell;
44
use std::collections::{BTreeMap, BTreeSet};
@@ -23,10 +23,9 @@ pub struct DedupSolver {
2323
/// The cliques that constraints are partitioned into. Constraints can only be merged if they belong to the same clique,
2424
/// and it's impossible for a constraint to be in more than one clique
2525
constraint_cliques: IndexVec<CliqueIndex, Vec<ConstraintIndex>>,
26-
/// A set of variables we cannot remove, i.e. they belong to a universe that the caller can name. We keep track of these
27-
/// to determine if there's a variable that we **can** remove that behaves like one of these, where in that case we just
28-
/// remove the unremovable var and keep the removable ones
29-
unremovable_vars: FxIndexSet<VarIndex>,
26+
/// The universes each var resides in. This is used because deduping prioritizes the removal of constraints
27+
/// that involve the highest universe indices
28+
var_universes: FxHashMap<VarIndex, usize>,
3029

3130
/// The below are internal variables used in the solving process:
3231
@@ -58,23 +57,17 @@ struct MappingInfo {
5857
/// a preexisting constraint. Therefore, the two constraints depend on each other
5958
dependencies: FxIndexMap<ConstraintIndex, BTreeSet<MappingIndex>>,
6059
}
61-
#[derive(Debug, PartialEq, Eq)]
62-
enum MapEval {
63-
Ok(MappingInfo),
64-
Conflicts,
65-
Unremovable,
66-
}
6760

6861
impl DedupSolver {
6962
pub fn dedup(
7063
constraint_vars: IndexVec<ConstraintIndex, Vec<VarIndex>>,
7164
constraint_cliques: IndexVec<CliqueIndex, Vec<ConstraintIndex>>,
72-
unremovable_vars: FxIndexSet<VarIndex>,
65+
var_universes: FxHashMap<VarIndex, usize>,
7366
) -> DedupResult {
7467
let mut deduper = Self {
7568
constraint_vars,
7669
constraint_cliques,
77-
unremovable_vars,
70+
var_universes,
7871

7972
mappings: FxIndexMap::default(),
8073
removed_constraints: RefCell::new(FxIndexSet::default()),
@@ -122,18 +115,20 @@ impl DedupSolver {
122115
// map the constraint [1] into [11], which is a constraint that doesn't exist
123116
let (eval_forward, eval_reverse) =
124117
(self.eval_mapping(&forward), self.eval_mapping(&reverse));
125-
if eval_forward == MapEval::Conflicts || eval_reverse == MapEval::Conflicts {
118+
let (Ok(eval_forward), Ok(eval_reverse)) = (eval_forward, eval_reverse) else {
126119
continue;
127-
}
128-
if let MapEval::Ok(eval_forward) = eval_forward {
129-
if self.try_apply_mapping(&forward, &eval_forward, false) == Err(true) {
130-
self.mappings.insert(forward, eval_forward);
131-
}
132-
}
133-
if let MapEval::Ok(eval_reverse) = eval_reverse {
134-
if self.try_apply_mapping(&reverse, &eval_reverse, false) == Err(true) {
135-
self.mappings.insert(reverse, eval_reverse);
136-
}
120+
};
121+
122+
let max_forward_universe = forward.max_removed_universe(&self.var_universes);
123+
let max_reverse_universe = reverse.max_removed_universe(&self.var_universes);
124+
let (chosen_mapping, chosen_eval) =
125+
if max_forward_universe >= max_reverse_universe {
126+
(forward, eval_forward)
127+
} else {
128+
(reverse, eval_reverse)
129+
};
130+
if self.try_apply_mapping(&chosen_mapping, &chosen_eval, false) == Err(true) {
131+
self.mappings.insert(chosen_mapping, chosen_eval);
137132
}
138133
}
139134
}
@@ -146,10 +141,7 @@ impl DedupSolver {
146141
/// MappingInfo can contain dependencies - these occur if a mapping *partially* maps
147142
/// a constraint onto another, so the mapping isn't immediately invalid, but we do need
148143
/// another mapping to complete that partial map for it to actually be valid
149-
fn eval_mapping(&self, mapping: &Mapping) -> MapEval {
150-
let maps_unremovable_var =
151-
mapping.0.iter().any(|(from, to)| self.unremovable_vars.contains(from) && from != to);
152-
144+
fn eval_mapping(&self, mapping: &Mapping) -> Result<MappingInfo, ()> {
153145
let mut info = MappingInfo::new();
154146
for clique in self.constraint_cliques.iter() {
155147
for constraint_1 in clique {
@@ -179,14 +171,11 @@ impl DedupSolver {
179171
}
180172
}
181173
if !found_non_conflicting {
182-
return MapEval::Conflicts;
174+
return Err(());
183175
}
184176
}
185177
}
186-
if maps_unremovable_var {
187-
return MapEval::Unremovable;
188-
}
189-
MapEval::Ok(info)
178+
Ok(info)
190179
}
191180
/// Currently, dependencies are in the form FxIndexMap<ConstraintIndex, Empty FxIndexSet>,
192181
/// where ConstraintIndex is the constraint we must *also* map in order to apply this mapping.
@@ -294,9 +283,6 @@ impl DedupSolver {
294283
// If we already applied a mapping, we now remove it from `from`, as its dependencies have
295284
// been resolved and therefore we don't need to worry about it
296285
from.retain(|x| !used_mappings.contains(x));
297-
if from.is_empty() {
298-
return Some(used_mappings);
299-
}
300286
used_mappings.extend(from.iter());
301287

302288
// For each unresolved dependency, we have a list of Mappings that can resolve it
@@ -314,6 +300,9 @@ impl DedupSolver {
314300
});
315301
unresolved_dependencies.extend(resolve_options);
316302
}
303+
if unresolved_dependencies.is_empty() {
304+
return Some(used_mappings);
305+
}
317306
if unresolved_dependencies.iter().any(|x| x.is_empty()) {
318307
return None;
319308
}
@@ -430,6 +419,9 @@ impl Mapping {
430419
}
431420
false
432421
}
422+
fn max_removed_universe(&self, var_universes: &FxHashMap<VarIndex, usize>) -> usize {
423+
self.0.keys().map(|x| *var_universes.get(x).unwrap()).max().unwrap_or(0)
424+
}
433425
}
434426
impl MappingInfo {
435427
fn new() -> Self {

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical/dedup_solver/solver/tests.rs

+26-45
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,9 @@ fn test_gh_issues_example() {
2929
let deduped = DedupSolver::dedup(
3030
constraint_vars(vec![vec![1], vec![2], vec![3]]),
3131
constraint_cliques(vec![vec![0, 1, 2]]),
32-
FxIndexSet::default(),
33-
);
34-
assert!(
35-
[constraint_set([0, 1]), constraint_set([0, 2]), constraint_set([1, 2])]
36-
.contains(&deduped.removed_constraints)
32+
FxHashMap::from_iter([(VarIndex::new(1), 1), (VarIndex::new(2), 2), (VarIndex::new(3), 3)]),
3733
);
34+
assert_eq!(constraint_set([1, 2]), deduped.removed_constraints);
3835
}
3936
#[test]
4037
fn test_noop() {
@@ -47,7 +44,7 @@ fn test_noop() {
4744
let deduped = DedupSolver::dedup(
4845
constraint_vars(vec![vec![1], vec![1, 1], vec![2], vec![3]]),
4946
constraint_cliques(vec![vec![0], vec![1], vec![2], vec![3]]),
50-
FxIndexSet::default(),
47+
FxHashMap::from_iter([(VarIndex::new(1), 1), (VarIndex::new(2), 2), (VarIndex::new(3), 3)]),
5148
);
5249
assert!(deduped.removed_constraints.is_empty());
5350
}
@@ -61,18 +58,9 @@ fn test_simple() {
6158
let deduped = DedupSolver::dedup(
6259
constraint_vars(vec![vec![1], vec![2], vec![3]]),
6360
constraint_cliques(vec![vec![0, 1], vec![2]]),
64-
FxIndexSet::default(),
65-
);
66-
assert!([constraint_set([0]), constraint_set([1])].contains(&deduped.removed_constraints));
67-
}
68-
#[test]
69-
fn test_simple_2() {
70-
let deduped = DedupSolver::dedup(
71-
constraint_vars(vec![vec![0, 1], vec![2, 1], vec![1, 0], vec![1, 2]]),
72-
constraint_cliques(vec![vec![0, 1, 2, 3]]),
73-
FxIndexSet::from_iter([VarIndex::new(0), VarIndex::new(1)]),
61+
FxHashMap::from_iter([(VarIndex::new(1), 1), (VarIndex::new(2), 2), (VarIndex::new(3), 3)]),
7462
);
75-
assert_eq!(deduped.removed_constraints, constraint_set([1, 3]));
63+
assert_eq!(constraint_set([1]), deduped.removed_constraints);
7664
}
7765
#[test]
7866
fn test_dependencies() {
@@ -93,34 +81,18 @@ fn test_dependencies() {
9381
vec![4, 5],
9482
]),
9583
constraint_cliques(vec![vec![0, 1], vec![2, 3], vec![4, 5]]),
96-
FxIndexSet::default(),
97-
);
98-
assert!(
99-
[constraint_set([0, 2, 4]), constraint_set([1, 3, 5])]
100-
.contains(&deduped.removed_constraints)
84+
FxHashMap::from_iter([
85+
(VarIndex::new(1), 1),
86+
(VarIndex::new(2), 2),
87+
(VarIndex::new(4), 3),
88+
(VarIndex::new(5), 4),
89+
(VarIndex::new(13), 5),
90+
(VarIndex::new(16), 6),
91+
(VarIndex::new(23), 7),
92+
(VarIndex::new(26), 8),
93+
]),
10194
);
102-
}
103-
#[test]
104-
fn test_unremovable_var() {
105-
fn try_dedup(unremovable_vars: FxIndexSet<VarIndex>) -> FxIndexSet<ConstraintIndex> {
106-
// Same constraints as `test_dependencies`, but just imagine that all the vars in
107-
// unremovable_vars are vars the caller can name, and therefore can't be removed
108-
DedupSolver::dedup(
109-
constraint_vars(vec![
110-
vec![1, 2, 13],
111-
vec![4, 5, 16],
112-
vec![1, 2, 23],
113-
vec![4, 5, 26],
114-
vec![1, 2],
115-
vec![4, 5],
116-
]),
117-
constraint_cliques(vec![vec![0, 1], vec![2, 3], vec![4, 5]]),
118-
unremovable_vars,
119-
)
120-
.removed_constraints
121-
}
122-
assert_eq!(try_dedup(FxIndexSet::from_iter([VarIndex::new(13)])), constraint_set([1, 3, 5]));
123-
assert_eq!(try_dedup(FxIndexSet::from_iter([VarIndex::new(16)])), constraint_set([0, 2, 4]));
95+
assert_eq!(constraint_set([1, 3, 5]), deduped.removed_constraints);
12496
}
12597
#[test]
12698
fn test_dependencies_unresolvable() {
@@ -134,7 +106,16 @@ fn test_dependencies_unresolvable() {
134106
vec![4, 5],
135107
]),
136108
constraint_cliques(vec![vec![0, 1], vec![2, 3], vec![4, 5]]),
137-
FxIndexSet::default(),
109+
FxHashMap::from_iter([
110+
(VarIndex::new(1), 1),
111+
(VarIndex::new(2), 2),
112+
(VarIndex::new(4), 3),
113+
(VarIndex::new(5), 4),
114+
(VarIndex::new(13), 5),
115+
(VarIndex::new(16), 6),
116+
(VarIndex::new(23), 7),
117+
(VarIndex::new(26), 8),
118+
]),
138119
);
139120
assert!(deduped.removed_constraints.is_empty());
140121
}

0 commit comments

Comments
 (0)