Skip to content

Commit 5dbaafd

Browse files
committed
Auto merge of #123340 - fmease:rustdoc-simplify-auto-trait-impl-synth, r=GuillaumeGomez
rustdoc: heavily simplify the synthesis of auto trait impls `gd --numstat HEAD~2 HEAD src/librustdoc/clean/auto_trait.rs` **+315 -705** 🟩🟥🟥🟥⬛ --- As outlined in issue #113015, there are currently 3[^1] large separate routines that “clean” `rustc_middle::ty` data types related to generics & predicates to rustdoc data types. Every single one has their own kinds of bugs. While I've patched a lot of bugs in each of the routines in the past, it's about time to unify them. This PR is only the first in a series. It completely **yanks** the custom “bounds cleaning” of mod `auto_trait` and reuses the routines found in mod `simplify`. As alluded to, `simplify` is also flawed but it's still more complete than `auto_trait`'s routines. [See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`](#123340 (comment)). This is preparatory work for rewriting “bounds cleaning” from scratch in follow-up PRs in order to finally [fix] #113015. Apart from that, I've eliminated all potential sources of *instability* in the rendered output. See also #119597. I'm pretty sure this fixes #119597. This PR does not attempt to fix [any other issues related to synthetic auto trait impls](https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AA-synthetic-impls%20label%3AA-auto-traits). However, it's definitely meant to be a *stepping stone* by making `auto_trait` more contributor-friendly. --- * Replace `FxHash{Map,Set}` with `FxIndex{Map,Set}` to guarantee a stable iteration order * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set. * Yes, we do make use of `swap_remove` but that shouldn't matter since all the callers are deterministic. It does make the output less “predictable” but it's still better than before. Ofc, I rely on `rustc_infer` being deterministic. I hope that holds. * Utilizing `clean::simplify` over the custom “bounds cleaning” routines wipes out the last reference to `collect_referenced_late_bound_regions` in rustdoc (`simplify` uses `bound_vars`) which was a source of instability / unpredictability (cc #116388) * Remove the types `RegionTarget` and `RegionDeps` from `librustdoc`. They were duplicates of the identical types found in `rustc`. Just import them from `rustc`. For some reason, they were duplicated when splitting `auto_trait` in two in #49711. * Get rid of the useless “type namespace” `AutoTraitFinder` in `librustdoc` * The struct only held a `DocContext`, it was over-engineered * Turn the associated functions into free ones * Eliminates rightward drift; increases legibility * `rustc` also contains a useless `AutoTraitFinder` struct but I plan on removing that in a follow-up PR * Rename a bunch of methods to be way more descriptive * Eliminate `use super::*;` * Lead to `clean/mod.rs` accumulating a lot of unnecessary imports * Made `auto_traits` less modular * Eliminate a custom `TypeFolder`: We can just use the rustc helper `fold_regions` which does that for us I plan on adding extensive documentation to `librustdoc`'s `auto_trait` in follow-up PRs. I don't want to do that in this PR because further refactoring & bug fix PRs may alter the overall structure of `librustdoc`'s & `rustc`'s `auto_trait` modules to a great degree. I'm slowly digging into the dark details of `rustc`'s `auto_trait` module again and once I have the full picture I will be able to provide proper docs. --- While this PR does indeed touch `rustc`'s `auto_trait` — mostly tiny refactorings — I argue this PR doesn't need any compiler reviewers next to rustdoc ones since that module falls under the purview of rustdoc — it used to be part of `librustdoc` after all (#49711). Sorry for not having split this into more commits. If you'd like me to I can try to split it into more atomic commits retroactively. However, I don't know if that would actually make reviewing easier. I think the best way to review this might just be to place the master version of `auto_trait` on the left of your screen and the patched one on the right, not joking. r? `@GuillaumeGomez` [^1]: Or even 4 depending on the way you're counting.
2 parents 0e682e9 + 069e7f2 commit 5dbaafd

File tree

11 files changed

+452
-849
lines changed

11 files changed

+452
-849
lines changed

compiler/rustc_trait_selection/src/traits/auto_trait.rs

+22-40
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@ use super::*;
66
use crate::errors::UnableToConstructConstantValue;
77
use crate::infer::region_constraints::{Constraint, RegionConstraintData};
88
use crate::traits::project::ProjectAndUnifyResult;
9+
10+
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet, IndexEntry};
11+
use rustc_data_structures::unord::UnordSet;
912
use rustc_infer::infer::DefineOpaqueTypes;
1013
use rustc_middle::mir::interpret::ErrorHandled;
1114
use rustc_middle::ty::{Region, RegionVid};
1215

13-
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
14-
15-
use std::collections::hash_map::Entry;
1616
use std::collections::VecDeque;
1717
use std::iter;
1818

@@ -25,8 +25,8 @@ pub enum RegionTarget<'tcx> {
2525

2626
#[derive(Default, Debug, Clone)]
2727
pub struct RegionDeps<'tcx> {
28-
larger: FxIndexSet<RegionTarget<'tcx>>,
29-
smaller: FxIndexSet<RegionTarget<'tcx>>,
28+
pub larger: FxIndexSet<RegionTarget<'tcx>>,
29+
pub smaller: FxIndexSet<RegionTarget<'tcx>>,
3030
}
3131

3232
pub enum AutoTraitResult<A> {
@@ -35,17 +35,10 @@ pub enum AutoTraitResult<A> {
3535
NegativeImpl,
3636
}
3737

38-
#[allow(dead_code)]
39-
impl<A> AutoTraitResult<A> {
40-
fn is_auto(&self) -> bool {
41-
matches!(self, AutoTraitResult::PositiveImpl(_) | AutoTraitResult::NegativeImpl)
42-
}
43-
}
44-
4538
pub struct AutoTraitInfo<'cx> {
4639
pub full_user_env: ty::ParamEnv<'cx>,
4740
pub region_data: RegionConstraintData<'cx>,
48-
pub vid_to_region: FxHashMap<ty::RegionVid, ty::Region<'cx>>,
41+
pub vid_to_region: FxIndexMap<ty::RegionVid, ty::Region<'cx>>,
4942
}
5043

5144
pub struct AutoTraitFinder<'tcx> {
@@ -88,19 +81,12 @@ impl<'tcx> AutoTraitFinder<'tcx> {
8881

8982
let infcx = tcx.infer_ctxt().build();
9083
let mut selcx = SelectionContext::new(&infcx);
91-
for polarity in [true, false] {
84+
for polarity in [ty::PredicatePolarity::Positive, ty::PredicatePolarity::Negative] {
9285
let result = selcx.select(&Obligation::new(
9386
tcx,
9487
ObligationCause::dummy(),
9588
orig_env,
96-
ty::TraitPredicate {
97-
trait_ref,
98-
polarity: if polarity {
99-
ty::PredicatePolarity::Positive
100-
} else {
101-
ty::PredicatePolarity::Negative
102-
},
103-
},
89+
ty::TraitPredicate { trait_ref, polarity },
10490
));
10591
if let Ok(Some(ImplSource::UserDefined(_))) = result {
10692
debug!(
@@ -114,7 +100,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
114100
}
115101

116102
let infcx = tcx.infer_ctxt().build();
117-
let mut fresh_preds = FxHashSet::default();
103+
let mut fresh_preds = FxIndexSet::default();
118104

119105
// Due to the way projections are handled by SelectionContext, we need to run
120106
// evaluate_predicates twice: once on the original param env, and once on the result of
@@ -239,7 +225,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
239225
ty: Ty<'tcx>,
240226
param_env: ty::ParamEnv<'tcx>,
241227
user_env: ty::ParamEnv<'tcx>,
242-
fresh_preds: &mut FxHashSet<ty::Predicate<'tcx>>,
228+
fresh_preds: &mut FxIndexSet<ty::Predicate<'tcx>>,
243229
) -> Option<(ty::ParamEnv<'tcx>, ty::ParamEnv<'tcx>)> {
244230
let tcx = infcx.tcx;
245231

@@ -252,7 +238,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
252238

253239
let mut select = SelectionContext::new(infcx);
254240

255-
let mut already_visited = FxHashSet::default();
241+
let mut already_visited = UnordSet::new();
256242
let mut predicates = VecDeque::new();
257243
predicates.push_back(ty::Binder::dummy(ty::TraitPredicate {
258244
trait_ref: ty::TraitRef::new(infcx.tcx, trait_did, [ty]),
@@ -473,9 +459,9 @@ impl<'tcx> AutoTraitFinder<'tcx> {
473459
fn map_vid_to_region<'cx>(
474460
&self,
475461
regions: &RegionConstraintData<'cx>,
476-
) -> FxHashMap<ty::RegionVid, ty::Region<'cx>> {
477-
let mut vid_map: FxHashMap<RegionTarget<'cx>, RegionDeps<'cx>> = FxHashMap::default();
478-
let mut finished_map = FxHashMap::default();
462+
) -> FxIndexMap<ty::RegionVid, ty::Region<'cx>> {
463+
let mut vid_map = FxIndexMap::<RegionTarget<'cx>, RegionDeps<'cx>>::default();
464+
let mut finished_map = FxIndexMap::default();
479465

480466
for (constraint, _) in &regions.constraints {
481467
match constraint {
@@ -513,25 +499,22 @@ impl<'tcx> AutoTraitFinder<'tcx> {
513499
}
514500

515501
while !vid_map.is_empty() {
516-
#[allow(rustc::potential_query_instability)]
517-
let target = *vid_map.keys().next().expect("Keys somehow empty");
518-
let deps = vid_map.remove(&target).expect("Entry somehow missing");
502+
let target = *vid_map.keys().next().unwrap();
503+
let deps = vid_map.swap_remove(&target).unwrap();
519504

520505
for smaller in deps.smaller.iter() {
521506
for larger in deps.larger.iter() {
522507
match (smaller, larger) {
523508
(&RegionTarget::Region(_), &RegionTarget::Region(_)) => {
524-
if let Entry::Occupied(v) = vid_map.entry(*smaller) {
509+
if let IndexEntry::Occupied(v) = vid_map.entry(*smaller) {
525510
let smaller_deps = v.into_mut();
526511
smaller_deps.larger.insert(*larger);
527-
// FIXME(#120456) - is `swap_remove` correct?
528512
smaller_deps.larger.swap_remove(&target);
529513
}
530514

531-
if let Entry::Occupied(v) = vid_map.entry(*larger) {
515+
if let IndexEntry::Occupied(v) = vid_map.entry(*larger) {
532516
let larger_deps = v.into_mut();
533517
larger_deps.smaller.insert(*smaller);
534-
// FIXME(#120456) - is `swap_remove` correct?
535518
larger_deps.smaller.swap_remove(&target);
536519
}
537520
}
@@ -542,24 +525,23 @@ impl<'tcx> AutoTraitFinder<'tcx> {
542525
// Do nothing; we don't care about regions that are smaller than vids.
543526
}
544527
(&RegionTarget::RegionVid(_), &RegionTarget::RegionVid(_)) => {
545-
if let Entry::Occupied(v) = vid_map.entry(*smaller) {
528+
if let IndexEntry::Occupied(v) = vid_map.entry(*smaller) {
546529
let smaller_deps = v.into_mut();
547530
smaller_deps.larger.insert(*larger);
548-
// FIXME(#120456) - is `swap_remove` correct?
549531
smaller_deps.larger.swap_remove(&target);
550532
}
551533

552-
if let Entry::Occupied(v) = vid_map.entry(*larger) {
534+
if let IndexEntry::Occupied(v) = vid_map.entry(*larger) {
553535
let larger_deps = v.into_mut();
554536
larger_deps.smaller.insert(*smaller);
555-
// FIXME(#120456) - is `swap_remove` correct?
556537
larger_deps.smaller.swap_remove(&target);
557538
}
558539
}
559540
}
560541
}
561542
}
562543
}
544+
563545
finished_map
564546
}
565547

@@ -588,7 +570,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
588570
ty: Ty<'_>,
589571
nested: impl Iterator<Item = PredicateObligation<'tcx>>,
590572
computed_preds: &mut FxIndexSet<ty::Predicate<'tcx>>,
591-
fresh_preds: &mut FxHashSet<ty::Predicate<'tcx>>,
573+
fresh_preds: &mut FxIndexSet<ty::Predicate<'tcx>>,
592574
predicates: &mut VecDeque<ty::PolyTraitPredicate<'tcx>>,
593575
selcx: &mut SelectionContext<'_, 'tcx>,
594576
) -> bool {

0 commit comments

Comments
 (0)