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.

Sign up for GitHub

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
simonwuelker merged 1 commit into from
Apr 12, 2025
+81 −67
Merged

Remove all direct uses of unwrap_or_return! in html5ever #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions html5ever/src/tokenizer/char_ref/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,19 +137,19 @@ impl CharRefTokenizer {
tokenizer: &Tokenizer<Sink>,
input: &BufferQueue,
) -> Status {
match unwrap_or_return!(tokenizer.peek(input), Stuck) {
'a'..='z' | 'A'..='Z' | '0'..='9' => {
match tokenizer.peek(input) {
Some('a'..='z' | 'A'..='Z' | '0'..='9') => {
self.state = Named;
self.name_buf_opt = Some(StrTendril::new());
Progress
},

'#' => {
Some('#') => {
tokenizer.discard_char(input);
self.state = Octothorpe;
Progress
},
_ => self.finish_none(),
Some(_) => self.finish_none(),
None => Stuck,
}
}

Expand All @@ -158,18 +158,17 @@ impl CharRefTokenizer {
tokenizer: &Tokenizer<Sink>,
input: &BufferQueue,
) -> Status {
let c = unwrap_or_return!(tokenizer.peek(input), Stuck);
match c {
'x' | 'X' => {
match tokenizer.peek(input) {
Some(c @ ('x' | 'X')) => {
tokenizer.discard_char(input);
self.hex_marker = Some(c);
self.state = Numeric(16);
},

_ => {
Some(_) => {
self.hex_marker = None;
self.state = Numeric(10);
},
None => return Stuck,
}
Progress
}
Expand All @@ -180,7 +179,9 @@ impl CharRefTokenizer {
input: &BufferQueue,
base: u32,
) -> Status {
let c = unwrap_or_return!(tokenizer.peek(input), Stuck);
let Some(c) = tokenizer.peek(input) else {
return Stuck;
};
match c.to_digit(base) {
Some(n) => {
tokenizer.discard_char(input);
Expand Down Expand Up @@ -209,11 +210,12 @@ impl CharRefTokenizer {
tokenizer: &Tokenizer<Sink>,
input: &BufferQueue,
) -> Status {
match unwrap_or_return!(tokenizer.peek(input), Stuck) {
';' => tokenizer.discard_char(input),
_ => tokenizer.emit_error(Borrowed(
match tokenizer.peek(input) {
Some(';') => tokenizer.discard_char(input),
Some(_) => tokenizer.emit_error(Borrowed(
"Semicolon missing after numeric character reference",
)),
None => return Stuck,
};
self.finish_numeric(tokenizer)
}
Expand Down Expand Up @@ -274,7 +276,9 @@ impl CharRefTokenizer {
) -> Status {
// peek + discard skips over newline normalization, therefore making it easier to
// un-consume
let c = unwrap_or_return!(tokenizer.peek(input), Stuck);
let Some(c) = tokenizer.peek(input) else {
return Stuck;
};
tokenizer.discard_char(input);
self.name_buf_mut().push_char(c);
match data::NAMED_ENTITIES.get(&self.name_buf()[..]) {
Expand Down Expand Up @@ -399,7 +403,9 @@ impl CharRefTokenizer {
) -> Status {
// peek + discard skips over newline normalization, therefore making it easier to
// un-consume
let c = unwrap_or_return!(tokenizer.peek(input), Stuck);
let Some(c) = tokenizer.peek(input) else {
return Stuck;
};
tokenizer.discard_char(input);
self.name_buf_mut().push_char(c);
match c {
Expand Down
106 changes: 56 additions & 50 deletions html5ever/src/tree_builder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,16 +350,16 @@ where
self.sink
.parse_error(Borrowed("Unacknowledged self-closing tag"));
}
token = unwrap_or_return!(
more_tokens.pop_front(),
tokenizer::TokenSinkResult::Continue
);
let Some(new_token) = more_tokens.pop_front() else {
return tokenizer::TokenSinkResult::Continue;
};
token = new_token;
},
ProcessResult::DoneAckSelfClosing => {
token = unwrap_or_return!(
more_tokens.pop_front(),
tokenizer::TokenSinkResult::Continue
);
let Some(new_token) = more_tokens.pop_front() else {
return tokenizer::TokenSinkResult::Continue;
};
token = new_token;
},
ProcessResult::Reprocess(m, t) => {
self.mode.set(m);
Expand All @@ -370,7 +370,9 @@ where
},
ProcessResult::SplitWhitespace(mut buf) => {
let p = buf.pop_front_char_run(|c| c.is_ascii_whitespace());
let (first, is_ws) = unwrap_or_return!(p, tokenizer::TokenSinkResult::Continue);
let Some((first, is_ws)) = p else {
return tokenizer::TokenSinkResult::Continue;
};
let status = if is_ws {
SplitStatus::Whitespace
} else {
Expand Down Expand Up @@ -714,33 +716,33 @@ where
// 2. 3. 4.
for _ in 0..8 {
// 5.
let (fmt_elem_index, fmt_elem, fmt_elem_tag) = unwrap_or_return!(
// We clone the Handle and Tag so they don't cause an immutable borrow of self.
self.active_formatting_end_to_marker()
.iter()
.find(|&(_, _, tag)| tag.name == subject)
.map(|(i, h, t)| (i, h.clone(), t.clone())),
{
self.process_end_tag_in_body(Tag {
kind: EndTag,
name: subject,
self_closing: false,
attrs: vec![],
});
}
);
// We clone the Handle and Tag so they don't cause an immutable borrow of self.
let maybe_fmt_entry = self
.active_formatting_end_to_marker()
.iter()
.find(|&(_, _, tag)| tag.name == subject)
.map(|(i, h, t)| (i, h.clone(), t.clone()));

let Some((fmt_elem_index, fmt_elem, fmt_elem_tag)) = maybe_fmt_entry else {
return self.process_end_tag_in_body(Tag {
kind: EndTag,
name: subject,
self_closing: false,
attrs: vec![],
});
};

let fmt_elem_stack_index = unwrap_or_return!(
self.open_elems
.borrow()
.iter()
.rposition(|n| self.sink.same_node(n, &fmt_elem)),
{
self.sink
.parse_error(Borrowed("Formatting element not open"));
self.active_formatting.borrow_mut().remove(fmt_elem_index);
}
);
let Some(fmt_elem_stack_index) = self
.open_elems
.borrow()
.iter()
.rposition(|n| self.sink.same_node(n, &fmt_elem))
else {
self.sink
.parse_error(Borrowed("Formatting element not open"));
self.active_formatting.borrow_mut().remove(fmt_elem_index);
return;
};

// 7.
if !self.in_scope(default_scope, |n| self.sink.same_node(&n, &fmt_elem)) {
Expand All @@ -756,20 +758,21 @@ where
}

// 9.
let (furthest_block_index, furthest_block) = unwrap_or_return!(
self.open_elems
.borrow()
.iter()
.enumerate()
.skip(fmt_elem_stack_index)
.find(|&(_, open_element)| self.elem_in(open_element, special_tag))
.map(|(i, h)| (i, h.clone())),
let maybe_furthest_block = self
.open_elems
.borrow()
.iter()
.enumerate()
.skip(fmt_elem_stack_index)
.find(|&(_, open_element)| self.elem_in(open_element, special_tag))
.map(|(i, h)| (i, h.clone()));

let Some((furthest_block_index, furthest_block)) = maybe_furthest_block else {
// 10.
{
self.open_elems.borrow_mut().truncate(fmt_elem_stack_index);
self.active_formatting.borrow_mut().remove(fmt_elem_index);
}
);
self.open_elems.borrow_mut().truncate(fmt_elem_stack_index);
self.active_formatting.borrow_mut().remove(fmt_elem_index);
return;
};

// 11.
let common_ancestor = self.open_elems.borrow()[fmt_elem_stack_index - 1].clone();
Expand Down Expand Up @@ -1569,11 +1572,14 @@ where
}

fn handle_misnested_a_tags(&self, tag: &Tag) {
let node = unwrap_or_return!(self
let Some(node) = self
.active_formatting_end_to_marker()
.iter()
.find(|&(_, n, _)| self.html_elem_named(n, local_name!("a")))
.map(|(_, n, _)| n.clone()));
.map(|(_, n, _)| n.clone())
else {
return;
};

self.unexpected(tag);
self.adoption_agency(local_name!("a"));
Expand Down
4 changes: 3 additions & 1 deletion html5ever/src/tree_builder/rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@mrobinson mrobinson Apr 12, 2025

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

@simonwuelker simonwuelker Apr 12, 2025

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

@mrobinson mrobinson Apr 12, 2025

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.

return ProcessResult::Done;
};
self.sink.remove_from_parent(&body);

// FIXME: can we get here in the fragment case?
Expand Down