From 2a83c11d4d2468fd753daddb0dfffd392d09f289 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Fri, 26 Nov 2021 22:06:08 +0000 Subject: [PATCH 01/21] Handle placeholder regions in NLL type outlive constraints --- .../src/type_check/constraint_conversion.rs | 15 ++++++++++++++- .../ui/lifetimes/issue-76168-hr-outlives.rs | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/lifetimes/issue-76168-hr-outlives.rs diff --git a/compiler/rustc_borrowck/src/type_check/constraint_conversion.rs b/compiler/rustc_borrowck/src/type_check/constraint_conversion.rs index ab1a7461b4b9b..a3b39591f8db2 100644 --- a/compiler/rustc_borrowck/src/type_check/constraint_conversion.rs +++ b/compiler/rustc_borrowck/src/type_check/constraint_conversion.rs @@ -6,6 +6,7 @@ use rustc_infer::infer::region_constraints::{GenericKind, VerifyBound}; use rustc_infer::infer::{self, InferCtxt, SubregionOrigin}; use rustc_middle::mir::ConstraintCategory; use rustc_middle::ty::subst::GenericArgKind; +use rustc_middle::ty::TypeFoldable; use rustc_middle::ty::{self, TyCtxt}; use rustc_span::DUMMY_SP; @@ -95,11 +96,23 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> { self.add_outlives(r1_vid, r2_vid); } - GenericArgKind::Type(t1) => { + GenericArgKind::Type(mut t1) => { // we don't actually use this for anything, but // the `TypeOutlives` code needs an origin. let origin = infer::RelateParamBound(DUMMY_SP, t1, None); + // Placeholder regions need to be converted now because it may + // create new region variables, which can't be done later when + // verifying these bounds. + if t1.has_placeholders() { + t1 = tcx.fold_regions(&t1, &mut false, |r, _| match *r { + ty::RegionKind::RePlaceholder(placeholder) => { + self.constraints.placeholder_region(self.infcx, placeholder) + } + _ => r, + }); + } + TypeOutlives::new( &mut *self, tcx, diff --git a/src/test/ui/lifetimes/issue-76168-hr-outlives.rs b/src/test/ui/lifetimes/issue-76168-hr-outlives.rs new file mode 100644 index 0000000000000..9366e94c90ff5 --- /dev/null +++ b/src/test/ui/lifetimes/issue-76168-hr-outlives.rs @@ -0,0 +1,19 @@ +// edition:2018 +// check-pass + +#![feature(unboxed_closures)] +use std::future::Future; + +async fn wrapper(f: F) +where for<'a> F: FnOnce<(&'a mut i32,)>, + for<'a> >::Output: Future + 'a +{ + let mut i = 41; + f(&mut i).await; +} + +async fn add_one(i: &mut i32) { + *i = *i + 1; +} + +fn main() {} From 4910fe6889174a432201958043e19e2222e46b69 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 28 Nov 2021 14:31:22 -0500 Subject: [PATCH 02/21] Fix incorrect usage of `EvaluatedToOk` when evaluating `TypeOutlives` A global predicate is not guarnatenteed to outlive all regions. If the predicate involves late-bound regions, then it may fail to outlive other regions (e.g. `for<'b> &'b bool: 'static` does not hold) We now only produce `EvaluatedToOk` when a global predicate has no late-bound regions - in that case, the ony region that can be present in the type is 'static --- .../src/traits/select/mod.rs | 6 +- src/test/ui/traits/project-modulo-regions.rs | 55 +++++++++++++++++++ .../project-modulo-regions.with_clause.stderr | 11 ++++ ...oject-modulo-regions.without_clause.stderr | 11 ++++ 4 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/traits/project-modulo-regions.rs create mode 100644 src/test/ui/traits/project-modulo-regions.with_clause.stderr create mode 100644 src/test/ui/traits/project-modulo-regions.without_clause.stderr diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 767cb1618bb67..ee16d6b3cfbe3 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -558,7 +558,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { }, ty::PredicateKind::TypeOutlives(pred) => { - if pred.0.is_known_global() { + // A global type with no late-bound regions can only + // contain the "'static" lifetime (any other lifetime + // would either be late-bound or local), so it is guaranteed + // to outlive any other lifetime + if pred.0.is_global(self.infcx.tcx) && !pred.0.has_late_bound_regions() { Ok(EvaluatedToOk) } else { Ok(EvaluatedToOkModuloRegions) diff --git a/src/test/ui/traits/project-modulo-regions.rs b/src/test/ui/traits/project-modulo-regions.rs new file mode 100644 index 0000000000000..f0c0dd3ed9578 --- /dev/null +++ b/src/test/ui/traits/project-modulo-regions.rs @@ -0,0 +1,55 @@ +// revisions: with_clause without_clause +// Tests that `EvaluatedToOkModuloRegions` from a projection sub-obligation +// is correctly propagated + +#![feature(rustc_attrs)] + +trait MyTrait { + type Assoc; +} + +struct MyStruct; + +impl MyTrait for MyStruct { + // Evaluating this projection will result in `EvaluatedToOkModuloRegions` + // (when `with_clause` is enabled) + type Assoc = ::Assoc; +} + +struct Bar; + +// The `where` clause on this impl will cause us to produce `EvaluatedToOkModuloRegions` +// when evaluating a projection involving this impl +#[cfg(with_clause)] +impl MyTrait for Bar where for<'b> &'b (): 'b { + type Assoc = bool; +} + +// This impl tests that the `EvaluatedToOkModuoRegions` result that we get +// is really due to the `where` clause on the `with_clause` impl +#[cfg(without_clause)] +impl MyTrait for Bar { + type Assoc = bool; +} + +// The implementation of `#[rustc_evaluate_where_clauses]` doesn't perform +// normalization, so we need to place the projection predicate behind a normal +// trait predicate +struct Helper {} +trait HelperTrait {} +impl HelperTrait for Helper where ::Assoc: Sized {} + +// Evaluating this 'where' clause will (recursively) end up evaluating +// `for<'b> &'b (): 'b`, which will produce `EvaluatedToOkModuloRegions` +#[rustc_evaluate_where_clauses] +fn test(val: MyStruct) where Helper: HelperTrait { + panic!() +} + +fn foo(val: MyStruct) { + test(val); + //[with_clause]~^ ERROR evaluate(Binder(TraitPredicate(, polarity:Positive), [])) = Ok(EvaluatedToOkModuloRegions) + //[without_clause]~^^ ERROR evaluate(Binder(TraitPredicate(, polarity:Positive), [])) = Ok(EvaluatedToOk) +} + +fn main() {} diff --git a/src/test/ui/traits/project-modulo-regions.with_clause.stderr b/src/test/ui/traits/project-modulo-regions.with_clause.stderr new file mode 100644 index 0000000000000..2434c32c81882 --- /dev/null +++ b/src/test/ui/traits/project-modulo-regions.with_clause.stderr @@ -0,0 +1,11 @@ +error: evaluate(Binder(TraitPredicate(, polarity:Positive), [])) = Ok(EvaluatedToOkModuloRegions) + --> $DIR/project-modulo-regions.rs:50:5 + | +LL | fn test(val: MyStruct) where Helper: HelperTrait { + | ----------- predicate +... +LL | test(val); + | ^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/traits/project-modulo-regions.without_clause.stderr b/src/test/ui/traits/project-modulo-regions.without_clause.stderr new file mode 100644 index 0000000000000..9d35690d5f0fe --- /dev/null +++ b/src/test/ui/traits/project-modulo-regions.without_clause.stderr @@ -0,0 +1,11 @@ +error: evaluate(Binder(TraitPredicate(, polarity:Positive), [])) = Ok(EvaluatedToOk) + --> $DIR/project-modulo-regions.rs:50:5 + | +LL | fn test(val: MyStruct) where Helper: HelperTrait { + | ----------- predicate +... +LL | test(val); + | ^^^^ + +error: aborting due to previous error + From 821b92b102064cdef6787fe1114dae948fbbb62d Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Mon, 29 Nov 2021 19:31:17 +0100 Subject: [PATCH 03/21] Improve error message for incorrect field accesses through raw pointers --- compiler/rustc_typeck/src/check/expr.rs | 32 ++++++++++++++++++- .../ui/typeck/issue-91210-ptr-method.fixed | 15 +++++++++ src/test/ui/typeck/issue-91210-ptr-method.rs | 15 +++++++++ .../ui/typeck/issue-91210-ptr-method.stderr | 11 +++++++ 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/typeck/issue-91210-ptr-method.fixed create mode 100644 src/test/ui/typeck/issue-91210-ptr-method.rs create mode 100644 src/test/ui/typeck/issue-91210-ptr-method.stderr diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index eb997b014c77b..311106474bea0 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -46,6 +46,7 @@ use rustc_span::hygiene::DesugaringKind; use rustc_span::lev_distance::find_best_match_for_name; use rustc_span::source_map::Span; use rustc_span::symbol::{kw, sym, Ident, Symbol}; +use rustc_span::{BytePos, Pos}; use rustc_trait_selection::traits::{self, ObligationCauseCode}; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { @@ -2063,7 +2064,36 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { Some(span), ); } else { - err.help("methods are immutable and cannot be assigned to"); + let mut found = false; + + if let ty::RawPtr(ty_and_mut) = expr_t.kind() { + if let ty::Adt(adt_def, _) = ty_and_mut.ty.kind() { + if adt_def.variants.len() == 1 + && adt_def + .variants + .iter() + .next() + .unwrap() + .fields + .iter() + .any(|f| f.ident == field) + { + if let Some(dot_loc) = expr_snippet.rfind('.') { + found = true; + err.span_suggestion( + expr.span.with_hi(expr.span.lo() + BytePos::from_usize(dot_loc)), + "to access the field, dereference first", + format!("(*{})", &expr_snippet[0..dot_loc]), + Applicability::MaybeIncorrect, + ); + } + } + } + } + + if !found { + err.help("methods are immutable and cannot be assigned to"); + } } err.emit(); diff --git a/src/test/ui/typeck/issue-91210-ptr-method.fixed b/src/test/ui/typeck/issue-91210-ptr-method.fixed new file mode 100644 index 0000000000000..94200cce73ec0 --- /dev/null +++ b/src/test/ui/typeck/issue-91210-ptr-method.fixed @@ -0,0 +1,15 @@ +// Regression test for issue #91210. + +// run-rustfix + +#![allow(unused)] + +struct Foo { read: i32 } + +unsafe fn blah(x: *mut Foo) { + (*x).read = 4; + //~^ ERROR: attempted to take value of method + //~| HELP: to access the field, dereference first +} + +fn main() {} diff --git a/src/test/ui/typeck/issue-91210-ptr-method.rs b/src/test/ui/typeck/issue-91210-ptr-method.rs new file mode 100644 index 0000000000000..ed0ce6effe7d9 --- /dev/null +++ b/src/test/ui/typeck/issue-91210-ptr-method.rs @@ -0,0 +1,15 @@ +// Regression test for issue #91210. + +// run-rustfix + +#![allow(unused)] + +struct Foo { read: i32 } + +unsafe fn blah(x: *mut Foo) { + x.read = 4; + //~^ ERROR: attempted to take value of method + //~| HELP: to access the field, dereference first +} + +fn main() {} diff --git a/src/test/ui/typeck/issue-91210-ptr-method.stderr b/src/test/ui/typeck/issue-91210-ptr-method.stderr new file mode 100644 index 0000000000000..503a32373d570 --- /dev/null +++ b/src/test/ui/typeck/issue-91210-ptr-method.stderr @@ -0,0 +1,11 @@ +error[E0615]: attempted to take value of method `read` on type `*mut Foo` + --> $DIR/issue-91210-ptr-method.rs:10:7 + | +LL | x.read = 4; + | - ^^^^ method, not a field + | | + | help: to access the field, dereference first: `(*x)` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0615`. From 7907fa8ec4cd4e7b60687e3f06b12e2b8fff6a04 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Mon, 29 Nov 2021 22:10:49 -0800 Subject: [PATCH 04/21] 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 05/21] 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 06/21] 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 07/21] 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 08/21] 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 09/21] 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 10/21] 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 11/21] 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 12/21] 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 ba7374e517415696c383a6c7b79214d2168dff21 Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Wed, 1 Dec 2021 22:36:50 +0100 Subject: [PATCH 13/21] Improve diagnostic for missing half of binary operator in `if` condition --- compiler/rustc_parse/src/parser/expr.rs | 51 +++++++++++++++++---- src/test/ui/expr/if/if-without-block.stderr | 6 ++- src/test/ui/parser/issue-91421.rs | 10 ++++ src/test/ui/parser/issue-91421.stderr | 21 +++++++++ 4 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 src/test/ui/parser/issue-91421.rs create mode 100644 src/test/ui/parser/issue-91421.stderr diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index f7ee874c83167..1dbd7bad0f023 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -1988,25 +1988,34 @@ impl<'a> Parser<'a> { let lo = self.prev_token.span; let cond = self.parse_cond_expr()?; + let missing_then_block_binop_span = || { + match cond.kind { + ExprKind::Binary(Spanned { span: binop_span, .. }, _, ref right) + if let ExprKind::Block(..) = right.kind => Some(binop_span), + _ => None + } + }; + // Verify that the parsed `if` condition makes sense as a condition. If it is a block, then // verify that the last statement is either an implicit return (no `;`) or an explicit // return. This won't catch blocks with an explicit `return`, but that would be caught by // the dead code lint. - let thn = if self.eat_keyword(kw::Else) || !cond.returns() { - self.error_missing_if_cond(lo, cond.span) + let thn = if self.token.is_keyword(kw::Else) || !cond.returns() { + if let Some(binop_span) = missing_then_block_binop_span() { + self.error_missing_if_then_block(lo, None, Some(binop_span)).emit(); + self.mk_block_err(cond.span) + } else { + self.error_missing_if_cond(lo, cond.span) + } } else { let attrs = self.parse_outer_attributes()?.take_for_recovery(); // For recovery. let not_block = self.token != token::OpenDelim(token::Brace); - let block = self.parse_block().map_err(|mut err| { + let block = self.parse_block().map_err(|err| { if not_block { - err.span_label(lo, "this `if` expression has a condition, but no block"); - if let ExprKind::Binary(_, _, ref right) = cond.kind { - if let ExprKind::Block(_, _) = right.kind { - err.help("maybe you forgot the right operand of the condition?"); - } - } + self.error_missing_if_then_block(lo, Some(err), missing_then_block_binop_span()) + } else { + err } - err })?; self.error_on_if_block_attrs(lo, false, block.span, &attrs); block @@ -2015,6 +2024,28 @@ impl<'a> Parser<'a> { Ok(self.mk_expr(lo.to(self.prev_token.span), ExprKind::If(cond, thn, els), attrs)) } + fn error_missing_if_then_block( + &self, + if_span: Span, + err: Option>, + binop_span: Option, + ) -> DiagnosticBuilder<'a> { + let msg = "this `if` expression has a condition, but no block"; + + let mut err = if let Some(mut err) = err { + err.span_label(if_span, msg); + err + } else { + self.struct_span_err(if_span, msg) + }; + + if let Some(binop_span) = binop_span { + err.span_help(binop_span, "maybe you forgot the right operand of the condition?"); + } + + err + } + fn error_missing_if_cond(&self, lo: Span, span: Span) -> P { let sp = self.sess.source_map().next_point(lo); self.struct_span_err(sp, "missing condition for `if` expression") diff --git a/src/test/ui/expr/if/if-without-block.stderr b/src/test/ui/expr/if/if-without-block.stderr index ee2bb62e2bb57..d3f6ca07617f4 100644 --- a/src/test/ui/expr/if/if-without-block.stderr +++ b/src/test/ui/expr/if/if-without-block.stderr @@ -7,7 +7,11 @@ LL | if 5 == { LL | } | ^ expected `{` | - = help: maybe you forgot the right operand of the condition? +help: maybe you forgot the right operand of the condition? + --> $DIR/if-without-block.rs:3:10 + | +LL | if 5 == { + | ^^ error: aborting due to previous error diff --git a/src/test/ui/parser/issue-91421.rs b/src/test/ui/parser/issue-91421.rs new file mode 100644 index 0000000000000..9959df5663837 --- /dev/null +++ b/src/test/ui/parser/issue-91421.rs @@ -0,0 +1,10 @@ +// Regression test for issue #91421. + +fn main() { + let value = if true && { + //~^ ERROR: this `if` expression has a condition, but no block + //~| HELP: maybe you forgot the right operand of the condition? + 3 + //~^ ERROR: mismatched types [E0308] + } else { 4 }; +} diff --git a/src/test/ui/parser/issue-91421.stderr b/src/test/ui/parser/issue-91421.stderr new file mode 100644 index 0000000000000..04284d5e3b2f7 --- /dev/null +++ b/src/test/ui/parser/issue-91421.stderr @@ -0,0 +1,21 @@ +error: this `if` expression has a condition, but no block + --> $DIR/issue-91421.rs:4:17 + | +LL | let value = if true && { + | ^^ + | +help: maybe you forgot the right operand of the condition? + --> $DIR/issue-91421.rs:4:25 + | +LL | let value = if true && { + | ^^ + +error[E0308]: mismatched types + --> $DIR/issue-91421.rs:7:9 + | +LL | 3 + | ^ expected `bool`, found integer + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0308`. From b11d88006cb03a8f50aa09d91aa457d6ec0b8cd8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 1 Dec 2021 22:48:59 -0500 Subject: [PATCH 14/21] 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 15/21] 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 16/21] 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 17/21] 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 18/21] 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; From 6df44a389cfab95807027a420903ee0b8cc08f0e Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 2 Dec 2021 17:51:44 +0000 Subject: [PATCH 19/21] Document how `last_os_error` should be used --- library/std/src/io/error.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/std/src/io/error.rs b/library/std/src/io/error.rs index d93c6172cfcf6..da88c8c9261b4 100644 --- a/library/std/src/io/error.rs +++ b/library/std/src/io/error.rs @@ -440,12 +440,18 @@ impl Error { /// `GetLastError` on Windows) and will return a corresponding instance of /// [`Error`] for the error code. /// + /// This should be called immediately after a call to a platform function, + /// otherwise the state of the error value is indeterminate. In particular, + /// other standard library functions may call platform functions that may + /// (or may not) reset the error value even if they succeed. + /// /// # Examples /// /// ``` /// use std::io::Error; /// - /// println!("last OS error: {:?}", Error::last_os_error()); + /// let os_error = Error::last_os_error(); + /// println!("last OS error: {:?}", os_error); /// ``` #[stable(feature = "rust1", since = "1.0.0")] #[must_use] From d8832425fc407f36191dcc65c4bc41319aca5c14 Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Thu, 2 Dec 2021 19:37:25 +0000 Subject: [PATCH 20/21] Document file path case sensitivity --- library/std/src/path.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index cf2cd5adc4848..9ade2847e8ea9 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -12,6 +12,13 @@ //! [`PathBuf`]; note that the paths may differ syntactically by the //! normalization described in the documentation for the [`components`] method. //! +//! ## Case sensitivity +//! +//! Unless otherwise indicated path methods that do not access the filesystem, +//! such as [`Path::starts_with`] and [`Path::ends_with`], are case sensitive no +//! matter the platform or filesystem. An exception to this is made for Windows +//! drive letters. +//! //! ## Simple usage //! //! Path manipulation includes both parsing components from slices and building From 66153d76e25834812d2785675c98e95d5ecb1830 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 3 Dec 2021 07:12:31 +1100 Subject: [PATCH 21/21] Improve the comments in `Symbol::interner`. --- compiler/rustc_span/src/symbol.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 247d69d6ee97b..dd6ce60abfb3d 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -1716,8 +1716,9 @@ pub(crate) struct Interner(Lock); // found that to regress performance up to 2% in some cases. This might be // revisited after further improvements to `indexmap`. // -// This type is private to prevent accidentally constructing more than one `Interner` on the same -// thread, which makes it easy to mixup `Symbol`s between `Interner`s. +// This type is private to prevent accidentally constructing more than one +// `Interner` on the same thread, which makes it easy to mixup `Symbol`s +// between `Interner`s. #[derive(Default)] struct InternerInner { arena: DroplessArena, @@ -1743,14 +1744,20 @@ impl Interner { let name = Symbol::new(inner.strings.len() as u32); - // `from_utf8_unchecked` is safe since we just allocated a `&str` which is known to be - // UTF-8. + // SAFETY: we convert from `&str` to `&[u8]`, clone it into the arena, + // and immediately convert the clone back to `&[u8], all because there + // is no `inner.arena.alloc_str()` method. This is clearly safe. let string: &str = unsafe { str::from_utf8_unchecked(inner.arena.alloc_slice(string.as_bytes())) }; - // It is safe to extend the arena allocation to `'static` because we only access - // these while the arena is still alive. + + // SAFETY: we can extend the arena allocation to `'static` because we + // only access these while the arena is still alive. let string: &'static str = unsafe { &*(string as *const str) }; inner.strings.push(string); + + // This second hash table lookup can be avoided by using `RawEntryMut`, + // but this code path isn't hot enough for it to be worth it. See + // #91445 for details. inner.names.insert(string, name); name }