From 169b84fee38827d0e4e437696baf7149d9c2adf7 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Tue, 9 Nov 2021 18:09:09 -0800 Subject: [PATCH 1/4] Replace where-bounded Clean impl with function This was the only Clean impl I found with `where` bounds. This impl was doubly-confusing: it was implemented on a tuple and it was polymorphic. Combined, this caused a "spooky action at a distance" effect to make the code very confusing. --- src/librustdoc/clean/mod.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 3db0ef17fd810..2fbccfda8e674 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -761,8 +761,9 @@ fn clean_fn_or_proc_macro( impl<'a> Clean for (&'a hir::FnSig<'a>, &'a hir::Generics<'a>, hir::BodyId) { fn clean(&self, cx: &mut DocContext<'_>) -> Function { - let (generics, decl) = - enter_impl_trait(cx, |cx| (self.1.clean(cx), (&*self.0.decl, self.2).clean(cx))); + let (generics, decl) = enter_impl_trait(cx, |cx| { + (self.1.clean(cx), clean_fn_decl_with_args(cx, &*self.0.decl, self.2)) + }); Function { decl, generics, header: self.0.header } } } @@ -804,16 +805,18 @@ impl<'a> Clean for (&'a [hir::Ty<'a>], hir::BodyId) { } } -impl<'a, A: Copy> Clean for (&'a hir::FnDecl<'a>, A) +fn clean_fn_decl_with_args<'a, A: Copy>( + cx: &mut DocContext<'_>, + decl: &'a hir::FnDecl<'a>, + args: A, +) -> FnDecl where (&'a [hir::Ty<'a>], A): Clean, { - fn clean(&self, cx: &mut DocContext<'_>) -> FnDecl { - FnDecl { - inputs: (self.0.inputs, self.1).clean(cx), - output: self.0.output.clean(cx), - c_variadic: self.0.c_variadic, - } + FnDecl { + inputs: (decl.inputs, args).clean(cx), + output: decl.output.clean(cx), + c_variadic: decl.c_variadic, } } @@ -894,7 +897,7 @@ impl Clean for hir::TraitItem<'_> { } hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Required(names)) => { let (generics, decl) = enter_impl_trait(cx, |cx| { - (self.generics.clean(cx), (sig.decl, names).clean(cx)) + (self.generics.clean(cx), clean_fn_decl_with_args(cx, sig.decl, names)) }); let mut t = Function { header: sig.header, decl, generics }; if t.header.constness == hir::Constness::Const @@ -1728,7 +1731,7 @@ impl Clean for hir::BareFnTy<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> BareFunctionDecl { let (generic_params, decl) = enter_impl_trait(cx, |cx| { let generic_params = self.generic_params.iter().map(|x| x.clean(cx)).collect(); - let decl = (self.decl, self.param_names).clean(cx); + let decl = clean_fn_decl_with_args(cx, self.decl, self.param_names); (generic_params, decl) }); BareFunctionDecl { unsafety: self.unsafety, abi: self.abi, decl, generic_params } @@ -2025,8 +2028,9 @@ impl Clean for (&hir::ForeignItem<'_>, Option) { let kind = match item.kind { hir::ForeignItemKind::Fn(decl, names, ref generics) => { let abi = cx.tcx.hir().get_foreign_abi(item.hir_id()); - let (generics, decl) = - enter_impl_trait(cx, |cx| (generics.clean(cx), (decl, names).clean(cx))); + let (generics, decl) = enter_impl_trait(cx, |cx| { + (generics.clean(cx), clean_fn_decl_with_args(cx, decl, names)) + }); ForeignFunctionItem(Function { decl, generics, From cf6a73c1a4a363a1239f2db32b89ff3d2affb6c2 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Tue, 9 Nov 2021 18:23:08 -0800 Subject: [PATCH 2/4] Remove where bound from `clean_fn_decl_with_args` Basically, this entails moving the arguments cleaning to the call site. I extracted several local variables because: 1. It makes the code easier to read and understand. 2. If I hadn't, the extra `clean()` calls would have caused complicated tuples to be split across several lines. 3. I couldn't just extract local variables for `args` because then the arguments would be cleaned *before* the generics, while rustdoc expects them to be cleaned *after*. Only extracting `args` caused panics like this: thread 'rustc' panicked at 'assertion failed: cx.impl_trait_bounds.is_empty()', src/librustdoc/clean/utils.rs:462:5 Extracting variables makes the control flow -- and the required order of cleaning -- more explicit. --- src/librustdoc/clean/mod.rs | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 2fbccfda8e674..9238a0bc3f38c 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -762,7 +762,10 @@ fn clean_fn_or_proc_macro( impl<'a> Clean for (&'a hir::FnSig<'a>, &'a hir::Generics<'a>, hir::BodyId) { fn clean(&self, cx: &mut DocContext<'_>) -> Function { let (generics, decl) = enter_impl_trait(cx, |cx| { - (self.1.clean(cx), clean_fn_decl_with_args(cx, &*self.0.decl, self.2)) + let generics = self.1.clean(cx); + let args = (self.0.decl.inputs, self.2).clean(cx); + let decl = clean_fn_decl_with_args(cx, &*self.0.decl, args); + (generics, decl) }); Function { decl, generics, header: self.0.header } } @@ -805,19 +808,12 @@ impl<'a> Clean for (&'a [hir::Ty<'a>], hir::BodyId) { } } -fn clean_fn_decl_with_args<'a, A: Copy>( +fn clean_fn_decl_with_args( cx: &mut DocContext<'_>, - decl: &'a hir::FnDecl<'a>, - args: A, -) -> FnDecl -where - (&'a [hir::Ty<'a>], A): Clean, -{ - FnDecl { - inputs: (decl.inputs, args).clean(cx), - output: decl.output.clean(cx), - c_variadic: decl.c_variadic, - } + decl: &hir::FnDecl<'_>, + args: Arguments, +) -> FnDecl { + FnDecl { inputs: args, output: decl.output.clean(cx), c_variadic: decl.c_variadic } } impl<'tcx> Clean for (DefId, ty::PolyFnSig<'tcx>) { @@ -897,7 +893,10 @@ impl Clean for hir::TraitItem<'_> { } hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Required(names)) => { let (generics, decl) = enter_impl_trait(cx, |cx| { - (self.generics.clean(cx), clean_fn_decl_with_args(cx, sig.decl, names)) + let generics = self.generics.clean(cx); + let args = (sig.decl.inputs, names).clean(cx); + let decl = clean_fn_decl_with_args(cx, sig.decl, args); + (generics, decl) }); let mut t = Function { header: sig.header, decl, generics }; if t.header.constness == hir::Constness::Const @@ -1731,7 +1730,8 @@ impl Clean for hir::BareFnTy<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> BareFunctionDecl { let (generic_params, decl) = enter_impl_trait(cx, |cx| { let generic_params = self.generic_params.iter().map(|x| x.clean(cx)).collect(); - let decl = clean_fn_decl_with_args(cx, self.decl, self.param_names); + let args = (self.decl.inputs, self.param_names).clean(cx); + let decl = clean_fn_decl_with_args(cx, self.decl, args); (generic_params, decl) }); BareFunctionDecl { unsafety: self.unsafety, abi: self.abi, decl, generic_params } @@ -2029,7 +2029,10 @@ impl Clean for (&hir::ForeignItem<'_>, Option) { hir::ForeignItemKind::Fn(decl, names, ref generics) => { let abi = cx.tcx.hir().get_foreign_abi(item.hir_id()); let (generics, decl) = enter_impl_trait(cx, |cx| { - (generics.clean(cx), clean_fn_decl_with_args(cx, decl, names)) + let generics = generics.clean(cx); + let args = (decl.inputs, names).clean(cx); + let decl = clean_fn_decl_with_args(cx, decl, args); + (generics, decl) }); ForeignFunctionItem(Function { decl, From c615b11aa7a4e41a7b11c9bfb3a4fe101c4f973f Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Tue, 9 Nov 2021 18:30:24 -0800 Subject: [PATCH 3/4] Remove unnecessary reborrows --- src/librustdoc/clean/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 9238a0bc3f38c..4e1fd4f1136bc 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -109,7 +109,10 @@ impl Clean for hir::GenericBound<'_> { }; GenericBound::TraitBound( - PolyTrait { trait_: (trait_ref, &*bindings).clean(cx), generic_params: vec![] }, + PolyTrait { + trait_: (trait_ref, &bindings[..]).clean(cx), + generic_params: vec![], + }, hir::TraitBoundModifier::None, ) } @@ -764,7 +767,7 @@ impl<'a> Clean for (&'a hir::FnSig<'a>, &'a hir::Generics<'a>, hir::Bo let (generics, decl) = enter_impl_trait(cx, |cx| { let generics = self.1.clean(cx); let args = (self.0.decl.inputs, self.2).clean(cx); - let decl = clean_fn_decl_with_args(cx, &*self.0.decl, args); + let decl = clean_fn_decl_with_args(cx, self.0.decl, args); (generics, decl) }); Function { decl, generics, header: self.0.header } From c20ee3e4d6cf00e80544227aee2e682ce52ab03e Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Tue, 9 Nov 2021 20:59:45 -0800 Subject: [PATCH 4/4] Add comments ensuring that generics are cleaned before args Otherwise, rustdoc panics with messages like this: thread 'rustc' panicked at 'assertion failed: cx.impl_trait_bounds.is_empty()', src/librustdoc/clean/utils.rs:462:5 This ordering requirement is unrelated to the `clean_fn_decl_with_args` refactoring, but the requirement was uncovered as part of that change. I'm not sure if *all* of these places have the requirement, but I added comments to them just in case. --- src/librustdoc/clean/inline.rs | 1 + src/librustdoc/clean/mod.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index d670288270a40..1324080b87e9c 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -229,6 +229,7 @@ fn build_external_function(cx: &mut DocContext<'_>, did: DefId) -> clean::Functi let asyncness = cx.tcx.asyncness(did); let predicates = cx.tcx.predicates_of(did); let (generics, decl) = clean::enter_impl_trait(cx, |cx| { + // NOTE: generics need to be cleaned before the decl! ((cx.tcx.generics_of(did), predicates).clean(cx), (did, sig).clean(cx)) }); clean::Function { diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 4e1fd4f1136bc..d7eecdc598c93 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -765,6 +765,7 @@ fn clean_fn_or_proc_macro( impl<'a> Clean for (&'a hir::FnSig<'a>, &'a hir::Generics<'a>, hir::BodyId) { fn clean(&self, cx: &mut DocContext<'_>) -> Function { let (generics, decl) = enter_impl_trait(cx, |cx| { + // NOTE: generics must be cleaned before args let generics = self.1.clean(cx); let args = (self.0.decl.inputs, self.2).clean(cx); let decl = clean_fn_decl_with_args(cx, self.0.decl, args); @@ -896,6 +897,7 @@ impl Clean for hir::TraitItem<'_> { } hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Required(names)) => { let (generics, decl) = enter_impl_trait(cx, |cx| { + // NOTE: generics must be cleaned before args let generics = self.generics.clean(cx); let args = (sig.decl.inputs, names).clean(cx); let decl = clean_fn_decl_with_args(cx, sig.decl, args); @@ -1732,6 +1734,7 @@ impl Clean for hir::PathSegment<'_> { impl Clean for hir::BareFnTy<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> BareFunctionDecl { let (generic_params, decl) = enter_impl_trait(cx, |cx| { + // NOTE: generics must be cleaned before args let generic_params = self.generic_params.iter().map(|x| x.clean(cx)).collect(); let args = (self.decl.inputs, self.param_names).clean(cx); let decl = clean_fn_decl_with_args(cx, self.decl, args); @@ -2032,6 +2035,7 @@ impl Clean for (&hir::ForeignItem<'_>, Option) { hir::ForeignItemKind::Fn(decl, names, ref generics) => { let abi = cx.tcx.hir().get_foreign_abi(item.hir_id()); let (generics, decl) = enter_impl_trait(cx, |cx| { + // NOTE: generics must be cleaned before args let generics = generics.clean(cx); let args = (decl.inputs, names).clean(cx); let decl = clean_fn_decl_with_args(cx, decl, args);