Opt-in automatic collection of expected values on `alt` by urso · Pull Request #415 · winnow-rs/winnow

urso · 2024-01-03T21:01:01Z

Continuation of #413 and #410

This PR introduces the MergeContext trait, so we can call merge_context during or.

epage · 2024-01-03T21:10:14Z

src/error.rs

Please clean up the commit history

This commit shouldn't have this change

Most other content from this commit should likely be squashed into a previous commit

Replace the AddContext::... commits with the MergeContext trait commit which will hopefully remove the roundtripping of ParseError getting a C parameter and then dropping it

Adding of tests is unrelated to the merge context commit and should either be in the LongestMatch commit or its own commit

Sure, will clean up the commits before undrafting the PR.

epage · 2024-01-03T21:10:50Z

src/error/tests.rs

Why are we adding a hand-written tag rather than just using the fact that &'static str implements Parser?

When using tag or &;static str then winnow uses a specialized compare to check if the inputs prefix matches that string.. That is on backtracking the checkpoints all point to the same location. Alternatively I maybe could have used ('a', 'b', 'c') as test input.

I see. Yeah, I'd prefer something else like a tuple or array so we don't have to deal with the correctness of custom parser implementations.

src/error/tests.rs

coveralls · 2024-01-03T21:14:36Z

Pull Request Test Coverage Report for Build 7402348403

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.8%) to 44.137%

Totals
Change from base Build 7400692687:	0.8%
Covered Lines:	1306
Relevant Lines:	2959

💛 - Coveralls

src/error.rs

src/error/tests.rs

urso · 2024-01-05T23:35:56Z

Made some changes like introducing the MergeContext trait and added a few more tests.

Also update the commit history.

epage · 2024-01-06T02:28:11Z

src/stream/mod.rs

+/// Used to compare checkpoints
+pub trait AsOrd {
+    /// The type used to compare checkpoint positions
+    type Ord: Ord + Clone + core::cmp::Ord + crate::lib::std::fmt::Debug;


Isn't Ord + core::cmp::Ord redundant?

Oops, some oversight when combining the commits into one.

epage · 2024-01-06T02:29:09Z

src/stream/mod.rs

+impl<T: PartialEq> PartialEq for Checkpoint<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.0.eq(&other.0)
+    }
+}
+
+impl<T: Eq> Eq for Checkpoint<T> {}


Do we still need these?

These are currently used by the tests. We can update the tests to compare the err.into_inner() or compare via format!(...).

epage · 2024-01-06T02:31:20Z

src/error.rs

+            let (mut context, other) = if self.context.capacity() >= other.context.capacity() {
+                (self.context, other.context)
+            } else {
+                (other.context, self.context)
+            };
+            context.extend(other);


I feel like this could lead to confusing ordering.

Should we instead check if one is empty and instead pick the other?

I didn't really care about order if there are multiple parsers with the same longest match, but I see how this might be confusing to developers or mess eventually mess with tests when they change parsers..

Should we instead check if one is empty and instead pick the other?

👍

epage · 2024-01-06T02:33:02Z

src/stream/mod.rs

+/// Used to compare checkpoints
+pub trait AsOrd {
+    /// The type used to compare checkpoint positions
+    type Ord: Ord + Clone + core::cmp::Ord + crate::lib::std::fmt::Debug;


For Clone and Debug, if this is because LongestMatch impls those traits, wouldn't that be dependent on whether the Ord does so, meaning we don't need these bounds?

We can remove them. Clone was required because I initially stored the ord in LongestMatch. I updated LongestMatch to store the actual checkpoint now.

epage · 2024-01-06T02:33:22Z

src/error/mod.rs

+impl<I, E> LongestMatch<I, E>
+where
+    I: Stream,
+    <I as Stream>::Checkpoint: AsOrd,
+    E: MergeContext + Default,
+{
+    /// Create an empty error
+    pub fn new() -> Self {
+        Self {
+            checkpoint: None,
+            inner: Default::default(),
+        }
+    }
+}


Why do we offer an empty error?

And is this the only reason checkpoint is an Option?

The reason for checkpoint to be Option is the LongestMatch currently implements the MergeContext trait as well. When calling clear_context we must set checkpoint: None, such that the empty context is always < then any non-empty context. Otherwise merging 2 context would give us some inconsistent output.

I'm removing impl MergeContext for LongestMatch, then we should get rid of the Option for checkpoint.

epage · 2024-01-06T02:34:31Z

src/error/mod.rs

+// For tests
+impl<I, E> core::cmp::PartialEq for LongestMatch<I, E>
+where
+    I: Stream,
+    <I as Stream>::Checkpoint: AsOrd + core::cmp::PartialEq,
+    E: MergeContext + core::cmp::PartialEq,
+{
+    fn eq(&self, other: &Self) -> bool {
+        self.checkpoint == other.checkpoint && self.inner == other.inner
+    }
+}


Do we need this? ContextError needs it because its the default and so we need this for testing parsers. Here, we likely shouldn't be doing equality check tests

These are used by the tests.

As LongestMatch is no error on itself itself, but a decorator most likely used with ContextError I did feel like being able to compare LongestMatch<ContextError> would be the right thing to have.

src/error/mod.rs

epage · 2024-01-06T02:40:47Z

src/error/mod.rs

+    fn append(mut self, input: &I, kind: ErrorKind) -> Self {
+        let checkpoint = input.checkpoint();
+        match self.cmp_with_checkpoint(&checkpoint) {


When is this needed?

epage · 2024-01-06T02:42:09Z

src/error/mod.rs

+
+    fn clear_context(self) -> Self {
+        Self {
+            checkpoint: None,


Why do we need to reset it when clearing?

See my comment here #415 (comment)

I'm removing impl MergeContext or LongestMatch. This also removes the need or checkpoint to be Option.

epage · 2024-01-06T02:43:43Z

src/error/tests.rs

+        let checkpoint1 = &&input[2..];
+        let checkpoint2 = &&input[3..];


If we keep these tests, we should name these checkpoints to make reading the asserts clearer

epage · 2024-01-06T02:45:39Z

src/error/tests.rs

@@ -0,0 +1,190 @@
+use super::*;
+
+mod longest_match {


My primary concern with these tests is they focus on all of the details but they don't make sure the details are actually correct for which we'd need tests that render error messages.

By "rendered error message" you mean using format!(...) ?

I didn't do this because LongestMatch does not directly implement any rendering itself. It will just delegates rendering to the inner error. That is the test output would depend on the current implementation of ContextError.

As LongestMatch acts as a 'guard' on the wrapped error when merging/adding context details I wanted to make sure with the tests that this 'guard' works as expected.

I just noticed that ErrMode uses Debug internally to render the error.

Is this what you have had in mind?

#[test] fn multi_longest_match() { let mut parser = alt(( pattern(('d', 'e', 'f'), "don't want"), pattern(('a', 'b', 'c', 'd'), "wanted 1"), pattern(('a', 'b', 'c'), "wanted 2"), pattern(('d', 'e', 'f', 'g'), "don't want"), )); let mut input = "abd"; let checkpoint = &&input[2..]; // 2 characters consumed by longest match assert_eq!( format!("{}", parser.parse_next(&mut input).unwrap_err(),), r#"Parsing Error: LongestMatch { checkpoint: Checkpoint("d"), inner: ContextError { context: ["wanted 1", "wanted 2"], cause: None } }"#, ); }

I didn't do this because LongestMatch does not directly implement any rendering itself. It will just delegates rendering to the inner error. That is the test output would depend on the current implementation of ContextError.

For me, without a Display test (not Debug or the current state of context), we aren't showing that we've been able to achieve what this is trying to accomplish.

axelkar · 2024-02-25T17:36:25Z

Any updates on this?

epage reviewed Jan 3, 2024

View reviewed changes

src/error/tests.rs Outdated Show resolved Hide resolved

epage reviewed Jan 3, 2024

View reviewed changes

src/error/tests.rs Outdated Show resolved Hide resolved

epage reviewed Jan 3, 2024

View reviewed changes

src/error/tests.rs Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jan 3, 2024

View reviewed changes

src/error/tests.rs Fixed Show fixed Hide fixed

urso mentioned this pull request Jan 3, 2024

Location aware error context #409

Open

2 tasks

epage reviewed Jan 4, 2024

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jan 4, 2024

View reviewed changes

src/error/tests.rs Fixed Show fixed Hide fixed

urso force-pushed the longest branch from 0a627f7 to 55b8390 Compare January 5, 2024 10:33

urso and others added 2 commits January 5, 2024 22:57

feat(stream): introduce AsOrd

b5d867a

feat(stream): Allow comparing Checkpoints

549ed3b

urso force-pushed the longest branch from 55b8390 to 3572dc4 Compare January 5, 2024 22:15

epage and others added 2 commits January 6, 2024 00:23

feat(error): Add MergeContext trait

93d91e3

feat(error): Add LongestMatch error decorator

1f272cf

urso force-pushed the longest branch from 3572dc4 to 1f272cf Compare January 5, 2024 23:27

urso requested a review from epage January 5, 2024 23:36

urso marked this pull request as ready for review January 5, 2024 23:36

epage changed the title ~~Longest~~ Opt-in automatic collection of expected values on alt Jan 6, 2024

epage reviewed Jan 6, 2024

View reviewed changes

src/error/mod.rs Show resolved Hide resolved

epage reviewed Jan 6, 2024

View reviewed changes

epage mentioned this pull request Jan 12, 2024

Context for built-in combinators #421

Closed

2 tasks

		let checkpoint1 = &&input[2..];
		let checkpoint2 = &&input[3..];

Conversation

urso commented Jan 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jan 3, 2024

Pull Request Test Coverage Report for Build 7402348403

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

urso commented Jan 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urso Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axelkar commented Feb 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

urso Jan 6, 2024 •

edited

Loading