From f84967f1ae2a8c56faad2d9d882ae93b805abf70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Wed, 12 Jun 2019 14:39:12 +0200 Subject: [PATCH 01/12] Use sharded maps for queries --- src/librustc/ty/query/config.rs | 4 +-- src/librustc/ty/query/on_disk_cache.rs | 6 ++-- src/librustc/ty/query/plumbing.rs | 44 ++++++++++++++------------ 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/librustc/ty/query/config.rs b/src/librustc/ty/query/config.rs index 1cc083ea93c6c..91082c59ba05a 100644 --- a/src/librustc/ty/query/config.rs +++ b/src/librustc/ty/query/config.rs @@ -11,7 +11,7 @@ use crate::util::profiling::ProfileCategory; use std::borrow::Cow; use std::hash::Hash; use std::fmt::Debug; -use rustc_data_structures::sync::Lock; +use rustc_data_structures::sharded::Sharded; use rustc_data_structures::fingerprint::Fingerprint; use crate::ich::StableHashingContext; @@ -34,7 +34,7 @@ pub(crate) trait QueryAccessors<'tcx>: QueryConfig<'tcx> { fn query(key: Self::Key) -> Query<'tcx>; // Don't use this method to access query results, instead use the methods on TyCtxt - fn query_cache<'a>(tcx: TyCtxt<'tcx>) -> &'a Lock>; + fn query_cache<'a>(tcx: TyCtxt<'tcx>) -> &'a Sharded>; fn to_dep_node(tcx: TyCtxt<'tcx>, key: &Self::Key) -> DepNode; diff --git a/src/librustc/ty/query/on_disk_cache.rs b/src/librustc/ty/query/on_disk_cache.rs index 45bc89f5a84ab..2a9f1949d514d 100644 --- a/src/librustc/ty/query/on_disk_cache.rs +++ b/src/librustc/ty/query/on_disk_cache.rs @@ -1063,9 +1063,9 @@ where ::std::any::type_name::()); time_ext(tcx.sess.time_extended(), Some(tcx.sess), desc, || { - let map = Q::query_cache(tcx).borrow(); - assert!(map.active.is_empty()); - for (key, entry) in map.results.iter() { + let shards = Q::query_cache(tcx).lock_shards(); + assert!(shards.iter().all(|shard| shard.active.is_empty())); + for (key, entry) in shards.iter().flat_map(|shard| shard.results.iter()) { if Q::cache_on_disk(tcx, key.clone(), Some(&entry.value)) { let dep_node = SerializedDepNodeIndex::new(entry.index.index()); diff --git a/src/librustc/ty/query/plumbing.rs b/src/librustc/ty/query/plumbing.rs index ce9f67db59232..4dce55f589233 100644 --- a/src/librustc/ty/query/plumbing.rs +++ b/src/librustc/ty/query/plumbing.rs @@ -17,6 +17,7 @@ use errors::Diagnostic; use errors::FatalError; use rustc_data_structures::fx::{FxHashMap}; use rustc_data_structures::sync::{Lrc, Lock}; +use rustc_data_structures::sharded::Sharded; use rustc_data_structures::thin_vec::ThinVec; #[cfg(not(parallel_compiler))] use rustc_data_structures::cold_path; @@ -90,7 +91,7 @@ macro_rules! profq_query_msg { /// A type representing the responsibility to execute the job in the `job` field. /// This will poison the relevant query if dropped. pub(super) struct JobOwner<'a, 'tcx, Q: QueryDescription<'tcx>> { - cache: &'a Lock>, + cache: &'a Sharded>, key: Q::Key, job: Lrc>, } @@ -107,7 +108,7 @@ impl<'a, 'tcx, Q: QueryDescription<'tcx>> JobOwner<'a, 'tcx, Q> { pub(super) fn try_get(tcx: TyCtxt<'tcx>, span: Span, key: &Q::Key) -> TryGetJob<'a, 'tcx, Q> { let cache = Q::query_cache(tcx); loop { - let mut lock = cache.borrow_mut(); + let mut lock = cache.get_shard_by_value(key).lock(); if let Some(value) = lock.results.get(key) { profq_msg!(tcx, ProfileQueriesMsg::CacheHit); tcx.sess.profiler(|p| p.record_query_hit(Q::NAME)); @@ -191,7 +192,7 @@ impl<'a, 'tcx, Q: QueryDescription<'tcx>> JobOwner<'a, 'tcx, Q> { let value = QueryValue::new(result.clone(), dep_node_index); { - let mut lock = cache.borrow_mut(); + let mut lock = cache.get_shard_by_value(&key).lock(); lock.active.remove(&key); lock.results.insert(key, value); } @@ -215,7 +216,8 @@ impl<'a, 'tcx, Q: QueryDescription<'tcx>> Drop for JobOwner<'a, 'tcx, Q> { #[cold] fn drop(&mut self) { // Poison the query so jobs waiting on it panic - self.cache.borrow_mut().active.insert(self.key.clone(), QueryResult::Poisoned); + let shard = self.cache.get_shard_by_value(&self.key); + shard.lock().active.insert(self.key.clone(), QueryResult::Poisoned); // Also signal the completion of the job, so waiters // will continue execution self.job.signal_complete(); @@ -708,7 +710,7 @@ macro_rules! define_queries_inner { use std::mem; #[cfg(parallel_compiler)] use ty::query::job::QueryResult; - use rustc_data_structures::sync::Lock; + use rustc_data_structures::sharded::Sharded; use crate::{ rustc_data_structures::stable_hasher::HashStable, rustc_data_structures::stable_hasher::StableHasherResult, @@ -740,18 +742,17 @@ macro_rules! define_queries_inner { pub fn collect_active_jobs(&self) -> Vec>> { let mut jobs = Vec::new(); - // We use try_lock here since we are only called from the + // We use try_lock_shards here since we are only called from the // deadlock handler, and this shouldn't be locked. $( - jobs.extend( - self.$name.try_lock().unwrap().active.values().filter_map(|v| - if let QueryResult::Started(ref job) = *v { - Some(job.clone()) - } else { - None - } - ) - ); + let shards = self.$name.try_lock_shards().unwrap(); + jobs.extend(shards.iter().flat_map(|shard| shard.active.values().filter_map(|v| + if let QueryResult::Started(ref job) = *v { + Some(job.clone()) + } else { + None + } + ))); )* jobs @@ -773,26 +774,27 @@ macro_rules! define_queries_inner { fn stats<'tcx, Q: QueryConfig<'tcx>>( name: &'static str, - map: &QueryCache<'tcx, Q> + map: &Sharded>, ) -> QueryStats { + let map = map.lock_shards(); QueryStats { name, #[cfg(debug_assertions)] - cache_hits: map.cache_hits, + cache_hits: map.iter().map(|shard| shard.cache_hits).sum(), #[cfg(not(debug_assertions))] cache_hits: 0, key_size: mem::size_of::(), key_type: type_name::(), value_size: mem::size_of::(), value_type: type_name::(), - entry_count: map.results.len(), + entry_count: map.iter().map(|shard| shard.results.len()).sum(), } } $( queries.push(stats::>( stringify!($name), - &*self.$name.lock() + &self.$name, )); )* @@ -967,7 +969,7 @@ macro_rules! define_queries_inner { } #[inline(always)] - fn query_cache<'a>(tcx: TyCtxt<$tcx>) -> &'a Lock> { + fn query_cache<'a>(tcx: TyCtxt<$tcx>) -> &'a Sharded> { &tcx.queries.$name } @@ -1099,7 +1101,7 @@ macro_rules! define_queries_struct { providers: IndexVec>, fallback_extern_providers: Box>, - $($(#[$attr])* $name: Lock>>,)* + $($(#[$attr])* $name: Sharded>>,)* } }; } From aa72b1d3e3b946cd25fc622b8c5eb07645dc2a58 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Aug 2019 18:41:33 +0200 Subject: [PATCH 02/12] note about stack-allocated variables being allocated objects --- src/libcore/ptr/mod.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/libcore/ptr/mod.rs b/src/libcore/ptr/mod.rs index 0ec4dd47b1ff0..d871c12cd63dc 100644 --- a/src/libcore/ptr/mod.rs +++ b/src/libcore/ptr/mod.rs @@ -1120,7 +1120,8 @@ impl *const T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset, **in bytes**, cannot overflow an `isize`. /// @@ -1223,7 +1224,8 @@ impl *const T { /// Behavior: /// /// * Both the starting and other pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The distance between the pointers, **in bytes**, cannot overflow an `isize`. /// @@ -1338,7 +1340,8 @@ impl *const T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset, **in bytes**, cannot overflow an `isize`. /// @@ -1395,7 +1398,8 @@ impl *const T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset cannot exceed `isize::MAX` **bytes**. /// @@ -1755,7 +1759,8 @@ impl *mut T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset, **in bytes**, cannot overflow an `isize`. /// @@ -1901,7 +1906,8 @@ impl *mut T { /// Behavior: /// /// * Both the starting and other pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The distance between the pointers, **in bytes**, cannot overflow an `isize`. /// @@ -2005,7 +2011,8 @@ impl *mut T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset, **in bytes**, cannot overflow an `isize`. /// @@ -2062,7 +2069,8 @@ impl *mut T { /// Behavior: /// /// * Both the starting and resulting pointer must be either in bounds or one - /// byte past the end of the same allocated object. + /// byte past the end of the same allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. /// /// * The computed offset cannot exceed `isize::MAX` **bytes**. /// From 0dc9e2a56502c1138c6faf021888844905e64cf3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Aug 2019 18:55:56 +0200 Subject: [PATCH 03/12] improve wrapping_ docs --- src/libcore/ptr/mod.rs | 138 +++++++++++++++++++++++++++++++++-------- 1 file changed, 112 insertions(+), 26 deletions(-) diff --git a/src/libcore/ptr/mod.rs b/src/libcore/ptr/mod.rs index d871c12cd63dc..fed6ef53be3a4 100644 --- a/src/libcore/ptr/mod.rs +++ b/src/libcore/ptr/mod.rs @@ -1141,10 +1141,12 @@ impl *const T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_offset`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_offset`]: #method.wrapping_offset + /// /// # Examples /// /// Basic usage: @@ -1173,15 +1175,26 @@ impl *const T { /// /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). - /// In particular, the resulting pointer may *not* be used to access a - /// different allocated object than the one `self` points to. In other - /// words, `x.wrapping_offset(y.wrapping_offset_from(x))` is + /// + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// In other words, `x.wrapping_offset(y.wrapping_offset_from(x))` is /// *not* the same as `y`, and dereferencing it is undefined behavior /// unless `x` and `y` point into the same allocated object. /// - /// Always use `.offset(count)` instead when possible, because `offset` - /// allows the compiler to optimize better. If you need to cross object - /// boundaries, cast the pointer to an integer and do the arithmetic there. + /// Compared to [`offset`], this method basically delays the requirement of staying + /// within the same allocated object: [`offset`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_offset` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`offset`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`offset`]: #method.offset /// /// # Examples /// @@ -1361,10 +1374,12 @@ impl *const T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_add`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_add`]: #method.wrapping_add + /// /// # Examples /// /// Basic usage: @@ -1419,10 +1434,12 @@ impl *const T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_sub`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_sub`]: #method.wrapping_sub + /// /// # Examples /// /// Basic usage: @@ -1455,8 +1472,21 @@ impl *const T { /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). /// - /// Always use `.add(count)` instead when possible, because `add` - /// allows the compiler to optimize better. + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// Compared to [`add`], this method basically delays the requirement of staying + /// within the same allocated object: [`add`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_add` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`add`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`add`]: #method.add /// /// # Examples /// @@ -1496,8 +1526,21 @@ impl *const T { /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). /// - /// Always use `.sub(count)` instead when possible, because `sub` - /// allows the compiler to optimize better. + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// Compared to [`sub`], this method basically delays the requirement of staying + /// within the same allocated object: [`sub`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_sub` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`sub`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`sub`]: #method.sub /// /// # Examples /// @@ -1780,10 +1823,12 @@ impl *mut T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_offset`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_offset`]: #method.wrapping_offset + /// /// # Examples /// /// Basic usage: @@ -1811,15 +1856,26 @@ impl *mut T { /// /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). - /// In particular, the resulting pointer may *not* be used to access a - /// different allocated object than the one `self` points to. In other - /// words, `x.wrapping_offset(y.wrapping_offset_from(x))` is + /// + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// In other words, `x.wrapping_offset(y.wrapping_offset_from(x))` is /// *not* the same as `y`, and dereferencing it is undefined behavior /// unless `x` and `y` point into the same allocated object. /// - /// Always use `.offset(count)` instead when possible, because `offset` - /// allows the compiler to optimize better. If you need to cross object - /// boundaries, cast the pointer to an integer and do the arithmetic there. + /// Compared to [`offset`], this method basically delays the requirement of staying + /// within the same allocated object: [`offset`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_offset` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`offset`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`offset`]: #method.offset /// /// # Examples /// @@ -2032,10 +2088,12 @@ impl *mut T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_add`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_add`]: #method.wrapping_add + /// /// # Examples /// /// Basic usage: @@ -2090,10 +2148,12 @@ impl *mut T { /// Extension. As such, memory acquired directly from allocators or memory /// mapped files *may* be too large to handle with this function. /// - /// Consider using `wrapping_offset` instead if these constraints are + /// Consider using [`wrapping_sub`] instead if these constraints are /// difficult to satisfy. The only advantage of this method is that it /// enables more aggressive compiler optimizations. /// + /// [`wrapping_sub`]: #method.wrapping_sub + /// /// # Examples /// /// Basic usage: @@ -2126,8 +2186,21 @@ impl *mut T { /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). /// - /// Always use `.add(count)` instead when possible, because `add` - /// allows the compiler to optimize better. + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// Compared to [`add`], this method basically delays the requirement of staying + /// within the same allocated object: [`add`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_add` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`add`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`add`]: #method.add /// /// # Examples /// @@ -2167,8 +2240,21 @@ impl *mut T { /// The resulting pointer does not need to be in bounds, but it is /// potentially hazardous to dereference (which requires `unsafe`). /// - /// Always use `.sub(count)` instead when possible, because `sub` - /// allows the compiler to optimize better. + /// In particular, the resulting pointer remains attached to the same allocated + /// object that `self` points to. It may *not* be used to access a + /// different allocated object. Note that in Rust, + /// every (stack-allocated) variable is considered a separate allocated object. + /// + /// Compared to [`sub`], this method basically delays the requirement of staying + /// within the same allocated object: [`sub`] is immediate Undefined Behavior when + /// crossing object boundaries; `wrapping_sub` produces a pointer but still leads + /// to Undefined Behavior if that pointer is dereferenced. [`sub`] can be optimized + /// better and is thus preferrable in performance-sensitive code. + /// + /// If you need to cross object boundaries, cast the pointer to an integer and + /// do the arithmetic there. + /// + /// [`sub`]: #method.sub /// /// # Examples /// From 676953fde9120cda62e4ef2f75a804af7481d6af Mon Sep 17 00:00:00 2001 From: Andreas Jonson Date: Sat, 10 Aug 2019 10:52:04 +0200 Subject: [PATCH 04/12] Revert "Simplify MIR generation for logical ops" This reverts commit e38e954a0d249f88d0a55504f70d6055e865a931. llvm were not able to optimize the code that well with the simplified mir. Closes: #62993 --- src/librustc_mir/build/expr/into.rs | 53 ++++++++++++++++------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index 2815361a64760..02ab53fe8c1b1 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -79,17 +79,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ExprKind::LogicalOp { op, lhs, rhs } => { // And: // - // [block: If(lhs)] -true-> [else_block: dest = (rhs)] - // | (false) - // [shortcurcuit_block: dest = false] + // [block: If(lhs)] -true-> [else_block: If(rhs)] -true-> [true_block] + // | | (false) + // +----------false-----------+------------------> [false_block] // // Or: // - // [block: If(lhs)] -false-> [else_block: dest = (rhs)] - // | (true) - // [shortcurcuit_block: dest = true] + // [block: If(lhs)] -false-> [else_block: If(rhs)] -true-> [true_block] + // | (true) | (false) + // [true_block] [false_block] - let (shortcircuit_block, mut else_block, join_block) = ( + let (true_block, false_block, mut else_block, join_block) = ( + this.cfg.start_new_block(), this.cfg.start_new_block(), this.cfg.start_new_block(), this.cfg.start_new_block(), @@ -97,41 +98,47 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let lhs = unpack!(block = this.as_local_operand(block, lhs)); let blocks = match op { - LogicalOp::And => (else_block, shortcircuit_block), - LogicalOp::Or => (shortcircuit_block, else_block), + LogicalOp::And => (else_block, false_block), + LogicalOp::Or => (true_block, else_block), }; let term = TerminatorKind::if_(this.hir.tcx(), lhs, blocks.0, blocks.1); this.cfg.terminate(block, source_info, term); + let rhs = unpack!(else_block = this.as_local_operand(else_block, rhs)); + let term = TerminatorKind::if_(this.hir.tcx(), rhs, true_block, false_block); + this.cfg.terminate(else_block, source_info, term); + this.cfg.push_assign_constant( - shortcircuit_block, + true_block, source_info, destination, Constant { span: expr_span, ty: this.hir.bool_ty(), user_ty: None, - literal: match op { - LogicalOp::And => this.hir.false_literal(), - LogicalOp::Or => this.hir.true_literal(), - }, + literal: this.hir.true_literal(), }, ); - this.cfg.terminate( - shortcircuit_block, + + this.cfg.push_assign_constant( + false_block, source_info, - TerminatorKind::Goto { target: join_block }, + destination, + Constant { + span: expr_span, + ty: this.hir.bool_ty(), + user_ty: None, + literal: this.hir.false_literal(), + }, ); - let rhs = unpack!(else_block = this.as_local_operand(else_block, rhs)); - this.cfg.push_assign( - else_block, + this.cfg.terminate( + true_block, source_info, - destination, - Rvalue::Use(rhs), + TerminatorKind::Goto { target: join_block }, ); this.cfg.terminate( - else_block, + false_block, source_info, TerminatorKind::Goto { target: join_block }, ); From fa7fe196018e5fec39dee7ca6567c2024e60daf6 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 10 Aug 2019 18:38:27 +0300 Subject: [PATCH 05/12] resolve: Remove remaining special cases from built-in macros --- src/librustc_resolve/build_reduced_graph.rs | 7 ------- src/librustc_resolve/lib.rs | 3 --- src/librustc_resolve/macros.rs | 2 -- src/libsyntax/ext/base.rs | 4 ++-- src/libsyntax/ext/build.rs | 2 +- src/test/pretty/dollar-crate.pp | 8 ++++---- src/test/pretty/issue-4264.pp | 2 +- src/test/ui-fulldeps/deriving-encodable-decodable-box.rs | 7 +++---- .../deriving-encodable-decodable-cell-refcell.rs | 7 +++---- src/test/ui-fulldeps/deriving-global.rs | 3 +-- src/test/ui-fulldeps/deriving-hygiene.rs | 3 +-- src/test/ui-fulldeps/issue-11881.rs | 9 ++++----- 12 files changed, 20 insertions(+), 37 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 6e5750e752e94..5dd7bc3054829 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -126,8 +126,6 @@ impl<'a> Resolver<'a> { }; if let Some(id) = self.definitions.as_local_node_id(def_id) { self.local_macro_def_scopes[&id] - } else if self.is_builtin_macro(Some(def_id)) { - self.injected_crate.unwrap_or(self.graph_root) } else { let module_def_id = ty::DefIdTree::parent(&*self, def_id).unwrap(); self.get_module(module_def_id) @@ -596,11 +594,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { }; self.r.populate_module_if_necessary(module); - if let Some(name) = self.r.session.parse_sess.injected_crate_name.try_get() { - if name.as_str() == ident.name.as_str() { - self.r.injected_crate = Some(module); - } - } let used = self.process_legacy_macro_imports(item, module); let binding = diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index e786e1020026a..4cde00404f0b6 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -929,8 +929,6 @@ pub struct Resolver<'a> { /// it's not used during normal resolution, only for better error reporting. struct_constructors: DefIdMap<(Res, ty::Visibility)>, - injected_crate: Option>, - /// Features enabled for this crate. active_features: FxHashSet, } @@ -1168,7 +1166,6 @@ impl<'a> Resolver<'a> { unused_macros: Default::default(), proc_macro_stubs: Default::default(), special_derives: Default::default(), - injected_crate: None, active_features: features.declared_lib_features.iter().map(|(feat, ..)| *feat) .chain(features.declared_lang_features.iter().map(|(feat, ..)| *feat)) diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 5af71a0170a7b..8e9e1380002cf 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -854,8 +854,6 @@ impl<'a> Resolver<'a> { if ext.is_builtin { // The macro is a built-in, replace only the expander function. result.kind = ext.kind; - // Also reset its edition to the global one for compatibility. - result.edition = self.session.edition(); } else { // The macro is from a plugin, the in-source definition is dummy, // take all the data from the resolver. diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index d69822c7c7f1a..7f4feff6be670 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -592,8 +592,8 @@ pub struct SyntaxExtension { pub helper_attrs: Vec, /// Edition of the crate in which this macro is defined. pub edition: Edition, - /// Built-in macros have a couple of special properties (meaning of `$crate`, - /// availability in `#[no_implicit_prelude]` modules), so we have to keep this flag. + /// Built-in macros have a couple of special properties like availability + /// in `#[no_implicit_prelude]` modules, so we have to keep this flag. pub is_builtin: bool, /// We have to identify macros providing a `Copy` impl early for compatibility reasons. pub is_derive_copy: bool, diff --git a/src/libsyntax/ext/build.rs b/src/libsyntax/ext/build.rs index db562840e8d3b..22962499a2b75 100644 --- a/src/libsyntax/ext/build.rs +++ b/src/libsyntax/ext/build.rs @@ -495,7 +495,7 @@ impl<'a> ExtCtxt<'a> { let expr_loc_ptr = self.expr_addr_of(span, expr_loc_tuple); self.expr_call_global( span, - self.std_path(&[sym::rt, sym::begin_panic]), + [sym::std, sym::rt, sym::begin_panic].iter().map(|s| Ident::new(*s, span)).collect(), vec![ self.expr_str(span, msg), expr_loc_ptr]) diff --git a/src/test/pretty/dollar-crate.pp b/src/test/pretty/dollar-crate.pp index 3d2d949be2b2e..131cd0a67c669 100644 --- a/src/test/pretty/dollar-crate.pp +++ b/src/test/pretty/dollar-crate.pp @@ -10,9 +10,9 @@ fn main() { { - ::std::io::_print(::std::fmt::Arguments::new_v1(&["rust\n"], - &match () { - () => [], - })); + ::std::io::_print(::core::fmt::Arguments::new_v1(&["rust\n"], + &match () { + () => [], + })); }; } diff --git a/src/test/pretty/issue-4264.pp b/src/test/pretty/issue-4264.pp index bd839d3542199..4cf2e90e635fd 100644 --- a/src/test/pretty/issue-4264.pp +++ b/src/test/pretty/issue-4264.pp @@ -30,7 +30,7 @@ ((::alloc::fmt::format as - for<'r> fn(std::fmt::Arguments<'r>) -> std::string::String {std::fmt::format})(((<::std::fmt::Arguments>::new_v1 + for<'r> fn(std::fmt::Arguments<'r>) -> std::string::String {std::fmt::format})(((<::core::fmt::Arguments>::new_v1 as fn(&[&str], &[std::fmt::ArgumentV1<'_>]) -> std::fmt::Arguments<'_> {std::fmt::Arguments::<'_>::new_v1})((&([("test" as diff --git a/src/test/ui-fulldeps/deriving-encodable-decodable-box.rs b/src/test/ui-fulldeps/deriving-encodable-decodable-box.rs index 877fb57a25132..29c1b8fb0da97 100644 --- a/src/test/ui-fulldeps/deriving-encodable-decodable-box.rs +++ b/src/test/ui-fulldeps/deriving-encodable-decodable-box.rs @@ -5,11 +5,10 @@ #![feature(box_syntax)] #![feature(rustc_private)] -extern crate serialize; -use serialize as rustc_serialize; +extern crate serialize as rustc_serialize; -use serialize::{Encodable, Decodable}; -use serialize::json; +use rustc_serialize::{Encodable, Decodable}; +use rustc_serialize::json; #[derive(RustcEncodable, RustcDecodable)] struct A { diff --git a/src/test/ui-fulldeps/deriving-encodable-decodable-cell-refcell.rs b/src/test/ui-fulldeps/deriving-encodable-decodable-cell-refcell.rs index a35b681641a4b..fe608890bbd4d 100644 --- a/src/test/ui-fulldeps/deriving-encodable-decodable-cell-refcell.rs +++ b/src/test/ui-fulldeps/deriving-encodable-decodable-cell-refcell.rs @@ -7,12 +7,11 @@ #![feature(rustc_private)] -extern crate serialize; -use serialize as rustc_serialize; +extern crate serialize as rustc_serialize; use std::cell::{Cell, RefCell}; -use serialize::{Encodable, Decodable}; -use serialize::json; +use rustc_serialize::{Encodable, Decodable}; +use rustc_serialize::json; #[derive(RustcEncodable, RustcDecodable)] struct A { diff --git a/src/test/ui-fulldeps/deriving-global.rs b/src/test/ui-fulldeps/deriving-global.rs index b59d55ff213ce..d7cc98fed2595 100644 --- a/src/test/ui-fulldeps/deriving-global.rs +++ b/src/test/ui-fulldeps/deriving-global.rs @@ -2,8 +2,7 @@ #![feature(rustc_private)] -extern crate serialize; -use serialize as rustc_serialize; +extern crate serialize as rustc_serialize; mod submod { // if any of these are implemented without global calls for any diff --git a/src/test/ui-fulldeps/deriving-hygiene.rs b/src/test/ui-fulldeps/deriving-hygiene.rs index 0d7439ef87246..b1bdfaceb887d 100644 --- a/src/test/ui-fulldeps/deriving-hygiene.rs +++ b/src/test/ui-fulldeps/deriving-hygiene.rs @@ -2,8 +2,7 @@ #![allow(non_upper_case_globals)] #![feature(rustc_private)] -extern crate serialize; -use serialize as rustc_serialize; +extern crate serialize as rustc_serialize; pub const other: u8 = 1; pub const f: u8 = 1; diff --git a/src/test/ui-fulldeps/issue-11881.rs b/src/test/ui-fulldeps/issue-11881.rs index c8893e629418a..bd046a6cdee5f 100644 --- a/src/test/ui-fulldeps/issue-11881.rs +++ b/src/test/ui-fulldeps/issue-11881.rs @@ -6,17 +6,16 @@ #![feature(rustc_private)] -extern crate serialize; -use serialize as rustc_serialize; +extern crate serialize as rustc_serialize; use std::io::Cursor; use std::io::prelude::*; use std::fmt; use std::slice; -use serialize::{Encodable, Encoder}; -use serialize::json; -use serialize::opaque; +use rustc_serialize::{Encodable, Encoder}; +use rustc_serialize::json; +use rustc_serialize::opaque; #[derive(RustcEncodable)] struct Foo { From 53a6304c2a431ae97c3b584066804c6acd667541 Mon Sep 17 00:00:00 2001 From: Jakub Adam Wieczorek Date: Fri, 9 Aug 2019 09:43:26 +0000 Subject: [PATCH 06/12] Suggest using a qualified path in patterns with inconsistent bindings A program like the following one: ```rust enum E { A, B, C } fn f(x: E) -> bool { match x { A | B => false, C => true } } ``` is rejected by the compiler due to `E` variant paths not being in scope. In this case `A`, `B` are resolved as pattern bindings and consequently the pattern is considered invalid as the inner or-patterns do not bind to the same set of identifiers. This is expected but the compiler errors that follow could be surprising or confusing to some users. This commit adds a help note explaining that if the user desired to match against variants or consts, they should use a qualified path. The note is restricted to cases where the identifier starts with an upper-case sequence so as to reduce the false negatives. Since this happens during resolution, there's no clean way to check what the patterns match against. The syntactic criterium, however, is in line with the convention that's assumed by the `non-camel-case-types` lint. --- src/librustc_resolve/diagnostics.rs | 19 ++-- src/librustc_resolve/late.rs | 70 +++++++-------- src/librustc_resolve/lib.rs | 1 + .../ui/resolve/resolve-inconsistent-names.rs | 31 ++++++- .../resolve/resolve-inconsistent-names.stderr | 87 ++++++++++++++++++- 5 files changed, 159 insertions(+), 49 deletions(-) diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index 01a4a3c4bb244..b3f6252e4aabd 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -20,7 +20,7 @@ use syntax_pos::{BytePos, Span, MultiSpan}; use crate::resolve_imports::{ImportDirective, ImportDirectiveSubclass, ImportResolver}; use crate::{path_names_to_string, KNOWN_TOOLS}; -use crate::{CrateLint, LegacyScope, Module, ModuleOrUniformRoot}; +use crate::{BindingError, CrateLint, LegacyScope, Module, ModuleOrUniformRoot}; use crate::{PathResult, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Segment}; type Res = def::Res; @@ -207,21 +207,30 @@ impl<'a> Resolver<'a> { err } ResolutionError::VariableNotBoundInPattern(binding_error) => { - let target_sp = binding_error.target.iter().cloned().collect::>(); + let BindingError { name, target, origin, could_be_variant } = binding_error; + + let target_sp = target.iter().cloned().collect::>(); + let origin_sp = origin.iter().cloned().collect::>(); + let msp = MultiSpan::from_spans(target_sp.clone()); - let msg = format!("variable `{}` is not bound in all patterns", binding_error.name); + let msg = format!("variable `{}` is not bound in all patterns", name); let mut err = self.session.struct_span_err_with_code( msp, &msg, DiagnosticId::Error("E0408".into()), ); for sp in target_sp { - err.span_label(sp, format!("pattern doesn't bind `{}`", binding_error.name)); + err.span_label(sp, format!("pattern doesn't bind `{}`", name)); } - let origin_sp = binding_error.origin.iter().cloned(); for sp in origin_sp { err.span_label(sp, "variable not in all patterns"); } + if *could_be_variant { + let help_msg = format!( + "if you meant to match on a variant or a const, consider \ + making the path in the pattern qualified: `?::{}`", name); + err.span_help(span, &help_msg); + } err } ResolutionError::VariableBoundWithDifferentMode(variable_name, diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 7cb11195ee02b..13bae25a69af0 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -1136,64 +1136,56 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { // Checks that all of the arms in an or-pattern have exactly the // same set of bindings, with the same binding modes for each. fn check_consistent_bindings(&mut self, pats: &[P]) { - if pats.is_empty() { + if pats.len() <= 1 { return; } let mut missing_vars = FxHashMap::default(); let mut inconsistent_vars = FxHashMap::default(); - for (i, p) in pats.iter().enumerate() { + for p in pats.iter() { let map_i = self.binding_mode_map(&p); - - for (j, q) in pats.iter().enumerate() { - if i == j { + for q in pats.iter() { + if p.id == q.id { continue; } let map_j = self.binding_mode_map(&q); - for (&key, &binding_i) in &map_i { - if map_j.is_empty() { // Account for missing bindings when - let binding_error = missing_vars // `map_j` has none. - .entry(key.name) - .or_insert(BindingError { - name: key.name, - origin: BTreeSet::new(), - target: BTreeSet::new(), - }); - binding_error.origin.insert(binding_i.span); - binding_error.target.insert(q.span); - } - for (&key_j, &binding_j) in &map_j { - match map_i.get(&key_j) { - None => { // missing binding - let binding_error = missing_vars + for (&key_j, &binding_j) in map_j.iter() { + match map_i.get(&key_j) { + None => { // missing binding + let binding_error = missing_vars + .entry(key_j.name) + .or_insert(BindingError { + name: key_j.name, + origin: BTreeSet::new(), + target: BTreeSet::new(), + could_be_variant: + key_j.name.as_str().starts_with(char::is_uppercase) + }); + binding_error.origin.insert(binding_j.span); + binding_error.target.insert(p.span); + } + Some(binding_i) => { // check consistent binding + if binding_i.binding_mode != binding_j.binding_mode { + inconsistent_vars .entry(key_j.name) - .or_insert(BindingError { - name: key_j.name, - origin: BTreeSet::new(), - target: BTreeSet::new(), - }); - binding_error.origin.insert(binding_j.span); - binding_error.target.insert(p.span); - } - Some(binding_i) => { // check consistent binding - if binding_i.binding_mode != binding_j.binding_mode { - inconsistent_vars - .entry(key.name) - .or_insert((binding_j.span, binding_i.span)); - } + .or_insert((binding_j.span, binding_i.span)); } } } } } } - let mut missing_vars = missing_vars.iter().collect::>(); + + let mut missing_vars = missing_vars.iter_mut().collect::>(); missing_vars.sort(); - for (_, v) in missing_vars { + for (name, mut v) in missing_vars { + if inconsistent_vars.contains_key(name) { + v.could_be_variant = false; + } self.r.report_error( - *v.origin.iter().next().unwrap(), ResolutionError::VariableNotBoundInPattern(v) - ); + *v.origin.iter().next().unwrap(), + ResolutionError::VariableNotBoundInPattern(v)); } let mut inconsistent_vars = inconsistent_vars.iter().collect::>(); inconsistent_vars.sort(); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index e786e1020026a..1cacc6b7d606d 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -135,6 +135,7 @@ struct BindingError { name: Name, origin: BTreeSet, target: BTreeSet, + could_be_variant: bool } impl PartialOrd for BindingError { diff --git a/src/test/ui/resolve/resolve-inconsistent-names.rs b/src/test/ui/resolve/resolve-inconsistent-names.rs index 59baa57d91b3d..2fb803c4b2ad4 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.rs +++ b/src/test/ui/resolve/resolve-inconsistent-names.rs @@ -1,7 +1,36 @@ +#![allow(non_camel_case_types)] + +enum E { A, B, c } + +mod m { + const CONST1: usize = 10; + const Const2: usize = 20; +} + fn main() { let y = 1; match y { a | b => {} //~ ERROR variable `a` is not bound in all patterns - //~^ ERROR variable `b` is not bound in all patterns + //~| ERROR variable `b` is not bound in all patterns + } + + let x = (E::A, E::B); + match x { + (A, B) | (ref B, c) | (c, A) => () + //~^ ERROR variable `A` is not bound in all patterns + //~| ERROR variable `B` is not bound in all patterns + //~| ERROR variable `B` is bound in inconsistent ways + //~| ERROR mismatched types + //~| ERROR variable `c` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::A` + } + + let z = (10, 20); + match z { + (CONST1, _) | (_, Const2) => () + //~^ ERROR variable `CONST1` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::CONST1` + //~| ERROR variable `Const2` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::Const2` } } diff --git a/src/test/ui/resolve/resolve-inconsistent-names.stderr b/src/test/ui/resolve/resolve-inconsistent-names.stderr index c75718149fa4e..5a6ae31411ee2 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.stderr +++ b/src/test/ui/resolve/resolve-inconsistent-names.stderr @@ -1,5 +1,5 @@ error[E0408]: variable `a` is not bound in all patterns - --> $DIR/resolve-inconsistent-names.rs:4:12 + --> $DIR/resolve-inconsistent-names.rs:13:12 | LL | a | b => {} | - ^ pattern doesn't bind `a` @@ -7,13 +7,92 @@ LL | a | b => {} | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns - --> $DIR/resolve-inconsistent-names.rs:4:8 + --> $DIR/resolve-inconsistent-names.rs:13:8 | LL | a | b => {} | ^ - variable not in all patterns | | | pattern doesn't bind `b` -error: aborting due to 2 previous errors +error[E0408]: variable `A` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:18 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - ^^^^^^^^^^ - variable not in all patterns + | | | + | | pattern doesn't bind `A` + | variable not in all patterns + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::A` + --> $DIR/resolve-inconsistent-names.rs:19:10 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^ + +error[E0408]: variable `B` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:31 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - - ^^^^^^ pattern doesn't bind `B` + | | | + | | variable not in all patterns + | variable not in all patterns + +error[E0408]: variable `c` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:9 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^^^^^^ - - variable not in all patterns + | | | + | | variable not in all patterns + | pattern doesn't bind `c` + +error[E0409]: variable `B` is bound in inconsistent ways within the same match arm + --> $DIR/resolve-inconsistent-names.rs:19:23 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - ^ bound in different ways + | | + | first binding + +error[E0408]: variable `CONST1` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:30:23 + | +LL | (CONST1, _) | (_, Const2) => () + | ------ ^^^^^^^^^^^ pattern doesn't bind `CONST1` + | | + | variable not in all patterns + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::CONST1` + --> $DIR/resolve-inconsistent-names.rs:30:10 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^ + +error[E0408]: variable `Const2` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:30:9 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^^^^^^ ------ variable not in all patterns + | | + | pattern doesn't bind `Const2` + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::Const2` + --> $DIR/resolve-inconsistent-names.rs:30:27 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^ + +error[E0308]: mismatched types + --> $DIR/resolve-inconsistent-names.rs:19:19 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^^^^^ expected enum `E`, found &E + | + = note: expected type `E` + found type `&E` + +error: aborting due to 9 previous errors -For more information about this error, try `rustc --explain E0408`. +Some errors have detailed explanations: E0308, E0408, E0409. +For more information about an error, try `rustc --explain E0308`. From 30db4ebdc225521853889b50cc5164646bbe66ed Mon Sep 17 00:00:00 2001 From: Jakub Adam Wieczorek Date: Fri, 9 Aug 2019 20:21:45 +0000 Subject: [PATCH 07/12] Apply suggestions from code review Co-Authored-By: Mazdak Farrokhzad --- src/librustc_resolve/diagnostics.rs | 14 +++--- src/librustc_resolve/late.rs | 48 +++++++++---------- src/librustc_resolve/lib.rs | 2 +- .../resolve/resolve-inconsistent-names.stderr | 6 +-- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index b3f6252e4aabd..9827ce6ef20f8 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -207,10 +207,10 @@ impl<'a> Resolver<'a> { err } ResolutionError::VariableNotBoundInPattern(binding_error) => { - let BindingError { name, target, origin, could_be_variant } = binding_error; + let BindingError { name, target, origin, could_be_path } = binding_error; - let target_sp = target.iter().cloned().collect::>(); - let origin_sp = origin.iter().cloned().collect::>(); + let target_sp = target.iter().copied().collect::>(); + let origin_sp = origin.iter().copied().collect::>(); let msp = MultiSpan::from_spans(target_sp.clone()); let msg = format!("variable `{}` is not bound in all patterns", name); @@ -225,10 +225,12 @@ impl<'a> Resolver<'a> { for sp in origin_sp { err.span_label(sp, "variable not in all patterns"); } - if *could_be_variant { + if *could_be_path { let help_msg = format!( - "if you meant to match on a variant or a const, consider \ - making the path in the pattern qualified: `?::{}`", name); + "if you meant to match on a variant or a `const` item, consider \ + making the path in the pattern qualified: `?::{}`", + name, + ); err.span_help(span, &help_msg); } err diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 13bae25a69af0..358eaae11e712 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -1136,40 +1136,35 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { // Checks that all of the arms in an or-pattern have exactly the // same set of bindings, with the same binding modes for each. fn check_consistent_bindings(&mut self, pats: &[P]) { - if pats.len() <= 1 { - return; - } - let mut missing_vars = FxHashMap::default(); let mut inconsistent_vars = FxHashMap::default(); - for p in pats.iter() { - let map_i = self.binding_mode_map(&p); - for q in pats.iter() { - if p.id == q.id { - continue; - } - let map_j = self.binding_mode_map(&q); - for (&key_j, &binding_j) in map_j.iter() { - match map_i.get(&key_j) { + for pat_outer in pats.iter() { + let map_outer = self.binding_mode_map(&pat_outer); + + for pat_inner in pats.iter().filter(|pat| pat.id != pat_outer.id) { + let map_inner = self.binding_mode_map(&pat_inner); + + for (&key_inner, &binding_inner) in map_inner.iter() { + match map_outer.get(&key_inner) { None => { // missing binding let binding_error = missing_vars - .entry(key_j.name) + .entry(key_inner.name) .or_insert(BindingError { - name: key_j.name, + name: key_inner.name, origin: BTreeSet::new(), target: BTreeSet::new(), - could_be_variant: - key_j.name.as_str().starts_with(char::is_uppercase) + could_be_path: + key_inner.name.as_str().starts_with(char::is_uppercase) }); - binding_error.origin.insert(binding_j.span); - binding_error.target.insert(p.span); + binding_error.origin.insert(binding_inner.span); + binding_error.target.insert(pat_outer.span); } - Some(binding_i) => { // check consistent binding - if binding_i.binding_mode != binding_j.binding_mode { + Some(binding_outer) => { // check consistent binding + if binding_outer.binding_mode != binding_inner.binding_mode { inconsistent_vars - .entry(key_j.name) - .or_insert((binding_j.span, binding_i.span)); + .entry(key_inner.name) + .or_insert((binding_inner.span, binding_outer.span)); } } } @@ -1181,12 +1176,13 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { missing_vars.sort(); for (name, mut v) in missing_vars { if inconsistent_vars.contains_key(name) { - v.could_be_variant = false; + v.could_be_path = false; } self.r.report_error( *v.origin.iter().next().unwrap(), ResolutionError::VariableNotBoundInPattern(v)); } + let mut inconsistent_vars = inconsistent_vars.iter().collect::>(); inconsistent_vars.sort(); for (name, v) in inconsistent_vars { @@ -1214,7 +1210,9 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { self.resolve_pattern(pat, source, &mut bindings_list); } // This has to happen *after* we determine which pat_idents are variants - self.check_consistent_bindings(pats); + if pats.len() > 1 { + self.check_consistent_bindings(pats); + } } fn resolve_block(&mut self, block: &Block) { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 1cacc6b7d606d..98782dfbc7a5f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -135,7 +135,7 @@ struct BindingError { name: Name, origin: BTreeSet, target: BTreeSet, - could_be_variant: bool + could_be_path: bool } impl PartialOrd for BindingError { diff --git a/src/test/ui/resolve/resolve-inconsistent-names.stderr b/src/test/ui/resolve/resolve-inconsistent-names.stderr index 5a6ae31411ee2..f02867a0024b5 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.stderr +++ b/src/test/ui/resolve/resolve-inconsistent-names.stderr @@ -23,7 +23,7 @@ LL | (A, B) | (ref B, c) | (c, A) => () | | pattern doesn't bind `A` | variable not in all patterns | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::A` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::A` --> $DIR/resolve-inconsistent-names.rs:19:10 | LL | (A, B) | (ref B, c) | (c, A) => () @@ -63,7 +63,7 @@ LL | (CONST1, _) | (_, Const2) => () | | | variable not in all patterns | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::CONST1` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::CONST1` --> $DIR/resolve-inconsistent-names.rs:30:10 | LL | (CONST1, _) | (_, Const2) => () @@ -77,7 +77,7 @@ LL | (CONST1, _) | (_, Const2) => () | | | pattern doesn't bind `Const2` | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::Const2` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::Const2` --> $DIR/resolve-inconsistent-names.rs:30:27 | LL | (CONST1, _) | (_, Const2) => () From af5625dc85d186060637acc5f9f0417a7e2b1088 Mon Sep 17 00:00:00 2001 From: Tom Milligan Date: Sun, 11 Aug 2019 12:49:02 +0100 Subject: [PATCH 08/12] docs: add stdlib env::var(_os) panic --- src/libstd/env.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libstd/env.rs b/src/libstd/env.rs index 1f5de25b65c90..eca93399e5807 100644 --- a/src/libstd/env.rs +++ b/src/libstd/env.rs @@ -182,6 +182,12 @@ impl fmt::Debug for VarsOs { /// * Environment variable is not present /// * Environment variable is not valid unicode /// +/// # Panics +/// +/// This function may panic if `key` is empty, contains an ASCII equals sign +/// `'='` or the NUL character `'\0'`, or when the value contains the NUL +/// character. +/// /// # Examples /// /// ``` @@ -210,6 +216,12 @@ fn _var(key: &OsStr) -> Result { /// /// [`None`]: ../option/enum.Option.html#variant.None /// +/// # Panics +/// +/// This function may panic if `key` is empty, contains an ASCII equals sign +/// `'='` or the NUL character `'\0'`, or when the value contains the NUL +/// character. +/// /// # Examples /// /// ``` From 75d2db97fe7ab1c17cf6865c6a5a575f5aede6f7 Mon Sep 17 00:00:00 2001 From: Adrian Budau Date: Sun, 11 Aug 2019 22:44:42 +0300 Subject: [PATCH 09/12] Regression test for #56870 --- src/test/ui/issues/issue-56870.rs | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 src/test/ui/issues/issue-56870.rs diff --git a/src/test/ui/issues/issue-56870.rs b/src/test/ui/issues/issue-56870.rs new file mode 100644 index 0000000000000..137a0ede0b33c --- /dev/null +++ b/src/test/ui/issues/issue-56870.rs @@ -0,0 +1,38 @@ +// build-pass +// Regression test for #56870: Internal compiler error (traits & associated consts) + +use std::fmt::Debug; + +pub trait Foo { + const FOO: *const u8; +} + +impl Foo for dyn Debug { + const FOO: *const u8 = ::fmt as *const u8; +} + +pub trait Bar { + const BAR: *const u8; +} + +pub trait Baz { + type Data: Debug; +} + +pub struct BarStruct(S); + +impl Bar for BarStruct { + const BAR: *const u8 = ::Data>>::FOO; +} + +struct AnotherStruct; +#[derive(Debug)] +struct SomeStruct; + +impl Baz for AnotherStruct { + type Data = SomeStruct; +} + +fn main() { + let _x = as Bar>::BAR; +} From 6ed4a42fcf2aee2197fbc9d3cd00f1a796ae53bd Mon Sep 17 00:00:00 2001 From: Adam Date: Sun, 11 Aug 2019 22:17:28 +0200 Subject: [PATCH 10/12] Add test for issue 53598 and 57700 --- src/test/ui/impl-trait/issue-53598.rs | 28 +++++++++++++++++++++++ src/test/ui/impl-trait/issue-53598.stderr | 12 ++++++++++ src/test/ui/impl-trait/issue-57700.rs | 22 ++++++++++++++++++ src/test/ui/impl-trait/issue-57700.stderr | 12 ++++++++++ 4 files changed, 74 insertions(+) create mode 100644 src/test/ui/impl-trait/issue-53598.rs create mode 100644 src/test/ui/impl-trait/issue-53598.stderr create mode 100644 src/test/ui/impl-trait/issue-57700.rs create mode 100644 src/test/ui/impl-trait/issue-57700.stderr diff --git a/src/test/ui/impl-trait/issue-53598.rs b/src/test/ui/impl-trait/issue-53598.rs new file mode 100644 index 0000000000000..61dff79d07bad --- /dev/null +++ b/src/test/ui/impl-trait/issue-53598.rs @@ -0,0 +1,28 @@ +// ignore-tidy-linelength +#![feature(type_alias_impl_trait)] + +use std::fmt::Debug; + +pub trait Foo { + type Item: Debug; + + fn foo(_: T) -> Self::Item; +} + +#[derive(Debug)] +pub struct S(std::marker::PhantomData); + +pub struct S2; + +impl Foo for S2 { + type Item = impl Debug; + + fn foo(_: T) -> Self::Item { + //~^ Error type parameter `T` is part of concrete type but not used in parameter list for the `impl Trait` type alias + S::(Default::default()) + } +} + +fn main() { + S2::foo(123); +} diff --git a/src/test/ui/impl-trait/issue-53598.stderr b/src/test/ui/impl-trait/issue-53598.stderr new file mode 100644 index 0000000000000..4c8144a235930 --- /dev/null +++ b/src/test/ui/impl-trait/issue-53598.stderr @@ -0,0 +1,12 @@ +error: type parameter `T` is part of concrete type but not used in parameter list for the `impl Trait` type alias + --> $DIR/issue-53598.rs:20:42 + | +LL | fn foo(_: T) -> Self::Item { + | __________________________________________^ +LL | | +LL | | S::(Default::default()) +LL | | } + | |_____^ + +error: aborting due to previous error + diff --git a/src/test/ui/impl-trait/issue-57700.rs b/src/test/ui/impl-trait/issue-57700.rs new file mode 100644 index 0000000000000..bfabef54724c8 --- /dev/null +++ b/src/test/ui/impl-trait/issue-57700.rs @@ -0,0 +1,22 @@ +// ignore-tidy-linelength +#![feature(arbitrary_self_types)] +#![feature(type_alias_impl_trait)] + +use std::ops::Deref; + +trait Foo { + type Bar: Foo; + + fn foo(self: impl Deref) -> Self::Bar; +} + +impl Foo for C { + type Bar = impl Foo; + + fn foo(self: impl Deref) -> Self::Bar { + //~^ Error type parameter `impl Deref` is part of concrete type but not used in parameter list for the `impl Trait` type alias + self + } +} + +fn main() {} diff --git a/src/test/ui/impl-trait/issue-57700.stderr b/src/test/ui/impl-trait/issue-57700.stderr new file mode 100644 index 0000000000000..c701e3e74ef59 --- /dev/null +++ b/src/test/ui/impl-trait/issue-57700.stderr @@ -0,0 +1,12 @@ +error: type parameter `impl Deref` is part of concrete type but not used in parameter list for the `impl Trait` type alias + --> $DIR/issue-57700.rs:16:58 + | +LL | fn foo(self: impl Deref) -> Self::Bar { + | __________________________________________________________^ +LL | | +LL | | self +LL | | } + | |_____^ + +error: aborting due to previous error + From 5981dfffbae081df0281de2cbe0da79fa1ceb7e6 Mon Sep 17 00:00:00 2001 From: Adam Date: Sun, 11 Aug 2019 22:30:21 +0200 Subject: [PATCH 11/12] Move tests into type-alias-impl-trait --- src/test/ui/{impl-trait => type-alias-impl-trait}/issue-53598.rs | 0 .../ui/{impl-trait => type-alias-impl-trait}/issue-53598.stderr | 0 src/test/ui/{impl-trait => type-alias-impl-trait}/issue-57700.rs | 0 .../ui/{impl-trait => type-alias-impl-trait}/issue-57700.stderr | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename src/test/ui/{impl-trait => type-alias-impl-trait}/issue-53598.rs (100%) rename src/test/ui/{impl-trait => type-alias-impl-trait}/issue-53598.stderr (100%) rename src/test/ui/{impl-trait => type-alias-impl-trait}/issue-57700.rs (100%) rename src/test/ui/{impl-trait => type-alias-impl-trait}/issue-57700.stderr (100%) diff --git a/src/test/ui/impl-trait/issue-53598.rs b/src/test/ui/type-alias-impl-trait/issue-53598.rs similarity index 100% rename from src/test/ui/impl-trait/issue-53598.rs rename to src/test/ui/type-alias-impl-trait/issue-53598.rs diff --git a/src/test/ui/impl-trait/issue-53598.stderr b/src/test/ui/type-alias-impl-trait/issue-53598.stderr similarity index 100% rename from src/test/ui/impl-trait/issue-53598.stderr rename to src/test/ui/type-alias-impl-trait/issue-53598.stderr diff --git a/src/test/ui/impl-trait/issue-57700.rs b/src/test/ui/type-alias-impl-trait/issue-57700.rs similarity index 100% rename from src/test/ui/impl-trait/issue-57700.rs rename to src/test/ui/type-alias-impl-trait/issue-57700.rs diff --git a/src/test/ui/impl-trait/issue-57700.stderr b/src/test/ui/type-alias-impl-trait/issue-57700.stderr similarity index 100% rename from src/test/ui/impl-trait/issue-57700.stderr rename to src/test/ui/type-alias-impl-trait/issue-57700.stderr From 3d38187afd68f161ef88423cada98edcf1f25d6e Mon Sep 17 00:00:00 2001 From: OptimisticPeach Date: Mon, 12 Aug 2019 00:15:14 -0400 Subject: [PATCH 12/12] Fixes #63477 Adds a closing parenthesis. --- src/libstd/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/macros.rs b/src/libstd/macros.rs index 839b4c5656a09..f2000936b9a21 100644 --- a/src/libstd/macros.rs +++ b/src/libstd/macros.rs @@ -119,7 +119,7 @@ macro_rules! print { /// Prints to the standard output, with a newline. /// /// On all platforms, the newline is the LINE FEED character (`\n`/`U+000A`) alone -/// (no additional CARRIAGE RETURN (`\r`/`U+000D`). +/// (no additional CARRIAGE RETURN (`\r`/`U+000D`)). /// /// Use the [`format!`] syntax to write data to the standard output. /// See [`std::fmt`] for more information.