Skip to content

Initial support for trait-variant#640

Merged
asomers merged 6 commits intoasomers:masterfrom
lasantosr:trait-variant-support
Apr 29, 2025
Merged

Initial support for trait-variant#640
asomers merged 6 commits intoasomers:masterfrom
lasantosr:trait-variant-support

Conversation

@lasantosr
Copy link
Copy Markdown
Contributor

This PR aims to fix #639

Feel free to update the readme, as English is not my primary language

@lasantosr lasantosr requested a review from asomers as a code owner February 11, 2025 19:02
@lasantosr
Copy link
Copy Markdown
Contributor Author

@asomers I think the nightly is failing because some sort of new lints unrelated to this PR, would you like me to fix those also here?

@asomers
Copy link
Copy Markdown
Owner

asomers commented Feb 23, 2025

@asomers I think the nightly is failing because some sort of new lints unrelated to this PR, would you like me to fix those also here?

It's already fixed on master. You just need to rebase.

@lasantosr
Copy link
Copy Markdown
Contributor Author

Great! It looks like it's ready now

Copy link
Copy Markdown
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests could really benefit from some kind of assertion to check that they actually implement the additional trait. That might be hard to do with Send, but what about something like this?

trait Foo {}
#[automock]
#[trait_variant::make(Foo)]
pub trait Bar {
    ...
}
#[test]
fn some_test_method_name() {
    let mock = MockBar::new();
    let p: Box<dyn Foo> = Box::new(mock);   // Ensures that MockBar implements Foo
    ...
}

Also, with this change, does the #[trait_variant::make(OtherTraitName: Foo)] syntax work?

Comment thread mockall/src/lib.rs
Comment thread mockall/tests/automock_trait_variant.rs Outdated
Comment thread mockall/tests/mock_trait_variant.rs Outdated
@lasantosr
Copy link
Copy Markdown
Contributor Author

lasantosr commented Feb 23, 2025

I think the tests could really benefit from some kind of assertion to check that they actually implement the additional trait. That might be hard to do with Send, but what about something like this?

trait Foo {}
#[automock]
#[trait_variant::make(Foo)]
pub trait Bar {
    ...
}
#[test]
fn some_test_method_name() {
    let mock = MockBar::new();
    let p: Box<dyn Foo> = Box::new(mock);   // Ensures that MockBar implements Foo
    ...
}

Also, with this change, does the #[trait_variant::make(OtherTraitName: Foo)] syntax work?

I'm not that familiar with the codebase to implement all of those changes.

On your first example, automock would not implement Foo trait for MockBar, because it might have any arbitrary number of unrelated methods, but it could be manually implemented:

trait Foo {}
#[automock]
#[trait_variant::make(Foo)]
pub trait Bar {
    ...
}
impl Foo for MockBar {}

The most common use case is to mark your trait (and async method futures) Send, and mocks are already Send so that's covered.

As for the second example, the syntax already works, but we would be mocking the initial trait only (Bar), not the other variant (OtherTraitName). With that syntax you're actually defining two different traits.

The main goal of this PR is to have feature parity with async_trait, other improvements could be added later.

As mentioend on #639 the async_trait annotation can be located before or after the automock annotation, and that means you need to return the actual pinned future or just the future's output on the mocks expectations.

With this PR we can do the same for trait_variant, so that the migration is painless.

@lasantosr lasantosr changed the title Add support for trait-variant Initial support for trait-variant Feb 23, 2025
@lasantosr lasantosr requested a review from asomers February 24, 2025 07:05
@lasantosr
Copy link
Copy Markdown
Contributor Author

@asomers can this PR be merged as is to allow initial basic support for trait_variant?

Other improvements can be added later, when needed

Copy link
Copy Markdown
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to decide what do do about the #[trait_variant::make(IntFactory: Send)] syntax. It might just work. Have you tried it? If so, we should add tests for it. If not, and it won't be easy to do, then you need to at least document that.
Also, there must be a CHANGELOG entry.

Comment thread mockall/src/lib.rs Outdated
Comment thread mockall/tests/automock_trait_variant.rs
Comment thread mockall/tests/mock_trait_variant.rs
@lasantosr
Copy link
Copy Markdown
Contributor Author

We need to decide what do do about the #[trait_variant::make(IntFactory: Send)] syntax. It might just work. Have you tried it?

@asomers It was already working but it mocks the original trait (not the variant)

On this example, #[automock] mocks LocalFoo, generating MockLocalFoo:

#[automock]
#[trait_variant::make(Foo: Send)]
pub trait LocalFoo {
    async fn foo(&self) -> u32;
    async fn bar() -> u32;
}

I've extended automock to allow mocking the variant as well (I've included examples and updated automock documentation)

On this example, #[automock(target = Foo)] mocks Foo, generating MockFoo:

#[automock(target = Foo)]
#[trait_variant::make(Foo: Send)]
pub trait LocalFoo {
    async fn foo(&self) -> u32;
    async fn bar() -> u32;
}

Note: trait-variant automatically generates a blanket impl for any type implementing Foo to also implement LocalFoo, so the resulting MockFoo can be used anywhere expecting both LocalFoo or Foo

All of the other comments have been resolved, except for the static assertion which is not required. If you still want me to include it let me know.

Copy link
Copy Markdown
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good now. But I do want to use static_assertions, like I mentioned.

Comment thread mockall/src/lib.rs Outdated
Comment thread mockall/src/lib.rs
Comment thread mockall/tests/automock_trait_variant.rs
Comment thread mockall_derive/src/lib.rs
self.doc
} else if *i.as_ref().unwrap() == "async_trait" {
self.async_trait
} else if *i.as_ref().unwrap() == "make" {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that the most significant word in this attribute name isn't the last one "make", but the first "trait_variant". That's unlike every other attribute that we process. And we may have to alter our processing in the future, if another crate defines an attribute called "make". But for now we can leave it.

@lasantosr
Copy link
Copy Markdown
Contributor Author

@asomers It should be ready now! Thanks for the review

Copy link
Copy Markdown
Owner

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks much better. I have just one more request: could you please add a CHANGELOG entry?

@lasantosr
Copy link
Copy Markdown
Contributor Author

@asomers There's already a changelog entry, let me know if you would like another sentence.

@asomers
Copy link
Copy Markdown
Owner

asomers commented Apr 29, 2025

@asomers There's already a changelog entry, let me know if you would like another sentence.

Of course you did. But yesterday I was only viewing "changes since your last review" so I didn't see it. My bad.

@asomers asomers merged commit ee24836 into asomers:master Apr 29, 2025
3 checks passed
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

Successfully merging this pull request may close these issues.

Compatibility with trait-variant for automock

2 participants