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.

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

StructMatcher procedural macro #447

Open
andriyDev opened this issue Aug 2, 2024 · 9 comments
Open

StructMatcher procedural macro #447

andriyDev opened this issue Aug 2, 2024 · 9 comments

Comments

@andriyDev
Copy link

andriyDev commented Aug 2, 2024

This is something I've dreamed of having in the C++ googletest library. The difference is in Rust this seems possible!

#[googletest::StructMatcher]
struct MyType {
  field_1: i32,
  field_2: String,
  ...
}

This attribute macro would generate something akin to:

#[cfg(test)]
struct MyTypeStructMatcher {
  field_1: Option<Box<dyn Matcher<ActualT = i32>>>,
  field_2: Option<Box<dyn Matcher<ActualT = String>>>,
  ...
}

#[cfg(test)]
impl googletest::matcher::Matcher for MyTypeStructMatcher {
  type ActualT = MyType;

  fn matches(&self, actual: &Self::ActualT) -> MatcherResult {
    if let Some(field_1) = &self.field_1 {
      if field_1.matches(&actual.field_1).is_no_match() {
        return MatcherResult::NoMatch;
      }
    }
    ...
  }
}

(A supporting enum in googletest code to make it easier to construct the field matcher)

pub enum StructFieldMatcher<ActualT> {
  None,
  Matcher(Box<dyn Matcher<ActualT = ActualT>>)
}

impl<M: Matcher + 'static> From<M> for StructFieldMatcher<<M as Matcher>::ActualT> {
  fn from(value: M) -> Self {
    Self::Matcher(Box::new(value))
  }
}

This enables usage similar to the following:

#[googletest::test]
fn my_test() {
  let value = MyType {
      field_1: 3,
      field_2: "helloworld".into(),
      ...
    };
  expect_that!(value, MyTypeStructMatcher{
      field_1: lt(3).into(),
      field_2: contains_substring("llow").into(),
    });
}

Disadvantages

We need to make googletest (or some related crate) a public dependency of crates. Thankfully the #[cfg(test)] attribute should prune the code gen for the entire implementation, but we do still pay the cost of the procedural macro.

@bjacotg
Copy link
Collaborator

bjacotg commented Aug 2, 2024

There is some support for this with matches_pattern! macro. But I like this approach as well, since it is more obvious what code is generated. However, currently, matches_pattern! is more powerful, as it handles methods as well.

To avoid the disadvantages, we could use cfg_attr, which should avoid adding the macro to the public dependencies.

Nice trick with .into(). I was pondering an option with generic field, but with other trade-offs. Adding them here for completeness.

Declaration

#[cfg(test)]
struct MyTypeStructMatcher<M1, M2, ...> {
  field_1: M1,
  field_2: M2,
  ...
}

impl<M1: Matcher<i32>, M2: Matcher<String>, ...> Matcher<MyType> for MyTypeStructMatcher<M1, M2, ...> {
  ...
}

But it is not possible to implement a Default implementation, hence all the field must be set.

Wait for rust-lang/rust#86555

rust-lang/rust#86555 will allow to implement:

impl MyTypeStructMatcher<Anything, Anything, ...> {
  fn is_anything() -> Self {
    ...
  }
 }

which can be passed as default:

MyTypeStructMatcher{
 field_1: eq(31),
  ..MyTypeStructMatcher::is_anything()
}

Use a builder pattern

We can easily implement a generic builder to set fields like:

impl MyTypestructMatcher<Anything, Anything, ...> {
  fn new() -> Self {...}
}

// Maybe lock M1 to Anything, to disallow overwrite
impl<M1, M2, ...> MyTypeStructMatcher<M1, M2, ...> {
  fn field_1<N: Matcher<i32>>(field_1_matcher: N) -> MyTypeStructMatcher<N, M2, ...> {
    ...
  }
}

but this is less elegant than using a constructor directly.

@andriyDev
Copy link
Author

I completely missed the matches_pattern tbh, so thank you! That does solve 90% of my pain. One thing it's missing is that it's not exhaustive. Part of the reason I want something like this is to ensure we catch every field. I've been bitten before by my tests checking some but not all fields.

The "declaration" idea was my first thought (not fully formed of course). Its main advantage being omitting .into() entirely. Perhaps this is the way to go? If you only want to specify some fields, users can just use matches_pattern. This struct's main advantage is that it's exhaustive.

I definitely don't like the builder pattern, especially for nesting these. I can only imagine what horrors rustfmt would come up with. It is an important alternative though.

@marcianx
Copy link
Contributor

marcianx commented Nov 2, 2024

In the matcher-generator approach, there's a further nuance of nested structs and figuring out how to do the right thing there if the nested struct has a corresponding matcher struct generated vs not.

So given that matches_pattern seems to meet most of the needs, is the only thing left to be solved the ability to ensure that all fields have been checked? Or is there additional functionality being sought here?

(Looking to possibly take this on.)

@andriyDev
Copy link
Author

@marcianx, nested structs should be handled like any other field. So for a struct like:

#[googletest::StructMatcher]
struct MyType {
  field_1: MyNestedStruct,
  field_2: String,
}

struct MyNestedStruct {
  a: i32,
  b: u32,
}

It should generate something like:

#[cfg(test)]
struct MyTypeStructMatcher {
  field_1: Option<Box<dyn Matcher<ActualT = MyNestedStruct>>>,
  field_2: Option<Box<dyn Matcher<ActualT = String>>>,
}

Whether a matcher exists that you can put there is up to the caller. For example, they can use a matches_pattern there, or any or whatever else.


As for matches_pattern replacing this, if there's a way to make it exhaustive, that would be great, but I don't think there is? matches_pattern doesn't get the definition of the struct, so it has no idea how many fields exist. Not to mention, matches_pattern also supports properties, which makes it a little confusing in terms of being exhaustive. Like can you still also test properties if the matcher is meant to be exhaustive? Unclear what the right behavior is there.

@marcianx
Copy link
Contributor

marcianx commented Nov 2, 2024

Ah, you're right about nested structs -- your representation is quite generic.

It would be a pity to make a second strongly overlapping way to do the same thing that matches_pattern! already does just to additionally support the specific use-case of exhaustiveness. But it might be possible to use a complementary approach that avoids duplication of functionality, like defining a derive macro that implements a custom trait, say:

trait ReflectFields {
  fn field_names() -> Vec<String>;
}

possibly even defined in a separate crate of we want for any reason to avoid making all of googletest-rust a dependency. They perhaps we could incorporate this into matches_pattern! and have matches_pattern! fail if not all fields are checked unless the pattern passed into it explicitly has a ..} suffix.

  • Tracking which fields are checked would require some careful thinking.
  • We'd also have to use specialization hacks (like inherent-method/auto-ref specialization) to support values that don't implement ReflectFields.

I've not tested the feasibility of this approach yet, though it would likely require converting matches_pattern! to a proc_macro first.

The disadvantage of this approach is that lack of exhaustiveness would be caught at runtime instead of compile time.

WDYT?

@marcianx
Copy link
Contributor

marcianx commented Nov 8, 2024

Per chat with @bjacotg:

  • matches_pattern! doesn't work well with autocomplete and you can't use it if fields are not public.
  • matches_pattern! doesn't use valid rust syntax, so that rustfmt doesn't work on it.
  • Would much prefer a #[derive(Matcher)] solution that creates a dumb struct implementing matcher for ease of auto-complete.

I'll investigate a solution similar to the original post. Please LMK @bjacotg if you'd like me to attempt a different direction.

@marcianx
Copy link
Contributor

marcianx commented Nov 13, 2024

I was experimenting and chatting with @bjacotg, and this is where we landed so far

Tool behavior

To understand the constraints imposed by rust-analyzer autocomplete and rustfmt, I ran a few tests with both vim + coc.vim and with vscode with the rust-analyzer plugin.

  • rustfmt requires valid rust syntax. The presence of Foo { a: ref 1 } within a macro prevents rustfmt from formatting the macro.
  • rust-analyzer auto-complete happens to work even when within a macro that matches against ($($tt:tt)*) (which is as untyped as a macro gets). It suggests autocompletions from ambient symbols, but when it matches some struct-like syntax, it will offer precise types if the struct name is defined, even in the presence of invalid syntax like the use of ref in Foo { a: ref 1 }:
    Image
    I get this full-signature autocomplete only after the first struct field and the following comma have been entered.

So maybe using a macro-based solution, even with nested macros, wouldn't hurt auto-complete too badly as noted above and maybe we could reduce the scope to just focus on making field matching exhaustive in the absence of an explicit ..} suffix in the pattern.

Field exhaustiveness

It occurred to me that we don't really need the full-powered StructMatcher in order to support field exhaustiveness. We could just generate a zero-sized struct with all the same fields as Struct. We construct the dummy struct constructor syntax as we parse fields. Then we embed the construction of the dummy struct in a noop way as long as the matches_pattern! matcher doesn't end in ..}. The disadvantage is that we still require an annotation on the original Struct.

@bjacotg then had the brilliant idea of modifying the strategy to generate a match or matches! instead! And that doesn't even require an annotation on the original struct! So to elaborate:

Given:

verify_that!(
    actual,
    matches_pattern!(
      Foo {
        a: eq(1),
        b: eq("foo"),
        method(): starts_with!("3"),
      }
    ),
)

matches_pattern! would generate:

{
  fn __matches_pattern_ensure_exhastive_match(val: Foo) {
    let _ = match val {
      Foo { a: _, b: _ } => "ensure exhaustive matches_pattern!",
    };
  }
  // ... actual matching logic ...
}

which would result in a compilation error when a new field (say, c) is added to Foo:

error[E0027]: pattern does not mention field `c`
 --> src/main.rs:9:7
  |
9 |       Foo { a: _, b: _ } => "ensure exhaustive matches_pattern!",
  |       ^^^^^^^^^^^^^^^^^^ missing field `c`
  |
help: include the missing field in the pattern
  |
9 |       Foo { a: _, b: _, c } => "ensure exhaustive matches_pattern!",
  |                       ~~~~~
help: if you don't care about this missing field, you can explicitly ignore it
  |
9 |       Foo { a: _, b: _, .. } => "ensure exhaustive matches_pattern!",
  |

The compilation error isn't the most direct, but hopefuly the inline string makes it obvious.

@andriyDev
Copy link
Author

Woah, the field exhaustiveness idea is so clever!!! The cost of a bad error message is sad, but for the simplicity of not needing a struct attribute, it might be worth it!

I wonder if the error message would get better or worse by forcing newlines into the generated code (if that's even allowed in declarative macros). I would imagine seeing the struct definition on multiple lines might make it more readable.

Thanks for proving me wrong about making the macro exhaustive hahaha.

copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
…implify subsequent improvements to its functionality (specifically #447).

PiperOrigin-RevId: 699526998
copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
…implify subsequent improvements to its functionality (specifically #447).

PiperOrigin-RevId: 699526998
copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
…implify subsequent improvements to its functionality (specifically #447).

PiperOrigin-RevId: 699526998
copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
…implify subsequent improvements to its functionality (specifically #447).

PiperOrigin-RevId: 700322995
@marcianx
Copy link
Contributor

marcianx commented Nov 27, 2024

Ugh, one of the issues I ran into after implementing (thanks to extensive tests!): if Foo above is an enum variant instead of a struct, then fn __matches_pattern_ensure_exhastive_match(val: Foo) doesn't compile since Foo isn't a type. So I need to use Foo strictly in a match pattern context and infer the type. To do this, I have to match against a val with inferred type. But then I also have to somehow declare and initialize this value without the inferred type being available. PhantomData comes to mind but I don't know how to get a value out of it to match against. Inspired by dtolnay's "Unit struct with type parameters" trick, I can do this with a 0-sized array:

quote! {
    fn __matches_pattern_ensure_exhastive_match(i: usize) {
        let val: [_; 0] = [];
        let _ = match val[i] {
            #struct_name { #(#field_names: _),* } => "ensure exhaustive matches_pattern!",
        };
    }
}

Seems to work for enums!

copybara-service bot pushed a commit that referenced this issue Nov 28, 2024
…tern` enforce exhaustive field checks for braced structs by default, to be suppressed by explicit `..` at the end of the pattern.

This change is not backward-compatible since:
* It enforces field exhaustiveness by default in the absence of a trailing `..` in the pattern.
* Things that previously fell back to `match` pattern matching now use `match_pattern!`'s specialized matching, which requires the matched object to implement `Debug`.

Toward #447

PiperOrigin-RevId: 700843379
copybara-service bot pushed a commit that referenced this issue Nov 28, 2024
This change is not backward-compatible since:
* Some things that previously fell back to `match` pattern matching now use `match_pattern!`'s specialized matching, which requires the matched object to implement `Debug`.

Toward #447

PiperOrigin-RevId: 700889703
copybara-service bot pushed a commit that referenced this issue Dec 2, 2024
This change is not backward-compatible since:
* Some things that previously fell back to `match` pattern matching now use `match_pattern!`'s specialized matching, which requires the matched object to implement `Debug`.

Toward #447

PiperOrigin-RevId: 700889703
copybara-service bot pushed a commit that referenced this issue Dec 2, 2024
This change is not backward-compatible since:
* Some things that previously fell back to `match` pattern matching now use `match_pattern!`'s specialized matching, which requires the matched object to implement `Debug`.

Toward #447

PiperOrigin-RevId: 701969181
copybara-service bot pushed a commit that referenced this issue Dec 2, 2024
…tern` enforce exhaustive field checks for braced structs by default, to be suppressed by explicit `..` at the end of the pattern.

This change is not backward-compatible since:
* It enforces field exhaustiveness by default in the absence of a trailing `..` in the pattern.
* Things that previously fell back to `match` pattern matching now use `match_pattern!`'s specialized matching, which requires the matched object to implement `Debug`.

Toward #447

PiperOrigin-RevId: 700843379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants