From 2c1cf5aa7233db9bf11220b23b18570ff423fe1a Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 3 Nov 2021 11:12:39 -0700 Subject: [PATCH 01/15] Document how recursion is handled for `ty::Ty` Based on this forum discussion: https://internals.rust-lang.org/t/recursive-type-representation-in-rustc/15235/4 --- compiler/rustc_middle/src/ty/adt.rs | 22 ++++++++++++++++++++++ compiler/rustc_middle/src/ty/mod.rs | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/ty/adt.rs b/compiler/rustc_middle/src/ty/adt.rs index 44f741c5df1a2..20d379706f652 100644 --- a/compiler/rustc_middle/src/ty/adt.rs +++ b/compiler/rustc_middle/src/ty/adt.rs @@ -64,6 +64,28 @@ bitflags! { /// Moreover, Rust only allows recursive data types through indirection. /// /// [adt]: https://en.wikipedia.org/wiki/Algebraic_data_type +/// +/// # Recursive types +/// +/// It may seem impossible to represent recursive types using [`Ty`], +/// since [`TyKind::Adt`] includes [`AdtDef`], which includes its fields, +/// creating a cycle. However, `AdtDef` does not actually include the *types* +/// of its fields; it includes just their [`DefId`]s. +/// +/// For example, the following type: +/// +/// ``` +/// struct S { x: Box } +/// ``` +/// +/// is essentially represented with [`Ty`] as the following pseudocode: +/// +/// ``` +/// struct S { x } +/// ``` +/// +/// where `x` here represents the `DefId` of `S.x`. Then, the `DefId` +/// can be used with [`TyCtxt::type_of()`] to get the type of the field. pub struct AdtDef { /// The `DefId` of the struct, enum or union item. pub did: DefId, diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index cf47da157d19f..d162578dd002f 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -1700,7 +1700,7 @@ impl ReprOptions { impl<'tcx> FieldDef { /// Returns the type of this field. The resulting type is not normalized. The `subst` is - /// typically obtained via the second field of `TyKind::AdtDef`. + /// typically obtained via the second field of [`TyKind::Adt`]. pub fn ty(&self, tcx: TyCtxt<'tcx>, subst: SubstsRef<'tcx>) -> Ty<'tcx> { tcx.type_of(self.did).subst(tcx, subst) } From 7907fa8ec4cd4e7b60687e3f06b12e2b8fff6a04 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 29 Nov 2021 22:10:49 -0800 Subject: [PATCH 02/15] Clarify and tidy up explanation of E0038 --- .../src/error_codes/E0038.md | 97 +++++++++++++------ 1 file changed, 65 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_error_codes/src/error_codes/E0038.md b/compiler/rustc_error_codes/src/error_codes/E0038.md index 019d54b6202ed..ca2eaa54057fa 100644 --- a/compiler/rustc_error_codes/src/error_codes/E0038.md +++ b/compiler/rustc_error_codes/src/error_codes/E0038.md @@ -1,34 +1,64 @@ -Trait objects like `Box` can only be constructed when certain -requirements are satisfied by the trait in question. - -Trait objects are a form of dynamic dispatch and use a dynamically sized type -for the inner type. So, for a given trait `Trait`, when `Trait` is treated as a -type, as in `Box`, the inner type is 'unsized'. In such cases the boxed -pointer is a 'fat pointer' that contains an extra pointer to a table of methods -(among other things) for dynamic dispatch. This design mandates some -restrictions on the types of traits that are allowed to be used in trait -objects, which are collectively termed as 'object safety' rules. - -Attempting to create a trait object for a non object-safe trait will trigger -this error. - -There are various rules: - -### The trait cannot require `Self: Sized` - -When `Trait` is treated as a type, the type does not implement the special -`Sized` trait, because the type does not have a known size at compile time and -can only be accessed behind a pointer. Thus, if we have a trait like the -following: +For any given trait `Trait` there may be a related _type_ called the _trait +object type_ which is typically written as `dyn Trait`. In earlier editions of +Rust, trait object types were written as plain `Trait` (just the name of the +trait, written in type positions) but this was a bit too confusing, so we now +write `dyn Trait`. + +Some traits are not allowed to be used as trait object types. The traits that +are allowed to be used as trait object types are called "object-safe" traits. +Attempting to use a trait object type for a trait that is not object-safe will +trigger error E0038. + +Two general aspects of trait object types give rise to the restrictions: + + 1. Trait object types are dynamically sized types (DSTs), and trait objects of + these types can only be accessed through pointers, such as `&dyn Trait` or + `Box`. The size of such a pointer is known, but the size of the + `dyn Trait` object pointed-to by the pointer is _opaque_ to code working + with it, and different tait objects with the same trait object type may + have different sizes. + + 2. The pointer used to access a trait object is paired with an extra pointer + to a "virtual method table" or "vtable", which is used to implement dynamic + dispatch to the object's implementations of the trait's methods. There is a + single such vtable for each trait implementation, but different trait + objects with the same trait object type may point to vtables from different + implementations. + +The specific conditions that violate object-safety follow, most of which relate +to missing size information and vtable polymorphism arising from these aspects. + +### The trait requires `Self: Sized` + +Traits that are declared as `Trait: Sized` or which otherwise inherit a +constraint of `Self:Sized` are not object-safe. + +The reasoning behind this is somewhat subtle. It derives from the fact that Rust +requires (and defines) that every trait object type `dyn Trait` automatically +implements `Trait`. Rust does this to simplify error reporting and ease +interoperation between static and dynamic polymorphism. For example, this code +works: ``` -trait Foo where Self: Sized { +trait Trait { +} + +fn static_foo(b: &T) { +} +fn dynamic_bar(a: &dyn Trait) { + static_foo(a) } ``` -We cannot create an object of type `Box` or `&Foo` since in this case -`Self` would not be `Sized`. +This code works because `dyn Trait`, if it exists, always implements `Trait`. + +However as we know, any `dyn Trait` is also unsized, and so it can never +implement a sized trait like `Trait:Sized`. So, rather than allow an exception +to the rule that `dyn Trait` always implements `Trait`, Rust chooses to prohibit +such a `dyn Trait` from existing at all. + +Only unsized traits are considered object-safe. Generally, `Self: Sized` is used to indicate that the trait should not be used as a trait object. If the trait comes from your own crate, consider removing @@ -67,7 +97,7 @@ trait Trait { fn foo(&self) -> Self; } -fn call_foo(x: Box) { +fn call_foo(x: Box) { let y = x.foo(); // What type is y? // ... } @@ -76,7 +106,8 @@ fn call_foo(x: Box) { If only some methods aren't object-safe, you can add a `where Self: Sized` bound on them to mark them as explicitly unavailable to trait objects. The functionality will still be available to all other implementers, including -`Box` which is itself sized (assuming you `impl Trait for Box`). +`Box` which is itself sized (assuming you `impl Trait for Box`). ``` trait Trait { @@ -115,7 +146,9 @@ impl Trait for u8 { ``` At compile time each implementation of `Trait` will produce a table containing -the various methods (and other items) related to the implementation. +the various methods (and other items) related to the implementation, which will +be used as the virtual method table for a `dyn Trait` object derived from that +implementation. This works fine, but when the method gains generic parameters, we can have a problem. @@ -174,7 +207,7 @@ Now, if we have the following code: # impl Trait for u8 { fn foo(&self, on: T) {} } # impl Trait for bool { fn foo(&self, on: T) {} } # // etc. -fn call_foo(thing: Box) { +fn call_foo(thing: Box) { thing.foo(true); // this could be any one of the 8 types above thing.foo(1); thing.foo("hello"); @@ -200,7 +233,7 @@ trait Trait { ``` If this is not an option, consider replacing the type parameter with another -trait object (e.g., if `T: OtherTrait`, use `on: Box`). If the +trait object (e.g., if `T: OtherTrait`, use `on: Box`). If the number of types you intend to feed to this method is limited, consider manually listing out the methods of different types. @@ -226,7 +259,7 @@ trait Foo { } ``` -### The trait cannot contain associated constants +### Trait contains associated constants Just like static functions, associated constants aren't stored on the method table. If the trait or any subtrait contain an associated constant, they cannot @@ -248,7 +281,7 @@ trait Foo { } ``` -### The trait cannot use `Self` as a type parameter in the supertrait listing +### Trait uses `Self` as a type parameter in the supertrait listing This is similar to the second sub-error, but subtler. It happens in situations like the following: From bb27b051046712c903670f7401a32e990a950f05 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 10:14:50 -0800 Subject: [PATCH 03/15] Separate `RemoveFalseEdges` from `SimplifyBranches` Otherwise dataflow state will propagate along false edges and cause things to be marked as maybe init unnecessarily. These should be separate, since `SimplifyBranches` also makes `if true {} else {}` into a `goto`, which means we wouldn't lint anything in the `else` block. --- compiler/rustc_mir_transform/src/lib.rs | 7 +++-- .../src/remove_false_edges.rs | 29 +++++++++++++++++++ .../src/simplify_branches.rs | 17 ++++------- 3 files changed, 38 insertions(+), 15 deletions(-) create mode 100644 compiler/rustc_mir_transform/src/remove_false_edges.rs diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index f9ef314627807..5ae06a3ececdc 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -60,6 +60,7 @@ mod match_branches; mod multiple_return_terminators; mod normalize_array_len; mod nrvo; +mod remove_false_edges; mod remove_noop_landing_pads; mod remove_storage_markers; mod remove_unneeded_drops; @@ -456,7 +457,7 @@ fn run_post_borrowck_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tc let post_borrowck_cleanup: &[&dyn MirPass<'tcx>] = &[ // Remove all things only needed by analysis - &simplify_branches::SimplifyBranches::new("initial"), + &simplify_branches::SimplifyConstCondition::new("initial"), &remove_noop_landing_pads::RemoveNoopLandingPads, &cleanup_post_borrowck::CleanupNonCodegenStatements, &simplify::SimplifyCfg::new("early-opt"), @@ -515,13 +516,13 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { &instcombine::InstCombine, &separate_const_switch::SeparateConstSwitch, &const_prop::ConstProp, - &simplify_branches::SimplifyBranches::new("after-const-prop"), + &simplify_branches::SimplifyConstCondition::new("after-const-prop"), &early_otherwise_branch::EarlyOtherwiseBranch, &simplify_comparison_integral::SimplifyComparisonIntegral, &simplify_try::SimplifyArmIdentity, &simplify_try::SimplifyBranchSame, &dest_prop::DestinationPropagation, - &simplify_branches::SimplifyBranches::new("final"), + &simplify_branches::SimplifyConstCondition::new("final"), &remove_noop_landing_pads::RemoveNoopLandingPads, &simplify::SimplifyCfg::new("final"), &nrvo::RenameReturnPlace, diff --git a/compiler/rustc_mir_transform/src/remove_false_edges.rs b/compiler/rustc_mir_transform/src/remove_false_edges.rs new file mode 100644 index 0000000000000..71f5ccf7e2465 --- /dev/null +++ b/compiler/rustc_mir_transform/src/remove_false_edges.rs @@ -0,0 +1,29 @@ +use rustc_middle::mir::{Body, TerminatorKind}; +use rustc_middle::ty::TyCtxt; + +use crate::MirPass; + +/// Removes `FalseEdge` and `FalseUnwind` terminators from the MIR. +/// +/// These are only needed for borrow checking, and can be removed afterwards. +/// +/// FIXME: This should probably have its own MIR phase. +pub struct RemoveFalseEdges; + +impl<'tcx> MirPass<'tcx> for RemoveFalseEdges { + fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + for block in body.basic_blocks_mut() { + let terminator = block.terminator_mut(); + terminator.kind = match terminator.kind { + TerminatorKind::FalseEdge { real_target, .. } => { + TerminatorKind::Goto { target: real_target } + } + TerminatorKind::FalseUnwind { real_target, .. } => { + TerminatorKind::Goto { target: real_target } + } + + _ => continue, + } + } + } +} diff --git a/compiler/rustc_mir_transform/src/simplify_branches.rs b/compiler/rustc_mir_transform/src/simplify_branches.rs index df90cfa318df0..4b261334f3e54 100644 --- a/compiler/rustc_mir_transform/src/simplify_branches.rs +++ b/compiler/rustc_mir_transform/src/simplify_branches.rs @@ -1,22 +1,21 @@ -//! A pass that simplifies branches when their condition is known. - use crate::MirPass; use rustc_middle::mir::*; use rustc_middle::ty::TyCtxt; use std::borrow::Cow; -pub struct SimplifyBranches { +/// A pass that replaces a branch with a goto when its condition is known. +pub struct SimplifyConstCondition { label: String, } -impl SimplifyBranches { +impl SimplifyConstCondition { pub fn new(label: &str) -> Self { - SimplifyBranches { label: format!("SimplifyBranches-{}", label) } + SimplifyConstCondition { label: format!("SimplifyConstCondition-{}", label) } } } -impl<'tcx> MirPass<'tcx> for SimplifyBranches { +impl<'tcx> MirPass<'tcx> for SimplifyConstCondition { fn name(&self) -> Cow<'_, str> { Cow::Borrowed(&self.label) } @@ -53,12 +52,6 @@ impl<'tcx> MirPass<'tcx> for SimplifyBranches { Some(v) if v == expected => TerminatorKind::Goto { target }, _ => continue, }, - TerminatorKind::FalseEdge { real_target, .. } => { - TerminatorKind::Goto { target: real_target } - } - TerminatorKind::FalseUnwind { real_target, .. } => { - TerminatorKind::Goto { target: real_target } - } _ => continue, }; } From d7077073648041ec505629b2f1df9ab800c422e2 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 14:56:26 -0800 Subject: [PATCH 04/15] Add "is" methods for projections to a given index --- compiler/rustc_middle/src/mir/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index a05b8a1da8d7f..9fc12ad4a0b91 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1803,6 +1803,16 @@ impl ProjectionElem { | Self::Downcast(_, _) => false, } } + + /// Returns `true` if this is a `Downcast` projection with the given `VariantIdx`. + pub fn is_downcast_to(&self, v: VariantIdx) -> bool { + matches!(*self, Self::Downcast(_, x) if x == v) + } + + /// Returns `true` if this is a `Field` projection with the given index. + pub fn is_field_to(&self, f: Field) -> bool { + matches!(*self, Self::Field(x, _) if x == f) + } } /// Alias for projections as they appear in places, where the base is a place From 4f7605b6fdb81c1f5be62446f56b25a7cbaa8eeb Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 15:04:49 -0800 Subject: [PATCH 05/15] Add `RemoveUninitDrops` MIR pass --- compiler/rustc_mir_transform/src/lib.rs | 1 + .../src/remove_uninit_drops.rs | 171 ++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 compiler/rustc_mir_transform/src/remove_uninit_drops.rs diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 5ae06a3ececdc..b9d670e651cd4 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -63,6 +63,7 @@ mod nrvo; mod remove_false_edges; mod remove_noop_landing_pads; mod remove_storage_markers; +mod remove_uninit_drops; mod remove_unneeded_drops; mod remove_zsts; mod required_consts; diff --git a/compiler/rustc_mir_transform/src/remove_uninit_drops.rs b/compiler/rustc_mir_transform/src/remove_uninit_drops.rs new file mode 100644 index 0000000000000..c219f26732441 --- /dev/null +++ b/compiler/rustc_mir_transform/src/remove_uninit_drops.rs @@ -0,0 +1,171 @@ +use rustc_index::bit_set::BitSet; +use rustc_middle::mir::{Body, Field, Rvalue, Statement, StatementKind, TerminatorKind}; +use rustc_middle::ty::subst::SubstsRef; +use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, VariantDef}; +use rustc_mir_dataflow::impls::MaybeInitializedPlaces; +use rustc_mir_dataflow::move_paths::{LookupResult, MoveData, MovePathIndex}; +use rustc_mir_dataflow::{self, move_path_children_matching, Analysis, MoveDataParamEnv}; + +use crate::MirPass; + +/// Removes `Drop` and `DropAndReplace` terminators whose target is known to be uninitialized at +/// that point. +/// +/// This is redundant with drop elaboration, but we need to do it prior to const-checking, and +/// running const-checking after drop elaboration makes it opimization dependent, causing issues +/// like [#90770]. +/// +/// [#90770]: https://github.com/rust-lang/rust/issues/90770 +pub struct RemoveUninitDrops; + +impl<'tcx> MirPass<'tcx> for RemoveUninitDrops { + fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { + let param_env = tcx.param_env(body.source.def_id()); + let Ok(move_data) = MoveData::gather_moves(body, tcx, param_env) else { + // We could continue if there are move errors, but there's not much point since our + // init data isn't complete. + return; + }; + + let mdpe = MoveDataParamEnv { move_data, param_env }; + let mut maybe_inits = MaybeInitializedPlaces::new(tcx, body, &mdpe) + .into_engine(tcx, body) + .pass_name("remove_uninit_drops") + .iterate_to_fixpoint() + .into_results_cursor(body); + + let mut to_remove = vec![]; + for (bb, block) in body.basic_blocks().iter_enumerated() { + let terminator = block.terminator(); + let (TerminatorKind::Drop { place, .. } | TerminatorKind::DropAndReplace { place, .. }) + = &terminator.kind + else { continue }; + + maybe_inits.seek_before_primary_effect(body.terminator_loc(bb)); + + // If there's no move path for the dropped place, it's probably a `Deref`. Let it alone. + let LookupResult::Exact(mpi) = mdpe.move_data.rev_lookup.find(place.as_ref()) else { + continue; + }; + + let should_keep = is_needs_drop_and_init( + tcx, + param_env, + maybe_inits.get(), + &mdpe.move_data, + place.ty(body, tcx).ty, + mpi, + ); + if !should_keep { + to_remove.push(bb) + } + } + + for bb in to_remove { + let block = &mut body.basic_blocks_mut()[bb]; + + let (TerminatorKind::Drop { target, .. } | TerminatorKind::DropAndReplace { target, .. }) + = &block.terminator().kind + else { unreachable!() }; + + // Replace block terminator with `Goto`. + let target = *target; + let old_terminator_kind = std::mem::replace( + &mut block.terminator_mut().kind, + TerminatorKind::Goto { target }, + ); + + // If this is a `DropAndReplace`, we need to emulate the assignment to the return place. + if let TerminatorKind::DropAndReplace { place, value, .. } = old_terminator_kind { + block.statements.push(Statement { + source_info: block.terminator().source_info, + kind: StatementKind::Assign(Box::new((place, Rvalue::Use(value)))), + }); + } + } + } +} + +fn is_needs_drop_and_init( + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, + maybe_inits: &BitSet, + move_data: &MoveData<'tcx>, + ty: Ty<'tcx>, + mpi: MovePathIndex, +) -> bool { + // No need to look deeper if the root is definitely uninit or if it has no `Drop` impl. + if !maybe_inits.contains(mpi) || !ty.needs_drop(tcx, param_env) { + return false; + } + + let field_needs_drop_and_init = |(f, f_ty, mpi)| { + let child = move_path_children_matching(move_data, mpi, |x| x.is_field_to(f)); + let Some(mpi) = child else { + return f_ty.needs_drop(tcx, param_env); + }; + + is_needs_drop_and_init(tcx, param_env, maybe_inits, move_data, f_ty, mpi) + }; + + // This pass is only needed for const-checking, so it doesn't handle as many cases as + // `DropCtxt::open_drop`, since they aren't relevant in a const-context. + match ty.kind() { + ty::Adt(adt, substs) => { + let dont_elaborate = adt.is_union() || adt.is_manually_drop() || adt.has_dtor(tcx); + if dont_elaborate { + return true; + } + + // Look at all our fields, or if we are an enum all our variants and their fields. + // + // If a field's projection *is not* present in `MoveData`, it has the same + // initializedness as its parent (maybe init). + // + // If its projection *is* present in `MoveData`, then the field may have been moved + // from separate from its parent. Recurse. + adt.variants.iter_enumerated().any(|(vid, variant)| { + // Enums have multiple variants, which are discriminated with a `Downcast` projection. + // Structs have a single variant, and don't use a `Downcast` projection. + let mpi = if adt.is_enum() { + let downcast = + move_path_children_matching(move_data, mpi, |x| x.is_downcast_to(vid)); + let Some(dc_mpi) = downcast else { + return variant_needs_drop(tcx, param_env, substs, variant); + }; + + dc_mpi + } else { + mpi + }; + + variant + .fields + .iter() + .enumerate() + .map(|(f, field)| (Field::from_usize(f), field.ty(tcx, substs), mpi)) + .any(field_needs_drop_and_init) + }) + } + + ty::Tuple(_) => ty + .tuple_fields() + .enumerate() + .map(|(f, f_ty)| (Field::from_usize(f), f_ty, mpi)) + .any(field_needs_drop_and_init), + + _ => true, + } +} + +fn variant_needs_drop( + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, + substs: SubstsRef<'tcx>, + variant: &VariantDef, +) -> bool { + variant.fields.iter().any(|field| { + let f_ty = field.ty(tcx, substs); + f_ty.needs_drop(tcx, param_env) + }) +} From ce2959da97aa98596ee041a3e42d30e50e3f2d7b Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 15:03:43 -0800 Subject: [PATCH 06/15] Add rationale for `RemoveUnneededDrops` ...since its name is very close to `RemoveUninitDrops`. --- compiler/rustc_mir_transform/src/remove_unneeded_drops.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/remove_unneeded_drops.rs b/compiler/rustc_mir_transform/src/remove_unneeded_drops.rs index c71bc512c31b5..39f78e9555e2f 100644 --- a/compiler/rustc_mir_transform/src/remove_unneeded_drops.rs +++ b/compiler/rustc_mir_transform/src/remove_unneeded_drops.rs @@ -1,4 +1,8 @@ -//! This pass replaces a drop of a type that does not need dropping, with a goto +//! This pass replaces a drop of a type that does not need dropping, with a goto. +//! +//! When the MIR is built, we check `needs_drop` before emitting a `Drop` for a place. This pass is +//! useful because (unlike MIR building) it runs after type checking, so it can make use of +//! `Reveal::All` to provide more precies type information. use crate::MirPass; use rustc_middle::mir::*; From 3e0e8be037f79a8d2da521e75e52795727474beb Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 15:45:16 -0800 Subject: [PATCH 07/15] Handle `DropAndReplace` in const-checking It runs before the real drop elaboration pass. --- .../src/transform/check_consts/post_drop_elaboration.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/check_consts/post_drop_elaboration.rs b/compiler/rustc_const_eval/src/transform/check_consts/post_drop_elaboration.rs index 7a2be3c3bad32..c1d47baa40531 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/post_drop_elaboration.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/post_drop_elaboration.rs @@ -80,7 +80,8 @@ impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> { trace!("visit_terminator: terminator={:?} location={:?}", terminator, location); match &terminator.kind { - mir::TerminatorKind::Drop { place: dropped_place, .. } => { + mir::TerminatorKind::Drop { place: dropped_place, .. } + | mir::TerminatorKind::DropAndReplace { place: dropped_place, .. } => { let dropped_ty = dropped_place.ty(self.body, self.tcx).ty; if !NeedsNonConstDrop::in_any_value_of_ty(self.ccx, dropped_ty) { // Instead of throwing a bug, we just return here. This is because we have to @@ -104,11 +105,6 @@ impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> { } } - mir::TerminatorKind::DropAndReplace { .. } => span_bug!( - terminator.source_info.span, - "`DropAndReplace` should be removed by drop elaboration", - ), - mir::TerminatorKind::Abort | mir::TerminatorKind::Call { .. } | mir::TerminatorKind::Assert { .. } From 58c996c3a768446a39d72265bac626bc0a89adee Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 17:05:40 -0800 Subject: [PATCH 08/15] Move post-elaboration const-checking earlier in the pipeline Instead we run `RemoveFalseEdges` and `RemoveUninitDrops` at the appropriate time. The extra `SimplifyCfg` avoids visiting unreachable blocks during `RemoveUninitDrops`. --- compiler/rustc_mir_transform/src/lib.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index b9d670e651cd4..d15ee3e32b785 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -77,7 +77,7 @@ mod simplify_try; mod uninhabited_enum_branching; mod unreachable_prop; -use rustc_const_eval::transform::check_consts; +use rustc_const_eval::transform::check_consts::{self, ConstCx}; use rustc_const_eval::transform::promote_consts; use rustc_const_eval::transform::validate; pub use rustc_const_eval::transform::MirPass; @@ -447,8 +447,20 @@ fn mir_drops_elaborated_and_const_checked<'tcx>( let (body, _) = tcx.mir_promoted(def); let mut body = body.steal(); + // IMPORTANT + remove_false_edges::RemoveFalseEdges.run_pass(tcx, &mut body); + + // Do a little drop elaboration before const-checking if `const_precise_live_drops` is enabled. + // + // FIXME: Can't use `run_passes` for these, since `run_passes` SILENTLY DOES NOTHING IF THE MIR + // PHASE DOESN'T CHANGE. + if check_consts::post_drop_elaboration::checking_enabled(&ConstCx::new(tcx, &body)) { + simplify::SimplifyCfg::new("remove-false-edges").run_pass(tcx, &mut body); + remove_uninit_drops::RemoveUninitDrops.run_pass(tcx, &mut body); + check_consts::post_drop_elaboration::check_live_drops(tcx, &body); + } + run_post_borrowck_cleanup_passes(tcx, &mut body); - check_consts::post_drop_elaboration::check_live_drops(tcx, &body); tcx.alloc_steal_mir(body) } From 9aaca1d38ea3218b7f5030bd486cbbdf8917985b Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 30 Nov 2021 18:54:37 -0800 Subject: [PATCH 09/15] Update MIR opt tests with new name --- ...tch_int.main.SimplifyConstCondition-after-const-prop.diff} | 4 ++-- src/test/mir-opt/const_prop/switch_int.rs | 2 +- src/test/mir-opt/early_otherwise_branch_68867.rs | 2 +- ...wiseBranch.before-SimplifyConstCondition-final.after.diff} | 2 +- ...lify_if.main.SimplifyConstCondition-after-const-prop.diff} | 4 ++-- src/test/mir-opt/simplify_if.rs | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) rename src/test/mir-opt/const_prop/{switch_int.main.SimplifyBranches-after-const-prop.diff => switch_int.main.SimplifyConstCondition-after-const-prop.diff} (92%) rename src/test/mir-opt/{early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyBranches-final.after.diff => early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff} (99%) rename src/test/mir-opt/{simplify_if.main.SimplifyBranches-after-const-prop.diff => simplify_if.main.SimplifyConstCondition-after-const-prop.diff} (93%) diff --git a/src/test/mir-opt/const_prop/switch_int.main.SimplifyBranches-after-const-prop.diff b/src/test/mir-opt/const_prop/switch_int.main.SimplifyConstCondition-after-const-prop.diff similarity index 92% rename from src/test/mir-opt/const_prop/switch_int.main.SimplifyBranches-after-const-prop.diff rename to src/test/mir-opt/const_prop/switch_int.main.SimplifyConstCondition-after-const-prop.diff index 6a5b88c4a7f0d..f2b02551146eb 100644 --- a/src/test/mir-opt/const_prop/switch_int.main.SimplifyBranches-after-const-prop.diff +++ b/src/test/mir-opt/const_prop/switch_int.main.SimplifyConstCondition-after-const-prop.diff @@ -1,5 +1,5 @@ -- // MIR for `main` before SimplifyBranches-after-const-prop -+ // MIR for `main` after SimplifyBranches-after-const-prop +- // MIR for `main` before SimplifyConstCondition-after-const-prop ++ // MIR for `main` after SimplifyConstCondition-after-const-prop fn main() -> () { let mut _0: (); // return place in scope 0 at $DIR/switch_int.rs:6:11: 6:11 diff --git a/src/test/mir-opt/const_prop/switch_int.rs b/src/test/mir-opt/const_prop/switch_int.rs index 9e7c73404487a..d7319eca18e2d 100644 --- a/src/test/mir-opt/const_prop/switch_int.rs +++ b/src/test/mir-opt/const_prop/switch_int.rs @@ -2,7 +2,7 @@ fn foo(_: i32) { } // EMIT_MIR switch_int.main.ConstProp.diff -// EMIT_MIR switch_int.main.SimplifyBranches-after-const-prop.diff +// EMIT_MIR switch_int.main.SimplifyConstCondition-after-const-prop.diff fn main() { match 1 { 1 => foo(0), diff --git a/src/test/mir-opt/early_otherwise_branch_68867.rs b/src/test/mir-opt/early_otherwise_branch_68867.rs index 02221c4cf4a1f..ca298e9211d48 100644 --- a/src/test/mir-opt/early_otherwise_branch_68867.rs +++ b/src/test/mir-opt/early_otherwise_branch_68867.rs @@ -11,7 +11,7 @@ pub enum ViewportPercentageLength { } // EMIT_MIR early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.diff -// EMIT_MIR early_otherwise_branch_68867.try_sum EarlyOtherwiseBranch.before SimplifyBranches-final.after +// EMIT_MIR early_otherwise_branch_68867.try_sum EarlyOtherwiseBranch.before SimplifyConstCondition-final.after #[no_mangle] pub extern "C" fn try_sum( x: &ViewportPercentageLength, diff --git a/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyBranches-final.after.diff b/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff similarity index 99% rename from src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyBranches-final.after.diff rename to src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff index f23d035545eec..44294030439f3 100644 --- a/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyBranches-final.after.diff +++ b/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff @@ -1,5 +1,5 @@ - // MIR for `try_sum` before EarlyOtherwiseBranch -+ // MIR for `try_sum` after SimplifyBranches-final ++ // MIR for `try_sum` after SimplifyConstCondition-final fn try_sum(_1: &ViewportPercentageLength, _2: &ViewportPercentageLength) -> Result { debug x => _1; // in scope 0 at $DIR/early_otherwise_branch_68867.rs:17:5: 17:6 diff --git a/src/test/mir-opt/simplify_if.main.SimplifyBranches-after-const-prop.diff b/src/test/mir-opt/simplify_if.main.SimplifyConstCondition-after-const-prop.diff similarity index 93% rename from src/test/mir-opt/simplify_if.main.SimplifyBranches-after-const-prop.diff rename to src/test/mir-opt/simplify_if.main.SimplifyConstCondition-after-const-prop.diff index def6f835131c9..d11c70b1efec6 100644 --- a/src/test/mir-opt/simplify_if.main.SimplifyBranches-after-const-prop.diff +++ b/src/test/mir-opt/simplify_if.main.SimplifyConstCondition-after-const-prop.diff @@ -1,5 +1,5 @@ -- // MIR for `main` before SimplifyBranches-after-const-prop -+ // MIR for `main` after SimplifyBranches-after-const-prop +- // MIR for `main` before SimplifyConstCondition-after-const-prop ++ // MIR for `main` after SimplifyConstCondition-after-const-prop fn main() -> () { let mut _0: (); // return place in scope 0 at $DIR/simplify_if.rs:5:11: 5:11 diff --git a/src/test/mir-opt/simplify_if.rs b/src/test/mir-opt/simplify_if.rs index 67b2027b710c9..2d093d9266bb5 100644 --- a/src/test/mir-opt/simplify_if.rs +++ b/src/test/mir-opt/simplify_if.rs @@ -1,7 +1,7 @@ #[inline(never)] fn noop() {} -// EMIT_MIR simplify_if.main.SimplifyBranches-after-const-prop.diff +// EMIT_MIR simplify_if.main.SimplifyConstCondition-after-const-prop.diff fn main() { if false { noop(); From 37fa92552586a9f91fefd92518b66dfde4c64771 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 1 Dec 2021 10:04:21 -0800 Subject: [PATCH 10/15] Add regression test for #90770 --- src/test/ui/consts/drop_zst.rs | 17 +++++++++++++++++ src/test/ui/consts/drop_zst.stderr | 9 +++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/test/ui/consts/drop_zst.rs create mode 100644 src/test/ui/consts/drop_zst.stderr diff --git a/src/test/ui/consts/drop_zst.rs b/src/test/ui/consts/drop_zst.rs new file mode 100644 index 0000000000000..f7c70d3978b7f --- /dev/null +++ b/src/test/ui/consts/drop_zst.rs @@ -0,0 +1,17 @@ +// check-fail + +#![feature(const_precise_live_drops)] + +struct S; + +impl Drop for S { + fn drop(&mut self) { + println!("Hello!"); + } +} + +const fn foo() { + let s = S; //~ destructor +} + +fn main() {} diff --git a/src/test/ui/consts/drop_zst.stderr b/src/test/ui/consts/drop_zst.stderr new file mode 100644 index 0000000000000..d4be5aa56d9af --- /dev/null +++ b/src/test/ui/consts/drop_zst.stderr @@ -0,0 +1,9 @@ +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/drop_zst.rs:14:9 + | +LL | let s = S; + | ^ constant functions cannot evaluate destructors + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0493`. From b11d88006cb03a8f50aa09d91aa457d6ec0b8cd8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 1 Dec 2021 22:48:59 -0500 Subject: [PATCH 11/15] disable tests in Miri that take too long --- library/core/tests/slice.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/core/tests/slice.rs b/library/core/tests/slice.rs index 7ba01caeefd7c..281df8a1326d0 100644 --- a/library/core/tests/slice.rs +++ b/library/core/tests/slice.rs @@ -2330,6 +2330,7 @@ macro_rules! empty_max_mut { }; } +#[cfg(not(miri))] // Comparing usize::MAX many elements takes forever in Miri (and in rustc without optimizations) take_tests! { slice: &[(); usize::MAX], method: take, (take_in_bounds_max_range_to, (..usize::MAX), Some(EMPTY_MAX), &[(); 0]), @@ -2337,6 +2338,7 @@ take_tests! { (take_in_bounds_max_range_from, (usize::MAX..), Some(&[] as _), EMPTY_MAX), } +#[cfg(not(miri))] // Comparing usize::MAX many elements takes forever in Miri (and in rustc without optimizations) take_tests! { slice: &mut [(); usize::MAX], method: take_mut, (take_mut_in_bounds_max_range_to, (..usize::MAX), Some(empty_max_mut!()), &mut [(); 0]), From bd8d7e4a454ea8b381c3108fccdbf3f5563de6f5 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 16 Oct 2021 17:52:06 +0200 Subject: [PATCH 12/15] Transform const generics if the function uses rustc_legacy_const_generics --- src/librustdoc/clean/mod.rs | 35 ++++++++++++++++++++++++++++++++++- src/librustdoc/clean/types.rs | 3 +++ src/librustdoc/html/format.rs | 4 ++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 4e1dabd05bb48..440ac5b1b41fc 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -749,11 +749,42 @@ fn clean_fn_or_proc_macro( } else { hir::Constness::NotConst }; + clean_fn_decl_legacy_const_generics(&mut func, attrs); FunctionItem(func) } } } +/// This is needed to make it more "readable" when documenting functions using +/// `rustc_legacy_const_generics`. More information in +/// . +fn clean_fn_decl_legacy_const_generics(func: &mut Function, attrs: &[ast::Attribute]) { + for meta_item_list in attrs + .iter() + .filter(|a| a.has_name(sym::rustc_legacy_const_generics)) + .filter_map(|a| a.meta_item_list()) + { + for (pos, literal) in meta_item_list.iter().filter_map(|meta| meta.literal()).enumerate() { + match literal.kind { + ast::LitKind::Int(a, _) => { + let gen = func.generics.params.remove(0); + if let GenericParamDef { name, kind: GenericParamDefKind::Const { ty, .. } } = + gen + { + func.decl + .inputs + .values + .insert(a as _, Argument { name, type_: *ty, is_const: true }); + } else { + panic!("unexpected non const in position {}", pos); + } + } + _ => panic!("invalid arg index"), + } + } + } +} + impl<'a> Clean for (&'a hir::FnSig<'a>, &'a hir::Generics<'a>, hir::BodyId) { fn clean(&self, cx: &mut DocContext<'_>) -> Function { let (generics, decl) = enter_impl_trait(cx, |cx| { @@ -779,7 +810,7 @@ impl<'a> Clean for (&'a [hir::Ty<'a>], &'a [Ident]) { if name.is_empty() { name = kw::Underscore; } - Argument { name, type_: ty.clean(cx) } + Argument { name, type_: ty.clean(cx), is_const: false } }) .collect(), } @@ -798,6 +829,7 @@ impl<'a> Clean for (&'a [hir::Ty<'a>], hir::BodyId) { .map(|(i, ty)| Argument { name: name_from_pat(body.params[i].pat), type_: ty.clean(cx), + is_const: false, }) .collect(), } @@ -828,6 +860,7 @@ impl<'tcx> Clean for (DefId, ty::PolyFnSig<'tcx>) { .map(|t| Argument { type_: t.clean(cx), name: names.next().map_or(kw::Empty, |i| i.name), + is_const: false, }) .collect(), }, diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 37acf68defd25..a5fd691d5ed2a 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1353,6 +1353,9 @@ crate struct Arguments { crate struct Argument { crate type_: Type, crate name: Symbol, + /// This field is used to represent "const" arguments from the `rustc_legacy_const_generics` + /// feature. More information in . + crate is_const: bool, } #[derive(Clone, PartialEq, Debug)] diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index 34742fac0e4b7..80a816ad46a7c 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -1177,6 +1177,10 @@ impl clean::FnDecl { args.push_str("
"); args_plain.push(' '); } + if input.is_const { + args.push_str("const "); + args_plain.push_str("const "); + } if !input.name.is_empty() { args.push_str(&format!("{}: ", input.name)); args_plain.push_str(&format!("{}: ", input.name)); From 5c75a4857ee447437ca71b981eadfb1a38ad7268 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 16 Oct 2021 17:52:24 +0200 Subject: [PATCH 13/15] Add test for legacy-const-generic arguments --- src/test/rustdoc/legacy-const-generic.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 src/test/rustdoc/legacy-const-generic.rs diff --git a/src/test/rustdoc/legacy-const-generic.rs b/src/test/rustdoc/legacy-const-generic.rs new file mode 100644 index 0000000000000..46a50e2fc30b4 --- /dev/null +++ b/src/test/rustdoc/legacy-const-generic.rs @@ -0,0 +1,16 @@ +#![crate_name = "foo"] +#![feature(rustc_attrs)] + +// @has 'foo/fn.foo.html' +// @has - '//*[@class="rust fn"]' 'fn foo(x: usize, const Y: usize, z: usize) -> [usize; 3]' +#[rustc_legacy_const_generics(1)] +pub fn foo(x: usize, z: usize) -> [usize; 3] { + [x, Y, z] +} + +// @has 'foo/fn.bar.html' +// @has - '//*[@class="rust fn"]' 'fn bar(x: usize, const Y: usize, const Z: usize) -> [usize; 3]' +#[rustc_legacy_const_generics(1, 2)] +pub fn bar(x: usize) -> [usize; 3] { + [x, Y, z] +} From 51875e3d5a9595663cf19e9ebffd182fe680869c Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Thu, 2 Dec 2021 17:46:57 +0100 Subject: [PATCH 14/15] Add additional test from rust issue number 91068 --- ...implied-bounds-unnorm-associated-type-2.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs diff --git a/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs b/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs new file mode 100644 index 0000000000000..cf004290b0cd2 --- /dev/null +++ b/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs @@ -0,0 +1,22 @@ +// build-pass + +trait Trait { + type Type; +} + +impl Trait for T { + type Type = (); +} + +fn f<'a, 'b>(_: <&'a &'b () as Trait>::Type) +where + 'a: 'a, + 'b: 'b, +{ +} + +fn g<'a, 'b>() { + f::<'a, 'b>(()); +} + +fn main() {} From b77ed83ee246631aefe5d00b65bb936b856807c7 Mon Sep 17 00:00:00 2001 From: Jack Huey <31162821+jackh726@users.noreply.github.com> Date: Thu, 2 Dec 2021 12:11:31 -0500 Subject: [PATCH 15/15] Change to check-pass --- src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs b/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs index cf004290b0cd2..5a92bcd37b6ee 100644 --- a/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs +++ b/src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs @@ -1,4 +1,4 @@ -// build-pass +// check-pass trait Trait { type Type;