Skip to content

[rustdoc] Make more functions return fmt::Result and reduce number of .unwrap() calls - #149208

Merged
bors merged 1 commit into
rust-lang:mainfrom
GuillaumeGomez:less-unwraps
Nov 24, 2025
Merged

[rustdoc] Make more functions return fmt::Result and reduce number of .unwrap() calls#149208
bors merged 1 commit into
rust-lang:mainfrom
GuillaumeGomez:less-unwraps

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented Nov 22, 2025

Copy link
Copy Markdown
Member

Following our discussion in #149028 (comment), this PR makes more function return fmt::Result, allowing to use ? a lot more, and also reducing number of .unwrap() calls.

r? @lolbinarycat

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Nov 22, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez

GuillaumeGomez commented Nov 22, 2025

Copy link
Copy Markdown
MemberAuthor

Updated tests as well.

yotamofek
yotamofek reviewed Nov 23, 2025
View reviewed changes
Comment threadsrc/librustdoc/html/render/print_item.rs Outdated
let res = f(w);
write!(w, "</code></pre>").unwrap();
res
write!(w, r#"<pre class="rust item-decl"><code>"#)?;

@yotamofekyotamofekNov 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated nit, but maybe change this to w.write_str? Seems to be more consistent with rest of the code.

@yotamofek

yotamofek commented Nov 23, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Nice cleanup.
r=me unless you prefer to wait for another approval from binarycat :)

lolbinarycat
lolbinarycat reviewed Nov 23, 2025
View reviewed changes

@lolbinarycatlolbinarycat left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

90% of this looks good, the only thing I don't agree with is the changes to the HtmlWithLimit methods, since those write to an internal String and are thus infallible.

View changes since this review

Comment threadsrc/librustdoc/html/length_limit.rs
Comment on lines 88 to 93
if let Some(tag_name) = self.unclosed_tags.pop() {
// Close the most recently opened tag.
write!(self.buf, "</{tag_name}>").unwrap()
write!(self.buf, "</{tag_name}>")?
}
// There are valid cases where `close_tag()` is called without
// there being any tags to close. For example, this occurs when

@lolbinarycatlolbinarycatNov 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're writing to String here, so this write can never fail, therefore I think it would actually make more sense to keep the unwrap, especailly if we're just going to be calling unwrap in finish anyways.

@GuillaumeGomezGuillaumeGomezNov 24, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense. Just wanted to unify but you're right, not really worth it here.

Comment threadsrc/librustdoc/html/length_limit.rs
fn flush_queue(&mut self) {
fn flush_queue(&mut self) -> fmt::Result {
for tag_name in self.queued_tags.drain(..) {
write!(self.buf, "<{tag_name}>").unwrap();

@lolbinarycatlolbinarycatNov 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also infallible.

Comment threadsrc/librustdoc/html/length_limit.rs Outdated
Comment on lines 111 to 117
fn close_all_tags(&mut self) -> fmt::Result {
while !self.unclosed_tags.is_empty() {
self.close_tag();
self.close_tag()?;
}
Ok(())
}
}

@lolbinarycatlolbinarycatNov 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would revert this one also.

@GuillaumeGomez

GuillaumeGomez commented Nov 24, 2025

Copy link
Copy Markdown
MemberAuthor

I reverted the changes to length_limit.rs and replaced the .unwrap() calls with .expect() to provide some more context (future-me proof!).

@yotamofek

yotamofek commented Nov 24, 2025

Copy link
Copy Markdown
Contributor

@bors r=yotamofek,lolbinarycat

@bors

bors commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit c524ed7 has been approved by yotamofek,lolbinarycat

It is now in the queue for this repository.

@borsbors 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 Nov 24, 2025
@GuillaumeGomez

GuillaumeGomez commented Nov 24, 2025

Copy link
Copy Markdown
MemberAuthor

@bors rollup

@matthiaskrgrmatthiaskrgr mentioned this pull request Nov 24, 2025
Merged
bors added a commit that referenced this pull request Nov 24, 2025
@bors
Auto merge of#149276- matthiaskrgr:rollup-wlrpdrr, r=matthiaskrgr
c871d09
Rollup of 6 pull requests
Successful merges:
- #148234 (rustdoc: make mergeable crate info more usable)
- #149201 (Add suggest alternatives for Out-of-range \x escapes)
- #149208 ([rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls)
- #149252 (miri: use `tikv-jemalloc-sys` from sysroot)
- #149255 (Use `let...else` consistently in user-facing diagnostics)
- #149275 (Fix missing double-quote in `std::env::consts::OS` values)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit a68aff6 into rust-lang:mainNov 24, 2025
11 checks passed
@rustbotrustbot added this to the 1.93.0 milestone Nov 24, 2025
rust-timer added a commit that referenced this pull request Nov 24, 2025
@rust-timer
Unrolled build for#149208
a7fce4e
Rollup merge of #149208 - GuillaumeGomez:less-unwraps, r=yotamofek,lolbinarycat
[rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls
Following our discussion in #149028 (comment), this PR makes more function return `fmt::Result`, allowing to use `?` a lot more, and also reducing number of `.unwrap()` calls.
r? `@lolbinarycat`
github-actionsBot pushed a commit to rust-lang/miri that referenced this pull request Nov 25, 2025
@bors
Auto merge of #149276 - matthiaskrgr:rollup-wlrpdrr, r=matthiaskrgr
9001f22
Rollup of 6 pull requests
Successful merges:
- rust-lang/rust#148234 (rustdoc: make mergeable crate info more usable)
- rust-lang/rust#149201 (Add suggest alternatives for Out-of-range \x escapes)
- rust-lang/rust#149208 ([rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls)
- rust-lang/rust#149252 (miri: use `tikv-jemalloc-sys` from sysroot)
- rust-lang/rust#149255 (Use `let...else` consistently in user-facing diagnostics)
- rust-lang/rust#149275 (Fix missing double-quote in `std::env::consts::OS` values)
r? `@ghost`
`@rustbot` modify labels: rollup
@GuillaumeGomez
GuillaumeGomez deleted the less-unwraps branch November 25, 2025 09:49
github-actionsBot pushed a commit to rust-lang/rust-analyzer that referenced this pull request Nov 27, 2025
@bors
Auto merge of #149276 - matthiaskrgr:rollup-wlrpdrr, r=matthiaskrgr
5bd6eaa
Rollup of 6 pull requests
Successful merges:
- rust-lang/rust#148234 (rustdoc: make mergeable crate info more usable)
- rust-lang/rust#149201 (Add suggest alternatives for Out-of-range \x escapes)
- rust-lang/rust#149208 ([rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls)
- rust-lang/rust#149252 (miri: use `tikv-jemalloc-sys` from sysroot)
- rust-lang/rust#149255 (Use `let...else` consistently in user-facing diagnostics)
- rust-lang/rust#149275 (Fix missing double-quote in `std::env::consts::OS` values)
r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lolbinarycatlolbinarycatlolbinarycat left review comments
@yotamofekyotamofekyotamofek approved these changes

Assignees

@yotamofekyotamofek

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.T-rustdoc-frontendRelevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

Projects

None yet

Milestone

1.93.0

Development

Successfully merging this pull request may close these issues.

6 participants

@GuillaumeGomez@rust-log-analyzer@yotamofek@bors@lolbinarycat@rustbot