Fix `buffer_position()` after resuming parsing after `IllFormed` errors #689

Mingun · 2023-11-29T20:00:48Z

Because since #677 we can continue parsing after Error::IllFormed we no longer can change buffer_position() so that it points to useful position of error. The best solution would be to add position to the Error itself, in form of:

struct Error {
  kind: ErrorKind, // former Error enum
  offset: usize,
}

but that would require massive changes, also in the code that I plan to remove soon during rewrite. To keep changes relative small and do not touch that code I decided to add a new error_position() getter which would report position of the error. After rewrite I'll try to implement the best solution.

reader-error.rs tests was updated to check buffer_position() result after resuming parsing after error. Also a couple of new cases was added, which also allowed me to find a bug in one case.

Dead code analysis should be able to remove this implementation from the final binary

…d::unmatched_end_tag2` in reader-errors.rs

…ltaneously I found that that way it is more easy to debug tests

codecov-commenter · 2023-11-29T20:08:13Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c383849) 65.47% compared to head (f295b01) 65.06%.

Files	Patch %	Lines
src/reader/mod.rs	93.75%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   65.47%   65.06%   -0.42%     
==========================================
  Files          38       38              
  Lines       18025    17942      -83     
==========================================
- Hits        11802    11674     -128     
- Misses       6223     6268      +45

Flag	Coverage Δ
unittests	`65.06% <96.07%> (-0.42%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dralley · 2023-11-30T01:43:51Z

Ignore that, I hit the button early by mistake.

dralley · 2023-11-30T01:50:27Z

tests/reader-errors.rs

@@ -14,23 +14,27 @@ macro_rules! ok {
            fn borrowed() {
                let mut reader = Reader::from_str($xml);
                reader.config_mut().enable_all_checks(true);
-                assert_eq!(reader.read_event().unwrap(), $event);
+                assert_eq!(reader.read_event().unwrap(), $event, "Reader");


Why not just split it up into individual tests under a mod reader and mod ns_reader?

Yes, I should do that. Will rewrite

dralley · 2023-11-30T06:42:54Z

I still have one more commit to review

…module That tests would panic after next commit which will fix behavior for new tests that will be added soon, because they calls internal method in a state in which Reader will never be in normal circumstances

…cial meaning in a markup failures (18): syntax::doctype::unclosed04::async_tokio syntax::doctype::unclosed04::borrowed syntax::doctype::unclosed04::buffered syntax::doctype::unclosed07::async_tokio syntax::doctype::unclosed07::borrowed syntax::doctype::unclosed07::buffered syntax::doctype::unclosed10::async_tokio syntax::doctype::unclosed10::borrowed syntax::doctype::unclosed10::buffered syntax::doctype::unclosed13::async_tokio syntax::doctype::unclosed13::borrowed syntax::doctype::unclosed13::buffered syntax::doctype::unclosed16::async_tokio syntax::doctype::unclosed16::borrowed syntax::doctype::unclosed16::buffered syntax::doctype::unclosed19::async_tokio syntax::doctype::unclosed19::borrowed syntax::doctype::unclosed19::buffered

…<!DOCTYPE` sequences All tests fixed

failures (66): ill_formed::double_hyphen_in_comment2::ns_reader::async_tokio ill_formed::double_hyphen_in_comment2::ns_reader::borrowed ill_formed::double_hyphen_in_comment2::ns_reader::buffered ill_formed::double_hyphen_in_comment2::reader::async_tokio ill_formed::double_hyphen_in_comment2::reader::borrowed ill_formed::double_hyphen_in_comment2::reader::buffered ill_formed::double_hyphen_in_comment3::ns_reader::async_tokio ill_formed::double_hyphen_in_comment3::ns_reader::borrowed ill_formed::double_hyphen_in_comment3::ns_reader::buffered ill_formed::double_hyphen_in_comment3::reader::async_tokio ill_formed::double_hyphen_in_comment3::reader::borrowed ill_formed::double_hyphen_in_comment3::reader::buffered ill_formed::double_hyphen_in_comment4::ns_reader::async_tokio ill_formed::double_hyphen_in_comment4::ns_reader::borrowed ill_formed::double_hyphen_in_comment4::ns_reader::buffered ill_formed::double_hyphen_in_comment4::reader::async_tokio ill_formed::double_hyphen_in_comment4::reader::borrowed ill_formed::double_hyphen_in_comment4::reader::buffered ill_formed::mismatched_end_tag2::ns_reader::async_tokio ill_formed::mismatched_end_tag2::ns_reader::borrowed ill_formed::mismatched_end_tag2::ns_reader::buffered ill_formed::mismatched_end_tag2::reader::async_tokio ill_formed::mismatched_end_tag2::reader::borrowed ill_formed::mismatched_end_tag2::reader::buffered ill_formed::mismatched_end_tag3::ns_reader::async_tokio ill_formed::mismatched_end_tag3::ns_reader::borrowed ill_formed::mismatched_end_tag3::ns_reader::buffered ill_formed::mismatched_end_tag3::reader::async_tokio ill_formed::mismatched_end_tag3::reader::borrowed ill_formed::mismatched_end_tag3::reader::buffered ill_formed::mismatched_end_tag4::ns_reader::async_tokio ill_formed::mismatched_end_tag4::ns_reader::borrowed ill_formed::mismatched_end_tag4::ns_reader::buffered ill_formed::mismatched_end_tag4::reader::async_tokio ill_formed::mismatched_end_tag4::reader::borrowed ill_formed::mismatched_end_tag4::reader::buffered ill_formed::missing_doctype_name1::ns_reader::async_tokio ill_formed::missing_doctype_name1::ns_reader::borrowed ill_formed::missing_doctype_name1::ns_reader::buffered ill_formed::missing_doctype_name1::reader::async_tokio ill_formed::missing_doctype_name1::reader::borrowed ill_formed::missing_doctype_name1::reader::buffered ill_formed::missing_doctype_name2::ns_reader::async_tokio ill_formed::missing_doctype_name2::ns_reader::borrowed ill_formed::missing_doctype_name2::ns_reader::buffered ill_formed::missing_doctype_name2::reader::async_tokio ill_formed::missing_doctype_name2::reader::borrowed ill_formed::missing_doctype_name2::reader::buffered ill_formed::unmatched_end_tag1::ns_reader::async_tokio ill_formed::unmatched_end_tag1::ns_reader::borrowed ill_formed::unmatched_end_tag1::ns_reader::buffered ill_formed::unmatched_end_tag1::reader::async_tokio ill_formed::unmatched_end_tag1::reader::borrowed ill_formed::unmatched_end_tag1::reader::buffered ill_formed::unmatched_end_tag2::ns_reader::async_tokio ill_formed::unmatched_end_tag2::ns_reader::borrowed ill_formed::unmatched_end_tag2::ns_reader::buffered ill_formed::unmatched_end_tag2::reader::async_tokio ill_formed::unmatched_end_tag2::reader::borrowed ill_formed::unmatched_end_tag2::reader::buffered ill_formed::unmatched_end_tag3::ns_reader::async_tokio ill_formed::unmatched_end_tag3::ns_reader::borrowed ill_formed::unmatched_end_tag3::ns_reader::buffered ill_formed::unmatched_end_tag3::reader::async_tokio ill_formed::unmatched_end_tag3::reader::borrowed ill_formed::unmatched_end_tag3::reader::buffered

src/reader/mod.rs

dralley · 2023-12-01T06:26:10Z

src/reader/state.rs

@@ -95,7 +100,7 @@ impl ReaderState {
                        off += p + 1;
                        // if next byte after `-` is also `-`, return an error
                        if buf[3 + off] == b'-' {
-                            self.offset -= len - 2 - p;
+                            self.last_error_offset = self.offset - len + 2 + p;


You mentioned that the constants are being cleaned up in the rewrite, yes?

No, with current approach some constants are always will be there because events don't include < and >, but the parser reports offsets that includes those characters. I should just add explanation to all such constants, but in new parser they will be more obvious. In that case, for example, self.offset just after >, buf contains !-- con--tent -- and p is counted from byte after : ~~~~~~~~~~~~~~~~ : - buf : |---p : - p is counted from | (| is 0) : : : ^ - self.offset ^ : : - self.offset - len ^ : - self.offset - len + 2 ^ - self.offset - len + 2 + p

In the perfect world we would add offset to the `Error` type, but right now this is impractical, because this significantly changes some API that would be removed soon Co-authored-by: Daniel Alley <dalley@redhat.com>

Mingun added 4 commits November 27, 2023 23:05

Implement Debug for ReaderState to help debugging

05afd03

Dead code analysis should be able to remove this implementation from the final binary

Fix comments with positions pointers

527e8b8

Remove test that checks the same thing that a more precise `ill_forme…

f95929a

…d::unmatched_end_tag2` in reader-errors.rs

Modify checks so they check both error cause and buffer position simu…

4db87fb

…ltaneously I found that that way it is more easy to debug tests

Mingun added the bug label Nov 29, 2023

Mingun requested a review from dralley November 29, 2023 20:01

dralley approved these changes Nov 30, 2023

View reviewed changes

dralley reviewed Nov 30, 2023

View reviewed changes

Mingun added 5 commits December 1, 2023 00:47

Split the tests for Reader and NsReader by different modules

1f51337

Remove tests that was replaced by tests in reader-errors.rs syntax …

40129e6

…module That tests would panic after next commit which will fix behavior for new tests that will be added soon, because they calls internal method in a state in which Reader will never be in normal circumstances

Fix incorrect error position for unfinished <![CDATA[, <!-- and `…

5d49266

…<!DOCTYPE` sequences All tests fixed

dralley reviewed Dec 1, 2023

View reviewed changes

src/reader/mod.rs Outdated Show resolved Hide resolved

dralley reviewed Dec 1, 2023

View reviewed changes

src/reader/mod.rs Outdated Show resolved Hide resolved

dralley reviewed Dec 1, 2023

View reviewed changes

dralley approved these changes Dec 1, 2023

View reviewed changes

Mingun and others added 2 commits December 1, 2023 21:10

Use separate offset for errors

c9ad25b

In the perfect world we would add offset to the `Error` type, but right now this is impractical, because this significantly changes some API that would be removed soon Co-authored-by: Daniel Alley <dalley@redhat.com>

Add explanation of some constants used to make content of events

f295b01

Mingun force-pushed the pos-in-error branch from 25f029d to f295b01 Compare December 1, 2023 16:49

Mingun merged commit 222532c into tafia:master Dec 1, 2023
6 checks passed

Mingun deleted the pos-in-error branch December 1, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `buffer_position()` after resuming parsing after `IllFormed` errors #689

Fix `buffer_position()` after resuming parsing after `IllFormed` errors #689

Mingun commented Nov 29, 2023

codecov-commenter commented Nov 29, 2023 •

edited

Loading

dralley commented Nov 30, 2023

dralley Nov 30, 2023 •

edited

Loading

Mingun Nov 30, 2023

dralley commented Nov 30, 2023

dralley Dec 1, 2023

Mingun Dec 1, 2023

Fix buffer_position() after resuming parsing after IllFormed errors #689

Fix buffer_position() after resuming parsing after IllFormed errors #689

Conversation

Mingun commented Nov 29, 2023

codecov-commenter commented Nov 29, 2023 • edited Loading

Codecov Report

dralley commented Nov 30, 2023

dralley Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Mingun Nov 30, 2023

Choose a reason for hiding this comment

dralley commented Nov 30, 2023

dralley Dec 1, 2023

Choose a reason for hiding this comment

Mingun Dec 1, 2023

Choose a reason for hiding this comment

Fix `buffer_position()` after resuming parsing after `IllFormed` errors #689

Fix `buffer_position()` after resuming parsing after `IllFormed` errors #689

codecov-commenter commented Nov 29, 2023 •

edited

Loading

dralley Nov 30, 2023 •

edited

Loading