Skip to content
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

scanner: eliminate panics via unwrap() #24

Closed
wants to merge 1 commit into from
Closed

scanner: eliminate panics via unwrap() #24

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
52 changes: 36 additions & 16 deletions src/scanner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,9 +463,13 @@ impl<T: Iterator<Item = char>> Scanner<T> {
return;
}
for _ in 0..(count - self.buffer.len()) {
self.buffer
if self
.buffer
.push_back(self.rdr.next().unwrap_or('\0'))
.unwrap();
.is_err()
{
break;
}
Comment on lines +466 to +472
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this change is the right thing to do. The buffer errors when we try to push past its capacity (since it is fixed and cannot reallocate). This is something that, if we hit, is because count > self.buffer.len(), which is a logic error in the scanner that should be fixed not in lookahead but on its call site.

The unwrap here should never be hit. Breaking out of the loop might trigger a plethora of panics in the scanner, since the code assumes that, after calling lookahead(n), it is safe to access buffer[0..n-1].

In my opinion, the best course of action here would be to rather assert(count <= self.buffer.len()), ensuring we never call lookahead with a count that is too high. But that would go against removing the panics. I'd gladly have your opinion on this.

}
}

Expand Down Expand Up @@ -1470,7 +1474,7 @@ impl<T: Iterator<Item = char>> Scanner<T> {
fn decrease_flow_level(&mut self) {
if self.flow_level > 0 {
self.flow_level -= 1;
self.simple_keys.pop().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this unwrap would silence a potential error in the scanner logic.

self.simple_keys.pop();
}
}

Expand Down Expand Up @@ -1777,7 +1781,7 @@ impl<T: Iterator<Item = char>> Scanner<T> {
// Our last character read is stored in `c`. It is either an EOF or a break. In any
// case, we need to push it back into `self.buffer` so it may be properly read
// after. We must not insert it in `string`.
self.buffer.push_back(c).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this unwrap would silence a potential error in the scanner logic.

self.buffer.push_back(c).ok();

// We need to manually update our position; we haven't called a `skip` function.
self.mark.col += line_buffer.len();
Expand Down Expand Up @@ -2094,13 +2098,13 @@ impl<T: Iterator<Item = char>> Scanner<T> {
'/' => ret = '/',
'\\' => ret = '\\',
// Unicode next line (#x85)
'N' => ret = char::from_u32(0x85).unwrap(),
'N' => ret = char::from_u32(0x85).unwrap_or('\n'),
// Unicode non-breaking space (#xA0)
'_' => ret = char::from_u32(0xA0).unwrap(),
'_' => ret = char::from_u32(0xA0).unwrap_or(' '),
// Unicode line separator (#x2028)
'L' => ret = char::from_u32(0x2028).unwrap(),
'L' => ret = char::from_u32(0x2028).unwrap_or('\n'),
// Unicode paragraph separator (#x2029)
'P' => ret = char::from_u32(0x2029).unwrap(),
'P' => ret = char::from_u32(0x2029).unwrap_or('\n'),
'x' => code_length = 2,
'u' => code_length = 4,
'U' => code_length = 8,
Expand Down Expand Up @@ -2323,7 +2327,12 @@ impl<T: Iterator<Item = char>> Scanner<T> {

/// Fetch a value from a mapping (after a `:`).
fn fetch_value(&mut self) -> ScanResult {
let sk = self.simple_keys.last().unwrap().clone();
let Some(sk) = self.simple_keys.last().cloned() else {
return Err(ScanError::new(
self.mark,
"internal scanner error: simple_keys is empty",
));
};
let start_mark = self.mark;
self.implicit_flow_mapping = self.flow_level > 0 && !self.flow_mapping_started;

Expand Down Expand Up @@ -2365,7 +2374,9 @@ impl<T: Iterator<Item = char>> Scanner<T> {
);
self.roll_one_col_indent();

self.simple_keys.last_mut().unwrap().possible = false;
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this unwrap would silence a potential error in the scanner logic.

if let Some(ref mut key) = self.simple_keys.last_mut() {
key.possible = false;
}
self.disallow_simple_key();
} else {
if self.implicit_flow_mapping {
Expand Down Expand Up @@ -2447,10 +2458,14 @@ impl<T: Iterator<Item = char>> Scanner<T> {
return;
}
while self.indent > col {
let indent = self.indents.pop().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this unwrap would silence a potential error in the scanner logic.

self.indent = indent.indent;
if indent.needs_block_end {
self.tokens.push_back(Token(self.mark, TokenType::BlockEnd));
if let Some(indent) = self.indents.pop() {
self.indent = indent.indent;
if indent.needs_block_end {
self.tokens.push_back(Token(self.mark, TokenType::BlockEnd));
}
} else {
self.indent = 0;
break;
}
}
}
Expand Down Expand Up @@ -2486,7 +2501,10 @@ impl<T: Iterator<Item = char>> Scanner<T> {
if self.simple_key_allowed {
let required = self.flow_level == 0
&& self.indent == (self.mark.col as isize)
&& self.indents.last().unwrap().needs_block_end;
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this unwrap would silence a potential error in the scanner logic.

Copy link
Owner

Choose a reason for hiding this comment

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

This could also be self.indents.last().map_or(false, |ident| ident.needs_block_end).

&& match self.indents.last() {
Some(indent) => indent.needs_block_end,
None => false,
};
let mut sk = SimpleKey::new(self.mark);
sk.possible = true;
sk.required = required;
Expand All @@ -2498,7 +2516,9 @@ impl<T: Iterator<Item = char>> Scanner<T> {
}

fn remove_simple_key(&mut self) -> ScanResult {
let last = self.simple_keys.last_mut().unwrap();
let Some(last) = self.simple_keys.last_mut() else {
return Err(ScanError::new(self.mark, "simple key missing"));
};
if last.possible && last.required {
return Err(ScanError::new(self.mark, "simple key expected"));
}
Expand Down
Loading