From 06546d4b4071f48888a5bef476aa01a9d5e6c4e4 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 12 Mar 2021 17:53:02 -0500 Subject: [PATCH 1/2] 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. --- compiler/rustc_typeck/src/astconv/mod.rs | 7 +++-- .../issue-82920-predicate-order-miscompile.rs | 31 +++++++++++++++++++ src/test/rustdoc/inline_cross/impl_trait.rs | 2 +- src/test/rustdoc/unit-return.rs | 4 +-- .../bad-bounds-on-assoc-in-trait.stderr | 29 ++++++++++------- ...rojection-from-multiple-supertraits.stderr | 20 ++++++------ .../defaults-unsound-62211-1.stderr | 28 ++++++++--------- .../defaults-unsound-62211-2.stderr | 28 ++++++++--------- src/test/ui/issues/issue-40827.stderr | 12 +++---- ...issing-trait-bounds-for-method-call.stderr | 4 +-- .../inductive-overflow/simultaneous.stderr | 2 +- 11 files changed, 102 insertions(+), 65 deletions(-) create mode 100644 src/test/incremental/issue-82920-predicate-order-miscompile.rs diff --git a/compiler/rustc_typeck/src/astconv/mod.rs b/compiler/rustc_typeck/src/astconv/mod.rs index 947363fc3ed08..89501d9ce9725 100644 --- a/compiler/rustc_typeck/src/astconv/mod.rs +++ b/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); diff --git a/src/test/incremental/issue-82920-predicate-order-miscompile.rs b/src/test/incremental/issue-82920-predicate-order-miscompile.rs new file mode 100644 index 0000000000000..793af679c9fc5 --- /dev/null +++ b/src/test/incremental/issue-82920-predicate-order-miscompile.rs @@ -0,0 +1,31 @@ +// revisions: rpass1 rpass2 + +trait MyTrait: One + Two {} +impl One for T { + fn method_one(&self) -> usize { + 1 + } +} +impl Two for T { + fn method_two(&self) -> usize { + 2 + } +} +impl 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; } diff --git a/src/test/rustdoc/inline_cross/impl_trait.rs b/src/test/rustdoc/inline_cross/impl_trait.rs index 0ab2fa99f877f..44e2c4d3fb253 100644 --- a/src/test/rustdoc/inline_cross/impl_trait.rs +++ b/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>)" +// @has - '//pre[@class="rust fn"]' "_x: impl Iterator> + Clone)" // @!has - '//pre[@class="rust fn"]' 'where' pub use impl_trait_aux::func3; diff --git a/src/test/rustdoc/unit-return.rs b/src/test/rustdoc/unit-return.rs index b1f251dae6eed..ae3a6031519fb 100644 --- a/src/test/rustdoc/unit-return.rs +++ b/src/test/rustdoc/unit-return.rs @@ -10,8 +10,8 @@ pub fn f0(f: F) {} // @has 'foo/fn.f1.html' '//*[@class="rust fn"]' 'F: FnMut(u16) + Clone' pub fn f1 () + 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; diff --git a/src/test/ui/associated-type-bounds/bad-bounds-on-assoc-in-trait.stderr b/src/test/ui/associated-type-bounds/bad-bounds-on-assoc-in-trait.stderr index 1c493581bc953..a5ebb80c83620 100644 --- a/src/test/ui/associated-type-bounds/bad-bounds-on-assoc-in-trait.stderr +++ b/src/test/ui/associated-type-bounds/bad-bounds-on-assoc-in-trait.stderr @@ -1,15 +1,3 @@ -error[E0277]: `<::C as Iterator>::Item` is not an iterator - --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:5 - | -LL | type C: Clone + Iterator Lam<&'a u8, App: Debug>> + Sync>; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `<::C as Iterator>::Item` is not an iterator - | - = help: the trait `Iterator` is not implemented for `<::C as Iterator>::Item` -help: consider further restricting the associated type - | -LL | trait Case1 where <::C as Iterator>::Item: Iterator { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - error[E0277]: `<::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 <::C as Iterator>::Item: Send { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +error[E0277]: `<::C as Iterator>::Item` is not an iterator + --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:43 + | +LL | type C: Clone + Iterator Lam<&'a u8, App: Debug>> + Sync>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `<::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 `<::C as Iterator>::Item` +help: consider further restricting the associated type + | +LL | trait Case1 where <::C as Iterator>::Item: Iterator { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error[E0277]: `<::C as Iterator>::Item` cannot be shared between threads safely --> $DIR/bad-bounds-on-assoc-in-trait.rs:27:93 | diff --git a/src/test/ui/associated-types/associated-type-projection-from-multiple-supertraits.stderr b/src/test/ui/associated-types/associated-type-projection-from-multiple-supertraits.stderr index b6a88179c1f63..cd7c0dc4a44d0 100644 --- a/src/test/ui/associated-types/associated-type-projection-from-multiple-supertraits.stderr +++ b/src/test/ui/associated-types/associated-type-projection-from-multiple-supertraits.stderr @@ -20,12 +20,12 @@ LL | fn dent(c: C, color: C::Color) { | help: use fully qualified syntax to disambiguate | -LL | fn dent(c: C, color: ::Color) { - | ^^^^^^^^^^^^^^^^^ -help: use fully qualified syntax to disambiguate - | LL | fn dent(c: C, color: ::Color) { | ^^^^^^^^^^^^^^^^^^^^^ +help: use fully qualified syntax to disambiguate + | +LL | fn dent(c: C, color: ::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(c: dyn BoxCar) { = 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: C, d: C::Color) { | help: use fully qualified syntax to disambiguate | -LL | fn paint(c: C, d: ::Color) { - | ^^^^^^^^^^^^^^^^^ -help: use fully qualified syntax to disambiguate - | LL | fn paint(c: C, d: ::Color) { | ^^^^^^^^^^^^^^^^^^^^^ +help: use fully qualified syntax to disambiguate + | +LL | fn paint(c: C, d: ::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 diff --git a/src/test/ui/associated-types/defaults-unsound-62211-1.stderr b/src/test/ui/associated-types/defaults-unsound-62211-1.stderr index 8e446cf051f47..bcdb50aa312cb 100644 --- a/src/test/ui/associated-types/defaults-unsound-62211-1.stderr +++ b/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 + AddAssign<&'static str> + From + 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 + AddAssign<&'static str> + From + 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 | diff --git a/src/test/ui/associated-types/defaults-unsound-62211-2.stderr b/src/test/ui/associated-types/defaults-unsound-62211-2.stderr index 93f4f497b38a2..fa5cf9196edbd 100644 --- a/src/test/ui/associated-types/defaults-unsound-62211-2.stderr +++ b/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 + AddAssign<&'static str> + From + 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 + AddAssign<&'static str> + From + 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 | diff --git a/src/test/ui/issues/issue-40827.stderr b/src/test/ui/issues/issue-40827.stderr index 5ea795d139715..95cacbc32ab69 100644 --- a/src/test/ui/issues/issue-40827.stderr +++ b/src/test/ui/issues/issue-40827.stderr @@ -1,27 +1,27 @@ -error[E0277]: `Rc` cannot be sent between threads safely +error[E0277]: `Rc` cannot be shared between threads safely --> $DIR/issue-40827.rs:14:5 | LL | fn f(_: T) {} | ---- required by this bound in `f` ... LL | f(Foo(Arc::new(Bar::B(None)))); - | ^ `Rc` cannot be sent between threads safely + | ^ `Rc` cannot be shared between threads safely | - = help: within `Bar`, the trait `Send` is not implemented for `Rc` + = help: within `Bar`, the trait `Sync` is not implemented for `Rc` = note: required because it appears within the type `Bar` = note: required because of the requirements on the impl of `Send` for `Arc` = note: required because it appears within the type `Foo` -error[E0277]: `Rc` cannot be shared between threads safely +error[E0277]: `Rc` cannot be sent between threads safely --> $DIR/issue-40827.rs:14:5 | LL | fn f(_: T) {} | ---- required by this bound in `f` ... LL | f(Foo(Arc::new(Bar::B(None)))); - | ^ `Rc` cannot be shared between threads safely + | ^ `Rc` cannot be sent between threads safely | - = help: within `Bar`, the trait `Sync` is not implemented for `Rc` + = help: within `Bar`, the trait `Send` is not implemented for `Rc` = note: required because it appears within the type `Bar` = note: required because of the requirements on the impl of `Send` for `Arc` = note: required because it appears within the type `Foo` diff --git a/src/test/ui/suggestions/missing-trait-bounds-for-method-call.stderr b/src/test/ui/suggestions/missing-trait-bounds-for-method-call.stderr index c7376b0007f97..0a64a0d7d5ebc 100644 --- a/src/test/ui/suggestions/missing-trait-bounds-for-method-call.stderr +++ b/src/test/ui/suggestions/missing-trait-bounds-for-method-call.stderr @@ -8,10 +8,10 @@ LL | self.foo(); | ^^^ method cannot be called on `&Foo` due to unsatisfied trait bounds | = note: the following trait bounds were not satisfied: - `T: Bar` - which is required by `Foo: Bar` `T: Default` which is required by `Foo: Bar` + `T: Bar` + which is required by `Foo: Bar` help: consider restricting the type parameters to satisfy the trait bounds | LL | struct Foo where T: Bar, T: Default { diff --git a/src/test/ui/traits/inductive-overflow/simultaneous.stderr b/src/test/ui/traits/inductive-overflow/simultaneous.stderr index 484ac8511790f..88e0631eeb27a 100644 --- a/src/test/ui/traits/inductive-overflow/simultaneous.stderr +++ b/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: T) { From 18f89790dacad42d11f7fbd5d9cb09ce5dd8ff2f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 14 Mar 2021 14:12:04 -0400 Subject: [PATCH 2/2] Bump recursion_limit in a few places This is needed to get rustdoc to succeed on `dist-x86_64-linux-alt` --- compiler/rustc_builtin_macros/src/lib.rs | 1 + compiler/rustc_parse/src/lib.rs | 1 + compiler/rustc_session/src/lib.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/compiler/rustc_builtin_macros/src/lib.rs b/compiler/rustc_builtin_macros/src/lib.rs index 1017b23e5675a..a46a5502b24ad 100644 --- a/compiler/rustc_builtin_macros/src/lib.rs +++ b/compiler/rustc_builtin_macros/src/lib.rs @@ -11,6 +11,7 @@ #![feature(or_patterns)] #![feature(proc_macro_internals)] #![feature(proc_macro_quote)] +#![recursion_limit = "256"] extern crate proc_macro; diff --git a/compiler/rustc_parse/src/lib.rs b/compiler/rustc_parse/src/lib.rs index cea4de72df549..001b52b56fc65 100644 --- a/compiler/rustc_parse/src/lib.rs +++ b/compiler/rustc_parse/src/lib.rs @@ -6,6 +6,7 @@ #![feature(or_patterns)] #![feature(box_syntax)] #![feature(box_patterns)] +#![recursion_limit = "256"] use rustc_ast as ast; use rustc_ast::token::{self, Nonterminal}; diff --git a/compiler/rustc_session/src/lib.rs b/compiler/rustc_session/src/lib.rs index d002f59739166..7eaeae504ebf8 100644 --- a/compiler/rustc_session/src/lib.rs +++ b/compiler/rustc_session/src/lib.rs @@ -1,6 +1,7 @@ #![feature(crate_visibility_modifier)] #![feature(once_cell)] #![feature(or_patterns)] +#![recursion_limit = "256"] #[macro_use] extern crate bitflags;