From 44d07df1cc5913b108d9207410ede33c38905bec Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 19 Feb 2018 20:27:28 -0500 Subject: [PATCH] Sort synthetic impls bounds before rendering This removes the implicit dependency on the iteration order of FxHashMap --- src/librustdoc/clean/auto_trait.rs | 45 ++++++++++++++++++++ src/test/rustdoc/synthetic_auto/complex.rs | 4 +- src/test/rustdoc/synthetic_auto/lifetimes.rs | 2 +- src/test/rustdoc/synthetic_auto/project.rs | 4 +- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/clean/auto_trait.rs b/src/librustdoc/clean/auto_trait.rs index 5951af6d70e..f1bba0e8361 100644 --- a/src/librustdoc/clean/auto_trait.rs +++ b/src/librustdoc/clean/auto_trait.rs @@ -1354,12 +1354,57 @@ fn param_env_to_generics<'b, 'c, 'cx>( } } + self.sort_where_predicates(&mut existing_predicates); + Generics { params: generic_params, where_predicates: existing_predicates, } } + // Ensure that the predicates are in a consistent order. The precise + // ordering doesn't actually matter, but it's important that + // 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) { + // 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. + format!("{:?}", first).cmp(&format!("{:?}", second)) + }); + } + fn is_fn_ty(&self, tcx: &TyCtxt, ty: &Type) -> bool { match &ty { &&Type::ResolvedPath { ref did, .. } => { diff --git a/src/test/rustdoc/synthetic_auto/complex.rs b/src/test/rustdoc/synthetic_auto/complex.rs index 81259a50108..531798c30c6 100644 --- a/src/test/rustdoc/synthetic_auto/complex.rs +++ b/src/test/rustdoc/synthetic_auto/complex.rs @@ -31,8 +31,8 @@ pub struct Foo { // @has complex/struct.NotOuter.html // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]/*/code' "impl<'a, T, K: \ -// ?Sized> Send for NotOuter<'a, T, K> where 'a: 'static, K: for<'b> Fn((&'b bool, &'a u8)) \ -// -> &'b i8, >::MyItem: Copy, T: MyTrait<'a>" +// ?Sized> Send for NotOuter<'a, T, K> where K: for<'b> Fn((&'b bool, &'a u8)) \ +// -> &'b i8, T: MyTrait<'a>, >::MyItem: Copy, 'a: 'static" pub use foo::{Foo, Inner as NotInner, MyTrait as NotMyTrait, Outer as NotOuter}; diff --git a/src/test/rustdoc/synthetic_auto/lifetimes.rs b/src/test/rustdoc/synthetic_auto/lifetimes.rs index 2f92627f954..272925e5db5 100644 --- a/src/test/rustdoc/synthetic_auto/lifetimes.rs +++ b/src/test/rustdoc/synthetic_auto/lifetimes.rs @@ -19,7 +19,7 @@ unsafe impl<'a, T> Send for Inner<'a, T> // @has lifetimes/struct.Foo.html // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]/*/code' "impl<'c, K> Send \ -// for Foo<'c, K> where 'c: 'static, K: for<'b> Fn(&'b bool) -> &'c u8" +// for Foo<'c, K> where K: for<'b> Fn(&'b bool) -> &'c u8, 'c: 'static" // // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]/*/code' "impl<'c, K> Sync \ // for Foo<'c, K> where K: Sync" diff --git a/src/test/rustdoc/synthetic_auto/project.rs b/src/test/rustdoc/synthetic_auto/project.rs index e1b8621ff6d..977607fb148 100644 --- a/src/test/rustdoc/synthetic_auto/project.rs +++ b/src/test/rustdoc/synthetic_auto/project.rs @@ -34,10 +34,10 @@ unsafe impl<'a, T> Sync for Inner<'a, T> // @has project/struct.Foo.html // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]/*/code' "impl<'c, K> Send \ -// for Foo<'c, K> where 'c: 'static, K: MyTrait" +// for Foo<'c, K> where K: MyTrait, 'c: 'static" // // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]/*/code' "impl<'c, K> Sync \ -// for Foo<'c, K> where 'c: 'static, K: MyTrait, ::MyItem: OtherTrait" +// for Foo<'c, K> where K: MyTrait, ::MyItem: OtherTrait, 'c: 'static," pub struct Foo<'c, K: 'c> { inner_field: Inner<'c, K>, } -- 2.44.0