From 57e3df3f879090207f5a128a497b66916aeabb6b Mon Sep 17 00:00:00 2001 From: Phlosioneer Date: Mon, 19 Mar 2018 23:35:23 -0400 Subject: [PATCH 1/2] Fix ordering of auto-generated trait bounds in rustdoc output While the order of the where clauses was deterministic, the ordering of bounds and lifetimes was not. This made the order flip- flop randomly when new traits and impls were added to libstd. This PR makes the ordering of bounds and lifetimes deterministic, and re-enables the test that was causing the issue. Fixes #49123 --- src/librustdoc/clean/auto_trait.rs | 102 ++++++++++++------ .../rustdoc/synthetic_auto/no-redundancy.rs | 2 - 2 files changed, 67 insertions(+), 37 deletions(-) diff --git a/src/librustdoc/clean/auto_trait.rs b/src/librustdoc/clean/auto_trait.rs index 370fc9bbca243..918bc1df0b1d6 100644 --- a/src/librustdoc/clean/auto_trait.rs +++ b/src/librustdoc/clean/auto_trait.rs @@ -9,6 +9,7 @@ // except according to those terms. use rustc::ty::TypeFoldable; +use std::fmt::Debug; use super::*; @@ -1081,18 +1082,25 @@ impl<'a, 'tcx, 'rcx> AutoTraitFinder<'a, 'tcx, 'rcx> { return None; } + let mut bounds_vec = bounds.into_iter().collect(); + self.sort_where_bounds(&mut bounds_vec); + Some(WherePredicate::BoundPredicate { ty, - bounds: bounds.into_iter().collect(), + bounds: bounds_vec, }) }) .chain( lifetime_to_bounds .into_iter() .filter(|&(_, ref bounds)| !bounds.is_empty()) - .map(|(lifetime, bounds)| WherePredicate::RegionPredicate { - lifetime, - bounds: bounds.into_iter().collect(), + .map(|(lifetime, bounds)| { + let mut bounds_vec = bounds.into_iter().collect(); + self.sort_where_lifetimes(&mut bounds_vec); + WherePredicate::RegionPredicate { + lifetime, + bounds: bounds_vec, + } }), ) .collect() @@ -1372,40 +1380,64 @@ impl<'a, 'tcx, 'rcx> AutoTraitFinder<'a, 'tcx, 'rcx> { // a given set of predicates always appears in the same order - // both for visual consistency between 'rustdoc' runs, and to // make writing tests much easier - fn sort_where_predicates(&self, predicates: &mut Vec) { + #[inline] + fn sort_where_predicates(&self, mut predicates: &mut Vec) { // We should never have identical bounds - and if we do, // they're visually identical as well. Therefore, using // an unstable sort is fine. - predicates.sort_unstable_by(|first, second| { - // This might look horrendously hacky, but it's actually not that bad. - // - // For performance reasons, we use several different FxHashMaps - // in the process of computing the final set of where predicates. - // However, the iteration order of a HashMap is completely unspecified. - // In fact, the iteration of an FxHashMap can even vary between platforms, - // since FxHasher has different behavior for 32-bit and 64-bit platforms. - // - // Obviously, it's extremely undesireable for documentation rendering - // to be depndent on the platform it's run on. Apart from being confusing - // to end users, it makes writing tests much more difficult, as predicates - // can appear in any order in the final result. - // - // To solve this problem, we sort WherePredicates by their Debug - // string. The thing to keep in mind is that we don't really - // care what the final order is - we're synthesizing an impl - // ourselves, so any order can be considered equally valid. - // By sorting the predicates, however, we ensure that for - // a given codebase, all auto-trait impls always render - // in exactly the same way. - // - // Using the Debug impementation for sorting prevents - // us from needing to write quite a bit of almost - // entirely useless code (e.g. how should two - // Types be sorted relative to each other). - // This approach is probably somewhat slower, but - // the small number of items involved (impls - // rarely have more than a few bounds) means - // that it shouldn't matter in practice. + self.unstable_debug_sort(&mut predicates); + } + + // Ensure that the bounds are in a consistent order. The precise + // ordering doesn't actually matter, but it's important that + // a given set of bounds always appears in the same order - + // both for visual consistency between 'rustdoc' runs, and to + // make writing tests much easier + #[inline] + fn sort_where_bounds(&self, mut bounds: &mut Vec) { + // We should never have identical bounds - and if we do, + // they're visually identical as well. Therefore, using + // an unstable sort is fine. + self.unstable_debug_sort(&mut bounds); + } + + #[inline] + fn sort_where_lifetimes(&self, mut bounds: &mut Vec) { + // We should never have identical bounds - and if we do, + // they're visually identical as well. Therefore, using + // an unstable sort is fine. + self.unstable_debug_sort(&mut bounds); + } + + // This might look horrendously hacky, but it's actually not that bad. + // + // For performance reasons, we use several different FxHashMaps + // in the process of computing the final set of where predicates. + // However, the iteration order of a HashMap is completely unspecified. + // In fact, the iteration of an FxHashMap can even vary between platforms, + // since FxHasher has different behavior for 32-bit and 64-bit platforms. + // + // Obviously, it's extremely undesireable for documentation rendering + // to be depndent on the platform it's run on. Apart from being confusing + // to end users, it makes writing tests much more difficult, as predicates + // can appear in any order in the final result. + // + // To solve this problem, we sort WherePredicates and TyParamBounds + // by their Debug string. The thing to keep in mind is that we don't really + // care what the final order is - we're synthesizing an impl or bound + // ourselves, so any order can be considered equally valid. By sorting the + // predicates and bounds, however, we ensure that for a given codebase, all + // auto-trait impls always render in exactly the same way. + // + // Using the Debug impementation for sorting prevents us from needing to + // write quite a bit of almost entirely useless code (e.g. how should two + // Types be sorted relative to each other). It also allows us to solve the + // problem for both WherePredicates and TyParamBounds at the same time. This + // approach is probably somewhat slower, but the small number of items + // involved (impls rarely have more than a few bounds) means that it + // shouldn't matter in practice. + fn unstable_debug_sort(&self, vec: &mut Vec) { + vec.sort_unstable_by(|first, second| { format!("{:?}", first).cmp(&format!("{:?}", second)) }); } diff --git a/src/test/rustdoc/synthetic_auto/no-redundancy.rs b/src/test/rustdoc/synthetic_auto/no-redundancy.rs index daa91c3e12bb8..0b37f2ed31790 100644 --- a/src/test/rustdoc/synthetic_auto/no-redundancy.rs +++ b/src/test/rustdoc/synthetic_auto/no-redundancy.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-test - pub struct Inner { field: T, } From 7daf3f941af996d6b816aa778aab4b9348d26703 Mon Sep 17 00:00:00 2001 From: Phlosioneer Date: Tue, 20 Mar 2018 01:02:15 -0400 Subject: [PATCH 2/2] Fix tidy trailing whitespace --- src/librustdoc/clean/auto_trait.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/clean/auto_trait.rs b/src/librustdoc/clean/auto_trait.rs index 918bc1df0b1d6..9ff3d25a45ae4 100644 --- a/src/librustdoc/clean/auto_trait.rs +++ b/src/librustdoc/clean/auto_trait.rs @@ -1387,7 +1387,7 @@ impl<'a, 'tcx, 'rcx> AutoTraitFinder<'a, 'tcx, 'rcx> { // an unstable sort is fine. self.unstable_debug_sort(&mut predicates); } - + // Ensure that the bounds are in a consistent order. The precise // ordering doesn't actually matter, but it's important that // a given set of bounds always appears in the same order - @@ -1400,7 +1400,7 @@ impl<'a, 'tcx, 'rcx> AutoTraitFinder<'a, 'tcx, 'rcx> { // an unstable sort is fine. self.unstable_debug_sort(&mut bounds); } - + #[inline] fn sort_where_lifetimes(&self, mut bounds: &mut Vec) { // We should never have identical bounds - and if we do,