Parser::expect_exhausted swallows non-UnexpectedEof errors

[jdm]

When I make the following change:

diff --git a/src/parser.rs b/src/parser.rs
index 65a08e2..ffab3a9 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -230,8 +230,10 @@ impl<'i, 't> Parser<'i, 't> {
     pub fn expect_exhausted(&mut self) -> Result<(), BasicParseError<'i>> {
         let start_position = self.position();
         let result = match self.next() {
-            //FIXME: swallowing non-UnexpectedEof errors seems wrong, but tests fail otherwise.
-            Err(_) => Ok(()),
+            // FIXME: swallowing non-UnexpectedEof errors seems wrong, but tests fail otherwise.
+            //Err(_) => Ok(()),
+            Err(BasicParseError::UnexpectedEof) => Ok(()),
+            Err(e) => unreachable!("unexpected error: {:?}", e),
             Ok(t) => Err(BasicParseError::UnexpectedToken(t)),
         };
         self.reset(start_position);

I see:

failures:

---- tests::expect_no_error_token stdout ----
	thread 'tests::expect_no_error_token' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim('}'))', src/parser.rs:236
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- tests::declaration_list stdout ----
	thread 'tests::declaration_list' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(';'))', src/parser.rs:236

---- tests::color3 stdout ----
	thread 'tests::color3' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::one_component_value stdout ----
	thread 'tests::one_component_value' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::component_value_list stdout ----
	thread 'tests::component_value_list' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::one_rule stdout ----
	thread 'tests::one_rule' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(';'))', src/parser.rs:236

---- tests::outer_block_end_consumed stdout ----
	thread 'tests::outer_block_end_consumed' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::rule_list stdout ----
	thread 'tests::rule_list' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(';'))', src/parser.rs:236

---- tests::stylesheet stdout ----
	thread 'tests::stylesheet' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(';'))', src/parser.rs:236

---- tests::serializer_preserving_comments stdout ----
	thread 'tests::serializer_preserving_comments' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::test_expect_url stdout ----
	thread 'tests::test_expect_url' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::serializer_not_preserving_comments stdout ----
	thread 'tests::serializer_not_preserving_comments' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236

---- tests::unicode_range stdout ----
	thread 'tests::unicode_range' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(','))', src/parser.rs:236

---- tests::stylesheet_from_bytes stdout ----
	thread 'tests::stylesheet_from_bytes' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(';'))', src/parser.rs:236

---- tests::color3_hsl stdout ----
	thread 'tests::color3_hsl' panicked at 'internal error: entered unreachable code: unexpected error: UnexpectedToken(Delim(')'))', src/parser.rs:236


failures:
    tests::color3
    tests::color3_hsl
    tests::component_value_list
    tests::declaration_list
    tests::expect_no_error_token
    tests::one_component_value
    tests::one_rule
    tests::outer_block_end_consumed
    tests::rule_list
    tests::serializer_not_preserving_comments
    tests::serializer_preserving_comments
    tests::stylesheet
    tests::stylesheet_from_bytes
    tests::test_expect_url
    tests::unicode_range

test result: FAILED. 14 passed; 15 failed; 0 ignored; 0 measured

Investigating expect_no_error_token, we see:

(lldb) bt
* thread #2: tid = 0x237bb2, 0x00000001001411b4 cssparser-5b049f7aab40ab8f`std::panicking::rust_panic + 4 at panicking.rs:580, name = 'tests::expect_no_error_token', stop reason = breakpoint 1.1
  * frame #0: 0x00000001001411b4 cssparser-5b049f7aab40ab8f`std::panicking::rust_panic + 4 at panicking.rs:580
    frame #1: 0x0000000100141177 cssparser-5b049f7aab40ab8f`std::panicking::rust_panic_with_hook + 439 at panicking.rs:565
    frame #2: 0x0000000100140fb5 cssparser-5b049f7aab40ab8f`std::panicking::begin_panic<collections::string::String> + 101 at panicking.rs:511
    frame #3: 0x0000000100140ed3 cssparser-5b049f7aab40ab8f`std::panicking::begin_panic_fmt + 147 at panicking.rs:495
    frame #4: 0x000000010003574d cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::expect_exhausted(self=0x00000001013ff4d0) + 733 at parser.rs:236
    frame #5: 0x00000001000395ec cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::parse_entirely<closure,(self=0x00000001013ff4d0, parse=closure at 0x00000001013ff1e8),()> + 124 at parser.rs:389
    frame #6: 0x000000010003b6b2 cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::parse_nested_block<closure,(self=0x00000001013ffd30, parse=closure at 0x00000001013ff450),()> + 482 at parser.rs:449
    frame #7: 0x00000001000439b8 cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::expect_no_error_token(self=0x00000001013ffd30) + 152 at parser.rs:725
    frame #8: 0x0000000100043f9f cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::expect_no_error_token::{{closure}}((null)=closure at 0x00000001013ff990, input=0x00000001013ffd30) + 47 at parser.rs:725
    frame #9: 0x00000001000395c5 cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::parse_entirely<closure,(self=0x00000001013ffd30, parse=closure at 0x00000001013ffa48),()> + 85 at parser.rs:388
    frame #10: 0x000000010003b6b2 cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::parse_nested_block<closure,(self=0x0000000101400248, parse=closure at 0x00000001013ffcb0),()> + 482 at parser.rs:449
    frame #11: 0x00000001000439b8 cssparser-5b049f7aab40ab8f`cssparser::parser::{{impl}}::expect_no_error_token(self=0x0000000101400248) + 152 at parser.rs:725
    frame #12: 0x000000010005ff08 cssparser-5b049f7aab40ab8f`cssparser::tests::expect_no_error_token + 56 at tests.rs:239
    frame #13: 0x00000001000fdeaf cssparser-5b049f7aab40ab8f`test::{{impl}}::call_box<(),closure> [inlined] test::run_test::{{closure}} + 15 at lib.rs:1368
    frame #14: 0x00000001000fdead cssparser-5b049f7aab40ab8f`test::{{impl}}::call_box<(),closure> [inlined] core::ops::FnOnce::call_once<closure,(())> at ops.rs:2626
    frame #15: 0x00000001000fdead cssparser-5b049f7aab40ab8f`test::{{impl}}::call_box<(),closure> + 13 at lib.rs:140
    frame #16: 0x000000010014213b cssparser-5b049f7aab40ab8f`panic_unwind::__rust_maybe_catch_panic + 27 at lib.rs:98
    frame #17: 0x00000001000efa37 cssparser-5b049f7aab40ab8f`std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> [inlined] std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> + 69 at panicking.rs:433
    frame #18: 0x00000001000ef9f2 cssparser-5b049f7aab40ab8f`std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> [inlined] std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> at panic.rs:361
    frame #19: 0x00000001000ef9f2 cssparser-5b049f7aab40ab8f`std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> [inlined] test::run_test::run_test_inner::{{closure}} + 201 at lib.rs:1313
    frame #20: 0x00000001000ef929 cssparser-5b049f7aab40ab8f`std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> [inlined] std::panic::{{impl}}::call_once<(),closure> + 123 at panic.rs:296
    frame #21: 0x00000001000ef8ae cssparser-5b049f7aab40ab8f`std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> + 30 at panicking.rs:454
    frame #22: 0x000000010014213b cssparser-5b049f7aab40ab8f`panic_unwind::__rust_maybe_catch_panic + 27 at lib.rs:98
    frame #23: 0x00000001000f82a5 cssparser-5b049f7aab40ab8f`alloc::boxed::{{impl}}::call_box<(),closure> [inlined] std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> + 57 at panicking.rs:433
    frame #24: 0x00000001000f826c cssparser-5b049f7aab40ab8f`alloc::boxed::{{impl}}::call_box<(),closure> [inlined] std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> at panic.rs:361
    frame #25: 0x00000001000f826c cssparser-5b049f7aab40ab8f`alloc::boxed::{{impl}}::call_box<(),closure> [inlined] std::thread::{{impl}}::spawn::{{closure}}<closure,()> + 62 at mod.rs:360
    frame #26: 0x00000001000f822e cssparser-5b049f7aab40ab8f`alloc::boxed::{{impl}}::call_box<(),closure> + 46 at boxed.rs:640
    frame #27: 0x000000010013e195 cssparser-5b049f7aab40ab8f`std::sys::imp::thread::{{impl}}::new::thread_start [inlined] alloc::boxed::{{impl}}::call_once<(),()> + 37 at boxed.rs:650
    frame #28: 0x000000010013e18f cssparser-5b049f7aab40ab8f`std::sys::imp::thread::{{impl}}::new::thread_start [inlined] std::sys_common::thread::start_thread + 15 at thread.rs:21
    frame #29: 0x000000010013e180 cssparser-5b049f7aab40ab8f`std::sys::imp::thread::{{impl}}::new::thread_start + 16 at thread.rs:84
    frame #30: 0x00007fff8fcbe899 libsystem_pthread.dylib`_pthread_body + 138
    frame #31: 0x00007fff8fcbe72a libsystem_pthread.dylib`_pthread_start + 137
    frame #32: 0x00007fff8fcc2fc9 libsystem_pthread.dylib`thread_start + 13

Parser::expect_exhausted calls Parser::next, which calls Parser::next_including_whitespace_and_comments. This method gets the next byte from the tokenizer, then returns Err(BasicParseError::UnexpectedToken(Token::Delim(byte.unwrap_or(0) as char))) (which was Err(()) before the changes in #143).

[emilio]

Yeah, I think this is intended. We use stop_before to say, for example: "Parse until the next comma". So with your changes, you're returning an Err(UnexpectedToken) when we actually find the comma there, which is ok, but we definitely need to ignore in expect_exhausted.

So I think this issue is invalid, and perhaps replacing the FIXME with a comment explaining the above could be worth.