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

Feat: Allow generics in method return types and arguments. #62

Merged
merged 4 commits into from
Dec 1, 2024

Conversation

o0Ignition0o
Copy link
Contributor

Partial fix fo #18

This changeset allows generics to method return types and arguments.
Generic impl traits are still not supported in return position (yet?).

Partial fix fo nrxus#18

This changeset allows generics to method return types and arguments.
Generic impl traits are still not supported in return position (yet?).
Sometimes Generics expose some static function relevant for a given type. This doesn't mean they arent't used.
This changeset allows to mock those as well.

Note that calling such mocks in faux::when! requires the receiver args syntax.
@o0Ignition0o
Copy link
Contributor Author

I've just added support for Generics that aren't necesarily used in the function signature (because they might expose static methods for example).

Happy to update the docs, or split it into an other PR if you feel this contribution would prove useful! :)

@nrxus
Copy link
Owner

nrxus commented Nov 20, 2024

Hey thanks for the PR, I haven't dove into the code yet but the tests look great.

Two things though:

  • Can you write a test to make sure that if a single mock instance has the same method with generics mocked multiple times but with different structs then the correct mock is called? I am fairly certain this already happens essentially for free but it'd be good to check.

  • Also my main hesitation, and one of the reasons my branch that was doing something similar to this became stale is that this still makes it impossible to mock methods with unnameable structs (e.g., closures). I don't think there is a solution at all though so I may just get over that limitation and merge this anyway. The impl Trait syntax for arguments still works for those use cases so maybe that's good enough

@o0Ignition0o
Copy link
Contributor Author

Hi, thanks a lot for your reply!

I'll try to work on both points you mentionned this week :)

…ric type name to the error message in case a stub is missing
@o0Ignition0o o0Ignition0o force-pushed the feat/generic_method_returns branch from 502bef6 to f0eead8 Compare November 23, 2024 22:47
@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Nov 23, 2024

Hi!

I've just adressed the first part, and added the generic name in the error messages when a stub is missing.
Unfortunately I don't think we could tell the user about the concrete type the function has been called with, but we might be able to provide hints via #[track_caller] (at least in the non async case).

Let me know if it's worth a try (potentially in an other PR, I'd like this one to not grow too much 😅)

this still makes it impossible to mock methods with unnameable structs (e.g., closures)

Indeed, I'm not sure if/how to deal with that, but I'll send a message your way if I figure something out sometimes.

@o0Ignition0o
Copy link
Contributor Author

I have a poorly written yet somewhat working implementation of the #[track_caller] idea I mentionned before, master...o0Ignition0o:faux:feat/track_caller_location_on_error.

Callsite information seems to be propagated for both aync and async calls, but there are probably cornercases I haven't seen.

The main issue at the moment is that I rely on + use<'_> syntax that is fairly recent, but we can always use a dummy trait if MSRV is important.

I stumbled upon this idea while trying to chase the second point you mentioned, but I keep hitting walls such as Sized bounds, opaque types being different etc.

Happy to dig further if you think it might be interesting!

@nrxus
Copy link
Owner

nrxus commented Nov 25, 2024

I am currently on a trip and I have misplaced my laptop so I won't get a chance to take a deep look on this again until this coming weekend but from what I can see through my phone it looks great and I will probably merge it as is. So thanks a lot!

Regarding the track call site, it's a pretty interesting idea and I am not opposed to bumping the MSRV for it (as long as it's all documented). From what I remember about the use syntax, isn't the desugaring of async functions edition dependant?

@o0Ignition0o
Copy link
Contributor Author

It seems like the "Capture rules" evolved in edition 2021 indeed (TIL!), so we would need to be mindful of that.

I'll open a separate issue to the repo so we can discuss it separately if that works for you.

https://rust-lang.github.io/rfcs/3617-precise-capturing.html#lifetime-capture-rules-2024

@nrxus
Copy link
Owner

nrxus commented Dec 1, 2024

opening a separate issue/PR for that makes sense.

I finally had time to go through this PR more in depth and everything looks great thank you!

I will probably take a bit to do a release with this because there is some lints/warnings (unrelated to this PR) that I need to sort through on the latest rust version but I will try to one without too much delay

@nrxus nrxus merged commit eb25d4b into nrxus:master Dec 1, 2024
@nrxus
Copy link
Owner

nrxus commented Dec 1, 2024

Hm it actually fails tests when run in release mode (I didn't notice until after I merged). Any chance you can take a look at why? I am guessing there is some optimization happening that makes the two structs that are used as the generic param collapse into one?

@nrxus
Copy link
Owner

nrxus commented Dec 2, 2024

I figured out the issue and fixed it so I did a release with this.

There is still the not-related new set of warnings that the macro expansions are creating regarding elided lifetimes that I gotta figure out but that's existing so I'll try to make another patch for that at a later time (unless you really like looking at macro code and feel like addressing that :p)

Thank you for your PR!

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Dec 2, 2024

Oh gosh sorry I'm on gh notification bankrupcy and didn't notice that one, glad you figured it out!

I might have a bit of time available to see if there's any followups I can hop on

@o0Ignition0o o0Ignition0o deleted the feat/generic_method_returns branch December 2, 2024 16:04
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.

2 participants