Add matcher combinators #118

9999years · 2023-10-02T20:23:15Z

These matcher combinators will let us express more complex logging concepts, like:

Match these two events in any order.
Match either of these two events.
Match either of these two events, as long as this other event doesn't match first.

There's also a system of checkpoints where log events are read into the default checkpoint. At any time, you can create another checkpoint with GhciWatch::checkpoint. Then, log events will be read into the new checkpoint. Later, you can assert that messages were logged in a particular checkpoint or range of checkpoints.

9999years · 2023-10-02T20:33:11Z

Current dependencies on/for this PR:

main
- PR Add matcher combinators #118 👈
  - PR Graceful shutdown #97
    - PR Support globs for reload/restart actions #138

This comment was auto-generated by Graphite.

test-harness/src/checkpoint.rs

Gabriella439 · 2023-10-04T23:45:42Z

test-harness/src/checkpoint.rs

+/// Checkpoints can be constructed with [`crate::GhcidNg::first_checkpoint`],
+/// [`crate::GhcidNg::current_checkpoint`], and [`crate::GhcidNg::checkpoint`].
+#[derive(Debug, Clone, Copy)]
+pub struct Checkpoint(pub(crate) usize);


I personally feel like this Checkpoint "newtype" doesn't buy you much over just using usize directly and the Checkpoint type adds a lot of complexity here. Most of the CheckpointIndex impls don't really do anything other than coerce Checkpoint to usize

It's a bounds-checked index, so it's guaranteed to be safe to use. If we used a usize the user could pass in garbage and cause the test to panic. Probably not that bad, but we do get a safety guarantee out of this.

Oh, actually, the thing it does give us is a SliceIndex implementation with slice output for single checkpoints. This lets us treat single checkpoints like 1 and ranges like 1..3 or 2.. the same. The standard SliceIndex implementations instead return a T for a usize and a [T] for ranges of usize.

So we'd need to keep the CheckpointIndex trait around, even if it was just implemented for usize and ranges of usize.

evanrelf

Left some thoughts, but nothing sticks out as incorrect or anything, so not blocking a merge.

evanrelf · 2023-10-16T18:18:17Z

test-harness/src/ghciwatch.rs

+        // Otherwise, wait for a log message.
        match tokio::time::timeout(timeout_duration, async {
            loop {
-                match self.tracing_reader.next_event().await {
-                    Err(err) => {
-                        return Err(err);
-                    }
-                    Ok(event) => {
-                        if matcher.matches(&event) {
-                            return Ok(event);
-                        } else if let Some(negative_matcher) = &negative_matcher {
-                            if negative_matcher.matches(&event) {
-                                return Err(miette!("Found a log event matching {negative_matcher}"));
-                            }
-                        }
-                    }
+                let event = self.read_event().await?;
+                if matcher.matches(event)? {
+                    return Ok(event.clone());


(struggled to select the exact region I want in GitHub, sorry)

This block reads really strangely to me. I don't think the Ok(Ok(event)) or Ok(Err(err)) match arms are reachable. You'll either return early from the whole function, or you'll get Err(_) from the timeout. Otherwise you're just looping infinitely.

I would either:

Change the current match the_block to if the_block.is_err()

break from the loop with the matching event value instead of returning from the function

I don't think your current code is incorrect, it's just weird lol.

Oh, this is a horrible quirk of Rust's async/await compiling down into state machines: an async block like the one used here is like a separate function, so the return on line 337 only returns from the async block passed to tokio::time::timeout.

The Ok(Ok(event)) branch is reached from the async block return on line 337.

The Ok(Err(_)) branch is reached from the try (?) expressions on lines 335-336.

The Err(_) branch is reached if the timeout expires.

Ah, that makes sense! I knew about the async/await state machine stuff, but I didn't realize return was "scoped" to an async block. TIL.

It's kind of evil because async is the only block scoped this way. There's been a proposal for try {} blocks that behave similarly since at least 2016 but there's some issues with type inference that have kept it from getting merged.

test-harness/src/ghciwatch.rs

evanrelf · 2023-10-16T18:30:57Z

test-harness/src/matcher/and_matcher.rs

+        assert!(!matcher.matches(&event).unwrap());
+        assert!(matcher
+            .matches(&Event {
+                message: "doggy".to_owned(),
+                ..event
+            })
+            .unwrap());


Feels strange to me that matches is mutating the matcher. I feel like consuming events and mutating state should be separate from checking whether the matcher is satisfied yet.

Something like this (dumb names aside):

matcher.consume(&event).unwrap(); assert!(!matcher.is_satisfied()); matcher.consume(&Event { message: "doggy".to_owned(), ..event }).unwrap(); assert!(matcher.is_satisfied());

Where consume takes a &mut self and is_satisfied takes a &self.

Hm. My thought process is that the only thing that can update whether the matcher is satisfied is feeding it an event. Splitting it up like this would also mean storing some extra state, like FusedMatcher does.

This is a parallel to the Iterator::next method:

Returns None when iteration is finished. Individual iterator implementations may choose to resume iteration, and so calling next() again may or may not eventually start returning Some(Item) again at some point.

To compensate for this, there's a FusedIterator trait:

An iterator that always continues to yield None when exhausted.

Calling next on a fused iterator that has returned None once is guaranteed to return None again. This trait should be implemented by all iterators that behave this way because it allows optimizing Iterator::fuse().

I guess I'm thinking of matchers like parsers in Haskell, where given some input, I get back a parse result and maybe some leftovers or whatever.

More of a pure function from some input to a result. And then the idea of incremental input consumption would just be an optimization or ergonomic convenience, where the thing remembers things you've given it before because it has internal mutable state, so you don't need to give it everything at once.

But I'm not that familiar with parsers in imperative languages, and I'm struggling to see the connection to Rust's iterators here (mayyybe it feels kinda like a peekable iterator?), so I might just be totally off base here 🤷

I'm struggling to see the connection to Rust's iterators here (mayyybe it feels kinda like a peekable iterator?)

You're correct, Peekable is similar, except instead of storing if the iterator has ever returned None, it stores the iterator's next item.

For some Matchers (like the BaseMatcher), the matching is stateless and pure; if we wanted to have separate consume and is_satisfied methods, BaseMatcher would need to store an is_satisfied: boolean field to remember if it has already matched. FusedMatcher behaves like this, storing an extra matched: boolean field and returning Ok(true) from Matcher::matches unconditionally if matched is true.

Similarly, Iterator's next() method mutates the iterator's state to (possibly) provide a next element. Because Iterator doesn't have an is_finished method, returning None from next() once doesn't mean it will never return Some again (e.g. for an Iterator reading lines from a file, while some other process writes data to the file). So std provides a FusedIterator that stores an extra bit of data: has the underlying iterator returned None yet? If it has, Iterator::next always returns None, and otherwise it calls the underlying method.

It's the same principle, adding an extra bit of state to make another guarantee on the API.

The underlying reason for Peekable is the same, too -- sometimes it's easy to check if there's a next element in advance, sometimes it's not, but it's always easy to take an unpeekable iterator, call next on it, and memoize the return value.

Ah okay, I see how it's similar to Iterator now, thanks for explaining.

evanrelf · 2023-10-16T18:33:10Z

test-harness/src/matcher/option_matcher.rs

+/// A matcher which may or may not contain a matcher.
+///
+/// If it does not contain a matcher, it never matches.
+pub struct OptionMatcher<M>(Option<M>);


Could you instead impl<M: Matcher> Matcher for Option<M> or something like that?

Not without writing my own Display replacement/wrapper trait at least:

error[E0277]: `std::option::Option<M>` doesn't implement `std::fmt::Display` --> test-harness/src/matcher/option_matcher.rs:13:30 | 13 | impl<M: Matcher> Matcher for Option<M> {} | ^^^^^^^^^ `std::option::Option<M>` cannot be formatted with the default formatter | = help: the trait `std::fmt::Display` is not implemented for `std::option::Option<M>` = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead note: required by a bound in `Matcher` --> test-harness/src/matcher/mod.rs:30:20 | 30 | pub trait Matcher: Display { | ^^^^^^^ required by this bound in `Matcher`

9999years requested a review from Gabriella439 October 2, 2023 20:23

9999years mentioned this pull request Oct 2, 2023

Graceful shutdown #97

Merged

9999years force-pushed the rebeccat/matcher branch 2 times, most recently from a645084 to 5a79976 Compare October 3, 2023 20:17

9999years requested a review from evanrelf October 4, 2023 17:57

Gabriella439 reviewed Oct 4, 2023

View reviewed changes

9999years requested a review from Gabriella439 October 5, 2023 00:39

9999years force-pushed the rebeccat/matcher branch 4 times, most recently from 749d13b to 9767a25 Compare October 6, 2023 17:30

9999years added 4 commits October 9, 2023 13:06

Add matcher combinators

6c76929

BaseMatcher helpers

e1cdeb6

Checkpoint support

60e8185

Checkpoint::into_index -> Checkpoint::into_inner

6bada10

9999years force-pushed the rebeccat/matcher branch from 9767a25 to 6bada10 Compare October 9, 2023 20:06

9999years mentioned this pull request Oct 9, 2023

Support globs for reload/restart actions #138

Merged

9999years added 2 commits October 9, 2023 13:13

Fix Clippy lints

3f4eb4d

Documentation lints and helpers

cc78620

9999years force-pushed the rebeccat/matcher branch from d41e2e9 to cc78620 Compare October 9, 2023 20:20

evanrelf previously approved these changes Oct 16, 2023

View reviewed changes

Use Ok(()) instead of the toilet closure

27a0095

9999years dismissed evanrelf’s stale review via 27a0095 October 16, 2023 19:08

evanrelf approved these changes Oct 16, 2023

View reviewed changes

9999years merged commit 8cc6a70 into main Oct 16, 2023
28 checks passed

9999years deleted the rebeccat/matcher branch October 16, 2023 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add matcher combinators #118

Add matcher combinators #118

9999years commented Oct 2, 2023 •

edited

Loading

9999years commented Oct 2, 2023 •

edited

Loading

Gabriella439 Oct 4, 2023

9999years Oct 5, 2023

9999years Oct 5, 2023

evanrelf left a comment

evanrelf Oct 16, 2023

9999years Oct 16, 2023

evanrelf Oct 16, 2023

9999years Oct 16, 2023

evanrelf Oct 16, 2023

9999years Oct 16, 2023

evanrelf Oct 16, 2023

9999years Oct 16, 2023

9999years Oct 16, 2023

evanrelf Oct 16, 2023

evanrelf Oct 16, 2023

9999years Oct 16, 2023

Add matcher combinators #118

Add matcher combinators #118

Conversation

9999years commented Oct 2, 2023 • edited Loading

9999years commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrelf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9999years commented Oct 2, 2023 •

edited

Loading

9999years commented Oct 2, 2023 •

edited

Loading