From c90b6b8d2937cc116c635aa5c58f90afcc535248 Mon Sep 17 00:00:00 2001 From: Skgland Date: Thu, 9 May 2024 15:41:15 +0200 Subject: [PATCH 01/21] stabilize `const_int_from_str` --- library/core/src/lib.rs | 1 - library/core/src/num/error.rs | 2 +- library/core/src/num/mod.rs | 6 ++++-- library/core/tests/lib.rs | 1 - tests/ui/consts/const-eval/parse_ints.rs | 2 -- tests/ui/consts/const-eval/parse_ints.stderr | 4 ++-- 6 files changed, 7 insertions(+), 9 deletions(-) diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs index c5a1fca667bc3..ef90333596fdb 100644 --- a/library/core/src/lib.rs +++ b/library/core/src/lib.rs @@ -128,7 +128,6 @@ #![feature(const_hash)] #![feature(const_heap)] #![feature(const_index_range_slice_index)] -#![feature(const_int_from_str)] #![feature(const_intrinsic_copy)] #![feature(const_intrinsic_forget)] #![feature(const_ipv4)] diff --git a/library/core/src/num/error.rs b/library/core/src/num/error.rs index a2d7e6f7b0754..b8e22a8aef955 100644 --- a/library/core/src/num/error.rs +++ b/library/core/src/num/error.rs @@ -113,7 +113,7 @@ pub enum IntErrorKind { impl ParseIntError { /// Outputs the detailed cause of parsing an integer failing. #[must_use] - #[rustc_const_unstable(feature = "const_int_from_str", issue = "59133")] + #[rustc_const_stable(feature = "const_int_from_str", since = "CURRENT_RUSTC_VERSION")] #[stable(feature = "int_error_matching", since = "1.55.0")] pub const fn kind(&self) -> &IntErrorKind { &self.kind diff --git a/library/core/src/num/mod.rs b/library/core/src/num/mod.rs index 034af6a0d5731..6010f7cee06eb 100644 --- a/library/core/src/num/mod.rs +++ b/library/core/src/num/mod.rs @@ -1387,6 +1387,7 @@ from_str_radix_int_impl! { isize i8 i16 i32 i64 i128 usize u8 u16 u32 u64 u128 } #[doc(hidden)] #[inline(always)] #[unstable(issue = "none", feature = "std_internals")] +#[rustc_const_unstable(issue = "none", feature = "const_int_cannot_overflow")] pub const fn can_not_overflow(radix: u32, is_signed_ty: bool, digits: &[u8]) -> bool { radix <= 16 && digits.len() <= mem::size_of::() * 2 - is_signed_ty as usize } @@ -1410,6 +1411,7 @@ const fn from_str_radix_panic(radix: u32) { intrinsics::const_eval_select((radix,), from_str_radix_panic_ct, from_str_radix_panic_rt); } +#[allow_internal_unstable(const_int_cannot_overflow)] macro_rules! from_str_radix { ($($int_ty:ty)+) => {$( impl $int_ty { @@ -1436,7 +1438,7 @@ macro_rules! from_str_radix { #[doc = concat!("assert_eq!(", stringify!($int_ty), "::from_str_radix(\"A\", 16), Ok(10));")] /// ``` #[stable(feature = "rust1", since = "1.0.0")] - #[rustc_const_unstable(feature = "const_int_from_str", issue = "59133")] + #[rustc_const_stable(feature = "const_int_from_str", since = "CURRENT_RUSTC_VERSION")] pub const fn from_str_radix(src: &str, radix: u32) -> Result<$int_ty, ParseIntError> { use self::IntErrorKind::*; use self::ParseIntError as PIE; @@ -1566,7 +1568,7 @@ macro_rules! from_str_radix_size_impl { #[doc = concat!("assert_eq!(", stringify!($size), "::from_str_radix(\"A\", 16), Ok(10));")] /// ``` #[stable(feature = "rust1", since = "1.0.0")] - #[rustc_const_unstable(feature = "const_int_from_str", issue = "59133")] + #[rustc_const_stable(feature = "const_int_from_str", since = "CURRENT_RUSTC_VERSION")] pub const fn from_str_radix(src: &str, radix: u32) -> Result<$size, ParseIntError> { match <$t>::from_str_radix(src, radix) { Ok(x) => Ok(x as $size), diff --git a/library/core/tests/lib.rs b/library/core/tests/lib.rs index 83a615fcd8be3..feef5e81480c4 100644 --- a/library/core/tests/lib.rs +++ b/library/core/tests/lib.rs @@ -16,7 +16,6 @@ #![feature(const_hash)] #![feature(const_heap)] #![feature(const_intrinsic_copy)] -#![feature(const_int_from_str)] #![feature(const_maybe_uninit_as_mut_ptr)] #![feature(const_nonnull_new)] #![feature(const_pointer_is_aligned)] diff --git a/tests/ui/consts/const-eval/parse_ints.rs b/tests/ui/consts/const-eval/parse_ints.rs index ff9fc47e65c33..cb9a3eb431299 100644 --- a/tests/ui/consts/const-eval/parse_ints.rs +++ b/tests/ui/consts/const-eval/parse_ints.rs @@ -1,5 +1,3 @@ -#![feature(const_int_from_str)] - const _OK: () = match i32::from_str_radix("-1234", 10) { Ok(x) => assert!(x == -1234), Err(_) => panic!(), diff --git a/tests/ui/consts/const-eval/parse_ints.stderr b/tests/ui/consts/const-eval/parse_ints.stderr index 9e49fe433a126..ec9249ece8e39 100644 --- a/tests/ui/consts/const-eval/parse_ints.stderr +++ b/tests/ui/consts/const-eval/parse_ints.stderr @@ -6,7 +6,7 @@ error[E0080]: evaluation of constant value failed note: inside `core::num::::from_str_radix` --> $SRC_DIR/core/src/num/mod.rs:LL:COL note: inside `_TOO_LOW` - --> $DIR/parse_ints.rs:7:24 + --> $DIR/parse_ints.rs:5:24 | LL | const _TOO_LOW: () = { u64::from_str_radix("12345ABCD", 1); }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -20,7 +20,7 @@ error[E0080]: evaluation of constant value failed note: inside `core::num::::from_str_radix` --> $SRC_DIR/core/src/num/mod.rs:LL:COL note: inside `_TOO_HIGH` - --> $DIR/parse_ints.rs:8:25 + --> $DIR/parse_ints.rs:6:25 | LL | const _TOO_HIGH: () = { u64::from_str_radix("12345ABCD", 37); }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From eb799cf634a811d1e0d719d30cba83d5611f87c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Thu, 4 Jul 2024 20:51:50 +0200 Subject: [PATCH 02/21] mark `can_not_overflow` as `#[rustc_const_stable(...)]` see https://github.com/rust-lang/rust/pull/124941#discussion_r1664676739 --- library/core/src/num/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/core/src/num/mod.rs b/library/core/src/num/mod.rs index 6010f7cee06eb..0522365e22e0a 100644 --- a/library/core/src/num/mod.rs +++ b/library/core/src/num/mod.rs @@ -1387,7 +1387,7 @@ from_str_radix_int_impl! { isize i8 i16 i32 i64 i128 usize u8 u16 u32 u64 u128 } #[doc(hidden)] #[inline(always)] #[unstable(issue = "none", feature = "std_internals")] -#[rustc_const_unstable(issue = "none", feature = "const_int_cannot_overflow")] +#[rustc_const_stable(feature = "const_int_from_str", since = "CURRENT_RUSTC_VERSION")] pub const fn can_not_overflow(radix: u32, is_signed_ty: bool, digits: &[u8]) -> bool { radix <= 16 && digits.len() <= mem::size_of::() * 2 - is_signed_ty as usize } @@ -1411,7 +1411,6 @@ const fn from_str_radix_panic(radix: u32) { intrinsics::const_eval_select((radix,), from_str_radix_panic_ct, from_str_radix_panic_rt); } -#[allow_internal_unstable(const_int_cannot_overflow)] macro_rules! from_str_radix { ($($int_ty:ty)+) => {$( impl $int_ty { From f99df29d9dcc1c313593967c8c390fae39d462d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Thu, 4 Jul 2024 21:23:55 +0200 Subject: [PATCH 03/21] fix tests after rebase --- src/tools/clippy/tests/ui/from_str_radix_10.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/clippy/tests/ui/from_str_radix_10.rs b/src/tools/clippy/tests/ui/from_str_radix_10.rs index 2d5b351f8da3e..0df6a0a202ae7 100644 --- a/src/tools/clippy/tests/ui/from_str_radix_10.rs +++ b/src/tools/clippy/tests/ui/from_str_radix_10.rs @@ -1,4 +1,3 @@ -#![feature(const_int_from_str)] #![warn(clippy::from_str_radix_10)] mod some_mod { @@ -61,7 +60,8 @@ fn main() -> Result<(), Box> { Ok(()) } -fn issue_12732() { +// https://github.com/rust-lang/rust-clippy/issues/12731 +fn issue_12731() { const A: Result = u32::from_str_radix("123", 10); const B: () = { let _ = u32::from_str_radix("123", 10); From 404519a6e5a2e95d0b8f057e35a7d6280fb9c562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Thu, 4 Jul 2024 22:31:53 +0200 Subject: [PATCH 04/21] bless tests --- .../clippy/tests/ui/from_str_radix_10.fixed | 4 ++-- .../clippy/tests/ui/from_str_radix_10.stderr | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/tools/clippy/tests/ui/from_str_radix_10.fixed b/src/tools/clippy/tests/ui/from_str_radix_10.fixed index f9ce1defda17c..6c582190b4424 100644 --- a/src/tools/clippy/tests/ui/from_str_radix_10.fixed +++ b/src/tools/clippy/tests/ui/from_str_radix_10.fixed @@ -1,4 +1,3 @@ -#![feature(const_int_from_str)] #![warn(clippy::from_str_radix_10)] mod some_mod { @@ -61,7 +60,8 @@ fn main() -> Result<(), Box> { Ok(()) } -fn issue_12732() { +// https://github.com/rust-lang/rust-clippy/issues/12731 +fn issue_12731() { const A: Result = u32::from_str_radix("123", 10); const B: () = { let _ = u32::from_str_radix("123", 10); diff --git a/src/tools/clippy/tests/ui/from_str_radix_10.stderr b/src/tools/clippy/tests/ui/from_str_radix_10.stderr index 01a1bf8940a12..4aa84eca26120 100644 --- a/src/tools/clippy/tests/ui/from_str_radix_10.stderr +++ b/src/tools/clippy/tests/ui/from_str_radix_10.stderr @@ -1,5 +1,5 @@ error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:29:5 + --> tests/ui/from_str_radix_10.rs:28:5 | LL | u32::from_str_radix("30", 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"30".parse::()` @@ -8,43 +8,43 @@ LL | u32::from_str_radix("30", 10)?; = help: to override `-D warnings` add `#[allow(clippy::from_str_radix_10)]` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:32:5 + --> tests/ui/from_str_radix_10.rs:31:5 | LL | i64::from_str_radix("24", 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"24".parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:34:5 + --> tests/ui/from_str_radix_10.rs:33:5 | LL | isize::from_str_radix("100", 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"100".parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:36:5 + --> tests/ui/from_str_radix_10.rs:35:5 | LL | u8::from_str_radix("7", 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"7".parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:38:5 + --> tests/ui/from_str_radix_10.rs:37:5 | LL | u16::from_str_radix(&("10".to_owned() + "5"), 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `("10".to_owned() + "5").parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:40:5 + --> tests/ui/from_str_radix_10.rs:39:5 | LL | i128::from_str_radix(Test + Test, 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(Test + Test).parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:44:5 + --> tests/ui/from_str_radix_10.rs:43:5 | LL | i32::from_str_radix(string, 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` - --> tests/ui/from_str_radix_10.rs:48:5 + --> tests/ui/from_str_radix_10.rs:47:5 | LL | i32::from_str_radix(&stringier, 10)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `stringier.parse::()` From 370fcce5646a55a15f2753bb3d8f249d065e192e Mon Sep 17 00:00:00 2001 From: binarycat Date: Thu, 25 Jul 2024 17:51:35 -0400 Subject: [PATCH 05/21] rustdoc: change title of search results the current title is too similar to that of the page for std::result::Result, which is a problem both for navigating to the Result docs via browser autocomplete, and for being able to tell which tab is which when the width of tabs is small. --- src/librustdoc/html/static/js/search.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js index 86af38f3ee75e..c6618c9859c79 100644 --- a/src/librustdoc/html/static/js/search.js +++ b/src/librustdoc/html/static/js/search.js @@ -2932,7 +2932,7 @@ ${item.displayPath}${name}\ } // Update document title to maintain a meaningful browser history - searchState.title = "Results for " + query.original + " - Rust"; + searchState.title = '"' + query.original + '" Search - Rust'; // Because searching is incremental by character, only the most // recent search query is added to the browser history. From 587b64e88b19d6e5ca0fed53f3538168df949ed5 Mon Sep 17 00:00:00 2001 From: binarycat Date: Thu, 25 Jul 2024 18:55:45 -0400 Subject: [PATCH 06/21] use double quotes --- src/librustdoc/html/static/js/search.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js index c6618c9859c79..3eb27ea087c1d 100644 --- a/src/librustdoc/html/static/js/search.js +++ b/src/librustdoc/html/static/js/search.js @@ -2932,7 +2932,7 @@ ${item.displayPath}${name}\ } // Update document title to maintain a meaningful browser history - searchState.title = '"' + query.original + '" Search - Rust'; + searchState.title = "\"" + query.original + "\" Search - Rust"; // Because searching is incremental by character, only the most // recent search query is added to the browser history. From caee195bdd922c25946276625993b93a5bc0eb41 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 08:42:43 +1000 Subject: [PATCH 07/21] Invert early exit conditions in `collect_tokens_trailing_token`. This has been bugging me for a while. I find complex "if any of these are true" conditions easier to think about than complex "if all of these are true" conditions, because you can stop as soon as one is true. --- .../rustc_parse/src/parser/attr_wrapper.rs | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index dc5f98f7be8b6..6413aa092414e 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -199,20 +199,20 @@ impl<'a> Parser<'a> { force_collect: ForceCollect, f: impl FnOnce(&mut Self, ast::AttrVec) -> PResult<'a, (R, bool)>, ) -> PResult<'a, R> { - // Skip collection when nothing could observe the collected tokens, i.e. - // all of the following conditions hold. - // - We are not force collecting tokens (because force collection - // requires tokens by definition). - if matches!(force_collect, ForceCollect::No) - // - None of our outer attributes require tokens. - && attrs.is_complete() - // - Our target doesn't support custom inner attributes (custom + // We must collect if anything could observe the collected tokens, i.e. + // if any of the following conditions hold. + // - We are force collecting tokens (because force collection requires + // tokens by definition). + let needs_collection = matches!(force_collect, ForceCollect::Yes) + // - Any of our outer attributes require tokens. + || !attrs.is_complete() + // - Our target supports custom inner attributes (custom // inner attribute invocation might require token capturing). - && !R::SUPPORTS_CUSTOM_INNER_ATTRS - // - We are not in `capture_cfg` mode (which requires tokens if + || R::SUPPORTS_CUSTOM_INNER_ATTRS + // - We are in `capture_cfg` mode (which requires tokens if // the parsed node has `#[cfg]` or `#[cfg_attr]` attributes). - && !self.capture_cfg - { + || self.capture_cfg; + if !needs_collection { return Ok(f(self, attrs.attrs)?.0); } @@ -250,28 +250,28 @@ impl<'a> Parser<'a> { return Ok(ret); } - // This is similar to the "skip collection" check at the start of this - // function, but now that we've parsed an AST node we have more + // This is similar to the `needs_collection` check at the start of this + // function, but now that we've parsed an AST node we have complete // information available. (If we return early here that means the // setup, such as cloning the token cursor, was unnecessary. That's // hard to avoid.) // - // Skip collection when nothing could observe the collected tokens, i.e. - // all of the following conditions hold. - // - We are not force collecting tokens. - if matches!(force_collect, ForceCollect::No) - // - None of our outer *or* inner attributes require tokens. + // We must collect if anything could observe the collected tokens, i.e. + // if any of the following conditions hold. + // - We are force collecting tokens. + let needs_collection = matches!(force_collect, ForceCollect::Yes) + // - Any of our outer *or* inner attributes require tokens. // (`attrs` was just outer attributes, but `ret.attrs()` is outer - // and inner attributes. That makes this check more precise than - // `attrs.is_complete()` at the start of the function, and we can - // skip the subsequent check on `R::SUPPORTS_CUSTOM_INNER_ATTRS`. - && crate::parser::attr::is_complete(ret.attrs()) - // - We are not in `capture_cfg` mode, or we are but there are no - // `#[cfg]` or `#[cfg_attr]` attributes. (During normal - // non-`capture_cfg` parsing, we don't need any special capturing - // for those attributes, because they're builtin.) - && (!self.capture_cfg || !has_cfg_or_cfg_attr(ret.attrs())) - { + // and inner attributes. So this check is more precise than the + // earlier `attrs.is_complete()` check, and we don't need to + // check `R::SUPPORTS_CUSTOM_INNER_ATTRS`.) + || !crate::parser::attr::is_complete(ret.attrs()) + // - We are in `capture_cfg` mode and there are `#[cfg]` or + // `#[cfg_attr]` attributes. (During normal non-`capture_cfg` + // parsing, we don't need any special capturing for those + // attributes, because they're builtin.) + || (self.capture_cfg && has_cfg_or_cfg_attr(ret.attrs())); + if !needs_collection { return Ok(ret); } From 4288edb219fb288af524b490bd3830fc7cfafe02 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 09:00:17 +1000 Subject: [PATCH 08/21] Inline and remove `AttrWrapper::is_complete`. It has a single call site. This change makes the two `needs_collect` conditions more similar to each other, and therefore easier to understand. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index 6413aa092414e..27b899300e7d9 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -60,10 +60,6 @@ impl AttrWrapper { pub fn is_empty(&self) -> bool { self.attrs.is_empty() } - - pub fn is_complete(&self) -> bool { - crate::parser::attr::is_complete(&self.attrs) - } } /// Returns `true` if `attrs` contains a `cfg` or `cfg_attr` attribute @@ -205,7 +201,7 @@ impl<'a> Parser<'a> { // tokens by definition). let needs_collection = matches!(force_collect, ForceCollect::Yes) // - Any of our outer attributes require tokens. - || !attrs.is_complete() + || !crate::parser::attr::is_complete(&attrs.attrs) // - Our target supports custom inner attributes (custom // inner attribute invocation might require token capturing). || R::SUPPORTS_CUSTOM_INNER_ATTRS @@ -261,9 +257,9 @@ impl<'a> Parser<'a> { // - We are force collecting tokens. let needs_collection = matches!(force_collect, ForceCollect::Yes) // - Any of our outer *or* inner attributes require tokens. - // (`attrs` was just outer attributes, but `ret.attrs()` is outer - // and inner attributes. So this check is more precise than the - // earlier `attrs.is_complete()` check, and we don't need to + // (`attr.attrs` was just outer attributes, but `ret.attrs()` is + // outer and inner attributes. So this check is more precise than + // the earlier `is_complete()` check, and we don't need to // check `R::SUPPORTS_CUSTOM_INNER_ATTRS`.) || !crate::parser::attr::is_complete(ret.attrs()) // - We are in `capture_cfg` mode and there are `#[cfg]` or From 3d363c3d99a5e27fa0b604aae4ca745b4cb4c43a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 09:03:30 +1000 Subject: [PATCH 09/21] Move `is_complete` to the module that uses it. And make it non-`pub`. --- compiler/rustc_parse/src/parser/attr.rs | 11 ----------- compiler/rustc_parse/src/parser/attr_wrapper.rs | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr.rs b/compiler/rustc_parse/src/parser/attr.rs index 0b2c304403941..535b53a836e98 100644 --- a/compiler/rustc_parse/src/parser/attr.rs +++ b/compiler/rustc_parse/src/parser/attr.rs @@ -457,14 +457,3 @@ impl<'a> Parser<'a> { Err(self.dcx().create_err(err)) } } - -/// The attributes are complete if all attributes are either a doc comment or a -/// builtin attribute other than `cfg_attr`. -pub fn is_complete(attrs: &[ast::Attribute]) -> bool { - attrs.iter().all(|attr| { - attr.is_doc_comment() - || attr.ident().is_some_and(|ident| { - ident.name != sym::cfg_attr && rustc_feature::is_builtin_attr_name(ident.name) - }) - }) -} diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index 27b899300e7d9..82fd2257481ed 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -201,7 +201,7 @@ impl<'a> Parser<'a> { // tokens by definition). let needs_collection = matches!(force_collect, ForceCollect::Yes) // - Any of our outer attributes require tokens. - || !crate::parser::attr::is_complete(&attrs.attrs) + || !is_complete(&attrs.attrs) // - Our target supports custom inner attributes (custom // inner attribute invocation might require token capturing). || R::SUPPORTS_CUSTOM_INNER_ATTRS @@ -261,7 +261,7 @@ impl<'a> Parser<'a> { // outer and inner attributes. So this check is more precise than // the earlier `is_complete()` check, and we don't need to // check `R::SUPPORTS_CUSTOM_INNER_ATTRS`.) - || !crate::parser::attr::is_complete(ret.attrs()) + || !is_complete(ret.attrs()) // - We are in `capture_cfg` mode and there are `#[cfg]` or // `#[cfg_attr]` attributes. (During normal non-`capture_cfg` // parsing, we don't need any special capturing for those @@ -457,6 +457,17 @@ fn make_attr_token_stream( AttrTokenStream::new(stack_top.inner) } +/// The attributes are complete if all attributes are either a doc comment or a +/// builtin attribute other than `cfg_attr`. +fn is_complete(attrs: &[ast::Attribute]) -> bool { + attrs.iter().all(|attr| { + attr.is_doc_comment() + || attr.ident().is_some_and(|ident| { + ident.name != sym::cfg_attr && rustc_feature::is_builtin_attr_name(ident.name) + }) + }) +} + // Some types are used a lot. Make sure they don't unintentionally get bigger. #[cfg(target_pointer_width = "64")] mod size_asserts { From e631b1ebfa273fc4703fa2fd60fde281340d4909 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 09:06:19 +1000 Subject: [PATCH 10/21] Invert the sense of `is_complete` and rename it as `needs_tokens`. I have always found `is_complete` an unhelpful name. The new name (and inverted sense) fits in better with the conditions at its call sites. --- .../rustc_parse/src/parser/attr_wrapper.rs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index 82fd2257481ed..1bc41e68cf372 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -201,7 +201,7 @@ impl<'a> Parser<'a> { // tokens by definition). let needs_collection = matches!(force_collect, ForceCollect::Yes) // - Any of our outer attributes require tokens. - || !is_complete(&attrs.attrs) + || needs_tokens(&attrs.attrs) // - Our target supports custom inner attributes (custom // inner attribute invocation might require token capturing). || R::SUPPORTS_CUSTOM_INNER_ATTRS @@ -259,9 +259,9 @@ impl<'a> Parser<'a> { // - Any of our outer *or* inner attributes require tokens. // (`attr.attrs` was just outer attributes, but `ret.attrs()` is // outer and inner attributes. So this check is more precise than - // the earlier `is_complete()` check, and we don't need to + // the earlier `needs_tokens` check, and we don't need to // check `R::SUPPORTS_CUSTOM_INNER_ATTRS`.) - || !is_complete(ret.attrs()) + || needs_tokens(ret.attrs()) // - We are in `capture_cfg` mode and there are `#[cfg]` or // `#[cfg_attr]` attributes. (During normal non-`capture_cfg` // parsing, we don't need any special capturing for those @@ -457,14 +457,16 @@ fn make_attr_token_stream( AttrTokenStream::new(stack_top.inner) } -/// The attributes are complete if all attributes are either a doc comment or a -/// builtin attribute other than `cfg_attr`. -fn is_complete(attrs: &[ast::Attribute]) -> bool { - attrs.iter().all(|attr| { - attr.is_doc_comment() - || attr.ident().is_some_and(|ident| { - ident.name != sym::cfg_attr && rustc_feature::is_builtin_attr_name(ident.name) - }) +/// Tokens are needed if: +/// - any non-single-segment attributes (other than doc comments) are present; or +/// - any `cfg_attr` attributes are present; +/// - any single-segment, non-builtin attributes are present. +fn needs_tokens(attrs: &[ast::Attribute]) -> bool { + attrs.iter().any(|attr| match attr.ident() { + None => !attr.is_doc_comment(), + Some(ident) => { + ident.name == sym::cfg_attr || !rustc_feature::is_builtin_attr_name(ident.name) + } }) } From a560810a69a09452b4cd0c3173b6af98496dba35 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 10:54:56 +1000 Subject: [PATCH 11/21] Don't include inner attribute ranges in `CaptureState`. The current code is this: ``` self.capture_state.replace_ranges.push((start_pos..end_pos, Some(target))); self.capture_state.replace_ranges.extend(inner_attr_replace_ranges); ``` What's not obvious is that every range in `inner_attr_replace_ranges` must be a strict sub-range of `start_pos..end_pos`. Which means, in `LazyAttrTokenStreamImpl::to_attr_token_stream`, they will be done first, and then the `start_pos..end_pos` replacement will just overwrite them. So they aren't needed. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index dc5f98f7be8b6..1fd0654e0aba9 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -337,8 +337,7 @@ impl<'a> Parser<'a> { // When parsing `m`: // - `start_pos..end_pos` is `0..34` (`mod m`, excluding the `#[cfg_eval]` attribute). // - `inner_attr_replace_ranges` is empty. - // - `replace_range_start..replace_ranges_end` has two entries. - // - One to delete the inner attribute (`17..27`), obtained when parsing `g` (see above). + // - `replace_range_start..replace_ranges_end` has one entry. // - One `AttrsTarget` (added below when parsing `g`) to replace all of `g` (`3..33`, // including its outer attribute), with: // - `attrs`: includes the outer and the inner attr. @@ -369,12 +368,10 @@ impl<'a> Parser<'a> { // What is the status here when parsing the example code at the top of this method? // - // When parsing `g`, we add two entries: + // When parsing `g`, we add one entry: // - The `start_pos..end_pos` (`3..33`) entry has a new `AttrsTarget` with: // - `attrs`: includes the outer and the inner attr. // - `tokens`: lazy tokens for `g` (with its inner attr deleted). - // - `inner_attr_replace_ranges` contains the one entry to delete the inner attr's - // tokens (`17..27`). // // When parsing `m`, we do nothing here. @@ -384,7 +381,6 @@ impl<'a> Parser<'a> { let start_pos = if has_outer_attrs { attrs.start_pos } else { start_pos }; let target = AttrsTarget { attrs: ret.attrs().iter().cloned().collect(), tokens }; self.capture_state.replace_ranges.push((start_pos..end_pos, Some(target))); - self.capture_state.replace_ranges.extend(inner_attr_replace_ranges); } else if matches!(self.capture_state.capturing, Capturing::No) { // Only clear the ranges once we've finished capturing entirely, i.e. we've finished // the outermost call to this method. From 6e87858f26bcb9fadbd60f9e8ee24816a55faa80 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 14:02:15 +1000 Subject: [PATCH 12/21] Fix a comment. Imagine you have replace ranges (2..20,X) and (5..15,Y), and these tokens: ``` a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x ``` If we replace (5..15,Y) first, then (2..20,X) we get this sequence ``` a,b,c,d,e,Y,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x ``` which is what we want. If we do it in the other order, we get this: ``` a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x a,b,X,_,_,Y,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x ``` which is wrong. So it's true that we need the `.rev()` but the comment is wrong about why. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index 1fd0654e0aba9..a90bfb574e2ee 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -137,9 +137,9 @@ impl ToAttrTokenStream for LazyAttrTokenStreamImpl { // `#[cfg(FALSE)] struct Foo { #[cfg(FALSE)] field: bool }` // // By starting processing from the replace range with the greatest - // start position, we ensure that any replace range which encloses - // another replace range will capture the *replaced* tokens for the inner - // range, not the original tokens. + // start position, we ensure that any (outer) replace range which + // encloses another (inner) replace range will fully overwrite the + // inner range's replacement. for (range, target) in replace_ranges.into_iter().rev() { assert!(!range.is_empty(), "Cannot replace an empty range: {range:?}"); From 6ea2da5a28580dba214b32831d0a6952e2ebce91 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 13:20:27 +1000 Subject: [PATCH 13/21] Tweak a loop. A fully imperative style is easier to read than a half-iterator, half-imperative style. Also, rename `inner_attr` as `attr` because it might be an outer attribute. --- compiler/rustc_parse/src/parser/attr_wrapper.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index a90bfb574e2ee..abee668cc6cb0 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -297,11 +297,13 @@ impl<'a> Parser<'a> { // with `None`, which means the relevant tokens will be removed. (More // details below.) let mut inner_attr_replace_ranges = Vec::new(); - for inner_attr in ret.attrs().iter().filter(|a| a.style == ast::AttrStyle::Inner) { - if let Some(attr_range) = self.capture_state.inner_attr_ranges.remove(&inner_attr.id) { - inner_attr_replace_ranges.push((attr_range, None)); - } else { - self.dcx().span_delayed_bug(inner_attr.span, "Missing token range for attribute"); + for attr in ret.attrs() { + if attr.style == ast::AttrStyle::Inner { + if let Some(attr_range) = self.capture_state.inner_attr_ranges.remove(&attr.id) { + inner_attr_replace_ranges.push((attr_range, None)); + } else { + self.dcx().span_delayed_bug(attr.span, "Missing token range for attribute"); + } } } From 55d37ae711b1a1ab82ef02d0594f92167e18af90 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 26 Jul 2024 14:00:29 +1000 Subject: [PATCH 14/21] Remove an unnecessary block. --- .../rustc_parse/src/parser/attr_wrapper.rs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_parse/src/parser/attr_wrapper.rs b/compiler/rustc_parse/src/parser/attr_wrapper.rs index abee668cc6cb0..389e4a6267d2e 100644 --- a/compiler/rustc_parse/src/parser/attr_wrapper.rs +++ b/compiler/rustc_parse/src/parser/attr_wrapper.rs @@ -114,17 +114,15 @@ impl ToAttrTokenStream for LazyAttrTokenStreamImpl { replace_ranges.sort_by_key(|(range, _)| range.start); #[cfg(debug_assertions)] - { - for [(range, tokens), (next_range, next_tokens)] in replace_ranges.array_windows() { - assert!( - range.end <= next_range.start || range.end >= next_range.end, - "Replace ranges should either be disjoint or nested: ({:?}, {:?}) ({:?}, {:?})", - range, - tokens, - next_range, - next_tokens, - ); - } + for [(range, tokens), (next_range, next_tokens)] in replace_ranges.array_windows() { + assert!( + range.end <= next_range.start || range.end >= next_range.end, + "Replace ranges should either be disjoint or nested: ({:?}, {:?}) ({:?}, {:?})", + range, + tokens, + next_range, + next_tokens, + ); } // Process the replace ranges, starting from the highest start From 33b98bf2566d1ac768fe979cb9c5ff5b30c6d9b6 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 26 Jul 2024 10:19:31 +0000 Subject: [PATCH 15/21] Remove redundant option that was just encoding that a slice was empty --- compiler/rustc_ast_lowering/src/item.rs | 6 +++--- compiler/rustc_ast_lowering/src/lib.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs index f990b4ba69b3f..f0186e36c42ae 100644 --- a/compiler/rustc_ast_lowering/src/item.rs +++ b/compiler/rustc_ast_lowering/src/item.rs @@ -172,7 +172,7 @@ impl<'hir> LoweringContext<'_, 'hir> { id: NodeId, hir_id: hir::HirId, ident: &mut Ident, - attrs: Option<&'hir [Attribute]>, + attrs: &'hir [Attribute], vis_span: Span, i: &ItemKind, ) -> hir::ItemKind<'hir> { @@ -488,7 +488,7 @@ impl<'hir> LoweringContext<'_, 'hir> { id: NodeId, vis_span: Span, ident: &mut Ident, - attrs: Option<&'hir [Attribute]>, + attrs: &'hir [Attribute], ) -> hir::ItemKind<'hir> { let path = &tree.prefix; let segments = prefix.segments.iter().chain(path.segments.iter()).cloned().collect(); @@ -566,7 +566,7 @@ impl<'hir> LoweringContext<'_, 'hir> { // `ItemLocalId` and the new owner. (See `lower_node_id`) let kind = this.lower_use_tree(use_tree, &prefix, id, vis_span, &mut ident, attrs); - if let Some(attrs) = attrs { + if !attrs.is_empty() { this.attrs.insert(hir::ItemLocalId::ZERO, attrs); } diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs index 0f5f4d8023bd1..01ba57e97b28f 100644 --- a/compiler/rustc_ast_lowering/src/lib.rs +++ b/compiler/rustc_ast_lowering/src/lib.rs @@ -913,15 +913,15 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { ret } - fn lower_attrs(&mut self, id: HirId, attrs: &[Attribute]) -> Option<&'hir [Attribute]> { + fn lower_attrs(&mut self, id: HirId, attrs: &[Attribute]) -> &'hir [Attribute] { if attrs.is_empty() { - None + &[] } else { debug_assert_eq!(id.owner, self.current_hir_id_owner); let ret = self.arena.alloc_from_iter(attrs.iter().map(|a| self.lower_attr(a))); debug_assert!(!ret.is_empty()); self.attrs.insert(id.local_id, ret); - Some(ret) + ret } } From 114e0dcf254eb88aa66cad76088a46bf47b82b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 26 Jul 2024 13:30:52 +0200 Subject: [PATCH 16/21] CI: do not respect custom try jobs for unrolled perf builds --- src/ci/github-actions/calculate-job-matrix.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/ci/github-actions/calculate-job-matrix.py b/src/ci/github-actions/calculate-job-matrix.py index d03bbda100807..7de6d5fcd5f75 100755 --- a/src/ci/github-actions/calculate-job-matrix.py +++ b/src/ci/github-actions/calculate-job-matrix.py @@ -97,9 +97,15 @@ def find_run_type(ctx: GitHubCtx) -> Optional[WorkflowRunType]: "refs/heads/automation/bors/try" ) + # Unrolled branch from a rollup for testing perf + # This should **not** allow custom try jobs + is_unrolled_perf_build = ctx.ref == "refs/heads/try-perf" + if try_build: - jobs = get_custom_jobs(ctx) - return TryRunType(custom_jobs=jobs) + custom_jobs = [] + if not is_unrolled_perf_build: + custom_jobs = get_custom_jobs(ctx) + return TryRunType(custom_jobs=custom_jobs) if ctx.ref == "refs/heads/auto": return AutoRunType() From 33dd288ef1d28eabb15c15e739496014198329ab Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Fri, 26 Jul 2024 13:59:43 +0200 Subject: [PATCH 17/21] Add a test case for `extern "C" unsafe` to the ui tests --- .../issue-87217-keyword-order/wrong-unsafe-abi.rs | 12 ++++++++++++ .../wrong-unsafe-abi.stderr | 8 ++++++++ 2 files changed, 20 insertions(+) create mode 100644 tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs create mode 100644 tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr diff --git a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs new file mode 100644 index 0000000000000..c97425d8ade88 --- /dev/null +++ b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs @@ -0,0 +1,12 @@ +//@ edition:2018 + +// There is an order to respect for keywords before a function: +// `, const, async, unsafe, extern, ""` +// +// This test ensures the compiler is helpful about them being misplaced. +// Visibilities are tested elsewhere. + +extern "C" unsafe fn test() {} +//~^ ERROR + +fn main() {} diff --git a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr new file mode 100644 index 0000000000000..42c9764cc1e9e --- /dev/null +++ b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr @@ -0,0 +1,8 @@ +error: expected `{`, found keyword `unsafe` + --> $DIR/wrong-unsafe-abi.rs:9:12 + | +LL | extern "C" unsafe fn test() {} + | ^^^^^^ expected `{` + +error: aborting due to 1 previous error + From 3fdc99193e95a29b6fd2228b0acc5f8b4a81feda Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Fri, 26 Jul 2024 15:14:05 +0200 Subject: [PATCH 18/21] Improve error message for `extern "C" unsafe fn()` This was handled correctly already for `extern unsafe fn()`. Co-authored-by: Folkert --- compiler/rustc_parse/src/parser/item.rs | 9 ++++++--- tests/ui/parser/issues/issue-19398.rs | 6 +++++- tests/ui/parser/issues/issue-19398.stderr | 14 +++++++------- .../issue-87217-keyword-order/wrong-unsafe-abi.rs | 6 +++++- .../wrong-unsafe-abi.stderr | 9 +++++++-- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs index 9aaf4b99243f5..112855e6d1f5a 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -2483,12 +2483,15 @@ impl<'a> Parser<'a> { /// `check_pub` adds additional `pub` to the checks in case users place it /// wrongly, can be used to ensure `pub` never comes after `default`. pub(super) fn check_fn_front_matter(&mut self, check_pub: bool, case: Case) -> bool { + const ALL_QUALS: &[Symbol] = + &[kw::Pub, kw::Gen, kw::Const, kw::Async, kw::Unsafe, kw::Safe, kw::Extern]; + // We use an over-approximation here. // `const const`, `fn const` won't parse, but we're not stepping over other syntax either. // `pub` is added in case users got confused with the ordering like `async pub fn`, // only if it wasn't preceded by `default` as `default pub` is invalid. let quals: &[Symbol] = if check_pub { - &[kw::Pub, kw::Gen, kw::Const, kw::Async, kw::Unsafe, kw::Safe, kw::Extern] + ALL_QUALS } else { &[kw::Gen, kw::Const, kw::Async, kw::Unsafe, kw::Safe, kw::Extern] }; @@ -2518,9 +2521,9 @@ impl<'a> Parser<'a> { || self.check_keyword_case(kw::Extern, case) && self.look_ahead(1, |t| t.can_begin_string_literal()) && (self.look_ahead(2, |t| t.is_keyword_case(kw::Fn, case)) || - // this branch is only for better diagnostic in later, `pub` is not allowed here + // this branch is only for better diagnostics; `pub`, `unsafe`, etc. are not allowed here (self.may_recover() - && self.look_ahead(2, |t| t.is_keyword(kw::Pub)) + && self.look_ahead(2, |t| ALL_QUALS.iter().any(|&kw| t.is_keyword(kw))) && self.look_ahead(3, |t| t.is_keyword_case(kw::Fn, case)))) } diff --git a/tests/ui/parser/issues/issue-19398.rs b/tests/ui/parser/issues/issue-19398.rs index 46eb320a172f2..358f65f1da51b 100644 --- a/tests/ui/parser/issues/issue-19398.rs +++ b/tests/ui/parser/issues/issue-19398.rs @@ -1,6 +1,10 @@ trait T { extern "Rust" unsafe fn foo(); - //~^ ERROR expected `{`, found keyword `unsafe` + //~^ ERROR expected `fn`, found keyword `unsafe` + //~| NOTE expected `fn` + //~| HELP `unsafe` must come before `extern "Rust"` + //~| SUGGESTION unsafe extern "Rust" + //~| NOTE keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern` } fn main() {} diff --git a/tests/ui/parser/issues/issue-19398.stderr b/tests/ui/parser/issues/issue-19398.stderr index 236fac673b6b3..2b97ec50c9172 100644 --- a/tests/ui/parser/issues/issue-19398.stderr +++ b/tests/ui/parser/issues/issue-19398.stderr @@ -1,13 +1,13 @@ -error: expected `{`, found keyword `unsafe` +error: expected `fn`, found keyword `unsafe` --> $DIR/issue-19398.rs:2:19 | -LL | trait T { - | - while parsing this item list starting here LL | extern "Rust" unsafe fn foo(); - | ^^^^^^ expected `{` -LL | -LL | } - | - the item list ends here + | --------------^^^^^^ + | | | + | | expected `fn` + | help: `unsafe` must come before `extern "Rust"`: `unsafe extern "Rust"` + | + = note: keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern` error: aborting due to 1 previous error diff --git a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs index c97425d8ade88..794ac635c7694 100644 --- a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs +++ b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.rs @@ -7,6 +7,10 @@ // Visibilities are tested elsewhere. extern "C" unsafe fn test() {} -//~^ ERROR +//~^ ERROR expected `fn`, found keyword `unsafe` +//~| NOTE expected `fn` +//~| HELP `unsafe` must come before `extern "C"` +//~| SUGGESTION unsafe extern "C" +//~| NOTE keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern` fn main() {} diff --git a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr index 42c9764cc1e9e..8ed037869c829 100644 --- a/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr +++ b/tests/ui/parser/issues/issue-87217-keyword-order/wrong-unsafe-abi.stderr @@ -1,8 +1,13 @@ -error: expected `{`, found keyword `unsafe` +error: expected `fn`, found keyword `unsafe` --> $DIR/wrong-unsafe-abi.rs:9:12 | LL | extern "C" unsafe fn test() {} - | ^^^^^^ expected `{` + | -----------^^^^^^ + | | | + | | expected `fn` + | help: `unsafe` must come before `extern "C"`: `unsafe extern "C"` + | + = note: keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern` error: aborting due to 1 previous error From 130ce490f55b6ca8dbe65c977bae18b04736d3f3 Mon Sep 17 00:00:00 2001 From: harryscholes Date: Fri, 26 Jul 2024 16:09:17 +0100 Subject: [PATCH 19/21] Fix docs --- library/core/src/iter/traits/iterator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index 733d414d44465..469097e484773 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -823,7 +823,7 @@ pub trait Iterator { /// /// Given an element the closure must return `true` or `false`. The returned /// iterator will yield only the elements for which the closure returns - /// true. + /// `true`. /// /// # Examples /// From d5656059a1e517d9c7b228d21a4134e097e20e00 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 25 Jul 2024 14:02:33 -0400 Subject: [PATCH 20/21] Make coroutine-closures possible to be cloned --- compiler/rustc_mir_transform/src/shim.rs | 3 ++ .../src/solve/assembly/structural_traits.rs | 5 ++- .../src/traits/select/mod.rs | 17 +++++++-- .../async-closures/clone-closure.rs | 24 +++++++++++++ .../async-closures/clone-closure.run.stdout | 2 ++ .../move-consuming-capture.stderr | 9 +++++ .../async-closures/not-clone-closure.rs | 36 +++++++++++++++++++ .../async-closures/not-clone-closure.stderr | 20 +++++++++++ ...thout-precise-captures-we-are-powerless.rs | 2 +- ...t-precise-captures-we-are-powerless.stderr | 23 ++++-------- 10 files changed, 120 insertions(+), 21 deletions(-) create mode 100644 tests/ui/async-await/async-closures/clone-closure.rs create mode 100644 tests/ui/async-await/async-closures/clone-closure.run.stdout create mode 100644 tests/ui/async-await/async-closures/not-clone-closure.rs create mode 100644 tests/ui/async-await/async-closures/not-clone-closure.stderr diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index 19e3bf5a59939..d2f5004082142 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -435,6 +435,9 @@ fn build_clone_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, self_ty: Ty<'tcx>) - match self_ty.kind() { ty::FnDef(..) | ty::FnPtr(_) => builder.copy_shim(), ty::Closure(_, args) => builder.tuple_like_shim(dest, src, args.as_closure().upvar_tys()), + ty::CoroutineClosure(_, args) => { + builder.tuple_like_shim(dest, src, args.as_coroutine_closure().upvar_tys()) + } ty::Tuple(..) => builder.tuple_like_shim(dest, src, self_ty.tuple_fields()), ty::Coroutine(coroutine_def_id, args) => { assert_eq!(tcx.coroutine_movability(*coroutine_def_id), hir::Movability::Movable); diff --git a/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs b/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs index 770ac9a929eae..60beaa0df84ca 100644 --- a/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs +++ b/compiler/rustc_next_trait_solver/src/solve/assembly/structural_traits.rs @@ -217,7 +217,10 @@ where // impl Copy/Clone for Closure where Self::TupledUpvars: Copy/Clone ty::Closure(_, args) => Ok(vec![ty::Binder::dummy(args.as_closure().tupled_upvars_ty())]), - ty::CoroutineClosure(..) => Err(NoSolution), + // impl Copy/Clone for CoroutineClosure where Self::TupledUpvars: Copy/Clone + ty::CoroutineClosure(_, args) => { + Ok(vec![ty::Binder::dummy(args.as_coroutine_closure().tupled_upvars_ty())]) + } // only when `coroutine_clone` is enabled and the coroutine is movable // impl Copy/Clone for Coroutine where T: Copy/Clone forall T in (upvars, witnesses) diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index c007cd5314a86..699c05466bd5a 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -2262,8 +2262,21 @@ impl<'tcx> SelectionContext<'_, 'tcx> { } } - // FIXME(async_closures): These are never clone, for now. - ty::CoroutineClosure(_, _) => None, + ty::CoroutineClosure(_, args) => { + // (*) binder moved here + let ty = self.infcx.shallow_resolve(args.as_coroutine_closure().tupled_upvars_ty()); + if let ty::Infer(ty::TyVar(_)) = ty.kind() { + // Not yet resolved. + Ambiguous + } else { + Where( + obligation + .predicate + .rebind(args.as_coroutine_closure().upvar_tys().to_vec()), + ) + } + } + // `Copy` and `Clone` are automatically implemented for an anonymous adt // if all of its fields are `Copy` and `Clone` ty::Adt(adt, args) if adt.is_anonymous() => { diff --git a/tests/ui/async-await/async-closures/clone-closure.rs b/tests/ui/async-await/async-closures/clone-closure.rs new file mode 100644 index 0000000000000..807897e3e0314 --- /dev/null +++ b/tests/ui/async-await/async-closures/clone-closure.rs @@ -0,0 +1,24 @@ +//@ aux-build:block-on.rs +//@ edition:2021 +//@ run-pass +//@ check-run-results + +#![feature(async_closure)] + +extern crate block_on; + +async fn for_each(f: impl async FnOnce(&str) + Clone) { + f.clone()("world").await; + f.clone()("world2").await; +} + +fn main() { + block_on::block_on(async_main()); +} + +async fn async_main() { + let x = String::from("Hello,"); + for_each(async move |s| { + println!("{x} {s}"); + }).await; +} diff --git a/tests/ui/async-await/async-closures/clone-closure.run.stdout b/tests/ui/async-await/async-closures/clone-closure.run.stdout new file mode 100644 index 0000000000000..0cfcf1923da88 --- /dev/null +++ b/tests/ui/async-await/async-closures/clone-closure.run.stdout @@ -0,0 +1,2 @@ +Hello, world +Hello, world2 diff --git a/tests/ui/async-await/async-closures/move-consuming-capture.stderr b/tests/ui/async-await/async-closures/move-consuming-capture.stderr index 45c1eac8f8f56..4ce71ec49d612 100644 --- a/tests/ui/async-await/async-closures/move-consuming-capture.stderr +++ b/tests/ui/async-await/async-closures/move-consuming-capture.stderr @@ -11,6 +11,15 @@ LL | x().await; | note: `async_call_once` takes ownership of the receiver `self`, which moves `x` --> $SRC_DIR/core/src/ops/async_function.rs:LL:COL +help: you could `clone` the value and consume it, if the `NoCopy: Clone` trait bound could be satisfied + | +LL | x.clone()().await; + | ++++++++ +help: consider annotating `NoCopy` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct NoCopy; + | error: aborting due to 1 previous error diff --git a/tests/ui/async-await/async-closures/not-clone-closure.rs b/tests/ui/async-await/async-closures/not-clone-closure.rs new file mode 100644 index 0000000000000..2776ce4690fe2 --- /dev/null +++ b/tests/ui/async-await/async-closures/not-clone-closure.rs @@ -0,0 +1,36 @@ +//@ edition: 2021 + +#![feature(async_closure)] + +struct NotClonableArg; +#[derive(Default)] +struct NotClonableReturnType; + +// Verify that the only components that we care about are the upvars, not the signature. +fn we_are_okay_with_not_clonable_signature() { + let x = async |x: NotClonableArg| -> NotClonableReturnType { Default::default() }; + x.clone(); // Okay +} + +#[derive(Debug)] +struct NotClonableUpvar; + +fn we_only_care_about_clonable_upvars() { + let x = NotClonableUpvar; + // Notably, this is clone because we capture `&x`. + let yes_clone = async || { + println!("{x:?}"); + }; + yes_clone.clone(); // Okay + + let z = NotClonableUpvar; + // However, this is not because the closure captures `z` by move. + // (Even though the future that is lent out captures `z by ref!) + let not_clone = async move || { + println!("{z:?}"); + }; + not_clone.clone(); + //~^ ERROR the trait bound `NotClonableUpvar: Clone` is not satisfied +} + +fn main() {} diff --git a/tests/ui/async-await/async-closures/not-clone-closure.stderr b/tests/ui/async-await/async-closures/not-clone-closure.stderr new file mode 100644 index 0000000000000..aea48a455c218 --- /dev/null +++ b/tests/ui/async-await/async-closures/not-clone-closure.stderr @@ -0,0 +1,20 @@ +error[E0277]: the trait bound `NotClonableUpvar: Clone` is not satisfied in `{async closure@$DIR/not-clone-closure.rs:29:21: 29:34}` + --> $DIR/not-clone-closure.rs:32:15 + | +LL | not_clone.clone(); + | ^^^^^ within `{async closure@$DIR/not-clone-closure.rs:29:21: 29:34}`, the trait `Clone` is not implemented for `NotClonableUpvar`, which is required by `{async closure@$DIR/not-clone-closure.rs:29:21: 29:34}: Clone` + | +note: required because it's used within this closure + --> $DIR/not-clone-closure.rs:29:21 + | +LL | let not_clone = async move || { + | ^^^^^^^^^^^^^ +help: consider annotating `NotClonableUpvar` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct NotClonableUpvar; + | + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs index 17681161e20ed..18f16ca4b2d30 100644 --- a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs +++ b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.rs @@ -19,7 +19,7 @@ fn simple<'a>(x: &'a i32) { let c = async move || { println!("{}", *x); }; outlives::<'a>(c()); //~ ERROR `c` does not live long enough - outlives::<'a>(call_once(c)); //~ ERROR cannot move out of `c` + outlives::<'a>(call_once(c)); } struct S<'a>(&'a i32); diff --git a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr index 569028934cbac..ed32a53e80728 100644 --- a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr +++ b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr @@ -29,22 +29,6 @@ LL | outlives::<'a>(call_once(c)); LL | } | - `c` dropped here while still borrowed -error[E0505]: cannot move out of `c` because it is borrowed - --> $DIR/without-precise-captures-we-are-powerless.rs:22:30 - | -LL | fn simple<'a>(x: &'a i32) { - | -- lifetime `'a` defined here -... -LL | let c = async move || { println!("{}", *x); }; - | - binding `c` declared here -LL | outlives::<'a>(c()); - | --- - | | - | borrow of `c` occurs here - | argument requires that `c` is borrowed for `'a` -LL | outlives::<'a>(call_once(c)); - | ^ move out of `c` occurs here - error[E0597]: `x` does not live long enough --> $DIR/without-precise-captures-we-are-powerless.rs:28:13 | @@ -72,6 +56,11 @@ LL | outlives::<'a>(call_once(c)); LL | LL | let c = async move || { println!("{}", *x.0); }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `x` occurs here + | +help: consider cloning the value if the performance cost is acceptable + | +LL | let c = async || { println!("{}", *x.0); }.clone(); + | ++++++++ error[E0597]: `c` does not live long enough --> $DIR/without-precise-captures-we-are-powerless.rs:33:20 @@ -146,7 +135,7 @@ LL | // outlives::<'a>(call_once(c)); // FIXME(async_closures): Figure out w LL | } | - `c` dropped here while still borrowed -error: aborting due to 10 previous errors +error: aborting due to 9 previous errors Some errors have detailed explanations: E0505, E0597, E0621. For more information about an error, try `rustc --explain E0505`. From 5a9959fd9daa17e77d609f3c2db4d18554484447 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 25 Jul 2024 14:38:55 -0400 Subject: [PATCH 21/21] Suppress useless clone suggestion --- compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs | 5 +++++ .../without-precise-captures-we-are-powerless.stderr | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 2d9bc45ebc823..e5bc4b3874823 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -1442,9 +1442,14 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> { // See `tests/ui/moves/needs-clone-through-deref.rs` return false; } + // We don't want to suggest `.clone()` in a move closure, since the value has already been captured. if self.in_move_closure(expr) { return false; } + // We also don't want to suggest cloning a closure itself, since the value has already been captured. + if let hir::ExprKind::Closure(_) = expr.kind { + return false; + } // Try to find predicates on *generic params* that would allow copying `ty` let mut suggestion = if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) { diff --git a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr index ed32a53e80728..1df5abdbb18e3 100644 --- a/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr +++ b/tests/ui/async-await/async-closures/without-precise-captures-we-are-powerless.stderr @@ -56,11 +56,6 @@ LL | outlives::<'a>(call_once(c)); LL | LL | let c = async move || { println!("{}", *x.0); }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `x` occurs here - | -help: consider cloning the value if the performance cost is acceptable - | -LL | let c = async || { println!("{}", *x.0); }.clone(); - | ++++++++ error[E0597]: `c` does not live long enough --> $DIR/without-precise-captures-we-are-powerless.rs:33:20