From 18c14add399c35153883f8b84a2e11a7c22f6721 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 9 Sep 2021 14:05:10 -0700 Subject: [PATCH 01/27] Add a `try_clone()` function to `OwnedFd`. As suggested in #88564. This adds a `try_clone()` to `OwnedFd` by refactoring the code out of the existing `File`/`Socket` code. --- library/std/src/os/fd/owned.rs | 23 +++++++++++ library/std/src/os/windows/io/handle.rs | 33 +++++++++++++++ library/std/src/os/windows/io/socket.rs | 54 +++++++++++++++++++++++++ library/std/src/sys/unix/fd.rs | 17 +------- library/std/src/sys/windows/fs.rs | 2 +- library/std/src/sys/windows/handle.rs | 15 +------ library/std/src/sys/windows/net.rs | 47 +-------------------- 7 files changed, 115 insertions(+), 76 deletions(-) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 52d7d4690d39f..597e050b00d5a 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -8,6 +8,7 @@ use crate::fmt; use crate::fs; use crate::marker::PhantomData; use crate::mem::forget; +use crate::sys::cvt; use crate::sys_common::{AsInner, FromInner, IntoInner}; /// A borrowed file descriptor. @@ -67,6 +68,28 @@ impl BorrowedFd<'_> { } } +impl OwnedFd { + /// Creates a new `OwnedFd` instance that shares the same underlying file handle + /// as the existing `OwnedFd` instance. + pub fn try_clone(&self) -> crate::io::Result { + // We want to atomically duplicate this file descriptor and set the + // CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This + // is a POSIX flag that was added to Linux in 2.6.24. + #[cfg(not(target_os = "espidf"))] + let cmd = libc::F_DUPFD_CLOEXEC; + + // For ESP-IDF, F_DUPFD is used instead, because the CLOEXEC semantics + // will never be supported, as this is a bare metal framework with + // no capabilities for multi-process execution. While F_DUPFD is also + // not supported yet, it might be (currently it returns ENOSYS). + #[cfg(target_os = "espidf")] + let cmd = libc::F_DUPFD; + + let fd = cvt(unsafe { libc::fcntl(self.as_raw_fd(), cmd, 0) })?; + Ok(unsafe { Self::from_raw_fd(fd) }) + } +} + #[unstable(feature = "io_safety", issue = "87074")] impl AsRawFd for BorrowedFd<'_> { #[inline] diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 72a17adb3b470..64c3761511425 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -11,6 +11,7 @@ use crate::marker::PhantomData; use crate::mem::forget; use crate::ptr::NonNull; use crate::sys::c; +use crate::sys::cvt; use crate::sys_common::{AsInner, FromInner, IntoInner}; /// A borrowed handle. @@ -110,6 +111,38 @@ impl BorrowedHandle<'_> { } } +impl OwnedHandle { + /// Creates a new `OwnedHandle` instance that shares the same underlying file handle + /// as the existing `OwnedHandle` instance. + pub fn try_clone(&self) -> crate::io::Result { + let handle = self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS)?; + + Ok(unsafe { OwnedHandle::from_raw_handle(handle) }) + } + + pub(crate) fn duplicate( + &self, + access: c::DWORD, + inherit: bool, + options: c::DWORD, + ) -> io::Result { + let mut ret = 0 as c::HANDLE; + cvt(unsafe { + let cur_proc = c::GetCurrentProcess(); + c::DuplicateHandle( + cur_proc, + self.as_raw_handle(), + cur_proc, + &mut ret, + access, + inherit as c::BOOL, + options, + ) + })?; + unsafe { Ok(Self::from_raw_handle(ret)) } + } +} + impl TryFrom for OwnedHandle { type Error = (); diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index 23db66df09f7a..9e27ead90fb56 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -7,6 +7,7 @@ use crate::fmt; use crate::marker::PhantomData; use crate::mem::forget; use crate::sys::c; +use crate::sys::cvt; /// A borrowed socket. /// @@ -69,6 +70,59 @@ impl BorrowedSocket<'_> { } } +impl OwnedSocket { + /// Creates a new `OwnedSocket` instance that shares the same underlying socket + /// as the existing `OwnedSocket` instance. + pub fn try_clone(&self) -> io::Result { + let mut info = unsafe { mem::zeroed::() }; + let result = unsafe { + c::WSADuplicateSocketW(self.as_raw_socket(), c::GetCurrentProcessId(), &mut info) + }; + cvt(result)?; + let socket = unsafe { + c::WSASocketW( + info.iAddressFamily, + info.iSocketType, + info.iProtocol, + &mut info, + 0, + c::WSA_FLAG_OVERLAPPED | c::WSA_FLAG_NO_HANDLE_INHERIT, + ) + }; + + if socket != c::INVALID_SOCKET { + unsafe { Ok(Self::from_inner(OwnedSocket::from_raw_socket(socket))) } + } else { + let error = unsafe { c::WSAGetLastError() }; + + if error != c::WSAEPROTOTYPE && error != c::WSAEINVAL { + return Err(io::Error::from_raw_os_error(error)); + } + + let socket = unsafe { + c::WSASocketW( + info.iAddressFamily, + info.iSocketType, + info.iProtocol, + &mut info, + 0, + c::WSA_FLAG_OVERLAPPED, + ) + }; + + if socket == c::INVALID_SOCKET { + return Err(last_error()); + } + + unsafe { + let socket = Self::from_inner(OwnedSocket::from_raw_socket(socket)); + socket.set_no_inherit()?; + Ok(socket) + } + } + } +} + impl AsRawSocket for BorrowedSocket<'_> { #[inline] fn as_raw_socket(&self) -> RawSocket { diff --git a/library/std/src/sys/unix/fd.rs b/library/std/src/sys/unix/fd.rs index 0956726084e02..3c2b36fe691bc 100644 --- a/library/std/src/sys/unix/fd.rs +++ b/library/std/src/sys/unix/fd.rs @@ -266,22 +266,9 @@ impl FileDesc { } } + #[inline] pub fn duplicate(&self) -> io::Result { - // We want to atomically duplicate this file descriptor and set the - // CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This - // is a POSIX flag that was added to Linux in 2.6.24. - #[cfg(not(target_os = "espidf"))] - let cmd = libc::F_DUPFD_CLOEXEC; - - // For ESP-IDF, F_DUPFD is used instead, because the CLOEXEC semantics - // will never be supported, as this is a bare metal framework with - // no capabilities for multi-process execution. While F_DUPFD is also - // not supported yet, it might be (currently it returns ENOSYS). - #[cfg(target_os = "espidf")] - let cmd = libc::F_DUPFD; - - let fd = cvt(unsafe { libc::fcntl(self.as_raw_fd(), cmd, 0) })?; - Ok(unsafe { FileDesc::from_raw_fd(fd) }) + Ok(Self(self.0.try_clone()?)) } } diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 0c1a50e231cd4..08ff35361f4b6 100644 --- a/library/std/src/sys/windows/fs.rs +++ b/library/std/src/sys/windows/fs.rs @@ -455,7 +455,7 @@ impl File { } pub fn duplicate(&self) -> io::Result { - Ok(File { handle: self.handle.duplicate(0, false, c::DUPLICATE_SAME_ACCESS)? }) + Ok(Self(self.0.try_clone()?)) } fn reparse_point<'a>( diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index 21d86b002264a..8de5729daa38d 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -235,20 +235,7 @@ impl Handle { inherit: bool, options: c::DWORD, ) -> io::Result { - let mut ret = 0 as c::HANDLE; - cvt(unsafe { - let cur_proc = c::GetCurrentProcess(); - c::DuplicateHandle( - cur_proc, - self.as_raw_handle(), - cur_proc, - &mut ret, - access, - inherit as c::BOOL, - options, - ) - })?; - unsafe { Ok(Handle::from_raw_handle(ret)) } + Ok(Self(self.0.duplicate(access, inherit, options)?)) } } diff --git a/library/std/src/sys/windows/net.rs b/library/std/src/sys/windows/net.rs index 33152cc97abc0..681875985bdc9 100644 --- a/library/std/src/sys/windows/net.rs +++ b/library/std/src/sys/windows/net.rs @@ -208,52 +208,7 @@ impl Socket { } pub fn duplicate(&self) -> io::Result { - let mut info = unsafe { mem::zeroed::() }; - let result = unsafe { - c::WSADuplicateSocketW(self.as_raw_socket(), c::GetCurrentProcessId(), &mut info) - }; - cvt(result)?; - let socket = unsafe { - c::WSASocketW( - info.iAddressFamily, - info.iSocketType, - info.iProtocol, - &mut info, - 0, - c::WSA_FLAG_OVERLAPPED | c::WSA_FLAG_NO_HANDLE_INHERIT, - ) - }; - - if socket != c::INVALID_SOCKET { - unsafe { Ok(Self::from_inner(OwnedSocket::from_raw_socket(socket))) } - } else { - let error = unsafe { c::WSAGetLastError() }; - - if error != c::WSAEPROTOTYPE && error != c::WSAEINVAL { - return Err(io::Error::from_raw_os_error(error)); - } - - let socket = unsafe { - c::WSASocketW( - info.iAddressFamily, - info.iSocketType, - info.iProtocol, - &mut info, - 0, - c::WSA_FLAG_OVERLAPPED, - ) - }; - - if socket == c::INVALID_SOCKET { - return Err(last_error()); - } - - unsafe { - let socket = Self::from_inner(OwnedSocket::from_raw_socket(socket)); - socket.set_no_inherit()?; - Ok(socket) - } - } + Ok(Self(self.0.duplicate()?)) } fn recv_with_flags(&self, buf: &mut [u8], flags: c_int) -> io::Result { From 622dfcceb9328b359e28adaec8192390e494ca1e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 9 Sep 2021 14:44:54 -0700 Subject: [PATCH 02/27] Fix Windows compilation errors. --- library/std/src/os/windows/io/handle.rs | 3 ++- library/std/src/os/windows/io/socket.rs | 11 +++++++++-- library/std/src/sys/windows/fs.rs | 2 +- library/std/src/sys/windows/net.rs | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 64c3761511425..92c5f49e58a00 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -7,6 +7,7 @@ use crate::convert::TryFrom; use crate::ffi::c_void; use crate::fmt; use crate::fs; +use crate::io; use crate::marker::PhantomData; use crate::mem::forget; use crate::ptr::NonNull; @@ -114,7 +115,7 @@ impl BorrowedHandle<'_> { impl OwnedHandle { /// Creates a new `OwnedHandle` instance that shares the same underlying file handle /// as the existing `OwnedHandle` instance. - pub fn try_clone(&self) -> crate::io::Result { + pub fn try_clone(&self) -> crate::io::Result { let handle = self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS)?; Ok(unsafe { OwnedHandle::from_raw_handle(handle) }) diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index 9e27ead90fb56..0d77643404742 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -4,7 +4,9 @@ use super::raw::{AsRawSocket, FromRawSocket, IntoRawSocket, RawSocket}; use crate::fmt; +use crate::io; use crate::marker::PhantomData; +use crate::mem; use crate::mem::forget; use crate::sys::c; use crate::sys::cvt; @@ -91,7 +93,7 @@ impl OwnedSocket { }; if socket != c::INVALID_SOCKET { - unsafe { Ok(Self::from_inner(OwnedSocket::from_raw_socket(socket))) } + unsafe { Ok(Self(OwnedSocket::from_raw_socket(socket))) } } else { let error = unsafe { c::WSAGetLastError() }; @@ -115,7 +117,7 @@ impl OwnedSocket { } unsafe { - let socket = Self::from_inner(OwnedSocket::from_raw_socket(socket)); + let socket = Self(OwnedSocket::from_raw_socket(socket)); socket.set_no_inherit()?; Ok(socket) } @@ -123,6 +125,11 @@ impl OwnedSocket { } } +/// Returns the last error from the Windows socket interface. +fn last_error() -> io::Error { + io::Error::from_raw_os_error(unsafe { c::WSAGetLastError() }) +} + impl AsRawSocket for BorrowedSocket<'_> { #[inline] fn as_raw_socket(&self) -> RawSocket { diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs index 08ff35361f4b6..34455b7e3161a 100644 --- a/library/std/src/sys/windows/fs.rs +++ b/library/std/src/sys/windows/fs.rs @@ -455,7 +455,7 @@ impl File { } pub fn duplicate(&self) -> io::Result { - Ok(Self(self.0.try_clone()?)) + Ok(Self { handle: self.handle.try_clone()? }) } fn reparse_point<'a>( diff --git a/library/std/src/sys/windows/net.rs b/library/std/src/sys/windows/net.rs index 681875985bdc9..39417baebd44c 100644 --- a/library/std/src/sys/windows/net.rs +++ b/library/std/src/sys/windows/net.rs @@ -208,7 +208,7 @@ impl Socket { } pub fn duplicate(&self) -> io::Result { - Ok(Self(self.0.duplicate()?)) + Ok(Self(self.0.try_clone()?)) } fn recv_with_flags(&self, buf: &mut [u8], flags: c_int) -> io::Result { From c986c6b4ffec634d3c0ecbc3867a818bc41a59be Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 9 Sep 2021 15:30:17 -0700 Subject: [PATCH 03/27] Fix more Windows compilation errors. --- library/std/src/os/windows/io/handle.rs | 4 +--- library/std/src/os/windows/io/socket.rs | 17 +++++++++++++++-- library/std/src/sys/windows/handle.rs | 6 +++++- library/std/src/sys/windows/net.rs | 15 +-------------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 92c5f49e58a00..9522a05495c6f 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -116,9 +116,7 @@ impl OwnedHandle { /// Creates a new `OwnedHandle` instance that shares the same underlying file handle /// as the existing `OwnedHandle` instance. pub fn try_clone(&self) -> crate::io::Result { - let handle = self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS)?; - - Ok(unsafe { OwnedHandle::from_raw_handle(handle) }) + self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS) } pub(crate) fn duplicate( diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index 0d77643404742..7acd0af88b39a 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -93,7 +93,7 @@ impl OwnedSocket { }; if socket != c::INVALID_SOCKET { - unsafe { Ok(Self(OwnedSocket::from_raw_socket(socket))) } + unsafe { Ok(OwnedSocket::from_raw_socket(socket)) } } else { let error = unsafe { c::WSAGetLastError() }; @@ -117,12 +117,25 @@ impl OwnedSocket { } unsafe { - let socket = Self(OwnedSocket::from_raw_socket(socket)); + let socket = OwnedSocket::from_raw_socket(socket); socket.set_no_inherit()?; Ok(socket) } } } + + #[cfg(not(target_vendor = "uwp"))] + pub(crate) fn set_no_inherit(&self) -> io::Result<()> { + sys::cvt(unsafe { + c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0) + }) + .map(drop) + } + + #[cfg(target_vendor = "uwp")] + pub(crate) fn set_no_inherit(&self) -> io::Result<()> { + Err(io::Error::new_const(io::ErrorKind::Unsupported, &"Unavailable on UWP")) + } } /// Returns the last error from the Windows socket interface. diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index 8de5729daa38d..76a97e89be194 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -229,12 +229,16 @@ impl Handle { Ok(written as usize) } + pub fn try_clone(&self) -> io::Result { + Ok(Self(self.0.try_clone()?)) + } + pub fn duplicate( &self, access: c::DWORD, inherit: bool, options: c::DWORD, - ) -> io::Result { + ) -> io::Result { Ok(Self(self.0.duplicate(access, inherit, options)?)) } } diff --git a/library/std/src/sys/windows/net.rs b/library/std/src/sys/windows/net.rs index 39417baebd44c..75d4b3bd76862 100644 --- a/library/std/src/sys/windows/net.rs +++ b/library/std/src/sys/windows/net.rs @@ -129,7 +129,7 @@ impl Socket { unsafe { let socket = Self::from_raw_socket(socket); - socket.set_no_inherit()?; + socket.0.set_no_inherit()?; Ok(socket) } } @@ -371,19 +371,6 @@ impl Socket { } } - #[cfg(not(target_vendor = "uwp"))] - fn set_no_inherit(&self) -> io::Result<()> { - sys::cvt(unsafe { - c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0) - }) - .map(drop) - } - - #[cfg(target_vendor = "uwp")] - fn set_no_inherit(&self) -> io::Result<()> { - Err(io::Error::new_const(io::ErrorKind::Unsupported, &"Unavailable on UWP")) - } - pub fn shutdown(&self, how: Shutdown) -> io::Result<()> { let how = match how { Shutdown::Write => c::SD_SEND, From 2d6a4c85ca037ac07cac6029c2b174678c39a40d Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 9 Sep 2021 15:37:43 -0700 Subject: [PATCH 04/27] Fix another Windows compilation error. --- library/std/src/os/windows/io/socket.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index 7acd0af88b39a..e0af4f1f6cb16 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -126,7 +126,7 @@ impl OwnedSocket { #[cfg(not(target_vendor = "uwp"))] pub(crate) fn set_no_inherit(&self) -> io::Result<()> { - sys::cvt(unsafe { + cvt(unsafe { c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0) }) .map(drop) From 53e072f2207747e8b60dcbb77b763c1598cfb192 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 5 Oct 2021 14:11:49 -0700 Subject: [PATCH 05/27] Fix compilation on WASI, which doesn't yet support `dup`. --- library/std/src/os/fd/owned.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 597e050b00d5a..0b6588db92c83 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -8,6 +8,7 @@ use crate::fmt; use crate::fs; use crate::marker::PhantomData; use crate::mem::forget; +#[cfg(not(target_os = "wasi"))] use crate::sys::cvt; use crate::sys_common::{AsInner, FromInner, IntoInner}; @@ -71,6 +72,7 @@ impl BorrowedFd<'_> { impl OwnedFd { /// Creates a new `OwnedFd` instance that shares the same underlying file handle /// as the existing `OwnedFd` instance. + #[cfg(not(target_os = "wasi"))] pub fn try_clone(&self) -> crate::io::Result { // We want to atomically duplicate this file descriptor and set the // CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This @@ -88,6 +90,14 @@ impl OwnedFd { let fd = cvt(unsafe { libc::fcntl(self.as_raw_fd(), cmd, 0) })?; Ok(unsafe { Self::from_raw_fd(fd) }) } + + #[cfg(target_os = "wasi")] + pub fn try_clone(&self) -> crate::io::Result { + Err(crate::io::Error::new_const( + crate::io::ErrorKind::Unsupported, + &"operation not supported on WASI yet", + )) + } } #[unstable(feature = "io_safety", issue = "87074")] From 83aebf8f7bb6d766f6b68accdc44acb9cea3d57e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 12 Jan 2022 11:41:48 -0800 Subject: [PATCH 06/27] Use the correct `cvt` for converting socket errors on Windows. `WSADuplicateSocketW` returns 0 on success, which differs from handle-oriented functions which return 0 on error. Use `sys::net::cvt` to handle its return value, which handles the socket convention of returning 0 on success, rather than `sys::cvt`, which handles the handle-oriented convention of returning 0 on failure. --- library/std/src/os/windows/io/socket.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index e0af4f1f6cb16..26b569bcdd362 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -8,6 +8,7 @@ use crate::io; use crate::marker::PhantomData; use crate::mem; use crate::mem::forget; +use crate::sys; use crate::sys::c; use crate::sys::cvt; @@ -80,7 +81,7 @@ impl OwnedSocket { let result = unsafe { c::WSADuplicateSocketW(self.as_raw_socket(), c::GetCurrentProcessId(), &mut info) }; - cvt(result)?; + sys::net::cvt(result)?; let socket = unsafe { c::WSASocketW( info.iAddressFamily, From 02f1a565fee32ac9710b19a73539646d6454b085 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 18 Jan 2022 23:03:55 -0500 Subject: [PATCH 07/27] Properly track `DepNode`s in trait evaluation provisional cache Fixes #92987 During evaluation of an auto trait predicate, we may encounter a cycle. This causes us to store the evaluation result in a special 'provisional cache;. If we later end up determining that the type can legitimately implement the auto trait despite the cycle, we remove the entry from the provisional cache, and insert it into the evaluation cache. Additionally, trait evaluation creates a special anonymous `DepNode`. All queries invoked during the predicate evaluation are added as outoging dependency edges from the `DepNode`. This `DepNode` is then store in the evaluation cache - if a different query ends up reading from the cache entry, it will also perform a read of the stored `DepNode`. As a result, the cached evaluation will still end up (transitively) incurring all of the same dependencies that it would if it actually performed the uncached evaluation (e.g. a call to `type_of` to determine constituent types). Previously, we did not correctly handle the interaction between the provisional cache and the created `DepNode`. Storing an evaluation result in the provisional cache would cause us to lose the `DepNode` created during the evaluation. If we later moved the entry from the provisional cache to the evaluation cache, we would use the `DepNode` associated with the evaluation that caused us to 'complete' the cycle, not the evaluatoon where we first discovered the cycle. As a result, future reads from the evaluation cache would miss some incremental compilation dependencies that would have otherwise been added if the evaluation was *not* cached. Under the right circumstances, this could lead to us trying to force a query with a no-longer-existing `DefPathHash`, since we were missing the (red) dependency edge that would have caused us to bail out before attempting forcing. This commit makes the provisional cache store the `DepNode` create during the provisional evaluation. When we move an entry from the provisional cache to the evaluation cache, we create a *new* `DepNode` that has dependencies going to *both* of the evaluation `DepNodes` we have available. This ensures that cached reads will incur all of the necessary dependency edges. --- .../src/traits/select/mod.rs | 63 +++++++++++++++---- .../issue-92987-provisional-dep-node.rs | 24 +++++++ 2 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 src/test/incremental/issue-92987-provisional-dep-node.rs diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 1414c742635c4..947fcdea7ab25 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -765,14 +765,38 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!(?result, "CACHE MISS"); self.insert_evaluation_cache(param_env, fresh_trait_pred, dep_node, result); - stack.cache().on_completion(stack.dfn, |fresh_trait_pred, provisional_result| { - self.insert_evaluation_cache( - param_env, - fresh_trait_pred, - dep_node, - provisional_result.max(result), - ); - }); + stack.cache().on_completion( + stack.dfn, + |fresh_trait_pred, provisional_result, provisional_dep_node| { + // Create a new `DepNode` that has dependencies on: + // * The `DepNode` for the original evaluation that resulted in a provisional cache + // entry being crated + // * The `DepNode` for the *current* evaluation, which resulted in us completing + // provisional caches entries and inserting them into the evaluation cache + // + // This ensures that when a query reads this entry from the evaluation cache, + // it will end up (transitively) dependening on all of the incr-comp dependencies + // created during the evaluation of this trait. For example, evaluating a trait + // will usually require us to invoke `type_of(field_def_id)` to determine the + // constituent types, and we want any queries reading from this evaluation + // cache entry to end up with a transitive `type_of(field_def_id`)` dependency. + // + // By using `in_task`, we're also creating an edge from the *current* query + // to the newly-created `combined_dep_node`. This is probably redundant, + // but it's better to add too many dep graph edges than to add too few + // dep graph edges. + let ((), combined_dep_node) = self.in_task(|this| { + this.tcx().dep_graph.read_index(provisional_dep_node); + this.tcx().dep_graph.read_index(dep_node); + }); + self.insert_evaluation_cache( + param_env, + fresh_trait_pred, + combined_dep_node, + provisional_result.max(result), + ); + }, + ); } else { debug!(?result, "PROVISIONAL"); debug!( @@ -781,7 +805,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fresh_trait_pred, stack.depth, reached_depth, ); - stack.cache().insert_provisional(stack.dfn, reached_depth, fresh_trait_pred, result); + stack.cache().insert_provisional( + stack.dfn, + reached_depth, + fresh_trait_pred, + result, + dep_node, + ); } Ok(result) @@ -2506,6 +2536,11 @@ struct ProvisionalEvaluation { from_dfn: usize, reached_depth: usize, result: EvaluationResult, + /// The `DepNodeIndex` created for the `evaluate_stack` call for this provisional + /// evaluation. When we create an entry in the evaluation cache using this provisional + /// cache entry (see `on_completion`), we use this `dep_node` to ensure that future reads from + /// the cache will have all of the necessary incr comp dependencies tracked. + dep_node: DepNodeIndex, } impl<'tcx> Default for ProvisionalEvaluationCache<'tcx> { @@ -2548,6 +2583,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { reached_depth: usize, fresh_trait_pred: ty::PolyTraitPredicate<'tcx>, result: EvaluationResult, + dep_node: DepNodeIndex, ) { debug!(?from_dfn, ?fresh_trait_pred, ?result, "insert_provisional"); @@ -2573,7 +2609,10 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { } } - map.insert(fresh_trait_pred, ProvisionalEvaluation { from_dfn, reached_depth, result }); + map.insert( + fresh_trait_pred, + ProvisionalEvaluation { from_dfn, reached_depth, result, dep_node }, + ); } /// Invoked when the node with dfn `dfn` does not get a successful @@ -2624,7 +2663,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { fn on_completion( &self, dfn: usize, - mut op: impl FnMut(ty::PolyTraitPredicate<'tcx>, EvaluationResult), + mut op: impl FnMut(ty::PolyTraitPredicate<'tcx>, EvaluationResult, DepNodeIndex), ) { debug!(?dfn, "on_completion"); @@ -2633,7 +2672,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { { debug!(?fresh_trait_pred, ?eval, "on_completion"); - op(fresh_trait_pred, eval.result); + op(fresh_trait_pred, eval.result, eval.dep_node); } } } diff --git a/src/test/incremental/issue-92987-provisional-dep-node.rs b/src/test/incremental/issue-92987-provisional-dep-node.rs new file mode 100644 index 0000000000000..a48a8373c2b59 --- /dev/null +++ b/src/test/incremental/issue-92987-provisional-dep-node.rs @@ -0,0 +1,24 @@ +// revisions: rpass1 rpass2 + +// Regression test for issue #92987 +// Tests that we properly manage `DepNode`s during trait evaluation +// involing an auto-trait cycle. + +#[cfg(rpass1)] +struct CycleOne(Box); + +#[cfg(rpass2)] +enum CycleOne { + Variant(Box) +} + +struct CycleTwo(CycleOne); + +fn assert_send() {} + +fn bar() { + assert_send::(); + assert_send::(); +} + +fn main() {} From f518827503f8a88e3ecad9a7645d4fc7cd8cfebe Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 10:50:42 -0300 Subject: [PATCH 08/27] Use impl1 and impl2 instead of a and b prefixes --- .../src/traits/coherence.rs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index af3540386f9fc..a875e2ccf950f 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -140,21 +140,21 @@ fn with_fresh_ty_vars<'cx, 'tcx>( fn overlap<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, skip_leak_check: SkipLeakCheck, - a_def_id: DefId, - b_def_id: DefId, + impl1_def_id: DefId, + impl2_def_id: DefId, ) -> Option> { - debug!("overlap(a_def_id={:?}, b_def_id={:?})", a_def_id, b_def_id); + debug!("overlap(impl1_def_id={:?}, impl2_def_id={:?})", impl1_def_id, impl2_def_id); selcx.infcx().probe_maybe_skip_leak_check(skip_leak_check.is_yes(), |snapshot| { - overlap_within_probe(selcx, skip_leak_check, a_def_id, b_def_id, snapshot) + overlap_within_probe(selcx, skip_leak_check, impl1_def_id, impl2_def_id, snapshot) }) } fn overlap_within_probe<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, skip_leak_check: SkipLeakCheck, - a_def_id: DefId, - b_def_id: DefId, + impl1_def_id: DefId, + impl2_def_id: DefId, snapshot: &CombinedSnapshot<'_, 'tcx>, ) -> Option> { fn loose_check<'cx, 'tcx>( @@ -182,17 +182,17 @@ fn overlap_within_probe<'cx, 'tcx>( // empty environment. let param_env = ty::ParamEnv::empty(); - let a_impl_header = with_fresh_ty_vars(selcx, param_env, a_def_id); - let b_impl_header = with_fresh_ty_vars(selcx, param_env, b_def_id); + let impl1_header = with_fresh_ty_vars(selcx, param_env, impl1_def_id); + let impl2_header = with_fresh_ty_vars(selcx, param_env, impl2_def_id); - debug!("overlap: a_impl_header={:?}", a_impl_header); - debug!("overlap: b_impl_header={:?}", b_impl_header); + debug!("overlap: impl1_header={:?}", impl1_header); + debug!("overlap: impl2_header={:?}", impl2_header); // Do `a` and `b` unify? If not, no overlap. let obligations = match selcx .infcx() .at(&ObligationCause::dummy(), param_env) - .eq_impl_headers(&a_impl_header, &b_impl_header) + .eq_impl_headers(&impl1_header, &impl2_header) { Ok(InferOk { obligations, value: () }) => obligations, Err(_) => { @@ -225,11 +225,11 @@ fn overlap_within_probe<'cx, 'tcx>( // at some point an impl for `&'?a str: Error` could be added. let infcx = selcx.infcx(); let tcx = infcx.tcx; - let opt_failing_obligation = a_impl_header + let opt_failing_obligation = impl1_header .predicates .iter() .copied() - .chain(b_impl_header.predicates) + .chain(impl2_header.predicates) .map(|p| infcx.resolve_vars_if_possible(p)) .map(|p| Obligation { cause: ObligationCause::dummy(), @@ -241,8 +241,8 @@ fn overlap_within_probe<'cx, 'tcx>( .find(|o| { // if both impl headers are set to strict coherence it means that this will be accepted // only if it's stated that T: !Trait. So only prove that the negated obligation holds. - if tcx.has_attr(a_def_id, sym::rustc_strict_coherence) - && tcx.has_attr(b_def_id, sym::rustc_strict_coherence) + if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) + && tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence) { strict_check(selcx, o) } else { @@ -265,7 +265,7 @@ fn overlap_within_probe<'cx, 'tcx>( } } - let impl_header = selcx.infcx().resolve_vars_if_possible(a_impl_header); + let impl_header = selcx.infcx().resolve_vars_if_possible(impl1_header); let intercrate_ambiguity_causes = selcx.take_intercrate_ambiguity_causes(); debug!("overlap: intercrate_ambiguity_causes={:#?}", intercrate_ambiguity_causes); From 052b31b5874a509a642a28cae0e20fe6031e766a Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 10:53:17 -0300 Subject: [PATCH 09/27] Move auxiliary fns out of overlap_with_probe --- .../src/traits/coherence.rs | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index a875e2ccf950f..4383836b98bb9 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -157,25 +157,6 @@ fn overlap_within_probe<'cx, 'tcx>( impl2_def_id: DefId, snapshot: &CombinedSnapshot<'_, 'tcx>, ) -> Option> { - fn loose_check<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, - o: &PredicateObligation<'tcx>, - ) -> bool { - !selcx.predicate_may_hold_fatal(o) - } - - fn strict_check<'cx, 'tcx>( - selcx: &SelectionContext<'cx, 'tcx>, - o: &PredicateObligation<'tcx>, - ) -> bool { - let infcx = selcx.infcx(); - let tcx = infcx.tcx; - o.flip_polarity(tcx) - .as_ref() - .map(|o| selcx.infcx().predicate_must_hold_modulo_regions(o)) - .unwrap_or(false) - } - // For the purposes of this check, we don't bring any placeholder // types into scope; instead, we replace the generic types with // fresh type variables, and hence we do our evaluations in an @@ -275,6 +256,25 @@ fn overlap_within_probe<'cx, 'tcx>( Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder }) } +fn loose_check<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + o: &PredicateObligation<'tcx>, +) -> bool { + !selcx.predicate_may_hold_fatal(o) +} + +fn strict_check<'cx, 'tcx>( + selcx: &SelectionContext<'cx, 'tcx>, + o: &PredicateObligation<'tcx>, +) -> bool { + let infcx = selcx.infcx(); + let tcx = infcx.tcx; + o.flip_polarity(tcx) + .as_ref() + .map(|o| selcx.infcx().predicate_must_hold_modulo_regions(o)) + .unwrap_or(false) +} + pub fn trait_ref_is_knowable<'tcx>( tcx: TyCtxt<'tcx>, trait_ref: ty::TraitRef<'tcx>, From f4b42946c8d0e2b74dc78c34a833cc94c2df251d Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Fri, 21 Jan 2022 13:01:58 -0500 Subject: [PATCH 10/27] Remove FIXME and fix inconsistency of local blanket impls by using HIR for them --- src/librustdoc/clean/blanket_impl.rs | 28 ++++++++++++++++++++++------ src/librustdoc/json/mod.rs | 15 +-------------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/librustdoc/clean/blanket_impl.rs b/src/librustdoc/clean/blanket_impl.rs index eafc74b9945ba..75ee663b926c4 100644 --- a/src/librustdoc/clean/blanket_impl.rs +++ b/src/librustdoc/clean/blanket_impl.rs @@ -101,6 +101,27 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> { cx.generated_synthetics.insert((ty, trait_def_id)); + let hir_imp = impl_def_id.as_local() + .map(|local| cx.tcx.hir().expect_item(local)) + .and_then(|item| if let hir::ItemKind::Impl(i) = &item.kind { + Some(i) + } else { + None + }); + + let items = match hir_imp { + Some(imp) => imp + .items + .iter() + .map(|ii| cx.tcx.hir().impl_item(ii.id).clean(cx)) + .collect::>(), + None => cx.tcx + .associated_items(impl_def_id) + .in_definition_order() + .map(|x| x.clean(cx)) + .collect::>(), + }; + impls.push(Item { name: None, attrs: Default::default(), @@ -117,12 +138,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> { // the post-inference `trait_ref`, as it's more accurate. trait_: Some(trait_ref.clean(cx)), for_: ty.clean(cx), - items: cx - .tcx - .associated_items(impl_def_id) - .in_definition_order() - .map(|x| x.clean(cx)) - .collect::>(), + items, polarity: ty::ImplPolarity::Positive, kind: ImplKind::Blanket(box trait_ref.self_ty().clean(cx)), }), diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs index 8f484766d9a5b..f9e9fe0d3cf20 100644 --- a/src/librustdoc/json/mod.rs +++ b/src/librustdoc/json/mod.rs @@ -172,21 +172,8 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> { /// the hashmap because certain items (traits and types) need to have their mappings for trait /// implementations filled out before they're inserted. fn item(&mut self, item: clean::Item) -> Result<(), Error> { - let local_blanket_impl = match item.def_id { - clean::ItemId::Blanket { impl_id, .. } => impl_id.is_local(), - clean::ItemId::Auto { .. } - | clean::ItemId::DefId(_) - | clean::ItemId::Primitive(_, _) => false, - }; - // Flatten items that recursively store other items - // FIXME(CraftSpider): We skip children of local blanket implementations, as we'll have - // already seen the actual generic impl, and the generated ones don't need documenting. - // This is necessary due to the visibility, return type, and self arg of the generated - // impls not quite matching, and will no longer be necessary when the mismatch is fixed. - if !local_blanket_impl { - item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); - } + item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap()); let id = item.def_id; if let Some(mut new_item) = self.convert_item(item) { From 66d056a9bf0f7d486c2ed637c1e1a7b9ce0c3bbc Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Fri, 21 Jan 2022 13:19:18 -0500 Subject: [PATCH 11/27] Update test to include `self` case --- src/test/rustdoc-json/impls/blanket_with_local.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/rustdoc-json/impls/blanket_with_local.rs b/src/test/rustdoc-json/impls/blanket_with_local.rs index 963ea2fe5aea8..e5f48820720fd 100644 --- a/src/test/rustdoc-json/impls/blanket_with_local.rs +++ b/src/test/rustdoc-json/impls/blanket_with_local.rs @@ -4,10 +4,12 @@ // @has blanket_with_local.json "$.index[*][?(@.name=='Load')]" pub trait Load { fn load() {} + fn write(self) {} } impl

Load for P { fn load() {} + fn write(self) {} } // @has - "$.index[*][?(@.name=='Wrapper')]" From 1a0278e1d17f66ddc5975f38387746f85c6602ce Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Thu, 20 Jan 2022 20:23:27 -0500 Subject: [PATCH 12/27] Work around missing code coverage data causing llvm-cov failures If we do not add code coverage instrumentation to the `Body` of a function, then when we go to generate the function record for it, we won't write any data and this later causes llvm-cov to fail when processing data for the entire coverage report. I've identified two main cases where we do not currently add code coverage instrumentation to the `Body` of a function: 1. If the function has a single `BasicBlock` and it ends with a `TerminatorKind::Unreachable`. 2. If the function is created using a proc macro of some kind. For case 1, this typically not important as this most often occurs as the result of function definitions that take or return uninhabited types. These kinds of functions, by definition, cannot even be called so they logically should not be counted in code coverage statistics. For case 2, I haven't looked into this very much but I've noticed while testing this patch that (other than functions which are covered by case 1) the skipped function coverage debug message is occasionally triggered in large crate graphs by functions generated from a proc macro. This may have something to do with weird spans being generated by the proc macro but this is just a guess. I think it's reasonable to land this change since currently, we fail to generate *any* results from llvm-cov when a function has no coverage instrumentation applied to it. With this change, we get coverage data for all functions other than the two cases discussed above. --- .../src/coverageinfo/mapgen.rs | 17 ++++++++--- .../expected_show_coverage.issue-93054.txt | 29 +++++++++++++++++++ .../run-make-fulldeps/coverage/issue-93054.rs | 28 ++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-93054.txt create mode 100644 src/test/run-make-fulldeps/coverage/issue-93054.rs diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 32f18419753e9..3014d2f1930ee 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -9,6 +9,7 @@ use rustc_data_structures::fx::FxIndexSet; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefIdSet; use rustc_llvm::RustString; +use rustc_middle::bug; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir::coverage::CodeRegion; use rustc_middle::ty::TyCtxt; @@ -76,10 +77,18 @@ pub fn finalize<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) { let coverage_mapping_buffer = llvm::build_byte_buffer(|coverage_mapping_buffer| { mapgen.write_coverage_mapping(expressions, counter_regions, coverage_mapping_buffer); }); - debug_assert!( - !coverage_mapping_buffer.is_empty(), - "Every `FunctionCoverage` should have at least one counter" - ); + + if coverage_mapping_buffer.is_empty() { + if function_coverage.is_used() { + bug!( + "A used function should have had coverage mapping data but did not: {}", + mangled_function_name + ); + } else { + debug!("unused function had no coverage mapping data: {}", mangled_function_name); + continue; + } + } function_data.push((mangled_function_name, source_hash, is_used, coverage_mapping_buffer)); } diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-93054.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-93054.txt new file mode 100644 index 0000000000000..a1655adedd447 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-93054.txt @@ -0,0 +1,29 @@ + 1| |// Regression test for #93054: Functions using uninhabited types often only have a single, + 2| |// unreachable basic block which doesn't get instrumented. This should not cause llvm-cov to fail. + 3| |// Since these kinds functions can't be invoked anyway, it's ok to not have coverage data for them. + 4| | + 5| |// compile-flags: --edition=2021 + 6| | + 7| |enum Never { } + 8| | + 9| |impl Never { + 10| | fn foo(self) { + 11| | match self { } + 12| | make().map(|never| match never { }); + 13| | } + 14| | + 15| | fn bar(&self) { + 16| | match *self { } + 17| | } + 18| |} + 19| | + 20| 0|async fn foo2(never: Never) { + 21| | match never { } + 22| |} + 23| | + 24| 0|fn make() -> Option { + 25| 0| None + 26| 0|} + 27| | + 28| 1|fn main() { } + diff --git a/src/test/run-make-fulldeps/coverage/issue-93054.rs b/src/test/run-make-fulldeps/coverage/issue-93054.rs new file mode 100644 index 0000000000000..c160b3db03f8e --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/issue-93054.rs @@ -0,0 +1,28 @@ +// Regression test for #93054: Functions using uninhabited types often only have a single, +// unreachable basic block which doesn't get instrumented. This should not cause llvm-cov to fail. +// Since these kinds functions can't be invoked anyway, it's ok to not have coverage data for them. + +// compile-flags: --edition=2021 + +enum Never { } + +impl Never { + fn foo(self) { + match self { } + make().map(|never| match never { }); + } + + fn bar(&self) { + match *self { } + } +} + +async fn foo2(never: Never) { + match never { } +} + +fn make() -> Option { + None +} + +fn main() { } From b2a45f064514e06908564153014f8860b8b206d2 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 11:02:52 -0300 Subject: [PATCH 13/27] Extract stable_disjoint fn --- .../src/traits/coherence.rs | 62 ++++++++++++------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 4383836b98bb9..84b934c19f9d3 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -157,6 +157,9 @@ fn overlap_within_probe<'cx, 'tcx>( impl2_def_id: DefId, snapshot: &CombinedSnapshot<'_, 'tcx>, ) -> Option> { + let infcx = selcx.infcx(); + let tcx = infcx.tcx; + // For the purposes of this check, we don't bring any placeholder // types into scope; instead, we replace the generic types with // fresh type variables, and hence we do our evaluations in an @@ -166,6 +169,39 @@ fn overlap_within_probe<'cx, 'tcx>( let impl1_header = with_fresh_ty_vars(selcx, param_env, impl1_def_id); let impl2_header = with_fresh_ty_vars(selcx, param_env, impl2_def_id); + let strict_coherence = tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) + && tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence); + + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, strict_coherence) { + return None; + } + + if !skip_leak_check.is_yes() { + if infcx.leak_check(true, snapshot).is_err() { + debug!("overlap: leak check failed"); + return None; + } + } + + let intercrate_ambiguity_causes = selcx.take_intercrate_ambiguity_causes(); + debug!("overlap: intercrate_ambiguity_causes={:#?}", intercrate_ambiguity_causes); + + let involves_placeholder = + matches!(selcx.infcx().region_constraints_added_in_snapshot(snapshot), Some(true)); + + let impl_header = selcx.infcx().resolve_vars_if_possible(impl1_header); + Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder }) +} + +/// Given impl1 and impl2 check if both impls can be satisfied by a common type (including +/// where-clauses) If so, return false, otherwise return true, they are disjoint. +fn stable_disjoint<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + param_env: ty::ParamEnv<'tcx>, + impl1_header: &ty::ImplHeader<'tcx>, + impl2_header: ty::ImplHeader<'tcx>, + strict_coherence: bool, +) -> bool { debug!("overlap: impl1_header={:?}", impl1_header); debug!("overlap: impl2_header={:?}", impl2_header); @@ -177,7 +213,7 @@ fn overlap_within_probe<'cx, 'tcx>( { Ok(InferOk { obligations, value: () }) => obligations, Err(_) => { - return None; + return true; } }; @@ -222,9 +258,7 @@ fn overlap_within_probe<'cx, 'tcx>( .find(|o| { // if both impl headers are set to strict coherence it means that this will be accepted // only if it's stated that T: !Trait. So only prove that the negated obligation holds. - if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) - && tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence) - { + if strict_coherence { strict_check(selcx, o) } else { loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) @@ -236,24 +270,10 @@ fn overlap_within_probe<'cx, 'tcx>( if let Some(failing_obligation) = opt_failing_obligation { debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); - return None; - } - - if !skip_leak_check.is_yes() { - if infcx.leak_check(true, snapshot).is_err() { - debug!("overlap: leak check failed"); - return None; - } + true + } else { + false } - - let impl_header = selcx.infcx().resolve_vars_if_possible(impl1_header); - let intercrate_ambiguity_causes = selcx.take_intercrate_ambiguity_causes(); - debug!("overlap: intercrate_ambiguity_causes={:#?}", intercrate_ambiguity_causes); - - let involves_placeholder = - matches!(selcx.infcx().region_constraints_added_in_snapshot(snapshot), Some(true)); - - Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder }) } fn loose_check<'cx, 'tcx>( From c2890ed426aaef4b680848e793a2a741e517a6cc Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 12:22:13 -0300 Subject: [PATCH 14/27] Add overlap mode --- .../src/traits/coherence.rs | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 84b934c19f9d3..170740d2cbdc1 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -135,6 +135,25 @@ fn with_fresh_ty_vars<'cx, 'tcx>( header } +enum OverlapMode { + Stable, + Strict, +} + +fn overlap_mode<'tcx>(tcx: TyCtxt<'tcx>, impl1_def_id: DefId, impl2_def_id: DefId) -> OverlapMode { + if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) + != tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence) + { + bug!("Use strict coherence on both impls",); + } + + if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) { + OverlapMode::Strict + } else { + OverlapMode::Stable + } +} + /// Can both impl `a` and impl `b` be satisfied by a common type (including /// where-clauses)? If so, returns an `ImplHeader` that unifies the two impls. fn overlap<'cx, 'tcx>( @@ -169,10 +188,8 @@ fn overlap_within_probe<'cx, 'tcx>( let impl1_header = with_fresh_ty_vars(selcx, param_env, impl1_def_id); let impl2_header = with_fresh_ty_vars(selcx, param_env, impl2_def_id); - let strict_coherence = tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) - && tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence); - - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, strict_coherence) { + let overlap_mode = overlap_mode(tcx, impl1_def_id, impl2_def_id); + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, overlap_mode) { return None; } @@ -200,7 +217,7 @@ fn stable_disjoint<'cx, 'tcx>( param_env: ty::ParamEnv<'tcx>, impl1_header: &ty::ImplHeader<'tcx>, impl2_header: ty::ImplHeader<'tcx>, - strict_coherence: bool, + overlap_mode: OverlapMode, ) -> bool { debug!("overlap: impl1_header={:?}", impl1_header); debug!("overlap: impl2_header={:?}", impl2_header); @@ -258,10 +275,11 @@ fn stable_disjoint<'cx, 'tcx>( .find(|o| { // if both impl headers are set to strict coherence it means that this will be accepted // only if it's stated that T: !Trait. So only prove that the negated obligation holds. - if strict_coherence { - strict_check(selcx, o) - } else { - loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) + match overlap_mode { + OverlapMode::Stable => { + loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) + } + OverlapMode::Strict => strict_check(selcx, o), } }); // FIXME: the call to `selcx.predicate_may_hold_fatal` above should be ported From d2d25a5be0c45b8aa247a3d0b6c7fda12e9ede9a Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 12:53:50 -0300 Subject: [PATCH 15/27] Implement stable with negative coherence mode --- compiler/rustc_feature/src/builtin_attrs.rs | 1 + compiler/rustc_span/src/symbol.rs | 1 + .../src/traits/coherence.rs | 99 ++++++++++++++++++- .../ui/coherence/auxiliary/option_future.rs | 8 ++ .../coherence-overlap-negative-trait2.rs | 18 ++++ 5 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/coherence/auxiliary/option_future.rs create mode 100644 src/test/ui/coherence/coherence-overlap-negative-trait2.rs diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs index 723cc06864a90..0e643ff599834 100644 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/compiler/rustc_feature/src/builtin_attrs.rs @@ -697,6 +697,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[ rustc_attr!(TEST, rustc_capture_analysis, Normal, template!(Word), WarnFollowing), rustc_attr!(TEST, rustc_insignificant_dtor, Normal, template!(Word), WarnFollowing), rustc_attr!(TEST, rustc_strict_coherence, Normal, template!(Word), WarnFollowing), + rustc_attr!(TEST, rustc_with_negative_coherence, Normal, template!(Word), WarnFollowing), rustc_attr!(TEST, rustc_variance, Normal, template!(Word), WarnFollowing), rustc_attr!(TEST, rustc_layout, Normal, template!(List: "field1, field2, ..."), WarnFollowing), rustc_attr!(TEST, rustc_regions, Normal, template!(Word), WarnFollowing), diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 702e359466079..7e0ce89be67fc 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -1203,6 +1203,7 @@ symbols! { rustc_trivial_field_reads, rustc_unsafe_specialization_marker, rustc_variance, + rustc_with_negative_coherence, rustdoc, rustdoc_internals, rustfmt, diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 170740d2cbdc1..42b7139c00698 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -7,9 +7,11 @@ use crate::infer::{CombinedSnapshot, InferOk, TyCtxtInferExt}; use crate::traits::query::evaluate_obligation::InferCtxtExt; use crate::traits::select::IntercrateAmbiguityCause; +use crate::traits::util::impl_trait_ref_and_oblig; use crate::traits::SkipLeakCheck; use crate::traits::{ - self, Normalized, Obligation, ObligationCause, PredicateObligation, SelectionContext, + self, FulfillmentContext, Normalized, Obligation, ObligationCause, PredicateObligation, + SelectionContext, }; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences}; @@ -137,6 +139,7 @@ fn with_fresh_ty_vars<'cx, 'tcx>( enum OverlapMode { Stable, + WithNegative, Strict, } @@ -147,8 +150,16 @@ fn overlap_mode<'tcx>(tcx: TyCtxt<'tcx>, impl1_def_id: DefId, impl2_def_id: DefI bug!("Use strict coherence on both impls",); } + if tcx.has_attr(impl1_def_id, sym::rustc_with_negative_coherence) + != tcx.has_attr(impl2_def_id, sym::rustc_with_negative_coherence) + { + bug!("Use with negative coherence on both impls",); + } + if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) { OverlapMode::Strict + } else if tcx.has_attr(impl1_def_id, sym::rustc_with_negative_coherence) { + OverlapMode::WithNegative } else { OverlapMode::Stable } @@ -188,9 +199,25 @@ fn overlap_within_probe<'cx, 'tcx>( let impl1_header = with_fresh_ty_vars(selcx, param_env, impl1_def_id); let impl2_header = with_fresh_ty_vars(selcx, param_env, impl2_def_id); - let overlap_mode = overlap_mode(tcx, impl1_def_id, impl2_def_id); - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, overlap_mode) { - return None; + match overlap_mode(tcx, impl1_def_id, impl2_def_id) { + OverlapMode::Stable => { + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Stable) { + return None; + } + } + OverlapMode::Strict => { + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Strict) { + return None; + } + } + OverlapMode::WithNegative => { + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Stable) + || explicit_disjoint(selcx, impl1_def_id, impl2_def_id) + || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) + { + return None; + } + } } if !skip_leak_check.is_yes() { @@ -280,6 +307,7 @@ fn stable_disjoint<'cx, 'tcx>( loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) } OverlapMode::Strict => strict_check(selcx, o), + OverlapMode::WithNegative => loose_check(selcx, o), } }); // FIXME: the call to `selcx.predicate_may_hold_fatal` above should be ported @@ -294,6 +322,69 @@ fn stable_disjoint<'cx, 'tcx>( } } +/// Given impl1 and impl2 check if both impls are never satisfied by a common type (including +/// where-clauses) If so, return true, they are disjoint and false otherwise. +fn explicit_disjoint<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + impl1_def_id: DefId, + impl2_def_id: DefId, +) -> bool { + let tcx = selcx.infcx().tcx; + + // create a parameter environment corresponding to a (placeholder) instantiation of impl1 + let impl1_env = tcx.param_env(impl1_def_id); + let impl1_trait_ref = tcx.impl_trait_ref(impl1_def_id).unwrap(); + + // Create an infcx, taking the predicates of impl1 as assumptions: + tcx.infer_ctxt().enter(|infcx| { + // Normalize the trait reference. The WF rules ought to ensure + // that this always succeeds. + let impl1_trait_ref = match traits::fully_normalize( + &infcx, + FulfillmentContext::new(), + ObligationCause::dummy(), + impl1_env, + impl1_trait_ref, + ) { + Ok(impl1_trait_ref) => impl1_trait_ref, + Err(err) => { + bug!("failed to fully normalize {:?}: {:?}", impl1_trait_ref, err); + } + }; + + // Attempt to prove that impl2 applies, given all of the above. + let selcx = &mut SelectionContext::new(&infcx); + let impl2_substs = infcx.fresh_substs_for_item(DUMMY_SP, impl2_def_id); + let (impl2_trait_ref, obligations) = + impl_trait_ref_and_oblig(selcx, impl1_env, impl2_def_id, impl2_substs); + + // do the impls unify? If not, not disjoint. + let more_obligations = match infcx + .at(&ObligationCause::dummy(), impl1_env) + .eq(impl1_trait_ref, impl2_trait_ref) + { + Ok(InferOk { obligations, .. }) => obligations, + Err(_) => { + debug!( + "explicit_disjoint: {:?} does not unify with {:?}", + impl1_trait_ref, impl2_trait_ref + ); + return false; + } + }; + + let opt_failing_obligation = + obligations.into_iter().chain(more_obligations).find(|o| strict_check(selcx, o)); + + if let Some(failing_obligation) = opt_failing_obligation { + debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); + true + } else { + false + } + }) +} + fn loose_check<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, o: &PredicateObligation<'tcx>, diff --git a/src/test/ui/coherence/auxiliary/option_future.rs b/src/test/ui/coherence/auxiliary/option_future.rs new file mode 100644 index 0000000000000..f71df1b87fcda --- /dev/null +++ b/src/test/ui/coherence/auxiliary/option_future.rs @@ -0,0 +1,8 @@ +#![crate_type = "lib"] +#![feature(negative_impls)] +#![feature(rustc_attrs)] + +pub trait Future {} + +#[rustc_with_negative_coherence] +impl !Future for Option where E: Sized {} diff --git a/src/test/ui/coherence/coherence-overlap-negative-trait2.rs b/src/test/ui/coherence/coherence-overlap-negative-trait2.rs new file mode 100644 index 0000000000000..1f47b5ba46e41 --- /dev/null +++ b/src/test/ui/coherence/coherence-overlap-negative-trait2.rs @@ -0,0 +1,18 @@ +// check-pass +// aux-build:option_future.rs +// +// Check that if we promise to not impl what would overlap it doesn't actually overlap + +#![feature(rustc_attrs)] + +extern crate option_future as lib; +use lib::Future; + +trait Termination {} + +#[rustc_with_negative_coherence] +impl Termination for Option where E: Sized {} +#[rustc_with_negative_coherence] +impl Termination for F where F: Future + Sized {} + +fn main() {} From 1ec962fb349f0d4a010af81e7ae0d53fec4e2bb2 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 15:16:42 -0300 Subject: [PATCH 16/27] Do not pass OverlapMode down, just create a closure to properly set the filtering --- .../src/traits/coherence.rs | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 42b7139c00698..f6101d78f4139 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -201,17 +201,17 @@ fn overlap_within_probe<'cx, 'tcx>( match overlap_mode(tcx, impl1_def_id, impl2_def_id) { OverlapMode::Stable => { - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Stable) { + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header) { return None; } } OverlapMode::Strict => { - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Strict) { + if strict_disjoint(selcx, param_env, &impl1_header, impl2_header) { return None; } } OverlapMode::WithNegative => { - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header, OverlapMode::Stable) + if stable_disjoint(selcx, param_env, &impl1_header, impl2_header) || explicit_disjoint(selcx, impl1_def_id, impl2_def_id) || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) { @@ -244,7 +244,35 @@ fn stable_disjoint<'cx, 'tcx>( param_env: ty::ParamEnv<'tcx>, impl1_header: &ty::ImplHeader<'tcx>, impl2_header: ty::ImplHeader<'tcx>, - overlap_mode: OverlapMode, +) -> bool { + let infcx = selcx.infcx(); + let tcx = infcx.tcx; + + disjoint_with_filter(selcx, param_env, impl1_header, impl2_header, |selcx, o| { + loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) + }) +} + +fn strict_disjoint<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + param_env: ty::ParamEnv<'tcx>, + impl1_header: &ty::ImplHeader<'tcx>, + impl2_header: ty::ImplHeader<'tcx>, +) -> bool { + disjoint_with_filter(selcx, param_env, impl1_header, impl2_header, |selcx, o| { + strict_check(selcx, o) + }) +} + +fn disjoint_with_filter<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + param_env: ty::ParamEnv<'tcx>, + impl1_header: &ty::ImplHeader<'tcx>, + impl2_header: ty::ImplHeader<'tcx>, + mut filter: impl FnMut( + &mut SelectionContext<'cx, 'tcx>, + &rustc_infer::traits::Obligation<'tcx, rustc_middle::ty::Predicate<'tcx>>, + ) -> bool, ) -> bool { debug!("overlap: impl1_header={:?}", impl1_header); debug!("overlap: impl2_header={:?}", impl2_header); @@ -285,7 +313,6 @@ fn stable_disjoint<'cx, 'tcx>( // hold we need to check if `&'?a str: !Error` holds, if doesn't hold there's overlap because // at some point an impl for `&'?a str: Error` could be added. let infcx = selcx.infcx(); - let tcx = infcx.tcx; let opt_failing_obligation = impl1_header .predicates .iter() @@ -299,17 +326,7 @@ fn stable_disjoint<'cx, 'tcx>( predicate: p, }) .chain(obligations) - .find(|o| { - // if both impl headers are set to strict coherence it means that this will be accepted - // only if it's stated that T: !Trait. So only prove that the negated obligation holds. - match overlap_mode { - OverlapMode::Stable => { - loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) - } - OverlapMode::Strict => strict_check(selcx, o), - OverlapMode::WithNegative => loose_check(selcx, o), - } - }); + .find(|o| filter(selcx, o)); // FIXME: the call to `selcx.predicate_may_hold_fatal` above should be ported // to the canonical trait query form, `infcx.predicate_may_hold`, once // the new system supports intercrate mode (which coherence needs). From 19e3c860037a8bb01157aa9da291003c8b69e18c Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 15:26:05 -0300 Subject: [PATCH 17/27] Make strict_disjoint use explicit_disjoint --- .../src/traits/coherence.rs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index f6101d78f4139..178cff9501d73 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -206,14 +206,19 @@ fn overlap_within_probe<'cx, 'tcx>( } } OverlapMode::Strict => { - if strict_disjoint(selcx, param_env, &impl1_header, impl2_header) { + if strict_disjoint(selcx, impl1_def_id, impl2_def_id) { return None; } + + // Equate for error reporting + let _ = selcx + .infcx() + .at(&ObligationCause::dummy(), param_env) + .eq_impl_headers(&impl1_header, &impl2_header); } OverlapMode::WithNegative => { if stable_disjoint(selcx, param_env, &impl1_header, impl2_header) - || explicit_disjoint(selcx, impl1_def_id, impl2_def_id) - || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) + || strict_disjoint(selcx, impl1_def_id, impl2_def_id) { return None; } @@ -255,13 +260,11 @@ fn stable_disjoint<'cx, 'tcx>( fn strict_disjoint<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, - param_env: ty::ParamEnv<'tcx>, - impl1_header: &ty::ImplHeader<'tcx>, - impl2_header: ty::ImplHeader<'tcx>, + impl1_def_id: DefId, + impl2_def_id: DefId, ) -> bool { - disjoint_with_filter(selcx, param_env, impl1_header, impl2_header, |selcx, o| { - strict_check(selcx, o) - }) + explicit_disjoint(selcx, impl1_def_id, impl2_def_id) + || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) } fn disjoint_with_filter<'cx, 'tcx>( From e2567b034dd6847cc026eb80db47bd1271fe71de Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 21 Jan 2022 15:30:10 -0300 Subject: [PATCH 18/27] Remove intermediate function doesn't make more sense --- .../src/traits/coherence.rs | 39 ++++++------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 178cff9501d73..4f3b08bd3add4 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -249,33 +249,6 @@ fn stable_disjoint<'cx, 'tcx>( param_env: ty::ParamEnv<'tcx>, impl1_header: &ty::ImplHeader<'tcx>, impl2_header: ty::ImplHeader<'tcx>, -) -> bool { - let infcx = selcx.infcx(); - let tcx = infcx.tcx; - - disjoint_with_filter(selcx, param_env, impl1_header, impl2_header, |selcx, o| { - loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o) - }) -} - -fn strict_disjoint<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, - impl1_def_id: DefId, - impl2_def_id: DefId, -) -> bool { - explicit_disjoint(selcx, impl1_def_id, impl2_def_id) - || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) -} - -fn disjoint_with_filter<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, - param_env: ty::ParamEnv<'tcx>, - impl1_header: &ty::ImplHeader<'tcx>, - impl2_header: ty::ImplHeader<'tcx>, - mut filter: impl FnMut( - &mut SelectionContext<'cx, 'tcx>, - &rustc_infer::traits::Obligation<'tcx, rustc_middle::ty::Predicate<'tcx>>, - ) -> bool, ) -> bool { debug!("overlap: impl1_header={:?}", impl1_header); debug!("overlap: impl2_header={:?}", impl2_header); @@ -316,6 +289,7 @@ fn disjoint_with_filter<'cx, 'tcx>( // hold we need to check if `&'?a str: !Error` holds, if doesn't hold there's overlap because // at some point an impl for `&'?a str: Error` could be added. let infcx = selcx.infcx(); + let tcx = infcx.tcx; let opt_failing_obligation = impl1_header .predicates .iter() @@ -329,7 +303,7 @@ fn disjoint_with_filter<'cx, 'tcx>( predicate: p, }) .chain(obligations) - .find(|o| filter(selcx, o)); + .find(|o| loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o)); // FIXME: the call to `selcx.predicate_may_hold_fatal` above should be ported // to the canonical trait query form, `infcx.predicate_may_hold`, once // the new system supports intercrate mode (which coherence needs). @@ -344,6 +318,15 @@ fn disjoint_with_filter<'cx, 'tcx>( /// Given impl1 and impl2 check if both impls are never satisfied by a common type (including /// where-clauses) If so, return true, they are disjoint and false otherwise. +fn strict_disjoint<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + impl1_def_id: DefId, + impl2_def_id: DefId, +) -> bool { + explicit_disjoint(selcx, impl1_def_id, impl2_def_id) + || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) +} + fn explicit_disjoint<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, impl1_def_id: DefId, From 762bdbfce72e2c3b3f81f457f77cd12736ba8a7f Mon Sep 17 00:00:00 2001 From: Jack Huey <31162821+jackh726@users.noreply.github.com> Date: Fri, 24 Dec 2021 22:57:31 -0500 Subject: [PATCH 19/27] Change signature of point_at_arg_instead_of_call_if_possible --- .../rustc_typeck/src/check/fn_ctxt/checks.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs index e42d94a6f403b..820a4054cb494 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs @@ -315,13 +315,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { assert_eq!(expected_input_tys.len(), formal_input_tys.len()); + let provided_arg_count: usize = provided_args.len(); + // Keep track of the fully coerced argument types - let mut final_arg_types: Vec<(usize, Ty<'_>, Ty<'_>)> = vec![]; + let mut final_arg_types: Vec, Ty<'_>)>> = vec![None; provided_arg_count]; // We introduce a helper function to demand that a given argument satisfy a given input // This is more complicated than just checking type equality, as arguments could be coerced // This version writes those types back so further type checking uses the narrowed types - let demand_compatible = |idx, final_arg_types: &mut Vec<(usize, Ty<'tcx>, Ty<'tcx>)>| { + let demand_compatible = |idx, final_arg_types: &mut Vec, Ty<'tcx>)>>| { let formal_input_ty: Ty<'tcx> = formal_input_tys[idx]; let expected_input_ty: Ty<'tcx> = expected_input_tys[idx]; let provided_arg = &provided_args[idx]; @@ -340,7 +342,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let coerced_ty = expectation.only_has_type(self).unwrap_or(formal_input_ty); // Keep track of these for below - final_arg_types.push((idx, checked_ty, coerced_ty)); + final_arg_types[idx] = Some((checked_ty, coerced_ty)); // Cause selection errors caused by resolving a single argument to point at the // argument and not the call. This is otherwise redundant with the `demand_coerce` @@ -975,7 +977,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { fn point_at_arg_instead_of_call_if_possible( &self, errors: &mut Vec>, - final_arg_types: &[(usize, Ty<'tcx>, Ty<'tcx>)], + final_arg_types: &[Option<(Ty<'tcx>, Ty<'tcx>)>], expr: &'tcx hir::Expr<'tcx>, call_sp: Span, args: &'tcx [hir::Expr<'tcx>], @@ -1030,8 +1032,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // `FulfillmentError`. let mut referenced_in = final_arg_types .iter() - .map(|&(i, checked_ty, _)| (i, checked_ty)) - .chain(final_arg_types.iter().map(|&(i, _, coerced_ty)| (i, coerced_ty))) + .enumerate() + .filter_map(|(i, arg)| match arg { + Some((checked_ty, coerce_ty)) => Some([(i, *checked_ty), (i, *coerce_ty)]), + _ => None, + }) + .flatten() .flat_map(|(i, ty)| { let ty = self.resolve_vars_if_possible(ty); // We walk the argument type because the argument's type could have From ce31f68f9e5886959350a85aaad92bc4374c81ab Mon Sep 17 00:00:00 2001 From: Jack Huey <31162821+jackh726@users.noreply.github.com> Date: Thu, 20 Jan 2022 10:18:23 -0500 Subject: [PATCH 20/27] Move param count error emission to near end of check_argument_types --- .../rustc_typeck/src/check/fn_ctxt/checks.rs | 261 +++++++++--------- .../mismatched_types/overloaded-calls-bad.rs | 1 + .../overloaded-calls-bad.stderr | 8 +- 3 files changed, 140 insertions(+), 130 deletions(-) diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs index 820a4054cb494..c39199f84b527 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs @@ -127,136 +127,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let expected_arg_count = formal_input_tys.len(); - let param_count_error = |expected_count: usize, - arg_count: usize, - error_code: &str, - c_variadic: bool, - sugg_unit: bool| { - let (span, start_span, args, ctor_of) = match &call_expr.kind { - hir::ExprKind::Call( - hir::Expr { - span, - kind: - hir::ExprKind::Path(hir::QPath::Resolved( - _, - hir::Path { res: Res::Def(DefKind::Ctor(of, _), _), .. }, - )), - .. - }, - args, - ) => (*span, *span, &args[..], Some(of)), - hir::ExprKind::Call(hir::Expr { span, .. }, args) => { - (*span, *span, &args[..], None) - } - hir::ExprKind::MethodCall(path_segment, args, _) => ( - path_segment.ident.span, - // `sp` doesn't point at the whole `foo.bar()`, only at `bar`. - path_segment - .args - .and_then(|args| args.args.iter().last()) - // Account for `foo.bar::()`. - .map(|arg| { - // Skip the closing `>`. - tcx.sess - .source_map() - .next_point(tcx.sess.source_map().next_point(arg.span())) - }) - .unwrap_or(path_segment.ident.span), - &args[1..], // Skip the receiver. - None, // methods are never ctors - ), - k => span_bug!(call_span, "checking argument types on a non-call: `{:?}`", k), - }; - let arg_spans = if provided_args.is_empty() { - // foo() - // ^^^-- supplied 0 arguments - // | - // expected 2 arguments - vec![tcx.sess.source_map().next_point(start_span).with_hi(call_span.hi())] - } else { - // foo(1, 2, 3) - // ^^^ - - - supplied 3 arguments - // | - // expected 2 arguments - args.iter().map(|arg| arg.span).collect::>() - }; - - let mut err = tcx.sess.struct_span_err_with_code( - span, - &format!( - "this {} takes {}{} but {} {} supplied", - match ctor_of { - Some(CtorOf::Struct) => "struct", - Some(CtorOf::Variant) => "enum variant", - None => "function", - }, - if c_variadic { "at least " } else { "" }, - potentially_plural_count(expected_count, "argument"), - potentially_plural_count(arg_count, "argument"), - if arg_count == 1 { "was" } else { "were" } - ), - DiagnosticId::Error(error_code.to_owned()), - ); - let label = format!("supplied {}", potentially_plural_count(arg_count, "argument")); - for (i, span) in arg_spans.into_iter().enumerate() { - err.span_label( - span, - if arg_count == 0 || i + 1 == arg_count { &label } else { "" }, - ); - } - - if let Some(def_id) = fn_def_id { - if let Some(def_span) = tcx.def_ident_span(def_id) { - let mut spans: MultiSpan = def_span.into(); - - let params = tcx - .hir() - .get_if_local(def_id) - .and_then(|node| node.body_id()) - .into_iter() - .map(|id| tcx.hir().body(id).params) - .flatten(); - - for param in params { - spans.push_span_label(param.span, String::new()); - } - - let def_kind = tcx.def_kind(def_id); - err.span_note(spans, &format!("{} defined here", def_kind.descr(def_id))); - } - } - - if sugg_unit { - let sugg_span = tcx.sess.source_map().end_point(call_expr.span); - // remove closing `)` from the span - let sugg_span = sugg_span.shrink_to_lo(); - err.span_suggestion( - sugg_span, - "expected the unit value `()`; create it with empty parentheses", - String::from("()"), - Applicability::MachineApplicable, - ); - } else { - err.span_label( - span, - format!( - "expected {}{}", - if c_variadic { "at least " } else { "" }, - potentially_plural_count(expected_count, "argument") - ), - ); - } - err.emit(); - }; + // expected_count, arg_count, error_code, sugg_unit + let mut error: Option<(usize, usize, &str, bool)> = None; + // If the arguments should be wrapped in a tuple (ex: closures), unwrap them here let (formal_input_tys, expected_input_tys) = if tuple_arguments == TupleArguments { let tuple_type = self.structurally_resolved_type(call_span, formal_input_tys[0]); match tuple_type.kind() { - ty::Tuple(arg_types) if arg_types.len() != provided_args.len() => { - param_count_error(arg_types.len(), provided_args.len(), "E0057", false, false); - (self.err_args(provided_args.len()), vec![]) - } + // We expected a tuple and got a tuple ty::Tuple(arg_types) => { + // Argument length differs + if arg_types.len() != provided_args.len() { + error = Some((arg_types.len(), provided_args.len(), "E0057", false)); + } let expected_input_tys = match expected_input_tys.get(0) { Some(&ty) => match ty.kind() { ty::Tuple(ref tys) => tys.iter().map(|k| k.expect_ty()).collect(), @@ -267,6 +150,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { (arg_types.iter().map(|k| k.expect_ty()).collect(), expected_input_tys) } _ => { + // Otherwise, there's a mismatch, so clear out what we're expecting, and set + // our input typs to err_args so we don't blow up the error messages struct_span_err!( tcx.sess, call_span, @@ -284,7 +169,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if supplied_arg_count >= expected_arg_count { (formal_input_tys.to_vec(), expected_input_tys) } else { - param_count_error(expected_arg_count, supplied_arg_count, "E0060", true, false); + error = Some((expected_arg_count, supplied_arg_count, "E0060", false)); (self.err_args(supplied_arg_count), vec![]) } } else { @@ -296,8 +181,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } else { false }; - param_count_error(expected_arg_count, supplied_arg_count, "E0061", false, sugg_unit); - + error = Some((expected_arg_count, supplied_arg_count, "E0061", sugg_unit)); (self.err_args(supplied_arg_count), vec![]) }; @@ -348,7 +232,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // argument and not the call. This is otherwise redundant with the `demand_coerce` // call immediately after, but it lets us customize the span pointed to in the // fulfillment error to be more accurate. - let _ = + let coerced_ty = self.resolve_vars_with_obligations_and_mutate_fulfillment(coerced_ty, |errors| { self.point_at_type_arg_instead_of_call_if_possible(errors, call_expr); self.point_at_arg_instead_of_call_if_possible( @@ -360,6 +244,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); }); + final_arg_types[idx] = Some((checked_ty, coerced_ty)); + // We're processing function arguments so we definitely want to use // two-phase borrows. self.demand_coerce(&provided_arg, checked_ty, coerced_ty, None, AllowTwoPhase::Yes); @@ -418,6 +304,123 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } + // If there was an error in parameter count, emit that here + if let Some((expected_count, arg_count, err_code, sugg_unit)) = error { + let (span, start_span, args, ctor_of) = match &call_expr.kind { + hir::ExprKind::Call( + hir::Expr { + span, + kind: + hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: Res::Def(DefKind::Ctor(of, _), _), .. }, + )), + .. + }, + args, + ) => (*span, *span, &args[..], Some(of)), + hir::ExprKind::Call(hir::Expr { span, .. }, args) => { + (*span, *span, &args[..], None) + } + hir::ExprKind::MethodCall(path_segment, args, _) => ( + path_segment.ident.span, + // `sp` doesn't point at the whole `foo.bar()`, only at `bar`. + path_segment + .args + .and_then(|args| args.args.iter().last()) + // Account for `foo.bar::()`. + .map(|arg| { + // Skip the closing `>`. + tcx.sess + .source_map() + .next_point(tcx.sess.source_map().next_point(arg.span())) + }) + .unwrap_or(path_segment.ident.span), + &args[1..], // Skip the receiver. + None, // methods are never ctors + ), + k => span_bug!(call_span, "checking argument types on a non-call: `{:?}`", k), + }; + let arg_spans = if provided_args.is_empty() { + // foo() + // ^^^-- supplied 0 arguments + // | + // expected 2 arguments + vec![tcx.sess.source_map().next_point(start_span).with_hi(call_span.hi())] + } else { + // foo(1, 2, 3) + // ^^^ - - - supplied 3 arguments + // | + // expected 2 arguments + args.iter().map(|arg| arg.span).collect::>() + }; + let call_name = match ctor_of { + Some(CtorOf::Struct) => "struct", + Some(CtorOf::Variant) => "enum variant", + None => "function", + }; + let mut err = tcx.sess.struct_span_err_with_code( + span, + &format!( + "this {} takes {}{} but {} {} supplied", + call_name, + if c_variadic { "at least " } else { "" }, + potentially_plural_count(expected_count, "argument"), + potentially_plural_count(arg_count, "argument"), + if arg_count == 1 { "was" } else { "were" } + ), + DiagnosticId::Error(err_code.to_owned()), + ); + let label = format!("supplied {}", potentially_plural_count(arg_count, "argument")); + for (i, span) in arg_spans.into_iter().enumerate() { + err.span_label( + span, + if arg_count == 0 || i + 1 == arg_count { &label } else { "" }, + ); + } + if let Some(def_id) = fn_def_id { + if let Some(def_span) = tcx.def_ident_span(def_id) { + let mut spans: MultiSpan = def_span.into(); + + let params = tcx + .hir() + .get_if_local(def_id) + .and_then(|node| node.body_id()) + .into_iter() + .map(|id| tcx.hir().body(id).params) + .flatten(); + + for param in params { + spans.push_span_label(param.span, String::new()); + } + + let def_kind = tcx.def_kind(def_id); + err.span_note(spans, &format!("{} defined here", def_kind.descr(def_id))); + } + } + if sugg_unit { + let sugg_span = tcx.sess.source_map().end_point(call_expr.span); + // remove closing `)` from the span + let sugg_span = sugg_span.shrink_to_lo(); + err.span_suggestion( + sugg_span, + "expected the unit value `()`; create it with empty parentheses", + String::from("()"), + Applicability::MachineApplicable, + ); + } else { + err.span_label( + span, + format!( + "expected {}{}", + if c_variadic { "at least " } else { "" }, + potentially_plural_count(expected_count, "argument") + ), + ); + } + err.emit(); + } + // We also need to make sure we at least write the ty of the other // arguments which we skipped above. if c_variadic { diff --git a/src/test/ui/mismatched_types/overloaded-calls-bad.rs b/src/test/ui/mismatched_types/overloaded-calls-bad.rs index 902a6ec81d60b..d62625faaaa08 100644 --- a/src/test/ui/mismatched_types/overloaded-calls-bad.rs +++ b/src/test/ui/mismatched_types/overloaded-calls-bad.rs @@ -30,4 +30,5 @@ fn main() { //~^ ERROR this function takes 1 argument but 0 arguments were supplied let ans = s("burma", "shave"); //~^ ERROR this function takes 1 argument but 2 arguments were supplied + //~| ERROR mismatched types } diff --git a/src/test/ui/mismatched_types/overloaded-calls-bad.stderr b/src/test/ui/mismatched_types/overloaded-calls-bad.stderr index 264d7cbb9b1cb..9ae9c474162d9 100644 --- a/src/test/ui/mismatched_types/overloaded-calls-bad.stderr +++ b/src/test/ui/mismatched_types/overloaded-calls-bad.stderr @@ -18,6 +18,12 @@ note: associated function defined here LL | extern "rust-call" fn call_mut(&mut self, args: Args) -> Self::Output; | ^^^^^^^^ +error[E0308]: mismatched types + --> $DIR/overloaded-calls-bad.rs:31:17 + | +LL | let ans = s("burma", "shave"); + | ^^^^^^^ expected `isize`, found `&str` + error[E0057]: this function takes 1 argument but 2 arguments were supplied --> $DIR/overloaded-calls-bad.rs:31:15 | @@ -32,7 +38,7 @@ note: associated function defined here LL | extern "rust-call" fn call_mut(&mut self, args: Args) -> Self::Output; | ^^^^^^^^ -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors Some errors have detailed explanations: E0057, E0308. For more information about an error, try `rustc --explain E0057`. From e5f2fdb53939dc01ea6cad6e369c36aaa96f6e57 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 22 Jan 2022 18:16:11 -0300 Subject: [PATCH 21/27] Restructure the code leveraging in abilities more than modes --- .../src/traits/coherence.rs | 98 +++++++++---------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 4f3b08bd3add4..f22ca11d1c36e 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -11,7 +11,7 @@ use crate::traits::util::impl_trait_ref_and_oblig; use crate::traits::SkipLeakCheck; use crate::traits::{ self, FulfillmentContext, Normalized, Obligation, ObligationCause, PredicateObligation, - SelectionContext, + PredicateObligations, SelectionContext, }; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences}; @@ -137,12 +137,23 @@ fn with_fresh_ty_vars<'cx, 'tcx>( header } +#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] enum OverlapMode { Stable, WithNegative, Strict, } +impl OverlapMode { + fn use_negative_impl(&self) -> bool { + *self == OverlapMode::Strict || *self == OverlapMode::WithNegative + } + + fn use_implicit_negative(&self) -> bool { + *self == OverlapMode::Stable || *self == OverlapMode::WithNegative + } +} + fn overlap_mode<'tcx>(tcx: TyCtxt<'tcx>, impl1_def_id: DefId, impl2_def_id: DefId) -> OverlapMode { if tcx.has_attr(impl1_def_id, sym::rustc_strict_coherence) != tcx.has_attr(impl2_def_id, sym::rustc_strict_coherence) @@ -190,6 +201,16 @@ fn overlap_within_probe<'cx, 'tcx>( let infcx = selcx.infcx(); let tcx = infcx.tcx; + let overlap_mode = overlap_mode(tcx, impl1_def_id, impl2_def_id); + + if overlap_mode.use_negative_impl() { + if negative_impl(selcx, impl1_def_id, impl2_def_id) + || negative_impl(selcx, impl2_def_id, impl1_def_id) + { + return None; + } + } + // For the purposes of this check, we don't bring any placeholder // types into scope; instead, we replace the generic types with // fresh type variables, and hence we do our evaluations in an @@ -199,29 +220,15 @@ fn overlap_within_probe<'cx, 'tcx>( let impl1_header = with_fresh_ty_vars(selcx, param_env, impl1_def_id); let impl2_header = with_fresh_ty_vars(selcx, param_env, impl2_def_id); - match overlap_mode(tcx, impl1_def_id, impl2_def_id) { - OverlapMode::Stable => { - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header) { - return None; - } - } - OverlapMode::Strict => { - if strict_disjoint(selcx, impl1_def_id, impl2_def_id) { - return None; - } + debug!("overlap: impl1_header={:?}", impl1_header); + debug!("overlap: impl2_header={:?}", impl2_header); - // Equate for error reporting - let _ = selcx - .infcx() - .at(&ObligationCause::dummy(), param_env) - .eq_impl_headers(&impl1_header, &impl2_header); - } - OverlapMode::WithNegative => { - if stable_disjoint(selcx, param_env, &impl1_header, impl2_header) - || strict_disjoint(selcx, impl1_def_id, impl2_def_id) - { - return None; - } + let obligations = equate_impl_headers(selcx, &impl1_header, &impl2_header)?; + debug!("overlap: unification check succeeded"); + + if overlap_mode.use_implicit_negative() { + if implicit_negative(selcx, param_env, &impl1_header, impl2_header, obligations) { + return None; } } @@ -242,31 +249,29 @@ fn overlap_within_probe<'cx, 'tcx>( Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder }) } +fn equate_impl_headers<'cx, 'tcx>( + selcx: &mut SelectionContext<'cx, 'tcx>, + impl1_header: &ty::ImplHeader<'tcx>, + impl2_header: &ty::ImplHeader<'tcx>, +) -> Option> { + // Do `a` and `b` unify? If not, no overlap. + selcx + .infcx() + .at(&ObligationCause::dummy(), ty::ParamEnv::empty()) + .eq_impl_headers(impl1_header, impl2_header) + .map(|infer_ok| infer_ok.obligations) + .ok() +} + /// Given impl1 and impl2 check if both impls can be satisfied by a common type (including /// where-clauses) If so, return false, otherwise return true, they are disjoint. -fn stable_disjoint<'cx, 'tcx>( +fn implicit_negative<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, param_env: ty::ParamEnv<'tcx>, impl1_header: &ty::ImplHeader<'tcx>, impl2_header: ty::ImplHeader<'tcx>, + obligations: PredicateObligations<'tcx>, ) -> bool { - debug!("overlap: impl1_header={:?}", impl1_header); - debug!("overlap: impl2_header={:?}", impl2_header); - - // Do `a` and `b` unify? If not, no overlap. - let obligations = match selcx - .infcx() - .at(&ObligationCause::dummy(), param_env) - .eq_impl_headers(&impl1_header, &impl2_header) - { - Ok(InferOk { obligations, value: () }) => obligations, - Err(_) => { - return true; - } - }; - - debug!("overlap: unification check succeeded"); - // There's no overlap if obligations are unsatisfiable or if the obligation negated is // satisfied. // @@ -318,16 +323,7 @@ fn stable_disjoint<'cx, 'tcx>( /// Given impl1 and impl2 check if both impls are never satisfied by a common type (including /// where-clauses) If so, return true, they are disjoint and false otherwise. -fn strict_disjoint<'cx, 'tcx>( - selcx: &mut SelectionContext<'cx, 'tcx>, - impl1_def_id: DefId, - impl2_def_id: DefId, -) -> bool { - explicit_disjoint(selcx, impl1_def_id, impl2_def_id) - || explicit_disjoint(selcx, impl2_def_id, impl1_def_id) -} - -fn explicit_disjoint<'cx, 'tcx>( +fn negative_impl<'cx, 'tcx>( selcx: &mut SelectionContext<'cx, 'tcx>, impl1_def_id: DefId, impl2_def_id: DefId, From 92206318bc0ca743507367e492aaaa3c6660dbfb Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 22 Jan 2022 19:55:25 -0500 Subject: [PATCH 22/27] Add has tests for blanket_with_local trait methods --- src/test/rustdoc-json/impls/blanket_with_local.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/rustdoc-json/impls/blanket_with_local.rs b/src/test/rustdoc-json/impls/blanket_with_local.rs index e5f48820720fd..a3d55b35f0018 100644 --- a/src/test/rustdoc-json/impls/blanket_with_local.rs +++ b/src/test/rustdoc-json/impls/blanket_with_local.rs @@ -3,7 +3,9 @@ // @has blanket_with_local.json "$.index[*][?(@.name=='Load')]" pub trait Load { + // @has - "$.index[*][?(@.name=='load')]" fn load() {} + // @has - "$.index[*][?(@.name=='write')]" fn write(self) {} } From 7847ca8c61f7dc49775f237efae9c7da007d20af Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 22 Jan 2022 21:24:11 -0300 Subject: [PATCH 23/27] Document OverlapMode --- compiler/rustc_trait_selection/src/traits/coherence.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index f22ca11d1c36e..abe51b3fe302f 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -137,10 +137,15 @@ fn with_fresh_ty_vars<'cx, 'tcx>( header } +/// What kind of overlap check are we doing -- this exists just for testing and feature-gating +/// purposes. #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] enum OverlapMode { + /// The 1.0 rules (either types fail to unify, or where clauses are not implemented for crate-local types) Stable, + /// Feature-gated test: Stable, *or* there is an explicit negative impl that rules out one of the where-clauses. WithNegative, + /// Just check for negative impls, not for "where clause not implemented": used for testing. Strict, } From 269383226f8f11e21883590d40270bdd58339477 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 22 Jan 2022 21:25:56 -0300 Subject: [PATCH 24/27] Rename strict_check to negative_impl_exists --- .../rustc_trait_selection/src/traits/coherence.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index abe51b3fe302f..d1a966455f0b2 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -313,7 +313,9 @@ fn implicit_negative<'cx, 'tcx>( predicate: p, }) .chain(obligations) - .find(|o| loose_check(selcx, o) || tcx.features().negative_impls && strict_check(selcx, o)); + .find(|o| { + loose_check(selcx, o) || tcx.features().negative_impls && negative_impl_exists(selcx, o) + }); // FIXME: the call to `selcx.predicate_may_hold_fatal` above should be ported // to the canonical trait query form, `infcx.predicate_may_hold`, once // the new system supports intercrate mode (which coherence needs). @@ -377,8 +379,10 @@ fn negative_impl<'cx, 'tcx>( } }; - let opt_failing_obligation = - obligations.into_iter().chain(more_obligations).find(|o| strict_check(selcx, o)); + let opt_failing_obligation = obligations + .into_iter() + .chain(more_obligations) + .find(|o| negative_impl_exists(selcx, o)); if let Some(failing_obligation) = opt_failing_obligation { debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); @@ -396,7 +400,7 @@ fn loose_check<'cx, 'tcx>( !selcx.predicate_may_hold_fatal(o) } -fn strict_check<'cx, 'tcx>( +fn negative_impl_exists<'cx, 'tcx>( selcx: &SelectionContext<'cx, 'tcx>, o: &PredicateObligation<'tcx>, ) -> bool { From 8189bac9636930155768150886e28e221523d6ac Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 22 Jan 2022 21:28:12 -0300 Subject: [PATCH 25/27] FIXME include regions too --- compiler/rustc_trait_selection/src/traits/coherence.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index d1a966455f0b2..80ed9023d9694 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -408,7 +408,10 @@ fn negative_impl_exists<'cx, 'tcx>( let tcx = infcx.tcx; o.flip_polarity(tcx) .as_ref() - .map(|o| selcx.infcx().predicate_must_hold_modulo_regions(o)) + .map(|o| { + // FIXME This isn't quite correct, regions should be included + selcx.infcx().predicate_must_hold_modulo_regions(o) + }) .unwrap_or(false) } From 11b17c6c04c027f10dad1ed3c6af571d56bb0a94 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Sun, 23 Jan 2022 15:11:10 -0800 Subject: [PATCH 26/27] rustdoc settings: use radio buttons for theme This reduces the number of clicks required to change theme. Also, simplify the UI a bit (remove setting grouping), and add a "Back" link close to the settings icon. --- src/librustdoc/html/render/mod.rs | 75 ++++++++++----------- src/librustdoc/html/static/css/settings.css | 24 +++++++ src/librustdoc/html/static/js/settings.js | 29 +++++--- 3 files changed, 78 insertions(+), 50 deletions(-) diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index c3edefc469864..32e4a82918421 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -376,25 +376,21 @@ impl Setting { description, ), Setting::Select { js_data_name, description, default_value, ref options } => format!( - "

\ -
{}
\ - \ -
", - description, + "
{}{}
", js_data_name, + description, options .iter() .map(|opt| format!( - "", - if opt == default_value { "selected" } else { "" }, + "", + js_data_name = js_data_name, name = opt, + checked = if opt == default_value { "checked" } else { "" }, )) .collect::(), - root_path, - suffix, ), } } @@ -418,31 +414,25 @@ impl> From<(&'static str, Vec)> for Setting { fn settings(root_path: &str, suffix: &str, theme_names: Vec) -> Result { // (id, explanation, default value) let settings: &[Setting] = &[ - ( - "Theme preferences", - vec![ - Setting::from(("use-system-theme", "Use system theme", true)), - Setting::Select { - js_data_name: "theme", - description: "Theme", - default_value: "light", - options: theme_names.clone(), - }, - Setting::Select { - js_data_name: "preferred-dark-theme", - description: "Preferred dark theme", - default_value: "dark", - options: theme_names.clone(), - }, - Setting::Select { - js_data_name: "preferred-light-theme", - description: "Preferred light theme", - default_value: "light", - options: theme_names, - }, - ], - ) - .into(), + Setting::from(("use-system-theme", "Use system theme", true)), + Setting::Select { + js_data_name: "theme", + description: "Theme", + default_value: "light", + options: theme_names.clone(), + }, + Setting::Select { + js_data_name: "preferred-light-theme", + description: "Preferred light theme", + default_value: "light", + options: theme_names.clone(), + }, + Setting::Select { + js_data_name: "preferred-dark-theme", + description: "Preferred dark theme", + default_value: "dark", + options: theme_names, + }, ("auto-hide-large-items", "Auto-hide item contents for large items.", true).into(), ("auto-hide-method-docs", "Auto-hide item methods' documentation", false).into(), ("auto-hide-trait-implementations", "Auto-hide trait implementation documentation", false) @@ -454,9 +444,14 @@ fn settings(root_path: &str, suffix: &str, theme_names: Vec) -> Result\ - Rustdoc settings\ - \ + "
+

\ + Rustdoc settings\ +

\ + \ + Back\ + \ +
\
{}
\ \ ", diff --git a/src/librustdoc/html/static/css/settings.css b/src/librustdoc/html/static/css/settings.css index fb8990b30e2ed..932000487b0a8 100644 --- a/src/librustdoc/html/static/css/settings.css +++ b/src/librustdoc/html/static/css/settings.css @@ -17,6 +17,30 @@ border-bottom: 1px solid; } +.setting-line .radio-line { + display: flex; + flex-wrap: wrap; +} + +.setting-line .radio-line > * { + padding: 0.3em; +} + +.setting-line .radio-line .setting-name { + flex-grow: 1; +} + +.setting-line .radio-line input { + margin-right: 0.3em; +} + +.radio-line .choice { + border-radius: 0.1em; + border: 1px solid; + margin-left: 0.5em; + min-width: 3.5em; +} + .toggle { position: relative; display: inline-block; diff --git a/src/librustdoc/html/static/js/settings.js b/src/librustdoc/html/static/js/settings.js index e5c7e1ea1a03c..47a8fcdfd5ecf 100644 --- a/src/librustdoc/html/static/js/settings.js +++ b/src/librustdoc/html/static/js/settings.js @@ -33,19 +33,15 @@ } function showLightAndDark() { - addClass(document.getElementById("theme").parentElement.parentElement, "hidden"); - removeClass(document.getElementById("preferred-light-theme").parentElement.parentElement, - "hidden"); - removeClass(document.getElementById("preferred-dark-theme").parentElement.parentElement, - "hidden"); + addClass(document.getElementById("theme").parentElement, "hidden"); + removeClass(document.getElementById("preferred-light-theme").parentElement, "hidden"); + removeClass(document.getElementById("preferred-dark-theme").parentElement, "hidden"); } function hideLightAndDark() { - addClass(document.getElementById("preferred-light-theme").parentElement.parentElement, - "hidden"); - addClass(document.getElementById("preferred-dark-theme").parentElement.parentElement, - "hidden"); - removeClass(document.getElementById("theme").parentElement.parentElement, "hidden"); + addClass(document.getElementById("preferred-light-theme").parentElement, "hidden"); + addClass(document.getElementById("preferred-dark-theme").parentElement, "hidden"); + removeClass(document.getElementById("theme").parentElement, "hidden"); } function updateLightAndDark() { @@ -82,6 +78,19 @@ changeSetting(this.id, this.value); }; }); + onEachLazy(document.querySelectorAll("input[type=\"radio\"]"), function(elem) { + const settingId = elem.name; + const settingValue = getSettingValue(settingId); + if (settingValue !== null && settingValue !== "null") { + elem.checked = settingValue === elem.value; + } + elem.addEventListener("change", function(ev) { + changeSetting(ev.target.name, ev.target.value); + }); + }); + document.getElementById("back").addEventListener("click", function() { + history.back(); + }); } window.addEventListener("DOMContentLoaded", setEvents); From e02e9582d253ffaa04c9137f732efb0b80f98907 Mon Sep 17 00:00:00 2001 From: Jacob Bramley Date: Wed, 19 Jan 2022 14:51:59 +0000 Subject: [PATCH 27/27] Use error-on-mismatch policy for PAuth module flags. This agrees with Clang, and avoids an error when using LTO with mixed C/Rust. LLVM considers different behaviour flags to be a mismatch, even when the flag value itself is the same. This also makes the flag setting explicit for all uses of LLVMRustAddModuleFlag. --- compiler/rustc_codegen_llvm/src/context.rs | 31 ++++++++++++++----- .../rustc_codegen_llvm/src/debuginfo/mod.rs | 15 +++++++-- compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 31 ++++++++++++++++++- .../rustc_llvm/llvm-wrapper/RustWrapper.cpp | 9 ++++-- 4 files changed, 73 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/context.rs b/compiler/rustc_codegen_llvm/src/context.rs index bb16bc5dccdfc..8672459b5da3a 100644 --- a/compiler/rustc_codegen_llvm/src/context.rs +++ b/compiler/rustc_codegen_llvm/src/context.rs @@ -215,16 +215,19 @@ pub unsafe fn create_module<'ll>( // to ensure intrinsic calls don't use it. if !sess.needs_plt() { let avoid_plt = "RtLibUseGOT\0".as_ptr().cast(); - llvm::LLVMRustAddModuleFlag(llmod, avoid_plt, 1); + llvm::LLVMRustAddModuleFlag(llmod, llvm::LLVMModFlagBehavior::Warning, avoid_plt, 1); } if sess.is_sanitizer_cfi_enabled() { // FIXME(rcvalle): Add support for non canonical jump tables. let canonical_jump_tables = "CFI Canonical Jump Tables\0".as_ptr().cast(); - // FIXME(rcvalle): Add it with Override behavior flag--LLVMRustAddModuleFlag adds it with - // Warning behavior flag. Add support for specifying the behavior flag to - // LLVMRustAddModuleFlag. - llvm::LLVMRustAddModuleFlag(llmod, canonical_jump_tables, 1); + // FIXME(rcvalle): Add it with Override behavior flag. + llvm::LLVMRustAddModuleFlag( + llmod, + llvm::LLVMModFlagBehavior::Warning, + canonical_jump_tables, + 1, + ); } // Control Flow Guard is currently only supported by the MSVC linker on Windows. @@ -233,11 +236,21 @@ pub unsafe fn create_module<'ll>( CFGuard::Disabled => {} CFGuard::NoChecks => { // Set `cfguard=1` module flag to emit metadata only. - llvm::LLVMRustAddModuleFlag(llmod, "cfguard\0".as_ptr() as *const _, 1) + llvm::LLVMRustAddModuleFlag( + llmod, + llvm::LLVMModFlagBehavior::Warning, + "cfguard\0".as_ptr() as *const _, + 1, + ) } CFGuard::Checks => { // Set `cfguard=2` module flag to emit metadata and checks. - llvm::LLVMRustAddModuleFlag(llmod, "cfguard\0".as_ptr() as *const _, 2) + llvm::LLVMRustAddModuleFlag( + llmod, + llvm::LLVMModFlagBehavior::Warning, + "cfguard\0".as_ptr() as *const _, + 2, + ) } } } @@ -247,24 +260,28 @@ pub unsafe fn create_module<'ll>( llvm::LLVMRustAddModuleFlag( llmod, + llvm::LLVMModFlagBehavior::Error, "branch-target-enforcement\0".as_ptr().cast(), bti.into(), ); llvm::LLVMRustAddModuleFlag( llmod, + llvm::LLVMModFlagBehavior::Error, "sign-return-address\0".as_ptr().cast(), pac.is_some().into(), ); let pac_opts = pac.unwrap_or(PacRet { leaf: false, key: PAuthKey::A }); llvm::LLVMRustAddModuleFlag( llmod, + llvm::LLVMModFlagBehavior::Error, "sign-return-address-all\0".as_ptr().cast(), pac_opts.leaf.into(), ); let is_bkey = if pac_opts.key == PAuthKey::A { false } else { true }; llvm::LLVMRustAddModuleFlag( llmod, + llvm::LLVMModFlagBehavior::Error, "sign-return-address-with-bkey\0".as_ptr().cast(), is_bkey.into(), ); diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs index 61e49fab6ff88..28eb8e2a0a462 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs @@ -108,18 +108,29 @@ impl<'a, 'tcx> CrateDebugContext<'a, 'tcx> { // This can be overridden using --llvm-opts -dwarf-version,N. // Android has the same issue (#22398) if let Some(version) = sess.target.dwarf_version { - llvm::LLVMRustAddModuleFlag(self.llmod, "Dwarf Version\0".as_ptr().cast(), version) + llvm::LLVMRustAddModuleFlag( + self.llmod, + llvm::LLVMModFlagBehavior::Warning, + "Dwarf Version\0".as_ptr().cast(), + version, + ) } // Indicate that we want CodeView debug information on MSVC if sess.target.is_like_msvc { - llvm::LLVMRustAddModuleFlag(self.llmod, "CodeView\0".as_ptr().cast(), 1) + llvm::LLVMRustAddModuleFlag( + self.llmod, + llvm::LLVMModFlagBehavior::Warning, + "CodeView\0".as_ptr().cast(), + 1, + ) } // Prevent bitcode readers from deleting the debug info. let ptr = "Debug Info Version\0".as_ptr(); llvm::LLVMRustAddModuleFlag( self.llmod, + llvm::LLVMModFlagBehavior::Warning, ptr.cast(), llvm::LLVMRustDebugMetadataVersion(), ); diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index a1c7d2b4f6156..2b10218879038 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -61,6 +61,26 @@ pub enum LLVMMachineType { ARM = 0x01c0, } +/// LLVM's Module::ModFlagBehavior, defined in llvm/include/llvm/IR/Module.h. +/// +/// When merging modules (e.g. during LTO), their metadata flags are combined. Conflicts are +/// resolved according to the merge behaviors specified here. Flags differing only in merge +/// behavior are still considered to be in conflict. +/// +/// In order for Rust-C LTO to work, we must specify behaviors compatible with Clang. Notably, +/// 'Error' and 'Warning' cannot be mixed for a given flag. +#[derive(Copy, Clone, PartialEq)] +#[repr(C)] +pub enum LLVMModFlagBehavior { + Error = 1, + Warning = 2, + Require = 3, + Override = 4, + Append = 5, + AppendUnique = 6, + Max = 7, +} + // Consts for the LLVM CallConv type, pre-cast to usize. /// LLVM CallingConv::ID. Should we wrap this? @@ -1895,7 +1915,16 @@ extern "C" { pub fn LLVMRustIsRustLLVM() -> bool; - pub fn LLVMRustAddModuleFlag(M: &Module, name: *const c_char, value: u32); + /// Add LLVM module flags. + /// + /// In order for Rust-C LTO to work, module flags must be compatible with Clang. What + /// "compatible" means depends on the merge behaviors involved. + pub fn LLVMRustAddModuleFlag( + M: &Module, + merge_behavior: LLVMModFlagBehavior, + name: *const c_char, + value: u32, + ); pub fn LLVMRustMetadataAsValue<'a>(C: &'a Context, MD: &'a Metadata) -> &'a Value; diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index c21e4acbefec0..dcd6327c92f6a 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -722,9 +722,12 @@ extern "C" bool LLVMRustIsRustLLVM() { #endif } -extern "C" void LLVMRustAddModuleFlag(LLVMModuleRef M, const char *Name, - uint32_t Value) { - unwrap(M)->addModuleFlag(Module::Warning, Name, Value); +extern "C" void LLVMRustAddModuleFlag( + LLVMModuleRef M, + Module::ModFlagBehavior MergeBehavior, + const char *Name, + uint32_t Value) { + unwrap(M)->addModuleFlag(MergeBehavior, Name, Value); } extern "C" LLVMValueRef LLVMRustMetadataAsValue(LLVMContextRef C, LLVMMetadataRef MD) {