Skip to content

Remove all direct uses of unwrap_or_return! in html5ever #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 12, 2025

Conversation

simonwuelker
Copy link
Contributor

In modern rust this macro can almost always easily be replaced by let-else expressions.

Some uses inside other macros remain, those are not as easy to remove.

@simonwuelker simonwuelker changed the title Remove all direct uses of unwrap_or_return! Remove all direct uses of unwrap_or_return! in html5ever Apr 12, 2025
@simonwuelker
Copy link
Contributor Author

CI needs #596 first

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Excellent!

@@ -329,7 +329,9 @@ where
self.unexpected(&tag);
if !self.frameset_ok.get() { return ProcessResult::Done; }

let body = unwrap_or_return!(self.body_elem(), ProcessResult::Done).clone();
let Some(body) = self.body_elem().map(|b| b.clone()) else {
Copy link
Member

Choose a reason for hiding this comment

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

Can this just call Option::clone()?

Suggested change
let Some(body) = self.body_elem().map(|b| b.clone()) else {
let Some(body) = self.body_elem().clone() else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly not!

error[E0599]: the method `clone` exists for enum `Option<Ref<'_, Handle>>`, but its trait bounds were not satisfied
    --> html5ever/src/tree_builder/rules.rs:332:55
     |
332  |                     let Some(body) = self.body_elem().clone() else {
     |                                                       ^^^^^ method cannot be called on `Option<Ref<'_, Handle>>` due to unsatisfied trait bounds
     |
    ::: /home/alaska/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:1463:1
     |
1463 | pub struct Ref<'b, T: ?Sized + 'b> {
     | ---------------------------------- doesn't satisfy `Ref<'_, Handle>: Clone`
     |
    ::: /home/alaska/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:571:1
     |
571  | pub enum Option<T> {
     | ------------------ doesn't satisfy `Option<Ref<'_, Handle>>: Clone`
     |
     = note: the following trait bounds were not satisfied:
             `Ref<'_, Handle>: Clone`
             which is required by `Option<Ref<'_, Handle>>: Clone`
note: the method `clone` exists on the type `Ref<'_, Handle>`
    --> /home/alaska/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/clone.rs:165:5
     |
165  |     fn clone(&self) -> Self;
     |     ^^^^^^^^^^^^^^^^^^^^^^^^
help: consider using `Option::expect` to unwrap the `Ref<'_, Handle>` value, panicking if the value is an `Option::None`
     |
332  |                     let Some(body) = self.body_elem().expect("REASON").clone() else {
     |                                                      +++++++++++++++++

For more information about this error, try `rustc --explain E0599`.
error: could not compile `html5ever` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `html5ever` (lib test) due to 1 previous error

To be honest, I have no idea what that error is complaining about.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, sometime when there is a Ref inside it needs to be dereferenced in order to access the real clone. Option::as_deref or Option::as_ref might work here, but otherwise the closure is fine.

In modern rust this macro can easily be replaced by let-else expressions.
Some uses inside other macros remain, those are not as easy to remove.

Signed-off-by: Simon Wülker <[email protected]>
@simonwuelker simonwuelker added this pull request to the merge queue Apr 12, 2025
Merged via the queue into servo:main with commit 809eec7 Apr 12, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants