Skip to content

Commit

Permalink
Auto merge of #123340 - fmease:rustdoc-simplify-auto-trait-impl-synth…
Browse files Browse the repository at this point in the history
…, r=<try>

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 mentioned, `simplify` is also flawed but still it's more complete than `auto_trait`'s routines. See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`.

This is preparatory work for rewriting “bounds cleaning” in follow-up PRs in order to finally [fix] #113015.

Apart from that, I've eliminated all potential sources of *unstable rendering 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 / more predictable iteration order
  * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set.
* 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 and therefore
  * made `auto_traits` less modular

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 retroactively separate it into more atomic commits. However, I don't know if that would actually make reviewing this PR easier.

r? `@GuillaumeGomez`

[^1]: Or even 4 depending on the way you're counting.
  • Loading branch information
bors committed Apr 1, 2024
2 parents b38b6ca + 785787f commit ce8cfc5
Show file tree
Hide file tree
Showing 11 changed files with 445 additions and 847 deletions.
62 changes: 22 additions & 40 deletions compiler/rustc_trait_selection/src/traits/auto_trait.rs
Expand Up @@ -6,13 +6,13 @@ use super::*;
use crate::errors::UnableToConstructConstantValue;
use crate::infer::region_constraints::{Constraint, RegionConstraintData};
use crate::traits::project::ProjectAndUnifyResult;

use rustc_data_structures::fx::{FxIndexMap, FxIndexSet, IndexEntry};
use rustc_data_structures::unord::UnordSet;
use rustc_infer::infer::DefineOpaqueTypes;
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::ty::{Region, RegionVid};

use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};

use std::collections::hash_map::Entry;
use std::collections::VecDeque;
use std::iter;

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

#[derive(Default, Debug, Clone)]
pub struct RegionDeps<'tcx> {
larger: FxIndexSet<RegionTarget<'tcx>>,
smaller: FxIndexSet<RegionTarget<'tcx>>,
pub larger: FxIndexSet<RegionTarget<'tcx>>,
pub smaller: FxIndexSet<RegionTarget<'tcx>>,
}

pub enum AutoTraitResult<A> {
Expand All @@ -35,17 +35,10 @@ pub enum AutoTraitResult<A> {
NegativeImpl,
}

#[allow(dead_code)]
impl<A> AutoTraitResult<A> {
fn is_auto(&self) -> bool {
matches!(self, AutoTraitResult::PositiveImpl(_) | AutoTraitResult::NegativeImpl)
}
}

pub struct AutoTraitInfo<'cx> {
pub full_user_env: ty::ParamEnv<'cx>,
pub region_data: RegionConstraintData<'cx>,
pub vid_to_region: FxHashMap<ty::RegionVid, ty::Region<'cx>>,
pub vid_to_region: FxIndexMap<ty::RegionVid, ty::Region<'cx>>,
}

pub struct AutoTraitFinder<'tcx> {
Expand Down Expand Up @@ -88,19 +81,12 @@ impl<'tcx> AutoTraitFinder<'tcx> {

let infcx = tcx.infer_ctxt().build();
let mut selcx = SelectionContext::new(&infcx);
for polarity in [true, false] {
for polarity in [ty::PredicatePolarity::Positive, ty::PredicatePolarity::Negative] {
let result = selcx.select(&Obligation::new(
tcx,
ObligationCause::dummy(),
orig_env,
ty::TraitPredicate {
trait_ref,
polarity: if polarity {
ty::PredicatePolarity::Positive
} else {
ty::PredicatePolarity::Negative
},
},
ty::TraitPredicate { trait_ref, polarity },
));
if let Ok(Some(ImplSource::UserDefined(_))) = result {
debug!(
Expand All @@ -114,7 +100,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
}

let infcx = tcx.infer_ctxt().build();
let mut fresh_preds = FxHashSet::default();
let mut fresh_preds = FxIndexSet::default();

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

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

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

let mut already_visited = FxHashSet::default();
let mut already_visited = UnordSet::new();
let mut predicates = VecDeque::new();
predicates.push_back(ty::Binder::dummy(ty::TraitPredicate {
trait_ref: ty::TraitRef::new(infcx.tcx, trait_did, [ty]),
Expand Down Expand Up @@ -473,9 +459,9 @@ impl<'tcx> AutoTraitFinder<'tcx> {
fn map_vid_to_region<'cx>(
&self,
regions: &RegionConstraintData<'cx>,
) -> FxHashMap<ty::RegionVid, ty::Region<'cx>> {
let mut vid_map: FxHashMap<RegionTarget<'cx>, RegionDeps<'cx>> = FxHashMap::default();
let mut finished_map = FxHashMap::default();
) -> FxIndexMap<ty::RegionVid, ty::Region<'cx>> {
let mut vid_map = FxIndexMap::<RegionTarget<'cx>, RegionDeps<'cx>>::default();
let mut finished_map = FxIndexMap::default();

for (constraint, _) in &regions.constraints {
match constraint {
Expand Down Expand Up @@ -513,25 +499,22 @@ impl<'tcx> AutoTraitFinder<'tcx> {
}

while !vid_map.is_empty() {
#[allow(rustc::potential_query_instability)]
let target = *vid_map.keys().next().expect("Keys somehow empty");
let deps = vid_map.remove(&target).expect("Entry somehow missing");
let target = *vid_map.keys().next().unwrap();
let deps = vid_map.swap_remove(&target).unwrap();

for smaller in deps.smaller.iter() {
for larger in deps.larger.iter() {
match (smaller, larger) {
(&RegionTarget::Region(_), &RegionTarget::Region(_)) => {
if let Entry::Occupied(v) = vid_map.entry(*smaller) {
if let IndexEntry::Occupied(v) = vid_map.entry(*smaller) {
let smaller_deps = v.into_mut();
smaller_deps.larger.insert(*larger);
// FIXME(#120456) - is `swap_remove` correct?
smaller_deps.larger.swap_remove(&target);
}

if let Entry::Occupied(v) = vid_map.entry(*larger) {
if let IndexEntry::Occupied(v) = vid_map.entry(*larger) {
let larger_deps = v.into_mut();
larger_deps.smaller.insert(*smaller);
// FIXME(#120456) - is `swap_remove` correct?
larger_deps.smaller.swap_remove(&target);
}
}
Expand All @@ -542,24 +525,23 @@ impl<'tcx> AutoTraitFinder<'tcx> {
// Do nothing; we don't care about regions that are smaller than vids.
}
(&RegionTarget::RegionVid(_), &RegionTarget::RegionVid(_)) => {
if let Entry::Occupied(v) = vid_map.entry(*smaller) {
if let IndexEntry::Occupied(v) = vid_map.entry(*smaller) {
let smaller_deps = v.into_mut();
smaller_deps.larger.insert(*larger);
// FIXME(#120456) - is `swap_remove` correct?
smaller_deps.larger.swap_remove(&target);
}

if let Entry::Occupied(v) = vid_map.entry(*larger) {
if let IndexEntry::Occupied(v) = vid_map.entry(*larger) {
let larger_deps = v.into_mut();
larger_deps.smaller.insert(*smaller);
// FIXME(#120456) - is `swap_remove` correct?
larger_deps.smaller.swap_remove(&target);
}
}
}
}
}
}

finished_map
}

Expand Down Expand Up @@ -588,7 +570,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
ty: Ty<'_>,
nested: impl Iterator<Item = PredicateObligation<'tcx>>,
computed_preds: &mut FxIndexSet<ty::Predicate<'tcx>>,
fresh_preds: &mut FxHashSet<ty::Predicate<'tcx>>,
fresh_preds: &mut FxIndexSet<ty::Predicate<'tcx>>,
predicates: &mut VecDeque<ty::PolyTraitPredicate<'tcx>>,
selcx: &mut SelectionContext<'_, 'tcx>,
) -> bool {
Expand Down

0 comments on commit ce8cfc5

Please sign in to comment.