From 3f73491589d99ad60d785add0309637423fe7368 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 12:54:07 -0800 Subject: [PATCH 01/11] Only count mutations with projections as borrows Because bindings also count as a mutation, the previous behavior counted all variables as borrowed, completely negating the effect of drop tracking. --- .../drop_ranges/record_consumed_borrow.rs | 18 +++++++++++++----- compiler/rustc_typeck/src/expr_use_visitor.rs | 6 +++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 03d3b23bb23d5..f2907bb2634ae 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -107,11 +107,19 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { assignee_place: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: HirId, ) { - debug!("mutate {:?}; diag_expr_id={:?}", assignee_place, diag_expr_id); - // Count mutations as a borrow. - self.places - .borrowed - .insert(TrackedValue::from_place_with_projections_allowed(assignee_place)); + debug!("mutate {assignee_place:?}; diag_expr_id={diag_expr_id:?}"); + // Count mutations as a borrow when done through a projection. + // + // The goal here is to catch cases such as `x.y = 42`, since MIR will count this + // as a borrow of `x`, and we need to match that behavior. + // + // FIXME(eholk): this is probably still more conservative than we need to be. For example, + // we may need to count `*x = 42` as a borrow of `x`, since it overwrites all of `x`. + if !assignee_place.place.projections.is_empty() { + self.places + .borrowed + .insert(TrackedValue::from_place_with_projections_allowed(assignee_place)); + } } fn fake_read( diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index db1c80ae433b2..9f45027094183 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -49,7 +49,11 @@ pub trait Delegate<'tcx> { /// The path at `assignee_place` is being assigned to. /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). - fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); + fn mutate( + &mut self, + assignee_place: &PlaceWithHirId<'tcx>, + diag_expr_id: hir::HirId + ); /// The `place` should be a fake read because of specified `cause`. fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId); From 09aa09f1f75a3fc1b71c65201a7c228c9f4e4225 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 12:55:50 -0800 Subject: [PATCH 02/11] Enable drop-tracking tests behind -Zdrop-tracking These were still disabled from the soft revert of drop tracking, which meant we were not catching regressions that were introduced while trying to fix drop tracking. --- src/test/ui/async-await/async-fn-nonsend.rs | 6 +---- .../ui/async-await/unresolved_type_param.rs | 5 +--- .../async-await/unresolved_type_param.stderr | 12 +++++----- src/test/ui/generator/drop-control-flow.rs | 4 ---- src/test/ui/generator/issue-57478.rs | 5 +--- src/test/ui/generator/partial-drop.rs | 4 +--- src/test/ui/generator/partial-drop.stderr | 24 +++++++++---------- 7 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/test/ui/async-await/async-fn-nonsend.rs b/src/test/ui/async-await/async-fn-nonsend.rs index a1a05c0acba47..d7f8d7ac546c0 100644 --- a/src/test/ui/async-await/async-fn-nonsend.rs +++ b/src/test/ui/async-await/async-fn-nonsend.rs @@ -1,9 +1,5 @@ // edition:2018 -// compile-flags: --crate-type lib - -// FIXME(eholk): temporarily disabled while drop range tracking is disabled -// (see generator_interior.rs:27) -// ignore-test +// compile-flags: --crate-type lib -Zdrop-tracking use std::{cell::RefCell, fmt::Debug, rc::Rc}; diff --git a/src/test/ui/async-await/unresolved_type_param.rs b/src/test/ui/async-await/unresolved_type_param.rs index 187356ca14021..6d6d806149104 100644 --- a/src/test/ui/async-await/unresolved_type_param.rs +++ b/src/test/ui/async-await/unresolved_type_param.rs @@ -2,10 +2,7 @@ // Error message should pinpoint the type parameter T as needing to be bound // (rather than give a general error message) // edition:2018 - -// FIXME(eholk): temporarily disabled while drop range tracking is disabled -// (see generator_interior.rs:27) -// ignore-test +// compile-flags: -Zdrop-tracking async fn bar() -> () {} diff --git a/src/test/ui/async-await/unresolved_type_param.stderr b/src/test/ui/async-await/unresolved_type_param.stderr index d19a3226ef9a4..7236c681f341c 100644 --- a/src/test/ui/async-await/unresolved_type_param.stderr +++ b/src/test/ui/async-await/unresolved_type_param.stderr @@ -1,35 +1,35 @@ error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 + --> $DIR/unresolved_type_param.rs:10:5 | LL | bar().await; | ^^^ cannot infer type for type parameter `T` declared on the function `bar` | note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 + --> $DIR/unresolved_type_param.rs:10:10 | LL | bar().await; | ^^^^^^ error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 + --> $DIR/unresolved_type_param.rs:10:5 | LL | bar().await; | ^^^ cannot infer type for type parameter `T` declared on the function `bar` | note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 + --> $DIR/unresolved_type_param.rs:10:10 | LL | bar().await; | ^^^^^^ error[E0698]: type inside `async fn` body must be known in this context - --> $DIR/unresolved_type_param.rs:9:5 + --> $DIR/unresolved_type_param.rs:10:5 | LL | bar().await; | ^^^ cannot infer type for type parameter `T` declared on the function `bar` | note: the type is part of the `async fn` body because of this `await` - --> $DIR/unresolved_type_param.rs:9:10 + --> $DIR/unresolved_type_param.rs:10:10 | LL | bar().await; | ^^^^^^ diff --git a/src/test/ui/generator/drop-control-flow.rs b/src/test/ui/generator/drop-control-flow.rs index 914a7d71dc421..d383680002f4b 100644 --- a/src/test/ui/generator/drop-control-flow.rs +++ b/src/test/ui/generator/drop-control-flow.rs @@ -1,10 +1,6 @@ // build-pass // compile-flags: -Zdrop-tracking -// FIXME(eholk): temporarily disabled while drop range tracking is disabled -// (see generator_interior.rs:27) -// ignore-test - // A test to ensure generators capture values that were conditionally dropped, // and also that values that are dropped along all paths to a yield do not get // included in the generator type. diff --git a/src/test/ui/generator/issue-57478.rs b/src/test/ui/generator/issue-57478.rs index 5c23ecbae3273..91407ea1844f5 100644 --- a/src/test/ui/generator/issue-57478.rs +++ b/src/test/ui/generator/issue-57478.rs @@ -1,8 +1,5 @@ // check-pass - -// FIXME(eholk): temporarily disabled while drop range tracking is disabled -// (see generator_interior.rs:27) -// ignore-test +// compile-flags: -Zdrop-tracking #![feature(negative_impls, generators)] diff --git a/src/test/ui/generator/partial-drop.rs b/src/test/ui/generator/partial-drop.rs index e89e4b61bbff7..c872fb7f3e630 100644 --- a/src/test/ui/generator/partial-drop.rs +++ b/src/test/ui/generator/partial-drop.rs @@ -1,6 +1,4 @@ -// FIXME(eholk): temporarily disabled while drop range tracking is disabled -// (see generator_interior.rs:27) -// ignore-test +// compile-flags: -Zdrop-tracking #![feature(negative_impls, generators)] diff --git a/src/test/ui/generator/partial-drop.stderr b/src/test/ui/generator/partial-drop.stderr index 9a1b0734d8c86..16b34c917ece4 100644 --- a/src/test/ui/generator/partial-drop.stderr +++ b/src/test/ui/generator/partial-drop.stderr @@ -1,12 +1,12 @@ error: generator cannot be sent between threads safely - --> $DIR/partial-drop.rs:12:5 + --> $DIR/partial-drop.rs:14:5 | LL | assert_send(|| { | ^^^^^^^^^^^ generator is not `Send` | - = help: within `[generator@$DIR/partial-drop.rs:12:17: 18:6]`, the trait `Send` is not implemented for `Foo` + = help: within `[generator@$DIR/partial-drop.rs:14:17: 20:6]`, the trait `Send` is not implemented for `Foo` note: generator is not `Send` as this value is used across a yield - --> $DIR/partial-drop.rs:17:9 + --> $DIR/partial-drop.rs:19:9 | LL | let guard = Bar { foo: Foo, x: 42 }; | ----- has type `Bar` which is not `Send` @@ -16,20 +16,20 @@ LL | yield; LL | }); | - `guard` is later dropped here note: required by a bound in `assert_send` - --> $DIR/partial-drop.rs:40:19 + --> $DIR/partial-drop.rs:42:19 | LL | fn assert_send(_: T) {} | ^^^^ required by this bound in `assert_send` error: generator cannot be sent between threads safely - --> $DIR/partial-drop.rs:20:5 + --> $DIR/partial-drop.rs:22:5 | LL | assert_send(|| { | ^^^^^^^^^^^ generator is not `Send` | - = help: within `[generator@$DIR/partial-drop.rs:20:17: 28:6]`, the trait `Send` is not implemented for `Foo` + = help: within `[generator@$DIR/partial-drop.rs:22:17: 30:6]`, the trait `Send` is not implemented for `Foo` note: generator is not `Send` as this value is used across a yield - --> $DIR/partial-drop.rs:27:9 + --> $DIR/partial-drop.rs:29:9 | LL | let guard = Bar { foo: Foo, x: 42 }; | ----- has type `Bar` which is not `Send` @@ -39,20 +39,20 @@ LL | yield; LL | }); | - `guard` is later dropped here note: required by a bound in `assert_send` - --> $DIR/partial-drop.rs:40:19 + --> $DIR/partial-drop.rs:42:19 | LL | fn assert_send(_: T) {} | ^^^^ required by this bound in `assert_send` error: generator cannot be sent between threads safely - --> $DIR/partial-drop.rs:30:5 + --> $DIR/partial-drop.rs:32:5 | LL | assert_send(|| { | ^^^^^^^^^^^ generator is not `Send` | - = help: within `[generator@$DIR/partial-drop.rs:30:17: 37:6]`, the trait `Send` is not implemented for `Foo` + = help: within `[generator@$DIR/partial-drop.rs:32:17: 39:6]`, the trait `Send` is not implemented for `Foo` note: generator is not `Send` as this value is used across a yield - --> $DIR/partial-drop.rs:36:9 + --> $DIR/partial-drop.rs:38:9 | LL | let guard = Bar { foo: Foo, x: 42 }; | ----- has type `Bar` which is not `Send` @@ -62,7 +62,7 @@ LL | yield; LL | }); | - `guard` is later dropped here note: required by a bound in `assert_send` - --> $DIR/partial-drop.rs:40:19 + --> $DIR/partial-drop.rs:42:19 | LL | fn assert_send(_: T) {} | ^^^^ required by this bound in `assert_send` From 1c12c92286375dd91e283a6b0cbfc32f67d045b3 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 28 Feb 2022 13:03:13 -0800 Subject: [PATCH 03/11] Fix formatting --- compiler/rustc_typeck/src/expr_use_visitor.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index 9f45027094183..db1c80ae433b2 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -49,11 +49,7 @@ pub trait Delegate<'tcx> { /// The path at `assignee_place` is being assigned to. /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). - fn mutate( - &mut self, - assignee_place: &PlaceWithHirId<'tcx>, - diag_expr_id: hir::HirId - ); + fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); /// The `place` should be a fake read because of specified `cause`. fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId); From e42733307169708d78f4373c32ecfd4019c279ca Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Mon, 28 Feb 2022 10:34:06 +0100 Subject: [PATCH 04/11] remove_dir_all(): try recursing first instead of trying to unlink() This only affects the `slow` code path, if there is no `dirent.d_type` or if the type is `DT_UNKNOWN`. POSIX specifies that calling `unlink()` or `unlinkat(..., 0)` on a directory can succeed: > "The _path_ argument shall not name a directory unless the process has > appropriate privileges and the implementation supports using _unlink()_ on > directories." This however can cause orphaned directories requiring an fsck e.g. on Illumos UFS, so we have to avoid that in the common case. We now just try to recurse into it first and unlink() if we can't open it as a directory. --- library/std/src/sys/unix/fs.rs | 37 ++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index 8bd0b9b14afed..8fc0b337761d7 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -1683,8 +1683,21 @@ mod remove_dir_impl { fn remove_dir_all_recursive(parent_fd: Option, p: &Path) -> io::Result<()> { let pcstr = cstr(p)?; - // entry is expected to be a directory, open as such - let fd = openat_nofollow_dironly(parent_fd, &pcstr)?; + // try opening as directory + let fd = match openat_nofollow_dironly(parent_fd, &pcstr) { + Err(err) if err.raw_os_error() == Some(libc::ENOTDIR) => { + // not a directory - don't traverse further + return match parent_fd { + // unlink... + Some(parent_fd) => { + cvt(unsafe { unlinkat(parent_fd, pcstr.as_ptr(), 0) }).map(drop) + } + // ...unless this was supposed to be the deletion root directory + None => Err(err), + }; + } + result => result?, + }; // open the directory passing ownership of the fd let (dir, fd) = fdreaddir(fd)?; @@ -1697,19 +1710,13 @@ mod remove_dir_impl { Some(false) => { cvt(unsafe { unlinkat(fd, child.name_cstr().as_ptr(), 0) })?; } - None => match cvt(unsafe { unlinkat(fd, child.name_cstr().as_ptr(), 0) }) { - // type unknown - try to unlink - Err(err) - if err.raw_os_error() == Some(libc::EISDIR) - || err.raw_os_error() == Some(libc::EPERM) => - { - // if the file is a directory unlink fails with EISDIR on Linux and EPERM everyhwere else - remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?; - } - result => { - result?; - } - }, + None => { + // POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed + // if the process has the appropriate privileges. This however can causing orphaned + // directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing + // into it first instead of trying to unlink() it. + remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?; + } } } From 41b4423cdfa4fe3aaee719a103d70368c85f4af7 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Mon, 28 Feb 2022 12:53:12 +0100 Subject: [PATCH 05/11] Integrate macos x86-64 remove_dir_all() impl. Step 1: remove --- library/std/src/sys/unix/fs.rs | 118 --------------------------------- 1 file changed, 118 deletions(-) diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index 8fc0b337761d7..d83160db2f23c 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -1482,124 +1482,6 @@ mod remove_dir_impl { pub use crate::sys_common::fs::remove_dir_all; } -// Dynamically choose implementation Macos x86-64: modern for 10.10+, fallback for older versions -#[cfg(all(target_os = "macos", target_arch = "x86_64"))] -mod remove_dir_impl { - use super::{cstr, lstat, Dir, InnerReadDir, ReadDir}; - use crate::ffi::CStr; - use crate::io; - use crate::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd}; - use crate::os::unix::prelude::{OwnedFd, RawFd}; - use crate::path::{Path, PathBuf}; - use crate::sync::Arc; - use crate::sys::weak::weak; - use crate::sys::{cvt, cvt_r}; - use libc::{c_char, c_int, DIR}; - - pub fn openat_nofollow_dironly(parent_fd: Option, p: &CStr) -> io::Result { - weak!(fn openat(c_int, *const c_char, c_int) -> c_int); - let fd = cvt_r(|| unsafe { - openat.get().unwrap()( - parent_fd.unwrap_or(libc::AT_FDCWD), - p.as_ptr(), - libc::O_CLOEXEC | libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_DIRECTORY, - ) - })?; - Ok(unsafe { OwnedFd::from_raw_fd(fd) }) - } - - fn fdreaddir(dir_fd: OwnedFd) -> io::Result<(ReadDir, RawFd)> { - weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64"); - let ptr = unsafe { fdopendir.get().unwrap()(dir_fd.as_raw_fd()) }; - if ptr.is_null() { - return Err(io::Error::last_os_error()); - } - let dirp = Dir(ptr); - // file descriptor is automatically closed by libc::closedir() now, so give up ownership - let new_parent_fd = dir_fd.into_raw_fd(); - // a valid root is not needed because we do not call any functions involving the full path - // of the DirEntrys. - let dummy_root = PathBuf::new(); - Ok(( - ReadDir { - inner: Arc::new(InnerReadDir { dirp, root: dummy_root }), - end_of_stream: false, - }, - new_parent_fd, - )) - } - - fn remove_dir_all_recursive(parent_fd: Option, p: &Path) -> io::Result<()> { - weak!(fn unlinkat(c_int, *const c_char, c_int) -> c_int); - - let pcstr = cstr(p)?; - - // entry is expected to be a directory, open as such - let fd = openat_nofollow_dironly(parent_fd, &pcstr)?; - - // open the directory passing ownership of the fd - let (dir, fd) = fdreaddir(fd)?; - for child in dir { - let child = child?; - match child.entry.d_type { - libc::DT_DIR => { - remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?; - } - libc::DT_UNKNOWN => { - match cvt(unsafe { unlinkat.get().unwrap()(fd, child.name_cstr().as_ptr(), 0) }) - { - // type unknown - try to unlink - Err(err) if err.raw_os_error() == Some(libc::EPERM) => { - // if the file is a directory unlink fails with EPERM - remove_dir_all_recursive(Some(fd), Path::new(&child.file_name()))?; - } - result => { - result?; - } - } - } - _ => { - // not a directory -> unlink - cvt(unsafe { unlinkat.get().unwrap()(fd, child.name_cstr().as_ptr(), 0) })?; - } - } - } - - // unlink the directory after removing its contents - cvt(unsafe { - unlinkat.get().unwrap()( - parent_fd.unwrap_or(libc::AT_FDCWD), - pcstr.as_ptr(), - libc::AT_REMOVEDIR, - ) - })?; - Ok(()) - } - - fn remove_dir_all_modern(p: &Path) -> io::Result<()> { - // We cannot just call remove_dir_all_recursive() here because that would not delete a passed - // symlink. No need to worry about races, because remove_dir_all_recursive() does not recurse - // into symlinks. - let attr = lstat(p)?; - if attr.file_type().is_symlink() { - crate::fs::remove_file(p) - } else { - remove_dir_all_recursive(None, p) - } - } - - pub fn remove_dir_all(p: &Path) -> io::Result<()> { - weak!(fn openat(c_int, *const c_char, c_int) -> c_int); - if openat.get().is_some() { - // openat() is available with macOS 10.10+, just like unlinkat() and fdopendir() - remove_dir_all_modern(p) - } else { - // fall back to classic implementation - crate::sys_common::fs::remove_dir_all(p) - } - } -} - // Modern implementation using openat(), unlinkat() and fdopendir() #[cfg(not(any( all(target_os = "macos", target_arch = "x86_64"), From 735f60c34fc0b13d3aeb3873dcadff03dc141b07 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Mon, 28 Feb 2022 12:30:23 +0100 Subject: [PATCH 06/11] Integrate macos x86-64 remove_dir_all() impl. Step 2: readd --- library/std/src/sys/unix/fs.rs | 66 ++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/library/std/src/sys/unix/fs.rs b/library/std/src/sys/unix/fs.rs index d83160db2f23c..0851f512fd01b 100644 --- a/library/std/src/sys/unix/fs.rs +++ b/library/std/src/sys/unix/fs.rs @@ -1483,11 +1483,7 @@ mod remove_dir_impl { } // Modern implementation using openat(), unlinkat() and fdopendir() -#[cfg(not(any( - all(target_os = "macos", target_arch = "x86_64"), - target_os = "redox", - target_os = "espidf" -)))] +#[cfg(not(any(target_os = "redox", target_os = "espidf")))] mod remove_dir_impl { use super::{cstr, lstat, Dir, DirEntry, InnerReadDir, ReadDir}; use crate::ffi::CStr; @@ -1497,7 +1493,49 @@ mod remove_dir_impl { use crate::path::{Path, PathBuf}; use crate::sync::Arc; use crate::sys::{cvt, cvt_r}; + + #[cfg(not(all(target_os = "macos", target_arch = "x86_64"),))] use libc::{fdopendir, openat, unlinkat}; + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + use macos_weak::{fdopendir, openat, unlinkat}; + + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + mod macos_weak { + use crate::sys::weak::weak; + use libc::{c_char, c_int, DIR}; + + fn get_openat_fn() -> Option c_int> { + weak!(fn openat(c_int, *const c_char, c_int) -> c_int); + openat.get() + } + + pub fn has_openat() -> bool { + get_openat_fn().is_some() + } + + pub unsafe fn openat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { + get_openat_fn().map(|openat| openat(dirfd, pathname, flags)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + -1 + }) + } + + pub unsafe fn fdopendir(fd: c_int) -> *mut DIR { + weak!(fn fdopendir(c_int) -> *mut DIR, "fdopendir$INODE64"); + fdopendir.get().map(|fdopendir| fdopendir(fd)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + crate::ptr::null_mut() + }) + } + + pub unsafe fn unlinkat(dirfd: c_int, pathname: *const c_char, flags: c_int) -> c_int { + weak!(fn unlinkat(c_int, *const c_char, c_int) -> c_int); + unlinkat.get().map(|unlinkat| unlinkat(dirfd, pathname, flags)).unwrap_or_else(|| { + crate::sys::unix::os::set_errno(libc::ENOSYS); + -1 + }) + } + } pub fn openat_nofollow_dironly(parent_fd: Option, p: &CStr) -> io::Result { let fd = cvt_r(|| unsafe { @@ -1609,7 +1647,7 @@ mod remove_dir_impl { Ok(()) } - pub fn remove_dir_all(p: &Path) -> io::Result<()> { + fn remove_dir_all_modern(p: &Path) -> io::Result<()> { // We cannot just call remove_dir_all_recursive() here because that would not delete a passed // symlink. No need to worry about races, because remove_dir_all_recursive() does not recurse // into symlinks. @@ -1620,4 +1658,20 @@ mod remove_dir_impl { remove_dir_all_recursive(None, p) } } + + #[cfg(not(all(target_os = "macos", target_arch = "x86_64")))] + pub fn remove_dir_all(p: &Path) -> io::Result<()> { + remove_dir_all_modern(p) + } + + #[cfg(all(target_os = "macos", target_arch = "x86_64"))] + pub fn remove_dir_all(p: &Path) -> io::Result<()> { + if macos_weak::has_openat() { + // openat() is available with macOS 10.10+, just like unlinkat() and fdopendir() + remove_dir_all_modern(p) + } else { + // fall back to classic implementation + crate::sys_common::fs::remove_dir_all(p) + } + } } From add169d4140c7583b876619b2e19ba76f4aa76e4 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 4 Mar 2022 11:03:24 -0800 Subject: [PATCH 07/11] Distinguish binding assignments, use Ty::needs_drop This better captures the actual behavior, rather than using hacks around whether the assignment has any projections. --- .../drop_ranges/record_consumed_borrow.rs | 33 +++++++++++-------- compiler/rustc_typeck/src/expr_use_visitor.rs | 13 ++++++-- src/test/ui/async-await/drop-and-assign.rs | 19 +++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 src/test/ui/async-await/drop-and-assign.rs diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index f2907bb2634ae..40ee6d863b5a7 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -6,14 +6,14 @@ use crate::{ use hir::{def_id::DefId, Body, HirId, HirIdMap}; use rustc_data_structures::stable_set::FxHashSet; use rustc_hir as hir; -use rustc_middle::hir::map::Map; +use rustc_middle::ty::{ParamEnv, TyCtxt}; pub(super) fn find_consumed_and_borrowed<'a, 'tcx>( fcx: &'a FnCtxt<'a, 'tcx>, def_id: DefId, body: &'tcx Body<'tcx>, ) -> ConsumedAndBorrowedPlaces { - let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx.hir()); + let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx, fcx.param_env); expr_use_visitor.consume_body(fcx, def_id, body); expr_use_visitor.places } @@ -36,14 +36,16 @@ pub(super) struct ConsumedAndBorrowedPlaces { /// Interesting values are those that are either dropped or borrowed. For dropped values, we also /// record the parent expression, which is the point where the drop actually takes place. struct ExprUseDelegate<'tcx> { - hir: Map<'tcx>, + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, places: ConsumedAndBorrowedPlaces, } impl<'tcx> ExprUseDelegate<'tcx> { - fn new(hir: Map<'tcx>) -> Self { + fn new(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Self { Self { - hir, + tcx, + param_env, places: ConsumedAndBorrowedPlaces { consumed: <_>::default(), borrowed: <_>::default(), @@ -77,7 +79,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: HirId, ) { - let parent = match self.hir.find_parent_node(place_with_id.hir_id) { + let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) { Some(parent) => parent, None => place_with_id.hir_id, }; @@ -108,20 +110,23 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { diag_expr_id: HirId, ) { debug!("mutate {assignee_place:?}; diag_expr_id={diag_expr_id:?}"); - // Count mutations as a borrow when done through a projection. - // - // The goal here is to catch cases such as `x.y = 42`, since MIR will count this - // as a borrow of `x`, and we need to match that behavior. - // - // FIXME(eholk): this is probably still more conservative than we need to be. For example, - // we may need to count `*x = 42` as a borrow of `x`, since it overwrites all of `x`. - if !assignee_place.place.projections.is_empty() { + // If the type being assigned needs dropped, then the mutation counts as a borrow + // since it is essentially doing `Drop::drop(&mut x); x = new_value;`. + if assignee_place.place.base_ty.needs_drop(self.tcx, self.param_env) { self.places .borrowed .insert(TrackedValue::from_place_with_projections_allowed(assignee_place)); } } + fn bind( + &mut self, + binding_place: &expr_use_visitor::PlaceWithHirId<'tcx>, + diag_expr_id: HirId, + ) { + debug!("bind {binding_place:?}; diag_expr_id={diag_expr_id:?}"); + } + fn fake_read( &mut self, _place: expr_use_visitor::Place<'tcx>, diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index db1c80ae433b2..8c19bbd3214ee 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -51,6 +51,15 @@ pub trait Delegate<'tcx> { /// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); + /// The path at `binding_place` is a binding that is being initialized. + /// + /// This covers cases such as `let x = 42;` + fn bind(&mut self, binding_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId) { + // Bindings can normally be treated as a regular assignment, so by default we + // forward this to the mutate callback. + self.mutate(binding_place, diag_expr_id) + } + /// The `place` should be a fake read because of specified `cause`. fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId); } @@ -648,11 +657,9 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { let pat_ty = return_if_err!(mc.node_ty(pat.hir_id)); debug!("walk_pat: pat_ty={:?}", pat_ty); - // Each match binding is effectively an assignment to the - // binding being produced. let def = Res::Local(canonical_id); if let Ok(ref binding_place) = mc.cat_res(pat.hir_id, pat.span, pat_ty, def) { - delegate.mutate(binding_place, binding_place.hir_id); + delegate.bind(binding_place, binding_place.hir_id); } // It is also a borrow or copy/move of the value being matched. diff --git a/src/test/ui/async-await/drop-and-assign.rs b/src/test/ui/async-await/drop-and-assign.rs new file mode 100644 index 0000000000000..fa3f3303677da --- /dev/null +++ b/src/test/ui/async-await/drop-and-assign.rs @@ -0,0 +1,19 @@ +// edition:2021 +// compile-flags: -Zdrop-tracking +// build-pass + +struct A; +impl Drop for A { fn drop(&mut self) {} } + +pub async fn f() { + let mut a = A; + a = A; + drop(a); + async {}.await; +} + +fn assert_send(_: T) {} + +fn main() { + let _ = f(); +} From f0257b1b4c76a23392c91db1b81754b85a9202c2 Mon Sep 17 00:00:00 2001 From: pierwill Date: Fri, 4 Mar 2022 13:31:23 -0600 Subject: [PATCH 08/11] Edit docs on consistency of `PartialOrd` and `PartialEq` Use ordered list to make the information about implementations more readable. --- library/core/src/cmp.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index a3e7747991190..ddaeb9eca975c 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -885,19 +885,18 @@ impl PartialOrd for Ordering { /// The `lt`, `le`, `gt`, and `ge` methods of this trait can be called using /// the `<`, `<=`, `>`, and `>=` operators, respectively. /// -/// The methods of this trait must be consistent with each other and with those of `PartialEq` in -/// the following sense: -/// -/// - `a == b` if and only if `partial_cmp(a, b) == Some(Equal)`. -/// - `a < b` if and only if `partial_cmp(a, b) == Some(Less)` -/// (ensured by the default implementation). -/// - `a > b` if and only if `partial_cmp(a, b) == Some(Greater)` -/// (ensured by the default implementation). -/// - `a <= b` if and only if `a < b || a == b` -/// (ensured by the default implementation). -/// - `a >= b` if and only if `a > b || a == b` -/// (ensured by the default implementation). -/// - `a != b` if and only if `!(a == b)` (already part of `PartialEq`). +/// The methods of this trait must be consistent with each other and with those of [`PartialEq`]. +/// The following conditions must hold: +/// +/// 1. `a == b` if and only if `partial_cmp(a, b) == Some(Equal)`. +/// 2. `a < b` if and only if `partial_cmp(a, b) == Some(Less)` +/// 3. `a > b` if and only if `partial_cmp(a, b) == Some(Greater)` +/// 4. `a <= b` if and only if `a < b || a == b` +/// 5. `a >= b` if and only if `a > b || a == b` +/// 6. `a != b` if and only if `!(a == b)`. +/// +/// Conditions 2–5 above are ensured by the default implementation. +/// Condition 6 is already ensured by [`PartialEq`]. /// /// If [`Ord`] is also implemented for `Self` and `Rhs`, it must also be consistent with /// `partial_cmp` (see the documentation of that trait for the exact requirements). It's From 050d589991ea2a79b747e1b559ca476f37299441 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Fri, 4 Mar 2022 20:34:10 +0000 Subject: [PATCH 09/11] Downgrade `#[test]` on macro call to warning Follow up to #92959. Address #94508. --- compiler/rustc_builtin_macros/src/test.rs | 20 ++++++---- src/test/ui/test-attrs/test-on-not-fn.rs | 2 +- src/test/ui/test-attrs/test-on-not-fn.stderr | 40 +++++++------------- 3 files changed, 27 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/test.rs b/compiler/rustc_builtin_macros/src/test.rs index e658631d09038..0c2d20b8f2dc8 100644 --- a/compiler/rustc_builtin_macros/src/test.rs +++ b/compiler/rustc_builtin_macros/src/test.rs @@ -105,14 +105,18 @@ pub fn expand_test_or_bench( // Note: non-associated fn items are already handled by `expand_test_or_bench` if !matches!(item.kind, ast::ItemKind::Fn(_)) { - cx.sess - .parse_sess - .span_diagnostic - .struct_span_err( - attr_sp, - "the `#[test]` attribute may only be used on a non-associated function", - ) - .note("the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions") + let diag = &cx.sess.parse_sess.span_diagnostic; + let msg = "the `#[test]` attribute may only be used on a non-associated function"; + let mut err = match item.kind { + // These were a warning before #92959 and need to continue being that to avoid breaking + // stable user code (#94508). + ast::ItemKind::MacCall(_) => diag.struct_span_warn(attr_sp, msg), + // `.forget_guarantee()` needed to get these two arms to match types. Because of how + // locally close the `.emit()` call is I'm comfortable with it, but if it can be + // reworked in the future to not need it, it'd be nice. + _ => diag.struct_span_err(attr_sp, msg).forget_guarantee(), + }; + err.span_label(attr_sp, "the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions") .span_label(item.span, format!("expected a non-associated function, found {} {}", item.kind.article(), item.kind.descr())) .span_suggestion(attr_sp, "replace with conditional compilation to make the item only exist when tests are being run", String::from("#[cfg(test)]"), Applicability::MaybeIncorrect) .emit(); diff --git a/src/test/ui/test-attrs/test-on-not-fn.rs b/src/test/ui/test-attrs/test-on-not-fn.rs index b2f681c01d156..a460480afb157 100644 --- a/src/test/ui/test-attrs/test-on-not-fn.rs +++ b/src/test/ui/test-attrs/test-on-not-fn.rs @@ -58,7 +58,7 @@ macro_rules! foo { () => {}; } -#[test] //~ ERROR: the `#[test]` attribute may only be used on a non-associated function +#[test] //~ WARN: the `#[test]` attribute may only be used on a non-associated function foo!(); // make sure it doesn't erroneously trigger on a real test diff --git a/src/test/ui/test-attrs/test-on-not-fn.stderr b/src/test/ui/test-attrs/test-on-not-fn.stderr index dd693cf316dc7..23efd5bc0d9fa 100644 --- a/src/test/ui/test-attrs/test-on-not-fn.stderr +++ b/src/test/ui/test-attrs/test-on-not-fn.stderr @@ -2,11 +2,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:3:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | mod test {} | ----------- expected a non-associated function, found a module | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -16,7 +15,7 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:6:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | / mod loooooooooooooong_teeeeeeeeeest { LL | | /* LL | | this is a comment @@ -26,7 +25,6 @@ LL | | */ LL | | } | |_- expected a non-associated function, found a module | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -36,11 +34,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:20:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | extern "C" {} | ------------- expected a non-associated function, found an extern block | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -50,11 +47,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:23:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | trait Foo {} | ------------ expected a non-associated function, found a trait | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -64,11 +60,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:26:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | impl Foo for i32 {} | ------------------- expected a non-associated function, found an implementation | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -78,11 +73,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:29:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | const FOO: i32 = -1_i32; | ------------------------ expected a non-associated function, found a constant item | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -92,11 +86,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:32:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | static BAR: u64 = 10_000_u64; | ----------------------------- expected a non-associated function, found a static item | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -106,13 +99,12 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:35:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | / enum MyUnit { LL | | Unit, LL | | } | |_- expected a non-associated function, found an enum | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -122,11 +114,10 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:40:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | struct NewI32(i32); | ------------------- expected a non-associated function, found a struct | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -136,14 +127,13 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:43:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | / union Spooky { LL | | x: i32, LL | | y: u32, LL | | } | |_- expected a non-associated function, found a union | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] @@ -153,7 +143,7 @@ error: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:50:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | #[derive(Copy, Clone, Debug)] LL | / struct MoreAttrs { LL | | a: i32, @@ -161,25 +151,23 @@ LL | | b: u64, LL | | } | |_- expected a non-associated function, found a struct | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] | ~~~~~~~~~~~~ -error: the `#[test]` attribute may only be used on a non-associated function +warning: the `#[test]` attribute may only be used on a non-associated function --> $DIR/test-on-not-fn.rs:61:1 | LL | #[test] - | ^^^^^^^ + | ^^^^^^^ the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions LL | foo!(); | ------- expected a non-associated function, found an item macro invocation | - = note: the `#[test]` macro causes a a function to be run on a test and has no effect on non-functions help: replace with conditional compilation to make the item only exist when tests are being run | LL | #[cfg(test)] | ~~~~~~~~~~~~ -error: aborting due to 12 previous errors +error: aborting due to 11 previous errors; 1 warning emitted From 9e03d7ddbe46492ea9487dd52b3c4024a5823099 Mon Sep 17 00:00:00 2001 From: marmeladema Date: Fri, 4 Mar 2022 22:47:56 +0100 Subject: [PATCH 10/11] Add known-bug directive to issue #47511 test case --- src/test/ui/issues/issue-47511.rs | 5 +++-- src/test/ui/issues/issue-47511.stderr | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/ui/issues/issue-47511.rs b/src/test/ui/issues/issue-47511.rs index 0f33b52577b70..98c141b6c6a1d 100644 --- a/src/test/ui/issues/issue-47511.rs +++ b/src/test/ui/issues/issue-47511.rs @@ -1,14 +1,15 @@ +// check-fail +// known-bug + // Regression test for #47511: anonymous lifetimes can appear // unconstrained in a return type, but only if they appear just once // in the input, as the input to a projection. fn f(_: X) -> X { - //~^ ERROR return type references an anonymous lifetime unimplemented!() } fn g<'a>(_: X<'a>) -> X<'a> { - //~^ ERROR return type references lifetime `'a`, which is not constrained unimplemented!() } diff --git a/src/test/ui/issues/issue-47511.stderr b/src/test/ui/issues/issue-47511.stderr index 4473c0e68cfc8..5b84f7ed62c3a 100644 --- a/src/test/ui/issues/issue-47511.stderr +++ b/src/test/ui/issues/issue-47511.stderr @@ -1,5 +1,5 @@ error[E0581]: return type references an anonymous lifetime, which is not constrained by the fn input types - --> $DIR/issue-47511.rs:5:15 + --> $DIR/issue-47511.rs:8:15 | LL | fn f(_: X) -> X { | ^ @@ -7,7 +7,7 @@ LL | fn f(_: X) -> X { = note: lifetimes appearing in an associated type are not considered constrained error[E0581]: return type references lifetime `'a`, which is not constrained by the fn input types - --> $DIR/issue-47511.rs:10:23 + --> $DIR/issue-47511.rs:12:23 | LL | fn g<'a>(_: X<'a>) -> X<'a> { | ^^^^^ From a4cb2bfe34fa0cde40d880bc79e4a7c98f7dde4f Mon Sep 17 00:00:00 2001 From: Nebula Date: Fri, 4 Mar 2022 21:34:38 -0500 Subject: [PATCH 11/11] Fix typo in c-variadic --- src/doc/unstable-book/src/language-features/c-variadic.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doc/unstable-book/src/language-features/c-variadic.md b/src/doc/unstable-book/src/language-features/c-variadic.md index 9e7968d906fbf..001c1bfa9ec03 100644 --- a/src/doc/unstable-book/src/language-features/c-variadic.md +++ b/src/doc/unstable-book/src/language-features/c-variadic.md @@ -7,7 +7,7 @@ The tracking issue for this feature is: [#44930] ------------------------ The `c_variadic` language feature enables C-variadic functions to be -defined in Rust. The may be called both from within Rust and via FFI. +defined in Rust. They may be called both from within Rust and via FFI. ## Examples