From e88512214729e7bde2ed91faf6e49b50c6d3677b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 12 Mar 2025 16:48:52 +1100 Subject: [PATCH 1/5] Add a pretty printing test for fn params. Note that multiple parts of the output are incorrect. The following commits will fix this. --- tests/pretty/hir-fn-params.pp | 38 +++++++++++++++++++++++++++++++++++ tests/pretty/hir-fn-params.rs | 34 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 tests/pretty/hir-fn-params.pp create mode 100644 tests/pretty/hir-fn-params.rs diff --git a/tests/pretty/hir-fn-params.pp b/tests/pretty/hir-fn-params.pp new file mode 100644 index 0000000000000..9749261175dd8 --- /dev/null +++ b/tests/pretty/hir-fn-params.pp @@ -0,0 +1,38 @@ +#[prelude_import] +use ::std::prelude::rust_2015::*; +#[macro_use] +extern crate std; +//@ pretty-compare-only +//@ pretty-mode:hir +//@ pp-exact:hir-fn-params.pp + +// This tests the pretty-printing of various kinds of function parameters. + +//--------------------------------------------------------------------------- +// Normal functions and methods. + +fn normal_fn(_: u32, a: u32) { } + +struct S; +impl S { + fn method(_: u32, a: u32) { } +} + +//--------------------------------------------------------------------------- +// More exotic forms, which get a different pretty-printing path. In the past, +// anonymous params and `_` params printed incorrectly, e.g. `fn(u32, _: u32)` +// was printed as `fn(: u32, : u32)`. +// +// Ideally we would also test invalid patterns, e.g. `fn(1: u32, &a: u32)`, +// because they had similar problems. But the pretty-printing tests currently +// can't contain compile errors. + +fn bare_fn(x: fn(: u32, : u32, a: u32)) { } + +extern "C" { + unsafe fn foreign_fn(: u32, a: u32); +} + +trait T { + fn trait_fn(: u32, : u32, a: u32); +} diff --git a/tests/pretty/hir-fn-params.rs b/tests/pretty/hir-fn-params.rs new file mode 100644 index 0000000000000..5ace5289d08d7 --- /dev/null +++ b/tests/pretty/hir-fn-params.rs @@ -0,0 +1,34 @@ +//@ pretty-compare-only +//@ pretty-mode:hir +//@ pp-exact:hir-fn-params.pp + +// This tests the pretty-printing of various kinds of function parameters. + +//--------------------------------------------------------------------------- +// Normal functions and methods. + +fn normal_fn(_: u32, a: u32) {} + +struct S; +impl S { + fn method(_: u32, a: u32) {} +} + +//--------------------------------------------------------------------------- +// More exotic forms, which get a different pretty-printing path. In the past, +// anonymous params and `_` params printed incorrectly, e.g. `fn(u32, _: u32)` +// was printed as `fn(: u32, : u32)`. +// +// Ideally we would also test invalid patterns, e.g. `fn(1: u32, &a: u32)`, +// because they had similar problems. But the pretty-printing tests currently +// can't contain compile errors. + +fn bare_fn(x: fn(u32, _: u32, a: u32)) {} + +extern "C" { + fn foreign_fn(_: u32, a: u32); +} + +trait T { + fn trait_fn(u32, _: u32, a: u32); +} From 9714f60f1d54450ac4d17416947d1c659429c311 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Mar 2025 08:39:14 +1100 Subject: [PATCH 2/5] Inline and remove `FnParam::name`. It has a single call site. --- .../rustc_hir_typeck/src/fn_ctxt/checks.rs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index b851770166702..8ab6e0c4159dd 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -19,7 +19,7 @@ use rustc_middle::ty::visit::TypeVisitableExt; use rustc_middle::ty::{self, IsSuggestable, Ty, TyCtxt}; use rustc_middle::{bug, span_bug}; use rustc_session::Session; -use rustc_span::{DUMMY_SP, Ident, Span, Symbol, kw, sym}; +use rustc_span::{DUMMY_SP, Ident, Span, kw, sym}; use rustc_trait_selection::error_reporting::infer::{FailureCode, ObligationCauseExt}; use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::{self, ObligationCauseCode, ObligationCtxt, SelectionContext}; @@ -2720,21 +2720,18 @@ impl FnParam<'_> { } } - fn name(&self) -> Option { - match self { - Self::Param(x) if let hir::PatKind::Binding(_, _, ident, _) = x.pat.kind => { - Some(ident.name) - } - Self::Name(x) if x.name != kw::Empty => Some(x.name), - _ => None, - } - } - fn display(&self, idx: usize) -> impl '_ + fmt::Display { struct D<'a>(FnParam<'a>, usize); impl fmt::Display for D<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(name) = self.0.name() { + let name = match self.0 { + FnParam::Param(x) if let hir::PatKind::Binding(_, _, ident, _) = x.pat.kind => { + Some(ident.name) + } + FnParam::Name(x) if x.name != kw::Empty => Some(x.name), + _ => None, + }; + if let Some(name) = name { write!(f, "`{name}`") } else { write!(f, "parameter #{}", self.1 + 1) From 958bc7b3655a0880361b11f9052fab84030cde26 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Mar 2025 08:40:43 +1100 Subject: [PATCH 3/5] Handle `_` properly in a couple of places. Currently (PatKind::Wild` (i.e. `_`) gets turned by `lower_fn_params_to_names` into an empty identifier, which means it is printed incorrectly by HIR pretty printing. And likewise for `lower_fn_params_to_names`, which affects some error messages. This commit fixes them. This requires a slight tweak in a couple of places to continue using parameter numbers in some error messages. And it improves the output of `tests/ui/typeck/cyclic_type_ice.rs`: `/* _ */` is a better suggestion than `/* */`. --- compiler/rustc_ast_lowering/src/lib.rs | 9 +++++++- .../rustc_hir_typeck/src/fn_ctxt/checks.rs | 23 +++++++++++++------ compiler/rustc_middle/src/hir/map.rs | 3 ++- .../src/error_reporting/traits/suggestions.rs | 5 +++- tests/pretty/hir-fn-params.pp | 6 ++--- tests/ui/typeck/cyclic_type_ice.stderr | 2 +- 6 files changed, 34 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs index c7d37e2704de9..df671cf4b8604 100644 --- a/compiler/rustc_ast_lowering/src/lib.rs +++ b/compiler/rustc_ast_lowering/src/lib.rs @@ -1516,7 +1516,14 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { fn lower_fn_params_to_names(&mut self, decl: &FnDecl) -> &'hir [Ident] { self.arena.alloc_from_iter(decl.inputs.iter().map(|param| match param.pat.kind { PatKind::Ident(_, ident, _) => self.lower_ident(ident), - _ => Ident::new(kw::Empty, self.lower_span(param.pat.span)), + PatKind::Wild => Ident::new(kw::Underscore, self.lower_span(param.pat.span)), + _ => { + self.dcx().span_delayed_bug( + param.pat.span, + "non-ident/wild param pat must trigger an error", + ); + Ident::new(kw::Empty, self.lower_span(param.pat.span)) + } })) } diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index 8ab6e0c4159dd..0a472c95221ee 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -2712,11 +2712,12 @@ enum FnParam<'hir> { Param(&'hir hir::Param<'hir>), Name(&'hir Ident), } + impl FnParam<'_> { fn span(&self) -> Span { match self { - Self::Param(x) => x.span, - Self::Name(x) => x.span, + Self::Param(param) => param.span, + Self::Name(ident) => ident.span, } } @@ -2724,15 +2725,23 @@ impl FnParam<'_> { struct D<'a>(FnParam<'a>, usize); impl fmt::Display for D<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let name = match self.0 { - FnParam::Param(x) if let hir::PatKind::Binding(_, _, ident, _) = x.pat.kind => { + // A "unique" param name is one that (a) exists, and (b) is guaranteed to be unique + // among the parameters, i.e. `_` does not count. + let unique_name = match self.0 { + FnParam::Param(param) + if let hir::PatKind::Binding(_, _, ident, _) = param.pat.kind => + { + Some(ident.name) + } + FnParam::Name(ident) + if ident.name != kw::Empty && ident.name != kw::Underscore => + { Some(ident.name) } - FnParam::Name(x) if x.name != kw::Empty => Some(x.name), _ => None, }; - if let Some(name) = name { - write!(f, "`{name}`") + if let Some(unique_name) = unique_name { + write!(f, "`{unique_name}`") } else { write!(f, "parameter #{}", self.1 + 1) } diff --git a/compiler/rustc_middle/src/hir/map.rs b/compiler/rustc_middle/src/hir/map.rs index c85af81ee25bd..5c34dd6309ed9 100644 --- a/compiler/rustc_middle/src/hir/map.rs +++ b/compiler/rustc_middle/src/hir/map.rs @@ -281,8 +281,9 @@ impl<'tcx> TyCtxt<'tcx> { } pub fn hir_body_param_names(self, id: BodyId) -> impl Iterator { - self.hir_body(id).params.iter().map(|arg| match arg.pat.kind { + self.hir_body(id).params.iter().map(|param| match param.pat.kind { PatKind::Binding(_, _, ident, _) => ident, + PatKind::Wild => Ident::new(kw::Underscore, param.pat.span), _ => Ident::empty(), }) } diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs index ad46a15a5ac6d..b56ed14437776 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs @@ -1998,7 +1998,10 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { .iter() .enumerate() .map(|(i, ident)| { - if ident.name.is_empty() || ident.name == kw::SelfLower { + if ident.name.is_empty() + || ident.name == kw::Underscore + || ident.name == kw::SelfLower + { format!("arg{i}") } else { format!("{ident}") diff --git a/tests/pretty/hir-fn-params.pp b/tests/pretty/hir-fn-params.pp index 9749261175dd8..f33e26b7e8c95 100644 --- a/tests/pretty/hir-fn-params.pp +++ b/tests/pretty/hir-fn-params.pp @@ -27,12 +27,12 @@ // because they had similar problems. But the pretty-printing tests currently // can't contain compile errors. -fn bare_fn(x: fn(: u32, : u32, a: u32)) { } +fn bare_fn(x: fn(: u32, _: u32, a: u32)) { } extern "C" { - unsafe fn foreign_fn(: u32, a: u32); + unsafe fn foreign_fn(_: u32, a: u32); } trait T { - fn trait_fn(: u32, : u32, a: u32); + fn trait_fn(: u32, _: u32, a: u32); } diff --git a/tests/ui/typeck/cyclic_type_ice.stderr b/tests/ui/typeck/cyclic_type_ice.stderr index 4dc02a53c0211..645766becbf72 100644 --- a/tests/ui/typeck/cyclic_type_ice.stderr +++ b/tests/ui/typeck/cyclic_type_ice.stderr @@ -23,7 +23,7 @@ LL | let f = |_, _| (); help: provide the argument | LL - f(f); -LL + f(/* */, /* */); +LL + f(/* _ */, /* _ */); | error: aborting due to 2 previous errors From bebd91feb32667b6a1bb7a11da91ab8a935b4c24 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Mar 2025 07:29:19 +1100 Subject: [PATCH 4/5] Fix HIR param pretty printing some more. Anonymous params are currently represented with `kw::Empty`, so handle that properly. (Subsequent commits will get rid of the `kw::Empty`.) --- compiler/rustc_hir_pretty/src/lib.rs | 8 +++++--- tests/pretty/hir-fn-params.pp | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs index 3067766fb4d73..1409310339a08 100644 --- a/compiler/rustc_hir_pretty/src/lib.rs +++ b/compiler/rustc_hir_pretty/src/lib.rs @@ -2148,9 +2148,11 @@ impl<'a> State<'a> { s.print_implicit_self(&decl.implicit_self); } else { if let Some(arg_name) = arg_names.get(i) { - s.word(arg_name.to_string()); - s.word(":"); - s.space(); + if arg_name.name != kw::Empty { + s.word(arg_name.to_string()); + s.word(":"); + s.space(); + } } else if let Some(body_id) = body_id { s.ann.nested(s, Nested::BodyParamPat(body_id, i)); s.word(":"); diff --git a/tests/pretty/hir-fn-params.pp b/tests/pretty/hir-fn-params.pp index f33e26b7e8c95..3799c8a3c3b7c 100644 --- a/tests/pretty/hir-fn-params.pp +++ b/tests/pretty/hir-fn-params.pp @@ -27,12 +27,12 @@ // because they had similar problems. But the pretty-printing tests currently // can't contain compile errors. -fn bare_fn(x: fn(: u32, _: u32, a: u32)) { } +fn bare_fn(x: fn(u32, _: u32, a: u32)) { } extern "C" { unsafe fn foreign_fn(_: u32, a: u32); } trait T { - fn trait_fn(: u32, _: u32, a: u32); + fn trait_fn(u32, _: u32, a: u32); } From 79e4be1e9f1bd8032f3573a8f44f88814632a342 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 14 Mar 2025 09:02:45 +1100 Subject: [PATCH 5/5] Remove the ref from `FnParam::Ident`. --- compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index 0a472c95221ee..96d0a0fc6ded3 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -2679,7 +2679,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { params.get(is_method as usize..params.len() - sig.decl.c_variadic as usize)?; debug_assert_eq!(params.len(), fn_inputs.len()); Some(( - fn_inputs.zip(params.iter().map(|param| FnParam::Name(param))).collect(), + fn_inputs.zip(params.iter().map(|¶m| FnParam::Name(param))).collect(), generics, )) } @@ -2710,7 +2710,7 @@ impl<'tcx> Visitor<'tcx> for FindClosureArg<'tcx> { #[derive(Clone, Copy)] enum FnParam<'hir> { Param(&'hir hir::Param<'hir>), - Name(&'hir Ident), + Name(Ident), } impl FnParam<'_> {