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.

Sign up for GitHub

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

Jump to bottom

Switch from pin-project to pin-project-lite #2388

Closed
jplatte opened this issue Jan 8, 2021 · 14 comments · Fixed by #2393
Closed

Switch from pin-project to pin-project-lite #2388

jplatte opened this issue Jan 8, 2021 · 14 comments · Fixed by #2393
Labels
A-dependencies Area: library dependencies. C-refactor Category: refactor. This would improve the clarity of internal code. E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@jplatte
Copy link
Contributor

jplatte commented Jan 8, 2021 •
edited
Loading

futures-rs just switched and tokio has been using the -lite one since 0.2.0. I haven't used either so I don't actually know how hard switching is, but it seems like a good idea since it greatly improves compile times for people who don't depend on syn & quote otherwise.
@seanmonstar
Copy link
Member

Considered, and agreed! It should be done!

@seanmonstar seanmonstar added A-dependencies Area: library dependencies. E-easy Effort: easy. A task that would be a great starting point for a new contributor. C-refactor Category: refactor. This would improve the clarity of internal code. labels Jan 8, 2021
@seanmonstar seanmonstar changed the title Consider switching from pin-project to pin-project-lite Switch from pin-project to pin-project-lite Jan 8, 2021
@taiki-e
Copy link
Contributor

taiki-e commented Jan 8, 2021 •
edited
Loading

This may require taiki-e/pin-project-lite#43.

UPDATE: merged and released in pin-project-lite 0.2.4.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 11, 2021

I'll take a stab at implementing this.

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Jan 11, 2021 •
edited
Loading

Depends on taiki-e/pin-project-lite#25 because of

hyper/src/client/dispatch.rs

Line 180 in fad42ac

#[pin_project::pinned_drop]

@jplatte
Copy link
Contributor Author

jplatte commented Jan 11, 2021

Just found that out myself. Thanks for the link 🙂

@jplatte
Copy link
Contributor Author

jplatte commented Jan 11, 2021

Also depends on taiki-e/pin-project-lite#3 because of

hyper/src/server/conn.rs

Lines 163 to 165 in c9c46ed

#[cfg(feature = "http1")]
#[cfg(feature = "http2")]
fallback: Fallback<E>,

@jplatte jplatte mentioned this issue Jan 11, 2021
Merged
@taiki-e
Copy link
Contributor

taiki-e commented Jan 12, 2021

Also depends on taiki-e/pin-project-lite#3 because of

hyper/src/server/conn.rs

Lines 163 to 165 in c9c46ed

#[cfg(feature = "http1")]
#[cfg(feature = "http2")]
fallback: Fallback<E>,

pin-project-lite#3 is about attributes that apply only to the original struct/enum.
Proper handling of cfg/cfg_attr on fields and variants requires a more complex approach than the fix for pin-project-lite#3 (it needs to be aware of the kind of attribute and propagate some of it to projected type). Even pin-project could not find the correct solution other than "leave to the compiler". And it is impossible to use the same trick in pin-project-lite.

However, there is a known workaround for the cfg issue, and probably hyper can use it as well.

for now, it is preferable to create a type alias / new type or create structs for each cfg.

Something like:

#[cfg(all(feature = "http1", feature = "http2"))]
#[derive(Clone, Debug)]
enum Fallback<E> { ... }

#[cfg(not(all(feature = "http1", feature = "http2")))]
#[derive(Clone, Debug)]
struct Fallback<E>(std::marker::PhantomData<E>);

pin_project! {
    pub struct Connection<T, S, E = Exec>
    where
        S: HttpService<Body>,
    {
        pub(super) conn: Option<ProtoServer<T, S::ResBody, S, E>>,
        fallback: Fallback<E>,
    }
}

@jplatte
Copy link
Contributor Author

jplatte commented Jan 12, 2021 •
edited
Loading

pin-project-lite#3 is about attributes that apply only to the original struct/enum.

The title and issue description are exactly about this.

Anyway, thanks for providing a workaround, I'll apply that after taiki-e/pin-project-lite#25 is merged and I can finish my PR.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 12, 2021

Hmm, by the way that struct is not using #[pin] at all, should it even be using pin_project!?

@taiki-e taiki-e mentioned this issue Jan 12, 2021
Open
@taiki-e
Copy link
Contributor

taiki-e commented Jan 12, 2021

The title and issue description are exactly about this.

Well, I agree that it is confusing.

Hmm, by the way that struct is not using #[pin] at all, should it even be using pin_project!?

Technically, pin_project! is not required for types that do not use #[pin].

@taiki-e
Copy link
Contributor

taiki-e commented Jan 13, 2021 •
edited
Loading

after taiki-e/pin-project-lite#25 is merged

I think I can merge that this weekend or the next week. (it only needs docs and tests)

EDIT: It's no longer a blocker, so I postponed the merge.

@paolobarbolini paolobarbolini mentioned this issue Jan 17, 2021
Merged
@seanmonstar
Copy link
Member

I had to revert in #2422, an issue was found after releasing: #2421

@seanmonstar seanmonstar reopened this Feb 5, 2021
@taiki-e taiki-e mentioned this issue Feb 6, 2021
Merged
@jplatte
Copy link
Contributor Author

jplatte commented Mar 22, 2021

I had a look, this is the problematic type: an enum that uses #[cfg] on its variants. The two solutions I can think of:

  • always have the variant but containing an uninhabited type if the corresponding feature is disabled. Probably requires adding extra Self::Foo { bar } => match bar {}, branches
  • three declarations of the type for the three possible feature combinations (assuming that it just doesn't need to exist if both http1 and http2 are disabled, otherwise four)

@jplatte
Copy link
Contributor Author

jplatte commented Jun 2, 2021

Working on this again, using the first solution I mentioned above (if still applicable).

BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this issue Jul 26, 2021
@jplatte
refactor(lib): Switch from pin-project to pin-project-lite (hyperium#…
f71dafd 
…2566)

Note the practical affects of this change:

- Dependency count with --features full dropped from 65 to 55.
- Time to compile after a clean dropped from 48s to 35s (on a pretty underpowered VM).

Closes hyperium#2388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-dependencies Area: library dependencies. C-refactor Category: refactor. This would improve the clarity of internal code. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Switch to pin-project-lite jplatte/hyper
4 participants
@seanmonstar @jplatte @paolobarbolini @taiki-e