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 f1f956779c94f..5a2cead33deff 100644 --- a/compiler/rustc_hir_typeck/src/pat.rs +++ b/compiler/rustc_hir_typeck/src/pat.rs @@ -632,10 +632,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { { 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 b63c0b6ab7e09..109023c513e35 100644 --- a/compiler/rustc_hir_typeck/src/writeback.rs +++ b/compiler/rustc_hir_typeck/src/writeback.rs @@ -81,9 +81,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { 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 06054e22e7601..3d3f4e2773a2d 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 @@ impl<'tcx> 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 667d59d858e3a..372453688d242 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 @@ impl<'tcx> ConstToPat<'tcx> { 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 @@ impl<'tcx> ConstToPat<'tcx> { 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 4bfeab44bf4b9..66c2223efc8c7 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 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> { // 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 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> { hir::PatKind::Never => 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 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> { /// - 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 @@ impl<'a, 'tcx> PatCtxt<'a, '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 66c2223efc8c7..d20e051548bf1 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_abi::{FieldIdx, Integer}; 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 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> { // 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 8f286971e6389..d077900587e9c 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 @@ impl<'tcx, M: Machine<'tcx>> Memory<'tcx, M> { 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 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { // 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 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { 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 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { ) -> 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 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { /// /// 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 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { }; 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 eb3f552cd2787..fb7ba6d7ef57e 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -1322,7 +1322,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { 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 b1ca434361b4a..923031dbbd1eb 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -717,7 +717,7 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // 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 839d5d3bb954d..1390895daacc9 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -863,7 +863,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { ); } } + visit::walk_item(self, item) } ItemKind::Trait(box Trait { is_auto, generics, ident, bounds, items, .. }) => { let is_const_trait = @@ -1033,7 +1030,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { // 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { // 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 1390895daacc9..dc617965f9fb0 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -727,6 +727,19 @@ impl<'a> AstValidator<'a> { ) } } + + // 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { }); } - 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { }); } - 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { // 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { }); 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { || 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 31a635b57e546..f24db78415e39 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 612301885dcc8..2773db9c85786 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 dc617965f9fb0..adb2522e36e29 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 @@ impl<'a> AstValidator<'a> { 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 @@ impl<'a> AstValidator<'a> { 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 @@ impl<'a> AstValidator<'a> { && 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 @@ impl<'a> AstValidator<'a> { 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 @@ impl<'a> AstValidator<'a> { } 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 @@ impl<'a> AstValidator<'a> { 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 @@ impl<'a> AstValidator<'a> { } } - 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 @@ impl<'a> AstValidator<'a> { } } 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 0000000000000..bb38490206b2c --- /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 0000000000000..412caf60f7d85 --- /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 | 179 +++++++++--------- 1 file changed, 88 insertions(+), 91 deletions(-) diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index adb2522e36e29..9916de8b7b130 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -855,31 +855,30 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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, - 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, - }); - } + 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 }); - this.with_tilde_const(disallowed, |this| this.visit_generics(generics)); - this.visit_trait_ref(t); - this.visit_ty(self_ty); + 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| { walk_list!(this, visit_assoc_item, items, AssocCtxt::Impl { of_trait: true }); }); } @@ -902,34 +901,33 @@ impl<'a> Visitor<'a> for AstValidator<'a> { }; 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { self.visit_fn(kind, item.span, item.id); } ItemKind::ForeignMod(ForeignMod { extern_span, abi, safety, .. }) => { - 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(), - }, - ); - } + 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() { - this.maybe_lint_missing_abi(*extern_span, item.id); - } + if abi.is_none() { + self.maybe_lint_missing_abi(*extern_span, item.id); + } + self.with_in_extern_mod(*safety, |this| { 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 @@ impl<'a> Visitor<'a> for AstValidator<'a> { 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 be34c7ef4bd52..644cdac5d5543 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 @@ rustc_query_append!(define_dep_nodes![ /// 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 739c0be1a91d6..931d67087acab 100644 --- a/compiler/rustc_middle/src/dep_graph/mod.rs +++ b/compiler/rustc_middle/src/dep_graph/mod.rs @@ -53,6 +53,7 @@ impl Deps for DepsType { 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 3238c7a0912c5..19ccc5587d6a6 100644 --- a/compiler/rustc_query_impl/src/plumbing.rs +++ b/compiler/rustc_query_impl/src/plumbing.rs @@ -870,6 +870,17 @@ macro_rules! define_queries { } } + 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 127dcd825da56..9f34417973ed5 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 @@ rustc_index::newtype_index! { 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 @@ impl DepGraph { 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 @@ impl DepGraphData { // 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 4eeb6078d1491..3a80835afad57 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 @@ pub trait Deps { /// 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 59f815974bebc..b9506676aae2a 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 a8208e4717bdf..9405b58ab3b4d 100644 --- a/compiler/rustc_parse/src/parser/item.rs +++ b/compiler/rustc_parse/src/parser/item.rs @@ -2960,13 +2960,30 @@ impl<'a> Parser<'a> { 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 @@ impl<'a> Parser<'a> { } // 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 0000000000000..1425e576d75a3 --- /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 0000000000000..aacb326f8a22a --- /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 +