From 18ad5a594ec4ee039897eb03d90dfeb117b4c6d6 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Wed, 19 Aug 2020 17:19:21 +0900 Subject: [PATCH 01/37] Add a regression test for issue-72793 --- .../ui/type-alias-impl-trait/issue-72793.rs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 src/test/ui/type-alias-impl-trait/issue-72793.rs diff --git a/src/test/ui/type-alias-impl-trait/issue-72793.rs b/src/test/ui/type-alias-impl-trait/issue-72793.rs new file mode 100644 index 0000000000000..e643a8cab5b02 --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/issue-72793.rs @@ -0,0 +1,27 @@ +// build-pass + +// Regression test for #72793. +// FIXME: This still shows ICE with `-Zmir-opt-level=2`. + +#![feature(type_alias_impl_trait)] + +trait T { type Item; } + +type Alias<'a> = impl T; + +struct S; +impl<'a> T for &'a S { + type Item = &'a (); +} + +fn filter_positive<'a>() -> Alias<'a> { + &S +} + +fn with_positive(fun: impl Fn(Alias<'_>)) { + fun(filter_positive()); +} + +fn main() { + with_positive(|_| ()); +} From e5f9d7ff92f62cde3ef1b7301ac4ac3adab990d9 Mon Sep 17 00:00:00 2001 From: Stein Somers Date: Thu, 23 Jul 2020 13:07:30 +0200 Subject: [PATCH 02/37] BTreeMap: introduce marker::ValMut and reserve marker::Mut for unique access --- library/alloc/src/collections/btree/map.rs | 134 +------- .../alloc/src/collections/btree/navigate.rs | 318 +++++++++++++++--- library/alloc/src/collections/btree/node.rs | 38 ++- 3 files changed, 319 insertions(+), 171 deletions(-) diff --git a/library/alloc/src/collections/btree/map.rs b/library/alloc/src/collections/btree/map.rs index f8729c33c6713..92f02fb60168b 100644 --- a/library/alloc/src/collections/btree/map.rs +++ b/library/alloc/src/collections/btree/map.rs @@ -5,7 +5,6 @@ use core::hash::{Hash, Hasher}; use core::iter::{FromIterator, FusedIterator, Peekable}; use core::marker::PhantomData; use core::mem::{self, ManuallyDrop}; -use core::ops::Bound::{Excluded, Included, Unbounded}; use core::ops::{Index, RangeBounds}; use core::ptr; @@ -408,8 +407,8 @@ impl fmt::Debug for Range<'_, K, V> { /// [`range_mut`]: BTreeMap::range_mut #[stable(feature = "btree_range", since = "1.17.0")] pub struct RangeMut<'a, K: 'a, V: 'a> { - front: Option, K, V, marker::Leaf>, marker::Edge>>, - back: Option, K, V, marker::Leaf>, marker::Edge>>, + front: Option, K, V, marker::Leaf>, marker::Edge>>, + back: Option, K, V, marker::Leaf>, marker::Edge>>, // Be invariant in `K` and `V` _marker: PhantomData<&'a mut (K, V)>, @@ -999,7 +998,7 @@ impl BTreeMap { R: RangeBounds, { if let Some(root) = &self.root { - let (f, b) = range_search(root.node_as_ref(), range); + let (f, b) = root.node_as_ref().range_search(range); Range { front: Some(f), back: Some(b) } } else { @@ -1045,7 +1044,7 @@ impl BTreeMap { R: RangeBounds, { if let Some(root) = &mut self.root { - let (f, b) = range_search(root.node_as_mut(), range); + let (f, b) = root.node_as_valmut().range_search(range); RangeMut { front: Some(f), back: Some(b), _marker: PhantomData } } else { @@ -1478,7 +1477,7 @@ impl IntoIterator for BTreeMap { fn into_iter(self) -> IntoIter { let mut me = ManuallyDrop::new(self); if let Some(root) = me.root.take() { - let (f, b) = full_range_search(root.into_ref()); + let (f, b) = root.into_ref().full_range(); IntoIter { front: Some(f), back: Some(b), length: me.length } } else { @@ -1942,7 +1941,7 @@ impl<'a, K, V> RangeMut<'a, K, V> { self.front == self.back } - unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) { + unsafe fn next_unchecked(&mut self) -> (&'a K, &'a mut V) { unsafe { unwrap_unchecked(self.front.as_mut()).next_unchecked() } } } @@ -1963,7 +1962,7 @@ impl<'a, K, V> DoubleEndedIterator for RangeMut<'a, K, V> { impl FusedIterator for RangeMut<'_, K, V> {} impl<'a, K, V> RangeMut<'a, K, V> { - unsafe fn next_back_unchecked(&mut self) -> (&'a mut K, &'a mut V) { + unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a mut V) { unsafe { unwrap_unchecked(self.back.as_mut()).next_back_unchecked() } } } @@ -2073,119 +2072,6 @@ where } } -/// Finds the leaf edges delimiting a specified range in or underneath a node. -fn range_search>( - root: NodeRef, - range: R, -) -> ( - Handle, marker::Edge>, - Handle, marker::Edge>, -) -where - Q: Ord, - K: Borrow, -{ - match (range.start_bound(), range.end_bound()) { - (Excluded(s), Excluded(e)) if s == e => { - panic!("range start and end are equal and excluded in BTreeMap") - } - (Included(s) | Excluded(s), Included(e) | Excluded(e)) if s > e => { - panic!("range start is greater than range end in BTreeMap") - } - _ => {} - }; - - // We duplicate the root NodeRef here -- we will never access it in a way - // that overlaps references obtained from the root. - let mut min_node = unsafe { ptr::read(&root) }; - let mut max_node = root; - let mut min_found = false; - let mut max_found = false; - - loop { - let front = match (min_found, range.start_bound()) { - (false, Included(key)) => match search::search_node(min_node, key) { - Found(kv) => { - min_found = true; - kv.left_edge() - } - GoDown(edge) => edge, - }, - (false, Excluded(key)) => match search::search_node(min_node, key) { - Found(kv) => { - min_found = true; - kv.right_edge() - } - GoDown(edge) => edge, - }, - (true, Included(_)) => min_node.last_edge(), - (true, Excluded(_)) => min_node.first_edge(), - (_, Unbounded) => min_node.first_edge(), - }; - - let back = match (max_found, range.end_bound()) { - (false, Included(key)) => match search::search_node(max_node, key) { - Found(kv) => { - max_found = true; - kv.right_edge() - } - GoDown(edge) => edge, - }, - (false, Excluded(key)) => match search::search_node(max_node, key) { - Found(kv) => { - max_found = true; - kv.left_edge() - } - GoDown(edge) => edge, - }, - (true, Included(_)) => max_node.first_edge(), - (true, Excluded(_)) => max_node.last_edge(), - (_, Unbounded) => max_node.last_edge(), - }; - - if front.partial_cmp(&back) == Some(Ordering::Greater) { - panic!("Ord is ill-defined in BTreeMap range"); - } - match (front.force(), back.force()) { - (Leaf(f), Leaf(b)) => { - return (f, b); - } - (Internal(min_int), Internal(max_int)) => { - min_node = min_int.descend(); - max_node = max_int.descend(); - } - _ => unreachable!("BTreeMap has different depths"), - }; - } -} - -/// Equivalent to `range_search(k, v, ..)` without the `Ord` bound. -fn full_range_search( - root: NodeRef, -) -> ( - Handle, marker::Edge>, - Handle, marker::Edge>, -) { - // We duplicate the root NodeRef here -- we will never access it in a way - // that overlaps references obtained from the root. - let mut min_node = unsafe { ptr::read(&root) }; - let mut max_node = root; - loop { - let front = min_node.first_edge(); - let back = max_node.last_edge(); - match (front.force(), back.force()) { - (Leaf(f), Leaf(b)) => { - return (f, b); - } - (Internal(min_int), Internal(max_int)) => { - min_node = min_int.descend(); - max_node = max_int.descend(); - } - _ => unreachable!("BTreeMap has different depths"), - }; - } -} - impl BTreeMap { /// Gets an iterator over the entries of the map, sorted by key. /// @@ -2211,7 +2097,7 @@ impl BTreeMap { #[stable(feature = "rust1", since = "1.0.0")] pub fn iter(&self) -> Iter<'_, K, V> { if let Some(root) = &self.root { - let (f, b) = full_range_search(root.node_as_ref()); + let (f, b) = root.node_as_ref().full_range(); Iter { range: Range { front: Some(f), back: Some(b) }, length: self.length } } else { @@ -2243,7 +2129,7 @@ impl BTreeMap { #[stable(feature = "rust1", since = "1.0.0")] pub fn iter_mut(&mut self) -> IterMut<'_, K, V> { if let Some(root) = &mut self.root { - let (f, b) = full_range_search(root.node_as_mut()); + let (f, b) = root.node_as_valmut().full_range(); IterMut { range: RangeMut { front: Some(f), back: Some(b), _marker: PhantomData }, @@ -2826,7 +2712,7 @@ impl<'a, K: 'a, V: 'a> Handle, K, V, marker::LeafOrInter if stole_from_left && at_leaf { // SAFETY: This is safe since we just added an element to our node. unsafe { - pos.next_unchecked(); + pos.move_next_unchecked(); } } break; diff --git a/library/alloc/src/collections/btree/navigate.rs b/library/alloc/src/collections/btree/navigate.rs index b7b66ac7ceccd..376060b3143de 100644 --- a/library/alloc/src/collections/btree/navigate.rs +++ b/library/alloc/src/collections/btree/navigate.rs @@ -1,10 +1,210 @@ +use core::borrow::Borrow; +use core::cmp::Ordering; use core::intrinsics; use core::mem; +use core::ops::Bound::{Excluded, Included, Unbounded}; +use core::ops::RangeBounds; use core::ptr; use super::node::{marker, ForceResult::*, Handle, NodeRef}; +use super::search::{self, SearchResult}; use super::unwrap_unchecked; +/// Finds the leaf edges delimiting a specified range in or underneath a node. +fn range_search( + root1: NodeRef, + root2: NodeRef, + range: R, +) -> ( + Handle, marker::Edge>, + Handle, marker::Edge>, +) +where + Q: ?Sized + Ord, + K: Borrow, + R: RangeBounds, +{ + match (range.start_bound(), range.end_bound()) { + (Excluded(s), Excluded(e)) if s == e => { + panic!("range start and end are equal and excluded in BTreeMap") + } + (Included(s) | Excluded(s), Included(e) | Excluded(e)) if s > e => { + panic!("range start is greater than range end in BTreeMap") + } + _ => {} + }; + + let mut min_node = root1; + let mut max_node = root2; + let mut min_found = false; + let mut max_found = false; + + loop { + let front = match (min_found, range.start_bound()) { + (false, Included(key)) => match search::search_node(min_node, key) { + SearchResult::Found(kv) => { + min_found = true; + kv.left_edge() + } + SearchResult::GoDown(edge) => edge, + }, + (false, Excluded(key)) => match search::search_node(min_node, key) { + SearchResult::Found(kv) => { + min_found = true; + kv.right_edge() + } + SearchResult::GoDown(edge) => edge, + }, + (true, Included(_)) => min_node.last_edge(), + (true, Excluded(_)) => min_node.first_edge(), + (_, Unbounded) => min_node.first_edge(), + }; + + let back = match (max_found, range.end_bound()) { + (false, Included(key)) => match search::search_node(max_node, key) { + SearchResult::Found(kv) => { + max_found = true; + kv.right_edge() + } + SearchResult::GoDown(edge) => edge, + }, + (false, Excluded(key)) => match search::search_node(max_node, key) { + SearchResult::Found(kv) => { + max_found = true; + kv.left_edge() + } + SearchResult::GoDown(edge) => edge, + }, + (true, Included(_)) => max_node.first_edge(), + (true, Excluded(_)) => max_node.last_edge(), + (_, Unbounded) => max_node.last_edge(), + }; + + if front.partial_cmp(&back) == Some(Ordering::Greater) { + panic!("Ord is ill-defined in BTreeMap range"); + } + match (front.force(), back.force()) { + (Leaf(f), Leaf(b)) => { + return (f, b); + } + (Internal(min_int), Internal(max_int)) => { + min_node = min_int.descend(); + max_node = max_int.descend(); + } + _ => unreachable!("BTreeMap has different depths"), + }; + } +} + +/// Equivalent to `range_search(k, v, ..)` but without the `Ord` bound. +fn full_range( + root1: NodeRef, + root2: NodeRef, +) -> ( + Handle, marker::Edge>, + Handle, marker::Edge>, +) { + let mut min_node = root1; + let mut max_node = root2; + loop { + let front = min_node.first_edge(); + let back = max_node.last_edge(); + match (front.force(), back.force()) { + (Leaf(f), Leaf(b)) => { + return (f, b); + } + (Internal(min_int), Internal(max_int)) => { + min_node = min_int.descend(); + max_node = max_int.descend(); + } + _ => unreachable!("BTreeMap has different depths"), + }; + } +} + +impl<'a, K: 'a, V: 'a> NodeRef, K, V, marker::LeafOrInternal> { + /// Creates a pair of leaf edges delimiting a specified range in or underneath a node. + pub fn range_search( + self, + range: R, + ) -> ( + Handle, K, V, marker::Leaf>, marker::Edge>, + Handle, K, V, marker::Leaf>, marker::Edge>, + ) + where + Q: ?Sized + Ord, + K: Borrow, + R: RangeBounds, + { + range_search(self, self, range) + } + + /// Returns (self.first_leaf_edge(), self.last_leaf_edge()), but more efficiently. + pub fn full_range( + self, + ) -> ( + Handle, K, V, marker::Leaf>, marker::Edge>, + Handle, K, V, marker::Leaf>, marker::Edge>, + ) { + full_range(self, self) + } +} + +impl<'a, K: 'a, V: 'a> NodeRef, K, V, marker::LeafOrInternal> { + /// Splits a unique reference into a pair of leaf edges delimiting a specified range. + /// The result are non-unique references allowing (some) mutation, which must be used + /// carefully. + pub fn range_search( + self, + range: R, + ) -> ( + Handle, K, V, marker::Leaf>, marker::Edge>, + Handle, K, V, marker::Leaf>, marker::Edge>, + ) + where + Q: ?Sized + Ord, + K: Borrow, + R: RangeBounds, + { + // We duplicate the root NodeRef here -- we will never visit the same KV + // twice, and never end up with overlapping value references. + let self2 = unsafe { ptr::read(&self) }; + range_search(self, self2, range) + } + + /// Splits a unique reference into a pair of leaf edges delimiting the full range of the tree. + /// The results are non-unique references allowing mutation (of values only), so must be used + /// with care. + pub fn full_range( + self, + ) -> ( + Handle, K, V, marker::Leaf>, marker::Edge>, + Handle, K, V, marker::Leaf>, marker::Edge>, + ) { + // We duplicate the root NodeRef here -- we will never visit the same KV + // twice, and never end up with overlapping value references. + let self2 = unsafe { ptr::read(&self) }; + full_range(self, self2) + } +} + +impl NodeRef { + /// Splits a unique reference into a pair of leaf edges delimiting the full range of the tree. + /// The results are non-unique references allowing massively destructive mutation, so must be + /// used with the utmost care. + pub fn full_range( + self, + ) -> ( + Handle, marker::Edge>, + Handle, marker::Edge>, + ) { + // We duplicate the root NodeRef here -- we will never access it in a way + // that overlaps references obtained from the root. + let self2 = unsafe { ptr::read(&self) }; + full_range(self, self2) + } +} + impl Handle, marker::Edge> { /// Given a leaf edge handle, returns [`Result::Ok`] with a handle to the neighboring KV /// on the right side, which is either in the same leaf node or in an ancestor node. @@ -75,12 +275,13 @@ impl Handle, marke macro_rules! def_next_kv_uncheched_dealloc { { unsafe fn $name:ident : $adjacent_kv:ident } => { /// Given a leaf edge handle into an owned tree, returns a handle to the next KV, - /// while deallocating any node left behind. - /// Unsafe for two reasons: - /// - The caller must ensure that the leaf edge is not the last one in the tree. - /// - The node pointed at by the given handle, and its ancestors, may be deallocated, - /// while the reference to those nodes in the surviving ancestors is left dangling; - /// thus using the returned handle to navigate further is dangerous. + /// while deallocating any node left behind yet leaving the corresponding edge + /// in its parent node dangling. + /// + /// # Safety + /// - The leaf edge must not be the last one in the direction travelled. + /// - The node carrying the next KV returned must not have been deallocated by a + /// previous call on any handle obtained for this tree. unsafe fn $name ( leaf_edge: Handle, marker::Edge>, ) -> Handle, marker::KV> { @@ -103,6 +304,15 @@ macro_rules! def_next_kv_uncheched_dealloc { def_next_kv_uncheched_dealloc! {unsafe fn next_kv_unchecked_dealloc: right_kv} def_next_kv_uncheched_dealloc! {unsafe fn next_back_kv_unchecked_dealloc: left_kv} +/// This replaces the value behind the `v` unique reference by calling the +/// relevant function. +/// +/// If a panic occurs in the `change` closure, the entire process will be aborted. +#[inline] +fn take_mut(v: &mut T, change: impl FnOnce(T) -> T) { + replace(v, |value| (change(value), ())) +} + /// This replaces the value behind the `v` unique reference by calling the /// relevant function, and returns a result obtained along the way. /// @@ -128,7 +338,9 @@ fn replace(v: &mut T, change: impl FnOnce(T) -> (T, R)) -> R { impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge> { /// Moves the leaf edge handle to the next leaf edge and returns references to the /// key and value in between. - /// Unsafe because the caller must ensure that the leaf edge is not the last one in the tree. + /// + /// # Safety + /// There must be another KV in the direction travelled. pub unsafe fn next_unchecked(&mut self) -> (&'a K, &'a V) { replace(self, |leaf_edge| { let kv = leaf_edge.next_kv(); @@ -139,7 +351,9 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Ed /// Moves the leaf edge handle to the previous leaf edge and returns references to the /// key and value in between. - /// Unsafe because the caller must ensure that the leaf edge is not the first one in the tree. + /// + /// # Safety + /// There must be another KV in the direction travelled. pub unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a V) { replace(self, |leaf_edge| { let kv = leaf_edge.next_back_kv(); @@ -149,53 +363,69 @@ impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Ed } } -impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge> { +impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge> { /// Moves the leaf edge handle to the next leaf edge and returns references to the /// key and value in between. - /// Unsafe for two reasons: - /// - The caller must ensure that the leaf edge is not the last one in the tree. - /// - Using the updated handle may well invalidate the returned references. - pub unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) { + /// The returned references might be invalidated when the updated handle is used again. + /// + /// # Safety + /// There must be another KV in the direction travelled. + pub unsafe fn next_unchecked(&mut self) -> (&'a K, &'a mut V) { let kv = replace(self, |leaf_edge| { let kv = leaf_edge.next_kv(); let kv = unsafe { unwrap_unchecked(kv.ok()) }; (unsafe { ptr::read(&kv) }.next_leaf_edge(), kv) }); // Doing the descend (and perhaps another move) invalidates the references - // returned by `into_kv_mut`, so we have to do this last. - kv.into_kv_mut() + // returned by `into_kv_valmut`, so we have to do this last. + kv.into_kv_valmut() } /// Moves the leaf edge handle to the previous leaf and returns references to the /// key and value in between. - /// Unsafe for two reasons: - /// - The caller must ensure that the leaf edge is not the first one in the tree. - /// - Using the updated handle may well invalidate the returned references. - pub unsafe fn next_back_unchecked(&mut self) -> (&'a mut K, &'a mut V) { + /// The returned references might be invalidated when the updated handle is used again. + /// + /// # Safety + /// There must be another KV in the direction travelled. + pub unsafe fn next_back_unchecked(&mut self) -> (&'a K, &'a mut V) { let kv = replace(self, |leaf_edge| { let kv = leaf_edge.next_back_kv(); let kv = unsafe { unwrap_unchecked(kv.ok()) }; (unsafe { ptr::read(&kv) }.next_back_leaf_edge(), kv) }); // Doing the descend (and perhaps another move) invalidates the references - // returned by `into_kv_mut`, so we have to do this last. - kv.into_kv_mut() + // returned by `into_kv_valmut`, so we have to do this last. + kv.into_kv_valmut() + } +} + +impl<'a, K, V> Handle, K, V, marker::Leaf>, marker::Edge> { + /// Moves the leaf edge handle to the next leaf edge. + /// + /// # Safety + /// There must be another KV in the direction travelled. + pub unsafe fn move_next_unchecked(&mut self) { + take_mut(self, |leaf_edge| { + let kv = leaf_edge.next_kv(); + let kv = unsafe { unwrap_unchecked(kv.ok()) }; + kv.next_leaf_edge() + }) } } impl Handle, marker::Edge> { /// Moves the leaf edge handle to the next leaf edge and returns the key and value - /// in between, while deallocating any node left behind. - /// Unsafe for two reasons: - /// - The caller must ensure that the leaf edge is not the last one in the tree - /// and is not a handle previously resulting from counterpart `next_back_unchecked`. - /// - Further use of the updated leaf edge handle is very dangerous. In particular, - /// if the leaf edge is the last edge of a node, that node and possibly ancestors - /// will be deallocated, while the reference to those nodes in the surviving ancestor - /// is left dangling. - /// The only safe way to proceed with the updated handle is to compare it, drop it, - /// call this method again subject to both preconditions listed in the first point, - /// or call counterpart `next_back_unchecked` subject to its preconditions. + /// in between, deallocating any node left behind while leaving the corresponding + /// edge in its parent node dangling. + /// + /// # Safety + /// - There must be another KV in the direction travelled. + /// - That KV was not previously returned by counterpart `next_back_unchecked` + /// on any copy of the handles being used to traverse the tree. + /// + /// The only safe way to proceed with the updated handle is to compare it, drop it, + /// call this method again subject to its safety conditions, or call counterpart + /// `next_back_unchecked` subject to its safety conditions. pub unsafe fn next_unchecked(&mut self) -> (K, V) { replace(self, |leaf_edge| { let kv = unsafe { next_kv_unchecked_dealloc(leaf_edge) }; @@ -205,18 +435,18 @@ impl Handle, marker::Edge> { }) } - /// Moves the leaf edge handle to the previous leaf edge and returns the key - /// and value in between, while deallocating any node left behind. - /// Unsafe for two reasons: - /// - The caller must ensure that the leaf edge is not the first one in the tree - /// and is not a handle previously resulting from counterpart `next_unchecked`. - /// - Further use of the updated leaf edge handle is very dangerous. In particular, - /// if the leaf edge is the first edge of a node, that node and possibly ancestors - /// will be deallocated, while the reference to those nodes in the surviving ancestor - /// is left dangling. - /// The only safe way to proceed with the updated handle is to compare it, drop it, - /// call this method again subject to both preconditions listed in the first point, - /// or call counterpart `next_unchecked` subject to its preconditions. + /// Moves the leaf edge handle to the previous leaf edge and returns the key and value + /// in between, deallocating any node left behind while leaving the corresponding + /// edge in its parent node dangling. + /// + /// # Safety + /// - There must be another KV in the direction travelled. + /// - That leaf edge was not previously returned by counterpart `next_unchecked` + /// on any copy of the handles being used to traverse the tree. + /// + /// The only safe way to proceed with the updated handle is to compare it, drop it, + /// call this method again subject to its safety conditions, or call counterpart + /// `next_unchecked` subject to its safety conditions. pub unsafe fn next_back_unchecked(&mut self) -> (K, V) { replace(self, |leaf_edge| { let kv = unsafe { next_back_kv_unchecked_dealloc(leaf_edge) }; diff --git a/library/alloc/src/collections/btree/node.rs b/library/alloc/src/collections/btree/node.rs index acc2ae73572ba..772bdf357de78 100644 --- a/library/alloc/src/collections/btree/node.rs +++ b/library/alloc/src/collections/btree/node.rs @@ -186,6 +186,15 @@ impl Root { } } + pub fn node_as_valmut(&mut self) -> NodeRef, K, V, marker::LeafOrInternal> { + NodeRef { + height: self.height, + node: self.node.as_ptr(), + root: ptr::null(), + _marker: PhantomData, + } + } + pub fn into_ref(self) -> NodeRef { NodeRef { height: self.height, @@ -253,9 +262,12 @@ impl Root { /// A reference to a node. /// /// This type has a number of parameters that controls how it acts: -/// - `BorrowType`: This can be `Immut<'a>` or `Mut<'a>` for some `'a` or `Owned`. +/// - `BorrowType`: This can be `Immut<'a>`, `Mut<'a>` or `ValMut<'a>' for some `'a` +/// or `Owned`. /// When this is `Immut<'a>`, the `NodeRef` acts roughly like `&'a Node`, /// when this is `Mut<'a>`, the `NodeRef` acts roughly like `&'a mut Node`, +/// when this is `ValMut<'a>`, the `NodeRef` acts as immutable with respect +/// to keys and tree structure, but allows mutable references to values, /// and when this is `Owned`, the `NodeRef` acts roughly like `Box`. /// - `K` and `V`: These control what types of things are stored in the nodes. /// - `Type`: This can be `Leaf`, `Internal`, or `LeafOrInternal`. When this is @@ -282,6 +294,7 @@ unsafe impl Sync for NodeRef Send for NodeRef, K, V, Type> {} unsafe impl<'a, K: Send + 'a, V: Send + 'a, Type> Send for NodeRef, K, V, Type> {} +unsafe impl<'a, K: Send + 'a, V: Send + 'a, Type> Send for NodeRef, K, V, Type> {} unsafe impl Send for NodeRef {} impl NodeRef { @@ -515,6 +528,22 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { } } +impl<'a, K: 'a, V: 'a, Type> NodeRef, K, V, Type> { + /// Same as the marker::Mut method, but far more dangerous because ValMut-based iterators: + /// - have front and back handles often refering to the same node, so `self` is not unique; + /// - hand out mutable references to parts of these slices to the public. + fn into_slices_mut(self) -> (&'a [K], &'a mut [V]) { + let len = self.len(); + let leaf = self.node.as_ptr(); + // SAFETY: The keys and values of a node must always be initialized up to length. + let keys = unsafe { slice::from_raw_parts(MaybeUninit::first_ptr(&(*leaf).keys), len) }; + let vals = unsafe { + slice::from_raw_parts_mut(MaybeUninit::first_ptr_mut(&mut (*leaf).vals), len) + }; + (keys, vals) + } +} + impl<'a, K, V> NodeRef, K, V, marker::Leaf> { /// Adds a key/value pair to the end of the node. pub fn push(&mut self, key: K, val: V) { @@ -1053,11 +1082,13 @@ impl<'a, K: 'a, V: 'a, NodeType> Handle, K, V, NodeType> let vals = self.node.into_val_slice_mut(); unsafe { vals.get_unchecked_mut(self.idx) } } +} - pub fn into_kv_mut(self) -> (&'a mut K, &'a mut V) { +impl<'a, K, V, NodeType> Handle, K, V, NodeType>, marker::KV> { + pub fn into_kv_valmut(self) -> (&'a K, &'a mut V) { unsafe { let (keys, vals) = self.node.into_slices_mut(); - (keys.get_unchecked_mut(self.idx), vals.get_unchecked_mut(self.idx)) + (keys.get_unchecked(self.idx), vals.get_unchecked_mut(self.idx)) } } } @@ -1558,6 +1589,7 @@ pub mod marker { pub enum Owned {} pub struct Immut<'a>(PhantomData<&'a ()>); pub struct Mut<'a>(PhantomData<&'a mut ()>); + pub struct ValMut<'a>(PhantomData<&'a mut ()>); pub enum KV {} pub enum Edge {} From aa40c028fcb20a47fb214fea2899ff9a8ae88840 Mon Sep 17 00:00:00 2001 From: Arkadiusz Piekarz Date: Wed, 26 Aug 2020 22:03:29 +0200 Subject: [PATCH 03/37] Unstable Book: add links to tracking issues for FFI features --- src/doc/unstable-book/src/language-features/ffi-const.md | 5 +++++ src/doc/unstable-book/src/language-features/ffi-pure.md | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/src/doc/unstable-book/src/language-features/ffi-const.md b/src/doc/unstable-book/src/language-features/ffi-const.md index 9a1ced4033b22..24a304437542d 100644 --- a/src/doc/unstable-book/src/language-features/ffi-const.md +++ b/src/doc/unstable-book/src/language-features/ffi-const.md @@ -1,5 +1,9 @@ # `ffi_const` +The tracking issue for this feature is: [#58328] + +------ + The `#[ffi_const]` attribute applies clang's `const` attribute to foreign functions declarations. @@ -42,6 +46,7 @@ implemented in this way on all of them. It is therefore also worth verifying that the semantics of the C toolchain used to compile the binary being linked against are compatible with those of the `#[ffi_const]`. +[#58328]: https://github.com/rust-lang/rust/issues/58328 [ARM C/C++ compiler]: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/Cacgigch.html [GCC]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-const-function-attribute [IBM ILE C/C++]: https://www.ibm.com/support/knowledgecenter/fr/ssw_ibm_i_71/rzarg/fn_attrib_const.htm diff --git a/src/doc/unstable-book/src/language-features/ffi-pure.md b/src/doc/unstable-book/src/language-features/ffi-pure.md index 7bfd7a378f00b..4aef4eeab5532 100644 --- a/src/doc/unstable-book/src/language-features/ffi-pure.md +++ b/src/doc/unstable-book/src/language-features/ffi-pure.md @@ -1,5 +1,9 @@ # `ffi_pure` +The tracking issue for this feature is: [#58329] + +------ + The `#[ffi_pure]` attribute applies clang's `pure` attribute to foreign functions declarations. @@ -46,6 +50,7 @@ that the semantics of the C toolchain used to compile the binary being linked against are compatible with those of the `#[ffi_pure]`. +[#58329]: https://github.com/rust-lang/rust/issues/58329 [ARM C/C++ compiler]: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/Cacigdac.html [GCC]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute [IBM ILE C/C++]: https://www.ibm.com/support/knowledgecenter/fr/ssw_ibm_i_71/rzarg/fn_attrib_pure.htm From f03d0b38d6a33a64307d83f8ddd3df8ef57ca537 Mon Sep 17 00:00:00 2001 From: mental32 Date: Thu, 27 Aug 2020 19:19:29 +0100 Subject: [PATCH 04/37] `impl Rc::new_cyclic` --- library/alloc/src/rc.rs | 34 ++++++++++++++++++ library/alloc/src/rc/tests.rs | 66 +++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 1046397f4be60..76266d77bb0d2 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -325,6 +325,40 @@ impl Rc { ) } + /// Constructs a new `Rc` using a weak reference to itself. Attempting + /// to upgrade the weak reference before this function retuns will result + /// in a `None` value. However, the weak reference may be cloned freely and + /// stored for use at a later time. + #[inline] + #[unstable(feature = "arc_new_cyclic", issue = "75861")] + pub fn new_cyclic(data_fn: impl FnOnce(&Weak) -> T) -> Rc { + let uninit_ptr: NonNull<_> = Box::leak(box RcBox { + strong: Cell::new(0), + weak: Cell::new(1), + value: mem::MaybeUninit::::uninit(), + }) + .into(); + + let init_ptr: NonNull> = uninit_ptr.cast(); + + let weak = Weak { ptr: init_ptr }; + + let data = data_fn(&weak); + + unsafe { + let inner = init_ptr.as_ptr(); + ptr::write(&raw mut (*inner).value, data); + + let prev_value = (*inner).strong.get(); + debug_assert_eq!(prev_value, 0, "No prior strong references should exist"); + (*inner).strong.set(1); + } + + let strong = Rc::from_inner(init_ptr); + mem::forget(weak); + strong + } + /// Constructs a new `Rc` with uninitialized contents. /// /// # Examples diff --git a/library/alloc/src/rc/tests.rs b/library/alloc/src/rc/tests.rs index e88385faf4fd4..fed48a59f809e 100644 --- a/library/alloc/src/rc/tests.rs +++ b/library/alloc/src/rc/tests.rs @@ -434,3 +434,69 @@ fn test_array_from_slice() { let a: Result, _> = r.clone().try_into(); assert!(a.is_err()); } + +#[test] +fn test_rc_cyclic_with_zero_refs() { + struct ZeroRefs { + inner: Weak, + } + + let zero_refs = Rc::new_cyclic(|inner| { + assert_eq!(inner.strong_count(), 0); + assert!(inner.upgrade().is_none()); + ZeroRefs { inner: Weak::new() } + }); + + assert_eq!(Rc::strong_count(&zero_refs), 1); + assert_eq!(Rc::weak_count(&zero_refs), 0); + assert_eq!(zero_refs.inner.strong_count(), 0); + assert_eq!(zero_refs.inner.weak_count(), 0); +} + +#[test] +fn test_rc_cyclic_with_one_ref() { + struct OneRef { + inner: Weak, + } + + let one_ref = Rc::new_cyclic(|inner| { + assert_eq!(inner.strong_count(), 0); + assert!(inner.upgrade().is_none()); + OneRef { inner: inner.clone() } + }); + + assert_eq!(Rc::strong_count(&one_ref), 1); + assert_eq!(Rc::weak_count(&one_ref), 1); + + let one_ref2 = Weak::upgrade(&one_ref.inner).unwrap(); + assert!(Rc::ptr_eq(&one_ref, &one_ref2)); + + assert_eq!(one_ref.inner.strong_count(), 2); + assert_eq!(one_ref.inner.weak_count(), 1); +} + +#[test] +fn test_rc_cyclic_with_two_ref() { + struct TwoRefs { + inner: Weak, + inner1: Weak, + } + + let two_refs = Rc::new_cyclic(|inner| { + assert_eq!(inner.strong_count(), 0); + assert!(inner.upgrade().is_none()); + TwoRefs { inner: inner.clone(), inner1: inner.clone() } + }); + + assert_eq!(Rc::strong_count(&two_refs), 1); + assert_eq!(Rc::weak_count(&two_refs), 2); + + let two_ref3 = Weak::upgrade(&two_refs.inner).unwrap(); + assert!(Rc::ptr_eq(&two_refs, &two_ref3)); + + let two_ref2 = Weak::upgrade(&two_refs.inner1).unwrap(); + assert!(Rc::ptr_eq(&two_refs, &two_ref2)); + + assert_eq!(Rc::strong_count(&two_refs), 3); + assert_eq!(Rc::weak_count(&two_refs), 2); +} From 42fb27001e07e832cb40604c7daeaa6aada07675 Mon Sep 17 00:00:00 2001 From: mental Date: Sat, 29 Aug 2020 07:39:03 +0100 Subject: [PATCH 05/37] typo Co-authored-by: Andrew Hickman --- library/alloc/src/rc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 76266d77bb0d2..7dbdc8f6017fe 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -326,7 +326,7 @@ impl Rc { } /// Constructs a new `Rc` using a weak reference to itself. Attempting - /// to upgrade the weak reference before this function retuns will result + /// to upgrade the weak reference before this function returns will result /// in a `None` value. However, the weak reference may be cloned freely and /// stored for use at a later time. #[inline] From bb5e79cbd11dbb8aa91840108b67085e5d5d04f8 Mon Sep 17 00:00:00 2001 From: Ivan Tham Date: Sat, 29 Aug 2020 18:47:11 +0800 Subject: [PATCH 06/37] Link vec doc to & reference It is not always obvious that people could see the docs for `&` especially for beginners, it also helps learnability. --- library/alloc/src/vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/vec.rs b/library/alloc/src/vec.rs index b4ad238680f79..5e6493c27ae0f 100644 --- a/library/alloc/src/vec.rs +++ b/library/alloc/src/vec.rs @@ -159,7 +159,7 @@ use crate::raw_vec::RawVec; /// # Slicing /// /// A `Vec` can be mutable. Slices, on the other hand, are read-only objects. -/// To get a slice, use `&`. Example: +/// To get a slice, use [`&`][prim@reference]. Example: /// /// ``` /// fn read_slice(slice: &[usize]) { From 1b1935452f26593d07fa17ccbe3308901dda891b Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 16:32:17 -0400 Subject: [PATCH 07/37] [WIP] Fix intra-doc links on pub re-exports This removes the incorrect error, but doesn't show the documentation anywhere. --- src/librustdoc/passes/collect_intra_doc_links.rs | 4 ++++ src/test/rustdoc/intra-link-pub-use.rs | 8 ++++++++ 2 files changed, 12 insertions(+) create mode 100644 src/test/rustdoc/intra-link-pub-use.rs diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 65d116b9c670c..be6a7e25c881d 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -578,6 +578,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { let parent_node = if item.is_fake() { // FIXME: is this correct? None + // If we're documenting the crate root itself, it has no parent. Use the root instead. + } else if item.def_id.is_top_level_module() { + Some(item.def_id) } else { let mut current = item.def_id; // The immediate parent might not always be a module. @@ -589,6 +592,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } current = parent; } else { + debug!("{:?} has no parent (kind={:?}, original was {:?})", current, self.cx.tcx.def_kind(current), item.def_id); break None; } } diff --git a/src/test/rustdoc/intra-link-pub-use.rs b/src/test/rustdoc/intra-link-pub-use.rs new file mode 100644 index 0000000000000..a5ceeafc80abc --- /dev/null +++ b/src/test/rustdoc/intra-link-pub-use.rs @@ -0,0 +1,8 @@ +#![deny(intra_doc_link_resolution_failure)] + +/// [std::env] [g] +// @has intra_link_pub_use/index.html '//a[@href="https://doc.rust-lang.org/nightly/std/env/fn.var.html"]' "std::env" +// @has - '//a[@href="../intra_link_pub_use/fn.f.html"]' "g" +pub use f as g; + +pub fn f() {} From 20a68666d89a85e14021f656233a086e46ff8a34 Mon Sep 17 00:00:00 2001 From: Ivan Tham Date: Sun, 30 Aug 2020 09:17:22 +0800 Subject: [PATCH 08/37] Try removing [prim@reference] --- library/alloc/src/vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/vec.rs b/library/alloc/src/vec.rs index 5e6493c27ae0f..35d64d85010c3 100644 --- a/library/alloc/src/vec.rs +++ b/library/alloc/src/vec.rs @@ -159,7 +159,7 @@ use crate::raw_vec::RawVec; /// # Slicing /// /// A `Vec` can be mutable. Slices, on the other hand, are read-only objects. -/// To get a slice, use [`&`][prim@reference]. Example: +/// To get a slice, use [`&`]. Example: /// /// ``` /// fn read_slice(slice: &[usize]) { From e885f00f24aab657b3a9835818fc96e638e7fb21 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 30 Aug 2020 08:44:20 -0400 Subject: [PATCH 09/37] Comment out test for generated docs until rustdoc changes its behavior around documenting re-exports --- src/librustdoc/passes/collect_intra_doc_links.rs | 7 ++++++- src/test/rustdoc/intra-link-pub-use.rs | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index be6a7e25c881d..55d7974a9ef6c 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -592,7 +592,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } current = parent; } else { - debug!("{:?} has no parent (kind={:?}, original was {:?})", current, self.cx.tcx.def_kind(current), item.def_id); + debug!( + "{:?} has no parent (kind={:?}, original was {:?})", + current, + self.cx.tcx.def_kind(current), + item.def_id + ); break None; } } diff --git a/src/test/rustdoc/intra-link-pub-use.rs b/src/test/rustdoc/intra-link-pub-use.rs index a5ceeafc80abc..f49edea410d73 100644 --- a/src/test/rustdoc/intra-link-pub-use.rs +++ b/src/test/rustdoc/intra-link-pub-use.rs @@ -1,8 +1,17 @@ -#![deny(intra_doc_link_resolution_failure)] +#![deny(broken_intra_doc_links)] /// [std::env] [g] -// @has intra_link_pub_use/index.html '//a[@href="https://doc.rust-lang.org/nightly/std/env/fn.var.html"]' "std::env" -// @has - '//a[@href="../intra_link_pub_use/fn.f.html"]' "g" +// FIXME: This can't be tested because rustdoc doesn't show documentation on pub re-exports. +// Until then, comment out the `htmldocck` test. +// This test still does something; namely check that no incorrect errors are emitted when +// documenting the re-export. + +// @has intra_link_pub_use/index.html +// @ has - '//a[@href="https://doc.rust-lang.org/nightly/std/env/fn.var.html"]' "std::env" +// @ has - '//a[@href="../intra_link_pub_use/fn.f.html"]' "g" pub use f as g; +/// [std::env] +extern crate self as _; + pub fn f() {} From d7150154fa5c35c0b570570f156ba3a5cc6dfb1d Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 30 Aug 2020 12:06:40 -0400 Subject: [PATCH 10/37] Improve tests Now this actually tests the links are generated correctly --- .../rustdoc/auxiliary/intra-link-pub-use.rs | 4 ++++ src/test/rustdoc/intra-link-pub-use.rs | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 src/test/rustdoc/auxiliary/intra-link-pub-use.rs diff --git a/src/test/rustdoc/auxiliary/intra-link-pub-use.rs b/src/test/rustdoc/auxiliary/intra-link-pub-use.rs new file mode 100644 index 0000000000000..a4db2ffc445f8 --- /dev/null +++ b/src/test/rustdoc/auxiliary/intra-link-pub-use.rs @@ -0,0 +1,4 @@ +#![crate_name = "inner"] + +/// Documentation, including a link to [std::ptr] +pub fn f() {} diff --git a/src/test/rustdoc/intra-link-pub-use.rs b/src/test/rustdoc/intra-link-pub-use.rs index f49edea410d73..dd52249abc6d0 100644 --- a/src/test/rustdoc/intra-link-pub-use.rs +++ b/src/test/rustdoc/intra-link-pub-use.rs @@ -1,17 +1,27 @@ +// aux-build: intra-link-pub-use.rs #![deny(broken_intra_doc_links)] +#![crate_name = "outer"] + +extern crate inner; + +/// [mod@std::env] [g] -/// [std::env] [g] // FIXME: This can't be tested because rustdoc doesn't show documentation on pub re-exports. // Until then, comment out the `htmldocck` test. // This test still does something; namely check that no incorrect errors are emitted when // documenting the re-export. -// @has intra_link_pub_use/index.html +// @has outer/index.html // @ has - '//a[@href="https://doc.rust-lang.org/nightly/std/env/fn.var.html"]' "std::env" -// @ has - '//a[@href="../intra_link_pub_use/fn.f.html"]' "g" +// @ has - '//a[@href="../outer/fn.f.html"]' "g" pub use f as g; +// FIXME: same as above /// [std::env] extern crate self as _; -pub fn f() {} +// Make sure the documentation is actually correct by documenting an inlined re-export +/// [mod@std::env] +// @has outer/fn.f.html +// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/env/index.html"]' "std::env" +pub use inner::f; From 81e85ce76d6630a8f87f81df2470e76fbe73de7d Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 30 Aug 2020 21:59:43 +0200 Subject: [PATCH 11/37] Move to Arc::clone(&x) over x.clone() in library/std --- library/std/src/collections/hash/map.rs | 4 ++-- library/std/src/sync/barrier.rs | 4 ++-- library/std/src/sync/condvar.rs | 18 +++++++++--------- library/std/src/sync/mutex.rs | 8 ++++---- library/std/src/sync/rwlock.rs | 4 ++-- library/std/src/sys_common/poison.rs | 4 ++-- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/library/std/src/collections/hash/map.rs b/library/std/src/collections/hash/map.rs index 70f7214e2f1d7..56f63002b7f11 100644 --- a/library/std/src/collections/hash/map.rs +++ b/library/std/src/collections/hash/map.rs @@ -2381,7 +2381,7 @@ impl<'a, K, V> OccupiedEntry<'a, K, V> { /// use std::rc::Rc; /// /// let mut map: HashMap, u32> = HashMap::new(); - /// let mut known_strings: Vec> = Vec::new(); + /// let known_strings: Vec> = Vec::new(); /// /// // Initialise known strings, run program, etc. /// @@ -2389,7 +2389,7 @@ impl<'a, K, V> OccupiedEntry<'a, K, V> { /// /// fn reclaim_memory(map: &mut HashMap, u32>, known_strings: &[Rc] ) { /// for s in known_strings { - /// if let Entry::Occupied(entry) = map.entry(s.clone()) { + /// if let Entry::Occupied(entry) = map.entry(Rc::clone(s)) { /// // Replaces the entry's key with our version of it in `known_strings`. /// entry.replace_key(); /// } diff --git a/library/std/src/sync/barrier.rs b/library/std/src/sync/barrier.rs index 23c989fd2fdfb..61f097ccf250c 100644 --- a/library/std/src/sync/barrier.rs +++ b/library/std/src/sync/barrier.rs @@ -13,7 +13,7 @@ use crate::sync::{Condvar, Mutex}; /// let mut handles = Vec::with_capacity(10); /// let barrier = Arc::new(Barrier::new(10)); /// for _ in 0..10 { -/// let c = barrier.clone(); +/// let c = Arc::clone(&barrier); /// // The same messages will be printed together. /// // You will NOT see any interleaving. /// handles.push(thread::spawn(move|| { @@ -110,7 +110,7 @@ impl Barrier { /// let mut handles = Vec::with_capacity(10); /// let barrier = Arc::new(Barrier::new(10)); /// for _ in 0..10 { - /// let c = barrier.clone(); + /// let c = Arc::clone(&barrier); /// // The same messages will be printed together. /// // You will NOT see any interleaving. /// handles.push(thread::spawn(move|| { diff --git a/library/std/src/sync/condvar.rs b/library/std/src/sync/condvar.rs index 4efd86aa3ede8..1fbcb0ac08e0f 100644 --- a/library/std/src/sync/condvar.rs +++ b/library/std/src/sync/condvar.rs @@ -33,7 +33,7 @@ impl WaitTimeoutResult { /// use std::time::Duration; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move || { /// let (lock, cvar) = &*pair2; @@ -90,7 +90,7 @@ impl WaitTimeoutResult { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); -/// let pair2 = pair.clone(); +/// let pair2 = Arc::clone(&pair); /// /// // Inside of our lock, spawn a new thread, and then wait for it to start. /// thread::spawn(move|| { @@ -173,7 +173,7 @@ impl Condvar { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -229,7 +229,7 @@ impl Condvar { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(true), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -288,7 +288,7 @@ impl Condvar { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -360,7 +360,7 @@ impl Condvar { /// use std::time::Duration; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -429,7 +429,7 @@ impl Condvar { /// use std::time::Duration; /// /// let pair = Arc::new((Mutex::new(true), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -493,7 +493,7 @@ impl Condvar { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; @@ -533,7 +533,7 @@ impl Condvar { /// use std::thread; /// /// let pair = Arc::new((Mutex::new(false), Condvar::new())); - /// let pair2 = pair.clone(); + /// let pair2 = Arc::clone(&pair); /// /// thread::spawn(move|| { /// let (lock, cvar) = &*pair2; diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index d7a4f00305c71..39a5a455fe4d5 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -85,7 +85,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// use std::thread; /// /// let lock = Arc::new(Mutex::new(0_u32)); -/// let lock2 = lock.clone(); +/// let lock2 = Arc::clone(&lock); /// /// let _ = thread::spawn(move || -> () { /// // This thread will acquire the mutex first, unwrapping the result of @@ -256,7 +256,7 @@ impl Mutex { /// use std::thread; /// /// let mutex = Arc::new(Mutex::new(0)); - /// let c_mutex = mutex.clone(); + /// let c_mutex = Arc::clone(&mutex); /// /// thread::spawn(move || { /// *c_mutex.lock().unwrap() = 10; @@ -292,7 +292,7 @@ impl Mutex { /// use std::thread; /// /// let mutex = Arc::new(Mutex::new(0)); - /// let c_mutex = mutex.clone(); + /// let c_mutex = Arc::clone(&mutex); /// /// thread::spawn(move || { /// let mut lock = c_mutex.try_lock(); @@ -328,7 +328,7 @@ impl Mutex { /// use std::thread; /// /// let mutex = Arc::new(Mutex::new(0)); - /// let c_mutex = mutex.clone(); + /// let c_mutex = Arc::clone(&mutex); /// /// let _ = thread::spawn(move || { /// let _lock = c_mutex.lock().unwrap(); diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 586093c916dea..b7f135f5c8af4 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -162,7 +162,7 @@ impl RwLock { /// use std::thread; /// /// let lock = Arc::new(RwLock::new(1)); - /// let c_lock = lock.clone(); + /// let c_lock = Arc::clone(&lock); /// /// let n = lock.read().unwrap(); /// assert_eq!(*n, 1); @@ -318,7 +318,7 @@ impl RwLock { /// use std::thread; /// /// let lock = Arc::new(RwLock::new(0)); - /// let c_lock = lock.clone(); + /// let c_lock = Arc::clone(&lock); /// /// let _ = thread::spawn(move || { /// let _lock = c_lock.write().unwrap(); diff --git a/library/std/src/sys_common/poison.rs b/library/std/src/sys_common/poison.rs index 3f079da9098c7..2ab2c700a1bf1 100644 --- a/library/std/src/sys_common/poison.rs +++ b/library/std/src/sys_common/poison.rs @@ -65,7 +65,7 @@ pub struct Guard { /// let mutex = Arc::new(Mutex::new(1)); /// /// // poison the mutex -/// let c_mutex = mutex.clone(); +/// let c_mutex = Arc::clone(&mutex); /// let _ = thread::spawn(move || { /// let mut data = c_mutex.lock().unwrap(); /// *data = 2; @@ -168,7 +168,7 @@ impl PoisonError { /// let mutex = Arc::new(Mutex::new(HashSet::new())); /// /// // poison the mutex - /// let c_mutex = mutex.clone(); + /// let c_mutex = Arc::clone(&mutex); /// let _ = thread::spawn(move || { /// let mut data = c_mutex.lock().unwrap(); /// data.insert(10); From 6b75e3d11b89671df0a3f284252584414bfe17d9 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 30 Aug 2020 22:14:17 +0200 Subject: [PATCH 12/37] Move to Arc::clone(&x) over x.clone() in library/core --- library/core/src/pin.rs | 2 +- library/core/src/sync/atomic.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/pin.rs b/library/core/src/pin.rs index 8f60c4787d459..fc17f64879a50 100644 --- a/library/core/src/pin.rs +++ b/library/core/src/pin.rs @@ -541,7 +541,7 @@ impl Pin

{ /// use std::pin::Pin; /// /// fn move_pinned_rc(mut x: Rc) { - /// let pinned = unsafe { Pin::new_unchecked(x.clone()) }; + /// let pinned = unsafe { Pin::new_unchecked(Rc::clone(&x)) }; /// { /// let p: Pin<&T> = pinned.as_ref(); /// // This should mean the pointee can never move again. diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 1aec7e1b5f871..38eabaaa396e0 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -76,7 +76,7 @@ //! fn main() { //! let spinlock = Arc::new(AtomicUsize::new(1)); //! -//! let spinlock_clone = spinlock.clone(); +//! let spinlock_clone = Arc::clone(&spinlock); //! let thread = thread::spawn(move|| { //! spinlock_clone.store(0, Ordering::SeqCst); //! }); From 864f2dce068268381c0c4cd807a411dbfac16178 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 20 Aug 2020 02:37:00 -0700 Subject: [PATCH 13/37] Refactor byteorder to std in rustc_middle Use std::io::{Read, Write} and {to, from}_{le, be}_bytes methods in order to remove byteorder from librustc_middle's dependency graph. --- Cargo.lock | 1 - compiler/rustc_middle/Cargo.toml | 1 - compiler/rustc_middle/src/mir/interpret/mod.rs | 16 +++++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ffc1f0dec1d9a..42180e9c3b518 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3726,7 +3726,6 @@ name = "rustc_middle" version = "0.0.0" dependencies = [ "bitflags", - "byteorder", "chalk-ir", "measureme", "polonius-engine", diff --git a/compiler/rustc_middle/Cargo.toml b/compiler/rustc_middle/Cargo.toml index 5a82cbf2997df..d555fbd5d29b5 100644 --- a/compiler/rustc_middle/Cargo.toml +++ b/compiler/rustc_middle/Cargo.toml @@ -26,7 +26,6 @@ rustc_index = { path = "../rustc_index" } rustc_serialize = { path = "../rustc_serialize" } rustc_ast = { path = "../rustc_ast" } rustc_span = { path = "../rustc_span" } -byteorder = { version = "1.3" } chalk-ir = "0.14.0" smallvec = { version = "1.0", features = ["union", "may_dangle"] } measureme = "0.7.1" diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs index 0dc3d6e344a7e..42a142a188b5e 100644 --- a/compiler/rustc_middle/src/mir/interpret/mod.rs +++ b/compiler/rustc_middle/src/mir/interpret/mod.rs @@ -98,10 +98,10 @@ mod value; use std::convert::TryFrom; use std::fmt; use std::io; +use std::io::{Read, Write}; use std::num::NonZeroU32; use std::sync::atomic::{AtomicU32, Ordering}; -use byteorder::{BigEndian, LittleEndian, ReadBytesExt, WriteBytesExt}; use rustc_ast::LitKind; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sync::{HashMapExt, Lock}; @@ -560,18 +560,20 @@ pub fn write_target_uint( mut target: &mut [u8], data: u128, ) -> Result<(), io::Error> { - let len = target.len(); match endianness { - Endian::Little => target.write_uint128::(data, len), - Endian::Big => target.write_uint128::(data, len), - } + Endian::Little => target.write(&data.to_le_bytes())?, + Endian::Big => target.write(&data.to_be_bytes())?, + }; + Ok(()) } #[inline] pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result { + let mut buf = [0; 16]; + source.read(&mut buf)?; match endianness { - Endian::Little => source.read_uint128::(source.len()), - Endian::Big => source.read_uint128::(source.len()), + Endian::Little => Ok(u128::from_le_bytes(buf)), + Endian::Big => Ok(u128::from_be_bytes(buf)), } } From fd959c1b8b627e17ef30fd17e0e5f783f3010107 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 20 Aug 2020 05:32:14 -0700 Subject: [PATCH 14/37] Remove reference to byteorder limits --- .../rustc_middle/src/mir/interpret/allocation.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_middle/src/mir/interpret/allocation.rs b/compiler/rustc_middle/src/mir/interpret/allocation.rs index 505939d56ed32..ee1ea816e0192 100644 --- a/compiler/rustc_middle/src/mir/interpret/allocation.rs +++ b/compiler/rustc_middle/src/mir/interpret/allocation.rs @@ -345,10 +345,8 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Reads a *non-ZST* scalar. /// - /// ZSTs can't be read for two reasons: - /// * byte-order cannot work with zero-element buffers; - /// * in order to obtain a `Pointer`, we need to check for ZSTness anyway due to integer - /// pointers being valid for ZSTs. + /// ZSTs can't be read because in order to obtain a `Pointer`, we need to check + /// for ZSTness anyway due to integer pointers being valid for ZSTs. /// /// It is the caller's responsibility to check bounds and alignment beforehand. /// Most likely, you want to call `InterpCx::read_scalar` instead of this method. @@ -397,10 +395,8 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Writes a *non-ZST* scalar. /// - /// ZSTs can't be read for two reasons: - /// * byte-order cannot work with zero-element buffers; - /// * in order to obtain a `Pointer`, we need to check for ZSTness anyway due to integer - /// pointers being valid for ZSTs. + /// ZSTs can't be read because in order to obtain a `Pointer`, we need to check + /// for ZSTness anyway due to integer pointers being valid for ZSTs. /// /// It is the caller's responsibility to check bounds and alignment beforehand. /// Most likely, you want to call `InterpCx::write_scalar` instead of this method. From 28d31001af32c8e54367e7080b66d19e057d8694 Mon Sep 17 00:00:00 2001 From: Jubilee <46493976+workingjubilee@users.noreply.github.com> Date: Thu, 20 Aug 2020 15:55:02 -0700 Subject: [PATCH 15/37] Be explicit that we're handling bytes Co-authored-by: Aleksey Kladov --- compiler/rustc_middle/src/mir/interpret/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs index 42a142a188b5e..75db4dac8006a 100644 --- a/compiler/rustc_middle/src/mir/interpret/mod.rs +++ b/compiler/rustc_middle/src/mir/interpret/mod.rs @@ -569,7 +569,7 @@ pub fn write_target_uint( #[inline] pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result { - let mut buf = [0; 16]; + let mut buf = [0u8; std::mem::size_of::()]; source.read(&mut buf)?; match endianness { Endian::Little => Ok(u128::from_le_bytes(buf)), From 91c9351559e2bf51664b876d3f3e2239492c55b5 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 21 Aug 2020 00:55:33 -0700 Subject: [PATCH 16/37] Explain contract of {read, write}_target_uint --- compiler/rustc_middle/src/mir/interpret/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs index 75db4dac8006a..11dc71ce08afd 100644 --- a/compiler/rustc_middle/src/mir/interpret/mod.rs +++ b/compiler/rustc_middle/src/mir/interpret/mod.rs @@ -560,17 +560,23 @@ pub fn write_target_uint( mut target: &mut [u8], data: u128, ) -> Result<(), io::Error> { + // This u128 holds an "any-size uint" (since smaller uints can fits in it) + // So we do not write all bytes of the u128, just the "payload". match endianness { Endian::Little => target.write(&data.to_le_bytes())?, Endian::Big => target.write(&data.to_be_bytes())?, }; + debug_assert!(target.len() == 0); // We should have filled the target buffer. Ok(()) } #[inline] pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result { + // This u128 holds an "any-size uint" (since smaller uints can fits in it) let mut buf = [0u8; std::mem::size_of::()]; + // So we do not read exactly 16 bytes into the u128, just the "payload". source.read(&mut buf)?; + debug_assert!(source.len() == 0); // We should have consumed the source buffer. match endianness { Endian::Little => Ok(u128::from_le_bytes(buf)), Endian::Big => Ok(u128::from_be_bytes(buf)), From b68831423a45503f2ae2c4b410e08dbf82165558 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sat, 22 Aug 2020 03:54:15 -0700 Subject: [PATCH 17/37] Fix big endian read/write Co-authored-by: matthewjasper --- compiler/rustc_middle/src/mir/interpret/mod.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs index 11dc71ce08afd..47a6ad86cadb1 100644 --- a/compiler/rustc_middle/src/mir/interpret/mod.rs +++ b/compiler/rustc_middle/src/mir/interpret/mod.rs @@ -564,7 +564,7 @@ pub fn write_target_uint( // So we do not write all bytes of the u128, just the "payload". match endianness { Endian::Little => target.write(&data.to_le_bytes())?, - Endian::Big => target.write(&data.to_be_bytes())?, + Endian::Big => target.write(&data.to_be_bytes()[16 - target.len()..])?, }; debug_assert!(target.len() == 0); // We should have filled the target buffer. Ok(()) @@ -575,12 +575,18 @@ pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result()]; // So we do not read exactly 16 bytes into the u128, just the "payload". - source.read(&mut buf)?; + let uint = match endianness { + Endian::Little => { + source.read(&mut buf)?; + Ok(u128::from_le_bytes(buf)) + } + Endian::Big => { + source.read(&mut buf[16 - source.len()..])?; + Ok(u128::from_be_bytes(buf)) + } + }; debug_assert!(source.len() == 0); // We should have consumed the source buffer. - match endianness { - Endian::Little => Ok(u128::from_le_bytes(buf)), - Endian::Big => Ok(u128::from_be_bytes(buf)), - } + uint } //////////////////////////////////////////////////////////////////////////////// From ec0924f9649ea472782df6d21595714cb594f038 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:07:33 +0200 Subject: [PATCH 18/37] do not apply DerefMut on union field --- compiler/rustc_typeck/src/check/place_op.rs | 16 +++++++++++++++- src/test/ui/union/union-deref.rs | 13 +++++++++++++ src/test/ui/union/union-deref.stderr | 11 +++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/union/union-deref.rs create mode 100644 src/test/ui/union/union-deref.stderr diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index 4bef9aecd2ecb..be2bc491c311b 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -193,7 +193,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Convert auto-derefs, indices, etc of an expression from `Deref` and `Index` /// into `DerefMut` and `IndexMut` respectively. /// - /// This is a second pass of typechecking derefs/indices. We need this we do not + /// This is a second pass of typechecking derefs/indices. We need this because we do not /// always know whether a place needs to be mutable or not in the first pass. /// This happens whether there is an implicit mutable reborrow, e.g. when the type /// is used as the receiver of a method call. @@ -236,6 +236,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let ty::Ref(region, _, mutbl) = method.sig.output().kind { *deref = OverloadedDeref { region, mutbl }; } + // If this is a union field, also throw an error. + // Union fields should not get mutable auto-deref'd (see RFC 2514). + if let hir::ExprKind::Field(ref outer_expr, _) = expr.kind { + let ty = self.node_ty(outer_expr.hir_id); + if ty.ty_adt_def().map_or(false, |adt| adt.is_union()) { + let mut err = self.tcx.sess.struct_span_err( + expr.span, + "not automatically applying `DerefMut` on union field", + ); + err.help("writing to this field calls the destructor for the old value"); + err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); + err.emit(); + } + } } } source = adjustment.target; diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs new file mode 100644 index 0000000000000..61bbe73354f04 --- /dev/null +++ b/src/test/ui/union/union-deref.rs @@ -0,0 +1,13 @@ +//! Test the part of RFC 2514 that is about not applying `DerefMut` coercions +//! of union fields. +#![feature(untagged_unions)] + +use std::mem::ManuallyDrop; + +union U { x:(), f: ManuallyDrop<(T,)> } + +fn main() { + let mut u : U> = U { x: () }; + unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles + unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field +} diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr new file mode 100644 index 0000000000000..66cc90cbd3d01 --- /dev/null +++ b/src/test/ui/union/union-deref.stderr @@ -0,0 +1,11 @@ +error: not automatically applying `DerefMut` on union field + --> $DIR/union-deref.rs:12:14 + | +LL | unsafe { u.f.0 = Vec::new() }; + | ^^^ + | + = help: writing to this field calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to previous error + From 44defaea3a2dd2e7e40336d3609df12b83db424a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:24:10 +0200 Subject: [PATCH 19/37] also detect DerefMut in nested union fields --- compiler/rustc_typeck/src/check/place_op.rs | 29 ++++++++++++--------- src/test/ui/union/union-deref.rs | 10 +++++-- src/test/ui/union/union-deref.stderr | 13 +++++++-- 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index be2bc491c311b..54b3fdd837e66 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -211,13 +211,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { debug!("convert_place_derefs_to_mutable: exprs={:?}", exprs); // Fix up autoderefs and derefs. + let mut inside_union = false; for (i, &expr) in exprs.iter().rev().enumerate() { debug!("convert_place_derefs_to_mutable: i={} expr={:?}", i, expr); + let mut source = self.node_ty(expr.hir_id); + if matches!(expr.kind, hir::ExprKind::Unary(hir::UnOp::UnDeref, _)) { + // Clear previous flag; after a pointer indirection it does not apply any more. + inside_union = false; + } + if source.ty_adt_def().map_or(false, |adt| adt.is_union()) { + inside_union = true; + } // Fix up the autoderefs. Autorefs can only occur immediately preceding // overloaded place ops, and will be fixed by them in order to get // the correct region. - let mut source = self.node_ty(expr.hir_id); // Do not mutate adjustments in place, but rather take them, // and replace them after mutating them, to avoid having the // typeck results borrowed during (`deref_mut`) method resolution. @@ -238,17 +246,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } // If this is a union field, also throw an error. // Union fields should not get mutable auto-deref'd (see RFC 2514). - if let hir::ExprKind::Field(ref outer_expr, _) = expr.kind { - let ty = self.node_ty(outer_expr.hir_id); - if ty.ty_adt_def().map_or(false, |adt| adt.is_union()) { - let mut err = self.tcx.sess.struct_span_err( - expr.span, - "not automatically applying `DerefMut` on union field", - ); - err.help("writing to this field calls the destructor for the old value"); - err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); - err.emit(); - } + if inside_union { + let mut err = self.tcx.sess.struct_span_err( + expr.span, + "not automatically applying `DerefMut` on union field", + ); + err.help("writing to this field calls the destructor for the old value"); + err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); + err.emit(); } } } diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index 61bbe73354f04..4578110cd54eb 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -4,10 +4,16 @@ use std::mem::ManuallyDrop; -union U { x:(), f: ManuallyDrop<(T,)> } +union U1 { x:(), f: ManuallyDrop<(T,)> } + +union U2 { x:(), f: (ManuallyDrop<(T,)>,) } fn main() { - let mut u : U> = U { x: () }; + let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + + let mut u : U2> = U2 { x: () }; + unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles + unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 66cc90cbd3d01..93374436d2f66 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -1,5 +1,5 @@ error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:12:14 + --> $DIR/union-deref.rs:14:14 | LL | unsafe { u.f.0 = Vec::new() }; | ^^^ @@ -7,5 +7,14 @@ LL | unsafe { u.f.0 = Vec::new() }; = help: writing to this field calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: aborting due to previous error +error: not automatically applying `DerefMut` on union field + --> $DIR/union-deref.rs:18:14 + | +LL | unsafe { u.f.0.0 = Vec::new() }; + | ^^^^^^^ + | + = help: writing to this field calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to 2 previous errors From 97974e3cabefe725b6bea24c20e5fb9709e08a02 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 16 Aug 2020 10:32:37 +0200 Subject: [PATCH 20/37] only emit error for ManuallyDrop derefs --- compiler/rustc_typeck/src/check/place_op.rs | 14 +++++++++----- src/test/ui/union/union-deref.rs | 5 +++-- src/test/ui/union/union-deref.stderr | 12 ++++++------ 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_typeck/src/check/place_op.rs b/compiler/rustc_typeck/src/check/place_op.rs index 54b3fdd837e66..c6d5c934a7e3a 100644 --- a/compiler/rustc_typeck/src/check/place_op.rs +++ b/compiler/rustc_typeck/src/check/place_op.rs @@ -244,14 +244,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let ty::Ref(region, _, mutbl) = method.sig.output().kind { *deref = OverloadedDeref { region, mutbl }; } - // If this is a union field, also throw an error. - // Union fields should not get mutable auto-deref'd (see RFC 2514). - if inside_union { + // If this is a union field, also throw an error for `DerefMut` of `ManuallyDrop` (see RFC 2514). + // This helps avoid accidental drops. + if inside_union + && source.ty_adt_def().map_or(false, |adt| adt.is_manually_drop()) + { let mut err = self.tcx.sess.struct_span_err( expr.span, - "not automatically applying `DerefMut` on union field", + "not automatically applying `DerefMut` on `ManuallyDrop` union field", + ); + err.help( + "writing to this reference calls the destructor for the old value", ); - err.help("writing to this field calls the destructor for the old value"); err.help("add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor"); err.emit(); } diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index 4578110cd54eb..d789659a807b5 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -1,3 +1,4 @@ +// ignore-tidy-linelength //! Test the part of RFC 2514 that is about not applying `DerefMut` coercions //! of union fields. #![feature(untagged_unions)] @@ -11,9 +12,9 @@ union U2 { x:(), f: (ManuallyDrop<(T,)>,) } fn main() { let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles - unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field let mut u : U2> = U2 { x: () }; unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles - unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on union field + unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 93374436d2f66..03428489318c0 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -1,19 +1,19 @@ -error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:14:14 +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:15:14 | LL | unsafe { u.f.0 = Vec::new() }; | ^^^ | - = help: writing to this field calls the destructor for the old value + = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: not automatically applying `DerefMut` on union field - --> $DIR/union-deref.rs:18:14 +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:19:14 | LL | unsafe { u.f.0.0 = Vec::new() }; | ^^^^^^^ | - = help: writing to this field calls the destructor for the old value + = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor error: aborting due to 2 previous errors From 66b340f5003910bd8b46f6675cd9a491809aa9fe Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 30 Aug 2020 19:12:36 +0200 Subject: [PATCH 21/37] test more ways of mutably accessing a place --- src/test/ui/union/union-deref.rs | 8 ++++++ src/test/ui/union/union-deref.stderr | 38 +++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/test/ui/union/union-deref.rs b/src/test/ui/union/union-deref.rs index d789659a807b5..df598eea9ef0f 100644 --- a/src/test/ui/union/union-deref.rs +++ b/src/test/ui/union/union-deref.rs @@ -13,8 +13,16 @@ fn main() { let mut u : U1> = U1 { x: () }; unsafe { (*u.f).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { &mut (*u.f).0 }; // explicit deref, this compiles + unsafe { &mut u.f.0 }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { (*u.f).0.push(0) }; // explicit deref, this compiles + unsafe { u.f.0.push(0) }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field let mut u : U2> = U2 { x: () }; unsafe { (*u.f.0).0 = Vec::new() }; // explicit deref, this compiles unsafe { u.f.0.0 = Vec::new() }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { &mut (*u.f.0).0 }; // explicit deref, this compiles + unsafe { &mut u.f.0.0 }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field + unsafe { (*u.f.0).0.push(0) }; // explicit deref, this compiles + unsafe { u.f.0.0.push(0) }; //~ERROR not automatically applying `DerefMut` on `ManuallyDrop` union field } diff --git a/src/test/ui/union/union-deref.stderr b/src/test/ui/union/union-deref.stderr index 03428489318c0..fb16649767fb7 100644 --- a/src/test/ui/union/union-deref.stderr +++ b/src/test/ui/union/union-deref.stderr @@ -7,14 +7,50 @@ LL | unsafe { u.f.0 = Vec::new() }; = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:17:19 + | +LL | unsafe { &mut u.f.0 }; + | ^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + error: not automatically applying `DerefMut` on `ManuallyDrop` union field --> $DIR/union-deref.rs:19:14 | +LL | unsafe { u.f.0.push(0) }; + | ^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:23:14 + | LL | unsafe { u.f.0.0 = Vec::new() }; | ^^^^^^^ | = help: writing to this reference calls the destructor for the old value = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor -error: aborting due to 2 previous errors +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:25:19 + | +LL | unsafe { &mut u.f.0.0 }; + | ^^^^^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: not automatically applying `DerefMut` on `ManuallyDrop` union field + --> $DIR/union-deref.rs:27:14 + | +LL | unsafe { u.f.0.0.push(0) }; + | ^^^^^^^ + | + = help: writing to this reference calls the destructor for the old value + = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor + +error: aborting due to 6 previous errors From 0f301e8bb40aaf7cbfefb8c16ce3b0a112c6d5c1 Mon Sep 17 00:00:00 2001 From: mental Date: Tue, 1 Sep 2020 09:46:48 +0100 Subject: [PATCH 22/37] Removed [inline] and copied over comments from Arc::new_cyclic --- library/alloc/src/rc.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 7dbdc8f6017fe..1a2dfd2888261 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -329,9 +329,10 @@ impl Rc { /// to upgrade the weak reference before this function returns will result /// in a `None` value. However, the weak reference may be cloned freely and /// stored for use at a later time. - #[inline] #[unstable(feature = "arc_new_cyclic", issue = "75861")] pub fn new_cyclic(data_fn: impl FnOnce(&Weak) -> T) -> Rc { + // Construct the inner in the "uninitialized" state with a single + // weak reference. let uninit_ptr: NonNull<_> = Box::leak(box RcBox { strong: Cell::new(0), weak: Cell::new(1), @@ -343,6 +344,12 @@ impl Rc { let weak = Weak { ptr: init_ptr }; + // It's important we don't give up ownership of the weak pointer, or + // else the memory might be freed by the time `data_fn` returns. If + // we really wanted to pass ownership, we could create an additional + // weak pointer for ourselves, but this would result in additional + // updates to the weak reference count which might not be necessary + // otherwise. let data = data_fn(&weak); unsafe { @@ -355,6 +362,9 @@ impl Rc { } let strong = Rc::from_inner(init_ptr); + + // Strong references should collectively own a shared weak reference, + // so don't run the destructor for our old weak reference. mem::forget(weak); strong } From 8783c62f0c8641ef56094fae13e2fed193b60d1f Mon Sep 17 00:00:00 2001 From: Camelid <37223377+camelid@users.noreply.github.com> Date: Tue, 1 Sep 2020 17:48:15 -0700 Subject: [PATCH 23/37] Add missing link in README --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index a7e23d8ac2caa..6cf9577d3330e 100644 --- a/README.md +++ b/README.md @@ -243,6 +243,8 @@ The Rust community congregates in a few places: If you are interested in contributing to the Rust project, please take a look at the [Getting Started][gettingstarted] guide in the [rustc-dev-guide]. +[rustc-dev-guide]: https://rustc-dev-guide.rust-lang.org + ## License Rust is primarily distributed under the terms of both the MIT license From 89ae59a360b18e34b4cdbec8e53c9a4e05b7db14 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 22:10:18 -0400 Subject: [PATCH 24/37] Remove needless .to_owned() for link --- src/librustdoc/passes/collect_intra_doc_links.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index f497f341e112d..b6707a55959c7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -695,9 +695,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { // This is an anchor to an element of the current page, nothing to do in here! continue; } - (parts[0].to_owned(), Some(parts[1].to_owned())) + (parts[0], Some(parts[1].to_owned())) } else { - (parts[0].to_owned(), None) + (parts[0], None) }; let resolved_self; let mut path_str; From d5495e215559e503e7ad6d3321bb3786184fc22b Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 22:45:44 -0400 Subject: [PATCH 25/37] Refactor `ItemLink` into its own struct --- src/librustdoc/clean/types.rs | 18 +++++++++++++++--- .../passes/collect_intra_doc_links.rs | 4 ++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index a458cdab30303..949790cfe26e1 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -425,10 +425,22 @@ pub struct Attributes { pub cfg: Option>, pub span: Option, /// map from Rust paths to resolved defs and potential URL fragments - pub links: Vec<(String, Option, Option)>, + pub links: Vec, pub inner_docs: bool, } +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] +/// A link that has not yet been rendered. +/// +/// This link will be turned into a rendered link by [`Attributes::links`] +pub struct ItemLink { + /// The original link written in the markdown + pub(crate) link: String, + pub(crate) did: Option, + /// The url fragment to append to the link + pub(crate) fragment: Option, +} + impl Attributes { /// Extracts the content from an attribute `#[doc(cfg(content))]`. pub fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> { @@ -611,8 +623,8 @@ impl Attributes { self.links .iter() - .filter_map(|&(ref s, did, ref fragment)| { - match did { + .filter_map(|ItemLink { link: s, did, fragment }| { + match *did { Some(did) => { if let Some((mut href, ..)) = href(did) { if let Some(ref fragment) = *fragment { diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index b6707a55959c7..a0cffc92ce195 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -904,7 +904,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - item.attrs.links.push((ori_link, None, fragment)) + item.attrs.links.push(ItemLink { link: ori_link, did: None, fragment }); } Some(other) => { report_mismatch(other, Disambiguator::Primitive); @@ -955,7 +955,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } } let id = register_res(cx, res); - item.attrs.links.push((ori_link, Some(id), fragment)); + item.attrs.links.push(ItemLink { link: ori_link, did: Some(id), fragment }); } } From 31a7b6e8323b6e9880f4691e624cf6aa6289f6a7 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 23:05:18 -0400 Subject: [PATCH 26/37] Refactor RenderedLink into its own type --- src/librustdoc/clean/types.rs | 21 ++++++++++++++------- src/librustdoc/html/markdown.rs | 21 +++++++++++---------- src/librustdoc/html/render/mod.rs | 4 ++-- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 949790cfe26e1..d79553a8be8de 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -118,7 +118,7 @@ impl Item { self.attrs.collapsed_doc_value() } - pub fn links(&self) -> Vec<(String, String)> { + pub fn links(&self) -> Vec { self.attrs.links(&self.def_id.krate) } @@ -441,6 +441,13 @@ pub struct ItemLink { pub(crate) fragment: Option, } +pub struct RenderedLink { + /// The text the link was original written as + pub(crate) original_text: String, + /// The URL to put in the `href` + pub(crate) href: String, +} + impl Attributes { /// Extracts the content from an attribute `#[doc(cfg(content))]`. pub fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> { @@ -617,7 +624,7 @@ impl Attributes { /// Gets links as a vector /// /// Cache must be populated before call - pub fn links(&self, krate: &CrateNum) -> Vec<(String, String)> { + pub fn links(&self, krate: &CrateNum) -> Vec { use crate::html::format::href; use crate::html::render::CURRENT_DEPTH; @@ -631,7 +638,7 @@ impl Attributes { href.push_str("#"); href.push_str(fragment); } - Some((s.clone(), href)) + Some(RenderedLink { original_text: s.clone(), href }) } else { None } @@ -651,16 +658,16 @@ impl Attributes { }; // This is a primitive so the url is done "by hand". let tail = fragment.find('#').unwrap_or_else(|| fragment.len()); - Some(( - s.clone(), - format!( + Some(RenderedLink { + original_text: s.clone(), + href: format!( "{}{}std/primitive.{}.html{}", url, if !url.ends_with('/') { "/" } else { "" }, &fragment[..tail], &fragment[tail..] ), - )) + }) } else { panic!("This isn't a primitive?!"); } diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index d54b8ea747899..e6484cd1c2c05 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -34,6 +34,7 @@ use std::fmt::Write; use std::ops::Range; use std::str; +use crate::clean::RenderedLink; use crate::doctest; use crate::html::highlight; use crate::html::toc::TocBuilder; @@ -52,7 +53,7 @@ fn opts() -> Options { pub struct Markdown<'a>( pub &'a str, /// A list of link replacements. - pub &'a [(String, String)], + pub &'a [RenderedLink], /// The current list of used header IDs. pub &'a mut IdMap, /// Whether to allow the use of explicit error codes in doctest lang strings. @@ -78,7 +79,7 @@ pub struct MarkdownHtml<'a>( pub &'a Option, ); /// A tuple struct like `Markdown` that renders only the first paragraph. -pub struct MarkdownSummaryLine<'a>(pub &'a str, pub &'a [(String, String)]); +pub struct MarkdownSummaryLine<'a>(pub &'a str, pub &'a [RenderedLink]); #[derive(Copy, Clone, PartialEq, Debug)] pub enum ErrorCodes { @@ -339,11 +340,11 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { /// Make headings links with anchor IDs and build up TOC. struct LinkReplacer<'a, 'b, I: Iterator>> { inner: I, - links: &'b [(String, String)], + links: &'b [RenderedLink], } impl<'a, 'b, I: Iterator>> LinkReplacer<'a, 'b, I> { - fn new(iter: I, links: &'b [(String, String)]) -> Self { + fn new(iter: I, links: &'b [RenderedLink]) -> Self { LinkReplacer { inner: iter, links } } } @@ -354,8 +355,8 @@ impl<'a, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> fn next(&mut self) -> Option { let event = self.inner.next(); if let Some(Event::Start(Tag::Link(kind, dest, text))) = event { - if let Some(&(_, ref replace)) = self.links.iter().find(|link| link.0 == *dest) { - Some(Event::Start(Tag::Link(kind, replace.to_owned().into(), text))) + if let Some(link) = self.links.iter().find(|link| link.original_text == *dest) { + Some(Event::Start(Tag::Link(kind, link.href.clone().into(), text))) } else { Some(Event::Start(Tag::Link(kind, dest, text))) } @@ -855,8 +856,8 @@ impl Markdown<'_> { return String::new(); } let replacer = |_: &str, s: &str| { - if let Some(&(_, ref replace)) = links.iter().find(|link| &*link.0 == s) { - Some((replace.clone(), s.to_owned())) + if let Some(link) = links.iter().find(|link| &*link.original_text == s) { + Some((link.original_text.clone(), link.href.clone())) } else { None } @@ -933,8 +934,8 @@ impl MarkdownSummaryLine<'_> { } let replacer = |_: &str, s: &str| { - if let Some(&(_, ref replace)) = links.iter().find(|link| &*link.0 == s) { - Some((replace.clone(), s.to_owned())) + if let Some(rendered_link) = links.iter().find(|link| &*link.original_text == s) { + Some((rendered_link.original_text.clone(), rendered_link.href.clone())) } else { None } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index e4aba8963c762..9dc85881482f5 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -63,7 +63,7 @@ use rustc_span::symbol::{sym, Symbol}; use serde::ser::SerializeSeq; use serde::{Serialize, Serializer}; -use crate::clean::{self, AttributesExt, Deprecation, GetDefId, SelfTy, TypeKind}; +use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind}; use crate::config::RenderInfo; use crate::config::RenderOptions; use crate::docfs::{DocFS, PathError}; @@ -1780,7 +1780,7 @@ fn render_markdown( w: &mut Buffer, cx: &Context, md_text: &str, - links: Vec<(String, String)>, + links: Vec, prefix: &str, is_hidden: bool, ) { From 9815010d8f6a46621ee96cb824f9939b9db0ae47 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 5 Jul 2020 23:38:31 -0400 Subject: [PATCH 27/37] Remove disambiguators from link text Related to https://github.com/rust-lang/rust/issues/65354 - Pass through the replacement text to `markdown.rs` - Add some tests - Add a state machine that actually replaces the text when parsing Markdown --- src/librustdoc/clean/types.rs | 16 +++- src/librustdoc/html/markdown.rs | 89 +++++++++++++++---- src/librustdoc/html/render/mod.rs | 3 +- .../passes/collect_intra_doc_links.rs | 22 ++++- src/test/rustdoc/disambiguator_removed.rs | 33 +++++++ 5 files changed, 141 insertions(+), 22 deletions(-) create mode 100644 src/test/rustdoc/disambiguator_removed.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index d79553a8be8de..05c90a7c4034f 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -436,6 +436,11 @@ pub struct Attributes { pub struct ItemLink { /// The original link written in the markdown pub(crate) link: String, + /// The link text displayed in the HTML. + /// + /// This may not be the same as `link` if there was a disambiguator + /// in an intra-doc link (e.g. [`fn@f`]) + pub(crate) link_text: String, pub(crate) did: Option, /// The url fragment to append to the link pub(crate) fragment: Option, @@ -444,6 +449,8 @@ pub struct ItemLink { pub struct RenderedLink { /// The text the link was original written as pub(crate) original_text: String, + /// The text to display in the HTML + pub(crate) new_text: String, /// The URL to put in the `href` pub(crate) href: String, } @@ -630,7 +637,7 @@ impl Attributes { self.links .iter() - .filter_map(|ItemLink { link: s, did, fragment }| { + .filter_map(|ItemLink { link: s, link_text, did, fragment }| { match *did { Some(did) => { if let Some((mut href, ..)) = href(did) { @@ -638,7 +645,11 @@ impl Attributes { href.push_str("#"); href.push_str(fragment); } - Some(RenderedLink { original_text: s.clone(), href }) + Some(RenderedLink { + original_text: s.clone(), + new_text: link_text.clone(), + href, + }) } else { None } @@ -660,6 +671,7 @@ impl Attributes { let tail = fragment.find('#').unwrap_or_else(|| fragment.len()); Some(RenderedLink { original_text: s.clone(), + new_text: link_text.clone(), href: format!( "{}{}std/primitive.{}.html{}", url, diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index e6484cd1c2c05..6d634bac7622d 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -340,29 +340,86 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { /// Make headings links with anchor IDs and build up TOC. struct LinkReplacer<'a, 'b, I: Iterator>> { inner: I, - links: &'b [RenderedLink], + links: &'a [RenderedLink], + shortcut_link: Option<&'b RenderedLink>, } -impl<'a, 'b, I: Iterator>> LinkReplacer<'a, 'b, I> { - fn new(iter: I, links: &'b [RenderedLink]) -> Self { - LinkReplacer { inner: iter, links } +impl<'a, I: Iterator>> LinkReplacer<'a, '_, I> { + fn new(iter: I, links: &'a [RenderedLink]) -> Self { + LinkReplacer { inner: iter, links, shortcut_link: None } } } -impl<'a, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { +impl<'a: 'b, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { type Item = Event<'a>; fn next(&mut self) -> Option { - let event = self.inner.next(); - if let Some(Event::Start(Tag::Link(kind, dest, text))) = event { - if let Some(link) = self.links.iter().find(|link| link.original_text == *dest) { - Some(Event::Start(Tag::Link(kind, link.href.clone().into(), text))) - } else { - Some(Event::Start(Tag::Link(kind, dest, text))) + let mut event = self.inner.next(); + + // Remove disambiguators from shortcut links (`[fn@f]`) + match &mut event { + Some(Event::Start(Tag::Link( + pulldown_cmark::LinkType::ShortcutUnknown, + dest, + title, + ))) => { + debug!("saw start of shortcut link to {} with title {}", dest, title); + let link = if let Some(link) = + self.links.iter().find(|&link| *link.original_text == **dest) + { + // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? + *dest = CowStr::Borrowed(link.href.as_ref()); + Some(link) + } else { + self.links.iter().find(|&link| *link.href == **dest) + }; + if let Some(link) = link { + trace!("it matched"); + assert!(self.shortcut_link.is_none(), "shortcut links cannot be nested"); + self.shortcut_link = Some(link); + } } - } else { - event + Some(Event::End(Tag::Link(pulldown_cmark::LinkType::ShortcutUnknown, dest, _))) => { + debug!("saw end of shortcut link to {}", dest); + if let Some(_link) = self.links.iter().find(|&link| *link.href == **dest) { + assert!(self.shortcut_link.is_some(), "saw closing link without opening tag"); + self.shortcut_link = None; + } + } + // Handle backticks in inline code blocks + Some(Event::Code(text)) => { + trace!("saw code {}", text); + if let Some(link) = self.shortcut_link { + trace!("original text was {}", link.original_text); + if **text == link.original_text[1..link.original_text.len() - 1] { + debug!("replacing {} with {}", text, link.new_text); + *text = link.new_text.clone().into(); + } + } + } + // Replace plain text in links + Some(Event::Text(text)) => { + trace!("saw text {}", text); + if let Some(link) = self.shortcut_link { + trace!("original text was {}", link.original_text); + if **text == *link.original_text { + debug!("replacing {} with {}", text, link.new_text); + *text = link.new_text.clone().into(); + } + } + } + Some(Event::Start(Tag::Link(_, dest, _))) => { + if let Some(link) = self.links.iter().find(|&link| *link.original_text == **dest) { + // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? + *dest = CowStr::Borrowed(link.href.as_ref()); + } + } + // Anything else couldn't have been a valid Rust path + _ => {} } + + // Yield the modified event + event } } @@ -857,7 +914,7 @@ impl Markdown<'_> { } let replacer = |_: &str, s: &str| { if let Some(link) = links.iter().find(|link| &*link.original_text == s) { - Some((link.original_text.clone(), link.href.clone())) + Some((link.href.clone(), link.new_text.clone())) } else { None } @@ -934,8 +991,8 @@ impl MarkdownSummaryLine<'_> { } let replacer = |_: &str, s: &str| { - if let Some(rendered_link) = links.iter().find(|link| &*link.original_text == s) { - Some((rendered_link.original_text.clone(), rendered_link.href.clone())) + if let Some(link) = links.iter().find(|link| &*link.original_text == s) { + Some((link.href.clone(), link.new_text.clone())) } else { None } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 9dc85881482f5..318b10e2af2b3 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -64,8 +64,7 @@ use serde::ser::SerializeSeq; use serde::{Serialize, Serializer}; use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind}; -use crate::config::RenderInfo; -use crate::config::RenderOptions; +use crate::config::{RenderInfo, RenderOptions}; use crate::docfs::{DocFS, PathError}; use crate::doctree; use crate::error::Error; diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index a0cffc92ce195..ff3c19da3cf75 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -685,6 +685,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } + //let had_backticks = ori_link.contains("`"); let link = ori_link.replace("`", ""); let parts = link.split('#').collect::>(); let (link, extra_fragment) = if parts.len() > 2 { @@ -700,6 +701,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { (parts[0], None) }; let resolved_self; + let link_text; let mut path_str; let disambiguator; let (mut res, mut fragment) = { @@ -716,6 +718,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } + // We stripped ` characters for `path_str`. + // The original link might have had multiple pairs of backticks, + // but we don't handle this case. + //link_text = if had_backticks { format!("`{}`", path_str) } else { path_str.to_owned() }; + link_text = path_str.to_owned(); + // In order to correctly resolve intra-doc-links we need to // pick a base AST node to work from. If the documentation for // this module came from an inner comment (//!) then we anchor @@ -904,7 +912,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - item.attrs.links.push(ItemLink { link: ori_link, did: None, fragment }); + item.attrs.links.push(ItemLink { + link: ori_link, + link_text: path_str.to_owned(), + did: None, + fragment, + }); } Some(other) => { report_mismatch(other, Disambiguator::Primitive); @@ -955,7 +968,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } } let id = register_res(cx, res); - item.attrs.links.push(ItemLink { link: ori_link, did: Some(id), fragment }); + item.attrs.links.push(ItemLink { + link: ori_link, + link_text, + did: Some(id), + fragment, + }); } } diff --git a/src/test/rustdoc/disambiguator_removed.rs b/src/test/rustdoc/disambiguator_removed.rs new file mode 100644 index 0000000000000..74411870e9f8a --- /dev/null +++ b/src/test/rustdoc/disambiguator_removed.rs @@ -0,0 +1,33 @@ +#![deny(intra_doc_link_resolution_failure)] +// first try backticks +/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] +// @has disambiguator_removed/struct.AtDisambiguator.html +// @has - '//a[@href="../disambiguator_removed/trait.Name.html"][code]' "Name" +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name" +pub struct AtDisambiguator; + +/// fn: [`Name()`], macro: [`Name!`] +// @has disambiguator_removed/struct.SymbolDisambiguator.html +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name()" +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name!" +pub struct SymbolDisambiguator; + +// Now make sure that backticks aren't added if they weren't already there +/// [fn@Name] +// @has disambiguator_removed/trait.Name.html +// @has - '//a[@href="../disambiguator_removed/fn.Name.html"]' "Name" +// @!has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" + +// FIXME: this will turn !() into ! alone +/// [Name!()] +// @has - '//a[@href="../disambiguator_removed/macro.Name.html"]' "Name!" +pub trait Name {} + +#[allow(non_snake_case)] +pub fn Name() {} + +#[macro_export] +macro_rules! Name { + () => () +} From 9d7e797514ed1ea60a761d272c1ba8426bc31739 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 15:13:28 -0400 Subject: [PATCH 28/37] display_for -> suggestion_for --- src/librustdoc/passes/collect_intra_doc_links.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index ff3c19da3cf75..feadf82a2b57e 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -685,7 +685,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } - //let had_backticks = ori_link.contains("`"); let link = ori_link.replace("`", ""); let parts = link.split('#').collect::>(); let (link, extra_fragment) = if parts.len() > 2 { @@ -1053,7 +1052,7 @@ impl Disambiguator { } /// Return (description of the change, suggestion) - fn display_for(self, path_str: &str) -> (&'static str, String) { + fn suggestion_for(self, path_str: &str) -> (&'static str, String) { const PREFIX: &str = "prefix with the item kind"; const FUNCTION: &str = "add parentheses"; const MACRO: &str = "add an exclamation mark"; @@ -1308,7 +1307,7 @@ fn suggest_disambiguator( sp: Option, link_range: &Option>, ) { - let (action, mut suggestion) = disambiguator.display_for(path_str); + let (action, mut suggestion) = disambiguator.suggestion_for(path_str); let help = format!("to link to the {}, {}", disambiguator.descr(), action); if let Some(sp) = sp { From 4df64905ea74f9732cb1448c14f72b17d3f32abc Mon Sep 17 00:00:00 2001 From: Ivan Tham Date: Thu, 3 Sep 2020 23:02:27 +0800 Subject: [PATCH 29/37] Link & primitive using relative link --- library/alloc/src/vec.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/alloc/src/vec.rs b/library/alloc/src/vec.rs index 35d64d85010c3..43a0b048b47cc 100644 --- a/library/alloc/src/vec.rs +++ b/library/alloc/src/vec.rs @@ -287,6 +287,7 @@ use crate::raw_vec::RawVec; /// [`insert`]: Vec::insert /// [`reserve`]: Vec::reserve /// [owned slice]: Box +/// [`&`]: ../../std/primitive.reference.html #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(not(test), rustc_diagnostic_item = "vec_type")] pub struct Vec { From 6e43ff5ceafb2674af709f09e72c4f1f53eecf22 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 24 Aug 2020 20:20:03 +0200 Subject: [PATCH 30/37] Add check for doc alias on associated const in trait impls --- compiler/rustc_passes/src/check_attr.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index 832cde86d0b7b..392070839dc2a 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -250,13 +250,23 @@ impl CheckAttrVisitor<'tcx> { None } } + Target::AssocConst => { + let parent_hir_id = self.tcx.hir().get_parent_item(hir_id); + let containing_item = self.tcx.hir().expect_item(parent_hir_id); + // We can't link to trait impl's consts. + let err = "associated constant in trait implementation block"; + match containing_item.kind { + ItemKind::Impl { of_trait: Some(_), .. } => Some(err), + _ => None, + } + } _ => None, } { self.tcx .sess .struct_span_err( meta.span(), - &format!("`#[doc(alias = \"...\")]` isn't allowed on {}", err,), + &format!("`#[doc(alias = \"...\")]` isn't allowed on {}", err), ) .emit(); } From b61eab5d514af27009769373360b57dd55036a23 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 24 Aug 2020 20:20:31 +0200 Subject: [PATCH 31/37] Add test for doc alias on associated const in trait impls --- src/test/rustdoc-ui/doc-alias-assoc-const.rs | 22 +++++++++++++++++++ .../rustdoc-ui/doc-alias-assoc-const.stderr | 8 +++++++ 2 files changed, 30 insertions(+) create mode 100644 src/test/rustdoc-ui/doc-alias-assoc-const.rs create mode 100644 src/test/rustdoc-ui/doc-alias-assoc-const.stderr diff --git a/src/test/rustdoc-ui/doc-alias-assoc-const.rs b/src/test/rustdoc-ui/doc-alias-assoc-const.rs new file mode 100644 index 0000000000000..73e23c152f268 --- /dev/null +++ b/src/test/rustdoc-ui/doc-alias-assoc-const.rs @@ -0,0 +1,22 @@ +#![feature(doc_alias)] +#![feature(trait_alias)] + +pub struct Foo; + +pub trait Bar { + const BAZ: u8; +} + +impl Bar for Foo { + #[doc(alias = "CONST_BAZ")] //~ ERROR + const BAZ: u8 = 0; +} + +impl Foo { + #[doc(alias = "CONST_FOO")] // ok! + pub const FOO: u8 = 0; + + pub fn bar() -> u8 { + Self::FOO + } +} diff --git a/src/test/rustdoc-ui/doc-alias-assoc-const.stderr b/src/test/rustdoc-ui/doc-alias-assoc-const.stderr new file mode 100644 index 0000000000000..3c64548cc204d --- /dev/null +++ b/src/test/rustdoc-ui/doc-alias-assoc-const.stderr @@ -0,0 +1,8 @@ +error: `#[doc(alias = "...")]` isn't allowed on associated constant in trait implementation block + --> $DIR/doc-alias-assoc-const.rs:11:11 + | +LL | #[doc(alias = "CONST_BAZ")] + | ^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From 85146b9db7bf5de79684f8595ad49c33e436d1ae Mon Sep 17 00:00:00 2001 From: Ivan Tham Date: Fri, 4 Sep 2020 09:50:50 +0800 Subject: [PATCH 32/37] Add slice primitive link to vec --- library/alloc/src/vec.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/vec.rs b/library/alloc/src/vec.rs index 43a0b048b47cc..a132cf17dff4a 100644 --- a/library/alloc/src/vec.rs +++ b/library/alloc/src/vec.rs @@ -159,7 +159,7 @@ use crate::raw_vec::RawVec; /// # Slicing /// /// A `Vec` can be mutable. Slices, on the other hand, are read-only objects. -/// To get a slice, use [`&`]. Example: +/// To get a [slice], use [`&`]. Example: /// /// ``` /// fn read_slice(slice: &[usize]) { @@ -287,6 +287,7 @@ use crate::raw_vec::RawVec; /// [`insert`]: Vec::insert /// [`reserve`]: Vec::reserve /// [owned slice]: Box +/// [slice]: ../../std/primitive.slice.html /// [`&`]: ../../std/primitive.reference.html #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(not(test), rustc_diagnostic_item = "vec_type")] From 18c14fde0d293a18fbd3c14487b52e1ce7daa205 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 29 Aug 2020 15:19:43 -0400 Subject: [PATCH 33/37] Misc cleanup - Preserve suffixes when displaying - Rename test file to match `intra-link*` - Remove unnecessary .clone()s - Improve comments and naming - Fix more bugs and add tests - Escape intra-doc link example in public documentation --- src/librustdoc/clean/types.rs | 6 +- src/librustdoc/html/markdown.rs | 65 ++++++++++++------- .../passes/collect_intra_doc_links.rs | 22 +++++-- src/test/rustdoc/disambiguator_removed.rs | 33 ---------- .../intra-link-disambiguators-removed.rs | 51 +++++++++++++++ 5 files changed, 114 insertions(+), 63 deletions(-) delete mode 100644 src/test/rustdoc/disambiguator_removed.rs create mode 100644 src/test/rustdoc/intra-link-disambiguators-removed.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 05c90a7c4034f..223fda84871e9 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -439,7 +439,7 @@ pub struct ItemLink { /// The link text displayed in the HTML. /// /// This may not be the same as `link` if there was a disambiguator - /// in an intra-doc link (e.g. [`fn@f`]) + /// in an intra-doc link (e.g. \[`fn@f`\]) pub(crate) link_text: String, pub(crate) did: Option, /// The url fragment to append to the link @@ -447,7 +447,9 @@ pub struct ItemLink { } pub struct RenderedLink { - /// The text the link was original written as + /// The text the link was original written as. + /// + /// This could potentially include disambiguators and backticks. pub(crate) original_text: String, /// The text to display in the HTML pub(crate) new_text: String, diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 6d634bac7622d..58d75c0747295 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -338,83 +338,102 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { } /// Make headings links with anchor IDs and build up TOC. -struct LinkReplacer<'a, 'b, I: Iterator>> { +struct LinkReplacer<'a, I: Iterator>> { inner: I, links: &'a [RenderedLink], - shortcut_link: Option<&'b RenderedLink>, + shortcut_link: Option<&'a RenderedLink>, } -impl<'a, I: Iterator>> LinkReplacer<'a, '_, I> { +impl<'a, I: Iterator>> LinkReplacer<'a, I> { fn new(iter: I, links: &'a [RenderedLink]) -> Self { LinkReplacer { inner: iter, links, shortcut_link: None } } } -impl<'a: 'b, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> { +impl<'a, I: Iterator>> Iterator for LinkReplacer<'a, I> { type Item = Event<'a>; fn next(&mut self) -> Option { + use pulldown_cmark::LinkType; + let mut event = self.inner.next(); - // Remove disambiguators from shortcut links (`[fn@f]`) + // Replace intra-doc links and remove disambiguators from shortcut links (`[fn@f]`). match &mut event { + // This is a shortcut link that was resolved by the broken_link_callback: `[fn@f]` + // Remove any disambiguator. Some(Event::Start(Tag::Link( - pulldown_cmark::LinkType::ShortcutUnknown, + // [fn@f] or [fn@f][] + LinkType::ShortcutUnknown | LinkType::CollapsedUnknown, dest, title, ))) => { debug!("saw start of shortcut link to {} with title {}", dest, title); - let link = if let Some(link) = - self.links.iter().find(|&link| *link.original_text == **dest) - { - // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? - *dest = CowStr::Borrowed(link.href.as_ref()); - Some(link) - } else { - self.links.iter().find(|&link| *link.href == **dest) - }; + // If this is a shortcut link, it was resolved by the broken_link_callback. + // So the URL will already be updated properly. + let link = self.links.iter().find(|&link| *link.href == **dest); + // Since this is an external iterator, we can't replace the inner text just yet. + // Store that we saw a link so we know to replace it later. if let Some(link) = link { trace!("it matched"); assert!(self.shortcut_link.is_none(), "shortcut links cannot be nested"); self.shortcut_link = Some(link); } } - Some(Event::End(Tag::Link(pulldown_cmark::LinkType::ShortcutUnknown, dest, _))) => { + // Now that we're done with the shortcut link, don't replace any more text. + Some(Event::End(Tag::Link( + LinkType::ShortcutUnknown | LinkType::CollapsedUnknown, + dest, + _, + ))) => { debug!("saw end of shortcut link to {}", dest); - if let Some(_link) = self.links.iter().find(|&link| *link.href == **dest) { + if self.links.iter().find(|&link| *link.href == **dest).is_some() { assert!(self.shortcut_link.is_some(), "saw closing link without opening tag"); self.shortcut_link = None; } } - // Handle backticks in inline code blocks + // Handle backticks in inline code blocks, but only if we're in the middle of a shortcut link. + // [`fn@f`] Some(Event::Code(text)) => { trace!("saw code {}", text); if let Some(link) = self.shortcut_link { trace!("original text was {}", link.original_text); + // NOTE: this only replaces if the code block is the *entire* text. + // If only part of the link has code highlighting, the disambiguator will not be removed. + // e.g. [fn@`f`] + // This is a limitation from `collect_intra_doc_links`: it passes a full link, + // and does not distinguish at all between code blocks. + // So we could never be sure we weren't replacing too much: + // [fn@my_`f`unc] is treated the same as [my_func()] in that pass. + // + // NOTE: &[1..len() - 1] is to strip the backticks if **text == link.original_text[1..link.original_text.len() - 1] { debug!("replacing {} with {}", text, link.new_text); - *text = link.new_text.clone().into(); + *text = CowStr::Borrowed(&link.new_text); } } } - // Replace plain text in links + // Replace plain text in links, but only in the middle of a shortcut link. + // [fn@f] Some(Event::Text(text)) => { trace!("saw text {}", text); if let Some(link) = self.shortcut_link { trace!("original text was {}", link.original_text); + // NOTE: same limitations as `Event::Code` if **text == *link.original_text { debug!("replacing {} with {}", text, link.new_text); - *text = link.new_text.clone().into(); + *text = CowStr::Borrowed(&link.new_text); } } } + // If this is a link, but not a shortcut link, + // replace the URL, since the broken_link_callback was not called. Some(Event::Start(Tag::Link(_, dest, _))) => { if let Some(link) = self.links.iter().find(|&link| *link.original_text == **dest) { - // Not sure why this is necessary - maybe the broken_link_callback doesn't always work? *dest = CowStr::Borrowed(link.href.as_ref()); } } - // Anything else couldn't have been a valid Rust path + // Anything else couldn't have been a valid Rust path, so no need to replace the text. _ => {} } diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index feadf82a2b57e..9a705293376a0 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -717,11 +717,11 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } - // We stripped ` characters for `path_str`. - // The original link might have had multiple pairs of backticks, - // but we don't handle this case. - //link_text = if had_backticks { format!("`{}`", path_str) } else { path_str.to_owned() }; - link_text = path_str.to_owned(); + // We stripped `()` and `!` when parsing the disambiguator. + // Add them back to be displayed, but not prefix disambiguators. + link_text = disambiguator + .map(|d| d.display_for(path_str)) + .unwrap_or_else(|| path_str.to_owned()); // In order to correctly resolve intra-doc-links we need to // pick a base AST node to work from. If the documentation for @@ -1000,6 +1000,18 @@ enum Disambiguator { } impl Disambiguator { + /// The text that should be displayed when the path is rendered as HTML. + /// + /// NOTE: `path` is not the original link given by the user, but a name suitable for passing to `resolve`. + fn display_for(&self, path: &str) -> String { + match self { + // FIXME: this will have different output if the user had `m!()` originally. + Self::Kind(DefKind::Macro(MacroKind::Bang)) => format!("{}!", path), + Self::Kind(DefKind::Fn) => format!("{}()", path), + _ => path.to_owned(), + } + } + /// (disambiguator, path_str) fn from_str(link: &str) -> Result<(Self, &str), ()> { use Disambiguator::{Kind, Namespace as NS, Primitive}; diff --git a/src/test/rustdoc/disambiguator_removed.rs b/src/test/rustdoc/disambiguator_removed.rs deleted file mode 100644 index 74411870e9f8a..0000000000000 --- a/src/test/rustdoc/disambiguator_removed.rs +++ /dev/null @@ -1,33 +0,0 @@ -#![deny(intra_doc_link_resolution_failure)] -// first try backticks -/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] -// @has disambiguator_removed/struct.AtDisambiguator.html -// @has - '//a[@href="../disambiguator_removed/trait.Name.html"][code]' "Name" -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name" -pub struct AtDisambiguator; - -/// fn: [`Name()`], macro: [`Name!`] -// @has disambiguator_removed/struct.SymbolDisambiguator.html -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name()" -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"][code]' "Name!" -pub struct SymbolDisambiguator; - -// Now make sure that backticks aren't added if they weren't already there -/// [fn@Name] -// @has disambiguator_removed/trait.Name.html -// @has - '//a[@href="../disambiguator_removed/fn.Name.html"]' "Name" -// @!has - '//a[@href="../disambiguator_removed/fn.Name.html"][code]' "Name" - -// FIXME: this will turn !() into ! alone -/// [Name!()] -// @has - '//a[@href="../disambiguator_removed/macro.Name.html"]' "Name!" -pub trait Name {} - -#[allow(non_snake_case)] -pub fn Name() {} - -#[macro_export] -macro_rules! Name { - () => () -} diff --git a/src/test/rustdoc/intra-link-disambiguators-removed.rs b/src/test/rustdoc/intra-link-disambiguators-removed.rs new file mode 100644 index 0000000000000..26d05b484b919 --- /dev/null +++ b/src/test/rustdoc/intra-link-disambiguators-removed.rs @@ -0,0 +1,51 @@ +// ignore-tidy-linelength +#![deny(intra_doc_link_resolution_failure)] +// first try backticks +/// Trait: [`trait@Name`], fn: [`fn@Name`], [`Name`][`macro@Name`] +// @has intra_link_disambiguators_removed/struct.AtDisambiguator.html +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"][code]' "Name" +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name" +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"][code]' "Name" +pub struct AtDisambiguator; + +/// fn: [`Name()`], macro: [`Name!`] +// @has intra_link_disambiguators_removed/struct.SymbolDisambiguator.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name()" +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"][code]' "Name!" +pub struct SymbolDisambiguator; + +// Now make sure that backticks aren't added if they weren't already there +/// [fn@Name] +// @has intra_link_disambiguators_removed/trait.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"]' "Name" +// @!has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"][code]' "Name" + +// FIXME: this will turn !() into ! alone +/// [Name!()] +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"]' "Name!" +pub trait Name {} + +#[allow(non_snake_case)] + +// Try collapsed reference links +/// [macro@Name][] +// @has intra_link_disambiguators_removed/fn.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/macro.Name.html"]' "Name" + +// Try links that have the same text as a generated URL +/// Weird URL aligned [../intra_link_disambiguators_removed/macro.Name.html][trait@Name] +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"]' "../intra_link_disambiguators_removed/macro.Name.html" +pub fn Name() {} + +#[macro_export] +// Rustdoc doesn't currently handle links that have weird interspersing of inline code blocks. +/// [fn@Na`m`e] +// @has intra_link_disambiguators_removed/macro.Name.html +// @has - '//a[@href="../intra_link_disambiguators_removed/fn.Name.html"]' "fn@Name" + +// It also doesn't handle any case where the code block isn't the whole link text: +/// [trait@`Name`] +// @has - '//a[@href="../intra_link_disambiguators_removed/trait.Name.html"]' "trait@Name" +macro_rules! Name { + () => () +} From ed3950ba5a907abf0e28b21907e8df8ab73e8c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Miku=C5=82a?= Date: Wed, 5 Aug 2020 10:58:04 +0200 Subject: [PATCH 34/37] Enable profiler tests on Windows-gnu --- .github/workflows/ci.yml | 4 ++-- src/ci/github-actions/ci.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6c6c816eec622..445658d1b2198 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -431,7 +431,7 @@ jobs: - name: x86_64-mingw-1 env: SCRIPT: make ci-mingw-subset-1 - RUST_CONFIGURE_ARGS: "--build=x86_64-pc-windows-gnu" + RUST_CONFIGURE_ARGS: "--build=x86_64-pc-windows-gnu --enable-profiler" CUSTOM_MINGW: 1 NO_DEBUG_ASSERTIONS: 1 NO_LLVM_ASSERTIONS: 1 @@ -439,7 +439,7 @@ jobs: - name: x86_64-mingw-2 env: SCRIPT: make ci-mingw-subset-2 - RUST_CONFIGURE_ARGS: "--build=x86_64-pc-windows-gnu" + RUST_CONFIGURE_ARGS: "--build=x86_64-pc-windows-gnu --enable-profiler" CUSTOM_MINGW: 1 os: windows-latest-xl - name: dist-x86_64-msvc diff --git a/src/ci/github-actions/ci.yml b/src/ci/github-actions/ci.yml index 522d5bd3d9c36..aef4374c54232 100644 --- a/src/ci/github-actions/ci.yml +++ b/src/ci/github-actions/ci.yml @@ -523,7 +523,7 @@ jobs: - name: x86_64-mingw-1 env: SCRIPT: make ci-mingw-subset-1 - RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-gnu + RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-gnu --enable-profiler CUSTOM_MINGW: 1 # FIXME(#59637) NO_DEBUG_ASSERTIONS: 1 @@ -533,7 +533,7 @@ jobs: - name: x86_64-mingw-2 env: SCRIPT: make ci-mingw-subset-2 - RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-gnu + RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-gnu --enable-profiler CUSTOM_MINGW: 1 <<: *job-windows-xl From 1ea121c74f816d55f11f73ed28e7a3478fe6e584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Miku=C5=82a?= Date: Mon, 24 Aug 2020 11:57:32 +0200 Subject: [PATCH 35/37] Fix warning whe building profiler_builtins crate --- library/profiler_builtins/build.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/profiler_builtins/build.rs b/library/profiler_builtins/build.rs index 2a5d5853fec64..7d5c601df535b 100644 --- a/library/profiler_builtins/build.rs +++ b/library/profiler_builtins/build.rs @@ -48,10 +48,10 @@ fn main() { // Turn off various features of gcc and such, mostly copying // compiler-rt's build system already cfg.flag("-fno-builtin"); - cfg.flag("-fvisibility=hidden"); cfg.flag("-fomit-frame-pointer"); cfg.define("VISIBILITY_HIDDEN", None); if !target.contains("windows") { + cfg.flag("-fvisibility=hidden"); cfg.define("COMPILER_RT_HAS_UNAME", Some("1")); } else { profile_sources.push("WindowsMMap.c"); From 03fd825ce1e4c7328ad994ce97a148837fc6933e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Miku=C5=82a?= Date: Mon, 24 Aug 2020 11:58:04 +0200 Subject: [PATCH 36/37] Ignore failing PGO/coverage tests on MinGW --- .../instrument-coverage-cov-reports-base/Makefile | 4 ++++ .../instrument-coverage-cov-reports-link-dead-code/Makefile | 6 +++++- src/test/run-make-fulldeps/pgo-branch-weights/Makefile | 4 ++++ .../run-make-fulldeps/pgo-indirect-call-promotion/Makefile | 4 ++++ src/test/run-make-fulldeps/pgo-use/Makefile | 4 ++++ 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/test/run-make-fulldeps/instrument-coverage-cov-reports-base/Makefile b/src/test/run-make-fulldeps/instrument-coverage-cov-reports-base/Makefile index a16b4f61dcb5b..cb081fb641b0e 100644 --- a/src/test/run-make-fulldeps/instrument-coverage-cov-reports-base/Makefile +++ b/src/test/run-make-fulldeps/instrument-coverage-cov-reports-base/Makefile @@ -1,4 +1,8 @@ # needs-profiler-support +# ignore-windows-gnu + +# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works +# properly. Since we only have GCC on the CI ignore the test for now. # ISSUE(76038): When targeting MSVC, Rust binaries built with both `-Z instrument-coverage` and # `-C link-dead-code` typically crash (with a seg-fault) or at best generate an empty `*.profraw`. diff --git a/src/test/run-make-fulldeps/instrument-coverage-cov-reports-link-dead-code/Makefile b/src/test/run-make-fulldeps/instrument-coverage-cov-reports-link-dead-code/Makefile index 08f311f17023d..ab826d07e056f 100644 --- a/src/test/run-make-fulldeps/instrument-coverage-cov-reports-link-dead-code/Makefile +++ b/src/test/run-make-fulldeps/instrument-coverage-cov-reports-link-dead-code/Makefile @@ -1,5 +1,9 @@ # needs-profiler-support # ignore-msvc +# ignore-windows-gnu + +# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works +# properly. Since we only have GCC on the CI ignore the test for now. # LINK_DEAD_CODE requires ignore-msvc due to Issue #76038 LINK_DEAD_CODE=yes @@ -8,4 +12,4 @@ LINK_DEAD_CODE=yes # ISSUE(76038): When targeting MSVC, Rust binaries built with both `-Z instrument-coverage` and # `-C link-dead-code` typically crash (with a seg-fault) or at best generate an empty `*.profraw`. -# See ../instrument-coverage/coverage_tools.mk for more information. \ No newline at end of file +# See ../instrument-coverage/coverage_tools.mk for more information. diff --git a/src/test/run-make-fulldeps/pgo-branch-weights/Makefile b/src/test/run-make-fulldeps/pgo-branch-weights/Makefile index c13297b3a6141..18828b66ce874 100644 --- a/src/test/run-make-fulldeps/pgo-branch-weights/Makefile +++ b/src/test/run-make-fulldeps/pgo-branch-weights/Makefile @@ -1,4 +1,8 @@ # needs-profiler-support +# ignore-windows-gnu + +# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works +# properly. Since we only have GCC on the CI ignore the test for now. -include ../tools.mk diff --git a/src/test/run-make-fulldeps/pgo-indirect-call-promotion/Makefile b/src/test/run-make-fulldeps/pgo-indirect-call-promotion/Makefile index e61018752c375..876a9b2c43991 100644 --- a/src/test/run-make-fulldeps/pgo-indirect-call-promotion/Makefile +++ b/src/test/run-make-fulldeps/pgo-indirect-call-promotion/Makefile @@ -1,4 +1,8 @@ # needs-profiler-support +# ignore-windows-gnu + +# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works +# properly. Since we only have GCC on the CI ignore the test for now. -include ../tools.mk diff --git a/src/test/run-make-fulldeps/pgo-use/Makefile b/src/test/run-make-fulldeps/pgo-use/Makefile index 61a73587759fe..cb5e9e9a45580 100644 --- a/src/test/run-make-fulldeps/pgo-use/Makefile +++ b/src/test/run-make-fulldeps/pgo-use/Makefile @@ -1,4 +1,8 @@ # needs-profiler-support +# ignore-windows-gnu + +# FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works +# properly. Since we only have GCC on the CI ignore the test for now. -include ../tools.mk From e8fc38d0dc7320ee22cb4eb4828dc0afded9d612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Miku=C5=82a?= Date: Mon, 31 Aug 2020 14:42:26 +0200 Subject: [PATCH 37/37] Update llvm submodule --- src/llvm-project | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llvm-project b/src/llvm-project index 45790d79496be..4d40ae500282d 160000 --- a/src/llvm-project +++ b/src/llvm-project @@ -1 +1 @@ -Subproject commit 45790d79496be37fbce6ec57abad5af8fa7a34d7 +Subproject commit 4d40ae500282dac444028358cbda8235f65e7e6a