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

Responder lifetime cannot be infered #97

Merged
merged 2 commits into from
Jul 9, 2021
Merged

Conversation

mrene
Copy link
Contributor

@mrene mrene commented Jul 7, 2021

This stackoverflow question shows that it's not sufficient to have elided lifetimes on Responder because these aren't correctly infered when the wrapped responder is less restrictive.

The root cause of this is the PhantomData holding 'r and 'o inside Responder. These aren't required because the bounds on the impl blocks are sufficient to provide the correct semantics, and R will already hold these bounds implicitly.

This is gonna be a breaking change, but it should make it easier to use in the long term. That is unless you can think of any cases that this doesn't handle?

@lawliet89 lawliet89 requested a review from ELD July 8, 2021 01:22
Copy link
Collaborator

@ELD ELD left a comment

Choose a reason for hiding this comment

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

I don't know what's going on with Clippy on the nightly compiler, but that looks like a serious false positive. The stable compiler tests pass, though, so I'm fine with that seemingly erroneous failure.

Thanks for fixing the lints. I imagine the workflow didn't run until it was approved since you're a first-time contributor to the repo.

The explanation on the SO post and the PR makes sense to me and the tests all seem to pass. Looks good!

If you wouldn't mind squashing the changes into a single commit, that would be my preference before merging. Thanks for the contribution! Just remembered I could squash and merge myself. Thanks, again!

@ELD ELD merged commit ad80993 into lawliet89:master Jul 9, 2021
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