rustdoc: Properly clean fn params in all contexts

Author: fmease

Diff for: src/librustdoc/clean/mod.rs

@@ -1065,16 +1065,12 @@ fn clean_fn_decl_legacy_const_generics(func: &mut Function, attrs: &[hir::Attrib
         for (pos, literal) in meta_item_list.iter().filter_map(|meta| meta.lit()).enumerate() {
             match literal.kind {
                 ast::LitKind::Int(a, _) => {
-                    let param = func.generics.params.remove(0);
-                    if let GenericParamDef {
-                        name,
-                        kind: GenericParamDefKind::Const { ty, .. },
-                        ..
-                    } = param
-                    {
-                        func.decl
-                            .inputs
-                            .insert(a.get() as _, Parameter { name, type_: *ty, is_const: true });
+                    let GenericParamDef { name, kind, .. } = func.generics.params.remove(0);
+                    if let GenericParamDefKind::Const { ty, .. } = kind {
+                        func.decl.inputs.insert(
+                            a.get() as _,
+                            Parameter { name: Some(name), type_: *ty, is_const: true },
+                        );
                     } else {
                         panic!("unexpected non const in position {pos}");
                     }
@@ -1101,7 +1097,10 @@ fn clean_function<'tcx>(
         let generics = clean_generics(generics, cx);
         let params = match params {
             ParamsSrc::Body(body_id) => clean_params_via_body(cx, sig.decl.inputs, body_id),
-            ParamsSrc::Idents(idents) => clean_params(cx, sig.decl.inputs, idents),
+            // Let's not perpetuate anon params from Rust 2015; use `_` for them.
+            ParamsSrc::Idents(idents) => clean_params(cx, sig.decl.inputs, idents, |ident| {
+                Some(ident.map_or(kw::Underscore, |ident| ident.name))
+            }),
         };
         let decl = clean_fn_decl_with_params(cx, sig.decl, Some(&sig.header), params);
         (generics, decl)
@@ -1113,30 +1112,13 @@ fn clean_params<'tcx>(
     cx: &mut DocContext<'tcx>,
     types: &[hir::Ty<'tcx>],
     idents: &[Option<Ident>],
+    postprocess: impl Fn(Option<Ident>) -> Option<Symbol>,
 ) -> Vec<Parameter> {
-    fn nonempty_name(ident: &Option<Ident>) -> Option<Symbol> {
-        if let Some(ident) = ident
-            && ident.name != kw::Underscore
-        {
-            Some(ident.name)
-        } else {
-            None
-        }
-    }
-
-    // If at least one argument has a name, use `_` as the name of unnamed
-    // arguments. Otherwise omit argument names.
-    let default_name = if idents.iter().any(|ident| nonempty_name(ident).is_some()) {
-        kw::Underscore
-    } else {
-        kw::Empty // FIXME: using kw::Empty is a bit of a hack
-    };
-
     types
         .iter()
         .enumerate()
         .map(|(i, ty)| Parameter {
-            name: idents.get(i).and_then(nonempty_name).unwrap_or(default_name),
+            name: postprocess(idents[i]),
             type_: clean_ty(ty, cx),
             is_const: false,
         })
@@ -1152,7 +1134,7 @@ fn clean_params_via_body<'tcx>(
         .iter()
         .zip(cx.tcx.hir_body(body_id).params)
         .map(|(ty, param)| Parameter {
-            name: name_from_pat(param.pat),
+            name: Some(name_from_pat(param.pat)),
             type_: clean_ty(ty, cx),
             is_const: false,
         })
@@ -1182,8 +1164,6 @@ fn clean_poly_fn_sig<'tcx>(
     did: Option<DefId>,
     sig: ty::PolyFnSig<'tcx>,
 ) -> FnDecl {
-    let mut names = did.map_or(&[] as &[_], |did| cx.tcx.fn_arg_idents(did)).iter();
-
     let mut output = clean_middle_ty(sig.output(), cx, None, None);
 
     // If the return type isn't an `impl Trait`, we can safely assume that this
@@ -1196,12 +1176,20 @@ fn clean_poly_fn_sig<'tcx>(
         output = output.sugared_async_return_type();
     }
 
+    let mut idents = did.map(|did| cx.tcx.fn_arg_idents(did)).unwrap_or_default().iter().copied();
+
+    // If this comes from a fn item, let's not perpetuate anon params from Rust 2015; use `_` for them.
+    // If this comes from a fn ptr ty, we just keep params unnamed since it's more conventional stylistically.
+    // Since the param name is not part of the semantic type, these params never bear a name unlike
+    // in the HIR case, thus we can't peform any fancy fallback logic unlike `clean_bare_fn_ty`.
+    let fallback = did.map(|_| kw::Underscore);
+
     let params = sig
         .inputs()
         .iter()
-        .map(|t| Parameter {
-            type_: clean_middle_ty(t.map_bound(|t| *t), cx, None, None),
-            name: if let Some(Some(ident)) = names.next() { ident.name } else { kw::Underscore },
+        .map(|ty| Parameter {
+            name: idents.next().flatten().map(|ident| ident.name).or(fallback),
+            type_: clean_middle_ty(ty.map_bound(|ty| *ty), cx, None, None),
             is_const: false,
         })
         .collect();
@@ -2591,7 +2579,17 @@ fn clean_bare_fn_ty<'tcx>(
             .filter(|p| !is_elided_lifetime(p))
             .map(|x| clean_generic_param(cx, None, x))
             .collect();
-        let params = clean_params(cx, bare_fn.decl.inputs, bare_fn.param_idents);
+        // Since it's more conventional stylistically, elide the name of all params called `_`
+        // unless there's at least one interestingly named param in which case don't elide any
+        // name since mixing named and unnamed params is less legible.
+        let filter = |ident: Option<Ident>| {
+            ident.map(|ident| ident.name).filter(|&ident| ident != kw::Underscore)
+        };
+        let fallback =
+            bare_fn.param_idents.iter().copied().find_map(filter).map(|_| kw::Underscore);
+        let params = clean_params(cx, bare_fn.decl.inputs, bare_fn.param_idents, |ident| {
+            filter(ident).or(fallback)
+        });
         let decl = clean_fn_decl_with_params(cx, bare_fn.decl, None, params);
         (generic_params, decl)
     });

Diff for: src/librustdoc/clean/types.rs

@@ -1421,7 +1421,7 @@ impl FnDecl {
 /// A function parameter.
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]
 pub(crate) struct Parameter {
-    pub(crate) name: Symbol,
+    pub(crate) name: Option<Symbol>,
     pub(crate) type_: Type,
     /// This field is used to represent "const" arguments from the `rustc_legacy_const_generics`
     /// feature. More information in <https://github.com/rust-lang/rust/issues/83167>.
@@ -1430,7 +1430,7 @@ pub(crate) struct Parameter {
 
 impl Parameter {
     pub(crate) fn to_receiver(&self) -> Option<&Type> {
-        (self.name == kw::SelfLower).then_some(&self.type_)
+        if self.name == Some(kw::SelfLower) { Some(&self.type_) } else { None }
     }
 }
 

Diff for: src/librustdoc/clean/utils.rs

@@ -303,13 +303,12 @@ pub(crate) fn name_from_pat(p: &hir::Pat<'_>) -> Symbol {
     debug!("trying to get a name from pattern: {p:?}");
 
     Symbol::intern(&match &p.kind {
-        // FIXME(never_patterns): does this make sense?
-        PatKind::Missing => unreachable!(),
-        PatKind::Wild
-        | PatKind::Err(_)
+        PatKind::Err(_)
+        | PatKind::Missing // Let's not perpetuate anon params from Rust 2015; use `_` for them.
         | PatKind::Never
+        | PatKind::Range(..)
         | PatKind::Struct(..)
-        | PatKind::Range(..) => {
+        | PatKind::Wild => {
             return kw::Underscore;
         }
         PatKind::Binding(_, _, ident, _) => return ident.name,

Diff for: src/librustdoc/html/format.rs

@@ -1237,8 +1237,8 @@ pub(crate) fn print_params(params: &[clean::Parameter], cx: &Context<'_>) -> imp
             .iter()
             .map(|param| {
                 fmt::from_fn(|f| {
-                    if !param.name.is_empty() {
-                        write!(f, "{}: ", param.name)?;
+                    if let Some(name) = param.name {
+                        write!(f, "{name}: ")?;
                     }
                     param.type_.print(cx).fmt(f)
                 })
@@ -1359,7 +1359,9 @@ impl clean::FnDecl {
                 if param.is_const {
                     write!(f, "const ")?;
                 }
-                write!(f, "{}: ", param.name)?;
+                if let Some(name) = param.name {
+                    write!(f, "{name}: ")?;
+                }
                 param.type_.print(cx).fmt(f)?;
             }
             match (line_wrapping_indent, last_input_index) {

Diff for: src/librustdoc/json/conversions.rs

@@ -11,7 +11,7 @@ use rustc_hir::def::CtorKind;
 use rustc_hir::def_id::DefId;
 use rustc_metadata::rendered_const;
 use rustc_middle::{bug, ty};
-use rustc_span::{Pos, Symbol};
+use rustc_span::{Pos, Symbol, kw};
 use rustdoc_json_types::*;
 
 use crate::clean::{self, ItemId};
@@ -610,7 +610,10 @@ impl FromClean<clean::FnDecl> for FunctionSignature {
         FunctionSignature {
             inputs: inputs
                 .into_iter()
-                .map(|param| (param.name.to_string(), param.type_.into_json(renderer)))
+                .map(|param| {
+                    // FIXME: using kw::Empty is a bit of a hack
+                    (param.name.unwrap_or(kw::Empty).to_string(), param.type_.into_json(renderer))
+                })
                 .collect(),
             output: if output.is_unit() { None } else { Some(output.into_json(renderer)) },
             is_c_variadic: c_variadic,

Diff for: tests/rustdoc/anon-fn-params.rs

@@ -0,0 +1,25 @@
+// Test that we render the deprecated anonymous trait function parameters from Rust 2015 as
+// underscores in order not to perpetuate it and for legibility.
+
+//@ edition: 2015
+#![expect(anonymous_parameters)]
+
+// Check the "local case" (HIR cleaning) //
+
+//@ has anon_fn_params/trait.Trait.html
+pub trait Trait {
+    //@ has - '//*[@id="tymethod.required"]' 'fn required(_: Option<i32>, _: impl Fn(&str) -> bool)'
+    fn required(Option<i32>, impl Fn(&str) -> bool);
+    //@ has - '//*[@id="method.provided"]' 'fn provided(_: [i32; 2])'
+    fn provided([i32; 2]) {}
+}
+
+// Check the "extern case" (middle cleaning) //
+
+//@ aux-build: ext-anon-fn-params.rs
+extern crate ext_anon_fn_params;
+
+//@ has anon_fn_params/trait.ExtTrait.html
+//@ has - '//*[@id="tymethod.required"]' 'fn required(_: Option<i32>, _: impl Fn(&str) -> bool)'
+//@ has - '//*[@id="method.provided"]' 'fn provided(_: [i32; 2])'
+pub use ext_anon_fn_params::Trait as ExtTrait;

Diff for: tests/rustdoc/assoc-fns.rs

@@ -0,0 +1,13 @@
+// Basic testing for associated functions (in traits, trait impls & inherent impls).
+
+//@ has assoc_fns/trait.Trait.html
+pub trait Trait {
+    //@ has - '//*[@id="tymethod.required"]' 'fn required(first: i32, second: &str)'
+    fn required(first: i32, second: &str);
+
+    //@ has - '//*[@id="method.provided"]' 'fn provided(only: ())'
+    fn provided(only: ()) {}
+
+    //@ has - '//*[@id="tymethod.params_are_unnamed"]' 'fn params_are_unnamed(_: i32, _: u32)'
+    fn params_are_unnamed(_: i32, _: u32);
+}

Diff for: tests/rustdoc/auxiliary/ext-anon-fn-params.rs

@@ -0,0 +1,7 @@
+//@ edition: 2015
+#![expect(anonymous_parameters)]
+
+pub trait Trait {
+    fn required(Option<i32>, impl Fn(&str) -> bool);
+    fn provided([i32; 2]) {}
+}

Diff for: tests/rustdoc/ffi.rs

@@ -9,4 +9,8 @@ pub use lib::foreigner;
 extern "C" {
     //@ has ffi/fn.another.html //pre 'pub unsafe extern "C" fn another(cold_as_ice: u32)'
     pub fn another(cold_as_ice: u32);
+
+    //@ has ffi/fn.params_are_unnamed.html //pre \
+    //      'pub unsafe extern "C" fn params_are_unnamed(_: i32, _: u32)'
+    pub fn params_are_unnamed(_: i32, _: u32);
 }

Diff for: tests/rustdoc/inline_cross/auxiliary/fn-type.rs renamed to tests/rustdoc/inline_cross/auxiliary/fn-ptr-ty.rs

@@ -53,17 +53,17 @@ pub use default_generic_args::R2;
 
 //@ has user/type.H0.html
 // Check that we handle higher-ranked regions correctly:
-//@ has - '//*[@class="rust item-decl"]//code' "fn(_: for<'a> fn(_: Re<'a>))"
+//@ has - '//*[@class="rust item-decl"]//code' "fn(for<'a> fn(Re<'a>))"
 pub use default_generic_args::H0;
 
 //@ has user/type.H1.html
 // Check that we don't conflate distinct universially quantified regions (#1):
-//@ has - '//*[@class="rust item-decl"]//code' "for<'b> fn(_: for<'a> fn(_: Re<'a, &'b ()>))"
+//@ has - '//*[@class="rust item-decl"]//code' "for<'b> fn(for<'a> fn(Re<'a, &'b ()>))"
 pub use default_generic_args::H1;
 
 //@ has user/type.H2.html
 // Check that we don't conflate distinct universially quantified regions (#2):
-//@ has - '//*[@class="rust item-decl"]//code' "for<'a> fn(_: for<'b> fn(_: Re<'a, &'b ()>))"
+//@ has - '//*[@class="rust item-decl"]//code' "for<'a> fn(for<'b> fn(Re<'a, &'b ()>))"
 pub use default_generic_args::H2;
 
 //@ has user/type.P0.html
@@ -86,7 +86,7 @@ pub use default_generic_args::A0;
 // Demonstrates that we currently don't elide generic arguments that are alpha-equivalent to their
 // respective generic parameter (after instantiation) for perf reasons (it would require us to
 // create an inference context).
-//@ has - '//*[@class="rust item-decl"]//code' "Alpha<for<'arbitrary> fn(_: &'arbitrary ())>"
+//@ has - '//*[@class="rust item-decl"]//code' "Alpha<for<'arbitrary> fn(&'arbitrary ())>"
 pub use default_generic_args::A1;
 
 //@ has user/type.M0.html

Diff for: tests/rustdoc/inline_cross/default-generic-args.rs

@@ -2,11 +2,11 @@
 // They should be rendered exactly as the user wrote it, i.e., in source order and with unused
 // parameters present, not stripped.
 
-//@ aux-crate:fn_type=fn-type.rs
+//@ aux-crate:fn_ptr_ty=fn-ptr-ty.rs
 //@ edition: 2021
 #![crate_name = "user"]
 
 //@ has user/type.F.html
 //@ has - '//*[@class="rust item-decl"]//code' \
-//     "for<'z, 'a, '_unused> fn(_: &'z for<'b> fn(_: &'b str), _: &'a ()) -> &'a ();"
-pub use fn_type::F;
+//     "for<'z, 'a, '_unused> fn(&'z for<'b> fn(&'b str), &'a ()) -> &'a ();"
+pub use fn_ptr_ty::F;

Diff for: tests/rustdoc/inline_cross/fn-type.rs renamed to tests/rustdoc/inline_cross/fn-ptr-ty.rs

@@ -29,7 +29,7 @@ pub use impl_trait_aux::func4;
 //@ has impl_trait/fn.func5.html
 //@ has - '//pre[@class="rust item-decl"]' "func5("
 //@ has - '//pre[@class="rust item-decl"]' "_f: impl for<'any> Fn(&'any str, &'any str) -> bool + for<'r> Other<T<'r> = ()>,"
-//@ has - '//pre[@class="rust item-decl"]' "_a: impl for<'beta, 'alpha, '_gamma> Auxiliary<'alpha, Item<'beta> = fn(_: &'beta ())>"
+//@ has - '//pre[@class="rust item-decl"]' "_a: impl for<'beta, 'alpha, '_gamma> Auxiliary<'alpha, Item<'beta> = fn(&'beta ())>"
 //@ !has - '//pre[@class="rust item-decl"]' 'where'
 pub use impl_trait_aux::func5;