From 707315857a506f99d123a98670341b2d619bca9c Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 20 Oct 2019 22:08:26 +0100 Subject: [PATCH 01/16] Don't ICE for completely unexpandable `impl Trait` types --- src/librustc_typeck/check/writeback.rs | 10 +++-- .../recursive-impl-trait-type-direct.rs | 7 ++++ .../recursive-impl-trait-type-direct.stderr | 11 +++++ ... => recursive-impl-trait-type-indirect.rs} | 0 ...recursive-impl-trait-type-indirect.stderr} | 40 +++++++++---------- ...-impl-trait-type-through-non-recursive.rs} | 0 ...l-trait-type-through-non-recursive.stderr} | 8 ++-- 7 files changed, 48 insertions(+), 28 deletions(-) create mode 100644 src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs create mode 100644 src/test/ui/impl-trait/recursive-impl-trait-type-direct.stderr rename src/test/ui/impl-trait/{recursive-impl-trait-type.rs => recursive-impl-trait-type-indirect.rs} (100%) rename src/test/ui/impl-trait/{recursive-impl-trait-type.stderr => recursive-impl-trait-type-indirect.stderr} (76%) rename src/test/ui/impl-trait/{recursive-impl-trait-type--through-non-recursize.rs => recursive-impl-trait-type-through-non-recursive.rs} (100%) rename src/test/ui/impl-trait/{recursive-impl-trait-type--through-non-recursize.stderr => recursive-impl-trait-type-through-non-recursive.stderr} (77%) diff --git a/src/librustc_typeck/check/writeback.rs b/src/librustc_typeck/check/writeback.rs index 7a8a209a5350c..3d380a5f1826c 100644 --- a/src/librustc_typeck/check/writeback.rs +++ b/src/librustc_typeck/check/writeback.rs @@ -479,10 +479,12 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { let mut skip_add = false; if let ty::Opaque(defin_ty_def_id, _substs) = definition_ty.kind { - if def_id == defin_ty_def_id { - debug!("Skipping adding concrete definition for opaque type {:?} {:?}", - opaque_defn, defin_ty_def_id); - skip_add = true; + if let hir::OpaqueTyOrigin::TypeAlias = opaque_defn.origin { + if def_id == defin_ty_def_id { + debug!("Skipping adding concrete definition for opaque type {:?} {:?}", + opaque_defn, defin_ty_def_id); + skip_add = true; + } } } diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs new file mode 100644 index 0000000000000..b22d216553389 --- /dev/null +++ b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs @@ -0,0 +1,7 @@ +// Test that an impl trait type that expands to itself is an error. + +fn test() -> impl Sized { //~ ERROR E0720 + test() +} + +fn main() {} diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type-direct.stderr b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.stderr new file mode 100644 index 0000000000000..1b5dbd814a481 --- /dev/null +++ b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.stderr @@ -0,0 +1,11 @@ +error[E0720]: opaque type expands to a recursive type + --> $DIR/recursive-impl-trait-type-direct.rs:3:14 + | +LL | fn test() -> impl Sized { + | ^^^^^^^^^^ expands to a recursive type + | + = note: type resolves to itself + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0720`. diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type.rs b/src/test/ui/impl-trait/recursive-impl-trait-type-indirect.rs similarity index 100% rename from src/test/ui/impl-trait/recursive-impl-trait-type.rs rename to src/test/ui/impl-trait/recursive-impl-trait-type-indirect.rs diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type.stderr b/src/test/ui/impl-trait/recursive-impl-trait-type-indirect.stderr similarity index 76% rename from src/test/ui/impl-trait/recursive-impl-trait-type.stderr rename to src/test/ui/impl-trait/recursive-impl-trait-type-indirect.stderr index 324607117dc50..b7ba0d6ab177c 100644 --- a/src/test/ui/impl-trait/recursive-impl-trait-type.stderr +++ b/src/test/ui/impl-trait/recursive-impl-trait-type-indirect.stderr @@ -1,5 +1,5 @@ error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:6:22 + --> $DIR/recursive-impl-trait-type-indirect.rs:6:22 | LL | fn option(i: i32) -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -7,7 +7,7 @@ LL | fn option(i: i32) -> impl Sized { = note: expanded type is `std::option::Option<(impl Sized, i32)>` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:14:15 + --> $DIR/recursive-impl-trait-type-indirect.rs:14:15 | LL | fn tuple() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -15,7 +15,7 @@ LL | fn tuple() -> impl Sized { = note: expanded type is `(impl Sized,)` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:18:15 + --> $DIR/recursive-impl-trait-type-indirect.rs:18:15 | LL | fn array() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -23,7 +23,7 @@ LL | fn array() -> impl Sized { = note: expanded type is `[impl Sized; 1]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:22:13 + --> $DIR/recursive-impl-trait-type-indirect.rs:22:13 | LL | fn ptr() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -31,7 +31,7 @@ LL | fn ptr() -> impl Sized { = note: expanded type is `*const impl Sized` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:26:16 + --> $DIR/recursive-impl-trait-type-indirect.rs:26:16 | LL | fn fn_ptr() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -39,47 +39,47 @@ LL | fn fn_ptr() -> impl Sized { = note: expanded type is `fn() -> impl Sized` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:30:25 + --> $DIR/recursive-impl-trait-type-indirect.rs:30:25 | LL | fn closure_capture() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[closure@$DIR/recursive-impl-trait-type.rs:32:5: 32:19 x:impl Sized]` + = note: expanded type is `[closure@$DIR/recursive-impl-trait-type-indirect.rs:32:5: 32:19 x:impl Sized]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:35:29 + --> $DIR/recursive-impl-trait-type-indirect.rs:35:29 | LL | fn closure_ref_capture() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[closure@$DIR/recursive-impl-trait-type.rs:37:5: 37:20 x:impl Sized]` + = note: expanded type is `[closure@$DIR/recursive-impl-trait-type-indirect.rs:37:5: 37:20 x:impl Sized]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:40:21 + --> $DIR/recursive-impl-trait-type-indirect.rs:40:21 | LL | fn closure_sig() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[closure@$DIR/recursive-impl-trait-type.rs:41:5: 41:21]` + = note: expanded type is `[closure@$DIR/recursive-impl-trait-type-indirect.rs:41:5: 41:21]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:44:23 + --> $DIR/recursive-impl-trait-type-indirect.rs:44:23 | LL | fn generator_sig() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[closure@$DIR/recursive-impl-trait-type.rs:45:5: 45:23]` + = note: expanded type is `[closure@$DIR/recursive-impl-trait-type-indirect.rs:45:5: 45:23]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:48:27 + --> $DIR/recursive-impl-trait-type-indirect.rs:48:27 | LL | fn generator_capture() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[generator@$DIR/recursive-impl-trait-type.rs:50:5: 50:26 x:impl Sized {()}]` + = note: expanded type is `[generator@$DIR/recursive-impl-trait-type-indirect.rs:50:5: 50:26 x:impl Sized {()}]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:53:26 + --> $DIR/recursive-impl-trait-type-indirect.rs:53:26 | LL | fn substs_change() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -87,15 +87,15 @@ LL | fn substs_change() -> impl Sized { = note: expanded type is `(impl Sized,)` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:57:24 + --> $DIR/recursive-impl-trait-type-indirect.rs:57:24 | LL | fn generator_hold() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type | - = note: expanded type is `[generator@$DIR/recursive-impl-trait-type.rs:58:5: 62:6 {impl Sized, ()}]` + = note: expanded type is `[generator@$DIR/recursive-impl-trait-type-indirect.rs:58:5: 62:6 {impl Sized, ()}]` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:69:26 + --> $DIR/recursive-impl-trait-type-indirect.rs:69:26 | LL | fn mutual_recursion() -> impl Sync { | ^^^^^^^^^ expands to a recursive type @@ -103,7 +103,7 @@ LL | fn mutual_recursion() -> impl Sync { = note: type resolves to itself error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type.rs:73:28 + --> $DIR/recursive-impl-trait-type-indirect.rs:73:28 | LL | fn mutual_recursion_b() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type--through-non-recursize.rs b/src/test/ui/impl-trait/recursive-impl-trait-type-through-non-recursive.rs similarity index 100% rename from src/test/ui/impl-trait/recursive-impl-trait-type--through-non-recursize.rs rename to src/test/ui/impl-trait/recursive-impl-trait-type-through-non-recursive.rs diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type--through-non-recursize.stderr b/src/test/ui/impl-trait/recursive-impl-trait-type-through-non-recursive.stderr similarity index 77% rename from src/test/ui/impl-trait/recursive-impl-trait-type--through-non-recursize.stderr rename to src/test/ui/impl-trait/recursive-impl-trait-type-through-non-recursive.stderr index 7572c6c1bf057..73c12f6137d24 100644 --- a/src/test/ui/impl-trait/recursive-impl-trait-type--through-non-recursize.stderr +++ b/src/test/ui/impl-trait/recursive-impl-trait-type-through-non-recursive.stderr @@ -1,5 +1,5 @@ error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type--through-non-recursize.rs:7:22 + --> $DIR/recursive-impl-trait-type-through-non-recursive.rs:7:22 | LL | fn recursive_id() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -7,7 +7,7 @@ LL | fn recursive_id() -> impl Sized { = note: type resolves to itself error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type--through-non-recursize.rs:11:23 + --> $DIR/recursive-impl-trait-type-through-non-recursive.rs:11:23 | LL | fn recursive_id2() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -15,7 +15,7 @@ LL | fn recursive_id2() -> impl Sized { = note: type resolves to itself error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type--through-non-recursize.rs:17:24 + --> $DIR/recursive-impl-trait-type-through-non-recursive.rs:17:24 | LL | fn recursive_wrap() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type @@ -23,7 +23,7 @@ LL | fn recursive_wrap() -> impl Sized { = note: expanded type is `((impl Sized,),)` error[E0720]: opaque type expands to a recursive type - --> $DIR/recursive-impl-trait-type--through-non-recursize.rs:21:25 + --> $DIR/recursive-impl-trait-type-through-non-recursive.rs:21:25 | LL | fn recursive_wrap2() -> impl Sized { | ^^^^^^^^^^ expands to a recursive type From 0c05ed29fd26eb1a7cc5fa77c0fa41d940a78346 Mon Sep 17 00:00:00 2001 From: matthewjasper Date: Fri, 25 Oct 2019 18:50:40 +0100 Subject: [PATCH 02/16] Apply suggestions from code review Co-Authored-By: Mazdak Farrokhzad --- src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs index b22d216553389..2b4f5e0975ac3 100644 --- a/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs +++ b/src/test/ui/impl-trait/recursive-impl-trait-type-direct.rs @@ -1,6 +1,6 @@ -// Test that an impl trait type that expands to itself is an error. +// Test that an `impl Trait` type that expands to itself is an error. -fn test() -> impl Sized { //~ ERROR E0720 +fn test() -> impl Sized { //~ ERROR E0720 test() } From 402a8af1d59a573ebd40cd953a779013969ec313 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 25 Oct 2019 19:27:57 -0400 Subject: [PATCH 03/16] Remove lint callback from driver This is leftover from a restructuring of lint registration for drivers; it should now happen via the register_lints field on Config rather than this function. --- src/librustc_driver/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/librustc_driver/lib.rs b/src/librustc_driver/lib.rs index 15adf7e4add73..6e8bc11162f66 100644 --- a/src/librustc_driver/lib.rs +++ b/src/librustc_driver/lib.rs @@ -106,8 +106,6 @@ pub fn abort_on_err(result: Result, sess: &Session) -> T { pub trait Callbacks { /// Called before creating the compiler instance fn config(&mut self, _config: &mut interface::Config) {} - /// Called early during compilation to allow other drivers to easily register lints. - fn extra_lints(&mut self, _ls: &mut lint::LintStore) {} /// Called after parsing. Return value instructs the compiler whether to /// continue the compilation afterwards (defaults to `Compilation::Continue`) fn after_parsing(&mut self, _compiler: &interface::Compiler) -> Compilation { From b2d021aa2b7c701406c10a3110dd31f990368c97 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 14:30:31 -0700 Subject: [PATCH 04/16] Make `check_consts::Item` work on non-const fns This was originally only needed for validation, which is never run on non-const `fn`s. The new promotion pass wants to use it, however. --- .../transform/check_consts/mod.rs | 98 ++++++++++++++----- .../transform/check_consts/ops.rs | 33 +++---- .../transform/check_consts/qualifs.rs | 21 ++-- .../transform/check_consts/validation.rs | 71 +------------- src/librustc_mir/transform/promote_consts.rs | 2 +- 5 files changed, 105 insertions(+), 120 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/mod.rs b/src/librustc_mir/transform/check_consts/mod.rs index 0c643f462432e..61496ebb4425f 100644 --- a/src/librustc_mir/transform/check_consts/mod.rs +++ b/src/librustc_mir/transform/check_consts/mod.rs @@ -4,10 +4,12 @@ //! has interior mutability or needs to be dropped, as well as the visitor that emits errors when //! it finds operations that are invalid in a certain context. -use rustc::hir::def_id::DefId; +use rustc::hir::{self, def_id::DefId}; use rustc::mir; use rustc::ty::{self, TyCtxt}; +use std::fmt; + pub use self::qualifs::Qualif; pub mod ops; @@ -15,15 +17,14 @@ pub mod qualifs; mod resolver; pub mod validation; -/// Information about the item currently being validated, as well as a reference to the global +/// Information about the item currently being const-checked, as well as a reference to the global /// context. pub struct Item<'mir, 'tcx> { body: &'mir mir::Body<'tcx>, tcx: TyCtxt<'tcx>, def_id: DefId, param_env: ty::ParamEnv<'tcx>, - mode: validation::Mode, - for_promotion: bool, + const_kind: Option, } impl Item<'mir, 'tcx> { @@ -33,41 +34,88 @@ impl Item<'mir, 'tcx> { body: &'mir mir::Body<'tcx>, ) -> Self { let param_env = tcx.param_env(def_id); - let mode = validation::Mode::for_item(tcx, def_id) - .expect("const validation must only be run inside a const context"); + let const_kind = ConstKind::for_item(tcx, def_id); Item { body, tcx, def_id, param_env, - mode, - for_promotion: false, + const_kind, } } - // HACK(eddyb) this is to get around the panic for a runtime fn from `Item::new`. - // Also, it allows promoting `&mut []`. - pub fn for_promotion( - tcx: TyCtxt<'tcx>, - def_id: DefId, - body: &'mir mir::Body<'tcx>, - ) -> Self { - let param_env = tcx.param_env(def_id); - let mode = validation::Mode::for_item(tcx, def_id) - .unwrap_or(validation::Mode::ConstFn); + /// Returns the kind of const context this `Item` represents (`const`, `static`, etc.). + /// + /// Panics if this `Item` is not const. + pub fn const_kind(&self) -> ConstKind { + self.const_kind.expect("`const_kind` must not be called on a non-const fn") + } +} - Item { - body, - tcx, - def_id, - param_env, - mode, - for_promotion: true, +/// The kinds of items which require compile-time evaluation. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ConstKind { + /// A `static` item. + Static, + /// A `static mut` item. + StaticMut, + /// A `const fn` item. + ConstFn, + /// A `const` item or an anonymous constant (e.g. in array lengths). + Const, +} + +impl ConstKind { + /// Returns the validation mode for the item with the given `DefId`, or `None` if this item + /// does not require validation (e.g. a non-const `fn`). + pub fn for_item(tcx: TyCtxt<'tcx>, def_id: DefId) -> Option { + use hir::BodyOwnerKind as HirKind; + + let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap(); + + let mode = match tcx.hir().body_owner_kind(hir_id) { + HirKind::Closure => return None, + + HirKind::Fn if tcx.is_const_fn(def_id) => ConstKind::ConstFn, + HirKind::Fn => return None, + + HirKind::Const => ConstKind::Const, + + HirKind::Static(hir::MutImmutable) => ConstKind::Static, + HirKind::Static(hir::MutMutable) => ConstKind::StaticMut, + }; + + Some(mode) + } + + pub fn is_static(self) -> bool { + match self { + ConstKind::Static | ConstKind::StaticMut => true, + ConstKind::ConstFn | ConstKind::Const => false, + } + } + + /// Returns `true` if the value returned by this item must be `Sync`. + /// + /// This returns false for `StaticMut` since all accesses to one are `unsafe` anyway. + pub fn requires_sync(self) -> bool { + match self { + ConstKind::Static => true, + ConstKind::ConstFn | ConstKind::Const | ConstKind::StaticMut => false, } } } +impl fmt::Display for ConstKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self { + ConstKind::Const => write!(f, "constant"), + ConstKind::Static | ConstKind::StaticMut => write!(f, "static"), + ConstKind::ConstFn => write!(f, "constant function"), + } + } +} fn is_lang_panic_fn(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool { Some(def_id) == tcx.lang_items().panic_fn() || diff --git a/src/librustc_mir/transform/check_consts/ops.rs b/src/librustc_mir/transform/check_consts/ops.rs index f457b739949c1..4b374cff80930 100644 --- a/src/librustc_mir/transform/check_consts/ops.rs +++ b/src/librustc_mir/transform/check_consts/ops.rs @@ -8,8 +8,7 @@ use syntax::feature_gate::{emit_feature_err, GateIssue}; use syntax::symbol::sym; use syntax_pos::{Span, Symbol}; -use super::Item; -use super::validation::Mode; +use super::{ConstKind, Item}; /// An operation that is not *always* allowed in a const context. pub trait NonConstOp: std::fmt::Debug { @@ -36,7 +35,7 @@ pub trait NonConstOp: std::fmt::Debug { span, E0019, "{} contains unimplemented expression type", - item.mode + item.const_kind() ); if item.tcx.sess.teach(&err.get_code().unwrap()) { err.note("A function call isn't allowed in the const's initialization expression \ @@ -76,7 +75,7 @@ impl NonConstOp for FnCallNonConst { E0015, "calls in {}s are limited to constant functions, \ tuple structs and tuple variants", - item.mode, + item.const_kind(), ); err.emit(); } @@ -121,8 +120,8 @@ impl NonConstOp for HeapAllocation { fn emit_error(&self, item: &Item<'_, '_>, span: Span) { let mut err = struct_span_err!(item.tcx.sess, span, E0010, - "allocations are not allowed in {}s", item.mode); - err.span_label(span, format!("allocation not allowed in {}s", item.mode)); + "allocations are not allowed in {}s", item.const_kind()); + err.span_label(span, format!("allocation not allowed in {}s", item.const_kind())); if item.tcx.sess.teach(&err.get_code().unwrap()) { err.note( "The value of statics and constants must be known at compile time, \ @@ -146,7 +145,7 @@ impl NonConstOp for LiveDrop { struct_span_err!(item.tcx.sess, span, E0493, "destructors cannot be evaluated at compile-time") .span_label(span, format!("{}s cannot evaluate destructors", - item.mode)) + item.const_kind())) .emit(); } } @@ -163,9 +162,9 @@ impl NonConstOp for MutBorrow { if let BorrowKind::Mut { .. } = kind { let mut err = struct_span_err!(item.tcx.sess, span, E0017, "references in {}s may only refer \ - to immutable values", item.mode); + to immutable values", item.const_kind()); err.span_label(span, format!("{}s require immutable values", - item.mode)); + item.const_kind())); if item.tcx.sess.teach(&err.get_code().unwrap()) { err.note("References in statics and constants may only refer \ to immutable values.\n\n\ @@ -202,7 +201,7 @@ impl NonConstOp for Panic { sym::const_panic, span, GateIssue::Language, - &format!("panicking in {}s is unstable", item.mode), + &format!("panicking in {}s is unstable", item.const_kind()), ); } } @@ -220,7 +219,7 @@ impl NonConstOp for RawPtrComparison { sym::const_compare_raw_pointers, span, GateIssue::Language, - &format!("comparing raw pointers inside {}", item.mode), + &format!("comparing raw pointers inside {}", item.const_kind()), ); } } @@ -238,7 +237,7 @@ impl NonConstOp for RawPtrDeref { span, GateIssue::Language, &format!( "dereferencing raw pointers in {}s is unstable", - item.mode, + item.const_kind(), ), ); } @@ -257,7 +256,7 @@ impl NonConstOp for RawPtrToIntCast { span, GateIssue::Language, &format!( "casting pointers to integers in {}s is unstable", - item.mode, + item.const_kind(), ), ); } @@ -268,13 +267,13 @@ impl NonConstOp for RawPtrToIntCast { pub struct StaticAccess; impl NonConstOp for StaticAccess { fn is_allowed_in_item(&self, item: &Item<'_, '_>) -> bool { - item.mode.is_static() + item.const_kind().is_static() } fn emit_error(&self, item: &Item<'_, '_>, span: Span) { let mut err = struct_span_err!(item.tcx.sess, span, E0013, "{}s cannot refer to statics, use \ - a constant instead", item.mode); + a constant instead", item.const_kind()); if item.tcx.sess.teach(&err.get_code().unwrap()) { err.note( "Static and const variables can refer to other const variables. \ @@ -313,7 +312,7 @@ impl NonConstOp for Transmute { &item.tcx.sess.parse_sess, sym::const_transmute, span, GateIssue::Language, &format!("The use of std::mem::transmute() \ - is gated in {}s", item.mode)); + is gated in {}s", item.const_kind())); } } @@ -322,7 +321,7 @@ pub struct UnionAccess; impl NonConstOp for UnionAccess { fn is_allowed_in_item(&self, item: &Item<'_, '_>) -> bool { // Union accesses are stable in all contexts except `const fn`. - item.mode != Mode::ConstFn || Self::feature_gate(item.tcx).unwrap() + item.const_kind() != ConstKind::ConstFn || Self::feature_gate(item.tcx).unwrap() } fn feature_gate(tcx: TyCtxt<'_>) -> Option { diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index e666dd9571f14..840ad30301607 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -5,8 +5,7 @@ use rustc::mir::interpret::ConstValue; use rustc::ty::{self, Ty}; use syntax_pos::DUMMY_SP; -use super::Item as ConstCx; -use super::validation::Mode; +use super::{ConstKind, Item as ConstCx}; #[derive(Clone, Copy)] pub struct QualifSet(u8); @@ -236,13 +235,17 @@ impl Qualif for HasMutInterior { // mutably without consequences. match ty.kind { // Inside a `static mut`, &mut [...] is also allowed. - ty::Array(..) | ty::Slice(_) if cx.mode == Mode::StaticMut => {}, - - // FIXME(eddyb) the `cx.for_promotion` condition - // seems unnecessary, given that this is merely a ZST. - ty::Array(_, len) - if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) - && cx.for_promotion => {}, + | ty::Array(..) + | ty::Slice(_) + if cx.const_kind == Some(ConstKind::StaticMut) + => {}, + + // FIXME(eddyb): We only return false for `&mut []` outside a const + // context which seems unnecessary given that this is merely a ZST. + | ty::Array(_, len) + if len.try_eval_usize(cx.tcx, cx.param_env) == Some(0) + && cx.const_kind == None + => {}, _ => return true, } diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index fa74663973656..c145fb41c9695 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -1,10 +1,9 @@ //! The `Visitor` responsible for actually checking a `mir::Body` for invalid operations. -use rustc::hir::{self, def_id::DefId}; use rustc::mir::visit::{PlaceContext, Visitor, MutatingUseContext, NonMutatingUseContext}; use rustc::mir::*; use rustc::ty::cast::CastTy; -use rustc::ty::{self, TyCtxt}; +use rustc::ty; use rustc_index::bit_set::BitSet; use rustc_target::spec::abi::Abi; use syntax::symbol::sym; @@ -15,7 +14,7 @@ use std::fmt; use std::ops::Deref; use crate::dataflow as old_dataflow; -use super::{Item, Qualif, is_lang_panic_fn}; +use super::{ConstKind, Item, Qualif, is_lang_panic_fn}; use super::resolver::{FlowSensitiveResolver, IndirectlyMutableResults, QualifResolver}; use super::qualifs::{HasMutInterior, NeedsDrop}; use super::ops::{self, NonConstOp}; @@ -27,70 +26,6 @@ pub enum CheckOpResult { Allowed, } -/// What kind of item we are in. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum Mode { - /// A `static` item. - Static, - /// A `static mut` item. - StaticMut, - /// A `const fn` item. - ConstFn, - /// A `const` item or an anonymous constant (e.g. in array lengths). - Const, -} - -impl Mode { - /// Returns the validation mode for the item with the given `DefId`, or `None` if this item - /// does not require validation (e.g. a non-const `fn`). - pub fn for_item(tcx: TyCtxt<'tcx>, def_id: DefId) -> Option { - use hir::BodyOwnerKind as HirKind; - - let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap(); - - let mode = match tcx.hir().body_owner_kind(hir_id) { - HirKind::Closure => return None, - - HirKind::Fn if tcx.is_const_fn(def_id) => Mode::ConstFn, - HirKind::Fn => return None, - - HirKind::Const => Mode::Const, - - HirKind::Static(hir::MutImmutable) => Mode::Static, - HirKind::Static(hir::MutMutable) => Mode::StaticMut, - }; - - Some(mode) - } - - pub fn is_static(self) -> bool { - match self { - Mode::Static | Mode::StaticMut => true, - Mode::ConstFn | Mode::Const => false, - } - } - - /// Returns `true` if the value returned by this item must be `Sync`. - /// - /// This returns false for `StaticMut` since all accesses to one are `unsafe` anyway. - pub fn requires_sync(self) -> bool { - match self { - Mode::Static => true, - Mode::ConstFn | Mode::Const | Mode::StaticMut => false, - } - } -} - -impl fmt::Display for Mode { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { - Mode::Const => write!(f, "constant"), - Mode::Static | Mode::StaticMut => write!(f, "static"), - Mode::ConstFn => write!(f, "constant function"), - } - } -} - pub struct Qualifs<'a, 'mir, 'tcx> { has_mut_interior: FlowSensitiveResolver<'a, 'mir, 'tcx, HasMutInterior>, needs_drop: FlowSensitiveResolver<'a, 'mir, 'tcx, NeedsDrop>, @@ -343,7 +278,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { let is_thread_local = self.tcx.has_attr(*def_id, sym::thread_local); if is_thread_local { self.check_op(ops::ThreadLocalAccess); - } else if self.mode == Mode::Static && context.is_mutating_use() { + } else if self.const_kind() == ConstKind::Static && context.is_mutating_use() { // this is not strictly necessary as miri will also bail out // For interior mutability we can't really catch this statically as that // goes through raw pointers and intermediate temporaries, so miri has diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 7aaff5735f696..4bf1fc60fe7b5 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -730,7 +730,7 @@ pub fn validate_candidates( is_non_const_fn: false, temps, - const_cx: ConstCx::for_promotion(tcx, def_id, body), + const_cx: ConstCx::new(tcx, def_id, body), explicit: false, }; From 8a462ff24a306a2a70235710a6bd96b695bc0197 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 14:54:10 -0700 Subject: [PATCH 05/16] Make `Item` fields pub --- src/librustc_mir/transform/check_consts/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/mod.rs b/src/librustc_mir/transform/check_consts/mod.rs index 61496ebb4425f..427d53397d595 100644 --- a/src/librustc_mir/transform/check_consts/mod.rs +++ b/src/librustc_mir/transform/check_consts/mod.rs @@ -20,11 +20,11 @@ pub mod validation; /// Information about the item currently being const-checked, as well as a reference to the global /// context. pub struct Item<'mir, 'tcx> { - body: &'mir mir::Body<'tcx>, - tcx: TyCtxt<'tcx>, - def_id: DefId, - param_env: ty::ParamEnv<'tcx>, - const_kind: Option, + pub body: &'mir mir::Body<'tcx>, + pub tcx: TyCtxt<'tcx>, + pub def_id: DefId, + pub param_env: ty::ParamEnv<'tcx>, + pub const_kind: Option, } impl Item<'mir, 'tcx> { From 748bbf259a4835792a37558def86d5ef85a93de3 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 14:55:04 -0700 Subject: [PATCH 06/16] Deduplicate `promote_consts::Validator` and `check_consts::Item` --- src/librustc_mir/transform/promote_consts.rs | 78 ++++++++------------ 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 4bf1fc60fe7b5..a3d535188f407 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -12,7 +12,6 @@ //! initialization and can otherwise silence errors, if //! move analysis runs after promotion on broken MIR. -use rustc::hir; use rustc::hir::def_id::DefId; use rustc::mir::*; use rustc::mir::interpret::ConstValue; @@ -30,7 +29,7 @@ use rustc_target::spec::abi::Abi; use std::{iter, mem, usize}; -use crate::transform::check_consts::{qualifs, Item as ConstCx}; +use crate::transform::check_consts::{qualifs, Item, ConstKind}; /// State of a temporary during collection and promotion. #[derive(Copy, Clone, PartialEq, Eq, Debug)] @@ -224,18 +223,13 @@ pub fn collect_temps_and_candidates( (collector.temps, collector.candidates) } +/// Checks whether locals that appear in a promotion context (`Candidate`) are actually promotable. +/// +/// This wraps an `Item`, and has access to all fields of that `Item` via `Deref` coercion. struct Validator<'a, 'tcx> { - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - body: &'a Body<'tcx>, - is_static: bool, - is_static_mut: bool, - is_non_const_fn: bool, + item: Item<'a, 'tcx>, temps: &'a IndexVec, - // FIXME(eddyb) deduplicate the data in this vs other fields. - const_cx: ConstCx<'a, 'tcx>, - /// Explicit promotion happens e.g. for constant arguments declared via /// `rustc_args_required_const`. /// Implicit promotion has almost the same rules, except that disallows `const fn` @@ -245,6 +239,14 @@ struct Validator<'a, 'tcx> { explicit: bool, } +impl std::ops::Deref for Validator<'a, 'tcx> { + type Target = Item<'a, 'tcx>; + + fn deref(&self) -> &Self::Target { + &self.item + } +} + struct Unpromotable; impl<'tcx> Validator<'_, 'tcx> { @@ -317,13 +319,14 @@ impl<'tcx> Validator<'_, 'tcx> { if self.qualif_local::(base) { return Err(Unpromotable); } + if let BorrowKind::Mut { .. } = kind { let ty = place.ty(self.body, self.tcx).ty; // In theory, any zero-sized value could be borrowed // mutably without consequences. However, only &mut [] // is allowed right now, and only in functions. - if self.is_static_mut { + if self.const_kind == Some(ConstKind::StaticMut) { // Inside a `static mut`, &mut [...] is also allowed. match ty.kind { ty::Array(..) | ty::Slice(_) => {} @@ -333,7 +336,7 @@ impl<'tcx> Validator<'_, 'tcx> { // FIXME(eddyb) the `self.is_non_const_fn` condition // seems unnecessary, given that this is merely a ZST. match len.try_eval_usize(self.tcx, self.param_env) { - Some(0) if self.is_non_const_fn => {}, + Some(0) if self.const_kind.is_none() => {}, _ => return Err(Unpromotable), } } else { @@ -386,7 +389,7 @@ impl<'tcx> Validator<'_, 'tcx> { let statement = &self.body[loc.block].statements[loc.statement_index]; match &statement.kind { StatementKind::Assign(box(_, rhs)) => { - Q::in_rvalue(&self.const_cx, per_local, rhs) + Q::in_rvalue(&self.item, per_local, rhs) } _ => { span_bug!(statement.source_info.span, "{:?} is not an assignment", @@ -398,7 +401,7 @@ impl<'tcx> Validator<'_, 'tcx> { match &terminator.kind { TerminatorKind::Call { func, args, .. } => { let return_ty = self.body.local_decls[local].ty; - Q::in_call(&self.const_cx, per_local, func, args, return_ty) + Q::in_call(&self.item, per_local, func, args, return_ty) } kind => { span_bug!(terminator.source_info.span, "{:?} not promotable", kind); @@ -462,8 +465,8 @@ impl<'tcx> Validator<'_, 'tcx> { } => { // Only allow statics (not consts) to refer to other statics. // FIXME(eddyb) does this matter at all for promotion? - let allowed = self.is_static || self.is_static_mut; - if !allowed { + let is_static = self.const_kind.map_or(false, |k| k.is_static()); + if !is_static { return Err(Unpromotable); } @@ -490,7 +493,7 @@ impl<'tcx> Validator<'_, 'tcx> { } ProjectionElem::Field(..) => { - if self.is_non_const_fn { + if self.const_kind.is_none() { let base_ty = Place::ty_from(place.base, proj_base, self.body, self.tcx).ty; if let Some(def) = base_ty.ty_adt_def() { @@ -545,7 +548,7 @@ impl<'tcx> Validator<'_, 'tcx> { fn validate_rvalue(&self, rvalue: &Rvalue<'tcx>) -> Result<(), Unpromotable> { match *rvalue { - Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if self.is_non_const_fn => { + Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if self.const_kind.is_none() => { let operand_ty = operand.ty(self.body, self.tcx); let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast"); let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast"); @@ -559,7 +562,7 @@ impl<'tcx> Validator<'_, 'tcx> { } } - Rvalue::BinaryOp(op, ref lhs, _) if self.is_non_const_fn => { + Rvalue::BinaryOp(op, ref lhs, _) if self.const_kind.is_none() => { if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(self.body, self.tcx).kind { assert!(op == BinOp::Eq || op == BinOp::Ne || op == BinOp::Le || op == BinOp::Lt || @@ -600,17 +603,17 @@ impl<'tcx> Validator<'_, 'tcx> { // In theory, any zero-sized value could be borrowed // mutably without consequences. However, only &mut [] // is allowed right now, and only in functions. - if self.is_static_mut { + if self.const_kind == Some(ConstKind::StaticMut) { // Inside a `static mut`, &mut [...] is also allowed. match ty.kind { ty::Array(..) | ty::Slice(_) => {} _ => return Err(Unpromotable), } } else if let ty::Array(_, len) = ty.kind { - // FIXME(eddyb) the `self.is_non_const_fn` condition - // seems unnecessary, given that this is merely a ZST. + // FIXME(eddyb): We only return `Unpromotable` for `&mut []` inside a + // const context which seems unnecessary given that this is merely a ZST. match len.try_eval_usize(self.tcx, self.param_env) { - Some(0) if self.is_non_const_fn => {}, + Some(0) if self.const_kind.is_none() => {}, _ => return Err(Unpromotable), } } else { @@ -683,7 +686,7 @@ impl<'tcx> Validator<'_, 'tcx> { ) -> Result<(), Unpromotable> { let fn_ty = callee.ty(self.body, self.tcx); - if !self.explicit && self.is_non_const_fn { + if !self.explicit && self.const_kind.is_none() { if let ty::FnDef(def_id, _) = fn_ty.kind { // Never promote runtime `const fn` calls of // functions without `#[rustc_promotable]`. @@ -714,6 +717,7 @@ impl<'tcx> Validator<'_, 'tcx> { } } +// FIXME(eddyb) remove the differences for promotability in `static`, `const`, `const fn`. pub fn validate_candidates( tcx: TyCtxt<'tcx>, body: &Body<'tcx>, @@ -722,33 +726,11 @@ pub fn validate_candidates( candidates: &[Candidate], ) -> Vec { let mut validator = Validator { - tcx, - param_env: tcx.param_env(def_id), - body, - is_static: false, - is_static_mut: false, - is_non_const_fn: false, + item: Item::new(tcx, def_id, body), temps, - - const_cx: ConstCx::new(tcx, def_id, body), - explicit: false, }; - // FIXME(eddyb) remove the distinctions that make this necessary. - let id = tcx.hir().as_local_hir_id(def_id).unwrap(); - match tcx.hir().body_owner_kind(id) { - hir::BodyOwnerKind::Closure => validator.is_non_const_fn = true, - hir::BodyOwnerKind::Fn => { - if !tcx.is_const_fn(def_id) { - validator.is_non_const_fn = true; - } - }, - hir::BodyOwnerKind::Static(hir::MutImmutable) => validator.is_static = true, - hir::BodyOwnerKind::Static(hir::MutMutable) => validator.is_static_mut = true, - _ => {} - } - candidates.iter().copied().filter(|&candidate| { validator.explicit = match candidate { Candidate::Ref(_) | From 653865658d8ac4001d712fb26f382a84ed36f951 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 15:00:51 -0700 Subject: [PATCH 07/16] Use `is_lang_panic_fn` from `check_consts` in `promote_consts` --- src/librustc_mir/transform/check_consts/mod.rs | 3 ++- src/librustc_mir/transform/promote_consts.rs | 9 ++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/mod.rs b/src/librustc_mir/transform/check_consts/mod.rs index 427d53397d595..364e23ed8d0f9 100644 --- a/src/librustc_mir/transform/check_consts/mod.rs +++ b/src/librustc_mir/transform/check_consts/mod.rs @@ -117,7 +117,8 @@ impl fmt::Display for ConstKind { } } -fn is_lang_panic_fn(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool { +/// Returns `true` if this `DefId` points to one of the official `panic` lang items. +pub fn is_lang_panic_fn(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool { Some(def_id) == tcx.lang_items().panic_fn() || Some(def_id) == tcx.lang_items().begin_panic_fn() } diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index a3d535188f407..3af08090853a6 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -29,7 +29,7 @@ use rustc_target::spec::abi::Abi; use std::{iter, mem, usize}; -use crate::transform::check_consts::{qualifs, Item, ConstKind}; +use crate::transform::check_consts::{qualifs, Item, ConstKind, is_lang_panic_fn}; /// State of a temporary during collection and promotion. #[derive(Copy, Clone, PartialEq, Eq, Debug)] @@ -250,11 +250,6 @@ impl std::ops::Deref for Validator<'a, 'tcx> { struct Unpromotable; impl<'tcx> Validator<'_, 'tcx> { - fn is_const_panic_fn(&self, def_id: DefId) -> bool { - Some(def_id) == self.tcx.lang_items().panic_fn() || - Some(def_id) == self.tcx.lang_items().begin_panic_fn() - } - fn validate_candidate(&self, candidate: Candidate) -> Result<(), Unpromotable> { match candidate { Candidate::Ref(loc) => { @@ -700,7 +695,7 @@ impl<'tcx> Validator<'_, 'tcx> { ty::FnDef(def_id, _) => { self.tcx.is_const_fn(def_id) || self.tcx.is_unstable_const_fn(def_id).is_some() || - self.is_const_panic_fn(def_id) + is_lang_panic_fn(self.tcx, self.def_id) } _ => false, }; From b93cdbce36b1a42c77b5f3f721d7d64dec80f8f3 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 23 Oct 2019 12:10:08 -0700 Subject: [PATCH 08/16] Remove `QualifResolver` abstraction This is a relic from earlier attempts at dataflow-based const validation that attempted to do promotion at the same time. #63812 takes a different approach: `IsNotPromotable` is no longer a `Qualif` and is computed lazily instead of eagerly. As a result, there's no need for an eager `TempPromotionResolver`, and we can use the only implementer of `QualifResolver` directly instead of through a trait. --- .../transform/check_consts/resolver.rs | 153 +---------------- .../transform/check_consts/validation.rs | 162 +++++++++++------- src/librustc_mir/transform/qualify_consts.rs | 3 +- 3 files changed, 112 insertions(+), 206 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index d3f1c760724f1..8909ef7db683d 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -1,21 +1,18 @@ //! Propagate `Qualif`s between locals and query the results. //! -//! This also contains the dataflow analysis used to track `Qualif`s on complex control-flow -//! graphs. +//! This contains the dataflow analysis used to track `Qualif`s on complex control-flow graphs. use rustc::mir::visit::Visitor; use rustc::mir::{self, BasicBlock, Local, Location}; use rustc_index::bit_set::BitSet; -use std::cell::RefCell; use std::marker::PhantomData; use crate::dataflow::{self as old_dataflow, generic as dataflow}; use super::{Item, Qualif}; -use self::old_dataflow::IndirectlyMutableLocals; /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of -/// `FlowSensitiveAnalysis` as well as the logic underlying `TempPromotionResolver`. +/// `FlowSensitiveAnalysis`. /// /// This transfer does nothing when encountering an indirect assignment. Consumers should rely on /// the `IndirectlyMutableLocals` dataflow pass to see if a `Local` may have become qualified via @@ -147,145 +144,6 @@ where } } -/// Types that can compute the qualifs of each local at each location in a `mir::Body`. -/// -/// Code that wishes to use a `QualifResolver` must call `visit_{statement,terminator}` for each -/// statement or terminator, processing blocks in reverse post-order beginning from the -/// `START_BLOCK`. Calling code may optionally call `get` after visiting each statement or -/// terminator to query the qualification state immediately before that statement or terminator. -/// -/// These conditions are much more restrictive than woud be required by `FlowSensitiveResolver` -/// alone. This is to allow a linear, on-demand `TempPromotionResolver` that can operate -/// efficiently on simple CFGs. -pub trait QualifResolver { - /// Get the qualifs of each local at the last location visited. - /// - /// This takes `&mut self` so qualifs can be computed lazily. - fn get(&mut self) -> &BitSet; - - /// A convenience method for `self.get().contains(local)`. - fn contains(&mut self, local: Local) -> bool { - self.get().contains(local) - } - - /// Resets the resolver to the `START_BLOCK`. This allows a resolver to be reused - /// for multiple passes over a `mir::Body`. - fn reset(&mut self); -} - -pub type IndirectlyMutableResults<'mir, 'tcx> = - old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>; - -/// A resolver for qualifs that works on arbitrarily complex CFGs. -/// -/// As soon as a `Local` becomes writable through a reference (as determined by the -/// `IndirectlyMutableLocals` dataflow pass), we must assume that it takes on all other qualifs -/// possible for its type. This is because no effort is made to track qualifs across indirect -/// assignments (e.g. `*p = x` or calls to opaque functions). -/// -/// It is possible to be more precise here by waiting until an indirect assignment actually occurs -/// before marking a borrowed `Local` as qualified. -pub struct FlowSensitiveResolver<'a, 'mir, 'tcx, Q> -where - Q: Qualif, -{ - location: Location, - indirectly_mutable_locals: &'a RefCell>, - cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>, - qualifs_per_local: BitSet, - - /// The value of `Q::in_any_value_of_ty` for each local. - qualifs_in_any_value_of_ty: BitSet, -} - -impl FlowSensitiveResolver<'a, 'mir, 'tcx, Q> -where - Q: Qualif, -{ - pub fn new( - _: Q, - item: &'a Item<'mir, 'tcx>, - indirectly_mutable_locals: &'a RefCell>, - dead_unwinds: &BitSet, - ) -> Self { - let analysis = FlowSensitiveAnalysis { - item, - _qualif: PhantomData, - }; - let results = - dataflow::Engine::new(item.tcx, item.body, item.def_id, dead_unwinds, analysis) - .iterate_to_fixpoint(); - let cursor = dataflow::ResultsCursor::new(item.body, results); - - let mut qualifs_in_any_value_of_ty = BitSet::new_empty(item.body.local_decls.len()); - for (local, decl) in item.body.local_decls.iter_enumerated() { - if Q::in_any_value_of_ty(item, decl.ty) { - qualifs_in_any_value_of_ty.insert(local); - } - } - - FlowSensitiveResolver { - cursor, - indirectly_mutable_locals, - qualifs_per_local: BitSet::new_empty(item.body.local_decls.len()), - qualifs_in_any_value_of_ty, - location: Location { block: mir::START_BLOCK, statement_index: 0 }, - } - } -} - -impl Visitor<'tcx> for FlowSensitiveResolver<'_, '_, 'tcx, Q> -where - Q: Qualif -{ - fn visit_statement(&mut self, _: &mir::Statement<'tcx>, location: Location) { - self.location = location; - } - - fn visit_terminator(&mut self, _: &mir::Terminator<'tcx>, location: Location) { - self.location = location; - } -} - -impl QualifResolver for FlowSensitiveResolver<'_, '_, '_, Q> -where - Q: Qualif -{ - fn get(&mut self) -> &BitSet { - let mut indirectly_mutable_locals = self.indirectly_mutable_locals.borrow_mut(); - - indirectly_mutable_locals.seek(self.location); - self.cursor.seek_before(self.location); - - self.qualifs_per_local.overwrite(indirectly_mutable_locals.get()); - self.qualifs_per_local.union(self.cursor.get()); - self.qualifs_per_local.intersect(&self.qualifs_in_any_value_of_ty); - &self.qualifs_per_local - } - - fn contains(&mut self, local: Local) -> bool { - // No need to update the cursor if we know that `Local` cannot possibly be qualified. - if !self.qualifs_in_any_value_of_ty.contains(local) { - return false; - } - - // Otherwise, return `true` if this local is qualified or was indirectly mutable at any - // point before this statement. - self.cursor.seek_before(self.location); - if self.cursor.get().contains(local) { - return true; - } - - let mut indirectly_mutable_locals = self.indirectly_mutable_locals.borrow_mut(); - indirectly_mutable_locals.seek(self.location); - indirectly_mutable_locals.get().contains(local) - } - - fn reset(&mut self) { - self.location = Location { block: mir::START_BLOCK, statement_index: 0 }; - } -} - /// The dataflow analysis used to propagate qualifs on arbitrary CFGs. pub(super) struct FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q> { item: &'a Item<'mir, 'tcx>, @@ -296,6 +154,13 @@ impl<'a, 'mir, 'tcx, Q> FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q> where Q: Qualif, { + pub(super) fn new(_: Q, item: &'a Item<'mir, 'tcx>) -> Self { + FlowSensitiveAnalysis { + item, + _qualif: PhantomData, + } + } + fn transfer_function( &self, state: &'a mut BitSet, diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index c145fb41c9695..244d434a51eab 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -9,15 +9,15 @@ use rustc_target::spec::abi::Abi; use syntax::symbol::sym; use syntax_pos::Span; -use std::cell::RefCell; use std::fmt; use std::ops::Deref; -use crate::dataflow as old_dataflow; -use super::{ConstKind, Item, Qualif, is_lang_panic_fn}; -use super::resolver::{FlowSensitiveResolver, IndirectlyMutableResults, QualifResolver}; -use super::qualifs::{HasMutInterior, NeedsDrop}; +use crate::dataflow::{self as old_dataflow, generic as dataflow}; +use self::old_dataflow::IndirectlyMutableLocals; use super::ops::{self, NonConstOp}; +use super::qualifs::{HasMutInterior, NeedsDrop}; +use super::resolver::FlowSensitiveAnalysis; +use super::{ConstKind, Item, Qualif, is_lang_panic_fn}; #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum CheckOpResult { @@ -26,9 +26,75 @@ pub enum CheckOpResult { Allowed, } +pub type IndirectlyMutableResults<'mir, 'tcx> = + old_dataflow::DataflowResultsCursor<'mir, 'tcx, IndirectlyMutableLocals<'mir, 'tcx>>; + +struct QualifCursor<'a, 'mir, 'tcx, Q: Qualif> { + cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>, + in_any_value_of_ty: BitSet, +} + +impl QualifCursor<'a, 'mir, 'tcx, Q> { + pub fn new( + q: Q, + item: &'a Item<'mir, 'tcx>, + dead_unwinds: &BitSet, + ) -> Self { + let analysis = FlowSensitiveAnalysis::new(q, item); + let results = + dataflow::Engine::new(item.tcx, item.body, item.def_id, dead_unwinds, analysis) + .iterate_to_fixpoint(); + let cursor = dataflow::ResultsCursor::new(item.body, results); + + let mut in_any_value_of_ty = BitSet::new_empty(item.body.local_decls.len()); + for (local, decl) in item.body.local_decls.iter_enumerated() { + if Q::in_any_value_of_ty(item, decl.ty) { + in_any_value_of_ty.insert(local); + } + } + + QualifCursor { + cursor, + in_any_value_of_ty, + } + } +} + pub struct Qualifs<'a, 'mir, 'tcx> { - has_mut_interior: FlowSensitiveResolver<'a, 'mir, 'tcx, HasMutInterior>, - needs_drop: FlowSensitiveResolver<'a, 'mir, 'tcx, NeedsDrop>, + has_mut_interior: QualifCursor<'a, 'mir, 'tcx, HasMutInterior>, + needs_drop: QualifCursor<'a, 'mir, 'tcx, NeedsDrop>, + indirectly_mutable: IndirectlyMutableResults<'mir, 'tcx>, +} + +impl Qualifs<'a, 'mir, 'tcx> { + fn indirectly_mutable(&mut self, local: Local, location: Location) -> bool { + self.indirectly_mutable.seek(location); + self.indirectly_mutable.get().contains(local) + } + + /// Returns `true` if `local` is `NeedsDrop` at the given `Location`. + /// + /// Only updates the cursor if absolutely necessary + fn needs_drop_lazy_seek(&mut self, local: Local, location: Location) -> bool { + if !self.needs_drop.in_any_value_of_ty.contains(local) { + return false; + } + + self.needs_drop.cursor.seek_before(location); + self.needs_drop.cursor.get().contains(local) + || self.indirectly_mutable(local, location) + } + + /// Returns `true` if `local` is `HasMutInterior`, but requires the `has_mut_interior` and + /// `indirectly_mutable` cursors to be updated beforehand. + fn has_mut_interior_eager_seek(&self, local: Local) -> bool { + if !self.has_mut_interior.in_any_value_of_ty.contains(local) { + return false; + } + + self.has_mut_interior.cursor.get().contains(local) + || self.indirectly_mutable.get().contains(local) + } } pub struct Validator<'a, 'mir, 'tcx> { @@ -63,53 +129,43 @@ impl Deref for Validator<'_, 'mir, 'tcx> { } } -pub fn compute_indirectly_mutable_locals<'mir, 'tcx>( - item: &Item<'mir, 'tcx>, -) -> RefCell> { - let dead_unwinds = BitSet::new_empty(item.body.basic_blocks().len()); - - let indirectly_mutable_locals = old_dataflow::do_dataflow( - item.tcx, - item.body, - item.def_id, - &item.tcx.get_attrs(item.def_id), - &dead_unwinds, - old_dataflow::IndirectlyMutableLocals::new(item.tcx, item.body, item.param_env), - |_, local| old_dataflow::DebugFormatted::new(&local), - ); - - let indirectly_mutable_locals = old_dataflow::DataflowResultsCursor::new( - indirectly_mutable_locals, - item.body, - ); - - RefCell::new(indirectly_mutable_locals) -} - impl Validator<'a, 'mir, 'tcx> { pub fn new( item: &'a Item<'mir, 'tcx>, - indirectly_mutable_locals: &'a RefCell>, ) -> Self { let dead_unwinds = BitSet::new_empty(item.body.basic_blocks().len()); - let needs_drop = FlowSensitiveResolver::new( + let needs_drop = QualifCursor::new( NeedsDrop, item, - indirectly_mutable_locals, &dead_unwinds, ); - let has_mut_interior = FlowSensitiveResolver::new( + let has_mut_interior = QualifCursor::new( HasMutInterior, item, - indirectly_mutable_locals, &dead_unwinds, ); + let indirectly_mutable = old_dataflow::do_dataflow( + item.tcx, + item.body, + item.def_id, + &item.tcx.get_attrs(item.def_id), + &dead_unwinds, + old_dataflow::IndirectlyMutableLocals::new(item.tcx, item.body, item.param_env), + |_, local| old_dataflow::DebugFormatted::new(&local), + ); + + let indirectly_mutable = old_dataflow::DataflowResultsCursor::new( + indirectly_mutable, + item.body, + ); + let qualifs = Qualifs { needs_drop, has_mut_interior, + indirectly_mutable, }; Validator { @@ -122,14 +178,6 @@ impl Validator<'a, 'mir, 'tcx> { } } - /// Resets the `QualifResolver`s used by this `Validator` and returns them so they can be - /// reused. - pub fn into_qualifs(mut self) -> Qualifs<'a, 'mir, 'tcx> { - self.qualifs.needs_drop.reset(); - self.qualifs.has_mut_interior.reset(); - self.qualifs - } - pub fn take_errors(&mut self) -> Vec<(Span, String)> { std::mem::replace(&mut self.errors, vec![]) } @@ -304,10 +352,16 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { // it depends on `HasMutInterior` being set for mutable borrows as well as values with // interior mutability. if let Rvalue::Ref(_, kind, ref borrowed_place) = *rvalue { - let rvalue_has_mut_interior = { - let has_mut_interior = self.qualifs.has_mut_interior.get(); - HasMutInterior::in_rvalue(&self.item, &|l| has_mut_interior.contains(l), rvalue) - }; + // FIXME: Change the `in_*` methods to take a `FnMut` so we don't have to manually seek + // the cursors beforehand. + self.qualifs.has_mut_interior.cursor.seek_before(location); + self.qualifs.indirectly_mutable.seek(location); + + let rvalue_has_mut_interior = HasMutInterior::in_rvalue( + &self.item, + &|local| self.qualifs.has_mut_interior_eager_seek(local), + rvalue, + ); if rvalue_has_mut_interior { let is_derived_from_illegal_borrow = match borrowed_place.as_local() { @@ -402,9 +456,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { trace!("visit_statement: statement={:?} location={:?}", statement, location); - self.qualifs.needs_drop.visit_statement(statement, location); - self.qualifs.has_mut_interior.visit_statement(statement, location); - match statement.kind { StatementKind::Assign(..) => { self.super_statement(statement, location); @@ -424,15 +475,6 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { } } - fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { - trace!("visit_terminator: terminator={:?} location={:?}", terminator, location); - - self.qualifs.needs_drop.visit_terminator(terminator, location); - self.qualifs.has_mut_interior.visit_terminator(terminator, location); - - self.super_terminator(terminator, location); - } - fn visit_terminator_kind(&mut self, kind: &TerminatorKind<'tcx>, location: Location) { trace!("visit_terminator_kind: kind={:?} location={:?}", kind, location); self.super_terminator_kind(kind, location); @@ -511,7 +553,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { let needs_drop = if let Some(local) = dropped_place.as_local() { // Use the span where the local was declared as the span of the drop error. err_span = self.body.local_decls[local].source_info.span; - self.qualifs.needs_drop.contains(local) + self.qualifs.needs_drop_lazy_seek(local, location) } else { true }; diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 21feeb1fad61d..2f77cd5ddf716 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -968,8 +968,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { } let item = new_checker::Item::new(self.tcx, self.def_id, self.body); - let mut_borrowed_locals = new_checker::validation::compute_indirectly_mutable_locals(&item); - let mut validator = new_checker::validation::Validator::new(&item, &mut_borrowed_locals); + let mut validator = new_checker::validation::Validator::new(&item); validator.suppress_errors = !use_new_validator; self.suppress_errors = use_new_validator; From 8f988bd92cf7d6d3e9be5310c18e472ba297e247 Mon Sep 17 00:00:00 2001 From: Ohad Ravid Date: Wed, 23 Oct 2019 22:22:13 +0200 Subject: [PATCH 09/16] Coherence should allow fundamental types to impl traits --- src/librustc/traits/coherence.rs | 14 ++++++++++---- ...pl-foreign-for-locally-defined-fundamental.rs | 16 ++++++++++++++++ ...n-for-locally-defined-fundamental[foreign].rs | 16 ++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental.rs create mode 100644 src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental[foreign].rs diff --git a/src/librustc/traits/coherence.rs b/src/librustc/traits/coherence.rs index 4696d4da58ec0..2734fce4ea55a 100644 --- a/src/librustc/traits/coherence.rs +++ b/src/librustc/traits/coherence.rs @@ -378,15 +378,21 @@ fn orphan_check_trait_ref<'tcx>( // Let Ti be the first such type. // - No uncovered type parameters P1..=Pn may appear in T0..Ti (excluding Ti) // - fn uncover_fundamental_ty(ty: Ty<'_>) -> Vec> { - if fundamental_ty(ty) { - ty.walk_shallow().flat_map(|ty| uncover_fundamental_ty(ty)).collect() + fn uncover_fundamental_ty<'a>( + tcx: TyCtxt<'_>, + ty: Ty<'a>, + in_crate: InCrate, + ) -> Vec> { + if fundamental_ty(ty) && !ty_is_local(tcx, ty, in_crate) { + ty.walk_shallow().flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate)).collect() } else { vec![ty] } } - for input_ty in trait_ref.input_types().flat_map(uncover_fundamental_ty) { + for input_ty in + trait_ref.input_types().flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate)) + { debug!("orphan_check_trait_ref: check ty `{:?}`", input_ty); if ty_is_local(tcx, input_ty, in_crate) { debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty); diff --git a/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental.rs b/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental.rs new file mode 100644 index 0000000000000..d461b5abd60ff --- /dev/null +++ b/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental.rs @@ -0,0 +1,16 @@ +#![feature(fundamental)] +#![feature(re_rebalance_coherence)] + +// compile-flags:--crate-name=test +// aux-build:coherence_lib.rs +// check-pass + +extern crate coherence_lib as lib; +use lib::*; + +#[fundamental] +struct Local; + +impl Remote for Local {} + +fn main() {} diff --git a/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental[foreign].rs b/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental[foreign].rs new file mode 100644 index 0000000000000..0a3d9e2e0e89c --- /dev/null +++ b/src/test/ui/coherence/impl-foreign-for-locally-defined-fundamental[foreign].rs @@ -0,0 +1,16 @@ +#![feature(fundamental)] +#![feature(re_rebalance_coherence)] + +// compile-flags:--crate-name=test +// aux-build:coherence_lib.rs +// check-pass + +extern crate coherence_lib as lib; +use lib::*; + +#[fundamental] +struct MyBox(T); + +impl Remote for MyBox {} + +fn main() {} From 6206a5a1b42072dfad10f923c68c66e552aa1c49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 18 Oct 2019 10:33:25 -0700 Subject: [PATCH 10/16] Use heuristics to suggest assignment When detecting a possible `=` -> `:` typo in a `let` binding, suggest assigning instead of setting the type. --- src/librustc_resolve/late.rs | 15 +++++++ src/librustc_resolve/late/diagnostics.rs | 25 +++++++++-- .../let-binding-init-expr-as-ty.rs | 11 +++++ .../let-binding-init-expr-as-ty.stderr | 41 +++++++++++++++++++ 4 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/suggestions/let-binding-init-expr-as-ty.rs create mode 100644 src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 136ab1f0444fa..048d4cefa574b 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -350,6 +350,9 @@ struct LateResolutionVisitor<'a, 'b> { /// Only used for better errors on `fn(): fn()`. current_type_ascription: Vec, + + /// Only used for better errors on `let : ;`. + current_let_binding: Option<(Span, Span)>, } /// Walks the whole crate in DFS order, visiting each item, resolving names as it goes. @@ -373,7 +376,18 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> { self.resolve_expr(expr, None); } fn visit_local(&mut self, local: &'tcx Local) { + debug!("visit_local {:?} {:?} {:?}", local, local.pat, local.pat.kind); + let val = match local { + Local { pat, ty: Some(ty), init: None, .. } => match pat.kind { + // We check for this to avoid tuple struct fields. + PatKind::Wild => None, + _ => Some((pat.span, ty.span)), + }, + _ => None, + }; + let original = replace(&mut self.current_let_binding, val); self.resolve_local(local); + self.current_let_binding = original; } fn visit_ty(&mut self, ty: &'tcx Ty) { match ty.kind { @@ -533,6 +547,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { current_function: None, unused_labels: Default::default(), current_type_ascription: Vec::new(), + current_let_binding: None, } } diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index 2721df4c68763..1912649f8b57c 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -72,10 +72,14 @@ impl<'a> LateResolutionVisitor<'a, '_> { let path_str = Segment::names_to_string(path); let item_str = path.last().unwrap().ident; let code = source.error_code(res.is_some()); - let (base_msg, fallback_label, base_span) = if let Some(res) = res { + let (base_msg, fallback_label, base_span, is_local) = if let Some(res) = res { (format!("expected {}, found {} `{}`", expected, res.descr(), path_str), format!("not a {}", expected), - span) + span, + match res { + Res::Local(_) => true, + _ => false, + }) } else { let item_span = path.last().unwrap().ident.span; let (mod_prefix, mod_str) = if path.len() == 1 { @@ -94,7 +98,8 @@ impl<'a> LateResolutionVisitor<'a, '_> { }; (format!("cannot find {} `{}` in {}{}", expected, item_str, mod_prefix, mod_str), format!("not found in {}", mod_str), - item_span) + item_span, + false) }; let code = DiagnosticId::Error(code.into()); @@ -257,6 +262,20 @@ impl<'a> LateResolutionVisitor<'a, '_> { if !levenshtein_worked { err.span_label(base_span, fallback_label); self.type_ascription_suggestion(&mut err, base_span); + if let Some(span) = self.current_let_binding.and_then(|(pat_sp, ty_sp)| { + if ty_sp.contains(base_span) && is_local { + Some(pat_sp.between(ty_sp)) + } else { + None + } + }) { + err.span_suggestion( + span, + "maybe you meant to assign a value instead of defining this let binding's type", + " = ".to_string(), + Applicability::MaybeIncorrect, + ); + } } (err, candidates) } diff --git a/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs b/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs new file mode 100644 index 0000000000000..4b572d6255bc6 --- /dev/null +++ b/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs @@ -0,0 +1,11 @@ +pub fn foo(num: i32) -> i32 { //~ ERROR mismatched types + let foo: i32::from_be(num); + //~^ ERROR expected type, found local variable `num` + //~| ERROR parenthesized type parameters may only be used with a `Fn` trait + //~| ERROR ambiguous associated type + //~| WARNING this was previously accepted by the compiler but is being phased out +} + +fn main() { + let _ = foo(42); +} diff --git a/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr b/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr new file mode 100644 index 0000000000000..b472c267987a9 --- /dev/null +++ b/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr @@ -0,0 +1,41 @@ +error[E0573]: expected type, found local variable `num` + --> $DIR/let-binding-init-expr-as-ty.rs:2:27 + | +LL | let foo: i32::from_be(num); + | ^^^ not a type +help: maybe you meant to assign a value instead of defining this let binding's type + | +LL | let foo = i32::from_be(num); + | ^ + +error: parenthesized type parameters may only be used with a `Fn` trait + --> $DIR/let-binding-init-expr-as-ty.rs:2:19 + | +LL | let foo: i32::from_be(num); + | ^^^^^^^^^^^^ + | + = note: `#[deny(parenthesized_params_in_types_and_modules)]` on by default + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #42238 + +error[E0223]: ambiguous associated type + --> $DIR/let-binding-init-expr-as-ty.rs:2:14 + | +LL | let foo: i32::from_be(num); + | ^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `::from_be` + +error[E0308]: mismatched types + --> $DIR/let-binding-init-expr-as-ty.rs:1:25 + | +LL | pub fn foo(num: i32) -> i32 { + | --- ^^^ expected i32, found () + | | + | implicitly returns `()` as its body has no tail or `return` expression + | + = note: expected type `i32` + found type `()` + +error: aborting due to 4 previous errors + +Some errors have detailed explanations: E0223, E0308, E0573. +For more information about an error, try `rustc --explain E0223`. From 55e4e2d52e563a02dc43804abdcc74b98976387b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 18 Oct 2019 10:37:28 -0700 Subject: [PATCH 11/16] Remove unnecessary error in test --- .../ui/suggestions/let-binding-init-expr-as-ty.rs | 3 ++- .../let-binding-init-expr-as-ty.stderr | 15 ++------------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs b/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs index 4b572d6255bc6..beea951a18a29 100644 --- a/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs +++ b/src/test/ui/suggestions/let-binding-init-expr-as-ty.rs @@ -1,9 +1,10 @@ -pub fn foo(num: i32) -> i32 { //~ ERROR mismatched types +pub fn foo(num: i32) -> i32 { let foo: i32::from_be(num); //~^ ERROR expected type, found local variable `num` //~| ERROR parenthesized type parameters may only be used with a `Fn` trait //~| ERROR ambiguous associated type //~| WARNING this was previously accepted by the compiler but is being phased out + foo } fn main() { diff --git a/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr b/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr index b472c267987a9..f4b0a38a105be 100644 --- a/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr +++ b/src/test/ui/suggestions/let-binding-init-expr-as-ty.stderr @@ -24,18 +24,7 @@ error[E0223]: ambiguous associated type LL | let foo: i32::from_be(num); | ^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `::from_be` -error[E0308]: mismatched types - --> $DIR/let-binding-init-expr-as-ty.rs:1:25 - | -LL | pub fn foo(num: i32) -> i32 { - | --- ^^^ expected i32, found () - | | - | implicitly returns `()` as its body has no tail or `return` expression - | - = note: expected type `i32` - found type `()` - -error: aborting due to 4 previous errors +error: aborting due to 3 previous errors -Some errors have detailed explanations: E0223, E0308, E0573. +Some errors have detailed explanations: E0223, E0573. For more information about an error, try `rustc --explain E0223`. From 93bb780e383ff22851ccda80f761d69314a7f1fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 19 Oct 2019 10:13:56 -0700 Subject: [PATCH 12/16] review comments and tweaks --- src/librustc_resolve/late.rs | 110 ++++++++++-------- src/librustc_resolve/late/diagnostics.rs | 48 +++++--- src/libsyntax/parse/parser/stmt.rs | 2 +- src/test/ui/privacy/privacy-ns2.rs | 1 + src/test/ui/privacy/privacy-ns2.stderr | 28 ++++- .../let-binding-init-expr-as-ty.stderr | 8 +- 6 files changed, 119 insertions(+), 78 deletions(-) diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 048d4cefa574b..3400b128e0444 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -316,22 +316,7 @@ impl<'a> PathSource<'a> { } } -struct LateResolutionVisitor<'a, 'b> { - r: &'b mut Resolver<'a>, - - /// The module that represents the current item scope. - parent_scope: ParentScope<'a>, - - /// The current set of local scopes for types and values. - /// FIXME #4948: Reuse ribs to avoid allocation. - ribs: PerNS>>, - - /// The current set of local scopes, for labels. - label_ribs: Vec>, - - /// The trait that the current context can refer to. - current_trait_ref: Option<(Module<'a>, TraitRef)>, - +struct DiagnosticMetadata { /// The current trait's associated types' ident, used for diagnostic suggestions. current_trait_assoc_types: Vec, @@ -352,7 +337,27 @@ struct LateResolutionVisitor<'a, 'b> { current_type_ascription: Vec, /// Only used for better errors on `let : ;`. - current_let_binding: Option<(Span, Span)>, + current_let_binding: Option<(Span, Option, Option)>, +} + +struct LateResolutionVisitor<'a, 'b> { + r: &'b mut Resolver<'a>, + + /// The module that represents the current item scope. + parent_scope: ParentScope<'a>, + + /// The current set of local scopes for types and values. + /// FIXME #4948: Reuse ribs to avoid allocation. + ribs: PerNS>>, + + /// The current set of local scopes, for labels. + label_ribs: Vec>, + + /// The trait that the current context can refer to. + current_trait_ref: Option<(Module<'a>, TraitRef)>, + + /// Fields used to add information to diagnostic errors. + diagnostic_metadata: DiagnosticMetadata, } /// Walks the whole crate in DFS order, visiting each item, resolving names as it goes. @@ -376,18 +381,18 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> { self.resolve_expr(expr, None); } fn visit_local(&mut self, local: &'tcx Local) { - debug!("visit_local {:?} {:?} {:?}", local, local.pat, local.pat.kind); - let val = match local { - Local { pat, ty: Some(ty), init: None, .. } => match pat.kind { - // We check for this to avoid tuple struct fields. - PatKind::Wild => None, - _ => Some((pat.span, ty.span)), - }, - _ => None, + let local_spans = match local.pat.kind { + // We check for this to avoid tuple struct fields. + PatKind::Wild => None, + _ => Some(( + local.pat.span, + local.ty.as_ref().map(|ty| ty.span), + local.init.as_ref().map(|init| init.span), + )), }; - let original = replace(&mut self.current_let_binding, val); + let original = replace(&mut self.diagnostic_metadata.current_let_binding, local_spans); self.resolve_local(local); - self.current_let_binding = original; + self.diagnostic_metadata.current_let_binding = original; } fn visit_ty(&mut self, ty: &'tcx Ty) { match ty.kind { @@ -429,7 +434,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> { } } fn visit_fn(&mut self, fn_kind: FnKind<'tcx>, declaration: &'tcx FnDecl, sp: Span, _: NodeId) { - let previous_value = replace(&mut self.current_function, Some(sp)); + let previous_value = replace(&mut self.diagnostic_metadata.current_function, Some(sp)); debug!("(resolving function) entering function"); let rib_kind = match fn_kind { FnKind::ItemFn(..) => FnItemRibKind, @@ -455,7 +460,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> { debug!("(resolving function) leaving function"); }) }); - self.current_function = previous_value; + self.diagnostic_metadata.current_function = previous_value; } fn visit_generics(&mut self, generics: &'tcx Generics) { @@ -489,7 +494,8 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> { // (We however cannot ban `Self` for defaults on *all* generic // lists; e.g. trait generics can usefully refer to `Self`, // such as in the case of `trait Add`.) - if self.current_self_item.is_some() { // (`Some` if + only if we are in ADT's generics.) + if self.diagnostic_metadata.current_self_item.is_some() { + // (`Some` if + only if we are in ADT's generics.) default_ban_rib.bindings.insert(Ident::with_dummy_span(kw::SelfUpper), Res::Err); } @@ -541,13 +547,15 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { }, label_ribs: Vec::new(), current_trait_ref: None, - current_trait_assoc_types: Vec::new(), - current_self_type: None, - current_self_item: None, - current_function: None, - unused_labels: Default::default(), - current_type_ascription: Vec::new(), - current_let_binding: None, + diagnostic_metadata: DiagnosticMetadata { + current_trait_assoc_types: Vec::new(), + current_self_type: None, + current_self_item: None, + current_function: None, + unused_labels: Default::default(), + current_type_ascription: Vec::new(), + current_let_binding: None, + } } } @@ -907,16 +915,22 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { fn with_current_self_type(&mut self, self_type: &Ty, f: impl FnOnce(&mut Self) -> T) -> T { // Handle nested impls (inside fn bodies) - let previous_value = replace(&mut self.current_self_type, Some(self_type.clone())); + let previous_value = replace( + &mut self.diagnostic_metadata.current_self_type, + Some(self_type.clone()), + ); let result = f(self); - self.current_self_type = previous_value; + self.diagnostic_metadata.current_self_type = previous_value; result } fn with_current_self_item(&mut self, self_item: &Item, f: impl FnOnce(&mut Self) -> T) -> T { - let previous_value = replace(&mut self.current_self_item, Some(self_item.id)); + let previous_value = replace( + &mut self.diagnostic_metadata.current_self_item, + Some(self_item.id), + ); let result = f(self); - self.current_self_item = previous_value; + self.diagnostic_metadata.current_self_item = previous_value; result } @@ -927,14 +941,14 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { f: impl FnOnce(&mut Self) -> T, ) -> T { let trait_assoc_types = replace( - &mut self.current_trait_assoc_types, + &mut self.diagnostic_metadata.current_trait_assoc_types, trait_items.iter().filter_map(|item| match &item.kind { TraitItemKind::Type(bounds, _) if bounds.len() == 0 => Some(item.ident), _ => None, }).collect(), ); let result = f(self); - self.current_trait_assoc_types = trait_assoc_types; + self.diagnostic_metadata.current_trait_assoc_types = trait_assoc_types; result } @@ -1761,7 +1775,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { fn with_resolved_label(&mut self, label: Option