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

Add support for generics to debug_handler macro #1265

Closed

Conversation

slessans
Copy link

@slessans slessans commented Aug 16, 2022

This pull requests adds support for generics in the debug_handler macro as described here.

Motivation

#1253

Solution

I added a Specializer type that is able to produce all specializations of generic types in a function declaration given a set of specializations for each parameter. This PR basically modified all existing checks (where relevant) to produce a check for each specialized version of the function.

axum-macros/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Awesome! I haven't fully reviewed this, but left a few comments.

axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
@slessans slessans marked this pull request as ready for review September 28, 2022 20:48
@slessans
Copy link
Author

@jplatte I believe this is ready for review. Thanks for all the feedback so far!

@slessans slessans changed the title [wip] Add support for generics to debug_handler macro Add support for generics to debug_handler macro Sep 29, 2022
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long, here's another round of review comments!

axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler/specializer.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler/specializer.rs Outdated Show resolved Hide resolved
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Needs a rebase and I have two more comments, but I think other than that (oh, and the one pending comment from my previous review) we can merge this :)

axum-macros/src/debug_handler.rs Outdated Show resolved Hide resolved
axum-macros/src/debug_handler/specializer.rs Outdated Show resolved Hide resolved
@slessans
Copy link
Author

@jplatte I think I have addressed everything and am pretty happy with how things are. A few small things to address:

  1. I did a big merge yesterday before I saw your comment to rebase -- would you still like me to rebase this or is squashing and merging the current changes fine
  2. I think the name Specializer is probably due for an update to like GenericItemFn or something but I don't have a great name right now off-hand and would like to get this merged before more conflicts, so if you are OK with it I can address this in a follow-up PR once this is merged and I have a better name for it

let specializer_checks = match Specializer::new(with_tys, item_fn.clone(), body_ty, state_ty) {
Ok(specializer) => {
let check_output_impls_into_response =
specializer.generate_check_output_impls_into_response();
Copy link
Author

Choose a reason for hiding this comment

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

@jplatte looks like this check was removed in main, was that intentional?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, try checking the file history through GitHub.

@jplatte
Copy link
Member

jplatte commented Oct 20, 2022

would you still like me to rebase this or is squashing and merging the current changes fine

No need to rebase, merge is fine.

Regarding the name "specializer", I don't find it too bad.

@davidpdrsn
Copy link
Member

Absolutely awesome work here @slessans. I'll try and take a look this weekend!

@slessans
Copy link
Author

@davidpdrsn gentle ping on this, just want to get it merged soon to minimize conflicts since it turned into such a big PR. 🙏

@slessans
Copy link
Author

@davidpdrsn @jplatte would it be helpful if i split this into a series of smaller PRs?

@davidpdrsn
Copy link
Member

Sorry for taking so long to look at this. I should get time Friday next week 😊

@MichaelScofield
Copy link

ping @davidpdrsn , any updates?

@jplatte jplatte requested a review from davidpdrsn March 7, 2023 12:02
@davidpdrsn
Copy link
Member

Okay first of all sorry for taking an eternity to answer. That's entirely my fault.

The reason I've been hesitant to reply is that I'm kinda conflicted about these changes. On the one hand I really appreciate the work you've done, on the other I'm not fully sure this feature is worth the additional maintenance cost.

While I think #[debug_handler] supporting generics is neat, I can't remember a time that I needed that myself. Nor have I heard from more than a few people asking about it on Discord or on GitHub. But it's not like there is zero interest either, we did get some +1s here and on the issue.

We (and by that I mean myself) should probably have more carefully considered this feature when the issue was opened, before you started implementing it.

@davidpdrsn
Copy link
Member

Given what I said above I think I'll close this for now. @slessans sorry about wasting your time 😞

@davidpdrsn davidpdrsn closed this Apr 14, 2023
@vaniusrb
Copy link

vaniusrb commented Jun 3, 2023

In my current project I need generics for debug_handler and I ended up here. IMHO I think this feature would bring great flexibility and help the axum ecosystem grow. I hope this can be reconsidered in the future.

@cdata
Copy link

cdata commented Jan 9, 2024

Going to add my voice of support for this feature. Not having support for generics in debug_handler has been a huge pain in my team's side.

@jplatte
Copy link
Member

jplatte commented Jan 9, 2024

The macro code does not depend on private things in axum's API. I think personally I would prefer axum itself to provide this too, but it would be possible to publish a separate crate that provides an extended version of the debug_handler attribute macro.

@jsantell
Copy link

The macro code does not depend on private things in axum's API. I think personally I would prefer axum itself to provide this too, but it would be possible to publish a separate crate that provides an extended version of the debug_handler attribute macro.

We're trying to update to Axum 0.7+ and in the process, one of the routes is no longer satisfying trait bounds; specifically a stream-up/stream-down route updated to Body from previous StreamBody/BodyStream, where other routes that either stream-up or stream-down are fine, suspecting some !Send across await.

As the routes use generics, something like this PR is very appealing! As the issue only surfaced while upgrading to 0.7, I attempted to use this specialized debug_handler (changing axum/core dependencies to use their 0.7 equivalents), and some changes may be needed in order to work with 0.7:

#[axum_macros::debug_handler(with(T = String))]
pub async fn test_route<T>(string: T) -> Result<(), StatusCode> {
    Ok(())
}
trait takes at most 2 generic arguments but 3 generic arguments were supplied
expected at most 2 generic arguments

mod.rs(79, 11): trait defined here, with at most 2 generic parameters: `S`, `M`
push.rs(50, 1): replace the generic bound with the associated type: `Rejection = `

Thanks all for the work here! Some questions:

  • Any pointers for updating this for Axum 0.7?
  • Would nested generics #[debug_handler(with(T = String; U = Box<T>))] be debuggable with this macro?

@jplatte
Copy link
Member

jplatte commented Jan 26, 2024

Would nested generics #[debug_handler(with(T = String; U = Box<T>))] be debuggable with this macro?

I think it might work if written the other way around (if replacements are done in order): #[debug_handler(with(U = Box<T>, T = String))]. But would there be a problem with expanding things fully, as in #[debug_handler(with(T = String, U = Box<String>))]?

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.

7 participants