From 2f305ff460862e18b67db2faa11b85ab223d4e60 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 15:02:19 +1000 Subject: [PATCH 01/22] Remove an unnecessary `?`. --- compiler/rustc_parse/src/parser/item.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs index d2fea5583b813..994e7c5d2db54 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -128,14 +128,10 @@ impl<'a> Parser<'a> { Some(item.into_inner()) }); - let item = - self.collect_tokens_trailing_token(attrs, force_collect, |this: &mut Self, attrs| { - let item = - this.parse_item_common_(attrs, mac_allowed, attrs_allowed, fn_parse_mode); - Ok((item?, TrailingToken::None)) - })?; - - Ok(item) + self.collect_tokens_trailing_token(attrs, force_collect, |this, attrs| { + let item = this.parse_item_common_(attrs, mac_allowed, attrs_allowed, fn_parse_mode)?; + Ok((item, TrailingToken::None)) + }) } fn parse_item_common_( From 48cdfc388d67a6bde420d8d44eddf8ef1e5b7eb2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 15:07:42 +1000 Subject: [PATCH 02/22] Inline `Parser::parse_item_common_`. It has a single call site, it isn't that big, and its name is confusingly similar to `Parser::parse_item_common`. --- compiler/rustc_parse/src/parser/item.rs | 75 +++++++++++-------------- 1 file changed, 32 insertions(+), 43 deletions(-) diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs index 994e7c5d2db54..2d5781c3bbfb3 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -128,52 +128,41 @@ impl<'a> Parser<'a> { Some(item.into_inner()) }); - self.collect_tokens_trailing_token(attrs, force_collect, |this, attrs| { - let item = this.parse_item_common_(attrs, mac_allowed, attrs_allowed, fn_parse_mode)?; - Ok((item, TrailingToken::None)) - }) - } - - fn parse_item_common_( - &mut self, - mut attrs: AttrVec, - mac_allowed: bool, - attrs_allowed: bool, - fn_parse_mode: FnParseMode, - ) -> PResult<'a, Option> { - let lo = self.token.span; - let vis = self.parse_visibility(FollowedByType::No)?; - let mut def = self.parse_defaultness(); - let kind = self.parse_item_kind( - &mut attrs, - mac_allowed, - lo, - &vis, - &mut def, - fn_parse_mode, - Case::Sensitive, - )?; - if let Some((ident, kind)) = kind { - self.error_on_unconsumed_default(def, &kind); - let span = lo.to(self.prev_token.span); - let id = DUMMY_NODE_ID; - let item = Item { ident, attrs, id, kind, vis, span, tokens: None }; - return Ok(Some(item)); - } + self.collect_tokens_trailing_token(attrs, force_collect, |this, mut attrs| { + let lo = this.token.span; + let vis = this.parse_visibility(FollowedByType::No)?; + let mut def = this.parse_defaultness(); + let kind = this.parse_item_kind( + &mut attrs, + mac_allowed, + lo, + &vis, + &mut def, + fn_parse_mode, + Case::Sensitive, + )?; + if let Some((ident, kind)) = kind { + this.error_on_unconsumed_default(def, &kind); + let span = lo.to(this.prev_token.span); + let id = DUMMY_NODE_ID; + let item = Item { ident, attrs, id, kind, vis, span, tokens: None }; + return Ok((Some(item), TrailingToken::None)); + } - // At this point, we have failed to parse an item. - if !matches!(vis.kind, VisibilityKind::Inherited) { - self.dcx().emit_err(errors::VisibilityNotFollowedByItem { span: vis.span, vis }); - } + // At this point, we have failed to parse an item. + if !matches!(vis.kind, VisibilityKind::Inherited) { + this.dcx().emit_err(errors::VisibilityNotFollowedByItem { span: vis.span, vis }); + } - if let Defaultness::Default(span) = def { - self.dcx().emit_err(errors::DefaultNotFollowedByItem { span }); - } + if let Defaultness::Default(span) = def { + this.dcx().emit_err(errors::DefaultNotFollowedByItem { span }); + } - if !attrs_allowed { - self.recover_attrs_no_item(&attrs)?; - } - Ok(None) + if !attrs_allowed { + this.recover_attrs_no_item(&attrs)?; + } + Ok((None, TrailingToken::None)) + }) } /// Error in-case `default` was parsed in an in-appropriate context. From d247489ac280147b48c72ed739ea9056e0ca6ff2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 15:54:34 +1000 Subject: [PATCH 03/22] Reorder `Parser::parse_expr_dot_or_call_with` arguments. Put `attrs` before `e0` because that matches the order in the source code, where outer attributes appear before expressions. --- compiler/rustc_parse/src/parser/expr.rs | 4 ++-- compiler/rustc_parse/src/parser/pat.rs | 2 +- compiler/rustc_parse/src/parser/stmt.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index b2df9a14eb01e..a1bb047e464f2 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -885,15 +885,15 @@ impl<'a> Parser<'a> { self.collect_tokens_for_expr(attrs, |this, attrs| { let base = this.parse_expr_bottom()?; let span = this.interpolated_or_expr_span(&base); - this.parse_expr_dot_or_call_with(base, span, attrs) + this.parse_expr_dot_or_call_with(attrs, base, span) }) } pub(super) fn parse_expr_dot_or_call_with( &mut self, + mut attrs: ast::AttrVec, e0: P, lo: Span, - mut attrs: ast::AttrVec, ) -> PResult<'a, P> { // Stitch the list of outer attributes onto the return value. // A little bit ugly, but the best way given the current code diff --git a/compiler/rustc_parse/src/parser/pat.rs b/compiler/rustc_parse/src/parser/pat.rs index 6f2b717715945..7ef6474c29e41 100644 --- a/compiler/rustc_parse/src/parser/pat.rs +++ b/compiler/rustc_parse/src/parser/pat.rs @@ -388,9 +388,9 @@ impl<'a> Parser<'a> { // Parse `?`, `.f`, `(arg0, arg1, ...)` or `[expr]` until they've all been eaten. if let Ok(expr) = snapshot .parse_expr_dot_or_call_with( + AttrVec::new(), self.mk_expr(pat_span, ExprKind::Dummy), // equivalent to transforming the parsed pattern into an `Expr` pat_span, - AttrVec::new(), ) .map_err(|err| err.cancel()) { diff --git a/compiler/rustc_parse/src/parser/stmt.rs b/compiler/rustc_parse/src/parser/stmt.rs index d65f6ff68eeeb..1259d223af6be 100644 --- a/compiler/rustc_parse/src/parser/stmt.rs +++ b/compiler/rustc_parse/src/parser/stmt.rs @@ -164,7 +164,7 @@ impl<'a> Parser<'a> { }; let expr = this.with_res(Restrictions::STMT_EXPR, |this| { - this.parse_expr_dot_or_call_with(expr, lo, attrs) + this.parse_expr_dot_or_call_with(attrs, expr, lo) })?; // `DUMMY_SP` will get overwritten later in this function Ok((this.mk_stmt(rustc_span::DUMMY_SP, StmtKind::Expr(expr)), TrailingToken::None)) @@ -206,7 +206,7 @@ impl<'a> Parser<'a> { // Since none of the above applied, this is an expression statement macro. let e = self.mk_expr(lo.to(hi), ExprKind::MacCall(mac)); let e = self.maybe_recover_from_bad_qpath(e)?; - let e = self.parse_expr_dot_or_call_with(e, lo, attrs)?; + let e = self.parse_expr_dot_or_call_with(attrs, e, lo)?; let e = self .parse_expr_assoc_with(0, LhsExpr::Parsed { expr: e, starts_statement: false })?; StmtKind::Expr(e) From 96cc9c99b29c373db41093dee3170f36dc15543b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 16:02:45 +1000 Subject: [PATCH 04/22] Inline and remove `Parser::parse_and_disallow_postfix_after_cast`. It has a single call site. Removing it removes the need for an `ExprKind` check. The commit also clarifies the relevant comment. --- compiler/rustc_parse/src/parser/expr.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index a1bb047e464f2..49a71b97be248 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -785,19 +785,10 @@ impl<'a> Parser<'a> { } }; - self.parse_and_disallow_postfix_after_cast(cast_expr) - } - - /// Parses a postfix operators such as `.`, `?`, or index (`[]`) after a cast, - /// then emits an error and returns the newly parsed tree. - /// The resulting parse tree for `&x as T[0]` has a precedence of `((&x) as T)[0]`. - fn parse_and_disallow_postfix_after_cast( - &mut self, - cast_expr: P, - ) -> PResult<'a, P> { - if let ExprKind::Type(_, _) = cast_expr.kind { - panic!("ExprKind::Type must not be parsed"); - } + // Try to parse a postfix operator such as `.`, `?`, or index (`[]`) + // after a cast. If one is present, emit an error then return a valid + // parse tree; For something like `&x as T[0]` will be as if it was + // written `((&x) as T)[0]`. let span = cast_expr.span; From 96b39f1204992368f059b44b56362a91e1e27ef9 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 16:12:58 +1000 Subject: [PATCH 05/22] Inline and remove `Parser::parse_expr_dot_or_call_with_`. It only has two call sites, and it extremely similar to `Parser::parse_expr_dot_or_call_with`, in both name and behaviour. The only difference is the latter has an `attrs` argument and an `ensure_sufficient_stack` call. We can pass in an empty `attrs` as necessary, as is already done at some `parse_expr_dot_or_call_with` call sites. --- compiler/rustc_parse/src/parser/expr.rs | 102 ++++++++++++------------ 1 file changed, 49 insertions(+), 53 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 49a71b97be248..896f84d3a36ca 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -792,7 +792,7 @@ impl<'a> Parser<'a> { let span = cast_expr.span; - let with_postfix = self.parse_expr_dot_or_call_with_(cast_expr, span)?; + let with_postfix = self.parse_expr_dot_or_call_with(AttrVec::new(), cast_expr, span)?; // Check if an illegal postfix operator has been added after the cast. // If the resulting expression is not a cast, it is an illegal postfix operator. @@ -883,16 +883,56 @@ impl<'a> Parser<'a> { pub(super) fn parse_expr_dot_or_call_with( &mut self, mut attrs: ast::AttrVec, - e0: P, + mut e: P, lo: Span, ) -> PResult<'a, P> { - // Stitch the list of outer attributes onto the return value. - // A little bit ugly, but the best way given the current code - // structure - let res = ensure_sufficient_stack( - // this expr demonstrates the recursion it guards against - || self.parse_expr_dot_or_call_with_(e0, lo), - ); + let res = ensure_sufficient_stack(|| { + loop { + let has_question = + if self.prev_token.kind == TokenKind::Ident(kw::Return, IdentIsRaw::No) { + // We are using noexpect here because we don't expect a `?` directly after + // a `return` which could be suggested otherwise. + self.eat_noexpect(&token::Question) + } else { + self.eat(&token::Question) + }; + if has_question { + // `expr?` + e = self.mk_expr(lo.to(self.prev_token.span), ExprKind::Try(e)); + continue; + } + let has_dot = + if self.prev_token.kind == TokenKind::Ident(kw::Return, IdentIsRaw::No) { + // We are using noexpect here because we don't expect a `.` directly after + // a `return` which could be suggested otherwise. + self.eat_noexpect(&token::Dot) + } else if self.token.kind == TokenKind::RArrow && self.may_recover() { + // Recovery for `expr->suffix`. + self.bump(); + let span = self.prev_token.span; + self.dcx().emit_err(errors::ExprRArrowCall { span }); + true + } else { + self.eat(&token::Dot) + }; + if has_dot { + // expr.f + e = self.parse_dot_suffix_expr(lo, e)?; + continue; + } + if self.expr_is_complete(&e) { + return Ok(e); + } + e = match self.token.kind { + token::OpenDelim(Delimiter::Parenthesis) => self.parse_expr_fn_call(lo, e), + token::OpenDelim(Delimiter::Bracket) => self.parse_expr_index(lo, e)?, + _ => return Ok(e), + } + } + }); + + // Stitch the list of outer attributes onto the return value. A little + // bit ugly, but the best way given the current code structure. if attrs.is_empty() { res } else { @@ -906,50 +946,6 @@ impl<'a> Parser<'a> { } } - fn parse_expr_dot_or_call_with_(&mut self, mut e: P, lo: Span) -> PResult<'a, P> { - loop { - let has_question = - if self.prev_token.kind == TokenKind::Ident(kw::Return, IdentIsRaw::No) { - // we are using noexpect here because we don't expect a `?` directly after a `return` - // which could be suggested otherwise - self.eat_noexpect(&token::Question) - } else { - self.eat(&token::Question) - }; - if has_question { - // `expr?` - e = self.mk_expr(lo.to(self.prev_token.span), ExprKind::Try(e)); - continue; - } - let has_dot = if self.prev_token.kind == TokenKind::Ident(kw::Return, IdentIsRaw::No) { - // we are using noexpect here because we don't expect a `.` directly after a `return` - // which could be suggested otherwise - self.eat_noexpect(&token::Dot) - } else if self.token.kind == TokenKind::RArrow && self.may_recover() { - // Recovery for `expr->suffix`. - self.bump(); - let span = self.prev_token.span; - self.dcx().emit_err(errors::ExprRArrowCall { span }); - true - } else { - self.eat(&token::Dot) - }; - if has_dot { - // expr.f - e = self.parse_dot_suffix_expr(lo, e)?; - continue; - } - if self.expr_is_complete(&e) { - return Ok(e); - } - e = match self.token.kind { - token::OpenDelim(Delimiter::Parenthesis) => self.parse_expr_fn_call(lo, e), - token::OpenDelim(Delimiter::Bracket) => self.parse_expr_index(lo, e)?, - _ => return Ok(e), - } - } - } - pub(super) fn parse_dot_suffix_expr( &mut self, lo: Span, From 8cb6bc3b5a33cbd7d9b131fad2f63cc243b26ea0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 16:39:04 +1000 Subject: [PATCH 06/22] Fix a comment. --- compiler/rustc_parse/src/parser/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 896f84d3a36ca..796a279188dba 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -1375,7 +1375,7 @@ impl<'a> Parser<'a> { /// Parses things like parenthesized exprs, macros, `return`, etc. /// /// N.B., this does not parse outer attributes, and is private because it only works - /// correctly if called from `parse_dot_or_call_expr()`. + /// correctly if called from `parse_expr_dot_or_call`. fn parse_expr_bottom(&mut self) -> PResult<'a, P> { maybe_recover_from_interpolated_ty_qpath!(self, true); From 9c4f3dbd066fd8e35f188388f66493522f7d4476 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 16 Jul 2024 16:40:35 +1000 Subject: [PATCH 07/22] Remove references to `maybe_whole_expr`. It was removed in #126571. --- compiler/rustc_ast/src/token.rs | 3 +-- compiler/rustc_parse/src/parser/mod.rs | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/rustc_ast/src/token.rs b/compiler/rustc_ast/src/token.rs index efe1956615216..9478da236c315 100644 --- a/compiler/rustc_ast/src/token.rs +++ b/compiler/rustc_ast/src/token.rs @@ -699,8 +699,7 @@ impl Token { false } - /// Would `maybe_whole_expr` in `parser.rs` return `Ok(..)`? - /// That is, is this a pre-parsed expression dropped into the token stream + /// Is this a pre-parsed expression dropped into the token stream /// (which happens while parsing the result of macro expansion)? pub fn is_whole_expr(&self) -> bool { if let Interpolated(nt) = &self.kind diff --git a/compiler/rustc_parse/src/parser/mod.rs b/compiler/rustc_parse/src/parser/mod.rs index 958458eda9a78..0117f993bcb33 100644 --- a/compiler/rustc_parse/src/parser/mod.rs +++ b/compiler/rustc_parse/src/parser/mod.rs @@ -101,7 +101,6 @@ pub enum TrailingToken { MaybeComma, } -/// Like `maybe_whole_expr`, but for things other than expressions. #[macro_export] macro_rules! maybe_whole { ($p:expr, $constructor:ident, |$x:ident| $e:expr) => { From 17c70a9aacd54fefa76d6e349e6d050a9445c9a7 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 22:50:25 -0700 Subject: [PATCH 08/22] unix: split stack_overflow::install_main_guard by os --- .../std/src/sys/pal/unix/stack_overflow.rs | 205 ++++++++++-------- 1 file changed, 115 insertions(+), 90 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 2e5bd85327a19..431fb9652a74e 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -325,104 +325,129 @@ mod imp { }) } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard() -> Option> { let page_size = PAGE_SIZE.load(Ordering::Relaxed); - if cfg!(all(target_os = "linux", not(target_env = "musl"))) { - // Linux doesn't allocate the whole stack right away, and - // the kernel has its own stack-guard mechanism to fault - // when growing too close to an existing mapping. If we map - // our own guard, then the kernel starts enforcing a rather - // large gap above that, rendering much of the possible - // stack space useless. See #43052. - // - // Instead, we'll just note where we expect rlimit to start - // faulting, so our handler can report "stack overflow", and - // trust that the kernel's own stack guard will work. - let stackptr = get_stack_start_aligned()?; - let stackaddr = stackptr.addr(); - Some(stackaddr - page_size..stackaddr) - } else if cfg!(all(target_os = "linux", target_env = "musl")) { - // For the main thread, the musl's pthread_attr_getstack - // returns the current stack size, rather than maximum size - // it can eventually grow to. It cannot be used to determine - // the position of kernel's stack guard. - None - } else if cfg!(target_os = "freebsd") { - // FreeBSD's stack autogrows, and optionally includes a guard page - // at the bottom. If we try to remap the bottom of the stack - // ourselves, FreeBSD's guard page moves upwards. So we'll just use - // the builtin guard page. - let stackptr = get_stack_start_aligned()?; - let guardaddr = stackptr.addr(); - // Technically the number of guard pages is tunable and controlled - // by the security.bsd.stack_guard_page sysctl. - // By default it is 1, checking once is enough since it is - // a boot time config value. - static PAGES: crate::sync::OnceLock = crate::sync::OnceLock::new(); - - let pages = PAGES.get_or_init(|| { - use crate::sys::weak::dlsym; - dlsym!(fn sysctlbyname(*const libc::c_char, *mut libc::c_void, *mut libc::size_t, *const libc::c_void, libc::size_t) -> libc::c_int); - let mut guard: usize = 0; - let mut size = crate::mem::size_of_val(&guard); - let oid = crate::ffi::CStr::from_bytes_with_nul( - b"security.bsd.stack_guard_page\0", - ) - .unwrap(); - match sysctlbyname.get() { - Some(fcn) => { - if fcn(oid.as_ptr(), core::ptr::addr_of_mut!(guard) as *mut _, core::ptr::addr_of_mut!(size) as *mut _, crate::ptr::null_mut(), 0) == 0 { - guard - } else { - 1 - } - }, - _ => 1, - } - }); - Some(guardaddr..guardaddr + pages * page_size) - } else if cfg!(any(target_os = "openbsd", target_os = "netbsd")) { - // OpenBSD stack already includes a guard page, and stack is - // immutable. - // NetBSD stack includes the guard page. - // - // We'll just note where we expect rlimit to start - // faulting, so our handler can report "stack overflow", and - // trust that the kernel's own stack guard will work. - let stackptr = get_stack_start_aligned()?; - let stackaddr = stackptr.addr(); - Some(stackaddr - page_size..stackaddr) - } else { - // Reallocate the last page of the stack. - // This ensures SIGBUS will be raised on - // stack overflow. - // Systems which enforce strict PAX MPROTECT do not allow - // to mprotect() a mapping with less restrictive permissions - // than the initial mmap() used, so we mmap() here with - // read/write permissions and only then mprotect() it to - // no permissions at all. See issue #50313. - let stackptr = get_stack_start_aligned()?; - let result = mmap64( - stackptr, - page_size, - PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANON | MAP_FIXED, - -1, - 0, - ); - if result != stackptr || result == MAP_FAILED { - panic!("failed to allocate a guard page: {}", io::Error::last_os_error()); + + unsafe { + // this way someone on any unix-y OS can check that all these compile + if cfg!(all(target_os = "linux", not(target_env = "musl"))) { + install_main_guard_linux(page_size) + } else if cfg!(all(target_os = "linux", target_env = "musl")) { + install_main_guard_linux_musl(page_size) + } else if cfg!(target_os = "freebsd") { + install_main_guard_freebsd(page_size) + } else if cfg!(any(target_os = "netbsd", target_os = "openbsd")) { + install_main_guard_bsds(page_size) + } else { + install_main_guard_default(page_size) } + } + } + + unsafe fn install_main_guard_linux(page_size: usize) -> Option> { + // Linux doesn't allocate the whole stack right away, and + // the kernel has its own stack-guard mechanism to fault + // when growing too close to an existing mapping. If we map + // our own guard, then the kernel starts enforcing a rather + // large gap above that, rendering much of the possible + // stack space useless. See #43052. + // + // Instead, we'll just note where we expect rlimit to start + // faulting, so our handler can report "stack overflow", and + // trust that the kernel's own stack guard will work. + let stackptr = get_stack_start_aligned()?; + let stackaddr = stackptr.addr(); + Some(stackaddr - page_size..stackaddr) + } - let result = mprotect(stackptr, page_size, PROT_NONE); - if result != 0 { - panic!("failed to protect the guard page: {}", io::Error::last_os_error()); + unsafe fn install_main_guard_linux_musl(_page_size: usize) -> Option> { + // For the main thread, the musl's pthread_attr_getstack + // returns the current stack size, rather than maximum size + // it can eventually grow to. It cannot be used to determine + // the position of kernel's stack guard. + None + } + + unsafe fn install_main_guard_freebsd(page_size: usize) -> Option> { + // FreeBSD's stack autogrows, and optionally includes a guard page + // at the bottom. If we try to remap the bottom of the stack + // ourselves, FreeBSD's guard page moves upwards. So we'll just use + // the builtin guard page. + let stackptr = get_stack_start_aligned()?; + let guardaddr = stackptr.addr(); + // Technically the number of guard pages is tunable and controlled + // by the security.bsd.stack_guard_page sysctl. + // By default it is 1, checking once is enough since it is + // a boot time config value. + static PAGES: crate::sync::OnceLock = crate::sync::OnceLock::new(); + + let pages = PAGES.get_or_init(|| { + use crate::sys::weak::dlsym; + dlsym!(fn sysctlbyname(*const libc::c_char, *mut libc::c_void, *mut libc::size_t, *const libc::c_void, libc::size_t) -> libc::c_int); + let mut guard: usize = 0; + let mut size = crate::mem::size_of_val(&guard); + let oid = crate::ffi::CStr::from_bytes_with_nul( + b"security.bsd.stack_guard_page\0", + ) + .unwrap(); + match sysctlbyname.get() { + Some(fcn) => { + if fcn(oid.as_ptr(), core::ptr::addr_of_mut!(guard) as *mut _, core::ptr::addr_of_mut!(size) as *mut _, crate::ptr::null_mut(), 0) == 0 { + guard + } else { + 1 + } + }, + _ => 1, } + }); + Some(guardaddr..guardaddr + pages * page_size) + } - let guardaddr = stackptr.addr(); + unsafe fn install_main_guard_bsds(page_size: usize) -> Option> { + // OpenBSD stack already includes a guard page, and stack is + // immutable. + // NetBSD stack includes the guard page. + // + // We'll just note where we expect rlimit to start + // faulting, so our handler can report "stack overflow", and + // trust that the kernel's own stack guard will work. + let stackptr = get_stack_start_aligned()?; + let stackaddr = stackptr.addr(); + Some(stackaddr - page_size..stackaddr) + } - Some(guardaddr..guardaddr + page_size) + unsafe fn install_main_guard_default(page_size: usize) -> Option> { + // Reallocate the last page of the stack. + // This ensures SIGBUS will be raised on + // stack overflow. + // Systems which enforce strict PAX MPROTECT do not allow + // to mprotect() a mapping with less restrictive permissions + // than the initial mmap() used, so we mmap() here with + // read/write permissions and only then mprotect() it to + // no permissions at all. See issue #50313. + let stackptr = get_stack_start_aligned()?; + let result = mmap64( + stackptr, + page_size, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON | MAP_FIXED, + -1, + 0, + ); + if result != stackptr || result == MAP_FAILED { + panic!("failed to allocate a guard page: {}", io::Error::last_os_error()); + } + + let result = mprotect(stackptr, page_size, PROT_NONE); + if result != 0 { + panic!("failed to protect the guard page: {}", io::Error::last_os_error()); } + + let guardaddr = stackptr.addr(); + + Some(guardaddr..guardaddr + page_size) } #[cfg(any(target_os = "macos", target_os = "openbsd", target_os = "solaris"))] From e285c95cee3b5aaa01d731df8a7096e028460eb9 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 23:37:18 -0700 Subject: [PATCH 09/22] unix: stack_start_aligned is a safe fn This function is purely informative, answering where a stack starts. This is a safe operation, even if an answer requires unsafe code, and even if the result is some unsafe code decides to trust the answer. It also doesn't need to fetch the PAGE_SIZE when its caller just did so! Let's complicate its signature and in doing so simplify its operation. This allows sprinkling around #[forbid(unsafe_op_in_unsafe_fn)] --- library/std/src/sys/pal/unix/stack_overflow.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 431fb9652a74e..5916be54fe846 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -306,9 +306,8 @@ mod imp { ret } - unsafe fn get_stack_start_aligned() -> Option<*mut libc::c_void> { - let page_size = PAGE_SIZE.load(Ordering::Relaxed); - let stackptr = get_stack_start()?; + fn stack_start_aligned(page_size: usize) -> Option<*mut libc::c_void> { + let stackptr = unsafe { get_stack_start()? }; let stackaddr = stackptr.addr(); // Ensure stackaddr is page aligned! A parent process might @@ -345,6 +344,7 @@ mod imp { } } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard_linux(page_size: usize) -> Option> { // Linux doesn't allocate the whole stack right away, and // the kernel has its own stack-guard mechanism to fault @@ -356,11 +356,12 @@ mod imp { // Instead, we'll just note where we expect rlimit to start // faulting, so our handler can report "stack overflow", and // trust that the kernel's own stack guard will work. - let stackptr = get_stack_start_aligned()?; + let stackptr = stack_start_aligned(page_size)?; let stackaddr = stackptr.addr(); Some(stackaddr - page_size..stackaddr) } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard_linux_musl(_page_size: usize) -> Option> { // For the main thread, the musl's pthread_attr_getstack // returns the current stack size, rather than maximum size @@ -374,7 +375,7 @@ mod imp { // at the bottom. If we try to remap the bottom of the stack // ourselves, FreeBSD's guard page moves upwards. So we'll just use // the builtin guard page. - let stackptr = get_stack_start_aligned()?; + let stackptr = stack_start_aligned(page_size)?; let guardaddr = stackptr.addr(); // Technically the number of guard pages is tunable and controlled // by the security.bsd.stack_guard_page sysctl. @@ -405,6 +406,7 @@ mod imp { Some(guardaddr..guardaddr + pages * page_size) } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard_bsds(page_size: usize) -> Option> { // OpenBSD stack already includes a guard page, and stack is // immutable. @@ -413,7 +415,7 @@ mod imp { // We'll just note where we expect rlimit to start // faulting, so our handler can report "stack overflow", and // trust that the kernel's own stack guard will work. - let stackptr = get_stack_start_aligned()?; + let stackptr = stack_start_aligned(page_size)?; let stackaddr = stackptr.addr(); Some(stackaddr - page_size..stackaddr) } @@ -427,7 +429,7 @@ mod imp { // than the initial mmap() used, so we mmap() here with // read/write permissions and only then mprotect() it to // no permissions at all. See issue #50313. - let stackptr = get_stack_start_aligned()?; + let stackptr = stack_start_aligned(page_size)?; let result = mmap64( stackptr, page_size, From 6ed563d49144f38b1066716833baf505646b274c Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Wed, 17 Jul 2024 00:06:00 -0700 Subject: [PATCH 10/22] unix: clean up install_main_guard_freebsd This just was a mess. --- .../std/src/sys/pal/unix/stack_overflow.rs | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 5916be54fe846..0e15049be9a1e 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -44,6 +44,7 @@ mod imp { use crate::ops::Range; use crate::ptr; use crate::sync::atomic::{AtomicBool, AtomicPtr, AtomicUsize, Ordering}; + use crate::sync::OnceLock; use crate::sys::pal::unix::os; use crate::thread; @@ -370,6 +371,7 @@ mod imp { None } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard_freebsd(page_size: usize) -> Option> { // FreeBSD's stack autogrows, and optionally includes a guard page // at the bottom. If we try to remap the bottom of the stack @@ -381,25 +383,22 @@ mod imp { // by the security.bsd.stack_guard_page sysctl. // By default it is 1, checking once is enough since it is // a boot time config value. - static PAGES: crate::sync::OnceLock = crate::sync::OnceLock::new(); + static PAGES: OnceLock = OnceLock::new(); let pages = PAGES.get_or_init(|| { use crate::sys::weak::dlsym; dlsym!(fn sysctlbyname(*const libc::c_char, *mut libc::c_void, *mut libc::size_t, *const libc::c_void, libc::size_t) -> libc::c_int); let mut guard: usize = 0; - let mut size = crate::mem::size_of_val(&guard); - let oid = crate::ffi::CStr::from_bytes_with_nul( - b"security.bsd.stack_guard_page\0", - ) - .unwrap(); + let mut size = mem::size_of_val(&guard); + let oid = c"security.bsd.stack_guard_page"; match sysctlbyname.get() { - Some(fcn) => { - if fcn(oid.as_ptr(), core::ptr::addr_of_mut!(guard) as *mut _, core::ptr::addr_of_mut!(size) as *mut _, crate::ptr::null_mut(), 0) == 0 { - guard - } else { - 1 - } - }, + Some(fcn) if unsafe { + fcn(oid.as_ptr(), + ptr::addr_of_mut!(guard).cast(), + ptr::addr_of_mut!(size), + ptr::null_mut(), + 0) == 0 + } => guard, _ => 1, } }); From d47cb26ddd6574de6c81caf9fdaadef1a108628a Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Wed, 17 Jul 2024 00:08:05 -0700 Subject: [PATCH 11/22] unix: unsafe-wrap install_main_guard_default --- .../std/src/sys/pal/unix/stack_overflow.rs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 0e15049be9a1e..0db08c1a926f5 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -419,6 +419,7 @@ mod imp { Some(stackaddr - page_size..stackaddr) } + #[forbid(unsafe_op_in_unsafe_fn)] unsafe fn install_main_guard_default(page_size: usize) -> Option> { // Reallocate the last page of the stack. // This ensures SIGBUS will be raised on @@ -429,19 +430,21 @@ mod imp { // read/write permissions and only then mprotect() it to // no permissions at all. See issue #50313. let stackptr = stack_start_aligned(page_size)?; - let result = mmap64( - stackptr, - page_size, - PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANON | MAP_FIXED, - -1, - 0, - ); + let result = unsafe { + mmap64( + stackptr, + page_size, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON | MAP_FIXED, + -1, + 0, + ) + }; if result != stackptr || result == MAP_FAILED { panic!("failed to allocate a guard page: {}", io::Error::last_os_error()); } - let result = mprotect(stackptr, page_size, PROT_NONE); + let result = unsafe { mprotect(stackptr, page_size, PROT_NONE) }; if result != 0 { panic!("failed to protect the guard page: {}", io::Error::last_os_error()); } From 21dc49c587c7df282b8e96dc01c288c7d59bbd2c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Jul 2024 13:49:32 +0200 Subject: [PATCH 12/22] ptr::metadata: avoid references to extern types --- library/core/src/ptr/metadata.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/core/src/ptr/metadata.rs b/library/core/src/ptr/metadata.rs index eb86bf6620652..21d1b7ea0ce77 100644 --- a/library/core/src/ptr/metadata.rs +++ b/library/core/src/ptr/metadata.rs @@ -5,6 +5,7 @@ use crate::hash::{Hash, Hasher}; use crate::intrinsics::aggregate_raw_ptr; use crate::intrinsics::ptr_metadata; use crate::marker::Freeze; +use crate::ptr::NonNull; /// Provides the pointer metadata type of any pointed-to type. /// @@ -153,7 +154,7 @@ pub const fn from_raw_parts_mut( /// compare equal (since identical vtables can be deduplicated within a codegen unit). #[lang = "dyn_metadata"] pub struct DynMetadata { - _vtable_ptr: &'static VTable, + _vtable_ptr: NonNull, _phantom: crate::marker::PhantomData, } @@ -174,7 +175,7 @@ impl DynMetadata { fn vtable_ptr(self) -> *const VTable { // SAFETY: this layout assumption is hard-coded into the compiler. // If it's somehow not a size match, the transmute will error. - unsafe { crate::mem::transmute::(self) } + unsafe { crate::mem::transmute::(self) } } /// Returns the size of the type associated with this vtable. From f9c0d3370f04d08cda2da268a54bc543eb310fc7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Jul 2024 13:50:17 +0200 Subject: [PATCH 13/22] ptr::metadata: update comment on vtable_ptr work-around --- library/core/src/ptr/metadata.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/library/core/src/ptr/metadata.rs b/library/core/src/ptr/metadata.rs index 21d1b7ea0ce77..06f205c0f2670 100644 --- a/library/core/src/ptr/metadata.rs +++ b/library/core/src/ptr/metadata.rs @@ -167,10 +167,13 @@ extern "C" { } impl DynMetadata { - /// One of the things that rustc_middle does with this being a lang item is - /// give it `FieldsShape::Primitive`, which means that as far as codegen can - /// tell, it *is* a reference, and thus doesn't have any fields. - /// That means we can't use field access, and have to transmute it instead. + /// When `DynMetadata` appears as the metadata field of a wide pointer, the rustc_middle layout + /// computation does magic and the resulting layout is *not* a `FieldsShape::Aggregate`, instead + /// it is a `FieldsShape::Primitive`. This means that the same type can have different layout + /// depending on whether it appears as the metadata field of a wide pointer or as a stand-alone + /// type, which understandably confuses codegen and leads to ICEs when trying to project to a + /// field of `DynMetadata`. To work around that issue, we use `transmute` instead of using a + /// field projection. #[inline] fn vtable_ptr(self) -> *const VTable { // SAFETY: this layout assumption is hard-coded into the compiler. From 99f879c32f43e87366a2888f146799bb659e4b21 Mon Sep 17 00:00:00 2001 From: Kriskras99 Date: Wed, 17 Jul 2024 14:10:41 +0200 Subject: [PATCH 14/22] Document the column numbers for the dbg! macro The line numbers were also made consistent, some examples used the line numbers as shown on the playground while others used the line numbers that you would expect when just seeing the documentation. The second option was chosen to make everything consistent. --- library/std/src/macros.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/library/std/src/macros.rs b/library/std/src/macros.rs index 972b6015932db..ba519afc62b07 100644 --- a/library/std/src/macros.rs +++ b/library/std/src/macros.rs @@ -230,7 +230,7 @@ macro_rules! eprintln { /// ```rust /// let a = 2; /// let b = dbg!(a * 2) + 1; -/// // ^-- prints: [src/main.rs:2] a * 2 = 4 +/// // ^-- prints: [src/main.rs:2:9] a * 2 = 4 /// assert_eq!(b, 5); /// ``` /// @@ -281,7 +281,7 @@ macro_rules! eprintln { /// This prints to [stderr]: /// /// ```text,ignore -/// [src/main.rs:4] n.checked_sub(4) = None +/// [src/main.rs:2:22] n.checked_sub(4) = None /// ``` /// /// Naive factorial implementation: @@ -301,15 +301,15 @@ macro_rules! eprintln { /// This prints to [stderr]: /// /// ```text,ignore -/// [src/main.rs:3] n <= 1 = false -/// [src/main.rs:3] n <= 1 = false -/// [src/main.rs:3] n <= 1 = false -/// [src/main.rs:3] n <= 1 = true -/// [src/main.rs:4] 1 = 1 -/// [src/main.rs:5] n * factorial(n - 1) = 2 -/// [src/main.rs:5] n * factorial(n - 1) = 6 -/// [src/main.rs:5] n * factorial(n - 1) = 24 -/// [src/main.rs:11] factorial(4) = 24 +/// [src/main.rs:2:8] n <= 1 = false +/// [src/main.rs:2:8] n <= 1 = false +/// [src/main.rs:2:8] n <= 1 = false +/// [src/main.rs:2:8] n <= 1 = true +/// [src/main.rs:3:9] 1 = 1 +/// [src/main.rs:7:9] n * factorial(n - 1) = 2 +/// [src/main.rs:7:9] n * factorial(n - 1) = 6 +/// [src/main.rs:7:9] n * factorial(n - 1) = 24 +/// [src/main.rs:9:1] factorial(4) = 24 /// ``` /// /// The `dbg!(..)` macro moves the input: From 1d40d4c4f496ce26d2cbf268b2f415e1f3bd5616 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 12 Jul 2024 16:18:51 -0400 Subject: [PATCH 15/22] Fix precise capturing suggestion for hidden type when APITs are involved --- compiler/rustc_infer/messages.ftl | 5 + .../src/error_reporting/infer/region.rs | 104 +++++++++++++++--- compiler/rustc_infer/src/errors/mod.rs | 22 ++++ .../hidden-type-suggestion.rs | 12 ++ .../hidden-type-suggestion.stderr | 44 +++++++- 5 files changed, 170 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_infer/messages.ftl b/compiler/rustc_infer/messages.ftl index 7a5e71599203e..c279195a7e99c 100644 --- a/compiler/rustc_infer/messages.ftl +++ b/compiler/rustc_infer/messages.ftl @@ -225,6 +225,8 @@ infer_outlives_content = lifetime of reference outlives lifetime of borrowed con infer_precise_capturing_existing = add `{$new_lifetime}` to the `use<...>` bound to explicitly capture it infer_precise_capturing_new = add a `use<...>` bound to explicitly capture `{$new_lifetime}` +infer_precise_capturing_new_but_apit = add a `use<...>` bound to explicitly capture `{$new_lifetime}` after turning all argument-position `impl Trait` into type parameters, noting that this possibly affects the API of this crate + infer_prlf_defined_with_sub = the lifetime `{$sub_symbol}` defined here... infer_prlf_defined_without_sub = the lifetime defined here... infer_prlf_known_limitation = this is a known limitation that will be removed in the future (see issue #100013 for more information) @@ -387,6 +389,9 @@ infer_type_annotations_needed = {$source_kind -> .label = type must be known at this point infer_types_declared_different = these two types are declared with different lifetimes... + +infer_warn_removing_apit_params = you could use a `use<...>` bound to explicitly capture `{$new_lifetime}`, but argument-position `impl Trait`s are not nameable + infer_where_copy_predicates = copy the `where` clause predicates from the trait infer_where_remove = remove the `where` clause diff --git a/compiler/rustc_infer/src/error_reporting/infer/region.rs b/compiler/rustc_infer/src/error_reporting/infer/region.rs index 093d2d3d743d0..5d41bb5d2710c 100644 --- a/compiler/rustc_infer/src/error_reporting/infer/region.rs +++ b/compiler/rustc_infer/src/error_reporting/infer/region.rs @@ -1269,9 +1269,13 @@ fn suggest_precise_capturing<'tcx>( captured_lifetime: ty::Region<'tcx>, diag: &mut Diag<'_>, ) { - let hir::OpaqueTy { bounds, .. } = + let hir::OpaqueTy { bounds, origin, .. } = tcx.hir_node_by_def_id(opaque_def_id).expect_item().expect_opaque_ty(); + let hir::OpaqueTyOrigin::FnReturn(fn_def_id) = *origin else { + return; + }; + let new_lifetime = Symbol::intern(&captured_lifetime.to_string()); if let Some((args, span)) = bounds.iter().find_map(|bound| match bound { @@ -1306,6 +1310,7 @@ fn suggest_precise_capturing<'tcx>( let variances = tcx.variances_of(opaque_def_id); let mut generics = tcx.generics_of(opaque_def_id); + let mut synthetics = vec![]; loop { for param in &generics.own_params { if variances[param.index as usize] == ty::Bivariant { @@ -1317,9 +1322,7 @@ fn suggest_precise_capturing<'tcx>( captured_lifetimes.insert(param.name); } ty::GenericParamDefKind::Type { synthetic: true, .. } => { - // FIXME: We can't provide a good suggestion for - // `use<...>` if we have an APIT. Bail for now. - return; + synthetics.push((tcx.def_span(param.def_id), param.name)); } ty::GenericParamDefKind::Type { .. } | ty::GenericParamDefKind::Const { .. } => { @@ -1340,17 +1343,86 @@ fn suggest_precise_capturing<'tcx>( return; } - let concatenated_bounds = captured_lifetimes - .into_iter() - .chain(captured_non_lifetimes) - .map(|sym| sym.to_string()) - .collect::>() - .join(", "); - - diag.subdiagnostic(errors::AddPreciseCapturing::New { - span: tcx.def_span(opaque_def_id).shrink_to_hi(), - new_lifetime, - concatenated_bounds, - }); + if synthetics.is_empty() { + let concatenated_bounds = captured_lifetimes + .into_iter() + .chain(captured_non_lifetimes) + .map(|sym| sym.to_string()) + .collect::>() + .join(", "); + + diag.subdiagnostic(errors::AddPreciseCapturing::New { + span: tcx.def_span(opaque_def_id).shrink_to_hi(), + new_lifetime, + concatenated_bounds, + }); + } else { + let mut next_fresh_param = || { + ["T", "U", "V", "W", "X", "Y", "A", "B", "C"] + .into_iter() + .map(Symbol::intern) + .chain((0..).map(|i| Symbol::intern(&format!("T{i}")))) + .find(|s| captured_non_lifetimes.insert(*s)) + .unwrap() + }; + + let mut new_params = String::new(); + let mut suggs = vec![]; + let mut apit_spans = vec![]; + + for (i, (span, name)) in synthetics.into_iter().enumerate() { + apit_spans.push(span); + + let fresh_param = next_fresh_param(); + + // Suggest renaming. + suggs.push((span, fresh_param.to_string())); + + // Super jank. Turn `impl Trait` into `T: Trait`. + // + // This currently involves stripping the `impl` from the name of + // the parameter, since APITs are always named after how they are + // rendered in the AST. This sucks! But to recreate the bound list + // from the APIT itself would be miserable, so we're stuck with + // this for now! + if i > 0 { + new_params += ", "; + } + let name_as_bounds = name.as_str().trim_start_matches("impl").trim_start(); + new_params += fresh_param.as_str(); + new_params += ": "; + new_params += name_as_bounds; + } + + let Some(generics) = tcx.hir().get_generics(fn_def_id) else { + // This shouldn't happen, but don't ICE. + return; + }; + + // Add generics or concatenate to the end of the list. + suggs.push(if let Some(params_span) = generics.span_for_param_suggestion() { + (params_span, format!(", {new_params}")) + } else { + (generics.span, format!("<{new_params}>")) + }); + + let concatenated_bounds = captured_lifetimes + .into_iter() + .chain(captured_non_lifetimes) + .map(|sym| sym.to_string()) + .collect::>() + .join(", "); + + suggs.push(( + tcx.def_span(opaque_def_id).shrink_to_hi(), + format!(" + use<{concatenated_bounds}>"), + )); + + diag.subdiagnostic(errors::AddPreciseCapturingAndParams { + suggs, + new_lifetime, + apit_spans, + }); + } } } diff --git a/compiler/rustc_infer/src/errors/mod.rs b/compiler/rustc_infer/src/errors/mod.rs index f849a1a732249..2ce712e0bff58 100644 --- a/compiler/rustc_infer/src/errors/mod.rs +++ b/compiler/rustc_infer/src/errors/mod.rs @@ -1609,3 +1609,25 @@ pub enum AddPreciseCapturing { post: &'static str, }, } + +pub struct AddPreciseCapturingAndParams { + pub suggs: Vec<(Span, String)>, + pub new_lifetime: Symbol, + pub apit_spans: Vec, +} + +impl Subdiagnostic for AddPreciseCapturingAndParams { + fn add_to_diag_with>( + self, + diag: &mut Diag<'_, G>, + _f: &F, + ) { + diag.arg("new_lifetime", self.new_lifetime); + diag.multipart_suggestion_verbose( + fluent::infer_precise_capturing_new_but_apit, + self.suggs, + Applicability::MaybeIncorrect, + ); + diag.span_note(self.apit_spans, fluent::infer_warn_removing_apit_params); + } +} diff --git a/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.rs b/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.rs index e0b115b0ce412..b50780643f1cc 100644 --- a/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.rs +++ b/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.rs @@ -27,4 +27,16 @@ fn missing<'a, 'captured, 'not_captured, Captured>(x: &'a ()) -> impl Captures<' //~^ ERROR hidden type for } +fn no_params_yet(_: impl Sized, y: &()) -> impl Sized { +//~^ HELP add a `use<...>` bound + y +//~^ ERROR hidden type for +} + +fn yes_params_yet<'a, T>(_: impl Sized, y: &'a ()) -> impl Sized { +//~^ HELP add a `use<...>` bound + y +//~^ ERROR hidden type for +} + fn main() {} diff --git a/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.stderr b/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.stderr index 391f16d012e4b..1007a835894e1 100644 --- a/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.stderr +++ b/tests/ui/impl-trait/precise-capturing/hidden-type-suggestion.stderr @@ -62,6 +62,48 @@ help: add a `use<...>` bound to explicitly capture `'a` LL | fn missing<'a, 'captured, 'not_captured, Captured>(x: &'a ()) -> impl Captures<'captured> + use<'captured, 'a, Captured> { | ++++++++++++++++++++++++++++++ -error: aborting due to 4 previous errors +error[E0700]: hidden type for `impl Sized` captures lifetime that does not appear in bounds + --> $DIR/hidden-type-suggestion.rs:32:5 + | +LL | fn no_params_yet(_: impl Sized, y: &()) -> impl Sized { + | --- ---------- opaque type defined here + | | + | hidden type `&()` captures the anonymous lifetime defined here +LL | +LL | y + | ^ + | +note: you could use a `use<...>` bound to explicitly capture `'_`, but argument-position `impl Trait`s are not nameable + --> $DIR/hidden-type-suggestion.rs:30:21 + | +LL | fn no_params_yet(_: impl Sized, y: &()) -> impl Sized { + | ^^^^^^^^^^ +help: add a `use<...>` bound to explicitly capture `'_` after turning all argument-position `impl Trait` into type parameters, noting that this possibly affects the API of this crate + | +LL | fn no_params_yet(_: T, y: &()) -> impl Sized + use<'_, T> { + | ++++++++++ ~ ++++++++++++ + +error[E0700]: hidden type for `impl Sized` captures lifetime that does not appear in bounds + --> $DIR/hidden-type-suggestion.rs:38:5 + | +LL | fn yes_params_yet<'a, T>(_: impl Sized, y: &'a ()) -> impl Sized { + | -- ---------- opaque type defined here + | | + | hidden type `&'a ()` captures the lifetime `'a` as defined here +LL | +LL | y + | ^ + | +note: you could use a `use<...>` bound to explicitly capture `'a`, but argument-position `impl Trait`s are not nameable + --> $DIR/hidden-type-suggestion.rs:36:29 + | +LL | fn yes_params_yet<'a, T>(_: impl Sized, y: &'a ()) -> impl Sized { + | ^^^^^^^^^^ +help: add a `use<...>` bound to explicitly capture `'a` after turning all argument-position `impl Trait` into type parameters, noting that this possibly affects the API of this crate + | +LL | fn yes_params_yet<'a, T, U: Sized>(_: U, y: &'a ()) -> impl Sized + use<'a, T, U> { + | ++++++++++ ~ +++++++++++++++ + +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0700`. From f3f0b572640e32297d58b4edf36ef69c62e47305 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Tue, 16 Jul 2024 15:08:18 -0500 Subject: [PATCH 16/22] Commonize `uname -m` results for `aarch64` in docker runner `uname -m` on Linux reports `aarch64`, but on MacOS reports `arm64`. Commonize this to `aarch64`. With this fix, it is now possible to run aarch64 CI docker images on Arm MacOS. --- src/ci/docker/run.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/ci/docker/run.sh b/src/ci/docker/run.sh index 40f421714118e..fad4b5af0953f 100755 --- a/src/ci/docker/run.sh +++ b/src/ci/docker/run.sh @@ -27,8 +27,11 @@ do shift done +# MacOS reports "arm64" while Linux reports "aarch64". Commonize this. +machine="$(uname -m | sed 's/arm64/aarch64/')" + script_dir="`dirname $script`" -docker_dir="${script_dir}/host-$(uname -m)" +docker_dir="${script_dir}/host-${machine}" ci_dir="`dirname $script_dir`" src_dir="`dirname $ci_dir`" root_dir="`dirname $src_dir`" @@ -68,7 +71,7 @@ if [ -f "$docker_dir/$image/Dockerfile" ]; then # Include the architecture in the hash key, since our Linux CI does not # only run in x86_64 machines. - uname -m >> $hash_key + echo "$machine" >> $hash_key # Include cache version. Can be used to manually bust the Docker cache. echo "2" >> $hash_key @@ -178,7 +181,7 @@ elif [ -f "$docker_dir/disabled/$image/Dockerfile" ]; then build \ --rm \ -t rust-ci \ - -f "host-$(uname -m)/$image/Dockerfile" \ + -f "host-${machine}/$image/Dockerfile" \ - else echo Invalid image: $image @@ -201,7 +204,7 @@ else else continue fi - echo "Note: the current host architecture is $(uname -m)" + echo "Note: the current host architecture is $machine" done exit 1 From e1852d0a3c657be38bfcf9b964983fcef2c6c42e Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 12 Jul 2024 14:25:31 +0200 Subject: [PATCH 17/22] Unignore cg_gcc fmt --- rustfmt.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rustfmt.toml b/rustfmt.toml index e060fd8fe8bfa..3110ba7be09b0 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -31,7 +31,6 @@ ignore = [ "library/backtrace", "library/portable-simd", "library/stdarch", - "compiler/rustc_codegen_gcc", "src/doc/book", "src/doc/edition-guide", "src/doc/embedded-book", @@ -50,4 +49,7 @@ ignore = [ # These are ignored by a standard cargo fmt run. "compiler/rustc_codegen_cranelift/scripts", "compiler/rustc_codegen_cranelift/example/gen_block_iterate.rs", # uses edition 2024 + "compiler/rustc_codegen_gcc/tests", + # Code automatically generated and included. + "compiler/rustc_codegen_gcc/src/intrinsic/archs.rs", ] From 261d92c07dc792ad59753363ce5cadfcbe05d7c8 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 17 Jul 2024 20:17:44 +0200 Subject: [PATCH 18/22] Align cg_gcc rustfmt.toml with rust's --- compiler/rustc_codegen_gcc/.rustfmt.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/rustc_codegen_gcc/.rustfmt.toml b/compiler/rustc_codegen_gcc/.rustfmt.toml index 2a35f0230c690..725aec25a0718 100644 --- a/compiler/rustc_codegen_gcc/.rustfmt.toml +++ b/compiler/rustc_codegen_gcc/.rustfmt.toml @@ -1 +1,3 @@ +version = "Two" use_small_heuristics = "Max" +merge_derives = false From 12bedc3e2b81a7d74fe2986d1582644fb8ed558b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 17 Jul 2024 20:21:52 +0200 Subject: [PATCH 19/22] Ignore files in cg_gcc example folder --- rustfmt.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/rustfmt.toml b/rustfmt.toml index 3110ba7be09b0..8c1d3b94f195b 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -52,4 +52,5 @@ ignore = [ "compiler/rustc_codegen_gcc/tests", # Code automatically generated and included. "compiler/rustc_codegen_gcc/src/intrinsic/archs.rs", + "compiler/rustc_codegen_gcc/example", ] From 213782dd715452afbf2af1c8f6ea5ae6a93d000c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 17 Jul 2024 20:22:07 +0200 Subject: [PATCH 20/22] Format cg_gcc with same formatting parameters --- .../build_system/src/clone_gcc.rs | 2 +- .../build_system/src/config.rs | 14 +++++++------- .../rustc_codegen_gcc/build_system/src/test.rs | 14 +++----------- .../build_system/src/utils.rs | 6 +----- compiler/rustc_codegen_gcc/src/abi.rs | 6 +----- compiler/rustc_codegen_gcc/src/asm.rs | 6 +----- compiler/rustc_codegen_gcc/src/builder.rs | 18 +++--------------- compiler/rustc_codegen_gcc/src/common.rs | 6 +----- 8 files changed, 18 insertions(+), 54 deletions(-) diff --git a/compiler/rustc_codegen_gcc/build_system/src/clone_gcc.rs b/compiler/rustc_codegen_gcc/build_system/src/clone_gcc.rs index aee46afaeb040..cbf590c0c321a 100644 --- a/compiler/rustc_codegen_gcc/build_system/src/clone_gcc.rs +++ b/compiler/rustc_codegen_gcc/build_system/src/clone_gcc.rs @@ -34,7 +34,7 @@ impl Args { "--out-path" => match args.next() { Some(path) if !path.is_empty() => out_path = Some(path), _ => { - return Err("Expected an argument after `--out-path`, found nothing".into()) + return Err("Expected an argument after `--out-path`, found nothing".into()); } }, "--help" => { diff --git a/compiler/rustc_codegen_gcc/build_system/src/config.rs b/compiler/rustc_codegen_gcc/build_system/src/config.rs index 965aedd8be891..bbb711c8428b6 100644 --- a/compiler/rustc_codegen_gcc/build_system/src/config.rs +++ b/compiler/rustc_codegen_gcc/build_system/src/config.rs @@ -54,7 +54,7 @@ impl ConfigFile { config.gcc_path = Some(value.as_str().to_string()) } ("gcc-path", _) => { - return failed_config_parsing(config_file, "Expected a string for `gcc-path`") + return failed_config_parsing(config_file, "Expected a string for `gcc-path`"); } ("download-gccjit", TomlValue::Boolean(value)) => { config.download_gccjit = Some(*value) @@ -63,7 +63,7 @@ impl ConfigFile { return failed_config_parsing( config_file, "Expected a boolean for `download-gccjit`", - ) + ); } _ => return failed_config_parsing(config_file, &format!("Unknown key `{}`", key)), } @@ -73,7 +73,7 @@ impl ConfigFile { return failed_config_parsing( config_file, "At least one of `gcc-path` or `download-gccjit` value must be set", - ) + ); } (Some(_), Some(true)) => { println!( @@ -144,7 +144,7 @@ impl ConfigInfo { _ => { return Err( "Expected a value after `--target-triple`, found nothing".to_string() - ) + ); } }, "--out-dir" => match args.next() { @@ -158,7 +158,7 @@ impl ConfigInfo { self.config_file = Some(arg.to_string()); } _ => { - return Err("Expected a value after `--config-file`, found nothing".to_string()) + return Err("Expected a value after `--config-file`, found nothing".to_string()); } }, "--release-sysroot" => self.sysroot_release_channel = true, @@ -169,7 +169,7 @@ impl ConfigInfo { self.cg_gcc_path = Some(arg.into()); } _ => { - return Err("Expected a value after `--cg_gcc-path`, found nothing".to_string()) + return Err("Expected a value after `--cg_gcc-path`, found nothing".to_string()); } }, "--use-backend" => match args.next() { @@ -277,7 +277,7 @@ impl ConfigInfo { self.gcc_path = match gcc_path { Some(path) => path, None => { - return Err(format!("missing `gcc-path` value from `{}`", config_file.display(),)) + return Err(format!("missing `gcc-path` value from `{}`", config_file.display(),)); } }; Ok(()) diff --git a/compiler/rustc_codegen_gcc/build_system/src/test.rs b/compiler/rustc_codegen_gcc/build_system/src/test.rs index 8d088a3aac318..06f28d13fb3ab 100644 --- a/compiler/rustc_codegen_gcc/build_system/src/test.rs +++ b/compiler/rustc_codegen_gcc/build_system/src/test.rs @@ -109,7 +109,7 @@ impl TestArg { test_arg.flags.extend_from_slice(&["--features".into(), feature]); } _ => { - return Err("Expected an argument after `--features`, found nothing".into()) + return Err("Expected an argument after `--features`, found nothing".into()); } }, "--use-system-gcc" => { @@ -458,11 +458,7 @@ fn setup_rustc(env: &mut Env, args: &TestArg) -> Result { .map_err(|error| format!("Failed to retrieve cargo path: {:?}", error)) .and_then(|cargo| { let cargo = cargo.trim().to_owned(); - if cargo.is_empty() { - Err(format!("`cargo` path is empty")) - } else { - Ok(cargo) - } + if cargo.is_empty() { Err(format!("`cargo` path is empty")) } else { Ok(cargo) } })?; let rustc = String::from_utf8( run_command_with_env(&[&"rustup", &toolchain, &"which", &"rustc"], rust_dir, Some(env))? @@ -471,11 +467,7 @@ fn setup_rustc(env: &mut Env, args: &TestArg) -> Result { .map_err(|error| format!("Failed to retrieve rustc path: {:?}", error)) .and_then(|rustc| { let rustc = rustc.trim().to_owned(); - if rustc.is_empty() { - Err(format!("`rustc` path is empty")) - } else { - Ok(rustc) - } + if rustc.is_empty() { Err(format!("`rustc` path is empty")) } else { Ok(rustc) } })?; let llvm_filecheck = match run_command_with_env( &[ diff --git a/compiler/rustc_codegen_gcc/build_system/src/utils.rs b/compiler/rustc_codegen_gcc/build_system/src/utils.rs index 3bba8df6c6504..e338d1b4992e8 100644 --- a/compiler/rustc_codegen_gcc/build_system/src/utils.rs +++ b/compiler/rustc_codegen_gcc/build_system/src/utils.rs @@ -175,11 +175,7 @@ pub fn cargo_install(to_install: &str) -> Result<(), String> { pub fn get_os_name() -> Result { let output = run_command(&[&"uname"], None)?; let name = std::str::from_utf8(&output.stdout).unwrap_or("").trim().to_string(); - if !name.is_empty() { - Ok(name) - } else { - Err("Failed to retrieve the OS name".to_string()) - } + if !name.is_empty() { Ok(name) } else { Err("Failed to retrieve the OS name".to_string()) } } #[derive(Default, PartialEq)] diff --git a/compiler/rustc_codegen_gcc/src/abi.rs b/compiler/rustc_codegen_gcc/src/abi.rs index 166dd080cf209..0a99e7213be56 100644 --- a/compiler/rustc_codegen_gcc/src/abi.rs +++ b/compiler/rustc_codegen_gcc/src/abi.rs @@ -26,11 +26,7 @@ impl<'a, 'gcc, 'tcx> AbiBuilderMethods<'tcx> for Builder<'a, 'gcc, 'tcx> { } else { false }; - if on_stack { - param.to_lvalue().get_address(None) - } else { - param.to_rvalue() - } + if on_stack { param.to_lvalue().get_address(None) } else { param.to_rvalue() } } } diff --git a/compiler/rustc_codegen_gcc/src/asm.rs b/compiler/rustc_codegen_gcc/src/asm.rs index aa485846cd429..1da691252ab94 100644 --- a/compiler/rustc_codegen_gcc/src/asm.rs +++ b/compiler/rustc_codegen_gcc/src/asm.rs @@ -858,11 +858,7 @@ fn modifier_to_gcc( InlineAsmRegClass::AArch64(AArch64InlineAsmRegClass::reg) => modifier, InlineAsmRegClass::AArch64(AArch64InlineAsmRegClass::vreg) | InlineAsmRegClass::AArch64(AArch64InlineAsmRegClass::vreg_low16) => { - if modifier == Some('v') { - None - } else { - modifier - } + if modifier == Some('v') { None } else { modifier } } InlineAsmRegClass::AArch64(AArch64InlineAsmRegClass::preg) => { unreachable!("clobber-only") diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 307348f595dc9..b9e4bd79fe1e6 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -1043,11 +1043,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { let llty = place.layout.scalar_pair_element_gcc_type(self, i); let load = self.load(llty, llptr, align); scalar_load_metadata(self, load, scalar); - if scalar.is_bool() { - self.trunc(load, self.type_i1()) - } else { - load - } + if scalar.is_bool() { self.trunc(load, self.type_i1()) } else { load } }; OperandValue::Pair( @@ -1795,18 +1791,10 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> { // This already happens today with u128::MAX = 2^128 - 1 > f32::MAX. let int_max = |signed: bool, int_width: u64| -> u128 { let shift_amount = 128 - int_width; - if signed { - i128::MAX as u128 >> shift_amount - } else { - u128::MAX >> shift_amount - } + if signed { i128::MAX as u128 >> shift_amount } else { u128::MAX >> shift_amount } }; let int_min = |signed: bool, int_width: u64| -> i128 { - if signed { - i128::MIN >> (128 - int_width) - } else { - 0 - } + if signed { i128::MIN >> (128 - int_width) } else { 0 } }; let compute_clamp_bounds_single = |signed: bool, int_width: u64| -> (u128, u128) { diff --git a/compiler/rustc_codegen_gcc/src/common.rs b/compiler/rustc_codegen_gcc/src/common.rs index 19333689aaa9f..70f0dc37e39da 100644 --- a/compiler/rustc_codegen_gcc/src/common.rs +++ b/compiler/rustc_codegen_gcc/src/common.rs @@ -58,11 +58,7 @@ pub fn type_is_pointer(typ: Type<'_>) -> bool { impl<'gcc, 'tcx> ConstMethods<'tcx> for CodegenCx<'gcc, 'tcx> { fn const_null(&self, typ: Type<'gcc>) -> RValue<'gcc> { - if type_is_pointer(typ) { - self.context.new_null(typ) - } else { - self.const_int(typ, 0) - } + if type_is_pointer(typ) { self.context.new_null(typ) } else { self.const_int(typ, 0) } } fn const_undef(&self, typ: Type<'gcc>) -> RValue<'gcc> { From 553279b152a9694914dd03e3469f7ef6c01c9e28 Mon Sep 17 00:00:00 2001 From: Caio Date: Wed, 17 Jul 2024 17:00:48 -0300 Subject: [PATCH 21/22] Add support for literals --- compiler/rustc_expand/src/mbe/metavar_expr.rs | 2 + compiler/rustc_expand/src/mbe/transcribe.rs | 59 ++++--- .../allowed-operations.rs | 61 ++++++- .../syntax-errors.rs | 54 ++++++- .../syntax-errors.stderr | 153 +++++++++++++++++- .../unicode-expansion.rs | 11 +- 6 files changed, 302 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_expand/src/mbe/metavar_expr.rs b/compiler/rustc_expand/src/mbe/metavar_expr.rs index dbbd948fd7073..2964ac8cc5854 100644 --- a/compiler/rustc_expand/src/mbe/metavar_expr.rs +++ b/compiler/rustc_expand/src/mbe/metavar_expr.rs @@ -119,6 +119,8 @@ impl MetaVarExpr { } } +/// Indicates what is placed in a `concat` parameter. For example, literals +/// (`${concat("foo", "bar")}`) or adhoc identifiers (`${concat(foo, bar)}`). #[derive(Debug, Decodable, Encodable, PartialEq)] pub(crate) enum MetaVarExprConcatElem { /// Identifier WITHOUT a preceding dollar sign, which means that this identifier should be diff --git a/compiler/rustc_expand/src/mbe/transcribe.rs b/compiler/rustc_expand/src/mbe/transcribe.rs index 9b4dc13c703a1..7e2ea8de5fca2 100644 --- a/compiler/rustc_expand/src/mbe/transcribe.rs +++ b/compiler/rustc_expand/src/mbe/transcribe.rs @@ -6,9 +6,10 @@ use crate::mbe::macro_parser::{NamedMatch, NamedMatch::*}; use crate::mbe::metavar_expr::{MetaVarExprConcatElem, RAW_IDENT_ERR}; use crate::mbe::{self, KleeneOp, MetaVarExpr}; use rustc_ast::mut_visit::{self, MutVisitor}; -use rustc_ast::token::IdentIsRaw; -use rustc_ast::token::{self, Delimiter, Token, TokenKind}; +use rustc_ast::token::{self, Delimiter, Nonterminal, Token, TokenKind}; +use rustc_ast::token::{IdentIsRaw, Lit, LitKind}; use rustc_ast::tokenstream::{DelimSpacing, DelimSpan, Spacing, TokenStream, TokenTree}; +use rustc_ast::ExprKind; use rustc_data_structures::fx::FxHashMap; use rustc_errors::{pluralize, Diag, DiagCtxtHandle, PResult}; use rustc_parse::lexer::nfc_normalize; @@ -17,7 +18,7 @@ use rustc_session::parse::ParseSess; use rustc_session::parse::SymbolGallery; use rustc_span::hygiene::{LocalExpnId, Transparency}; use rustc_span::symbol::{sym, Ident, MacroRulesNormalizedIdent}; -use rustc_span::{with_metavar_spans, Span, SyntaxContext}; +use rustc_span::{with_metavar_spans, Span, Symbol, SyntaxContext}; use smallvec::{smallvec, SmallVec}; use std::mem; @@ -691,12 +692,12 @@ fn transcribe_metavar_expr<'a>( MetaVarExpr::Concat(ref elements) => { let mut concatenated = String::new(); for element in elements.into_iter() { - let string = match element { - MetaVarExprConcatElem::Ident(elem) => elem.to_string(), - MetaVarExprConcatElem::Literal(elem) => elem.as_str().into(), - MetaVarExprConcatElem::Var(elem) => extract_ident(dcx, *elem, interp)?, + let symbol = match element { + MetaVarExprConcatElem::Ident(elem) => elem.name, + MetaVarExprConcatElem::Literal(elem) => *elem, + MetaVarExprConcatElem::Var(elem) => extract_var_symbol(dcx, *elem, interp)?, }; - concatenated.push_str(&string); + concatenated.push_str(symbol.as_str()); } let symbol = nfc_normalize(&concatenated); let concatenated_span = visited_span(); @@ -750,32 +751,42 @@ fn transcribe_metavar_expr<'a>( Ok(()) } -/// Extracts an identifier that can be originated from a `$var:ident` variable or from a token tree. -fn extract_ident<'a>( +/// Extracts an metavariable symbol that can be an identifier, a token tree or a literal. +fn extract_var_symbol<'a>( dcx: DiagCtxtHandle<'a>, ident: Ident, interp: &FxHashMap, -) -> PResult<'a, String> { +) -> PResult<'a, Symbol> { if let NamedMatch::MatchedSingle(pnr) = matched_from_ident(dcx, ident, interp)? { if let ParseNtResult::Ident(nt_ident, is_raw) = pnr { if let IdentIsRaw::Yes = is_raw { return Err(dcx.struct_span_err(ident.span, RAW_IDENT_ERR)); } - return Ok(nt_ident.to_string()); + return Ok(nt_ident.name); } - if let ParseNtResult::Tt(TokenTree::Token( - Token { kind: TokenKind::Ident(token_ident, is_raw), .. }, - _, - )) = pnr - { - if let IdentIsRaw::Yes = is_raw { - return Err(dcx.struct_span_err(ident.span, RAW_IDENT_ERR)); + + if let ParseNtResult::Tt(TokenTree::Token(Token { kind, .. }, _)) = pnr { + if let TokenKind::Ident(symbol, is_raw) = kind { + if let IdentIsRaw::Yes = is_raw { + return Err(dcx.struct_span_err(ident.span, RAW_IDENT_ERR)); + } + return Ok(*symbol); } - return Ok(token_ident.to_string()); + + if let TokenKind::Literal(Lit { kind: LitKind::Str, symbol, suffix: None }) = kind { + return Ok(*symbol); + } + } + + if let ParseNtResult::Nt(nt) = pnr + && let Nonterminal::NtLiteral(expr) = &**nt + && let ExprKind::Lit(Lit { kind: LitKind::Str, symbol, suffix: None }) = &expr.kind + { + return Ok(*symbol); } } - Err(dcx.struct_span_err( - ident.span, - "`${concat(..)}` currently only accepts identifiers or meta-variables as parameters", - )) + Err(dcx + .struct_err("metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt`") + .with_note("currently only string literals are supported") + .with_span(ident.span)) } diff --git a/tests/ui/macros/macro-metavar-expr-concat/allowed-operations.rs b/tests/ui/macros/macro-metavar-expr-concat/allowed-operations.rs index 1acefa314aab8..695a752fe17d4 100644 --- a/tests/ui/macros/macro-metavar-expr-concat/allowed-operations.rs +++ b/tests/ui/macros/macro-metavar-expr-concat/allowed-operations.rs @@ -1,6 +1,6 @@ //@ run-pass -#![allow(dead_code, non_camel_case_types, non_upper_case_globals)] +#![allow(dead_code, non_camel_case_types, non_upper_case_globals, unused_variables)] #![feature(macro_metavar_expr_concat)] macro_rules! create_things { @@ -37,13 +37,58 @@ macro_rules! without_dollar_sign_is_an_ident { }; } -macro_rules! literals { - ($ident:ident) => {{ - let ${concat(_a, "_b")}: () = (); - let ${concat("_b", _a)}: () = (); +macro_rules! combinations { + ($ident:ident, $literal:literal, $tt_ident:tt, $tt_literal:tt) => {{ + // tt ident + let ${concat($tt_ident, b)} = (); + let ${concat($tt_ident, _b)} = (); + let ${concat($tt_ident, "b")} = (); + let ${concat($tt_ident, $tt_ident)} = (); + let ${concat($tt_ident, $tt_literal)} = (); + let ${concat($tt_ident, $ident)} = (); + let ${concat($tt_ident, $literal)} = (); + // tt literal + let ${concat($tt_literal, b)} = (); + let ${concat($tt_literal, _b)} = (); + let ${concat($tt_literal, "b")} = (); + let ${concat($tt_literal, $tt_ident)} = (); + let ${concat($tt_literal, $tt_literal)} = (); + let ${concat($tt_literal, $ident)} = (); + let ${concat($tt_literal, $literal)} = (); - let ${concat($ident, "_b")}: () = (); - let ${concat("_b", $ident)}: () = (); + // ident (adhoc) + let ${concat(_b, b)} = (); + let ${concat(_b, _b)} = (); + let ${concat(_b, "b")} = (); + let ${concat(_b, $tt_ident)} = (); + let ${concat(_b, $tt_literal)} = (); + let ${concat(_b, $ident)} = (); + let ${concat(_b, $literal)} = (); + // ident (param) + let ${concat($ident, b)} = (); + let ${concat($ident, _b)} = (); + let ${concat($ident, "b")} = (); + let ${concat($ident, $tt_ident)} = (); + let ${concat($ident, $tt_literal)} = (); + let ${concat($ident, $ident)} = (); + let ${concat($ident, $literal)} = (); + + // literal (adhoc) + let ${concat("a", b)} = (); + let ${concat("a", _b)} = (); + let ${concat("a", "b")} = (); + let ${concat("a", $tt_ident)} = (); + let ${concat("a", $tt_literal)} = (); + let ${concat("a", $ident)} = (); + let ${concat("a", $literal)} = (); + // literal (param) + let ${concat($literal, b)} = (); + let ${concat($literal, _b)} = (); + let ${concat($literal, "b")} = (); + let ${concat($literal, $tt_ident)} = (); + let ${concat($literal, $tt_literal)} = (); + let ${concat($literal, $ident)} = (); + let ${concat($literal, $literal)} = (); }}; } @@ -66,5 +111,5 @@ fn main() { assert_eq!(VARident, 1); assert_eq!(VAR_123, 2); - literals!(_hello); + combinations!(_hello, "a", b, "b"); } diff --git a/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.rs b/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.rs index b2845c8d1c1fc..7673bd3200fff 100644 --- a/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.rs +++ b/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.rs @@ -20,7 +20,7 @@ macro_rules! wrong_concat_declarations { //~^ ERROR `concat` must have at least two elements ${concat($ex, aaaa)} - //~^ ERROR `${concat(..)}` currently only accepts identifiers + //~^ ERROR metavariables of `${concat(..)}` must be of type ${concat($ex, aaaa 123)} //~^ ERROR expected comma @@ -98,6 +98,39 @@ macro_rules! unsupported_literals { }}; } +macro_rules! bad_literal_string { + ($literal:literal) => { + const ${concat(_foo, $literal)}: () = (); + //~^ ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + //~| ERROR `${concat(..)}` is not generating a valid identifier + } +} + +macro_rules! bad_literal_non_string { + ($literal:literal) => { + const ${concat(_foo, $literal)}: () = (); + //~^ ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + } +} + +macro_rules! bad_tt_literal { + ($tt:tt) => { + const ${concat(_foo, $tt)}: () = (); + //~^ ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + //~| ERROR metavariables of `${concat(..)}` must be of type + } +} + fn main() { wrong_concat_declarations!(1); @@ -113,4 +146,23 @@ fn main() { unsupported_literals!(_abc); empty!(); + + bad_literal_string!("\u{00BD}"); + bad_literal_string!("\x41"); + bad_literal_string!("🤷"); + bad_literal_string!("d[-_-]b"); + + bad_literal_string!("-1"); + bad_literal_string!("1.0"); + bad_literal_string!("'1'"); + + bad_literal_non_string!(1); + bad_literal_non_string!(-1); + bad_literal_non_string!(1.0); + bad_literal_non_string!('1'); + bad_literal_non_string!(false); + + bad_tt_literal!(1); + bad_tt_literal!(1.0); + bad_tt_literal!('1'); } diff --git a/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.stderr b/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.stderr index 2fe5842b39eb5..2de6d2b3ce3b8 100644 --- a/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.stderr +++ b/tests/ui/macros/macro-metavar-expr-concat/syntax-errors.stderr @@ -64,11 +64,13 @@ error: expected identifier or string literal LL | let ${concat($ident, 1)}: () = (); | ^ -error: `${concat(..)}` currently only accepts identifiers or meta-variables as parameters +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` --> $DIR/syntax-errors.rs:22:19 | LL | ${concat($ex, aaaa)} | ^^ + | + = note: currently only string literals are supported error: variable `foo` is not recognized in meta-variable expression --> $DIR/syntax-errors.rs:35:30 @@ -131,5 +133,152 @@ LL | empty!(); | = note: this error originates in the macro `empty` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 18 previous errors +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("\u{00BD}"); + | ------------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("\x41"); + | --------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("🤷"); + | ------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("d[-_-]b"); + | ------------------------------ in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("-1"); + | ------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("1.0"); + | -------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: `${concat(..)}` is not generating a valid identifier + --> $DIR/syntax-errors.rs:103:16 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | bad_literal_string!("'1'"); + | -------------------------- in this macro invocation + | + = note: this error originates in the macro `bad_literal_string` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:116:31 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^ + | + = note: currently only string literals are supported + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:116:31 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:116:31 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:116:31 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:116:31 + | +LL | const ${concat(_foo, $literal)}: () = (); + | ^^^^^^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:127:31 + | +LL | const ${concat(_foo, $tt)}: () = (); + | ^^ + | + = note: currently only string literals are supported + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:127:31 + | +LL | const ${concat(_foo, $tt)}: () = (); + | ^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: metavariables of `${concat(..)}` must be of type `ident`, `literal` or `tt` + --> $DIR/syntax-errors.rs:127:31 + | +LL | const ${concat(_foo, $tt)}: () = (); + | ^^ + | + = note: currently only string literals are supported + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: aborting due to 33 previous errors diff --git a/tests/ui/macros/macro-metavar-expr-concat/unicode-expansion.rs b/tests/ui/macros/macro-metavar-expr-concat/unicode-expansion.rs index b2cfb211e2d1e..4eeb2384deb78 100644 --- a/tests/ui/macros/macro-metavar-expr-concat/unicode-expansion.rs +++ b/tests/ui/macros/macro-metavar-expr-concat/unicode-expansion.rs @@ -3,12 +3,17 @@ #![feature(macro_metavar_expr_concat)] macro_rules! turn_to_page { - ($ident:ident) => { + ($ident:ident, $literal:literal, $tt:tt) => { const ${concat("Ḧ", $ident)}: i32 = 394; + const ${concat("Ḧ", $literal)}: i32 = 394; + const ${concat("Ḧ", $tt)}: i32 = 394; }; } fn main() { - turn_to_page!(P); - assert_eq!(ḦP, 394); + turn_to_page!(P1, "Ṕ2", Ṕ); + assert_eq!(ḦṔ, 394); + assert_eq!(ḦP1, 394); + assert_eq!(ḦṔ2, 394); + } From 37257b45e9030fa28cea1ac7c0a55bf78049a1f8 Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Wed, 17 Jul 2024 13:21:45 -0700 Subject: [PATCH 22/22] style-guide: Clarify version-sorting Every time we apply version-sorting, we also say to sort non-lowercase before lowercase. This seems likely to be what we'll want for future sorting, as well. For simplicity, just incorporate that into the definition, "unless otherwise specified". --- src/doc/style-guide/src/README.md | 34 ++++++++++++++++++------------- src/doc/style-guide/src/items.md | 10 +++------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/doc/style-guide/src/README.md b/src/doc/style-guide/src/README.md index dce50ebf29c46..f42b9cb597843 100644 --- a/src/doc/style-guide/src/README.md +++ b/src/doc/style-guide/src/README.md @@ -117,8 +117,7 @@ fn baz() {} In various cases, the default Rust style specifies to sort things. If not otherwise specified, such sorting should be "version sorting", which ensures that (for instance) `x8` comes before `x16` even though the character `1` comes -before the character `8`. (If not otherwise specified, version-sorting is -lexicographical.) +before the character `8`. For the purposes of the Rust style, to compare two strings for version-sorting: @@ -132,12 +131,13 @@ For the purposes of the Rust style, to compare two strings for version-sorting: these strings, treat the chunks as equal (moving on to the next chunk) but remember which string had more leading zeroes. - To compare two chunks if both are not numeric, compare them by Unicode - character lexicographically, except that `_` (underscore) sorts immediately - after ` ` (space) but before any other character. (This treats underscore as - a word separator, as commonly used in identifiers.) - - If the use of version sorting specifies further modifiers, such as sorting - non-lowercase before lowercase, apply those modifiers to the lexicographic - sort in this step. + character lexicographically, with two exceptions: + - `_` (underscore) sorts immediately after ` ` (space) but before any other + character. (This treats underscore as a word separator, as commonly used in + identifiers.) + - Unless otherwise specified, version-sorting should sort non-lowercase + characters (characters that can start an `UpperCamelCase` identifier) + before lowercase characters. - If the comparison reaches the end of the string and considers each pair of chunks equal: - If one of the numeric comparisons noted the earliest point at which one @@ -157,7 +157,17 @@ leading zeroes. As an example, version-sorting will sort the following strings in the order given: -- `_ZYWX` +- `_ZYXW` +- `_abcd` +- `A2` +- `ABCD` +- `Z_YXW` +- `ZY_XW` +- `ZY_XW` +- `ZYXW` +- `ZYXW_` +- `a1` +- `abcd` - `u_zzz` - `u8` - `u16` @@ -190,11 +200,7 @@ given: - `x86_64` - `x86_128` - `x87` -- `Z_YWX` -- `ZY_WX` -- `ZYW_X` -- `ZYWX` -- `ZYWX_` +- `zyxw` ### [Module-level items](items.md) diff --git a/src/doc/style-guide/src/items.md b/src/doc/style-guide/src/items.md index c0628691b7734..8e1b1d80fb644 100644 --- a/src/doc/style-guide/src/items.md +++ b/src/doc/style-guide/src/items.md @@ -489,10 +489,8 @@ foo::{ A *group* of imports is a set of imports on the same or sequential lines. One or more blank lines or other items (e.g., a function) separate groups of imports. -Within a group of imports, imports must be version-sorted, except that -non-lowercase characters (characters that can start an `UpperCamelCase` -identifier) must be sorted before lowercase characters. Groups of imports must -not be merged or re-ordered. +Within a group of imports, imports must be version-sorted. Groups of imports +must not be merged or re-ordered. E.g., input: @@ -520,9 +518,7 @@ re-ordering. ### Ordering list import Names in a list import must be version-sorted, except that: -- `self` and `super` always come first if present, -- non-lowercase characters (characters that can start an `UpperCamelCase` - identifier) must be sorted before lowercase characters, and +- `self` and `super` always come first if present, and - groups and glob imports always come last if present. This applies recursively. For example, `a::*` comes before `b::a` but `a::b`