From 0a05677d22825f66018361c3b6a55148bfc85650 Mon Sep 17 00:00:00 2001 From: dianne Date: Wed, 26 Mar 2025 06:37:56 -0700 Subject: [PATCH 01/11] `lower_pat_expr`: use the pattern's type instead of the literal's This allows us to remove the field `treat_byte_string_as_slice` from `TypeckResults`, since the pattern's type contains everything necessary to get the correct lowering for byte string literal patterns. This leaves the implementation of `string_deref_patterns` broken, to be fixed in the next commit. --- compiler/rustc_hir_typeck/src/pat.rs | 4 --- compiler/rustc_hir_typeck/src/writeback.rs | 3 -- .../rustc_middle/src/ty/typeck_results.rs | 7 ----- .../src/thir/pattern/const_to_pat.rs | 31 ++----------------- .../rustc_mir_build/src/thir/pattern/mod.rs | 17 +++++++--- 5 files changed, 15 insertions(+), 47 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/pat.rs b/compiler/rustc_hir_typeck/src/pat.rs index f1f956779c94..5a2cead33def 100644 --- a/compiler/rustc_hir_typeck/src/pat.rs +++ b/compiler/rustc_hir_typeck/src/pat.rs @@ -632,10 +632,6 @@ fn check_pat_lit( { let tcx = self.tcx; trace!(?lt.hir_id.local_id, "polymorphic byte string lit"); - self.typeck_results - .borrow_mut() - .treat_byte_string_as_slice - .insert(lt.hir_id.local_id); pat_ty = Ty::new_imm_ref(tcx, tcx.lifetimes.re_static, Ty::new_slice(tcx, tcx.types.u8)); } diff --git a/compiler/rustc_hir_typeck/src/writeback.rs b/compiler/rustc_hir_typeck/src/writeback.rs index b63c0b6ab7e0..109023c513e3 100644 --- a/compiler/rustc_hir_typeck/src/writeback.rs +++ b/compiler/rustc_hir_typeck/src/writeback.rs @@ -81,9 +81,6 @@ pub(crate) fn resolve_type_vars_in_body( debug!("used_trait_imports({:?}) = {:?}", item_def_id, used_trait_imports); wbcx.typeck_results.used_trait_imports = used_trait_imports; - wbcx.typeck_results.treat_byte_string_as_slice = - mem::take(&mut self.typeck_results.borrow_mut().treat_byte_string_as_slice); - debug!("writeback: typeck results for {:?} are {:#?}", item_def_id, wbcx.typeck_results); self.tcx.arena.alloc(wbcx.typeck_results) diff --git a/compiler/rustc_middle/src/ty/typeck_results.rs b/compiler/rustc_middle/src/ty/typeck_results.rs index 06054e22e760..3d3f4e2773a2 100644 --- a/compiler/rustc_middle/src/ty/typeck_results.rs +++ b/compiler/rustc_middle/src/ty/typeck_results.rs @@ -197,12 +197,6 @@ pub struct TypeckResults<'tcx> { /// formatting modified file tests/ui/coroutine/retain-resume-ref.rs pub coroutine_stalled_predicates: FxIndexSet<(ty::Predicate<'tcx>, ObligationCause<'tcx>)>, - /// We sometimes treat byte string literals (which are of type `&[u8; N]`) - /// as `&[u8]`, depending on the pattern in which they are used. - /// This hashset records all instances where we behave - /// like this to allow `const_to_pat` to reliably handle this situation. - pub treat_byte_string_as_slice: ItemLocalSet, - /// Contains the data for evaluating the effect of feature `capture_disjoint_fields` /// on closure size. pub closure_size_eval: LocalDefIdMap>, @@ -237,7 +231,6 @@ pub fn new(hir_owner: OwnerId) -> TypeckResults<'tcx> { closure_fake_reads: Default::default(), rvalue_scopes: Default::default(), coroutine_stalled_predicates: Default::default(), - treat_byte_string_as_slice: Default::default(), closure_size_eval: Default::default(), offset_of_data: Default::default(), } diff --git a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs index 667d59d858e3..372453688d24 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs @@ -58,25 +58,13 @@ struct ConstToPat<'tcx> { span: Span, id: hir::HirId, - treat_byte_string_as_slice: bool, - c: ty::Const<'tcx>, } impl<'tcx> ConstToPat<'tcx> { fn new(pat_ctxt: &PatCtxt<'_, 'tcx>, id: hir::HirId, span: Span, c: ty::Const<'tcx>) -> Self { trace!(?pat_ctxt.typeck_results.hir_owner); - ConstToPat { - tcx: pat_ctxt.tcx, - typing_env: pat_ctxt.typing_env, - span, - id, - treat_byte_string_as_slice: pat_ctxt - .typeck_results - .treat_byte_string_as_slice - .contains(&id.local_id), - c, - } + ConstToPat { tcx: pat_ctxt.tcx, typing_env: pat_ctxt.typing_env, span, id, c } } fn type_marked_structural(&self, ty: Ty<'tcx>) -> bool { @@ -108,8 +96,6 @@ fn unevaluated_to_pat( uv: ty::UnevaluatedConst<'tcx>, ty: Ty<'tcx>, ) -> Box> { - trace!(self.treat_byte_string_as_slice); - // It's not *technically* correct to be revealing opaque types here as borrowcheck has // not run yet. However, CTFE itself uses `TypingMode::PostAnalysis` unconditionally even // during typeck and not doing so has a lot of (undesirable) fallout (#101478, #119821). @@ -307,21 +293,8 @@ fn valtree_to_pat(&self, cv: ValTree<'tcx>, ty: Ty<'tcx>) -> Box> { ty, ); } else { - // `b"foo"` produces a `&[u8; 3]`, but you can't use constants of array type when - // matching against references, you can only use byte string literals. - // The typechecker has a special case for byte string literals, by treating them - // as slices. This means we turn `&[T; N]` constants into slice patterns, which - // has no negative effects on pattern matching, even if we're actually matching on - // arrays. - let pointee_ty = match *pointee_ty.kind() { - ty::Array(elem_ty, _) if self.treat_byte_string_as_slice => { - Ty::new_slice(tcx, elem_ty) - } - _ => *pointee_ty, - }; // References have the same valtree representation as their pointee. - let subpattern = self.valtree_to_pat(cv, pointee_ty); - PatKind::Deref { subpattern } + PatKind::Deref { subpattern: self.valtree_to_pat(cv, *pointee_ty) } } } }, diff --git a/compiler/rustc_mir_build/src/thir/pattern/mod.rs b/compiler/rustc_mir_build/src/thir/pattern/mod.rs index 4bfeab44bf4b..66c2223efc8c 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/mod.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/mod.rs @@ -130,7 +130,7 @@ fn lower_pattern_range_endpoint( // Lower the endpoint into a temporary `PatKind` that will then be // deconstructed to obtain the constant value and other data. - let mut kind: PatKind<'tcx> = self.lower_pat_expr(expr); + let mut kind: PatKind<'tcx> = self.lower_pat_expr(expr, None); // Unpeel any ascription or inline-const wrapper nodes. loop { @@ -294,7 +294,7 @@ fn lower_pattern_unadjusted(&mut self, pat: &'tcx hir::Pat<'tcx>) -> Box PatKind::Never, - hir::PatKind::Expr(value) => self.lower_pat_expr(value), + hir::PatKind::Expr(value) => self.lower_pat_expr(value, Some(ty)), hir::PatKind::Range(ref lo_expr, ref hi_expr, end) => { let (lo_expr, hi_expr) = (lo_expr.as_deref(), hi_expr.as_deref()); @@ -630,7 +630,11 @@ fn lower_inline_const( /// - Paths (e.g. `FOO`, `foo::BAR`, `Option::None`) /// - Inline const blocks (e.g. `const { 1 + 1 }`) /// - Literals, possibly negated (e.g. `-128u8`, `"hello"`) - fn lower_pat_expr(&mut self, expr: &'tcx hir::PatExpr<'tcx>) -> PatKind<'tcx> { + fn lower_pat_expr( + &mut self, + expr: &'tcx hir::PatExpr<'tcx>, + pat_ty: Option>, + ) -> PatKind<'tcx> { let (lit, neg) = match &expr.kind { hir::PatExprKind::Path(qpath) => { return self.lower_path(qpath, expr.hir_id, expr.span).kind; @@ -641,7 +645,12 @@ fn lower_pat_expr(&mut self, expr: &'tcx hir::PatExpr<'tcx>) -> PatKind<'tcx> { hir::PatExprKind::Lit { lit, negated } => (lit, *negated), }; - let ct_ty = self.typeck_results.node_type(expr.hir_id); + // We handle byte string literal patterns by using the pattern's type instead of the + // literal's type in `const_to_pat`: if the literal `b"..."` matches on a slice reference, + // the pattern's type will be `&[u8]` whereas the literal's type is `&[u8; 3]`; using the + // pattern's type means we'll properly translate it to a slice reference pattern. This works + // because slices and arrays have the same valtree representation. + let ct_ty = pat_ty.unwrap_or_else(|| self.typeck_results.node_type(expr.hir_id)); let lit_input = LitToConstInput { lit: &lit.node, ty: ct_ty, neg }; let constant = self.tcx.at(expr.span).lit_to_const(lit_input); self.const_to_pat(constant, ct_ty, expr.hir_id, lit.span).kind From 3779c6b7974f715c1fba0e6b559798a16178193c Mon Sep 17 00:00:00 2001 From: dianne Date: Wed, 26 Mar 2025 08:20:20 -0700 Subject: [PATCH 02/11] add a temporary workaround for `string_deref_patterns` --- .../rustc_mir_build/src/thir/pattern/mod.rs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/mod.rs b/compiler/rustc_mir_build/src/thir/pattern/mod.rs index 66c2223efc8c..d20e051548bf 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/mod.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/mod.rs @@ -11,7 +11,7 @@ use rustc_errors::codes::*; use rustc_hir::def::{CtorOf, DefKind, Res}; use rustc_hir::pat_util::EnumerateAndAdjustIterator; -use rustc_hir::{self as hir, RangeEnd}; +use rustc_hir::{self as hir, LangItem, RangeEnd}; use rustc_index::Idx; use rustc_middle::mir::interpret::LitToConstInput; use rustc_middle::thir::{ @@ -650,7 +650,26 @@ fn lower_pat_expr( // the pattern's type will be `&[u8]` whereas the literal's type is `&[u8; 3]`; using the // pattern's type means we'll properly translate it to a slice reference pattern. This works // because slices and arrays have the same valtree representation. - let ct_ty = pat_ty.unwrap_or_else(|| self.typeck_results.node_type(expr.hir_id)); + // HACK: As an exception, use the literal's type if `pat_ty` is `String`; this can happen if + // `string_deref_patterns` is enabled. There's a special case for that when lowering to MIR. + // FIXME(deref_patterns): This hack won't be necessary once `string_deref_patterns` is + // superseded by a more general implementation of deref patterns. + let ct_ty = match pat_ty { + Some(pat_ty) + if let ty::Adt(def, _) = *pat_ty.kind() + && self.tcx.is_lang_item(def.did(), LangItem::String) => + { + if !self.tcx.features().string_deref_patterns() { + span_bug!( + expr.span, + "matching on `String` went through without enabling string_deref_patterns" + ); + } + self.typeck_results.node_type(expr.hir_id) + } + Some(pat_ty) => pat_ty, + None => self.typeck_results.node_type(expr.hir_id), + }; let lit_input = LitToConstInput { lit: &lit.node, ty: ct_ty, neg }; let constant = self.tcx.at(expr.span).lit_to_const(lit_input); self.const_to_pat(constant, ct_ty, expr.hir_id, lit.span).kind From e638ba69f09d72b0f1e2f00b03ff530460d1bfe3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 1 Apr 2025 15:11:10 +0200 Subject: [PATCH 03/11] interpret: add a version of run_for_validation for &self --- .../rustc_const_eval/src/interpret/memory.rs | 43 ++++++++++++++----- .../src/interpret/validity.rs | 2 +- src/tools/miri/src/concurrency/data_race.rs | 2 +- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index 8f286971e638..d077900587e9 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -8,8 +8,9 @@ use std::assert_matches::assert_matches; use std::borrow::{Borrow, Cow}; +use std::cell::Cell; use std::collections::VecDeque; -use std::{fmt, mem, ptr}; +use std::{fmt, ptr}; use rustc_abi::{Align, HasDataLayout, Size}; use rustc_ast::Mutability; @@ -131,7 +132,7 @@ pub struct Memory<'tcx, M: Machine<'tcx>> { /// This stores whether we are currently doing reads purely for the purpose of validation. /// Those reads do not trigger the machine's hooks for memory reads. /// Needless to say, this must only be set with great care! - validation_in_progress: bool, + validation_in_progress: Cell, } /// A reference to some allocation that was already bounds-checked for the given region @@ -158,7 +159,7 @@ pub fn new() -> Self { alloc_map: M::MemoryMap::default(), extra_fn_ptr_map: FxIndexMap::default(), dead_alloc_map: FxIndexMap::default(), - validation_in_progress: false, + validation_in_progress: Cell::new(false), } } @@ -715,7 +716,7 @@ pub fn get_ptr_alloc<'a>( // We want to call the hook on *all* accesses that involve an AllocId, including zero-sized // accesses. That means we cannot rely on the closure above or the `Some` branch below. We // do this after `check_and_deref_ptr` to ensure some basic sanity has already been checked. - if !self.memory.validation_in_progress { + if !self.memory.validation_in_progress.get() { if let Ok((alloc_id, ..)) = self.ptr_try_get_alloc_id(ptr, size_i64) { M::before_alloc_read(self, alloc_id)?; } @@ -723,7 +724,7 @@ pub fn get_ptr_alloc<'a>( if let Some((alloc_id, offset, prov, alloc)) = ptr_and_alloc { let range = alloc_range(offset, size); - if !self.memory.validation_in_progress { + if !self.memory.validation_in_progress.get() { M::before_memory_read( self.tcx, &self.machine, @@ -801,7 +802,7 @@ pub fn get_ptr_alloc_mut<'a>( ) -> InterpResult<'tcx, Option>> { let tcx = self.tcx; - let validation_in_progress = self.memory.validation_in_progress; + let validation_in_progress = self.memory.validation_in_progress.get(); let size_i64 = i64::try_from(size.bytes()).unwrap(); // it would be an error to even ask for more than isize::MAX bytes let ptr_and_alloc = Self::check_and_deref_ptr( @@ -1087,23 +1088,43 @@ pub fn take_leaked_allocations( /// /// We do this so Miri's allocation access tracking does not show the validation /// reads as spurious accesses. - pub fn run_for_validation(&mut self, f: impl FnOnce(&mut Self) -> R) -> R { + pub fn run_for_validation_mut(&mut self, f: impl FnOnce(&mut Self) -> R) -> R { // This deliberately uses `==` on `bool` to follow the pattern // `assert!(val.replace(new) == old)`. assert!( - mem::replace(&mut self.memory.validation_in_progress, true) == false, + self.memory.validation_in_progress.replace(true) == false, "`validation_in_progress` was already set" ); let res = f(self); assert!( - mem::replace(&mut self.memory.validation_in_progress, false) == true, + self.memory.validation_in_progress.replace(false) == true, + "`validation_in_progress` was unset by someone else" + ); + res + } + + /// Runs the closure in "validation" mode, which means the machine's memory read hooks will be + /// suppressed. Needless to say, this must only be set with great care! Cannot be nested. + /// + /// We do this so Miri's allocation access tracking does not show the validation + /// reads as spurious accesses. + pub fn run_for_validation_ref(&self, f: impl FnOnce(&Self) -> R) -> R { + // This deliberately uses `==` on `bool` to follow the pattern + // `assert!(val.replace(new) == old)`. + assert!( + self.memory.validation_in_progress.replace(true) == false, + "`validation_in_progress` was already set" + ); + let res = f(self); + assert!( + self.memory.validation_in_progress.replace(false) == true, "`validation_in_progress` was unset by someone else" ); res } pub(super) fn validation_in_progress(&self) -> bool { - self.memory.validation_in_progress + self.memory.validation_in_progress.get() } } @@ -1375,7 +1396,7 @@ pub fn mem_copy_repeatedly( }; let src_alloc = self.get_alloc_raw(src_alloc_id)?; let src_range = alloc_range(src_offset, size); - assert!(!self.memory.validation_in_progress, "we can't be copying during validation"); + assert!(!self.memory.validation_in_progress.get(), "we can't be copying during validation"); // For the overlapping case, it is crucial that we trigger the read hook // before the write hook -- the aliasing model cares about the order. M::before_memory_read( diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index eb3f552cd278..fb7ba6d7ef57 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -1322,7 +1322,7 @@ fn validate_operand_internal( trace!("validate_operand_internal: {:?}, {:?}", *val, val.layout.ty); // Run the visitor. - self.run_for_validation(|ecx| { + self.run_for_validation_mut(|ecx| { let reset_padding = reset_provenance_and_padding && { // Check if `val` is actually stored in memory. If not, padding is not even // represented and we need not reset it. diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index b1ca434361b4..923031dbbd1e 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -717,7 +717,7 @@ fn write_scalar_atomic( // The program didn't actually do a read, so suppress the memory access hooks. // This is also a very special exception where we just ignore an error -- if this read // was UB e.g. because the memory is uninitialized, we don't want to know! - let old_val = this.run_for_validation(|this| this.read_scalar(dest)).discard_err(); + let old_val = this.run_for_validation_mut(|this| this.read_scalar(dest)).discard_err(); this.allow_data_races_mut(move |this| this.write_scalar(val, dest))?; this.validate_atomic_store(dest, atomic)?; this.buffered_atomic_write(val, dest, atomic, old_val) From 9bdac177fc2ba94af3529f511a2db6ab2198d100 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Apr 2025 09:55:38 +1100 Subject: [PATCH 04/11] Simplify control flow in `AstValidator::visit_item`. Currently some code paths return early, while others fall through to the `visit::walk_item` call, which is easy to overlook (I did, at first), even with the explanatory comments. This commit removes the early returns and moves the `visit::walk_item` calls up where necessary. This makes the function easier to read and slightly shorter. --- .../rustc_ast_passes/src/ast_validation.rs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index 839d5d3bb954..1390895daacc 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -863,7 +863,6 @@ fn visit_item(&mut self, item: &'a Item) { walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: true }); }); walk_list!(self, visit_attribute, &item.attrs); - return; // Avoid visiting again. } ItemKind::Impl(box Impl { safety, @@ -915,7 +914,6 @@ fn visit_item(&mut self, item: &'a Item) { walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: false }); }); walk_list!(self, visit_attribute, &item.attrs); - return; // Avoid visiting again. } ItemKind::Fn( func @ box Fn { @@ -960,7 +958,6 @@ fn visit_item(&mut self, item: &'a Item) { let kind = FnKind::Fn(FnCtxt::Free, &item.vis, &*func); self.visit_fn(kind, item.span, item.id); walk_list!(self, visit_attribute, &item.attrs); - return; // Avoid visiting again. } ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => { self.with_in_extern_mod(*safety, |this| { @@ -991,7 +988,6 @@ fn visit_item(&mut self, item: &'a Item) { visit::walk_item(this, item); this.extern_mod = old_item; }); - return; // Avoid visiting again. } ItemKind::Enum(_, def, _) => { for variant in &def.variants { @@ -1006,6 +1002,7 @@ fn visit_item(&mut self, item: &'a Item) { ); } } + visit::walk_item(self, item) } ItemKind::Trait(box Trait { is_auto, generics, ident, bounds, items, .. }) => { let is_const_trait = @@ -1033,7 +1030,6 @@ fn visit_item(&mut self, item: &'a Item) { walk_list!(this, visit_assoc_item, items, AssocCtxt::Trait); }); walk_list!(self, visit_attribute, &item.attrs); - return; // Avoid visiting again } ItemKind::Mod(safety, ident, mod_kind) => { if let &Safety::Unsafe(span) = safety { @@ -1045,6 +1041,7 @@ fn visit_item(&mut self, item: &'a Item) { { self.check_mod_file_item_asciionly(*ident); } + visit::walk_item(self, item) } ItemKind::Struct(ident, vdata, generics) => match vdata { VariantData::Struct { fields, .. } => { @@ -1054,9 +1051,8 @@ fn visit_item(&mut self, item: &'a Item) { // Permit `Anon{Struct,Union}` as field type. walk_list!(self, visit_struct_field_def, fields); walk_list!(self, visit_attribute, &item.attrs); - return; } - _ => {} + _ => visit::walk_item(self, item), }, ItemKind::Union(ident, vdata, generics) => { if vdata.fields().is_empty() { @@ -1070,9 +1066,8 @@ fn visit_item(&mut self, item: &'a Item) { // Permit `Anon{Struct,Union}` as field type. walk_list!(self, visit_struct_field_def, fields); walk_list!(self, visit_attribute, &item.attrs); - return; } - _ => {} + _ => visit::walk_item(self, item), } } ItemKind::Const(box ConstItem { defaultness, expr, .. }) => { @@ -1083,6 +1078,7 @@ fn visit_item(&mut self, item: &'a Item) { replace_span: self.ending_semi_or_hi(item.span), }); } + visit::walk_item(self, item); } ItemKind::Static(box StaticItem { expr, safety, .. }) => { self.check_item_safety(item.span, *safety); @@ -1096,6 +1092,7 @@ fn visit_item(&mut self, item: &'a Item) { replace_span: self.ending_semi_or_hi(item.span), }); } + visit::walk_item(self, item); } ItemKind::TyAlias( ty_alias @ box TyAlias { defaultness, bounds, where_clauses, ty, .. }, @@ -1119,11 +1116,10 @@ fn visit_item(&mut self, item: &'a Item) { help: self.sess.is_nightly_build(), }); } + visit::walk_item(self, item); } - _ => {} + _ => visit::walk_item(self, item), } - - visit::walk_item(self, item); } fn visit_foreign_item(&mut self, fi: &'a ForeignItem) { From ccb2194f9674c5be8bcc4d39ece15a382c863c2a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Apr 2025 12:04:56 +1100 Subject: [PATCH 05/11] Factor some code out of `AstValidator::visit_items`. Currently it uses `walk_item` on some item kinds. For other item kinds it visits the fields individually. For the latter group, this commit adds `visit_attrs_vis` and `visit_attrs_vis_ident` which bundle up visits to the fields that don't need special handling. This makes it clearer that they haven't been forgotten about. Also, it's better to do the attribute visits at the start because attributes precede the items in the source code. Because of this, a couple of tests have their output improved: errors appear in an order that matches the source code order. --- .../rustc_ast_passes/src/ast_validation.rs | 39 ++++++++++--------- tests/ui/coverage-attr/name-value.stderr | 34 ++++++++-------- tests/ui/coverage-attr/word-only.stderr | 30 +++++++------- 3 files changed, 52 insertions(+), 51 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index 1390895daacc..dc617965f9fb 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -727,6 +727,19 @@ fn maybe_lint_missing_abi(&mut self, span: Span, id: NodeId) { ) } } + + // Used within `visit_item` for item kinds where we don't call `visit::walk_item`. + fn visit_attrs_vis(&mut self, attrs: &'a AttrVec, vis: &'a Visibility) { + walk_list!(self, visit_attribute, attrs); + self.visit_vis(vis); + } + + // Used within `visit_item` for item kinds where we don't call `visit::walk_item`. + fn visit_attrs_vis_ident(&mut self, attrs: &'a AttrVec, vis: &'a Visibility, ident: &'a Ident) { + walk_list!(self, visit_attribute, attrs); + self.visit_vis(vis); + self.visit_ident(ident); + } } /// Checks that generic parameters are in the correct order, @@ -834,6 +847,7 @@ fn visit_item(&mut self, item: &'a Item) { self_ty, items, }) => { + self.visit_attrs_vis(&item.attrs, &item.vis); self.with_in_trait_impl(Some((*constness, *polarity, t)), |this| { this.visibility_not_permitted( &item.vis, @@ -853,7 +867,6 @@ fn visit_item(&mut self, item: &'a Item) { }); } - this.visit_vis(&item.vis); let disallowed = matches!(constness, Const::No) .then(|| TildeConstReason::TraitImpl { span: item.span }); this.with_tilde_const(disallowed, |this| this.visit_generics(generics)); @@ -862,7 +875,6 @@ fn visit_item(&mut self, item: &'a Item) { walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: true }); }); - walk_list!(self, visit_attribute, &item.attrs); } ItemKind::Impl(box Impl { safety, @@ -882,6 +894,7 @@ fn visit_item(&mut self, item: &'a Item) { only_trait, }; + self.visit_attrs_vis(&item.attrs, &item.vis); self.with_in_trait_impl(None, |this| { this.visibility_not_permitted( &item.vis, @@ -905,7 +918,6 @@ fn visit_item(&mut self, item: &'a Item) { this.dcx().emit_err(error(span, "`const`", true)); } - this.visit_vis(&item.vis); this.with_tilde_const( Some(TildeConstReason::Impl { span: item.span }), |this| this.visit_generics(generics), @@ -913,7 +925,6 @@ fn visit_item(&mut self, item: &'a Item) { this.visit_ty(self_ty); walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: false }); }); - walk_list!(self, visit_attribute, &item.attrs); } ItemKind::Fn( func @ box Fn { @@ -926,6 +937,7 @@ fn visit_item(&mut self, item: &'a Item) { define_opaque: _, }, ) => { + self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident); self.check_defaultness(item.span, *defaultness); let is_intrinsic = @@ -953,11 +965,8 @@ fn visit_item(&mut self, item: &'a Item) { }); } - self.visit_vis(&item.vis); - self.visit_ident(ident); let kind = FnKind::Fn(FnCtxt::Free, &item.vis, &*func); self.visit_fn(kind, item.span, item.id); - walk_list!(self, visit_attribute, &item.attrs); } ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => { self.with_in_extern_mod(*safety, |this| { @@ -1005,6 +1014,7 @@ fn visit_item(&mut self, item: &'a Item) { visit::walk_item(self, item) } ItemKind::Trait(box Trait { is_auto, generics, ident, bounds, items, .. }) => { + self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident); let is_const_trait = attr::find_by_name(&item.attrs, sym::const_trait).map(|attr| attr.span); self.with_in_trait(item.span, is_const_trait, |this| { @@ -1018,8 +1028,6 @@ fn visit_item(&mut self, item: &'a Item) { // Equivalent of `visit::walk_item` for `ItemKind::Trait` that inserts a bound // context for the supertraits. - this.visit_vis(&item.vis); - this.visit_ident(ident); let disallowed = is_const_trait .is_none() .then(|| TildeConstReason::Trait { span: item.span }); @@ -1029,7 +1037,6 @@ fn visit_item(&mut self, item: &'a Item) { }); walk_list!(this, visit_assoc_item, items, AssocCtxt::Trait); }); - walk_list!(self, visit_attribute, &item.attrs); } ItemKind::Mod(safety, ident, mod_kind) => { if let &Safety::Unsafe(span) = safety { @@ -1045,12 +1052,10 @@ fn visit_item(&mut self, item: &'a Item) { } ItemKind::Struct(ident, vdata, generics) => match vdata { VariantData::Struct { fields, .. } => { - self.visit_vis(&item.vis); - self.visit_ident(ident); + self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident); self.visit_generics(generics); // Permit `Anon{Struct,Union}` as field type. walk_list!(self, visit_struct_field_def, fields); - walk_list!(self, visit_attribute, &item.attrs); } _ => visit::walk_item(self, item), }, @@ -1060,12 +1065,10 @@ fn visit_item(&mut self, item: &'a Item) { } match vdata { VariantData::Struct { fields, .. } => { - self.visit_vis(&item.vis); - self.visit_ident(ident); + self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident); self.visit_generics(generics); // Permit `Anon{Struct,Union}` as field type. walk_list!(self, visit_struct_field_def, fields); - walk_list!(self, visit_attribute, &item.attrs); } _ => visit::walk_item(self, item), } @@ -1484,10 +1487,8 @@ fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) { || ctxt == AssocCtxt::Trait || matches!(func.sig.header.constness, Const::Yes(_)) => { - self.visit_vis(&item.vis); - self.visit_ident(&func.ident); + self.visit_attrs_vis_ident(&item.attrs, &item.vis, &func.ident); let kind = FnKind::Fn(FnCtxt::Assoc(ctxt), &item.vis, &*func); - walk_list!(self, visit_attribute, &item.attrs); self.visit_fn(kind, item.span, item.id); } AssocItemKind::Type(_) => { diff --git a/tests/ui/coverage-attr/name-value.stderr b/tests/ui/coverage-attr/name-value.stderr index 31a635b57e54..f24db78415e3 100644 --- a/tests/ui/coverage-attr/name-value.stderr +++ b/tests/ui/coverage-attr/name-value.stderr @@ -43,6 +43,21 @@ LL - #[coverage = "off"] LL + #[coverage(on)] | +error: malformed `coverage` attribute input + --> $DIR/name-value.rs:26:1 + | +LL | #[coverage = "off"] + | ^^^^^^^^^^^^^^^^^^^ + | +help: the following are the possible correct uses + | +LL - #[coverage = "off"] +LL + #[coverage(off)] + | +LL - #[coverage = "off"] +LL + #[coverage(on)] + | + error: malformed `coverage` attribute input --> $DIR/name-value.rs:29:5 | @@ -59,7 +74,7 @@ LL + #[coverage(on)] | error: malformed `coverage` attribute input - --> $DIR/name-value.rs:26:1 + --> $DIR/name-value.rs:35:1 | LL | #[coverage = "off"] | ^^^^^^^^^^^^^^^^^^^ @@ -104,7 +119,7 @@ LL + #[coverage(on)] | error: malformed `coverage` attribute input - --> $DIR/name-value.rs:35:1 + --> $DIR/name-value.rs:50:1 | LL | #[coverage = "off"] | ^^^^^^^^^^^^^^^^^^^ @@ -148,21 +163,6 @@ LL - #[coverage = "off"] LL + #[coverage(on)] | -error: malformed `coverage` attribute input - --> $DIR/name-value.rs:50:1 - | -LL | #[coverage = "off"] - | ^^^^^^^^^^^^^^^^^^^ - | -help: the following are the possible correct uses - | -LL - #[coverage = "off"] -LL + #[coverage(off)] - | -LL - #[coverage = "off"] -LL + #[coverage(on)] - | - error: malformed `coverage` attribute input --> $DIR/name-value.rs:64:1 | diff --git a/tests/ui/coverage-attr/word-only.stderr b/tests/ui/coverage-attr/word-only.stderr index 612301885dcc..2773db9c8578 100644 --- a/tests/ui/coverage-attr/word-only.stderr +++ b/tests/ui/coverage-attr/word-only.stderr @@ -37,6 +37,19 @@ LL | #[coverage(off)] LL | #[coverage(on)] | ++++ +error: malformed `coverage` attribute input + --> $DIR/word-only.rs:26:1 + | +LL | #[coverage] + | ^^^^^^^^^^^ + | +help: the following are the possible correct uses + | +LL | #[coverage(off)] + | +++++ +LL | #[coverage(on)] + | ++++ + error: malformed `coverage` attribute input --> $DIR/word-only.rs:29:5 | @@ -51,7 +64,7 @@ LL | #[coverage(on)] | ++++ error: malformed `coverage` attribute input - --> $DIR/word-only.rs:26:1 + --> $DIR/word-only.rs:35:1 | LL | #[coverage] | ^^^^^^^^^^^ @@ -90,7 +103,7 @@ LL | #[coverage(on)] | ++++ error: malformed `coverage` attribute input - --> $DIR/word-only.rs:35:1 + --> $DIR/word-only.rs:50:1 | LL | #[coverage] | ^^^^^^^^^^^ @@ -128,19 +141,6 @@ LL | #[coverage(off)] LL | #[coverage(on)] | ++++ -error: malformed `coverage` attribute input - --> $DIR/word-only.rs:50:1 - | -LL | #[coverage] - | ^^^^^^^^^^^ - | -help: the following are the possible correct uses - | -LL | #[coverage(off)] - | +++++ -LL | #[coverage(on)] - | ++++ - error: malformed `coverage` attribute input --> $DIR/word-only.rs:64:1 | From fb01485690da6f4c3dbb7f5a3068784f5a3d87bf Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 2 Apr 2025 14:21:33 +1100 Subject: [PATCH 06/11] Rename span-related names in `AstValidator`. A bunch of span-related names in `AstValidator` don't end in `span`, which goes against the usual naming conventions and makes the code surprisingly hard to read. E.g. a name like `body` doesn't sound like it's a span. This commit adds `_span` suffixes. --- .../rustc_ast_passes/src/ast_validation.rs | 79 ++++++++++--------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index dc617965f9fb..adb2522e36e2 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -47,14 +47,14 @@ enum SelfSemantic { } enum TraitOrTraitImpl { - Trait { span: Span, constness: Option }, - TraitImpl { constness: Const, polarity: ImplPolarity, trait_ref: Span }, + Trait { span: Span, constness_span: Option }, + TraitImpl { constness: Const, polarity: ImplPolarity, trait_ref_span: Span }, } impl TraitOrTraitImpl { fn constness(&self) -> Option { match self { - Self::Trait { constness: Some(span), .. } + Self::Trait { constness_span: Some(span), .. } | Self::TraitImpl { constness: Const::Yes(span), .. } => Some(*span), _ => None, } @@ -66,7 +66,7 @@ struct AstValidator<'a> { features: &'a Features, /// The span of the `extern` in an `extern { ... }` block, if any. - extern_mod: Option, + extern_mod_span: Option, outer_trait_or_trait_impl: Option, @@ -75,7 +75,7 @@ struct AstValidator<'a> { /// Used to ban nested `impl Trait`, e.g., `impl Into`. /// Nested `impl Trait` _is_ allowed in associated type position, /// e.g., `impl Iterator`. - outer_impl_trait: Option, + outer_impl_trait_span: Option, disallow_tilde_const: Option, @@ -96,17 +96,22 @@ fn with_in_trait_impl( trait_.map(|(constness, polarity, trait_ref)| TraitOrTraitImpl::TraitImpl { constness, polarity, - trait_ref: trait_ref.path.span, + trait_ref_span: trait_ref.path.span, }), ); f(self); self.outer_trait_or_trait_impl = old; } - fn with_in_trait(&mut self, span: Span, constness: Option, f: impl FnOnce(&mut Self)) { + fn with_in_trait( + &mut self, + span: Span, + constness_span: Option, + f: impl FnOnce(&mut Self), + ) { let old = mem::replace( &mut self.outer_trait_or_trait_impl, - Some(TraitOrTraitImpl::Trait { span, constness }), + Some(TraitOrTraitImpl::Trait { span, constness_span }), ); f(self); self.outer_trait_or_trait_impl = old; @@ -170,10 +175,10 @@ fn check_type_alias_where_clause_location( Err(errors::WhereClauseBeforeTypeAlias { span, sugg }) } - fn with_impl_trait(&mut self, outer: Option, f: impl FnOnce(&mut Self)) { - let old = mem::replace(&mut self.outer_impl_trait, outer); + fn with_impl_trait(&mut self, outer_span: Option, f: impl FnOnce(&mut Self)) { + let old = mem::replace(&mut self.outer_impl_trait_span, outer_span); f(self); - self.outer_impl_trait = old; + self.outer_impl_trait_span = old; } // Mirrors `visit::walk_ty`, but tracks relevant state. @@ -258,21 +263,22 @@ fn check_trait_fn_not_const(&self, constness: Const, parent: &TraitOrTraitImpl) && let TraitOrTraitImpl::TraitImpl { constness: Const::No, polarity: ImplPolarity::Positive, - trait_ref, + trait_ref_span, .. } = parent { - Some(trait_ref.shrink_to_lo()) + Some(trait_ref_span.shrink_to_lo()) } else { None }; - let make_trait_const_sugg = - if const_trait_impl && let TraitOrTraitImpl::Trait { span, constness: None } = parent { - Some(span.shrink_to_lo()) - } else { - None - }; + let make_trait_const_sugg = if const_trait_impl + && let TraitOrTraitImpl::Trait { span, constness_span: None } = parent + { + Some(span.shrink_to_lo()) + } else { + None + }; let parent_constness = parent.constness(); self.dcx().emit_err(errors::TraitFnConst { @@ -448,13 +454,13 @@ fn check_foreign_ty_genericless( check_where_clause(where_clauses.after); } - fn check_foreign_kind_bodyless(&self, ident: Ident, kind: &str, body: Option) { - let Some(body) = body else { + fn check_foreign_kind_bodyless(&self, ident: Ident, kind: &str, body_span: Option) { + let Some(body_span) = body_span else { return; }; self.dcx().emit_err(errors::BodyInExtern { span: ident.span, - body, + body: body_span, block: self.current_extern_span(), kind, }); @@ -473,7 +479,7 @@ fn check_foreign_fn_bodyless(&self, ident: Ident, body: Option<&Block>) { } fn current_extern_span(&self) -> Span { - self.sess.source_map().guess_head_span(self.extern_mod.unwrap()) + self.sess.source_map().guess_head_span(self.extern_mod_span.unwrap()) } /// An `fn` in `extern { ... }` cannot have qualifiers, e.g. `async fn`. @@ -583,9 +589,10 @@ fn check_mod_file_item_asciionly(&self, ident: Ident) { self.dcx().emit_err(errors::ModuleNonAscii { span: ident.span, name: ident.name }); } - fn deny_generic_params(&self, generics: &Generics, ident: Span) { + fn deny_generic_params(&self, generics: &Generics, ident_span: Span) { if !generics.params.is_empty() { - self.dcx().emit_err(errors::AutoTraitGeneric { span: generics.span, ident }); + self.dcx() + .emit_err(errors::AutoTraitGeneric { span: generics.span, ident: ident_span }); } } @@ -605,11 +612,11 @@ fn deny_where_clause(&self, where_clause: &WhereClause, ident_span: Span) { } } - fn deny_items(&self, trait_items: &[P], ident: Span) { + fn deny_items(&self, trait_items: &[P], ident_span: Span) { if !trait_items.is_empty() { let spans: Vec<_> = trait_items.iter().map(|i| i.kind.ident().unwrap().span).collect(); let total = trait_items.first().unwrap().span.to(trait_items.last().unwrap().span); - self.dcx().emit_err(errors::AutoTraitItems { spans, total, ident }); + self.dcx().emit_err(errors::AutoTraitItems { spans, total, ident: ident_span }); } } @@ -694,7 +701,7 @@ fn visit_ty_common(&mut self, ty: &'a Ty) { } } TyKind::ImplTrait(_, bounds) => { - if let Some(outer_impl_trait_sp) = self.outer_impl_trait { + if let Some(outer_impl_trait_sp) = self.outer_impl_trait_span { self.dcx().emit_err(errors::NestedImplTrait { span: ty.span, outer: outer_impl_trait_sp, @@ -970,7 +977,7 @@ fn visit_item(&mut self, item: &'a Item) { } ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => { self.with_in_extern_mod(*safety, |this| { - let old_item = mem::replace(&mut this.extern_mod, Some(item.span)); + let old_item = mem::replace(&mut this.extern_mod_span, Some(item.span)); this.visibility_not_permitted( &item.vis, errors::VisibilityNotPermittedNote::IndividualForeignItems, @@ -995,7 +1002,7 @@ fn visit_item(&mut self, item: &'a Item) { this.maybe_lint_missing_abi(*extern_span, item.id); } visit::walk_item(this, item); - this.extern_mod = old_item; + this.extern_mod_span = old_item; }); } ItemKind::Enum(_, def, _) => { @@ -1593,7 +1600,7 @@ fn deny_equality_constraints( generics.where_clause.span } else { let mut span = predicate_span; - let mut prev: Option = None; + let mut prev_span: Option = None; let mut preds = generics.where_clause.predicates.iter().peekable(); // Find the predicate that shouldn't have been in the where bound list. while let Some(pred) = preds.next() { @@ -1603,12 +1610,12 @@ fn deny_equality_constraints( if let Some(next) = preds.peek() { // This is the first predicate, remove the trailing comma as well. span = span.with_hi(next.span.lo()); - } else if let Some(prev) = prev { + } else if let Some(prev_span) = prev_span { // Remove the previous comma as well. - span = span.with_lo(prev.hi()); + span = span.with_lo(prev_span.hi()); } } - prev = Some(pred.span); + prev_span = Some(pred.span); } span }; @@ -1683,10 +1690,10 @@ pub fn check_crate( let mut validator = AstValidator { sess, features, - extern_mod: None, + extern_mod_span: None, outer_trait_or_trait_impl: None, has_proc_macro_decls: false, - outer_impl_trait: None, + outer_impl_trait_span: None, disallow_tilde_const: Some(TildeConstReason::Item), extern_mod_safety: None, lint_buffer: lints, From 7a4d4de8a87f3dca6870e79e4110d8e3ecd7385b Mon Sep 17 00:00:00 2001 From: dianne Date: Tue, 1 Apr 2025 21:35:12 -0700 Subject: [PATCH 07/11] add tests for array/slice const patterns --- .../const_in_pattern/arrays-and-slices.rs | 53 ++++++++++++ .../const_in_pattern/arrays-and-slices.stderr | 84 +++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 tests/ui/consts/const_in_pattern/arrays-and-slices.rs create mode 100644 tests/ui/consts/const_in_pattern/arrays-and-slices.stderr diff --git a/tests/ui/consts/const_in_pattern/arrays-and-slices.rs b/tests/ui/consts/const_in_pattern/arrays-and-slices.rs new file mode 100644 index 000000000000..bb38490206b2 --- /dev/null +++ b/tests/ui/consts/const_in_pattern/arrays-and-slices.rs @@ -0,0 +1,53 @@ +//! Tests that arrays and slices in constants aren't interchangeable when used as patterns. + +#[derive(PartialEq, Eq)] +struct SomeStruct(T); + +const BSTR_SIZED: &'static [u8; 3] = b"012"; +const BSTR_UNSIZED: &'static [u8] = BSTR_SIZED; +const STRUCT_SIZED: &'static SomeStruct<[u8; 3]> = &SomeStruct(*BSTR_SIZED); +const STRUCT_UNSIZED: &'static SomeStruct<[u8]> = STRUCT_SIZED; + +fn type_mismatches() { + // Test that array consts can't be used where a slice pattern is expected. This helps ensure + // that `const_to_pat` won't produce irrefutable `thir::PatKind::Array` patterns when matching + // on slices, which would result in missing length checks. + // See also `tests/ui/match/pattern-deref-miscompile.rs`, which tests that byte string literal + // patterns check slices' length appropriately when matching on slices. + match BSTR_UNSIZED { + BSTR_SIZED => {} + //~^ ERROR: mismatched types + _ => {} + } + match STRUCT_UNSIZED { + STRUCT_SIZED => {} + //~^ ERROR: mismatched types + _ => {} + } + + // Test that slice consts can't be used where an array pattern is expected. + match BSTR_UNSIZED { + BSTR_SIZED => {} + //~^ ERROR: mismatched types + _ => {} + } + // If the types matched here, this would still error, since unsized structs aren't permitted in + // constant patterns. See the `invalid_patterns` test below. + match STRUCT_UNSIZED { + STRUCT_SIZED => {} + //~^ ERROR: mismatched types + _ => {} + } +} + +fn invalid_patterns() { + // Test that unsized structs containing slices can't be used as patterns. + // See `tests/ui/consts/issue-87046.rs` for an example with `str`. + match STRUCT_UNSIZED { + STRUCT_UNSIZED => {} + //~^ ERROR: cannot use unsized non-slice type `SomeStruct<[u8]>` in constant patterns + _ => {} + } +} + +fn main() {} diff --git a/tests/ui/consts/const_in_pattern/arrays-and-slices.stderr b/tests/ui/consts/const_in_pattern/arrays-and-slices.stderr new file mode 100644 index 000000000000..412caf60f7d8 --- /dev/null +++ b/tests/ui/consts/const_in_pattern/arrays-and-slices.stderr @@ -0,0 +1,84 @@ +error[E0308]: mismatched types + --> $DIR/arrays-and-slices.rs:18:9 + | +LL | const BSTR_SIZED: &'static [u8; 3] = b"012"; + | ---------------------------------- constant defined here +... +LL | match BSTR_UNSIZED { + | ------------ this expression has type `&[u8]` +LL | BSTR_SIZED => {} + | ^^^^^^^^^^ + | | + | expected `&[u8]`, found `&[u8; 3]` + | `BSTR_SIZED` is interpreted as a constant, not a new binding + | help: introduce a new binding instead: `other_bstr_sized` + | + = note: expected reference `&[u8]` + found reference `&'static [u8; 3]` + +error[E0308]: mismatched types + --> $DIR/arrays-and-slices.rs:23:9 + | +LL | const STRUCT_SIZED: &'static SomeStruct<[u8; 3]> = &SomeStruct(*BSTR_SIZED); + | ------------------------------------------------ constant defined here +... +LL | match STRUCT_UNSIZED { + | -------------- this expression has type `&SomeStruct<[u8]>` +LL | STRUCT_SIZED => {} + | ^^^^^^^^^^^^ + | | + | expected `&SomeStruct<[u8]>`, found `&SomeStruct<[u8; 3]>` + | `STRUCT_SIZED` is interpreted as a constant, not a new binding + | help: introduce a new binding instead: `other_struct_sized` + | + = note: expected reference `&SomeStruct<[u8]>` + found reference `&'static SomeStruct<[u8; 3]>` + +error[E0308]: mismatched types + --> $DIR/arrays-and-slices.rs:30:9 + | +LL | const BSTR_SIZED: &'static [u8; 3] = b"012"; + | ---------------------------------- constant defined here +... +LL | match BSTR_UNSIZED { + | ------------ this expression has type `&[u8]` +LL | BSTR_SIZED => {} + | ^^^^^^^^^^ + | | + | expected `&[u8]`, found `&[u8; 3]` + | `BSTR_SIZED` is interpreted as a constant, not a new binding + | help: introduce a new binding instead: `other_bstr_sized` + | + = note: expected reference `&[u8]` + found reference `&'static [u8; 3]` + +error[E0308]: mismatched types + --> $DIR/arrays-and-slices.rs:37:9 + | +LL | const STRUCT_SIZED: &'static SomeStruct<[u8; 3]> = &SomeStruct(*BSTR_SIZED); + | ------------------------------------------------ constant defined here +... +LL | match STRUCT_UNSIZED { + | -------------- this expression has type `&SomeStruct<[u8]>` +LL | STRUCT_SIZED => {} + | ^^^^^^^^^^^^ + | | + | expected `&SomeStruct<[u8]>`, found `&SomeStruct<[u8; 3]>` + | `STRUCT_SIZED` is interpreted as a constant, not a new binding + | help: introduce a new binding instead: `other_struct_sized` + | + = note: expected reference `&SomeStruct<[u8]>` + found reference `&'static SomeStruct<[u8; 3]>` + +error: cannot use unsized non-slice type `SomeStruct<[u8]>` in constant patterns + --> $DIR/arrays-and-slices.rs:47:9 + | +LL | const STRUCT_UNSIZED: &'static SomeStruct<[u8]> = STRUCT_SIZED; + | ----------------------------------------------- constant defined here +... +LL | STRUCT_UNSIZED => {} + | ^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0308`. From 2e7de1a924850b4f084ac3b83e372b54748aadb0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 2 Apr 2025 14:31:27 +1100 Subject: [PATCH 08/11] Reduce scope of `AstValidator::with_*` calls. `AstValidator` has several `with_*` methods, each one setting a field that adjust how checking takes place for items within certain other items. E.g. `with_in_trait_impl` is used to adjust the checking done on items inside an `impl` item. Weirdly, the scopes used for most of the `with_*` calls are very broad, and include things that aren't "inside" the item, such as visibility, unsafety, and constness. This commit minimizes the scope of these `with_*` calls so they only apply to the things inside the item. --- .../rustc_ast_passes/src/ast_validation.rs | 183 +++++++++--------- 1 file changed, 90 insertions(+), 93 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index adb2522e36e2..9916de8b7b13 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -855,31 +855,30 @@ fn visit_item(&mut self, item: &'a Item) { items, }) => { self.visit_attrs_vis(&item.attrs, &item.vis); + self.visibility_not_permitted( + &item.vis, + errors::VisibilityNotPermittedNote::TraitImpl, + ); + if let TyKind::Dummy = self_ty.kind { + // Abort immediately otherwise the `TyKind::Dummy` will reach HIR lowering, + // which isn't allowed. Not a problem for this obscure, obsolete syntax. + self.dcx().emit_fatal(errors::ObsoleteAuto { span: item.span }); + } + if let (&Safety::Unsafe(span), &ImplPolarity::Negative(sp)) = (safety, polarity) { + self.dcx().emit_err(errors::UnsafeNegativeImpl { + span: sp.to(t.path.span), + negative: sp, + r#unsafe: span, + }); + } + + let disallowed = matches!(constness, Const::No) + .then(|| TildeConstReason::TraitImpl { span: item.span }); + self.with_tilde_const(disallowed, |this| this.visit_generics(generics)); + self.visit_trait_ref(t); + self.visit_ty(self_ty); + self.with_in_trait_impl(Some((*constness, *polarity, t)), |this| { - this.visibility_not_permitted( - &item.vis, - errors::VisibilityNotPermittedNote::TraitImpl, - ); - if let TyKind::Dummy = self_ty.kind { - // Abort immediately otherwise the `TyKind::Dummy` will reach HIR lowering, - // which isn't allowed. Not a problem for this obscure, obsolete syntax. - this.dcx().emit_fatal(errors::ObsoleteAuto { span: item.span }); - } - if let (&Safety::Unsafe(span), &ImplPolarity::Negative(sp)) = (safety, polarity) - { - this.dcx().emit_err(errors::UnsafeNegativeImpl { - span: sp.to(t.path.span), - negative: sp, - r#unsafe: span, - }); - } - - let disallowed = matches!(constness, Const::No) - .then(|| TildeConstReason::TraitImpl { span: item.span }); - this.with_tilde_const(disallowed, |this| this.visit_generics(generics)); - this.visit_trait_ref(t); - this.visit_ty(self_ty); - walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: true }); }); } @@ -902,34 +901,33 @@ fn visit_item(&mut self, item: &'a Item) { }; self.visit_attrs_vis(&item.attrs, &item.vis); - self.with_in_trait_impl(None, |this| { - this.visibility_not_permitted( - &item.vis, - errors::VisibilityNotPermittedNote::IndividualImplItems, - ); - if let &Safety::Unsafe(span) = safety { - this.dcx().emit_err(errors::InherentImplCannotUnsafe { - span: self_ty.span, - annotation_span: span, - annotation: "unsafe", - self_ty: self_ty.span, - }); - } - if let &ImplPolarity::Negative(span) = polarity { - this.dcx().emit_err(error(span, "negative", false)); - } - if let &Defaultness::Default(def_span) = defaultness { - this.dcx().emit_err(error(def_span, "`default`", true)); - } - if let &Const::Yes(span) = constness { - this.dcx().emit_err(error(span, "`const`", true)); - } + self.visibility_not_permitted( + &item.vis, + errors::VisibilityNotPermittedNote::IndividualImplItems, + ); + if let &Safety::Unsafe(span) = safety { + self.dcx().emit_err(errors::InherentImplCannotUnsafe { + span: self_ty.span, + annotation_span: span, + annotation: "unsafe", + self_ty: self_ty.span, + }); + } + if let &ImplPolarity::Negative(span) = polarity { + self.dcx().emit_err(error(span, "negative", false)); + } + if let &Defaultness::Default(def_span) = defaultness { + self.dcx().emit_err(error(def_span, "`default`", true)); + } + if let &Const::Yes(span) = constness { + self.dcx().emit_err(error(span, "`const`", true)); + } - this.with_tilde_const( - Some(TildeConstReason::Impl { span: item.span }), - |this| this.visit_generics(generics), - ); - this.visit_ty(self_ty); + self.with_tilde_const(Some(TildeConstReason::Impl { span: item.span }), |this| { + this.visit_generics(generics) + }); + self.visit_ty(self_ty); + self.with_in_trait_impl(None, |this| { walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: false }); }); } @@ -976,34 +974,34 @@ fn visit_item(&mut self, item: &'a Item) { self.visit_fn(kind, item.span, item.id); } ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => { + let old_item = mem::replace(&mut self.extern_mod_span, Some(item.span)); + self.visibility_not_permitted( + &item.vis, + errors::VisibilityNotPermittedNote::IndividualForeignItems, + ); + + if &Safety::Default == safety { + if item.span.at_least_rust_2024() { + self.dcx().emit_err(errors::MissingUnsafeOnExtern { span: item.span }); + } else { + self.lint_buffer.buffer_lint( + MISSING_UNSAFE_ON_EXTERN, + item.id, + item.span, + BuiltinLintDiag::MissingUnsafeOnExtern { + suggestion: item.span.shrink_to_lo(), + }, + ); + } + } + + if abi.is_none() { + self.maybe_lint_missing_abi(*extern_span, item.id); + } self.with_in_extern_mod(*safety, |this| { - let old_item = mem::replace(&mut this.extern_mod_span, Some(item.span)); - this.visibility_not_permitted( - &item.vis, - errors::VisibilityNotPermittedNote::IndividualForeignItems, - ); - - if &Safety::Default == safety { - if item.span.at_least_rust_2024() { - this.dcx().emit_err(errors::MissingUnsafeOnExtern { span: item.span }); - } else { - this.lint_buffer.buffer_lint( - MISSING_UNSAFE_ON_EXTERN, - item.id, - item.span, - BuiltinLintDiag::MissingUnsafeOnExtern { - suggestion: item.span.shrink_to_lo(), - }, - ); - } - } - - if abi.is_none() { - this.maybe_lint_missing_abi(*extern_span, item.id); - } visit::walk_item(this, item); - this.extern_mod_span = old_item; }); + self.extern_mod_span = old_item; } ItemKind::Enum(_, def, _) => { for variant in &def.variants { @@ -1024,24 +1022,23 @@ fn visit_item(&mut self, item: &'a Item) { self.visit_attrs_vis_ident(&item.attrs, &item.vis, ident); let is_const_trait = attr::find_by_name(&item.attrs, sym::const_trait).map(|attr| attr.span); - self.with_in_trait(item.span, is_const_trait, |this| { - if *is_auto == IsAuto::Yes { - // Auto traits cannot have generics, super traits nor contain items. - this.deny_generic_params(generics, ident.span); - this.deny_super_traits(bounds, ident.span); - this.deny_where_clause(&generics.where_clause, ident.span); - this.deny_items(items, ident.span); - } + if *is_auto == IsAuto::Yes { + // Auto traits cannot have generics, super traits nor contain items. + self.deny_generic_params(generics, ident.span); + self.deny_super_traits(bounds, ident.span); + self.deny_where_clause(&generics.where_clause, ident.span); + self.deny_items(items, ident.span); + } - // Equivalent of `visit::walk_item` for `ItemKind::Trait` that inserts a bound - // context for the supertraits. - let disallowed = is_const_trait - .is_none() - .then(|| TildeConstReason::Trait { span: item.span }); - this.with_tilde_const(disallowed, |this| { - this.visit_generics(generics); - walk_list!(this, visit_param_bound, bounds, BoundKind::SuperTraits) - }); + // Equivalent of `visit::walk_item` for `ItemKind::Trait` that inserts a bound + // context for the supertraits. + let disallowed = + is_const_trait.is_none().then(|| TildeConstReason::Trait { span: item.span }); + self.with_tilde_const(disallowed, |this| { + this.visit_generics(generics); + walk_list!(this, visit_param_bound, bounds, BoundKind::SuperTraits) + }); + self.with_in_trait(item.span, is_const_trait, |this| { walk_list!(this, visit_assoc_item, items, AssocCtxt::Trait); }); } From 927ad1659ab4c918a4f41aef71cc8847a5198326 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Wed, 2 Apr 2025 07:35:05 +0200 Subject: [PATCH 09/11] Add a dep kind for use of the anon node with zero dependencies --- compiler/rustc_middle/src/dep_graph/dep_node.rs | 1 + compiler/rustc_middle/src/dep_graph/mod.rs | 1 + compiler/rustc_query_impl/src/plumbing.rs | 11 +++++++++++ compiler/rustc_query_system/src/dep_graph/graph.rs | 10 +++++----- compiler/rustc_query_system/src/dep_graph/mod.rs | 3 +++ 5 files changed, 21 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_middle/src/dep_graph/dep_node.rs b/compiler/rustc_middle/src/dep_graph/dep_node.rs index be34c7ef4bd5..644cdac5d554 100644 --- a/compiler/rustc_middle/src/dep_graph/dep_node.rs +++ b/compiler/rustc_middle/src/dep_graph/dep_node.rs @@ -89,6 +89,7 @@ pub mod label_strs { /// We use this to create a forever-red node. [] fn Red() -> (), [] fn SideEffect() -> (), + [] fn AnonZeroDeps() -> (), [] fn TraitSelect() -> (), [] fn CompileCodegenUnit() -> (), [] fn CompileMonoItem() -> (), diff --git a/compiler/rustc_middle/src/dep_graph/mod.rs b/compiler/rustc_middle/src/dep_graph/mod.rs index 739c0be1a91d..931d67087aca 100644 --- a/compiler/rustc_middle/src/dep_graph/mod.rs +++ b/compiler/rustc_middle/src/dep_graph/mod.rs @@ -53,6 +53,7 @@ fn name(&self, dep_kind: DepKind) -> &'static str { const DEP_KIND_NULL: DepKind = dep_kinds::Null; const DEP_KIND_RED: DepKind = dep_kinds::Red; const DEP_KIND_SIDE_EFFECT: DepKind = dep_kinds::SideEffect; + const DEP_KIND_ANON_ZERO_DEPS: DepKind = dep_kinds::AnonZeroDeps; const DEP_KIND_MAX: u16 = dep_node::DEP_KIND_VARIANTS - 1; } diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs index 3238c7a0912c..19ccc5587d6a 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -870,6 +870,17 @@ pub(crate) fn SideEffect<'tcx>() -> DepKindStruct<'tcx> { } } + pub(crate) fn AnonZeroDeps<'tcx>() -> DepKindStruct<'tcx> { + DepKindStruct { + is_anon: true, + is_eval_always: false, + fingerprint_style: FingerprintStyle::Opaque, + force_from_dep_node: Some(|_, _, _| bug!("cannot force an anon node")), + try_load_from_on_disk_cache: None, + name: &"AnonZeroDeps", + } + } + pub(crate) fn TraitSelect<'tcx>() -> DepKindStruct<'tcx> { DepKindStruct { is_anon: true, diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 127dcd825da5..9f34417973ed 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -50,7 +50,7 @@ pub struct DepNodeIndex {} rustc_data_structures::static_assert_size!(Option, 4); impl DepNodeIndex { - const SINGLETON_DEPENDENCYLESS_ANON_NODE: DepNodeIndex = DepNodeIndex::ZERO; + const SINGLETON_ZERO_DEPS_ANON_NODE: DepNodeIndex = DepNodeIndex::ZERO; pub const FOREVER_RED_NODE: DepNodeIndex = DepNodeIndex::from_u32(1); } @@ -140,13 +140,13 @@ pub fn new( let colors = DepNodeColorMap::new(prev_graph_node_count); - // Instantiate a dependy-less node only once for anonymous queries. + // Instantiate a node with zero dependencies only once for anonymous queries. let _green_node_index = current.alloc_node( - DepNode { kind: D::DEP_KIND_NULL, hash: current.anon_id_seed.into() }, + DepNode { kind: D::DEP_KIND_ANON_ZERO_DEPS, hash: current.anon_id_seed.into() }, EdgesVec::new(), Fingerprint::ZERO, ); - assert_eq!(_green_node_index, DepNodeIndex::SINGLETON_DEPENDENCYLESS_ANON_NODE); + assert_eq!(_green_node_index, DepNodeIndex::SINGLETON_ZERO_DEPS_ANON_NODE); // Instantiate a dependy-less red node only once for anonymous queries. let red_node_index = current.alloc_node( @@ -407,7 +407,7 @@ pub(crate) fn with_anon_task_inner, OP, R>( // going to be (i.e. equal to the precomputed // `SINGLETON_DEPENDENCYLESS_ANON_NODE`). As a consequence we can skip creating // a `StableHasher` and sending the node through interning. - DepNodeIndex::SINGLETON_DEPENDENCYLESS_ANON_NODE + DepNodeIndex::SINGLETON_ZERO_DEPS_ANON_NODE } 1 => { // When there is only one dependency, don't bother creating a node. diff --git a/compiler/rustc_query_system/src/dep_graph/mod.rs b/compiler/rustc_query_system/src/dep_graph/mod.rs index 4eeb6078d149..3a80835afad5 100644 --- a/compiler/rustc_query_system/src/dep_graph/mod.rs +++ b/compiler/rustc_query_system/src/dep_graph/mod.rs @@ -111,6 +111,9 @@ fn read_deps(op: OP) /// We use this to create a side effect node. const DEP_KIND_SIDE_EFFECT: DepKind; + /// We use this to create the anon node with zero dependencies. + const DEP_KIND_ANON_ZERO_DEPS: DepKind; + /// This is the highest value a `DepKind` can have. It's used during encoding to /// pack information into the unused bits. const DEP_KIND_MAX: u16; From d5f7e9c200458346bfa6cbb2f88291abe703ff7a Mon Sep 17 00:00:00 2001 From: dianqk Date: Wed, 2 Apr 2025 22:23:19 +0800 Subject: [PATCH 10/11] Add dianqk to codegen reviewers --- triagebot.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/triagebot.toml b/triagebot.toml index 59f815974beb..b9506676aae2 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -1191,8 +1191,8 @@ docs = [ "@ehuss", "@GuillaumeGomez", ] - codegen = [ + "@dianqk", "@saethlin", "@workingjubilee", ] From d8d27ca82249555906f5deb687e44813beef0062 Mon Sep 17 00:00:00 2001 From: Freya Arbjerg Date: Wed, 2 Apr 2025 17:39:42 +0200 Subject: [PATCH 11/11] Fix two incorrect turbofish suggestions Fixes #121901 --- compiler/rustc_parse/src/parser/item.rs | 32 +++++++++++++++---- .../ui/fn/bad-turbofish-hints-issue-121901.rs | 8 +++++ .../bad-turbofish-hints-issue-121901.stderr | 25 +++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 tests/ui/fn/bad-turbofish-hints-issue-121901.rs create mode 100644 tests/ui/fn/bad-turbofish-hints-issue-121901.stderr diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs index a8208e4717bd..9405b58ab3b4 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -2960,13 +2960,30 @@ fn parse_param_general(&mut self, req_name: ReqName, first_param: bool) -> PResu let parser_snapshot_before_ty = this.create_snapshot_for_diagnostic(); this.eat_incorrect_doc_comment_for_param_type(); let mut ty = this.parse_ty_for_param(); - if ty.is_ok() - && this.token != token::Comma - && this.token != token::CloseDelim(Delimiter::Parenthesis) - { - // This wasn't actually a type, but a pattern looking like a type, - // so we are going to rollback and re-parse for recovery. - ty = this.unexpected_any(); + + if let Ok(t) = &ty { + // Check for trailing angle brackets + if let TyKind::Path(_, Path { segments, .. }) = &t.kind { + if let Some(segment) = segments.last() { + if let Some(guar) = + this.check_trailing_angle_brackets(segment, &[exp!(CloseParen)]) + { + return Ok(( + dummy_arg(segment.ident, guar), + Trailing::No, + UsePreAttrPos::No, + )); + } + } + } + + if this.token != token::Comma + && this.token != token::CloseDelim(Delimiter::Parenthesis) + { + // This wasn't actually a type, but a pattern looking like a type, + // so we are going to rollback and re-parse for recovery. + ty = this.unexpected_any(); + } } match ty { Ok(ty) => { @@ -2977,6 +2994,7 @@ fn parse_param_general(&mut self, req_name: ReqName, first_param: bool) -> PResu } // If this is a C-variadic argument and we hit an error, return the error. Err(err) if this.token == token::DotDotDot => return Err(err), + Err(err) if this.unmatched_angle_bracket_count > 0 => return Err(err), // Recover from attempting to parse the argument as a type without pattern. Err(err) => { err.cancel(); diff --git a/tests/ui/fn/bad-turbofish-hints-issue-121901.rs b/tests/ui/fn/bad-turbofish-hints-issue-121901.rs new file mode 100644 index 000000000000..1425e576d75a --- /dev/null +++ b/tests/ui/fn/bad-turbofish-hints-issue-121901.rs @@ -0,0 +1,8 @@ +// Regression test for the parser wrongfully suggesting turbofish syntax in below syntax errors + +type One = for<'a> fn(Box`, found `)` +type Two = for<'a> fn(Box>); +//~^ ERROR: unmatched angle bracket + +fn main() {} diff --git a/tests/ui/fn/bad-turbofish-hints-issue-121901.stderr b/tests/ui/fn/bad-turbofish-hints-issue-121901.stderr new file mode 100644 index 000000000000..aacb326f8a22 --- /dev/null +++ b/tests/ui/fn/bad-turbofish-hints-issue-121901.stderr @@ -0,0 +1,25 @@ +error: expected one of `+`, `,`, or `>`, found `)` + --> $DIR/bad-turbofish-hints-issue-121901.rs:3:40 + | +LL | type One = for<'a> fn(Box` + | +help: you might have meant to end the type parameters here + | +LL | type One = for<'a> fn(Box); + | + + +error: unmatched angle bracket + --> $DIR/bad-turbofish-hints-issue-121901.rs:5:41 + | +LL | type Two = for<'a> fn(Box>); + | ^ + | +help: remove extra angle bracket + | +LL - type Two = for<'a> fn(Box>); +LL + type Two = for<'a> fn(Box); + | + +error: aborting due to 2 previous errors +