From dbc0ed2a109a3409bd21529ca8375a43d5580739 Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Sun, 20 Nov 2022 20:35:40 +0100 Subject: [PATCH 01/20] Unify stable and unstable sort implementations in same core module This moves the stable sort implementation to the core::slice::sort module. By virtue of being in core it can't access `Vec`. The two `Vec` used by merge sort, `buf` and `runs`, are modelled as custom types that implement the very limited required `Vec` interface with the help of provided allocation and free functions. This is done to allow future re-use of functions and logic between stable and unstable sort. Such as `insert_head`. --- library/alloc/src/slice.rs | 348 +++-------------------- library/core/src/slice/mod.rs | 8 +- library/core/src/slice/sort.rs | 494 +++++++++++++++++++++++++++++++++ 3 files changed, 540 insertions(+), 310 deletions(-) diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 1b61ede3476c3..e5d614ab1ff67 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -19,10 +19,12 @@ use core::cmp::Ordering::{self, Less}; use core::mem::{self, SizedTypeProperties}; #[cfg(not(no_global_oom_handling))] use core::ptr; +#[cfg(not(no_global_oom_handling))] +use core::slice::sort; use crate::alloc::Allocator; #[cfg(not(no_global_oom_handling))] -use crate::alloc::Global; +use crate::alloc::{self, Global}; #[cfg(not(no_global_oom_handling))] use crate::borrow::ToOwned; use crate::boxed::Box; @@ -203,7 +205,7 @@ impl [T] { where T: Ord, { - merge_sort(self, T::lt); + stable_sort(self, T::lt); } /// Sorts the slice with a comparator function. @@ -259,7 +261,7 @@ impl [T] { where F: FnMut(&T, &T) -> Ordering, { - merge_sort(self, |a, b| compare(a, b) == Less); + stable_sort(self, |a, b| compare(a, b) == Less); } /// Sorts the slice with a key extraction function. @@ -302,7 +304,7 @@ impl [T] { F: FnMut(&T) -> K, K: Ord, { - merge_sort(self, |a, b| f(a).lt(&f(b))); + stable_sort(self, |a, b| f(a).lt(&f(b))); } /// Sorts the slice with a key extraction function. @@ -809,324 +811,52 @@ impl ToOwned for [T] { // Sorting //////////////////////////////////////////////////////////////////////////////// -/// Inserts `v[0]` into pre-sorted sequence `v[1..]` so that whole `v[..]` becomes sorted. -/// -/// This is the integral subroutine of insertion sort. -#[cfg(not(no_global_oom_handling))] -fn insert_head(v: &mut [T], is_less: &mut F) -where - F: FnMut(&T, &T) -> bool, -{ - if v.len() >= 2 && is_less(&v[1], &v[0]) { - unsafe { - // There are three ways to implement insertion here: - // - // 1. Swap adjacent elements until the first one gets to its final destination. - // However, this way we copy data around more than is necessary. If elements are big - // structures (costly to copy), this method will be slow. - // - // 2. Iterate until the right place for the first element is found. Then shift the - // elements succeeding it to make room for it and finally place it into the - // remaining hole. This is a good method. - // - // 3. Copy the first element into a temporary variable. Iterate until the right place - // for it is found. As we go along, copy every traversed element into the slot - // preceding it. Finally, copy data from the temporary variable into the remaining - // hole. This method is very good. Benchmarks demonstrated slightly better - // performance than with the 2nd method. - // - // All methods were benchmarked, and the 3rd showed best results. So we chose that one. - let tmp = mem::ManuallyDrop::new(ptr::read(&v[0])); - - // Intermediate state of the insertion process is always tracked by `hole`, which - // serves two purposes: - // 1. Protects integrity of `v` from panics in `is_less`. - // 2. Fills the remaining hole in `v` in the end. - // - // Panic safety: - // - // If `is_less` panics at any point during the process, `hole` will get dropped and - // fill the hole in `v` with `tmp`, thus ensuring that `v` still holds every object it - // initially held exactly once. - let mut hole = InsertionHole { src: &*tmp, dest: &mut v[1] }; - ptr::copy_nonoverlapping(&v[1], &mut v[0], 1); - - for i in 2..v.len() { - if !is_less(&v[i], &*tmp) { - break; - } - ptr::copy_nonoverlapping(&v[i], &mut v[i - 1], 1); - hole.dest = &mut v[i]; - } - // `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`. - } - } - - // When dropped, copies from `src` into `dest`. - struct InsertionHole { - src: *const T, - dest: *mut T, - } - - impl Drop for InsertionHole { - fn drop(&mut self) { - unsafe { - ptr::copy_nonoverlapping(self.src, self.dest, 1); - } - } - } -} - -/// Merges non-decreasing runs `v[..mid]` and `v[mid..]` using `buf` as temporary storage, and -/// stores the result into `v[..]`. -/// -/// # Safety -/// -/// The two slices must be non-empty and `mid` must be in bounds. Buffer `buf` must be long enough -/// to hold a copy of the shorter slice. Also, `T` must not be a zero-sized type. -#[cfg(not(no_global_oom_handling))] -unsafe fn merge(v: &mut [T], mid: usize, buf: *mut T, is_less: &mut F) -where - F: FnMut(&T, &T) -> bool, -{ - let len = v.len(); - let v = v.as_mut_ptr(); - let (v_mid, v_end) = unsafe { (v.add(mid), v.add(len)) }; - - // The merge process first copies the shorter run into `buf`. Then it traces the newly copied - // run and the longer run forwards (or backwards), comparing their next unconsumed elements and - // copying the lesser (or greater) one into `v`. - // - // As soon as the shorter run is fully consumed, the process is done. If the longer run gets - // consumed first, then we must copy whatever is left of the shorter run into the remaining - // hole in `v`. - // - // Intermediate state of the process is always tracked by `hole`, which serves two purposes: - // 1. Protects integrity of `v` from panics in `is_less`. - // 2. Fills the remaining hole in `v` if the longer run gets consumed first. - // - // Panic safety: - // - // If `is_less` panics at any point during the process, `hole` will get dropped and fill the - // hole in `v` with the unconsumed range in `buf`, thus ensuring that `v` still holds every - // object it initially held exactly once. - let mut hole; - - if mid <= len - mid { - // The left run is shorter. - unsafe { - ptr::copy_nonoverlapping(v, buf, mid); - hole = MergeHole { start: buf, end: buf.add(mid), dest: v }; - } - - // Initially, these pointers point to the beginnings of their arrays. - let left = &mut hole.start; - let mut right = v_mid; - let out = &mut hole.dest; - - while *left < hole.end && right < v_end { - // Consume the lesser side. - // If equal, prefer the left run to maintain stability. - unsafe { - let to_copy = if is_less(&*right, &**left) { - get_and_increment(&mut right) - } else { - get_and_increment(left) - }; - ptr::copy_nonoverlapping(to_copy, get_and_increment(out), 1); - } - } - } else { - // The right run is shorter. - unsafe { - ptr::copy_nonoverlapping(v_mid, buf, len - mid); - hole = MergeHole { start: buf, end: buf.add(len - mid), dest: v_mid }; - } - - // Initially, these pointers point past the ends of their arrays. - let left = &mut hole.dest; - let right = &mut hole.end; - let mut out = v_end; - - while v < *left && buf < *right { - // Consume the greater side. - // If equal, prefer the right run to maintain stability. - unsafe { - let to_copy = if is_less(&*right.sub(1), &*left.sub(1)) { - decrement_and_get(left) - } else { - decrement_and_get(right) - }; - ptr::copy_nonoverlapping(to_copy, decrement_and_get(&mut out), 1); - } - } - } - // Finally, `hole` gets dropped. If the shorter run was not fully consumed, whatever remains of - // it will now be copied into the hole in `v`. - - unsafe fn get_and_increment(ptr: &mut *mut T) -> *mut T { - let old = *ptr; - *ptr = unsafe { ptr.add(1) }; - old - } - - unsafe fn decrement_and_get(ptr: &mut *mut T) -> *mut T { - *ptr = unsafe { ptr.sub(1) }; - *ptr - } - - // When dropped, copies the range `start..end` into `dest..`. - struct MergeHole { - start: *mut T, - end: *mut T, - dest: *mut T, - } - - impl Drop for MergeHole { - fn drop(&mut self) { - // `T` is not a zero-sized type, and these are pointers into a slice's elements. - unsafe { - let len = self.end.sub_ptr(self.start); - ptr::copy_nonoverlapping(self.start, self.dest, len); - } - } - } -} - -/// This merge sort borrows some (but not all) ideas from TimSort, which is described in detail -/// [here](https://github.com/python/cpython/blob/main/Objects/listsort.txt). -/// -/// The algorithm identifies strictly descending and non-descending subsequences, which are called -/// natural runs. There is a stack of pending runs yet to be merged. Each newly found run is pushed -/// onto the stack, and then some pairs of adjacent runs are merged until these two invariants are -/// satisfied: -/// -/// 1. for every `i` in `1..runs.len()`: `runs[i - 1].len > runs[i].len` -/// 2. for every `i` in `2..runs.len()`: `runs[i - 2].len > runs[i - 1].len + runs[i].len` -/// -/// The invariants ensure that the total running time is *O*(*n* \* log(*n*)) worst-case. +#[inline] #[cfg(not(no_global_oom_handling))] -fn merge_sort(v: &mut [T], mut is_less: F) +fn stable_sort(v: &mut [T], mut is_less: F) where F: FnMut(&T, &T) -> bool, { - // Slices of up to this length get sorted using insertion sort. - const MAX_INSERTION: usize = 20; - // Very short runs are extended using insertion sort to span at least this many elements. - const MIN_RUN: usize = 10; - - // Sorting has no meaningful behavior on zero-sized types. if T::IS_ZST { + // Sorting has no meaningful behavior on zero-sized types. Do nothing. return; } - let len = v.len(); - - // Short arrays get sorted in-place via insertion sort to avoid allocations. - if len <= MAX_INSERTION { - if len >= 2 { - for i in (0..len - 1).rev() { - insert_head(&mut v[i..], &mut is_less); - } - } - return; - } - - // Allocate a buffer to use as scratch memory. We keep the length 0 so we can keep in it - // shallow copies of the contents of `v` without risking the dtors running on copies if - // `is_less` panics. When merging two sorted runs, this buffer holds a copy of the shorter run, - // which will always have length at most `len / 2`. - let mut buf = Vec::with_capacity(len / 2); + let elem_alloc_fn = |len: usize| -> *mut T { + // SAFETY: Creating the layout is safe as long as merge_sort never calls this with len > + // v.len(). Alloc in general will only be used as 'shadow-region' to store temporary swap + // elements. + unsafe { alloc::alloc(alloc::Layout::array::(len).unwrap_unchecked()) as *mut T } + }; - // In order to identify natural runs in `v`, we traverse it backwards. That might seem like a - // strange decision, but consider the fact that merges more often go in the opposite direction - // (forwards). According to benchmarks, merging forwards is slightly faster than merging - // backwards. To conclude, identifying runs by traversing backwards improves performance. - let mut runs = vec![]; - let mut end = len; - while end > 0 { - // Find the next natural run, and reverse it if it's strictly descending. - let mut start = end - 1; - if start > 0 { - start -= 1; - unsafe { - if is_less(v.get_unchecked(start + 1), v.get_unchecked(start)) { - while start > 0 && is_less(v.get_unchecked(start), v.get_unchecked(start - 1)) { - start -= 1; - } - v[start..end].reverse(); - } else { - while start > 0 && !is_less(v.get_unchecked(start), v.get_unchecked(start - 1)) - { - start -= 1; - } - } - } - } - - // Insert some more elements into the run if it's too short. Insertion sort is faster than - // merge sort on short sequences, so this significantly improves performance. - while start > 0 && end - start < MIN_RUN { - start -= 1; - insert_head(&mut v[start..end], &mut is_less); + let elem_dealloc_fn = |buf_ptr: *mut T, len: usize| { + // SAFETY: Creating the layout is safe as long as merge_sort never calls this with len > + // v.len(). The caller must ensure that buf_ptr was created by elem_alloc_fn with the same + // len. + unsafe { + alloc::dealloc(buf_ptr as *mut u8, alloc::Layout::array::(len).unwrap_unchecked()); } + }; - // Push this run onto the stack. - runs.push(Run { start, len: end - start }); - end = start; - - // Merge some pairs of adjacent runs to satisfy the invariants. - while let Some(r) = collapse(&runs) { - let left = runs[r + 1]; - let right = runs[r]; - unsafe { - merge( - &mut v[left.start..right.start + right.len], - left.len, - buf.as_mut_ptr(), - &mut is_less, - ); - } - runs[r] = Run { start: left.start, len: left.len + right.len }; - runs.remove(r + 1); + let run_alloc_fn = |len: usize| -> *mut sort::TimSortRun { + // SAFETY: Creating the layout is safe as long as merge_sort never calls this with an + // obscene length or 0. + unsafe { + alloc::alloc(alloc::Layout::array::(len).unwrap_unchecked()) + as *mut sort::TimSortRun } - } - - // Finally, exactly one run must remain in the stack. - debug_assert!(runs.len() == 1 && runs[0].start == 0 && runs[0].len == len); + }; - // Examines the stack of runs and identifies the next pair of runs to merge. More specifically, - // if `Some(r)` is returned, that means `runs[r]` and `runs[r + 1]` must be merged next. If the - // algorithm should continue building a new run instead, `None` is returned. - // - // TimSort is infamous for its buggy implementations, as described here: - // http://envisage-project.eu/timsort-specification-and-verification/ - // - // The gist of the story is: we must enforce the invariants on the top four runs on the stack. - // Enforcing them on just top three is not sufficient to ensure that the invariants will still - // hold for *all* runs in the stack. - // - // This function correctly checks invariants for the top four runs. Additionally, if the top - // run starts at index 0, it will always demand a merge operation until the stack is fully - // collapsed, in order to complete the sort. - #[inline] - fn collapse(runs: &[Run]) -> Option { - let n = runs.len(); - if n >= 2 - && (runs[n - 1].start == 0 - || runs[n - 2].len <= runs[n - 1].len - || (n >= 3 && runs[n - 3].len <= runs[n - 2].len + runs[n - 1].len) - || (n >= 4 && runs[n - 4].len <= runs[n - 3].len + runs[n - 2].len)) - { - if n >= 3 && runs[n - 3].len < runs[n - 1].len { Some(n - 3) } else { Some(n - 2) } - } else { - None + let run_dealloc_fn = |buf_ptr: *mut sort::TimSortRun, len: usize| { + // SAFETY: The caller must ensure that buf_ptr was created by elem_alloc_fn with the same + // len. + unsafe { + alloc::dealloc( + buf_ptr as *mut u8, + alloc::Layout::array::(len).unwrap_unchecked(), + ); } - } + }; - #[derive(Clone, Copy)] - struct Run { - start: usize, - len: usize, - } + sort::merge_sort(v, &mut is_less, elem_alloc_fn, elem_dealloc_fn, run_alloc_fn, run_dealloc_fn); } diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index ad90e00b8a095..4075c0c3c1a7a 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -28,13 +28,19 @@ use crate::slice; /// Pure rust memchr implementation, taken from rust-memchr pub mod memchr; +#[unstable( + feature = "slice_internals", + issue = "none", + reason = "exposed from core to be reused in std;" +)] +pub mod sort; + mod ascii; mod cmp; mod index; mod iter; mod raw; mod rotate; -mod sort; mod specialize; #[stable(feature = "rust1", since = "1.0.0")] diff --git a/library/core/src/slice/sort.rs b/library/core/src/slice/sort.rs index 87f77b7f21d62..07c68bf6aa40a 100644 --- a/library/core/src/slice/sort.rs +++ b/library/core/src/slice/sort.rs @@ -5,6 +5,11 @@ //! //! Unstable sorting is compatible with libcore because it doesn't allocate memory, unlike our //! stable sorting implementation. +//! +//! In addition it also contains the core logic of the stable sort used by `slice::sort` based on +//! TimSort. + +#![allow(unused)] // FIXME debug use crate::cmp; use crate::mem::{self, MaybeUninit, SizedTypeProperties}; @@ -883,6 +888,7 @@ fn partition_at_index_loop<'a, T, F>( } } +/// Reorder the slice such that the element at `index` is at its final sorted position. pub fn partition_at_index( v: &mut [T], index: usize, @@ -927,3 +933,491 @@ where let pivot = &mut pivot[0]; (left, pivot, right) } + +/// Inserts `v[0]` into pre-sorted sequence `v[1..]` so that whole `v[..]` becomes sorted. +/// +/// This is the integral subroutine of insertion sort. +fn insert_head(v: &mut [T], is_less: &mut F) +where + F: FnMut(&T, &T) -> bool, +{ + if v.len() >= 2 && is_less(&v[1], &v[0]) { + unsafe { + // There are three ways to implement insertion here: + // + // 1. Swap adjacent elements until the first one gets to its final destination. + // However, this way we copy data around more than is necessary. If elements are big + // structures (costly to copy), this method will be slow. + // + // 2. Iterate until the right place for the first element is found. Then shift the + // elements succeeding it to make room for it and finally place it into the + // remaining hole. This is a good method. + // + // 3. Copy the first element into a temporary variable. Iterate until the right place + // for it is found. As we go along, copy every traversed element into the slot + // preceding it. Finally, copy data from the temporary variable into the remaining + // hole. This method is very good. Benchmarks demonstrated slightly better + // performance than with the 2nd method. + // + // All methods were benchmarked, and the 3rd showed best results. So we chose that one. + let tmp = mem::ManuallyDrop::new(ptr::read(&v[0])); + + // Intermediate state of the insertion process is always tracked by `hole`, which + // serves two purposes: + // 1. Protects integrity of `v` from panics in `is_less`. + // 2. Fills the remaining hole in `v` in the end. + // + // Panic safety: + // + // If `is_less` panics at any point during the process, `hole` will get dropped and + // fill the hole in `v` with `tmp`, thus ensuring that `v` still holds every object it + // initially held exactly once. + let mut hole = InsertionHole { src: &*tmp, dest: &mut v[1] }; + ptr::copy_nonoverlapping(&v[1], &mut v[0], 1); + + for i in 2..v.len() { + if !is_less(&v[i], &*tmp) { + break; + } + ptr::copy_nonoverlapping(&v[i], &mut v[i - 1], 1); + hole.dest = &mut v[i]; + } + // `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`. + } + } + + // When dropped, copies from `src` into `dest`. + struct InsertionHole { + src: *const T, + dest: *mut T, + } + + impl Drop for InsertionHole { + fn drop(&mut self) { + unsafe { + ptr::copy_nonoverlapping(self.src, self.dest, 1); + } + } + } +} + +/// Merges non-decreasing runs `v[..mid]` and `v[mid..]` using `buf` as temporary storage, and +/// stores the result into `v[..]`. +/// +/// # Safety +/// +/// The two slices must be non-empty and `mid` must be in bounds. Buffer `buf` must be long enough +/// to hold a copy of the shorter slice. Also, `T` must not be a zero-sized type. +unsafe fn merge(v: &mut [T], mid: usize, buf: *mut T, is_less: &mut F) +where + F: FnMut(&T, &T) -> bool, +{ + let len = v.len(); + let v = v.as_mut_ptr(); + let (v_mid, v_end) = unsafe { (v.add(mid), v.add(len)) }; + + // The merge process first copies the shorter run into `buf`. Then it traces the newly copied + // run and the longer run forwards (or backwards), comparing their next unconsumed elements and + // copying the lesser (or greater) one into `v`. + // + // As soon as the shorter run is fully consumed, the process is done. If the longer run gets + // consumed first, then we must copy whatever is left of the shorter run into the remaining + // hole in `v`. + // + // Intermediate state of the process is always tracked by `hole`, which serves two purposes: + // 1. Protects integrity of `v` from panics in `is_less`. + // 2. Fills the remaining hole in `v` if the longer run gets consumed first. + // + // Panic safety: + // + // If `is_less` panics at any point during the process, `hole` will get dropped and fill the + // hole in `v` with the unconsumed range in `buf`, thus ensuring that `v` still holds every + // object it initially held exactly once. + let mut hole; + + if mid <= len - mid { + // The left run is shorter. + unsafe { + ptr::copy_nonoverlapping(v, buf, mid); + hole = MergeHole { start: buf, end: buf.add(mid), dest: v }; + } + + // Initially, these pointers point to the beginnings of their arrays. + let left = &mut hole.start; + let mut right = v_mid; + let out = &mut hole.dest; + + while *left < hole.end && right < v_end { + // Consume the lesser side. + // If equal, prefer the left run to maintain stability. + unsafe { + let to_copy = if is_less(&*right, &**left) { + get_and_increment(&mut right) + } else { + get_and_increment(left) + }; + ptr::copy_nonoverlapping(to_copy, get_and_increment(out), 1); + } + } + } else { + // The right run is shorter. + unsafe { + ptr::copy_nonoverlapping(v_mid, buf, len - mid); + hole = MergeHole { start: buf, end: buf.add(len - mid), dest: v_mid }; + } + + // Initially, these pointers point past the ends of their arrays. + let left = &mut hole.dest; + let right = &mut hole.end; + let mut out = v_end; + + while v < *left && buf < *right { + // Consume the greater side. + // If equal, prefer the right run to maintain stability. + unsafe { + let to_copy = if is_less(&*right.sub(1), &*left.sub(1)) { + decrement_and_get(left) + } else { + decrement_and_get(right) + }; + ptr::copy_nonoverlapping(to_copy, decrement_and_get(&mut out), 1); + } + } + } + // Finally, `hole` gets dropped. If the shorter run was not fully consumed, whatever remains of + // it will now be copied into the hole in `v`. + + unsafe fn get_and_increment(ptr: &mut *mut T) -> *mut T { + let old = *ptr; + *ptr = unsafe { ptr.add(1) }; + old + } + + unsafe fn decrement_and_get(ptr: &mut *mut T) -> *mut T { + *ptr = unsafe { ptr.sub(1) }; + *ptr + } + + // When dropped, copies the range `start..end` into `dest..`. + struct MergeHole { + start: *mut T, + end: *mut T, + dest: *mut T, + } + + impl Drop for MergeHole { + fn drop(&mut self) { + // `T` is not a zero-sized type, and these are pointers into a slice's elements. + unsafe { + let len = self.end.sub_ptr(self.start); + ptr::copy_nonoverlapping(self.start, self.dest, len); + } + } + } +} + +/// This merge sort borrows some (but not all) ideas from TimSort, which used to be described in +/// detail [here](https://github.com/python/cpython/blob/main/Objects/listsort.txt). However Python +/// has switched to a Powersort based implementation. +/// +/// The algorithm identifies strictly descending and non-descending subsequences, which are called +/// natural runs. There is a stack of pending runs yet to be merged. Each newly found run is pushed +/// onto the stack, and then some pairs of adjacent runs are merged until these two invariants are +/// satisfied: +/// +/// 1. for every `i` in `1..runs.len()`: `runs[i - 1].len > runs[i].len` +/// 2. for every `i` in `2..runs.len()`: `runs[i - 2].len > runs[i - 1].len + runs[i].len` +/// +/// The invariants ensure that the total running time is *O*(*n* \* log(*n*)) worst-case. +pub fn merge_sort( + v: &mut [T], + is_less: &mut CmpF, + elem_alloc_fn: ElemAllocF, + elem_dealloc_fn: ElemDeallocF, + run_alloc_fn: RunAllocF, + run_dealloc_fn: RunDeallocF, +) where + CmpF: FnMut(&T, &T) -> bool, + ElemAllocF: Fn(usize) -> *mut T, + ElemDeallocF: Fn(*mut T, usize), + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), +{ + // Slices of up to this length get sorted using insertion sort. + const MAX_INSERTION: usize = 20; + // Very short runs are extended using insertion sort to span at least this many elements. + const MIN_RUN: usize = 10; + + // The caller should have already checked that. + debug_assert!(!T::IS_ZST); + + let len = v.len(); + + // Short arrays get sorted in-place via insertion sort to avoid allocations. + if len <= MAX_INSERTION { + if len >= 2 { + for i in (0..len - 1).rev() { + insert_head(&mut v[i..], is_less); + } + } + return; + } + + // Allocate a buffer to use as scratch memory. We keep the length 0 so we can keep in it + // shallow copies of the contents of `v` without risking the dtors running on copies if + // `is_less` panics. When merging two sorted runs, this buffer holds a copy of the shorter run, + // which will always have length at most `len / 2`. + let mut buf = BufGuard::new(len / 2, elem_alloc_fn, elem_dealloc_fn); + let buf_ptr = buf.buf_ptr; + + let mut runs = RunVec::new(run_alloc_fn, run_dealloc_fn); + + // In order to identify natural runs in `v`, we traverse it backwards. That might seem like a + // strange decision, but consider the fact that merges more often go in the opposite direction + // (forwards). According to benchmarks, merging forwards is slightly faster than merging + // backwards. To conclude, identifying runs by traversing backwards improves performance. + let mut end = len; + while end > 0 { + // Find the next natural run, and reverse it if it's strictly descending. + let mut start = end - 1; + if start > 0 { + start -= 1; + unsafe { + if is_less(v.get_unchecked(start + 1), v.get_unchecked(start)) { + while start > 0 && is_less(v.get_unchecked(start), v.get_unchecked(start - 1)) { + start -= 1; + } + v[start..end].reverse(); + } else { + while start > 0 && !is_less(v.get_unchecked(start), v.get_unchecked(start - 1)) + { + start -= 1; + } + } + } + } + + // Insert some more elements into the run if it's too short. Insertion sort is faster than + // merge sort on short sequences, so this significantly improves performance. + while start > 0 && end - start < MIN_RUN { + start -= 1; + insert_head(&mut v[start..end], is_less); + } + + // Push this run onto the stack. + runs.push(TimSortRun { start, len: end - start }); + end = start; + + // Merge some pairs of adjacent runs to satisfy the invariants. + while let Some(r) = collapse(runs.as_slice()) { + let left = runs[r + 1]; + let right = runs[r]; + unsafe { + merge(&mut v[left.start..right.start + right.len], left.len, buf_ptr, is_less); + } + runs[r] = TimSortRun { start: left.start, len: left.len + right.len }; + runs.remove(r + 1); + } + } + + // Finally, exactly one run must remain in the stack. + debug_assert!(runs.len() == 1 && runs[0].start == 0 && runs[0].len == len); + + // Examines the stack of runs and identifies the next pair of runs to merge. More specifically, + // if `Some(r)` is returned, that means `runs[r]` and `runs[r + 1]` must be merged next. If the + // algorithm should continue building a new run instead, `None` is returned. + // + // TimSort is infamous for its buggy implementations, as described here: + // http://envisage-project.eu/timsort-specification-and-verification/ + // + // The gist of the story is: we must enforce the invariants on the top four runs on the stack. + // Enforcing them on just top three is not sufficient to ensure that the invariants will still + // hold for *all* runs in the stack. + // + // This function correctly checks invariants for the top four runs. Additionally, if the top + // run starts at index 0, it will always demand a merge operation until the stack is fully + // collapsed, in order to complete the sort. + #[inline] + fn collapse(runs: &[TimSortRun]) -> Option { + let n = runs.len(); + if n >= 2 + && (runs[n - 1].start == 0 + || runs[n - 2].len <= runs[n - 1].len + || (n >= 3 && runs[n - 3].len <= runs[n - 2].len + runs[n - 1].len) + || (n >= 4 && runs[n - 4].len <= runs[n - 3].len + runs[n - 2].len)) + { + if n >= 3 && runs[n - 3].len < runs[n - 1].len { Some(n - 3) } else { Some(n - 2) } + } else { + None + } + } + + // Extremely basic versions of Vec. + // Their use is super limited and by having the code here, it allows reuse between the sort + // implementations. + struct BufGuard + where + ElemAllocF: Fn(usize) -> *mut T, + ElemDeallocF: Fn(*mut T, usize), + { + buf_ptr: *mut T, + capacity: usize, + elem_alloc_fn: ElemAllocF, + elem_dealloc_fn: ElemDeallocF, + } + + impl BufGuard + where + ElemAllocF: Fn(usize) -> *mut T, + ElemDeallocF: Fn(*mut T, usize), + { + fn new(len: usize, elem_alloc_fn: ElemAllocF, elem_dealloc_fn: ElemDeallocF) -> Self { + Self { buf_ptr: elem_alloc_fn(len), capacity: len, elem_alloc_fn, elem_dealloc_fn } + } + } + + impl Drop for BufGuard + where + ElemAllocF: Fn(usize) -> *mut T, + ElemDeallocF: Fn(*mut T, usize), + { + fn drop(&mut self) { + (self.elem_dealloc_fn)(self.buf_ptr, self.capacity); + } + } + + struct RunVec + where + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), + { + buf_ptr: *mut TimSortRun, + capacity: usize, + len: usize, + run_alloc_fn: RunAllocF, + run_dealloc_fn: RunDeallocF, + } + + impl RunVec + where + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), + { + fn new(run_alloc_fn: RunAllocF, run_dealloc_fn: RunDeallocF) -> Self { + // Most slices can be sorted with at most 16 runs in-flight. + const START_RUN_CAPACITY: usize = 16; + + Self { + buf_ptr: run_alloc_fn(START_RUN_CAPACITY), + capacity: START_RUN_CAPACITY, + len: 0, + run_alloc_fn, + run_dealloc_fn, + } + } + + fn push(&mut self, val: TimSortRun) { + if self.len == self.capacity { + let old_capacity = self.capacity; + let old_buf_ptr = self.buf_ptr; + + self.capacity = self.capacity * 2; + self.buf_ptr = (self.run_alloc_fn)(self.capacity); + + // SAFETY: buf_ptr new and old were correctly allocated and old_buf_ptr has + // old_capacity valid elements. + unsafe { + ptr::copy_nonoverlapping(old_buf_ptr, self.buf_ptr, old_capacity); + } + + (self.run_dealloc_fn)(old_buf_ptr, old_capacity); + } + + // SAFETY: The invariant was just checked. + unsafe { + self.buf_ptr.add(self.len).write(val); + } + self.len += 1; + } + + fn remove(&mut self, index: usize) { + if index >= self.len { + panic!("Index out of bounds"); + } + + // SAFETY: buf_ptr needs to be valid and len invariant upheld. + unsafe { + // the place we are taking from. + let ptr = self.buf_ptr.add(index); + + // Shift everything down to fill in that spot. + ptr::copy(ptr.add(1), ptr, self.len - index - 1); + } + self.len -= 1; + } + + fn as_slice(&self) -> &[TimSortRun] { + // SAFETY: Safe as long as buf_ptr is valid and len invariant was upheld. + unsafe { &*ptr::slice_from_raw_parts(self.buf_ptr, self.len) } + } + + fn len(&self) -> usize { + self.len + } + } + + impl core::ops::Index for RunVec + where + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), + { + type Output = TimSortRun; + + fn index(&self, index: usize) -> &Self::Output { + if index < self.len { + // SAFETY: buf_ptr and len invariant must be upheld. + unsafe { + return &*(self.buf_ptr.add(index)); + } + } + + panic!("Index out of bounds"); + } + } + + impl core::ops::IndexMut for RunVec + where + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), + { + fn index_mut(&mut self, index: usize) -> &mut Self::Output { + if index < self.len { + // SAFETY: buf_ptr and len invariant must be upheld. + unsafe { + return &mut *(self.buf_ptr.add(index)); + } + } + + panic!("Index out of bounds"); + } + } + + impl Drop for RunVec + where + RunAllocF: Fn(usize) -> *mut TimSortRun, + RunDeallocF: Fn(*mut TimSortRun, usize), + { + fn drop(&mut self) { + // As long as TimSortRun is Copy we don't need to drop them individually but just the + // whole allocation. + (self.run_dealloc_fn)(self.buf_ptr, self.capacity); + } + } +} + +/// Internal type used by merge_sort. +#[derive(Clone, Copy, Debug)] +pub struct TimSortRun { + len: usize, + start: usize, +} From 1ec59cdcd16fb2821bdf5944c3bbef51c57f7a5c Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Mon, 21 Nov 2022 14:20:31 +0100 Subject: [PATCH 02/20] Remove debug unused --- library/core/src/slice/sort.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/library/core/src/slice/sort.rs b/library/core/src/slice/sort.rs index 07c68bf6aa40a..8ef3535af2d53 100644 --- a/library/core/src/slice/sort.rs +++ b/library/core/src/slice/sort.rs @@ -9,8 +9,6 @@ //! In addition it also contains the core logic of the stable sort used by `slice::sort` based on //! TimSort. -#![allow(unused)] // FIXME debug - use crate::cmp; use crate::mem::{self, MaybeUninit, SizedTypeProperties}; use crate::ptr; @@ -1167,7 +1165,7 @@ pub fn merge_sort( // shallow copies of the contents of `v` without risking the dtors running on copies if // `is_less` panics. When merging two sorted runs, this buffer holds a copy of the shorter run, // which will always have length at most `len / 2`. - let mut buf = BufGuard::new(len / 2, elem_alloc_fn, elem_dealloc_fn); + let buf = BufGuard::new(len / 2, elem_alloc_fn, elem_dealloc_fn); let buf_ptr = buf.buf_ptr; let mut runs = RunVec::new(run_alloc_fn, run_dealloc_fn); @@ -1255,30 +1253,33 @@ pub fn merge_sort( // Extremely basic versions of Vec. // Their use is super limited and by having the code here, it allows reuse between the sort // implementations. - struct BufGuard + struct BufGuard where - ElemAllocF: Fn(usize) -> *mut T, ElemDeallocF: Fn(*mut T, usize), { buf_ptr: *mut T, capacity: usize, - elem_alloc_fn: ElemAllocF, elem_dealloc_fn: ElemDeallocF, } - impl BufGuard + impl BufGuard where - ElemAllocF: Fn(usize) -> *mut T, ElemDeallocF: Fn(*mut T, usize), { - fn new(len: usize, elem_alloc_fn: ElemAllocF, elem_dealloc_fn: ElemDeallocF) -> Self { - Self { buf_ptr: elem_alloc_fn(len), capacity: len, elem_alloc_fn, elem_dealloc_fn } + fn new( + len: usize, + elem_alloc_fn: ElemAllocF, + elem_dealloc_fn: ElemDeallocF, + ) -> Self + where + ElemAllocF: Fn(usize) -> *mut T, + { + Self { buf_ptr: elem_alloc_fn(len), capacity: len, elem_dealloc_fn } } } - impl Drop for BufGuard + impl Drop for BufGuard where - ElemAllocF: Fn(usize) -> *mut T, ElemDeallocF: Fn(*mut T, usize), { fn drop(&mut self) { From 4b5844fbe95a35370c19d74f9b70286897c3e153 Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Mon, 21 Nov 2022 14:30:56 +0100 Subject: [PATCH 03/20] Document all unsafe blocks There were several unsafe blocks in the existing implementation that were not documented with a SAFETY comment. --- library/core/src/slice/sort.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/library/core/src/slice/sort.rs b/library/core/src/slice/sort.rs index 8ef3535af2d53..3da00b8f79672 100644 --- a/library/core/src/slice/sort.rs +++ b/library/core/src/slice/sort.rs @@ -940,6 +940,7 @@ where F: FnMut(&T, &T) -> bool, { if v.len() >= 2 && is_less(&v[1], &v[0]) { + // SAFETY: Copy tmp back even if panic, and ensure unique observation. unsafe { // There are three ways to implement insertion here: // @@ -992,6 +993,7 @@ where impl Drop for InsertionHole { fn drop(&mut self) { + // SAFETY: The caller must ensure that src and dest are correctly set. unsafe { ptr::copy_nonoverlapping(self.src, self.dest, 1); } @@ -1012,6 +1014,8 @@ where { let len = v.len(); let v = v.as_mut_ptr(); + + // SAFETY: mid and len must be in-bounds of v. let (v_mid, v_end) = unsafe { (v.add(mid), v.add(len)) }; // The merge process first copies the shorter run into `buf`. Then it traces the newly copied @@ -1035,6 +1039,8 @@ where if mid <= len - mid { // The left run is shorter. + + // SAFETY: buf must have enough capacity for `v[..mid]`. unsafe { ptr::copy_nonoverlapping(v, buf, mid); hole = MergeHole { start: buf, end: buf.add(mid), dest: v }; @@ -1048,6 +1054,8 @@ where while *left < hole.end && right < v_end { // Consume the lesser side. // If equal, prefer the left run to maintain stability. + + // SAFETY: left and right must be valid and part of v same for out. unsafe { let to_copy = if is_less(&*right, &**left) { get_and_increment(&mut right) @@ -1059,6 +1067,8 @@ where } } else { // The right run is shorter. + + // SAFETY: buf must have enough capacity for `v[mid..]`. unsafe { ptr::copy_nonoverlapping(v_mid, buf, len - mid); hole = MergeHole { start: buf, end: buf.add(len - mid), dest: v_mid }; @@ -1072,6 +1082,8 @@ where while v < *left && buf < *right { // Consume the greater side. // If equal, prefer the right run to maintain stability. + + // SAFETY: left and right must be valid and part of v same for out. unsafe { let to_copy = if is_less(&*right.sub(1), &*left.sub(1)) { decrement_and_get(left) @@ -1087,11 +1099,14 @@ where unsafe fn get_and_increment(ptr: &mut *mut T) -> *mut T { let old = *ptr; + + // SAFETY: ptr.add(1) must still be a valid pointer and part of `v`. *ptr = unsafe { ptr.add(1) }; old } unsafe fn decrement_and_get(ptr: &mut *mut T) -> *mut T { + // SAFETY: ptr.sub(1) must still be a valid pointer and part of `v`. *ptr = unsafe { ptr.sub(1) }; *ptr } @@ -1105,7 +1120,7 @@ where impl Drop for MergeHole { fn drop(&mut self) { - // `T` is not a zero-sized type, and these are pointers into a slice's elements. + // SAFETY: `T` is not a zero-sized type, and these are pointers into a slice's elements. unsafe { let len = self.end.sub_ptr(self.start); ptr::copy_nonoverlapping(self.start, self.dest, len); @@ -1180,6 +1195,8 @@ pub fn merge_sort( let mut start = end - 1; if start > 0 { start -= 1; + + // SAFETY: The v.get_unchecked must be fed with correct inbound indicies. unsafe { if is_less(v.get_unchecked(start + 1), v.get_unchecked(start)) { while start > 0 && is_less(v.get_unchecked(start), v.get_unchecked(start - 1)) { @@ -1210,6 +1227,8 @@ pub fn merge_sort( while let Some(r) = collapse(runs.as_slice()) { let left = runs[r + 1]; let right = runs[r]; + // SAFETY: `buf_ptr` must hold enough capacity for the shorter of the two sides, and + // neither side may be on length 0. unsafe { merge(&mut v[left.start..right.start + right.len], left.len, buf_ptr, is_less); } From 280f69d8585b676b0441cdb476634882ebe1b983 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 19 Jan 2023 15:24:00 +0000 Subject: [PATCH 04/20] Fix IndexVec::drain_enumerated --- compiler/rustc_index/src/vec.rs | 7 ++++++- .../src/solve/trait_goals/structural_traits.rs | 15 +++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_index/src/vec.rs b/compiler/rustc_index/src/vec.rs index c18a911b2fbcd..68cdc6d7711d4 100644 --- a/compiler/rustc_index/src/vec.rs +++ b/compiler/rustc_index/src/vec.rs @@ -207,7 +207,12 @@ impl IndexVec { &'a mut self, range: R, ) -> impl Iterator + 'a { - self.raw.drain(range).enumerate().map(|(n, t)| (I::new(n), t)) + let begin = match range.start_bound() { + std::ops::Bound::Included(i) => *i, + std::ops::Bound::Excluded(i) => i.checked_add(1).unwrap(), + std::ops::Bound::Unbounded => 0, + }; + self.raw.drain(range).enumerate().map(move |(n, t)| (I::new(begin + n), t)) } #[inline] diff --git a/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs b/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs index bbc0c77253278..2c75d0907b739 100644 --- a/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs +++ b/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs @@ -30,10 +30,7 @@ pub(super) fn instantiate_constituent_tys_for_auto_trait<'tcx>( | ty::Foreign(..) | ty::Alias(ty::Projection, ..) | ty::Bound(..) - | ty::Infer(ty::TyVar(_)) => { - // FIXME: Do we need to mark anything as ambiguous here? Yeah? - Err(NoSolution) - } + | ty::Infer(ty::TyVar(_)) => Err(NoSolution), ty::Infer(ty::FreshTy(_) | ty::FreshIntTy(_) | ty::FreshFloatTy(_)) => bug!(), @@ -101,9 +98,8 @@ pub(super) fn instantiate_constituent_tys_for_sized_trait<'tcx>( | ty::Dynamic(..) | ty::Foreign(..) | ty::Alias(..) - | ty::Param(_) => Err(NoSolution), - - ty::Infer(ty::TyVar(_)) => bug!("FIXME: ambiguous"), + | ty::Param(_) + | ty::Infer(ty::TyVar(_)) => Err(NoSolution), ty::Placeholder(..) | ty::Bound(..) @@ -151,9 +147,8 @@ pub(super) fn instantiate_constituent_tys_for_copy_clone_trait<'tcx>( | ty::Ref(_, _, Mutability::Mut) | ty::Adt(_, _) | ty::Alias(_, _) - | ty::Param(_) => Err(NoSolution), - - ty::Infer(ty::TyVar(_)) => bug!("FIXME: ambiguous"), + | ty::Param(_) + | ty::Infer(ty::TyVar(_)) => Err(NoSolution), ty::Placeholder(..) | ty::Bound(..) From c9c8e294d2b6fc7c83641476f4986a7bf5e84817 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 19 Jan 2023 03:26:54 +0000 Subject: [PATCH 05/20] HACK: self ty ambiguity hack --- compiler/rustc_trait_selection/src/solve/assembly.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs index 2336fb53aec28..2c92e7eb9ad3f 100644 --- a/compiler/rustc_trait_selection/src/solve/assembly.rs +++ b/compiler/rustc_trait_selection/src/solve/assembly.rs @@ -1,7 +1,7 @@ //! Code shared by trait and projection goals for candidate assembly. use super::infcx_ext::InferCtxtExt; -use super::{CanonicalResponse, EvalCtxt, Goal, QueryResult}; +use super::{CanonicalResponse, Certainty, EvalCtxt, Goal, MaybeCause, QueryResult}; use rustc_hir::def_id::DefId; use rustc_infer::traits::query::NoSolution; use rustc_infer::traits::util::elaborate_predicates; @@ -124,6 +124,16 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { &mut self, goal: Goal<'tcx, G>, ) -> Vec> { + // HACK: `_: Trait` is ambiguous, because it may be satisfied via a builtin rule, + // object bound, alias bound, etc. We are unable to determine this until we can at + // least structually resolve the type one layer. + if goal.predicate.self_ty().is_ty_var() { + return vec![Candidate { + source: CandidateSource::BuiltinImpl, + result: self.make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)).unwrap(), + }]; + } + let mut candidates = Vec::new(); self.assemble_candidates_after_normalizing_self_ty(goal, &mut candidates); From aee75f25cbfb187f74d2b8453b0d902ad6ced391 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 19 Jan 2023 15:32:20 +0000 Subject: [PATCH 06/20] Assert goal is fully normalized during assemble --- compiler/rustc_trait_selection/src/solve/assembly.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs index 2c92e7eb9ad3f..45bce34b1edd8 100644 --- a/compiler/rustc_trait_selection/src/solve/assembly.rs +++ b/compiler/rustc_trait_selection/src/solve/assembly.rs @@ -79,7 +79,7 @@ pub(super) enum CandidateSource { AliasBound(usize), } -pub(super) trait GoalKind<'tcx>: TypeFoldable<'tcx> + Copy { +pub(super) trait GoalKind<'tcx>: TypeFoldable<'tcx> + Copy + Eq { fn self_ty(self) -> Ty<'tcx>; fn with_self_ty(self, tcx: TyCtxt<'tcx>, self_ty: Ty<'tcx>) -> Self; @@ -124,6 +124,8 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { &mut self, goal: Goal<'tcx, G>, ) -> Vec> { + debug_assert_eq!(goal, self.infcx.resolve_vars_if_possible(goal)); + // HACK: `_: Trait` is ambiguous, because it may be satisfied via a builtin rule, // object bound, alias bound, etc. We are unable to determine this until we can at // least structually resolve the type one layer. @@ -179,6 +181,7 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { Ok((_, certainty)) => certainty, Err(NoSolution) => return, }; + let normalized_ty = self.infcx.resolve_vars_if_possible(normalized_ty); // NOTE: Alternatively we could call `evaluate_goal` here and only have a `Normalized` candidate. // This doesn't work as long as we use `CandidateSource` in winnowing. From f53f5b4463a28b44d036342f0a849390495f6a9b Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 19 Jan 2023 04:50:14 +0000 Subject: [PATCH 07/20] swap Ambiguity and Unimplemented in new trait engine --- compiler/rustc_trait_selection/src/solve/fulfill.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/fulfill.rs b/compiler/rustc_trait_selection/src/solve/fulfill.rs index b086c0684d28d..a6240666ed43a 100644 --- a/compiler/rustc_trait_selection/src/solve/fulfill.rs +++ b/compiler/rustc_trait_selection/src/solve/fulfill.rs @@ -52,7 +52,7 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentCtxt<'tcx> { .drain(..) .map(|obligation| FulfillmentError { obligation: obligation.clone(), - code: FulfillmentErrorCode::CodeSelectionError(SelectionError::Unimplemented), + code: FulfillmentErrorCode::CodeAmbiguity, root_obligation: obligation, }) .collect() @@ -75,7 +75,9 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentCtxt<'tcx> { Err(NoSolution) => { errors.push(FulfillmentError { obligation: obligation.clone(), - code: FulfillmentErrorCode::CodeAmbiguity, + code: FulfillmentErrorCode::CodeSelectionError( + SelectionError::Unimplemented, + ), root_obligation: obligation, }); continue; From 69890b2df447e7d3febff1a0f8974f3fcf694128 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 18 Jan 2023 23:21:12 +0000 Subject: [PATCH 08/20] trait solver: PointerSized --- .../src/solve/assembly.rs | 7 +++++ .../src/solve/project_goals.rs | 7 +++++ .../src/solve/trait_goals.rs | 27 +++++++++++++++++-- tests/ui/traits/new-solver/pointer-sized.rs | 12 +++++++++ .../ui/traits/new-solver/pointer-sized.stderr | 24 +++++++++++++++++ 5 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 tests/ui/traits/new-solver/pointer-sized.rs create mode 100644 tests/ui/traits/new-solver/pointer-sized.stderr diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs index 45bce34b1edd8..52155aa0f1894 100644 --- a/compiler/rustc_trait_selection/src/solve/assembly.rs +++ b/compiler/rustc_trait_selection/src/solve/assembly.rs @@ -117,6 +117,11 @@ pub(super) trait GoalKind<'tcx>: TypeFoldable<'tcx> + Copy + Eq { ecx: &mut EvalCtxt<'_, 'tcx>, goal: Goal<'tcx, Self>, ) -> QueryResult<'tcx>; + + fn consider_builtin_pointer_sized_candidate( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx>; } impl<'tcx> EvalCtxt<'_, 'tcx> { @@ -237,6 +242,8 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { || lang_items.clone_trait() == Some(trait_def_id) { G::consider_builtin_copy_clone_candidate(self, goal) + } else if lang_items.pointer_sized() == Some(trait_def_id) { + G::consider_builtin_pointer_sized_candidate(self, goal) } else { Err(NoSolution) }; diff --git a/compiler/rustc_trait_selection/src/solve/project_goals.rs b/compiler/rustc_trait_selection/src/solve/project_goals.rs index ffc1c70e0cb81..3f3d32efe7f53 100644 --- a/compiler/rustc_trait_selection/src/solve/project_goals.rs +++ b/compiler/rustc_trait_selection/src/solve/project_goals.rs @@ -351,6 +351,13 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> { ) -> QueryResult<'tcx> { bug!("`Copy`/`Clone` does not have an associated type: {:?}", goal); } + + fn consider_builtin_pointer_sized_candidate( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx> { + bug!("`PointerSized` does not have an associated type: {:?}", goal); + } } /// This behavior is also implemented in `rustc_ty_utils` and in the old `project` code. diff --git a/compiler/rustc_trait_selection/src/solve/trait_goals.rs b/compiler/rustc_trait_selection/src/solve/trait_goals.rs index 1ebcfd03c14ea..f68a296922aae 100644 --- a/compiler/rustc_trait_selection/src/solve/trait_goals.rs +++ b/compiler/rustc_trait_selection/src/solve/trait_goals.rs @@ -4,13 +4,13 @@ use std::iter; use super::assembly::{self, Candidate, CandidateSource}; use super::infcx_ext::InferCtxtExt; -use super::{EvalCtxt, Goal, QueryResult}; +use super::{Certainty, EvalCtxt, Goal, QueryResult}; use rustc_hir::def_id::DefId; use rustc_infer::infer::InferCtxt; use rustc_infer::traits::query::NoSolution; use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams}; -use rustc_middle::ty::TraitPredicate; use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_middle::ty::{TraitPredicate, TypeVisitable}; use rustc_span::DUMMY_SP; mod structural_traits; @@ -127,6 +127,29 @@ impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> { structural_traits::instantiate_constituent_tys_for_copy_clone_trait, ) } + + fn consider_builtin_pointer_sized_candidate( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx> { + if goal.predicate.self_ty().has_non_region_infer() { + return ecx.make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)); + } + + let tcx = ecx.tcx(); + let self_ty = tcx.erase_regions(goal.predicate.self_ty()); + + if let Ok(layout) = tcx.layout_of(goal.param_env.and(self_ty)) + && let usize_layout = tcx.layout_of(ty::ParamEnv::empty().and(tcx.types.usize)).unwrap().layout + && layout.layout.size() == usize_layout.size() + && layout.layout.align().abi == usize_layout.align().abi + { + // FIXME: We could make this faster by making a no-constraints response + ecx.make_canonical_response(Certainty::Yes) + } else { + Err(NoSolution) + } + } } impl<'tcx> EvalCtxt<'_, 'tcx> { diff --git a/tests/ui/traits/new-solver/pointer-sized.rs b/tests/ui/traits/new-solver/pointer-sized.rs new file mode 100644 index 0000000000000..15681cd132ec6 --- /dev/null +++ b/tests/ui/traits/new-solver/pointer-sized.rs @@ -0,0 +1,12 @@ +#![feature(pointer_sized_trait)] + +use std::marker::PointerSized; + +fn require_pointer_sized(_: impl PointerSized) {} + +fn main() { + require_pointer_sized(1usize); + require_pointer_sized(1u16); + //~^ ERROR `u16` needs to be a pointer-sized type + require_pointer_sized(&1i16); +} diff --git a/tests/ui/traits/new-solver/pointer-sized.stderr b/tests/ui/traits/new-solver/pointer-sized.stderr new file mode 100644 index 0000000000000..b250b1331bbf9 --- /dev/null +++ b/tests/ui/traits/new-solver/pointer-sized.stderr @@ -0,0 +1,24 @@ +error[E0277]: `u16` needs to be a pointer-sized type + --> $DIR/pointer-sized.rs:9:27 + | +LL | require_pointer_sized(1u16); + | --------------------- ^^^^ the trait `PointerSized` is not implemented for `u16` + | | + | required by a bound introduced by this call + | + = note: the trait bound `u16: PointerSized` is not satisfied +note: required by a bound in `require_pointer_sized` + --> $DIR/pointer-sized.rs:5:34 + | +LL | fn require_pointer_sized(_: impl PointerSized) {} + | ^^^^^^^^^^^^ required by this bound in `require_pointer_sized` +help: consider borrowing here + | +LL | require_pointer_sized(&1u16); + | + +LL | require_pointer_sized(&mut 1u16); + | ++++ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. From ed6aebbfecb56ae9ec2702572b70d6e350433de2 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 19 Jan 2023 01:20:34 +0000 Subject: [PATCH 09/20] trait solver: Implement Fn traits and tuple trait --- .../src/solve/assembly.rs | 19 ++++++- .../src/solve/project_goals.rs | 38 +++++++++++++- .../src/solve/trait_goals.rs | 41 +++++++++++++-- .../solve/trait_goals/structural_traits.rs | 51 ++++++++++++++++++- .../ui/traits/new-solver/fn-trait-closure.rs | 15 ++++++ tests/ui/traits/new-solver/fn-trait.rs | 13 +++++ 6 files changed, 170 insertions(+), 7 deletions(-) create mode 100644 tests/ui/traits/new-solver/fn-trait-closure.rs create mode 100644 tests/ui/traits/new-solver/fn-trait.rs diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs index 52155aa0f1894..31c1bc9ecc062 100644 --- a/compiler/rustc_trait_selection/src/solve/assembly.rs +++ b/compiler/rustc_trait_selection/src/solve/assembly.rs @@ -122,6 +122,17 @@ pub(super) trait GoalKind<'tcx>: TypeFoldable<'tcx> + Copy + Eq { ecx: &mut EvalCtxt<'_, 'tcx>, goal: Goal<'tcx, Self>, ) -> QueryResult<'tcx>; + + fn consider_builtin_fn_trait_candidates( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + kind: ty::ClosureKind, + ) -> QueryResult<'tcx>; + + fn consider_builtin_tuple_candidate( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx>; } impl<'tcx> EvalCtxt<'_, 'tcx> { @@ -137,7 +148,9 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { if goal.predicate.self_ty().is_ty_var() { return vec![Candidate { source: CandidateSource::BuiltinImpl, - result: self.make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)).unwrap(), + result: self + .make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)) + .unwrap(), }]; } @@ -244,6 +257,10 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { G::consider_builtin_copy_clone_candidate(self, goal) } else if lang_items.pointer_sized() == Some(trait_def_id) { G::consider_builtin_pointer_sized_candidate(self, goal) + } else if let Some(kind) = self.tcx().fn_trait_kind_from_def_id(trait_def_id) { + G::consider_builtin_fn_trait_candidates(self, goal, kind) + } else if lang_items.tuple_trait() == Some(trait_def_id) { + G::consider_builtin_tuple_candidate(self, goal) } else { Err(NoSolution) }; diff --git a/compiler/rustc_trait_selection/src/solve/project_goals.rs b/compiler/rustc_trait_selection/src/solve/project_goals.rs index 3f3d32efe7f53..e39fa05339286 100644 --- a/compiler/rustc_trait_selection/src/solve/project_goals.rs +++ b/compiler/rustc_trait_selection/src/solve/project_goals.rs @@ -2,6 +2,7 @@ use crate::traits::{specialization_graph, translate_substs}; use super::assembly::{self, Candidate, CandidateSource}; use super::infcx_ext::InferCtxtExt; +use super::trait_goals::structural_traits; use super::{Certainty, EvalCtxt, Goal, MaybeCause, QueryResult}; use rustc_errors::ErrorGuaranteed; use rustc_hir::def::DefKind; @@ -11,9 +12,9 @@ use rustc_infer::traits::query::NoSolution; use rustc_infer::traits::specialization_graph::LeafDef; use rustc_infer::traits::Reveal; use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams}; -use rustc_middle::ty::TypeVisitable; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_middle::ty::{ProjectionPredicate, TypeSuperVisitable, TypeVisitor}; +use rustc_middle::ty::{ToPredicate, TypeVisitable}; use rustc_span::DUMMY_SP; use std::iter; use std::ops::ControlFlow; @@ -353,11 +354,44 @@ impl<'tcx> assembly::GoalKind<'tcx> for ProjectionPredicate<'tcx> { } fn consider_builtin_pointer_sized_candidate( - ecx: &mut EvalCtxt<'_, 'tcx>, + _ecx: &mut EvalCtxt<'_, 'tcx>, goal: Goal<'tcx, Self>, ) -> QueryResult<'tcx> { bug!("`PointerSized` does not have an associated type: {:?}", goal); } + + fn consider_builtin_fn_trait_candidates( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + goal_kind: ty::ClosureKind, + ) -> QueryResult<'tcx> { + if let Some(tupled_inputs_and_output) = + structural_traits::extract_tupled_inputs_and_output_from_callable( + ecx.tcx(), + goal.predicate.self_ty(), + goal_kind, + )? + { + let pred = tupled_inputs_and_output + .map_bound(|(inputs, output)| ty::ProjectionPredicate { + projection_ty: ecx + .tcx() + .mk_alias_ty(goal.predicate.def_id(), [goal.predicate.self_ty(), inputs]), + term: output.into(), + }) + .to_predicate(ecx.tcx()); + Self::consider_assumption(ecx, goal, pred) + } else { + ecx.make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)) + } + } + + fn consider_builtin_tuple_candidate( + _ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx> { + bug!("`Tuple` does not have an associated type: {:?}", goal); + } } /// This behavior is also implemented in `rustc_ty_utils` and in the old `project` code. diff --git a/compiler/rustc_trait_selection/src/solve/trait_goals.rs b/compiler/rustc_trait_selection/src/solve/trait_goals.rs index f68a296922aae..9985d7181bb7d 100644 --- a/compiler/rustc_trait_selection/src/solve/trait_goals.rs +++ b/compiler/rustc_trait_selection/src/solve/trait_goals.rs @@ -4,16 +4,16 @@ use std::iter; use super::assembly::{self, Candidate, CandidateSource}; use super::infcx_ext::InferCtxtExt; -use super::{Certainty, EvalCtxt, Goal, QueryResult}; +use super::{Certainty, EvalCtxt, Goal, MaybeCause, QueryResult}; use rustc_hir::def_id::DefId; use rustc_infer::infer::InferCtxt; use rustc_infer::traits::query::NoSolution; use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams}; -use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_middle::ty::{self, ToPredicate, Ty, TyCtxt}; use rustc_middle::ty::{TraitPredicate, TypeVisitable}; use rustc_span::DUMMY_SP; -mod structural_traits; +pub mod structural_traits; impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> { fn self_ty(self) -> Ty<'tcx> { @@ -150,6 +150,41 @@ impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> { Err(NoSolution) } } + + fn consider_builtin_fn_trait_candidates( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + goal_kind: ty::ClosureKind, + ) -> QueryResult<'tcx> { + if let Some(tupled_inputs_and_output) = + structural_traits::extract_tupled_inputs_and_output_from_callable( + ecx.tcx(), + goal.predicate.self_ty(), + goal_kind, + )? + { + let pred = tupled_inputs_and_output + .map_bound(|(inputs, _)| { + ecx.tcx() + .mk_trait_ref(goal.predicate.def_id(), [goal.predicate.self_ty(), inputs]) + }) + .to_predicate(ecx.tcx()); + Self::consider_assumption(ecx, goal, pred) + } else { + ecx.make_canonical_response(Certainty::Maybe(MaybeCause::Ambiguity)) + } + } + + fn consider_builtin_tuple_candidate( + ecx: &mut EvalCtxt<'_, 'tcx>, + goal: Goal<'tcx, Self>, + ) -> QueryResult<'tcx> { + if let ty::Tuple(..) = goal.predicate.self_ty().kind() { + ecx.make_canonical_response(Certainty::Yes) + } else { + Err(NoSolution) + } + } } impl<'tcx> EvalCtxt<'_, 'tcx> { diff --git a/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs b/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs index 2c75d0907b739..a11cd13cb0856 100644 --- a/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs +++ b/compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs @@ -1,6 +1,6 @@ use rustc_hir::{Movability, Mutability}; use rustc_infer::{infer::InferCtxt, traits::query::NoSolution}; -use rustc_middle::ty::{self, Ty}; +use rustc_middle::ty::{self, Ty, TyCtxt}; // Calculates the constituent types of a type for `auto trait` purposes. // @@ -172,3 +172,52 @@ pub(super) fn instantiate_constituent_tys_for_copy_clone_trait<'tcx>( } } } + +pub(crate) fn extract_tupled_inputs_and_output_from_callable<'tcx>( + tcx: TyCtxt<'tcx>, + self_ty: Ty<'tcx>, + goal_kind: ty::ClosureKind, +) -> Result, Ty<'tcx>)>>, NoSolution> { + match *self_ty.kind() { + ty::FnDef(def_id, substs) => Ok(Some( + tcx.bound_fn_sig(def_id) + .subst(tcx, substs) + .map_bound(|sig| (tcx.mk_tup(sig.inputs().iter()), sig.output())), + )), + ty::FnPtr(sig) => { + Ok(Some(sig.map_bound(|sig| (tcx.mk_tup(sig.inputs().iter()), sig.output())))) + } + ty::Closure(_, substs) => { + let closure_substs = substs.as_closure(); + match closure_substs.kind_ty().to_opt_closure_kind() { + Some(closure_kind) if closure_kind.extends(goal_kind) => {} + None => return Ok(None), + _ => return Err(NoSolution), + } + Ok(Some(closure_substs.sig().map_bound(|sig| (sig.inputs()[0], sig.output())))) + } + ty::Bool + | ty::Char + | ty::Int(_) + | ty::Uint(_) + | ty::Float(_) + | ty::Adt(_, _) + | ty::Foreign(_) + | ty::Str + | ty::Array(_, _) + | ty::Slice(_) + | ty::RawPtr(_) + | ty::Ref(_, _, _) + | ty::Dynamic(_, _, _) + | ty::Generator(_, _, _) + | ty::GeneratorWitness(_) + | ty::Never + | ty::Tuple(_) + | ty::Alias(_, _) + | ty::Param(_) + | ty::Placeholder(_) + | ty::Bound(_, _) + | ty::Infer(_) + | ty::Error(_) => Err(NoSolution), + } +} diff --git a/tests/ui/traits/new-solver/fn-trait-closure.rs b/tests/ui/traits/new-solver/fn-trait-closure.rs new file mode 100644 index 0000000000000..c0ecf1c91fb38 --- /dev/null +++ b/tests/ui/traits/new-solver/fn-trait-closure.rs @@ -0,0 +1,15 @@ +// compile-flags: -Ztrait-solver=next +// known-bug: unknown +// failure-status: 101 +// dont-check-compiler-stderr + +// This test will fail until we fix `FulfillmentCtxt::relationships`. That's +// because we create a type variable for closure upvar types, which is not +// constrained until after we try to do fallback on diverging type variables. +// Thus, we will call that function, which is unimplemented. + +fn require_fn(_: impl Fn() -> i32) {} + +fn main() { + require_fn(|| -> i32 { 1i32 }); +} diff --git a/tests/ui/traits/new-solver/fn-trait.rs b/tests/ui/traits/new-solver/fn-trait.rs new file mode 100644 index 0000000000000..d566ead105c86 --- /dev/null +++ b/tests/ui/traits/new-solver/fn-trait.rs @@ -0,0 +1,13 @@ +// compile-flags: -Ztrait-solver=next +// check-pass + +fn require_fn(_: impl Fn() -> i32) {} + +fn f() -> i32 { + 1i32 +} + +fn main() { + require_fn(f); + require_fn(f as fn() -> i32); +} From 05889fc52e8d18f96df659660eff9cd5e9ff3f08 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Thu, 19 Jan 2023 16:30:05 -0700 Subject: [PATCH 10/20] rustdoc: remove redundant CSS selector `.sidebar .current` Since the current sidebar item is already a link, it doesn't do anything. --- src/librustdoc/html/static/css/rustdoc.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index a08b8d89db67d..a93f60da2adf2 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -497,7 +497,7 @@ ul.block, .block li { padding-left: 24px; } -.sidebar a, .sidebar .current { +.sidebar a { color: var(--sidebar-link-color); } .sidebar .current, From 734f375019ecceaf4adad2423b0035f070dda354 Mon Sep 17 00:00:00 2001 From: --global <39689890+timrobertsdev@users.noreply.github.com> Date: Tue, 8 Nov 2022 10:24:06 -0500 Subject: [PATCH 11/20] Change `bindings_with_variant_name` to deny-by-default --- compiler/rustc_lint_defs/src/builtin.rs | 2 +- tests/ui/issues/issue-19100.fixed | 5 ++-- tests/ui/issues/issue-19100.rs | 5 ++-- tests/ui/issues/issue-19100.stderr | 12 ++++---- tests/ui/lint/issue-30302.rs | 2 +- tests/ui/lint/issue-30302.stderr | 6 ++-- tests/ui/lint/lint-uppercase-variables.rs | 6 ++-- tests/ui/lint/lint-uppercase-variables.stderr | 10 +++---- tests/ui/pattern/issue-14221.rs | 4 +-- tests/ui/pattern/issue-14221.stderr | 8 +++--- ...67776-match-same-name-enum-variant-refs.rs | 16 +++++------ ...6-match-same-name-enum-variant-refs.stderr | 28 +++++++++---------- tests/ui/suggestions/issue-88730.rs | 3 +- tests/ui/suggestions/issue-88730.stderr | 10 ++----- 14 files changed, 54 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 6cdf50970836a..0748dd94c457c 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -743,7 +743,7 @@ declare_lint! { /// [identifier pattern]: https://doc.rust-lang.org/reference/patterns.html#identifier-patterns /// [path pattern]: https://doc.rust-lang.org/reference/patterns.html#path-patterns pub BINDINGS_WITH_VARIANT_NAME, - Warn, + Deny, "detects pattern bindings with the same name as one of the matched variants" } diff --git a/tests/ui/issues/issue-19100.fixed b/tests/ui/issues/issue-19100.fixed index 6dc8f7ddbc972..029855de2dea0 100644 --- a/tests/ui/issues/issue-19100.fixed +++ b/tests/ui/issues/issue-19100.fixed @@ -1,4 +1,3 @@ -// run-pass // run-rustfix #![allow(non_snake_case)] @@ -16,11 +15,11 @@ impl Foo { match self { & Foo::Bar if true -//~^ WARN pattern binding `Bar` is named the same as one of the variants of the type `Foo` +//~^ ERROR pattern binding `Bar` is named the same as one of the variants of the type `Foo` => println!("bar"), & Foo::Baz if false -//~^ WARN pattern binding `Baz` is named the same as one of the variants of the type `Foo` +//~^ ERROR pattern binding `Baz` is named the same as one of the variants of the type `Foo` => println!("baz"), _ => () } diff --git a/tests/ui/issues/issue-19100.rs b/tests/ui/issues/issue-19100.rs index cfdc7c9e75488..bd9e4ea5b601b 100644 --- a/tests/ui/issues/issue-19100.rs +++ b/tests/ui/issues/issue-19100.rs @@ -1,4 +1,3 @@ -// run-pass // run-rustfix #![allow(non_snake_case)] @@ -16,11 +15,11 @@ impl Foo { match self { & Bar if true -//~^ WARN pattern binding `Bar` is named the same as one of the variants of the type `Foo` +//~^ ERROR pattern binding `Bar` is named the same as one of the variants of the type `Foo` => println!("bar"), & Baz if false -//~^ WARN pattern binding `Baz` is named the same as one of the variants of the type `Foo` +//~^ ERROR pattern binding `Baz` is named the same as one of the variants of the type `Foo` => println!("baz"), _ => () } diff --git a/tests/ui/issues/issue-19100.stderr b/tests/ui/issues/issue-19100.stderr index 293430691ddcf..ebbf083b7dea8 100644 --- a/tests/ui/issues/issue-19100.stderr +++ b/tests/ui/issues/issue-19100.stderr @@ -1,17 +1,17 @@ -warning[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-19100.rs:18:1 +error[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-19100.rs:17:1 | LL | Bar if true | ^^^ help: to match on the variant, qualify the path: `Foo::Bar` | - = note: `#[warn(bindings_with_variant_name)]` on by default + = note: `#[deny(bindings_with_variant_name)]` on by default -warning[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-19100.rs:22:1 +error[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-19100.rs:21:1 | LL | Baz if false | ^^^ help: to match on the variant, qualify the path: `Foo::Baz` -warning: 2 warnings emitted +error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0170`. diff --git a/tests/ui/lint/issue-30302.rs b/tests/ui/lint/issue-30302.rs index c37d4f29d10e3..5eccb8cd5d8d2 100644 --- a/tests/ui/lint/issue-30302.rs +++ b/tests/ui/lint/issue-30302.rs @@ -11,7 +11,7 @@ enum Stack { fn is_empty(s: Stack) -> bool { match s { Nil => true, -//~^ WARN pattern binding `Nil` is named the same as one of the variants of the type `Stack` +//~^ ERROR pattern binding `Nil` is named the same as one of the variants of the type `Stack` _ => false //~^ ERROR unreachable pattern } diff --git a/tests/ui/lint/issue-30302.stderr b/tests/ui/lint/issue-30302.stderr index 849ff1ebd9236..baf6c0d7a59d8 100644 --- a/tests/ui/lint/issue-30302.stderr +++ b/tests/ui/lint/issue-30302.stderr @@ -1,10 +1,10 @@ -warning[E0170]: pattern binding `Nil` is named the same as one of the variants of the type `Stack` +error[E0170]: pattern binding `Nil` is named the same as one of the variants of the type `Stack` --> $DIR/issue-30302.rs:13:9 | LL | Nil => true, | ^^^ help: to match on the variant, qualify the path: `Stack::Nil` | - = note: `#[warn(bindings_with_variant_name)]` on by default + = note: `#[deny(bindings_with_variant_name)]` on by default error: unreachable pattern --> $DIR/issue-30302.rs:15:9 @@ -21,6 +21,6 @@ note: the lint level is defined here LL | #![deny(unreachable_patterns)] | ^^^^^^^^^^^^^^^^^^^^ -error: aborting due to previous error; 1 warning emitted +error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0170`. diff --git a/tests/ui/lint/lint-uppercase-variables.rs b/tests/ui/lint/lint-uppercase-variables.rs index d4e88aa264361..59dba536f24b6 100644 --- a/tests/ui/lint/lint-uppercase-variables.rs +++ b/tests/ui/lint/lint-uppercase-variables.rs @@ -21,18 +21,18 @@ fn main() { match foo::Foo::Foo { Foo => {} //~^ ERROR variable `Foo` should have a snake case name - //~^^ WARN `Foo` is named the same as one of the variants of the type `foo::Foo` + //~^^ ERROR `Foo` is named the same as one of the variants of the type `foo::Foo` //~^^^ WARN unused variable: `Foo` } let Foo = foo::Foo::Foo; //~^ ERROR variable `Foo` should have a snake case name - //~^^ WARN `Foo` is named the same as one of the variants of the type `foo::Foo` + //~^^ ERROR `Foo` is named the same as one of the variants of the type `foo::Foo` //~^^^ WARN unused variable: `Foo` fn in_param(Foo: foo::Foo) {} //~^ ERROR variable `Foo` should have a snake case name - //~^^ WARN `Foo` is named the same as one of the variants of the type `foo::Foo` + //~^^ ERROR `Foo` is named the same as one of the variants of the type `foo::Foo` //~^^^ WARN unused variable: `Foo` test(1); diff --git a/tests/ui/lint/lint-uppercase-variables.stderr b/tests/ui/lint/lint-uppercase-variables.stderr index d476d856e24c5..42ec9364bc6e6 100644 --- a/tests/ui/lint/lint-uppercase-variables.stderr +++ b/tests/ui/lint/lint-uppercase-variables.stderr @@ -1,18 +1,18 @@ -warning[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` +error[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` --> $DIR/lint-uppercase-variables.rs:22:9 | LL | Foo => {} | ^^^ help: to match on the variant, qualify the path: `foo::Foo::Foo` | - = note: `#[warn(bindings_with_variant_name)]` on by default + = note: `#[deny(bindings_with_variant_name)]` on by default -warning[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` +error[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` --> $DIR/lint-uppercase-variables.rs:28:9 | LL | let Foo = foo::Foo::Foo; | ^^^ help: to match on the variant, qualify the path: `foo::Foo::Foo` -warning[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` +error[E0170]: pattern binding `Foo` is named the same as one of the variants of the type `foo::Foo` --> $DIR/lint-uppercase-variables.rs:33:17 | LL | fn in_param(Foo: foo::Foo) {} @@ -85,6 +85,6 @@ error: variable `Foo` should have a snake case name LL | fn in_param(Foo: foo::Foo) {} | ^^^ help: convert the identifier to snake case (notice the capitalization): `foo` -error: aborting due to 6 previous errors; 6 warnings emitted +error: aborting due to 9 previous errors; 3 warnings emitted For more information about this error, try `rustc --explain E0170`. diff --git a/tests/ui/pattern/issue-14221.rs b/tests/ui/pattern/issue-14221.rs index 282c411136922..13427d2c9b208 100644 --- a/tests/ui/pattern/issue-14221.rs +++ b/tests/ui/pattern/issue-14221.rs @@ -11,9 +11,9 @@ pub mod b { pub fn key(e: ::E) -> &'static str { match e { A => "A", -//~^ WARN pattern binding `A` is named the same as one of the variants of the type `E` +//~^ ERROR pattern binding `A` is named the same as one of the variants of the type `E` B => "B", //~ ERROR: unreachable pattern -//~^ WARN pattern binding `B` is named the same as one of the variants of the type `E` +//~^ ERROR pattern binding `B` is named the same as one of the variants of the type `E` } } } diff --git a/tests/ui/pattern/issue-14221.stderr b/tests/ui/pattern/issue-14221.stderr index fc8ae1ed7b5b0..7ea51b5f804c0 100644 --- a/tests/ui/pattern/issue-14221.stderr +++ b/tests/ui/pattern/issue-14221.stderr @@ -1,12 +1,12 @@ -warning[E0170]: pattern binding `A` is named the same as one of the variants of the type `E` +error[E0170]: pattern binding `A` is named the same as one of the variants of the type `E` --> $DIR/issue-14221.rs:13:13 | LL | A => "A", | ^ help: to match on the variant, qualify the path: `E::A` | - = note: `#[warn(bindings_with_variant_name)]` on by default + = note: `#[deny(bindings_with_variant_name)]` on by default -warning[E0170]: pattern binding `B` is named the same as one of the variants of the type `E` +error[E0170]: pattern binding `B` is named the same as one of the variants of the type `E` --> $DIR/issue-14221.rs:15:13 | LL | B => "B", @@ -27,6 +27,6 @@ note: the lint level is defined here LL | #![deny(unreachable_patterns)] | ^^^^^^^^^^^^^^^^^^^^ -error: aborting due to previous error; 2 warnings emitted +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0170`. diff --git a/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.rs b/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.rs index 6fd5768a5a26d..05d097eaf14e4 100644 --- a/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.rs +++ b/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.rs @@ -1,7 +1,5 @@ // Test for issue #67776: binding named the same as enum variant -// should report a warning even when matching against a reference type - -// check-pass +// should report an error even when matching against a reference type #![allow(unused_variables)] #![allow(non_snake_case)] @@ -15,27 +13,27 @@ enum Foo { fn fn1(e: Foo) { match e { Bar => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` Baz => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` } } fn fn2(e: &Foo) { match e { Bar => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` Baz => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` } } fn fn3(e: &mut &&mut Foo) { match e { Bar => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` Baz => {}, - //~^ WARNING named the same as one of the variants of the type `Foo` + //~^ ERROR named the same as one of the variants of the type `Foo` } } diff --git a/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.stderr b/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.stderr index 6f3613b63c9aa..da580c7accb97 100644 --- a/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.stderr +++ b/tests/ui/pattern/issue-67776-match-same-name-enum-variant-refs.stderr @@ -1,41 +1,41 @@ -warning[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:17:9 +error[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:15:9 | LL | Bar => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Bar` | - = note: `#[warn(bindings_with_variant_name)]` on by default + = note: `#[deny(bindings_with_variant_name)]` on by default -warning[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:19:9 +error[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:17:9 | LL | Baz => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Baz` -warning[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:26:9 +error[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:24:9 | LL | Bar => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Bar` -warning[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:28:9 +error[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:26:9 | LL | Baz => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Baz` -warning[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:35:9 +error[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:33:9 | LL | Bar => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Bar` -warning[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:37:9 +error[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` + --> $DIR/issue-67776-match-same-name-enum-variant-refs.rs:35:9 | LL | Baz => {}, | ^^^ help: to match on the variant, qualify the path: `Foo::Baz` -warning: 6 warnings emitted +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0170`. diff --git a/tests/ui/suggestions/issue-88730.rs b/tests/ui/suggestions/issue-88730.rs index e63210a3e987e..d161ed284f6d9 100644 --- a/tests/ui/suggestions/issue-88730.rs +++ b/tests/ui/suggestions/issue-88730.rs @@ -1,9 +1,8 @@ #![allow(unused, nonstandard_style)] -#![deny(bindings_with_variant_name)] // If an enum has two different variants, // then it cannot be matched upon in a function argument. -// It still gets a warning, but no suggestions. +// It still gets an error, but no suggestions. enum Foo { C, D, diff --git a/tests/ui/suggestions/issue-88730.stderr b/tests/ui/suggestions/issue-88730.stderr index eb22b0ea5c83d..0bd1b7ba4bacf 100644 --- a/tests/ui/suggestions/issue-88730.stderr +++ b/tests/ui/suggestions/issue-88730.stderr @@ -1,17 +1,13 @@ error[E0170]: pattern binding `C` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-88730.rs:12:8 + --> $DIR/issue-88730.rs:11:8 | LL | fn foo(C: Foo) {} | ^ | -note: the lint level is defined here - --> $DIR/issue-88730.rs:2:9 - | -LL | #![deny(bindings_with_variant_name)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `#[deny(bindings_with_variant_name)]` on by default error[E0170]: pattern binding `C` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-88730.rs:15:9 + --> $DIR/issue-88730.rs:14:9 | LL | let C = Foo::D; | ^ From 1cbce729c74c3c3b50dabdb776e41cc5c23f5601 Mon Sep 17 00:00:00 2001 From: --global <39689890+timrobertsdev@users.noreply.github.com> Date: Tue, 8 Nov 2022 11:01:03 -0500 Subject: [PATCH 12/20] Add `compile_fail` to doctest for `bindings_with_variant_name` --- compiler/rustc_lint_defs/src/builtin.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 0748dd94c457c..1ac97849ec611 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -708,7 +708,7 @@ declare_lint! { /// /// ### Example /// - /// ```rust + /// ```rust,compile_fail /// pub enum Enum { /// Foo, /// Bar, From 1adb4d6e5f5d242459655a6ef071669422ce0019 Mon Sep 17 00:00:00 2001 From: Ikko Eltociear Ashimine Date: Fri, 20 Jan 2023 17:56:29 +0900 Subject: [PATCH 13/20] Fix typo in opaque_types.rs paramters -> parameters --- compiler/rustc_infer/src/infer/opaque_types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_infer/src/infer/opaque_types.rs b/compiler/rustc_infer/src/infer/opaque_types.rs index e22ba9785e1fd..c54c66eab2799 100644 --- a/compiler/rustc_infer/src/infer/opaque_types.rs +++ b/compiler/rustc_infer/src/infer/opaque_types.rs @@ -479,7 +479,7 @@ where } ty::Alias(ty::Opaque, ty::AliasTy { def_id, ref substs, .. }) => { - // Skip lifetime paramters that are not captures. + // Skip lifetime parameters that are not captures. let variances = self.tcx.variances_of(*def_id); for (v, s) in std::iter::zip(variances, substs.iter()) { @@ -492,7 +492,7 @@ where ty::Alias(ty::Projection, proj) if self.tcx.def_kind(proj.def_id) == DefKind::ImplTraitPlaceholder => { - // Skip lifetime paramters that are not captures. + // Skip lifetime parameters that are not captures. let variances = self.tcx.variances_of(proj.def_id); for (v, s) in std::iter::zip(variances, proj.substs.iter()) { From 540ca2ff2acf5539868b1f1c156f6c895fdc9ffc Mon Sep 17 00:00:00 2001 From: DebugSteven Date: Wed, 4 Jan 2023 21:57:20 -0700 Subject: [PATCH 14/20] run cargo install to check for x installation and version --- Cargo.lock | 1 + src/tools/tidy/Cargo.toml | 1 + src/tools/tidy/src/lib.rs | 1 + src/tools/tidy/src/main.rs | 2 + src/tools/tidy/src/x_version.rs | 68 +++++++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+) create mode 100644 src/tools/tidy/src/x_version.rs diff --git a/Cargo.lock b/Cargo.lock index 5511d30177559..f6a5fb14d53b3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5607,6 +5607,7 @@ dependencies = [ "lazy_static", "miropt-test-tools", "regex", + "semver", "termcolor", "walkdir", ] diff --git a/src/tools/tidy/Cargo.toml b/src/tools/tidy/Cargo.toml index 19812fc6f55b6..cdf1dd366046c 100644 --- a/src/tools/tidy/Cargo.toml +++ b/src/tools/tidy/Cargo.toml @@ -12,6 +12,7 @@ miropt-test-tools = { path = "../miropt-test-tools" } lazy_static = "1" walkdir = "2" ignore = "0.4.18" +semver = "1.0" termcolor = "1.1.3" [[bin]] diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 97e56720b9852..35000320d1abf 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -70,3 +70,4 @@ pub mod ui_tests; pub mod unit_tests; pub mod unstable_book; pub mod walk; +pub mod x_version; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 0b9a1b37e947e..4c446b72a4495 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -114,6 +114,8 @@ fn main() { check!(alphabetical, &compiler_path); check!(alphabetical, &library_path); + check!(x_version, &root_path, &cargo); + let collected = { drain_handles(&mut handles); diff --git a/src/tools/tidy/src/x_version.rs b/src/tools/tidy/src/x_version.rs new file mode 100644 index 0000000000000..c470d502a6548 --- /dev/null +++ b/src/tools/tidy/src/x_version.rs @@ -0,0 +1,68 @@ +use semver::Version; +use std::path::Path; +use std::process::{Command, Stdio}; + +pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { + let cargo_list = Command::new(cargo).args(["install", "--list"]).stdout(Stdio::piped()).spawn(); + + let child = match cargo_list { + Ok(child) => child, + Err(e) => return tidy_error!(bad, "failed to run `cargo`: {}", e), + }; + + let cargo_list = child.wait_with_output().unwrap(); + + if cargo_list.status.success() { + let exe_list = String::from_utf8_lossy(&cargo_list.stdout); + let exe_list = exe_list.lines(); + + let mut installed: Option = None; + + for line in exe_list { + let mut iter = line.split_whitespace(); + if iter.next() == Some("x") { + if let Some(version) = iter.next() { + // Check this is the rust-lang/rust x tool installation since it should be + // installed at a path containing `src/tools/x`. + if let Some(path) = iter.next() { + if path.contains(&"src/tools/x") { + let version = version.strip_prefix("v").unwrap(); + installed = Some(Version::parse(version).unwrap()); + break; + } + }; + } + } else { + continue; + } + } + // Unwrap the some if x is installed, otherwise return because it's fine if x isn't installed. + let installed = if let Some(i) = installed { i } else { return }; + + if let Some(expected) = get_x_wrapper_version(root, cargo) { + if installed < expected { + return println!( + "Current version of x is {installed}, but the latest version is {expected}\nConsider updating to the newer version of x by running `cargo install --path src/tools/x`" + ); + } + } else { + return tidy_error!( + bad, + "Unable to parse the latest version of `x` at `src/tools/x/Cargo.toml`" + ); + } + } else { + return tidy_error!(bad, "failed to check version of `x`: {}", cargo_list.status); + } +} + +// Parse latest version out of `x` Cargo.toml +fn get_x_wrapper_version(root: &Path, cargo: &Path) -> Option { + let mut cmd = cargo_metadata::MetadataCommand::new(); + cmd.cargo_path(cargo) + .manifest_path(root.join("src/tools/x/Cargo.toml")) + .no_deps() + .features(cargo_metadata::CargoOpt::AllFeatures); + let mut metadata = t!(cmd.exec()); + metadata.packages.pop().map(|x| x.version) +} From a641b929ad35e262ed6d0e0e275b291da26a588a Mon Sep 17 00:00:00 2001 From: DebugSteven Date: Thu, 19 Jan 2023 13:56:15 -0700 Subject: [PATCH 15/20] remove leading comma when there are no args in check macro expansion --- src/tools/tidy/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 0b9a1b37e947e..17cba0f6ccd4e 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -60,7 +60,7 @@ fn main() { let handle = s.spawn(|| { let mut flag = false; - $p::check($($args),* , &mut flag); + $p::check($($args, )* &mut flag); if (flag) { bad.store(true, Ordering::Relaxed); } From 112d85c1ec8d5294ece4850bec306df4a2ed6dab Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Fri, 20 Jan 2023 14:28:16 -0700 Subject: [PATCH 16/20] rustdoc: use CSS inline layout for radio line instead of flexbox This uses less code to lay them out the same way. --- src/librustdoc/html/static/css/settings.css | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/librustdoc/html/static/css/settings.css b/src/librustdoc/html/static/css/settings.css index 7211ffb779568..4e9803fe2366d 100644 --- a/src/librustdoc/html/static/css/settings.css +++ b/src/librustdoc/html/static/css/settings.css @@ -3,11 +3,6 @@ position: relative; } -.setting-line .choices { - display: flex; - flex-wrap: wrap; -} - .setting-line .radio-line input, .setting-line .settings-toggle input { margin-right: 0.3em; @@ -38,7 +33,7 @@ margin-bottom: 0.1em; min-width: 3.8em; padding: 0.3em; - display: flex; + display: inline-flex; align-items: center; cursor: pointer; } From e9d8d238ef76c7991b316bbfbd9f857d84bd39cf Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 12 Nov 2022 21:46:05 -0700 Subject: [PATCH 17/20] diagnostics: suggest changing `s@self::{macro}@::macro` for exported Fixes #99695 --- compiler/rustc_resolve/src/diagnostics.rs | 30 ++++++++++++++++++++++- tests/ui/imports/issue-99695.fixed | 17 +++++++++++++ tests/ui/imports/issue-99695.rs | 16 ++++++++++++ tests/ui/imports/issue-99695.stderr | 16 ++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/ui/imports/issue-99695.fixed create mode 100644 tests/ui/imports/issue-99695.rs create mode 100644 tests/ui/imports/issue-99695.stderr diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index f24e405018b74..2a5cc288380f4 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2125,9 +2125,31 @@ impl<'a, 'b> ImportResolver<'a, 'b> { let source_map = self.r.session.source_map(); + // Make sure this is actually crate-relative. + let use_and_crate = import.use_span.with_hi(after_crate_name.lo()); + let is_definitely_crate = + source_map.span_to_snippet(use_and_crate).map_or(false, |s| { + let mut s = s.trim(); + debug!("check_for_module_export_macro: s={s:?}",); + s = s + .split_whitespace() + .rev() + .next() + .expect("split_whitespace always yields at least once"); + debug!("check_for_module_export_macro: s={s:?}",); + if s.ends_with("::") { + s = &s[..s.len() - 2]; + } else { + return false; + } + s = s.trim(); + debug!("check_for_module_export_macro: s={s:?}",); + s != "self" && s != "super" + }); + // Add the import to the start, with a `{` if required. let start_point = source_map.start_point(after_crate_name); - if let Ok(start_snippet) = source_map.span_to_snippet(start_point) { + if is_definitely_crate && let Ok(start_snippet) = source_map.span_to_snippet(start_point) { corrections.push(( start_point, if has_nested { @@ -2139,6 +2161,12 @@ impl<'a, 'b> ImportResolver<'a, 'b> { format!("{{{}, {}", import_snippet, start_snippet) }, )); + } else { + // If the root import is module-relative, add the import separately + corrections.push(( + source_map.start_point(import.use_span).shrink_to_lo(), + format!("use {module_name}::{import_snippet};\n"), + )); } // Add a `};` to the end if nested, matching the `{` added at the start. diff --git a/tests/ui/imports/issue-99695.fixed b/tests/ui/imports/issue-99695.fixed new file mode 100644 index 0000000000000..6bf228b23aad2 --- /dev/null +++ b/tests/ui/imports/issue-99695.fixed @@ -0,0 +1,17 @@ +// run-rustfix +#![allow(unused, nonstandard_style)] +mod m { + #[macro_export] + macro_rules! nu { + {} => {}; + } + + pub struct other_item; + + use ::nu; +pub use self::{other_item as _}; + //~^ ERROR unresolved import `self::nu` [E0432] + //~| HELP a macro with this name exists at the root of the crate +} + +fn main() {} diff --git a/tests/ui/imports/issue-99695.rs b/tests/ui/imports/issue-99695.rs new file mode 100644 index 0000000000000..f7199f1497ab0 --- /dev/null +++ b/tests/ui/imports/issue-99695.rs @@ -0,0 +1,16 @@ +// run-rustfix +#![allow(unused, nonstandard_style)] +mod m { + #[macro_export] + macro_rules! nu { + {} => {}; + } + + pub struct other_item; + + pub use self::{nu, other_item as _}; + //~^ ERROR unresolved import `self::nu` [E0432] + //~| HELP a macro with this name exists at the root of the crate +} + +fn main() {} diff --git a/tests/ui/imports/issue-99695.stderr b/tests/ui/imports/issue-99695.stderr new file mode 100644 index 0000000000000..0ef762e1c8230 --- /dev/null +++ b/tests/ui/imports/issue-99695.stderr @@ -0,0 +1,16 @@ +error[E0432]: unresolved import `self::nu` + --> $DIR/issue-99695.rs:11:20 + | +LL | pub use self::{nu, other_item as _}; + | ^^ no `nu` in `m` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL ~ use ::nu; +LL ~ pub use self::{other_item as _}; + | + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0432`. From e237690a28f0d38ab478616fe4b247fb6eb8013f Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 12 Nov 2022 22:08:07 -0700 Subject: [PATCH 18/20] diagnostics: add `};` only if `{` was added too --- compiler/rustc_resolve/src/diagnostics.rs | 10 +++++----- tests/ui/imports/issue-99695-b.fixed | 20 ++++++++++++++++++++ tests/ui/imports/issue-99695-b.rs | 19 +++++++++++++++++++ tests/ui/imports/issue-99695-b.stderr | 16 ++++++++++++++++ 4 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 tests/ui/imports/issue-99695-b.fixed create mode 100644 tests/ui/imports/issue-99695-b.rs create mode 100644 tests/ui/imports/issue-99695-b.stderr diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 2a5cc288380f4..af4d7a8eafff3 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2161,6 +2161,11 @@ impl<'a, 'b> ImportResolver<'a, 'b> { format!("{{{}, {}", import_snippet, start_snippet) }, )); + + // Add a `};` to the end if nested, matching the `{` added at the start. + if !has_nested { + corrections.push((source_map.end_point(after_crate_name), "};".to_string())); + } } else { // If the root import is module-relative, add the import separately corrections.push(( @@ -2168,11 +2173,6 @@ impl<'a, 'b> ImportResolver<'a, 'b> { format!("use {module_name}::{import_snippet};\n"), )); } - - // Add a `};` to the end if nested, matching the `{` added at the start. - if !has_nested { - corrections.push((source_map.end_point(after_crate_name), "};".to_string())); - } } let suggestion = Some(( diff --git a/tests/ui/imports/issue-99695-b.fixed b/tests/ui/imports/issue-99695-b.fixed new file mode 100644 index 0000000000000..0e60c73b67a44 --- /dev/null +++ b/tests/ui/imports/issue-99695-b.fixed @@ -0,0 +1,20 @@ +// run-rustfix +#![allow(unused, nonstandard_style)] +mod m { + + mod p { + #[macro_export] + macro_rules! nu { + {} => {}; + } + + pub struct other_item; + } + + use ::nu; +pub use self::p::{other_item as _}; + //~^ ERROR unresolved import `self::p::nu` [E0432] + //~| HELP a macro with this name exists at the root of the crate +} + +fn main() {} diff --git a/tests/ui/imports/issue-99695-b.rs b/tests/ui/imports/issue-99695-b.rs new file mode 100644 index 0000000000000..031443a1f5df8 --- /dev/null +++ b/tests/ui/imports/issue-99695-b.rs @@ -0,0 +1,19 @@ +// run-rustfix +#![allow(unused, nonstandard_style)] +mod m { + + mod p { + #[macro_export] + macro_rules! nu { + {} => {}; + } + + pub struct other_item; + } + + pub use self::p::{nu, other_item as _}; + //~^ ERROR unresolved import `self::p::nu` [E0432] + //~| HELP a macro with this name exists at the root of the crate +} + +fn main() {} diff --git a/tests/ui/imports/issue-99695-b.stderr b/tests/ui/imports/issue-99695-b.stderr new file mode 100644 index 0000000000000..b6f5c726a5ca9 --- /dev/null +++ b/tests/ui/imports/issue-99695-b.stderr @@ -0,0 +1,16 @@ +error[E0432]: unresolved import `self::p::nu` + --> $DIR/issue-99695-b.rs:14:23 + | +LL | pub use self::p::{nu, other_item as _}; + | ^^ no `nu` in `m::p` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL ~ use ::nu; +LL ~ pub use self::p::{other_item as _}; + | + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0432`. From dca160a06a85cfe6d961a586543112e772e55b13 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Tue, 6 Dec 2022 08:42:30 -0700 Subject: [PATCH 19/20] diagnostics: use `module_path` to check crate import instead of strings --- compiler/rustc_resolve/src/diagnostics.rs | 24 ++++------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index af4d7a8eafff3..0b60952ae3c08 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2126,26 +2126,10 @@ impl<'a, 'b> ImportResolver<'a, 'b> { let source_map = self.r.session.source_map(); // Make sure this is actually crate-relative. - let use_and_crate = import.use_span.with_hi(after_crate_name.lo()); - let is_definitely_crate = - source_map.span_to_snippet(use_and_crate).map_or(false, |s| { - let mut s = s.trim(); - debug!("check_for_module_export_macro: s={s:?}",); - s = s - .split_whitespace() - .rev() - .next() - .expect("split_whitespace always yields at least once"); - debug!("check_for_module_export_macro: s={s:?}",); - if s.ends_with("::") { - s = &s[..s.len() - 2]; - } else { - return false; - } - s = s.trim(); - debug!("check_for_module_export_macro: s={s:?}",); - s != "self" && s != "super" - }); + let is_definitely_crate = import + .module_path + .first() + .map_or(false, |f| f.ident.name != kw::SelfLower && f.ident.name != kw::Super); // Add the import to the start, with a `{` if required. let start_point = source_map.start_point(after_crate_name); From c07a722847497233e6590d62a3d63946409385c3 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Tue, 6 Dec 2022 08:44:48 -0700 Subject: [PATCH 20/20] diagnostics: remvoe unnecessary use of `source_map.start_point` --- compiler/rustc_resolve/src/diagnostics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 0b60952ae3c08..8d104aa5cc592 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -2153,7 +2153,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { } else { // If the root import is module-relative, add the import separately corrections.push(( - source_map.start_point(import.use_span).shrink_to_lo(), + import.use_span.shrink_to_lo(), format!("use {module_name}::{import_snippet};\n"), )); }