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

fmease · 2025-04-16T12:15:25Z

(0) PR #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:

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 #44306 I once fixed in PR #103885.

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

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 #137978. Independently done in PR #139846 a day prior.

rustbot · 2025-04-16T12:15:31Z

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

src/librustdoc/clean/mod.rs

src/librustdoc/clean/utils.rs

src/librustdoc/json/conversions.rs

src/librustdoc/clean/mod.rs

src/librustdoc/clean/types.rs

GuillaumeGomez · 2025-04-16T20:09:15Z

It's great, thanks a lot!

@bors r+ rollup

bors · 2025-04-16T20:09:18Z

📌 Commit a662fd9 has been approved by GuillaumeGomez

It is now in the queue for this repository.

fmease · 2025-04-16T22:12:33Z

This will conflict which nnethercote's PR #139846 which incidentally also changed the type of fn param names from Symbol to Option<Symbol>, so let's wait for that to merge.

@bors r-
@rustbot blocked

…params from "argument" to "parameter"

fmease · 2025-04-17T06:57:51Z

Rebased.
@bors r=GuillaumeGomez

bors · 2025-04-17T06:57:55Z

📌 Commit 82ff0a0 has been approved by GuillaumeGomez

It is now in the queue for this repository.

…ng, 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.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139114 (Implement `pin!()` using `super let`) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…ng, 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.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

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.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned GuillaumeGomez Apr 16, 2025

fmease commented Apr 16, 2025

View reviewed changes

src/librustdoc/clean/mod.rs Show resolved Hide resolved

src/librustdoc/clean/utils.rs Show resolved Hide resolved

src/librustdoc/json/conversions.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Apr 16, 2025

View reviewed changes

src/librustdoc/clean/mod.rs Show resolved Hide resolved

GuillaumeGomez reviewed Apr 16, 2025

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Apr 16, 2025

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Apr 16, 2025

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

fmease force-pushed the rustdoc-fix-fn-param-handling branch 3 times, most recently from d38d576 to a662fd9 Compare April 16, 2025 13:49

fmease requested a review from GuillaumeGomez April 16, 2025 15:38

GuillaumeGomez approved these changes Apr 16, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2025

rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Apr 16, 2025

fmease added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2025

This comment was marked as resolved.

Sign in to view

fmease added 2 commits April 17, 2025 08:55

rustdoc/clean: Change terminology of items pertaining to (formal) fn …

6214674

…params from "argument" to "parameter"

rustdoc: Properly clean fn params in all contexts

82ff0a0

fmease force-pushed the rustdoc-fix-fn-param-handling branch from a662fd9 to 82ff0a0 Compare April 17, 2025 06:56

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 17, 2025

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 9 pull requests #139957

Closed

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 8 pull requests #139980

Closed

matthiaskrgr mentioned this pull request Apr 18, 2025

Rollup of 7 pull requests #139996

Merged

bors merged commit cbf2662 into rust-lang:master Apr 18, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 18, 2025

fmease deleted the rustdoc-fix-fn-param-handling branch April 18, 2025 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fmease commented Apr 16, 2025 •

edited

Loading

rustbot commented Apr 16, 2025

GuillaumeGomez commented Apr 16, 2025

bors commented Apr 16, 2025

fmease commented Apr 16, 2025 •

edited

Loading

This comment was marked as resolved.

fmease commented Apr 17, 2025

bors commented Apr 17, 2025

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

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

Conversation

fmease commented Apr 16, 2025 • edited Loading

rustbot commented Apr 16, 2025

GuillaumeGomez commented Apr 16, 2025

bors commented Apr 16, 2025

fmease commented Apr 16, 2025 • edited Loading

This comment was marked as resolved.

fmease commented Apr 17, 2025

bors commented Apr 17, 2025

fmease commented Apr 16, 2025 •

edited

Loading

fmease commented Apr 16, 2025 •

edited

Loading