Avoid sorting predicates by DefId

Fixes issue #82920

Even if an item does not change between compilation sessions, it may end
up with a different `DefId`, since inserting/deleting an item affects
the `DefId`s of all subsequent items. Therefore, we use a `DefPathHash`
in the incremental compilation system, which is stable in the face of
changes to unrelated items.

In particular, the query system will consider the inputs to a query to
be unchanged if any `DefId`s in the inputs have their `DefPathHash`es
unchanged. Queries are pure functions, so the query result should be
unchanged if the query inputs are unchanged.

Unfortunately, it's possible to inadvertantly make a query result
incorrectly change across compilations, by relying on the specific value
of a `DefId`. Specifically, if the query result is a slice that gets
sorted by `DefId`, the precise order will depend on how the `DefId`s got
assigned in a particular compilation session. If some definitions end up
with different `DefId`s (but the same `DefPathHash`es) in a subsequent
compilation session, we will end up re-computing a *different* value for
the query, even though the query system expects the result to unchanged
due to the unchanged inputs.

It turns out that we have been sorting the predicates computed during
`astconv` by their `DefId`. These predicates make their way into the
`super_predicates_that_define_assoc_type`, which ends up getting used to
compute the vtables of trait objects. This, re-ordering these predicates
between compilation sessions can lead to undefined behavior at runtime -
the query system will re-use code built with a *differently ordered*
vtable, resulting in the wrong method being invoked at runtime.

This PR avoids sorting by `DefId` in `astconv`, fixing the
miscompilation. However, it's possible that other instances of this
issue exist - they could also be easily introduced in the future.

To fully fix this issue, we should
1. Turn on `-Z incremental-verify-ich` by default. This will cause the
   compiler to ICE whenver an 'unchanged' query result changes between
   compilation sessions, instead of causing a miscompilation.
2. Remove the `Ord` impls for `CrateNum` and `DefId`. This will make it
   difficult to introduce ICEs in the first place.

Author: Aaron1011

compiler/rustc_typeck/src/astconv/mod.rs

@@ -942,7 +942,6 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         let mut bounds = Bounds::default();
 
         self.add_bounds(param_ty, ast_bounds, &mut bounds);
-        bounds.trait_bounds.sort_by_key(|(t, _, _)| t.def_id());
 
         bounds.implicitly_sized = if let SizedByDefault::Yes = sized_by_default {
             if !self.is_unsized(ast_bounds, span) { Some(span) } else { None }
@@ -1318,8 +1317,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
 
         // De-duplicate auto traits so that, e.g., `dyn Trait + Send + Send` is the same as
         // `dyn Trait + Send`.
-        auto_traits.sort_by_key(|i| i.trait_ref().def_id());
-        auto_traits.dedup_by_key(|i| i.trait_ref().def_id());
+        // We remove duplicates by inserting into a `FxHashSet` to avoid re-ordering
+        // the bounds
+        let mut duplicates = FxHashSet::default();
+        auto_traits.retain(|i| duplicates.insert(i.trait_ref().def_id()));
         debug!("regular_traits: {:?}", regular_traits);
         debug!("auto_traits: {:?}", auto_traits);
 

src/test/incremental/issue-82920-predicate-order-miscompile.rs

@@ -0,0 +1,31 @@
+// revisions: rpass1 rpass2
+
+trait MyTrait: One + Two {}
+impl<T> One for T {
+    fn method_one(&self) -> usize {
+        1
+    }
+}
+impl<T> Two for T {
+    fn method_two(&self) -> usize {
+        2
+    }
+}
+impl<T: One + Two> MyTrait for T {}
+
+fn main() {
+    let a: &dyn MyTrait = &true;
+    assert_eq!(a.method_one(), 1);
+    assert_eq!(a.method_two(), 2);
+}
+
+// Re-order traits 'One' and 'Two' between compilation
+// sessions
+
+#[cfg(rpass1)]
+trait One { fn method_one(&self) -> usize; }
+
+trait Two { fn method_two(&self) -> usize; }
+
+#[cfg(rpass2)]
+trait One { fn method_one(&self) -> usize; }

src/test/rustdoc/inline_cross/impl_trait.rs

@@ -18,7 +18,7 @@ pub use impl_trait_aux::func2;
 
 // @has impl_trait/fn.func3.html
 // @has - '//pre[@class="rust fn"]' "func3("
-// @has - '//pre[@class="rust fn"]' "_x: impl Clone + Iterator<Item = impl Iterator<Item = u8>>)"
+// @has - '//pre[@class="rust fn"]' "_x: impl Iterator<Item = impl Iterator<Item = u8>> + Clone)"
 // @!has - '//pre[@class="rust fn"]' 'where'
 pub use impl_trait_aux::func3;
 

src/test/rustdoc/unit-return.rs

@@ -10,8 +10,8 @@ pub fn f0<F: FnMut(u8) + Clone>(f: F) {}
 // @has 'foo/fn.f1.html' '//*[@class="rust fn"]' 'F: FnMut(u16) + Clone'
 pub fn f1<F: FnMut(u16) -> () + Clone>(f: F) {}
 
-// @has 'foo/fn.f2.html' '//*[@class="rust fn"]' 'F: Clone + FnMut(u32)'
+// @has 'foo/fn.f2.html' '//*[@class="rust fn"]' 'F: FnMut(u32) + Clone'
 pub use unit_return::f2;
 
-// @has 'foo/fn.f3.html' '//*[@class="rust fn"]' 'F: Clone + FnMut(u64)'
+// @has 'foo/fn.f3.html' '//*[@class="rust fn"]' 'F: FnMut(u64) + Clone'
 pub use unit_return::f3;

src/test/ui/associated-type-bounds/bad-bounds-on-assoc-in-trait.stderr

@@ -1,15 +1,3 @@
-error[E0277]: `<<Self as Case1>::C as Iterator>::Item` is not an iterator
-  --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:5
-   |
-LL |     type C: Clone + Iterator<Item: Send + Iterator<Item: for<'a> Lam<&'a u8, App: Debug>> + Sync>;
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `<<Self as Case1>::C as Iterator>::Item` is not an iterator
-   |
-   = help: the trait `Iterator` is not implemented for `<<Self as Case1>::C as Iterator>::Item`
-help: consider further restricting the associated type
-   |
-LL | trait Case1 where <<Self as Case1>::C as Iterator>::Item: Iterator {
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
 error[E0277]: `<<Self as Case1>::C as Iterator>::Item` cannot be sent between threads safely
   --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:36
    |
@@ -27,6 +15,23 @@ help: consider further restricting the associated type
 LL | trait Case1 where <<Self as Case1>::C as Iterator>::Item: Send {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error[E0277]: `<<Self as Case1>::C as Iterator>::Item` is not an iterator
+  --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:43
+   |
+LL |     type C: Clone + Iterator<Item: Send + Iterator<Item: for<'a> Lam<&'a u8, App: Debug>> + Sync>;
+   |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `<<Self as Case1>::C as Iterator>::Item` is not an iterator
+   | 
+  ::: $SRC_DIR/core/src/iter/traits/iterator.rs:LL:COL
+   |
+LL | pub trait Iterator {
+   | ------------------ required by this bound in `Iterator`
+   |
+   = help: the trait `Iterator` is not implemented for `<<Self as Case1>::C as Iterator>::Item`
+help: consider further restricting the associated type
+   |
+LL | trait Case1 where <<Self as Case1>::C as Iterator>::Item: Iterator {
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error[E0277]: `<<Self as Case1>::C as Iterator>::Item` cannot be shared between threads safely
   --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:93
    |

src/test/ui/associated-types/associated-type-projection-from-multiple-supertraits.stderr

@@ -20,12 +20,12 @@ LL | fn dent<C:BoxCar>(c: C, color: C::Color) {
    |
 help: use fully qualified syntax to disambiguate
    |
-LL | fn dent<C:BoxCar>(c: C, color: <C as Box>::Color) {
-   |                                ^^^^^^^^^^^^^^^^^
-help: use fully qualified syntax to disambiguate
-   |
 LL | fn dent<C:BoxCar>(c: C, color: <C as Vehicle>::Color) {
    |                                ^^^^^^^^^^^^^^^^^^^^^
+help: use fully qualified syntax to disambiguate
+   |
+LL | fn dent<C:BoxCar>(c: C, color: <C as Box>::Color) {
+   |                                ^^^^^^^^^^^^^^^^^
 
 error[E0222]: ambiguous associated type `Color` in bounds of `BoxCar`
   --> $DIR/associated-type-projection-from-multiple-supertraits.rs:23:37
@@ -42,8 +42,8 @@ LL | fn dent_object<COLOR>(c: dyn BoxCar<Color=COLOR>) {
    = help: consider introducing a new type parameter `T` and adding `where` constraints:
                where
                    T: BoxCar,
-                   T: Box::Color = COLOR,
-                   T: Vehicle::Color = COLOR
+                   T: Vehicle::Color = COLOR,
+                   T: Box::Color = COLOR
 
 error[E0191]: the value of the associated types `Color` (from trait `Box`), `Color` (from trait `Vehicle`) must be specified
   --> $DIR/associated-type-projection-from-multiple-supertraits.rs:23:30
@@ -73,12 +73,12 @@ LL | fn paint<C:BoxCar>(c: C, d: C::Color) {
    |
 help: use fully qualified syntax to disambiguate
    |
-LL | fn paint<C:BoxCar>(c: C, d: <C as Box>::Color) {
-   |                             ^^^^^^^^^^^^^^^^^
-help: use fully qualified syntax to disambiguate
-   |
 LL | fn paint<C:BoxCar>(c: C, d: <C as Vehicle>::Color) {
    |                             ^^^^^^^^^^^^^^^^^^^^^
+help: use fully qualified syntax to disambiguate
+   |
+LL | fn paint<C:BoxCar>(c: C, d: <C as Box>::Color) {
+   |                             ^^^^^^^^^^^^^^^^^
 
 error[E0191]: the value of the associated types `Color` (from trait `Box`), `Color` (from trait `Vehicle`) must be specified
   --> $DIR/associated-type-projection-from-multiple-supertraits.rs:32:32

src/test/ui/associated-types/defaults-unsound-62211-1.stderr

@@ -13,20 +13,6 @@ help: consider further restricting `Self`
 LL | trait UncheckedCopy: Sized + std::fmt::Display {
    |                            ^^^^^^^^^^^^^^^^^^^
 
-error[E0277]: the trait bound `Self: Deref` is not satisfied
-  --> $DIR/defaults-unsound-62211-1.rs:20:5
-   |
-LL |     type Output: Copy + Deref<Target = str> + AddAssign<&'static str> + From<Self> + Display = Self;
-   |     ^^^^^^^^^^^^^^^^^^^^-------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |     |                   |
-   |     |                   required by this bound in `UncheckedCopy::Output`
-   |     the trait `Deref` is not implemented for `Self`
-   |
-help: consider further restricting `Self`
-   |
-LL | trait UncheckedCopy: Sized + Deref {
-   |                            ^^^^^^^
-
 error[E0277]: cannot add-assign `&'static str` to `Self`
   --> $DIR/defaults-unsound-62211-1.rs:20:5
    |
@@ -41,6 +27,20 @@ help: consider further restricting `Self`
 LL | trait UncheckedCopy: Sized + AddAssign<&'static str> {
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error[E0277]: the trait bound `Self: Deref` is not satisfied
+  --> $DIR/defaults-unsound-62211-1.rs:20:5
+   |
+LL |     type Output: Copy + Deref<Target = str> + AddAssign<&'static str> + From<Self> + Display = Self;
+   |     ^^^^^^^^^^^^^^^^^^^^-------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |     |                   |
+   |     |                   required by this bound in `UncheckedCopy::Output`
+   |     the trait `Deref` is not implemented for `Self`
+   |
+help: consider further restricting `Self`
+   |
+LL | trait UncheckedCopy: Sized + Deref {
+   |                            ^^^^^^^
+
 error[E0277]: the trait bound `Self: Copy` is not satisfied
   --> $DIR/defaults-unsound-62211-1.rs:20:5
    |

src/test/ui/associated-types/defaults-unsound-62211-2.stderr

@@ -13,20 +13,6 @@ help: consider further restricting `Self`
 LL | trait UncheckedCopy: Sized + std::fmt::Display {
    |                            ^^^^^^^^^^^^^^^^^^^
 
-error[E0277]: the trait bound `Self: Deref` is not satisfied
-  --> $DIR/defaults-unsound-62211-2.rs:20:5
-   |
-LL |     type Output: Copy + Deref<Target = str> + AddAssign<&'static str> + From<Self> + Display = Self;
-   |     ^^^^^^^^^^^^^^^^^^^^-------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |     |                   |
-   |     |                   required by this bound in `UncheckedCopy::Output`
-   |     the trait `Deref` is not implemented for `Self`
-   |
-help: consider further restricting `Self`
-   |
-LL | trait UncheckedCopy: Sized + Deref {
-   |                            ^^^^^^^
-
 error[E0277]: cannot add-assign `&'static str` to `Self`
   --> $DIR/defaults-unsound-62211-2.rs:20:5
    |
@@ -41,6 +27,20 @@ help: consider further restricting `Self`
 LL | trait UncheckedCopy: Sized + AddAssign<&'static str> {
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error[E0277]: the trait bound `Self: Deref` is not satisfied
+  --> $DIR/defaults-unsound-62211-2.rs:20:5
+   |
+LL |     type Output: Copy + Deref<Target = str> + AddAssign<&'static str> + From<Self> + Display = Self;
+   |     ^^^^^^^^^^^^^^^^^^^^-------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |     |                   |
+   |     |                   required by this bound in `UncheckedCopy::Output`
+   |     the trait `Deref` is not implemented for `Self`
+   |
+help: consider further restricting `Self`
+   |
+LL | trait UncheckedCopy: Sized + Deref {
+   |                            ^^^^^^^
+
 error[E0277]: the trait bound `Self: Copy` is not satisfied
   --> $DIR/defaults-unsound-62211-2.rs:20:5
    |

src/test/ui/issues/issue-40827.stderr

@@ -1,27 +1,27 @@
-error[E0277]: `Rc<Foo>` cannot be sent between threads safely
+error[E0277]: `Rc<Foo>` cannot be shared between threads safely
   --> $DIR/issue-40827.rs:14:5
    |
 LL | fn f<T: Send>(_: T) {}
    |         ---- required by this bound in `f`
 ...
 LL |     f(Foo(Arc::new(Bar::B(None))));
-   |     ^ `Rc<Foo>` cannot be sent between threads safely
+   |     ^ `Rc<Foo>` cannot be shared between threads safely
    |
-   = help: within `Bar`, the trait `Send` is not implemented for `Rc<Foo>`
+   = help: within `Bar`, the trait `Sync` is not implemented for `Rc<Foo>`
    = note: required because it appears within the type `Bar`
    = note: required because of the requirements on the impl of `Send` for `Arc<Bar>`
    = note: required because it appears within the type `Foo`
 
-error[E0277]: `Rc<Foo>` cannot be shared between threads safely
+error[E0277]: `Rc<Foo>` cannot be sent between threads safely
   --> $DIR/issue-40827.rs:14:5
    |
 LL | fn f<T: Send>(_: T) {}
    |         ---- required by this bound in `f`
 ...
 LL |     f(Foo(Arc::new(Bar::B(None))));
-   |     ^ `Rc<Foo>` cannot be shared between threads safely
+   |     ^ `Rc<Foo>` cannot be sent between threads safely
    |
-   = help: within `Bar`, the trait `Sync` is not implemented for `Rc<Foo>`
+   = help: within `Bar`, the trait `Send` is not implemented for `Rc<Foo>`
    = note: required because it appears within the type `Bar`
    = note: required because of the requirements on the impl of `Send` for `Arc<Bar>`
    = note: required because it appears within the type `Foo`

src/test/ui/suggestions/missing-trait-bounds-for-method-call.stderr

@@ -8,10 +8,10 @@ LL |         self.foo();
    |              ^^^ method cannot be called on `&Foo<T>` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
-           `T: Bar`
-           which is required by `Foo<T>: Bar`
            `T: Default`
            which is required by `Foo<T>: Bar`
+           `T: Bar`
+           which is required by `Foo<T>: Bar`
 help: consider restricting the type parameters to satisfy the trait bounds
    |
 LL | struct Foo<T> where T: Bar, T: Default {

src/test/ui/traits/inductive-overflow/simultaneous.stderr

@@ -1,4 +1,4 @@
-error[E0275]: overflow evaluating the requirement `{integer}: Tweedledum`
+error[E0275]: overflow evaluating the requirement `{integer}: Tweedledee`
   --> $DIR/simultaneous.rs:18:5
    |
 LL | fn is_ee<T: Combo>(t: T) {