Rollup merge of rust-lang#139913 - fmease:rustdoc-fix-fn-param-handling, r=GuillaumeGomez

rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)

**(0)** PR rust-lang#136411 aimed to stop rendering unnamed params of fn ptr types as underscores in the common case (e.g., `fn(_: i32)` → `fn(i32)`) to make the rendered output stylistically more conventional.

**(0.a)** However, since the cleaning fn that the PR modified is also used for lowering the HIR params of foreign fns and required assoc fns in traits, it accidentally butchered the rendering of the latter two:

```rs
pub trait Trait { fn assoc_fn(_: i32); } // as well as (Rust 2015 only): fn assoc_fn(i32);
unsafe extern "C" { pub fn foreign_fn(_: i32); }

// Since 1.86 the fns above gets mis-rendered as:
pub fn assoc_fn(: i32) // <-- BUTCHERED
pub unsafe extern "C" fn foreign_fn(: i32) // <-- BUTCHERED
```

**(0.b)** Furthermore, it broke parity with middle cleaning (which includes inlined cross-crate re-exports) re-regressing parts of rust-lang#44306 I once fixed in PR rust-lang#103885.

**(1)** Lastly, PR rust-lang#139035 introduced an ICE triggered by the following input file:

```rs
trait Trait { fn anon(()) {} } // internal error: entered unreachable code
```

---

This PR fixes all of these regressions and in the first commit renames several types and fns to be more ~~correct~~ descriptive and legible.

~~It also refactors `Param.name` to be of type `Option<Symbol>` instead `Symbol` (where `None` ~ `kw::Empty`), so rendering mistakes like that can no longer creep in like that (ignoring tests). CC rust-lang#137978.~~ Independently done in PR rust-lang#139846 a day prior.

Author: matthiaskrgr

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

@@ -1052,7 +1052,7 @@ fn clean_fn_or_proc_macro<'tcx>(
     match macro_kind {
         Some(kind) => clean_proc_macro(item, name, kind, cx),
         None => {
-            let mut func = clean_function(cx, sig, generics, FunctionArgs::Body(body_id));
+            let mut func = clean_function(cx, sig, generics, ParamsSrc::Body(body_id));
             clean_fn_decl_legacy_const_generics(&mut func, attrs);
             FunctionItem(func)
         }
@@ -1071,16 +1071,11 @@ 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.values.insert(
+                    let GenericParamDef { name, kind, .. } = func.generics.params.remove(0);
+                    if let GenericParamDefKind::Const { ty, .. } = kind {
+                        func.decl.inputs.insert(
                             a.get() as _,
-                            Argument { name: Some(name), type_: *ty, is_const: true },
+                            Parameter { name: Some(name), type_: *ty, is_const: true },
                         );
                     } else {
                         panic!("unexpected non const in position {pos}");
@@ -1092,7 +1087,7 @@ fn clean_fn_decl_legacy_const_generics(func: &mut Function, attrs: &[hir::Attrib
     }
 }
 
-enum FunctionArgs<'tcx> {
+enum ParamsSrc<'tcx> {
     Body(hir::BodyId),
     Idents(&'tcx [Option<Ident>]),
 }
@@ -1101,86 +1096,62 @@ fn clean_function<'tcx>(
     cx: &mut DocContext<'tcx>,
     sig: &hir::FnSig<'tcx>,
     generics: &hir::Generics<'tcx>,
-    args: FunctionArgs<'tcx>,
+    params: ParamsSrc<'tcx>,
 ) -> Box<Function> {
     let (generics, decl) = enter_impl_trait(cx, |cx| {
-        // NOTE: generics must be cleaned before args
+        // NOTE: Generics must be cleaned before params.
         let generics = clean_generics(generics, cx);
-        let args = match args {
-            FunctionArgs::Body(body_id) => {
-                clean_args_from_types_and_body_id(cx, sig.decl.inputs, body_id)
-            }
-            FunctionArgs::Idents(idents) => {
-                clean_args_from_types_and_names(cx, sig.decl.inputs, idents)
-            }
+        let params = match params {
+            ParamsSrc::Body(body_id) => clean_params_via_body(cx, sig.decl.inputs, body_id),
+            // 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_args(cx, sig.decl, Some(&sig.header), args);
+        let decl = clean_fn_decl_with_params(cx, sig.decl, Some(&sig.header), params);
         (generics, decl)
     });
     Box::new(Function { decl, generics })
 }
 
-fn clean_args_from_types_and_names<'tcx>(
+fn clean_params<'tcx>(
     cx: &mut DocContext<'tcx>,
     types: &[hir::Ty<'tcx>],
     idents: &[Option<Ident>],
-) -> Arguments {
-    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()) {
-        Some(kw::Underscore)
-    } else {
-        None
-    };
-
-    Arguments {
-        values: types
-            .iter()
-            .enumerate()
-            .map(|(i, ty)| Argument {
-                type_: clean_ty(ty, cx),
-                name: idents.get(i).and_then(nonempty_name).or(default_name),
-                is_const: false,
-            })
-            .collect(),
-    }
+    postprocess: impl Fn(Option<Ident>) -> Option<Symbol>,
+) -> Vec<Parameter> {
+    types
+        .iter()
+        .enumerate()
+        .map(|(i, ty)| Parameter {
+            name: postprocess(idents[i]),
+            type_: clean_ty(ty, cx),
+            is_const: false,
+        })
+        .collect()
 }
 
-fn clean_args_from_types_and_body_id<'tcx>(
+fn clean_params_via_body<'tcx>(
     cx: &mut DocContext<'tcx>,
     types: &[hir::Ty<'tcx>],
     body_id: hir::BodyId,
-) -> Arguments {
-    let body = cx.tcx.hir_body(body_id);
-
-    Arguments {
-        values: types
-            .iter()
-            .zip(body.params)
-            .map(|(ty, param)| Argument {
-                name: Some(name_from_pat(param.pat)),
-                type_: clean_ty(ty, cx),
-                is_const: false,
-            })
-            .collect(),
-    }
+) -> Vec<Parameter> {
+    types
+        .iter()
+        .zip(cx.tcx.hir_body(body_id).params)
+        .map(|(ty, param)| Parameter {
+            name: Some(name_from_pat(param.pat)),
+            type_: clean_ty(ty, cx),
+            is_const: false,
+        })
+        .collect()
 }
 
-fn clean_fn_decl_with_args<'tcx>(
+fn clean_fn_decl_with_params<'tcx>(
     cx: &mut DocContext<'tcx>,
     decl: &hir::FnDecl<'tcx>,
     header: Option<&hir::FnHeader>,
-    args: Arguments,
+    params: Vec<Parameter>,
 ) -> FnDecl {
     let mut output = match decl.output {
         hir::FnRetTy::Return(typ) => clean_ty(typ, cx),
@@ -1191,18 +1162,14 @@ fn clean_fn_decl_with_args<'tcx>(
     {
         output = output.sugared_async_return_type();
     }
-    FnDecl { inputs: args, output, c_variadic: decl.c_variadic }
+    FnDecl { inputs: params, output, c_variadic: decl.c_variadic }
 }
 
 fn clean_poly_fn_sig<'tcx>(
     cx: &mut DocContext<'tcx>,
     did: Option<DefId>,
     sig: ty::PolyFnSig<'tcx>,
 ) -> FnDecl {
-    let mut names = did.map_or(&[] as &[_], |did| cx.tcx.fn_arg_idents(did)).iter();
-
-    // We assume all empty tuples are default return type. This theoretically can discard `-> ()`,
-    // but shouldn't change any code meaning.
     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
@@ -1215,25 +1182,25 @@ fn clean_poly_fn_sig<'tcx>(
         output = output.sugared_async_return_type();
     }
 
-    FnDecl {
-        output,
-        c_variadic: sig.skip_binder().c_variadic,
-        inputs: Arguments {
-            values: sig
-                .inputs()
-                .iter()
-                .map(|t| Argument {
-                    type_: clean_middle_ty(t.map_bound(|t| *t), cx, None, None),
-                    name: Some(if let Some(Some(ident)) = names.next() {
-                        ident.name
-                    } else {
-                        kw::Underscore
-                    }),
-                    is_const: false,
-                })
-                .collect(),
-        },
-    }
+    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(|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();
+
+    FnDecl { inputs: params, output, c_variadic: sig.skip_binder().c_variadic }
 }
 
 fn clean_trait_ref<'tcx>(trait_ref: &hir::TraitRef<'tcx>, cx: &mut DocContext<'tcx>) -> Path {
@@ -1273,11 +1240,11 @@ fn clean_trait_item<'tcx>(trait_item: &hir::TraitItem<'tcx>, cx: &mut DocContext
                 RequiredAssocConstItem(generics, Box::new(clean_ty(ty, cx)))
             }
             hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Provided(body)) => {
-                let m = clean_function(cx, sig, trait_item.generics, FunctionArgs::Body(body));
+                let m = clean_function(cx, sig, trait_item.generics, ParamsSrc::Body(body));
                 MethodItem(m, None)
             }
             hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Required(idents)) => {
-                let m = clean_function(cx, sig, trait_item.generics, FunctionArgs::Idents(idents));
+                let m = clean_function(cx, sig, trait_item.generics, ParamsSrc::Idents(idents));
                 RequiredMethodItem(m)
             }
             hir::TraitItemKind::Type(bounds, Some(default)) => {
@@ -1318,7 +1285,7 @@ pub(crate) fn clean_impl_item<'tcx>(
                 type_: clean_ty(ty, cx),
             })),
             hir::ImplItemKind::Fn(ref sig, body) => {
-                let m = clean_function(cx, sig, impl_.generics, FunctionArgs::Body(body));
+                let m = clean_function(cx, sig, impl_.generics, ParamsSrc::Body(body));
                 let defaultness = cx.tcx.defaultness(impl_.owner_id);
                 MethodItem(m, Some(defaultness))
             }
@@ -1390,14 +1357,14 @@ pub(crate) fn clean_middle_assoc_item(assoc_item: &ty::AssocItem, cx: &mut DocCo
                     }
                     ty::AssocItemContainer::Trait => tcx.types.self_param,
                 };
-                let self_arg_ty =
+                let self_param_ty =
                     tcx.fn_sig(assoc_item.def_id).instantiate_identity().input(0).skip_binder();
-                if self_arg_ty == self_ty {
-                    item.decl.inputs.values[0].type_ = SelfTy;
-                } else if let ty::Ref(_, ty, _) = *self_arg_ty.kind()
+                if self_param_ty == self_ty {
+                    item.decl.inputs[0].type_ = SelfTy;
+                } else if let ty::Ref(_, ty, _) = *self_param_ty.kind()
                     && ty == self_ty
                 {
-                    match item.decl.inputs.values[0].type_ {
+                    match item.decl.inputs[0].type_ {
                         BorrowedRef { ref mut type_, .. } => **type_ = SelfTy,
                         _ => unreachable!(),
                     }
@@ -2611,15 +2578,25 @@ fn clean_bare_fn_ty<'tcx>(
     cx: &mut DocContext<'tcx>,
 ) -> BareFunctionDecl {
     let (generic_params, decl) = enter_impl_trait(cx, |cx| {
-        // NOTE: generics must be cleaned before args
+        // NOTE: Generics must be cleaned before params.
         let generic_params = bare_fn
             .generic_params
             .iter()
             .filter(|p| !is_elided_lifetime(p))
             .map(|x| clean_generic_param(cx, None, x))
             .collect();
-        let args = clean_args_from_types_and_names(cx, bare_fn.decl.inputs, bare_fn.param_idents);
-        let decl = clean_fn_decl_with_args(cx, bare_fn.decl, None, args);
+        // 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)
     });
     BareFunctionDecl { safety: bare_fn.safety, abi: bare_fn.abi, decl, generic_params }
@@ -2629,7 +2606,6 @@ fn clean_unsafe_binder_ty<'tcx>(
     unsafe_binder_ty: &hir::UnsafeBinderTy<'tcx>,
     cx: &mut DocContext<'tcx>,
 ) -> UnsafeBinderTy {
-    // NOTE: generics must be cleaned before args
     let generic_params = unsafe_binder_ty
         .generic_params
         .iter()
@@ -3155,7 +3131,7 @@ fn clean_maybe_renamed_foreign_item<'tcx>(
     cx.with_param_env(def_id, |cx| {
         let kind = match item.kind {
             hir::ForeignItemKind::Fn(sig, idents, generics) => ForeignFunctionItem(
-                clean_function(cx, &sig, generics, FunctionArgs::Idents(idents)),
+                clean_function(cx, &sig, generics, ParamsSrc::Idents(idents)),
                 sig.header.safety(),
             ),
             hir::ForeignItemKind::Static(ty, mutability, safety) => ForeignStaticItem(

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

@@ -1407,32 +1407,28 @@ pub(crate) struct Function {
 
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]
 pub(crate) struct FnDecl {
-    pub(crate) inputs: Arguments,
+    pub(crate) inputs: Vec<Parameter>,
     pub(crate) output: Type,
     pub(crate) c_variadic: bool,
 }
 
 impl FnDecl {
     pub(crate) fn receiver_type(&self) -> Option<&Type> {
-        self.inputs.values.first().and_then(|v| v.to_receiver())
+        self.inputs.first().and_then(|v| v.to_receiver())
     }
 }
 
+/// A function parameter.
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]
-pub(crate) struct Arguments {
-    pub(crate) values: Vec<Argument>,
-}
-
-#[derive(Clone, PartialEq, Eq, Debug, Hash)]
-pub(crate) struct Argument {
-    pub(crate) type_: Type,
+pub(crate) struct Parameter {
     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>.
     pub(crate) is_const: bool,
 }
 
-impl Argument {
+impl Parameter {
     pub(crate) fn to_receiver(&self) -> Option<&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,