From b2b8cba3e625bf0800e2de29cba06a472521b0a4 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 7 Aug 2017 20:50:34 +0000 Subject: [PATCH 1/9] make `for_all_relevant_impls` O(1) again A change in #41911 had made `for_all_relevant_impls` do a linear scan over all impls, instead of using an HashMap. Use an HashMap again to avoid quadratic blowup when there is a large number of structs with impls. I think this fixes #43141 completely, but I want better measurements in order to be sure. As a perf patch, please don't roll this up. --- src/librustc/traits/error_reporting.rs | 9 +- src/librustc/traits/select.rs | 6 +- src/librustc/traits/specialize/mod.rs | 3 +- src/librustc/ty/maps.rs | 15 +- src/librustc/ty/mod.rs | 2 - src/librustc/ty/trait_def.rs | 168 +++++++------------ src/librustc/ty/util.rs | 2 +- src/librustc_lint/builtin.rs | 3 +- src/librustc_mir/transform/qualify_consts.rs | 3 +- src/librustc_typeck/check/method/probe.rs | 4 +- 10 files changed, 77 insertions(+), 138 deletions(-) diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index c02d1394f6bb5..65c0a9f8ffd58 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -278,8 +278,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { let mut self_match_impls = vec![]; let mut fuzzy_match_impls = vec![]; - self.tcx.trait_def(trait_ref.def_id) - .for_each_relevant_impl(self.tcx, trait_self_ty, |def_id| { + self.tcx.for_each_relevant_impl( + trait_ref.def_id, trait_self_ty, |def_id| { let impl_substs = self.fresh_substs_for_item(obligation.cause.span, def_id); let impl_trait_ref = tcx .impl_trait_ref(def_id) @@ -396,10 +396,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { trait_ref.skip_binder().self_ty(), true); let mut impl_candidates = Vec::new(); - let trait_def = self.tcx.trait_def(trait_ref.def_id()); match simp { - Some(simp) => trait_def.for_each_impl(self.tcx, |def_id| { + Some(simp) => self.tcx.for_each_impl(trait_ref.def_id(), |def_id| { let imp = self.tcx.impl_trait_ref(def_id).unwrap(); let imp_simp = fast_reject::simplify_type(self.tcx, imp.self_ty(), @@ -411,7 +410,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } impl_candidates.push(imp); }), - None => trait_def.for_each_impl(self.tcx, |def_id| { + None => self.tcx.for_each_impl(trait_ref.def_id(), |def_id| { impl_candidates.push( self.tcx.impl_trait_ref(def_id).unwrap()); }) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 884f6bdb70b5c..f046cad82f38a 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -1430,10 +1430,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { { debug!("assemble_candidates_from_impls(obligation={:?})", obligation); - let def = self.tcx().trait_def(obligation.predicate.def_id()); - - def.for_each_relevant_impl( - self.tcx(), + self.tcx().for_each_relevant_impl( + obligation.predicate.def_id(), obligation.predicate.0.trait_ref.self_ty(), |impl_def_id| { self.probe(|this, snapshot| { /* [1] */ diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index 18734e2dbc3f1..36b2027a3d356 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -300,7 +300,8 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx -> Rc { let mut sg = specialization_graph::Graph::new(); - let mut trait_impls: Vec = tcx.trait_impls_of(trait_id).iter().collect(); + let mut trait_impls = Vec::new(); + tcx.for_each_impl(trait_id, |impl_did| trait_impls.push(impl_did)); // The coherence checking implementation seems to rely on impls being // iterated over (roughly) in definition order, so we are sorting by diff --git a/src/librustc/ty/maps.rs b/src/librustc/ty/maps.rs index e94308f351011..e803010383e24 100644 --- a/src/librustc/ty/maps.rs +++ b/src/librustc/ty/maps.rs @@ -470,12 +470,6 @@ impl<'tcx> QueryDescription for queries::trait_impls_of<'tcx> { } } -impl<'tcx> QueryDescription for queries::relevant_trait_impls_for<'tcx> { - fn describe(tcx: TyCtxt, (def_id, ty): (DefId, SimplifiedType)) -> String { - format!("relevant impls for: `({}, {:?})`", tcx.item_path_str(def_id), ty) - } -} - impl<'tcx> QueryDescription for queries::is_object_safe<'tcx> { fn describe(tcx: TyCtxt, def_id: DefId) -> String { format!("determine object safety of trait `{}`", tcx.item_path_str(def_id)) @@ -966,10 +960,7 @@ define_maps! { <'tcx> [] const_is_rvalue_promotable_to_static: ConstIsRvaluePromotableToStatic(DefId) -> bool, [] is_mir_available: IsMirAvailable(DefId) -> bool, - [] trait_impls_of: TraitImpls(DefId) -> ty::trait_def::TraitImpls, - // Note that TraitDef::for_each_relevant_impl() will do type simplication for you. - [] relevant_trait_impls_for: relevant_trait_impls_for((DefId, SimplifiedType)) - -> ty::trait_def::TraitImpls, + [] trait_impls_of: TraitImpls(DefId) -> Rc, [] specialization_graph_of: SpecializationGraph(DefId) -> Rc, [] is_object_safe: ObjectSafety(DefId) -> bool, @@ -1045,10 +1036,6 @@ fn crate_variances<'tcx>(_: CrateNum) -> DepConstructor<'tcx> { DepConstructor::CrateVariances } -fn relevant_trait_impls_for<'tcx>((def_id, t): (DefId, SimplifiedType)) -> DepConstructor<'tcx> { - DepConstructor::RelevantTraitImpls(def_id, t) -} - fn is_copy_dep_node<'tcx>(key: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> DepConstructor<'tcx> { let def_id = ty::item_path::characteristic_def_id_of_type(key.value) .unwrap_or(DefId::local(CRATE_DEF_INDEX)); diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 3f1302e72e974..8bd05f5d70730 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -2530,7 +2530,6 @@ pub fn provide(providers: &mut ty::maps::Providers) { param_env, trait_of_item, trait_impls_of: trait_def::trait_impls_of_provider, - relevant_trait_impls_for: trait_def::relevant_trait_impls_provider, ..*providers }; } @@ -2540,7 +2539,6 @@ pub fn provide_extern(providers: &mut ty::maps::Providers) { adt_sized_constraint, adt_dtorck_constraint, trait_impls_of: trait_def::trait_impls_of_provider, - relevant_trait_impls_for: trait_def::relevant_trait_impls_provider, param_env, ..*providers }; diff --git a/src/librustc/ty/trait_def.rs b/src/librustc/ty/trait_def.rs index ef6bce8a3d9d1..9990472c6b4ca 100644 --- a/src/librustc/ty/trait_def.rs +++ b/src/librustc/ty/trait_def.rs @@ -8,14 +8,17 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use hir; use hir::def_id::DefId; use hir::map::DefPathHash; use traits::specialization_graph; use ty::fast_reject; use ty::fold::TypeFoldable; use ty::{Ty, TyCtxt}; + +use rustc_data_structures::fx::FxHashMap; + use std::rc::Rc; -use hir; /// A trait's definition with type information. pub struct TraitDef { @@ -36,60 +39,12 @@ pub struct TraitDef { pub def_path_hash: DefPathHash, } -// We don't store the list of impls in a flat list because each cached list of -// `relevant_impls_for` we would then duplicate all blanket impls. By keeping -// blanket and non-blanket impls separate, we can share the list of blanket -// impls. -#[derive(Clone)] pub struct TraitImpls { - blanket_impls: Rc>, - non_blanket_impls: Rc>, + blanket_impls: Vec, + /// Impls indexed by their simplified self-type, for fast lookup. + non_blanket_impls: FxHashMap>, } -impl TraitImpls { - pub fn iter(&self) -> TraitImplsIter { - TraitImplsIter { - blanket_impls: self.blanket_impls.clone(), - non_blanket_impls: self.non_blanket_impls.clone(), - index: 0 - } - } -} - -#[derive(Clone)] -pub struct TraitImplsIter { - blanket_impls: Rc>, - non_blanket_impls: Rc>, - index: usize, -} - -impl Iterator for TraitImplsIter { - type Item = DefId; - - fn next(&mut self) -> Option { - if self.index < self.blanket_impls.len() { - let bi_index = self.index; - self.index += 1; - Some(self.blanket_impls[bi_index]) - } else { - let nbi_index = self.index - self.blanket_impls.len(); - if nbi_index < self.non_blanket_impls.len() { - self.index += 1; - Some(self.non_blanket_impls[nbi_index]) - } else { - None - } - } - } - - fn size_hint(&self) -> (usize, Option) { - let items_left = (self.blanket_impls.len() + self.non_blanket_impls.len()) - self.index; - (items_left, Some(items_left)) - } -} - -impl ExactSizeIterator for TraitImplsIter {} - impl<'a, 'gcx, 'tcx> TraitDef { pub fn new(def_id: DefId, unsafety: hir::Unsafety, @@ -111,20 +66,36 @@ impl<'a, 'gcx, 'tcx> TraitDef { -> specialization_graph::Ancestors { specialization_graph::ancestors(tcx, self.def_id, of_impl) } +} - pub fn for_each_impl(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>, mut f: F) { - for impl_def_id in tcx.trait_impls_of(self.def_id).iter() { +impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { + pub fn for_each_impl(self, def_id: DefId, mut f: F) { + let impls = self.trait_impls_of(def_id); + + for &impl_def_id in impls.blanket_impls.iter() { f(impl_def_id); } + + for v in impls.non_blanket_impls.values() { + for &impl_def_id in v { + f(impl_def_id); + } + } } /// Iterate over every impl that could possibly match the /// self-type `self_ty`. - pub fn for_each_relevant_impl(&self, - tcx: TyCtxt<'a, 'gcx, 'tcx>, + pub fn for_each_relevant_impl(self, + def_id: DefId, self_ty: Ty<'tcx>, mut f: F) { + let impls = self.trait_impls_of(def_id); + + for &impl_def_id in impls.blanket_impls.iter() { + f(impl_def_id); + } + // simplify_type(.., false) basically replaces type parameters and // projections with infer-variables. This is, of course, done on // the impl trait-ref when it is instantiated, but not on the @@ -137,15 +108,31 @@ impl<'a, 'gcx, 'tcx> TraitDef { // replace `S` with anything - this impl of course can't be // selected, and as there are hundreds of similar impls, // considering them would significantly harm performance. - let relevant_impls = if let Some(simplified_self_ty) = - fast_reject::simplify_type(tcx, self_ty, true) { - tcx.relevant_trait_impls_for((self.def_id, simplified_self_ty)) - } else { - tcx.trait_impls_of(self.def_id) - }; - for impl_def_id in relevant_impls.iter() { - f(impl_def_id); + // This depends on the set of all impls for the trait. That is + // unfortunate. When we get red-green recompilation, we would like + // to have a way of knowing whether the set of relevant impls + // changed. The most naive + // way would be to compute the Vec of relevant impls and see whether + // it differs between compilations. That shouldn't be too slow by + // itself - we do quite a bit of work for each relevant impl anyway. + // + // If we want to be faster, we could have separate queries for + // blanket and non-blanket impls, and compare them separately. + // + // I think we'll cross that bridge when we get to it. + if let Some(simp) = fast_reject::simplify_type(self, self_ty, true) { + if let Some(impls) = impls.non_blanket_impls.get(&simp) { + for &impl_def_id in impls { + f(impl_def_id); + } + } + } else { + for v in impls.non_blanket_impls.values() { + for &impl_def_id in v { + f(impl_def_id); + } + } } } } @@ -153,7 +140,7 @@ impl<'a, 'gcx, 'tcx> TraitDef { // Query provider for `trait_impls_of`. pub(super) fn trait_impls_of_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, trait_id: DefId) - -> TraitImpls { + -> Rc { let remote_impls = if trait_id.is_local() { // Traits defined in the current crate can't have impls in upstream // crates, so we don't bother querying the cstore. @@ -163,7 +150,7 @@ pub(super) fn trait_impls_of_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, }; let mut blanket_impls = Vec::new(); - let mut non_blanket_impls = Vec::new(); + let mut non_blanket_impls = FxHashMap(); let local_impls = tcx.hir .trait_impls(trait_id) @@ -176,47 +163,20 @@ pub(super) fn trait_impls_of_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, continue } - if fast_reject::simplify_type(tcx, impl_self_ty, false).is_some() { - non_blanket_impls.push(impl_def_id); + if let Some(simplified_self_ty) = + fast_reject::simplify_type(tcx, impl_self_ty, false) + { + non_blanket_impls + .entry(simplified_self_ty) + .or_insert(vec![]) + .push(impl_def_id); } else { blanket_impls.push(impl_def_id); } } - TraitImpls { - blanket_impls: Rc::new(blanket_impls), - non_blanket_impls: Rc::new(non_blanket_impls), - } -} - -// Query provider for `relevant_trait_impls_for`. -pub(super) fn relevant_trait_impls_provider<'a, 'tcx>( - tcx: TyCtxt<'a, 'tcx, 'tcx>, - (trait_id, self_ty): (DefId, fast_reject::SimplifiedType)) - -> TraitImpls -{ - let all_trait_impls = tcx.trait_impls_of(trait_id); - - let relevant: Vec = all_trait_impls - .non_blanket_impls - .iter() - .cloned() - .filter(|&impl_def_id| { - let impl_self_ty = tcx.type_of(impl_def_id); - let impl_simple_self_ty = fast_reject::simplify_type(tcx, - impl_self_ty, - false).unwrap(); - impl_simple_self_ty == self_ty - }) - .collect(); - - if all_trait_impls.non_blanket_impls.len() == relevant.len() { - // If we didn't filter anything out, re-use the existing vec. - all_trait_impls - } else { - TraitImpls { - blanket_impls: all_trait_impls.blanket_impls.clone(), - non_blanket_impls: Rc::new(relevant), - } - } + Rc::new(TraitImpls { + blanket_impls: blanket_impls, + non_blanket_impls: non_blanket_impls, + }) } diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index df4bbad3859f4..d1c7383154541 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -422,7 +422,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { let mut dtor_did = None; let ty = self.type_of(adt_did); - self.trait_def(drop_trait).for_each_relevant_impl(self, ty, |impl_did| { + self.for_each_relevant_impl(drop_trait, ty, |impl_did| { if let Some(item) = self.associated_items(impl_did).next() { if let Ok(()) = validate(self, impl_did) { dtor_did = Some(item.def_id); diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 02d68a41b4cc4..6726a6fa1ff69 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -542,9 +542,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingDebugImplementations { }; if self.impling_types.is_none() { - let debug_def = cx.tcx.trait_def(debug); let mut impls = NodeSet(); - debug_def.for_each_impl(cx.tcx, |d| { + cx.tcx.for_each_impl(debug, |d| { if let Some(ty_def) = cx.tcx.type_of(d).ty_to_def_id() { if let Some(node_id) = cx.tcx.hir.as_local_node_id(ty_def) { impls.insert(node_id); diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 68b687a2e6182..502c64a209b89 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -244,8 +244,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> { let mut span = None; self.tcx - .trait_def(drop_trait_id) - .for_each_relevant_impl(self.tcx, self.mir.return_ty, |impl_did| { + .for_each_relevant_impl(drop_trait_id, self.mir.return_ty, |impl_did| { self.tcx.hir .as_local_node_id(impl_did) .and_then(|impl_node_id| self.tcx.hir.find(impl_node_id)) diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index dfc5cd00b6eab..558cb1add9cfd 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -718,10 +718,8 @@ impl<'a, 'gcx, 'tcx> ProbeContext<'a, 'gcx, 'tcx> { import_id: Option, trait_def_id: DefId, item: ty::AssociatedItem) { - let trait_def = self.tcx.trait_def(trait_def_id); - // FIXME(arielb1): can we use for_each_relevant_impl here? - trait_def.for_each_impl(self.tcx, |impl_def_id| { + self.tcx.for_each_impl(trait_def_id, |impl_def_id| { debug!("assemble_extension_candidates_for_trait_impl: trait_def_id={:?} \ impl_def_id={:?}", trait_def_id, From f0cd69592b49821f2f1a1b9f5cf5416ef120b6a9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 12 Aug 2017 15:31:37 -0700 Subject: [PATCH 2/9] std: Respect formatting flags for str-like OsStr Historically many `Display` and `Debug` implementations for `OsStr`-like abstractions have gone through `String::from_utf8_lossy`, but this was updated in #42613 to use an internal `Utf8Lossy` abstraction instead. This had the unfortunate side effect of causing a regression (#43765) in code which relied on these `fmt` trait implementations respecting the various formatting flags specified. This commit opportunistically adds back interpretation of formatting trait flags in the "common case" where where `OsStr`-like "thing" is all valid utf-8 and can delegate to the formatting implementation for `str`. This doesn't entirely solve the regression as non-utf8 paths will format differently than they did before still (in that they will not respect formatting flags), but this should solve the regression for all "real world" use cases of paths and such. The door's also still open for handling these flags in the future! Closes #43765 --- src/libstd/path.rs | 6 ++++++ src/libstd/sys_common/wtf8.rs | 10 +++++++--- src/libstd_unicode/lib.rs | 1 - src/libstd_unicode/lossy.rs | 18 ++++++++++++++++-- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/libstd/path.rs b/src/libstd/path.rs index a1a1825ffbf8e..62ffe746f3b2b 100644 --- a/src/libstd/path.rs +++ b/src/libstd/path.rs @@ -3953,4 +3953,10 @@ mod tests { assert_eq!(path, path_buf); assert!(path_buf.into_os_string().capacity() >= 15); } + + #[test] + fn display_format_flags() { + assert_eq!(format!("a{:#<5}b", Path::new("").display()), "a#####b"); + assert_eq!(format!("a{:#<5}b", Path::new("a").display()), "aa####b"); + } } diff --git a/src/libstd/sys_common/wtf8.rs b/src/libstd/sys_common/wtf8.rs index 4e4a6e77d1242..b89a73cd28a09 100644 --- a/src/libstd/sys_common/wtf8.rs +++ b/src/libstd/sys_common/wtf8.rs @@ -452,10 +452,14 @@ impl fmt::Display for Wtf8 { pos = surrogate_pos + 3; }, None => { - formatter.write_str(unsafe { + let s = unsafe { str::from_utf8_unchecked(&wtf8_bytes[pos..]) - })?; - return Ok(()); + }; + if pos == 0 { + return s.fmt(formatter) + } else { + return formatter.write_str(s) + } } } } diff --git a/src/libstd_unicode/lib.rs b/src/libstd_unicode/lib.rs index 698210e83f3e3..d568baa2cd770 100644 --- a/src/libstd_unicode/lib.rs +++ b/src/libstd_unicode/lib.rs @@ -34,7 +34,6 @@ #![feature(core_char_ext)] #![feature(str_internals)] -#![feature(core_intrinsics)] #![feature(decode_utf8)] #![feature(fused)] #![feature(fn_traits)] diff --git a/src/libstd_unicode/lossy.rs b/src/libstd_unicode/lossy.rs index b914cbcf4b85f..253dcb6a15951 100644 --- a/src/libstd_unicode/lossy.rs +++ b/src/libstd_unicode/lossy.rs @@ -12,7 +12,7 @@ use core::str as core_str; use core::fmt; use core::fmt::Write; use char; -use core::intrinsics; +use core::mem; /// Lossy UTF-8 string. @@ -27,7 +27,7 @@ impl Utf8Lossy { } pub fn from_bytes(bytes: &[u8]) -> &Utf8Lossy { - unsafe { intrinsics::transmute(bytes) } + unsafe { mem::transmute(bytes) } } pub fn chunks(&self) -> Utf8LossyChunksIter { @@ -153,7 +153,21 @@ impl<'a> Iterator for Utf8LossyChunksIter<'a> { impl fmt::Display for Utf8Lossy { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + // If we're the empty string then our iterator won't actually yield + // anything, so perform the formatting manually + if self.bytes.len() == 0 { + return "".fmt(f) + } + for Utf8LossyChunk { valid, broken } in self.chunks() { + // If we successfully decoded the whole chunk as a valid string then + // we can return a direct formatting of the string which will also + // respect various formatting flags if possible. + if valid.len() == self.bytes.len() { + assert!(broken.is_empty()); + return valid.fmt(f) + } + f.write_str(valid)?; if !broken.is_empty() { f.write_char(char::REPLACEMENT_CHARACTER)?; From 5bb0cac84891de785ce803ca2bd36a55cd9775d9 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 13 Aug 2017 16:59:54 +0300 Subject: [PATCH 3/9] ast_validation: forbid "nonstandard" literal patterns Since #42886, macros can create "nonstandard" PatKind::Lit patterns, that contain path expressions instead of the usual literal expr. These can cause trouble, including ICEs. We *could* map these nonstandard patterns to PatKind::Path patterns during HIR lowering, but that would be much effort for little gain, and I think is too risky for beta. So let's just forbid them during AST validation. Fixes #43250. --- src/librustc_passes/ast_validation.rs | 27 +++++++++++++++++++++------ src/test/compile-fail/issue-43250.rs | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 src/test/compile-fail/issue-43250.rs diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index 72c7b92fe6e30..66736507abc9c 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/src/librustc_passes/ast_validation.rs @@ -94,10 +94,25 @@ impl<'a> AstValidator<'a> { } } - /// matches '-' lit | lit (cf. parser::Parser::parse_pat_literal_maybe_minus) - fn check_expr_within_pat(&self, expr: &Expr) { + /// matches '-' lit | lit (cf. parser::Parser::parse_pat_literal_maybe_minus), + /// or path for ranges. + /// + /// FIXME: do we want to allow expr -> pattern conversion to create path expressions? + /// That means making this work: + /// + /// ```rust,ignore (FIXME) + /// struct S; + /// macro_rules! m { + /// ($a:expr) => { + /// let $a = S; + /// } + /// } + /// m!(S); + /// ``` + fn check_expr_within_pat(&self, expr: &Expr, allow_paths: bool) { match expr.node { - ExprKind::Lit(..) | ExprKind::Path(..) => {} + ExprKind::Lit(..) => {} + ExprKind::Path(..) if allow_paths => {} ExprKind::Unary(UnOp::Neg, ref inner) if match inner.node { ExprKind::Lit(_) => true, _ => false } => {} _ => self.err_handler().span_err(expr.span, "arbitrary expressions aren't allowed \ @@ -340,11 +355,11 @@ impl<'a> Visitor<'a> for AstValidator<'a> { fn visit_pat(&mut self, pat: &'a Pat) { match pat.node { PatKind::Lit(ref expr) => { - self.check_expr_within_pat(expr); + self.check_expr_within_pat(expr, false); } PatKind::Range(ref start, ref end, _) => { - self.check_expr_within_pat(start); - self.check_expr_within_pat(end); + self.check_expr_within_pat(start, true); + self.check_expr_within_pat(end, true); } _ => {} } diff --git a/src/test/compile-fail/issue-43250.rs b/src/test/compile-fail/issue-43250.rs new file mode 100644 index 0000000000000..e1d34f339dc60 --- /dev/null +++ b/src/test/compile-fail/issue-43250.rs @@ -0,0 +1,23 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() { + let mut y; + const C: u32 = 0; + macro_rules! m { + ($a:expr) => { + let $a = 0; + } + } + m!(y); + //~^ ERROR arbitrary expressions aren't allowed in patterns + m!(C); + //~^ ERROR arbitrary expressions aren't allowed in patterns +} From bd20a035e46739c64ccd616ce8ad9926a86a269f Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 21 Aug 2017 14:11:57 +0300 Subject: [PATCH 4/9] register fn-ptr coercion obligations out of a snapshot Fixes #43923. --- src/librustc_typeck/check/coercion.rs | 3 +-- src/test/run-pass/issue-43923.rs | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 src/test/run-pass/issue-43923.rs diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index 968e893b9a00b..e4dea7d798332 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -807,8 +807,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let lub_ty = self.commit_if_ok(|_| { self.at(cause, self.param_env) .lub(prev_ty, new_ty) - .map(|ok| self.register_infer_ok_obligations(ok)) - }); + }).map(|ok| self.register_infer_ok_obligations(ok)); if lub_ty.is_ok() { // We have a LUB of prev_ty and new_ty, just return it. diff --git a/src/test/run-pass/issue-43923.rs b/src/test/run-pass/issue-43923.rs new file mode 100644 index 0000000000000..e1992e4fc5032 --- /dev/null +++ b/src/test/run-pass/issue-43923.rs @@ -0,0 +1,19 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +struct A { ptr: T } + +fn foo(x: &A<[T]>) {} + +fn main() { + let a = foo; + let b = A { ptr: [a, a, a] }; + a(&A { ptr: [()] }); +} From 4b6a82150bf3093c9dab12038b3e749cb8a6d1fd Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 21 Aug 2017 14:26:33 +0300 Subject: [PATCH 5/9] fix other cases of registering obligations in a snapshot No test cases for these ones, but they would all ICE if they ever run with a non-empty set of obligations. --- src/librustc_typeck/check/coercion.rs | 6 ++---- src/librustc_typeck/check/regionck.rs | 10 +++++----- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index e4dea7d798332..f18d52ba40dfb 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -883,8 +883,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { return self.commit_if_ok(|_| { self.at(cause, self.param_env) .lub(prev_ty, new_ty) - .map(|ok| self.register_infer_ok_obligations(ok)) - }); + }).map(|ok| self.register_infer_ok_obligations(ok)); } } @@ -897,8 +896,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { self.commit_if_ok(|_| { self.at(cause, self.param_env) .lub(prev_ty, new_ty) - .map(|ok| self.register_infer_ok_obligations(ok)) - }) + }).map(|ok| self.register_infer_ok_obligations(ok)) } } Ok(ok) => { diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs index 82207428efc4f..3437aec892a55 100644 --- a/src/librustc_typeck/check/regionck.rs +++ b/src/librustc_typeck/check/regionck.rs @@ -1805,12 +1805,12 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { // check whether this predicate applies to our current projection let cause = self.fcx.misc(span); match self.at(&cause, self.fcx.param_env).eq(outlives.0, ty) { - Ok(ok) => { - self.register_infer_ok_obligations(ok); - Ok(outlives.1) - } - Err(_) => { Err(()) } + Ok(ok) => Ok((ok, outlives.1)), + Err(_) => Err(()) } + }).map(|(ok, result)| { + self.register_infer_ok_obligations(ok); + result }); debug!("projection_bounds: region_result={:?}", From 76a1144cd8f7e195126ab20bc7cb218058b6d0a6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 22 Aug 2017 14:36:49 -0700 Subject: [PATCH 6/9] std: Mark allocation functions as nounwind This commit flags all allocation-related functions in liballoc as "this can't unwind" which should largely resolve the size-related issues found on #42808. The documentation on the trait was updated with such a restriction (they can't panic) as well as some other words about the relative instability about implementing a bullet-proof allocator. Closes #42808 --- src/liballoc/allocator.rs | 21 ++++++++++++++++++ src/liballoc/heap.rs | 11 +++++++++ src/librustc_trans/attributes.rs | 2 ++ src/test/codegen/dealloc-no-unwind.rs | 32 +++++++++++++++++++++++++++ 4 files changed, 66 insertions(+) create mode 100644 src/test/codegen/dealloc-no-unwind.rs diff --git a/src/liballoc/allocator.rs b/src/liballoc/allocator.rs index 23a005cbaf788..7c9c239560da2 100644 --- a/src/liballoc/allocator.rs +++ b/src/liballoc/allocator.rs @@ -452,6 +452,27 @@ impl fmt::Display for CannotReallocInPlace { /// * if a layout `k` fits a memory block (denoted by `ptr`) /// currently allocated via an allocator `a`, then it is legal to /// use that layout to deallocate it, i.e. `a.dealloc(ptr, k);`. +/// +/// # Unsafety +/// +/// The `Alloc` trait is an `unsafe` trait for a number of reasons, and +/// implementors must ensure that they adhere to these contracts: +/// +/// * Pointers returned from allocation functions must point to valid memory and +/// retain their validity until at least the instance of `Alloc` is dropped +/// itself. +/// +/// * Implementations of `Alloc` are not currently memory safe if they unwind. +/// This restriction may be lifted in the future, but currently an panic from +/// any of these functions may lead to memory unsafety. +/// +/// * `Layout` queries and calculations in general must be correct. Callers of +/// this trait are allowed to rely on the contracts defined on each method, +/// and implementors must ensure such contracts remain true. +/// +/// Note that this list may get tweaked over time as clarifications are made in +/// the future. Additionally global allocators may gain unique requirements for +/// how to safely implement one in the future as well. pub unsafe trait Alloc { // (Note: existing allocators have unspecified but well-defined diff --git a/src/liballoc/heap.rs b/src/liballoc/heap.rs index 1d959ac5bf6dc..feab1feadfb8c 100644 --- a/src/liballoc/heap.rs +++ b/src/liballoc/heap.rs @@ -27,23 +27,32 @@ pub mod __core { extern "Rust" { #[allocator] + #[allocator_nounwind] fn __rust_alloc(size: usize, align: usize, err: *mut u8) -> *mut u8; + #[cold] + #[allocator_nounwind] fn __rust_oom(err: *const u8) -> !; + #[allocator_nounwind] fn __rust_dealloc(ptr: *mut u8, size: usize, align: usize); + #[allocator_nounwind] fn __rust_usable_size(layout: *const u8, min: *mut usize, max: *mut usize); + #[allocator_nounwind] fn __rust_realloc(ptr: *mut u8, old_size: usize, old_align: usize, new_size: usize, new_align: usize, err: *mut u8) -> *mut u8; + #[allocator_nounwind] fn __rust_alloc_zeroed(size: usize, align: usize, err: *mut u8) -> *mut u8; + #[allocator_nounwind] fn __rust_alloc_excess(size: usize, align: usize, excess: *mut usize, err: *mut u8) -> *mut u8; + #[allocator_nounwind] fn __rust_realloc_excess(ptr: *mut u8, old_size: usize, old_align: usize, @@ -51,11 +60,13 @@ extern "Rust" { new_align: usize, excess: *mut usize, err: *mut u8) -> *mut u8; + #[allocator_nounwind] fn __rust_grow_in_place(ptr: *mut u8, old_size: usize, old_align: usize, new_size: usize, new_align: usize) -> u8; + #[allocator_nounwind] fn __rust_shrink_in_place(ptr: *mut u8, old_size: usize, old_align: usize, diff --git a/src/librustc_trans/attributes.rs b/src/librustc_trans/attributes.rs index cbad43066e4eb..839614f4a2964 100644 --- a/src/librustc_trans/attributes.rs +++ b/src/librustc_trans/attributes.rs @@ -119,6 +119,8 @@ pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRe llvm::AttributePlace::ReturnValue(), llfn); } else if attr.check_name("unwind") { unwind(llfn, true); + } else if attr.check_name("allocator_nounwind") { + unwind(llfn, false); } } if !target_features.is_empty() { diff --git a/src/test/codegen/dealloc-no-unwind.rs b/src/test/codegen/dealloc-no-unwind.rs new file mode 100644 index 0000000000000..551b66e103a11 --- /dev/null +++ b/src/test/codegen/dealloc-no-unwind.rs @@ -0,0 +1,32 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. +// +// no-system-llvm +// compile-flags: -O + +#![crate_type="lib"] + +struct A; + +impl Drop for A { + fn drop(&mut self) { + extern { fn foo(); } + unsafe { foo(); } + } +} + +#[no_mangle] +pub fn a(a: Box) { + // CHECK-LABEL: define void @a + // CHECK: call void @__rust_dealloc + // CHECK-NEXT: call void @foo + let _a = A; + drop(a); +} From c5beaac564f3a14b5a8390131706959d49499abc Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Aug 2017 07:54:56 -0700 Subject: [PATCH 7/9] Bump to beta .3 --- src/bootstrap/channel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/channel.rs b/src/bootstrap/channel.rs index aae02761708f7..16a7aa8b0aedb 100644 --- a/src/bootstrap/channel.rs +++ b/src/bootstrap/channel.rs @@ -28,7 +28,7 @@ pub const CFG_RELEASE_NUM: &str = "1.20.0"; // An optional number to put after the label, e.g. '.2' -> '-beta.2' // Be sure to make this starts with a dot to conform to semver pre-release // versions (section 9) -pub const CFG_PRERELEASE_VERSION: &str = ".2"; +pub const CFG_PRERELEASE_VERSION: &str = ".3"; pub struct GitInfo { inner: Option, From 7c076fc82a4197759044dd912c37c7fc9c33caf3 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 17 Aug 2017 11:44:28 -0700 Subject: [PATCH 8/9] Ensure that generic arguments don't end up in attribute paths. --- src/libsyntax/parse/parser.rs | 8 +++++++- src/test/compile-fail/issue-43424.rs | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 src/test/compile-fail/issue-43424.rs diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 90d9ae383a491..1a1f35505d020 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -1751,7 +1751,13 @@ impl<'a> Parser<'a> { fn parse_path_common(&mut self, mode: PathStyle, parse_generics: bool) -> PResult<'a, ast::Path> { - maybe_whole!(self, NtPath, |x| x); + maybe_whole!(self, NtPath, |path| { + if style == PathStyle::Mod && + path.segments.iter().any(|segment| segment.parameters.is_some()) { + self.diagnostic().span_err(path.span, "unexpected generic arguments in path"); + } + path + }); let lo = self.meta_var_span.unwrap_or(self.span); let is_global = self.eat(&token::ModSep); diff --git a/src/test/compile-fail/issue-43424.rs b/src/test/compile-fail/issue-43424.rs new file mode 100644 index 0000000000000..431fc8a5aa2b2 --- /dev/null +++ b/src/test/compile-fail/issue-43424.rs @@ -0,0 +1,22 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![allow(unused)] + +macro_rules! m { + ($attr_path: path) => { + #[$attr_path] + fn f() {} + } +} + +m!(inline); //~ ERROR: unexpected generic arguments in path + +fn main() {} From 0e9969c81c6df7b13b75638a7c3635d0a4515c36 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 23 Aug 2017 08:47:50 -0700 Subject: [PATCH 9/9] Fix tests and fallout --- src/libsyntax/parse/parser.rs | 2 +- src/test/compile-fail/import-ty-params.rs | 2 +- src/test/compile-fail/privacy/restricted/ty-params.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 1a1f35505d020..573e01eab2abf 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -1752,7 +1752,7 @@ impl<'a> Parser<'a> { -> PResult<'a, ast::Path> { maybe_whole!(self, NtPath, |path| { - if style == PathStyle::Mod && + if mode == PathStyle::Mod && path.segments.iter().any(|segment| segment.parameters.is_some()) { self.diagnostic().span_err(path.span, "unexpected generic arguments in path"); } diff --git a/src/test/compile-fail/import-ty-params.rs b/src/test/compile-fail/import-ty-params.rs index 7344f31535fba..a345523ded57d 100644 --- a/src/test/compile-fail/import-ty-params.rs +++ b/src/test/compile-fail/import-ty-params.rs @@ -20,6 +20,6 @@ macro_rules! import { ($p: path) => (use $p;); } -import! { a::b::c::S } //~ERROR type or lifetime parameters in import path +import! { a::b::c::S } //~ERROR unexpected generic arguments in path fn main() {} diff --git a/src/test/compile-fail/privacy/restricted/ty-params.rs b/src/test/compile-fail/privacy/restricted/ty-params.rs index c83a4e568528f..5708d31998435 100644 --- a/src/test/compile-fail/privacy/restricted/ty-params.rs +++ b/src/test/compile-fail/privacy/restricted/ty-params.rs @@ -13,7 +13,7 @@ macro_rules! m { } struct S(T); -m!{ S } //~ ERROR type or lifetime parameters in visibility path +m!{ S } //~ ERROR unexpected generic arguments in path //~^ ERROR expected module, found struct `S` fn main() {}