Making the Stream adaptor structures public in tokio-stream #6658

sharpened-nacho · 2024-06-25T10:07:46Z

Motivation

The StreamExt trait provides the helpful method take, map, and the like on Streams. But the structures themselves (Take, Map, etc.) are not visible outside the crate, as they are not pub use in stream_ext.rs.

Solution

Re-exported publicly all the structures in stream_ext.rs except for Collect, which is different and should not be exported as stated in its documentation. Re-exported them publicly again in lib.rs.

Added a function in a test with return type Chain to show that it can be part of an external function signature.

Closes: #6656

Darksonn · 2024-06-25T10:15:21Z

tokio-stream/src/lib.rs

+pub use stream_ext::{
+    AllFuture, AnyFuture, Chain, Filter, FilterMap, FoldFuture, Fuse, Map, MapWhile, Merge, Next,
+    Peekable, Skip, SkipWhile, Take, TakeWhile, Then, TryNext,
+};


Please put them in a sub-module so they don't clutter the front page of the crate documentation.

I could define a pub mod adapters in lib.rs that pub uses all the structures like so

/// Some doc pub mod adapters { pub use crate::stream_ext::{ AllFuture, AnyFuture, Chain, Filter, FilterMap, FoldFuture, Fuse, Map, MapWhile, Merge, Next, Peekable, Skip, SkipWhile, Take, TakeWhile, Then, TryNext, }; }

This seemed very hacky to me, so I thought : should all the mods for those structures really be in the stream_ext folder/mod? The exposed structure of the crate is very different from its folder structure. How about having adapters.rs next to stream_ext.rs, and an adapters folder with all the mods' files? That would mimic the std::iter structure.
That way we could also put the "sources" (std::iter terminology) related mods (empty, iter, once, etc.) in their folder - but expose them the same as they are right now to not break anything.

I would have 2 questions with this though :

is that considered a breaking change (the interface is kept the same)? And even if not, do you want to do this sort of big changes?

what to do with the all-alone collect mod? Keeping it in stream_ext? Lift it next to stream_ext and adapters?

But perhaps I'm going too far, I have no idea 😅

Defining an inline module like you suggested in the beginning is fine. (But the other approaches would not be considered breaking.)

sharpened-nacho · 2024-06-25T12:07:19Z

My bad again, sorry for pushing bad commits like those, I'm not familiar enough with large projects to understand how to run all tests.
I hope you can squash all that mess together...

sharpened-nacho · 2024-06-25T13:21:06Z

I just realized the equivalence between tokio_stream::StreamExt and futures_util::StreamExt. But in futures-util the structures are exported and accessible, so the functionality I needed is ultimately not needed here, I can get it from futures-util.
Do you want to roll back to the beginning and drop this PR?

Darksonn

Please don't worry about pushing bad commits. We always squash together PRs on merge, so it's really not a problem.

tokio-stream/src/lib.rs

sharpened-nacho · 2024-06-30T14:27:42Z

tokio-stream/src/lib.rs

+        Next, Peekable, Skip, SkipWhile, Take, TakeWhile, Then, TryNext,
+    };
+}
+
 cfg_time! {
    pub use stream_ext::timeout::{Elapsed, Timeout};


And I just saw that PR #4601 exposed Timeout directly in tokio_stream. What to do about this ?

Expose them all in adapters, but also keep this one exposed flush in tokio_stream to not break anything?

You can make the top-level ones #[doc(hidden)].

Done, and added the error struct Elapsed in the new imports, so that's it's still somewhere to be found if a user who used it wants to update.

Looks like the tests don't like the addition of #[doc(hidden)]...

@obi1kenobi is there any way to tell cargo-semver-checks to treat a #[doc(hidden)] item as part of the public API? Context is that we want to change the path of an exported item.

Actually, it seems that we can't deplicate re-export yet :(

rust-lang/rust#30827

Marking the re-export as deprecated doesn't have any effect in rustc / clippy / generated rustdoc HTML (it is simply ignored) but does have the desirable effect in cargo-semver-checks.

Unless I've missed something, to me that still sounds fine to use. After all, it didn't seem like originally (before I suggested it) you were planning to deprecate the re-export, so you weren't going to get any rustc / clippy / HTML docs notices on it anyway.

I don't think anything bad happens here when rustc / clippy / generated docs start fully supporting deprecations on re-exports.

What does it do for a re-export to be considered deprecated as you say?

Deprecations and doc(hidden)-ness are properties of import paths like tokio_stream::stream_ext::timeout::Timeout, not the items behind those names. The full details are in this post, but as a summary:

#[doc(hidden)] pub mod example { pub struct Something; } pub use example::Something;

In this example, this_crate::Something is public API because in writing that path, we didn't touch any hidden items along the way. this_crate::example::Something is hidden and non-public API because the this_crate::example prefix is hidden. Conceptually, you can imagine walking the components ["this_crate", "example", "Something"] and checking whether each of them is hidden or deprecated — if so, the entire path is hidden / deprecated as well.

Broadly speaking, the rustc / clippy / rustdoc HTML implementations only consider deprecations as properties of items like pub struct Something, not of import paths. (There are some subtleties — check out the linked post for more details.) cargo-semver-checks used to do the same thing — it's much simpler and it isn't a priori obvious that deprecation is a path property. This led to tons of false-positives so I spent a couple of months to fix it, as the post describes.

Thank you for the in-depth message. Now I have the full picture, and - as you say on your blog - the semantics are perfectly aligned with what we want.

Awesome! As always, please feel free to ping me if you have further questions or run into any issues 👍

Thank you @obi1kenobi. I agree that using #[deprecated] is reasonable in this case.

Darksonn · 2024-07-02T17:16:23Z

tokio-stream/src/lib.rs

+    pub use crate::stream_ext::{
+        AllFuture, AnyFuture, Chain, Filter, FilterMap, FoldFuture, Fuse, Map, MapWhile, Merge,
+        Next, Peekable, Skip, SkipWhile, Take, TakeWhile, Then, TryNext,
+    };


Actually, can we drop Next from the list? I'd like to only export the ones that implement Stream, not the ones that implement Future.

It looks like there are a few more futures. Can you remove those too? You can view the generated docs with your change here.

Also, we should probably move Elapsed to the root. It's an error type.

The preview is very cool!
Done!

I removed Elapsed from the doc(hidden) and deprecated combo as well, since it is the only way to access it.

Added pub to all use declarations in stream_ext.rs, except for Collect as it should stay private, as stated in its doc. Exported the structures in lib.rs directly at the root of the library. Fixes: #6656

Added an indirection function that has tokio_stream::Chain in its interface to see that the Chain type can be imported and used. Ref: #6656

Reformatted the previous commit. Ref: #6656

Created an inline adapters mod that contains the adapter types, to keep the tokio_stream interface organized. Fixes: #6656

Removed redundant target in label. Fixes: #6656

Added an export for the three last stream structures in the adapters mod. Timeout was already exposed in tokio_stream directly due to #4601 ; left it there. Fixes: #6656

Excluded the Timeout and its error struct exposed flush in tokio_stream. Added that exposed struct in the new adapters mod, so that it's still somewhere. Ref: #6656

My previous phrasing was catastrophic. Ref: #6656

Deprecated the import on top of having it doc hidden. Ref: #6656

Removed Next as it's a Future, not a Stream, from the interface. Ref: #6656

Removed the various structures implementing Future and not Stream. Ref: #6656

Moved the Elapsed error to the root, un-deprecating it. Ref: #6656

Darksonn

Thanks.

devanoneth · 2024-07-03T20:26:16Z

Any idea when a new version of tokio-stream will be rolled with this in it? Thanks!

Darksonn · 2024-07-14T17:52:47Z

I don't have concrete plans for a release right now.

djc · 2024-09-05T09:15:10Z

Releasing in #6825.

Darksonn added the A-tokio-stream Area: The tokio-stream crate label Jun 25, 2024

Darksonn reviewed Jun 25, 2024

View reviewed changes

Darksonn reviewed Jun 30, 2024

View reviewed changes

tokio-stream/src/lib.rs Outdated Show resolved Hide resolved

sharpened-nacho commented Jun 30, 2024

View reviewed changes

Darksonn reviewed Jul 2, 2024

View reviewed changes

sharpened-nacho added 12 commits July 2, 2024 20:37

stream: make stream adaptor structures visible outside the crate

0dfeeef

Added pub to all use declarations in stream_ext.rs, except for Collect as it should stay private, as stated in its doc. Exported the structures in lib.rs directly at the root of the library. Fixes: #6656

stream: reference a newly public structure in a test

58da64c

Added an indirection function that has tokio_stream::Chain in its interface to see that the Chain type can be imported and used. Ref: #6656

stream: formatting

d24f360

Reformatted the previous commit. Ref: #6656

stream: move the adapter types to an inline mod

261ce7c

Created an inline adapters mod that contains the adapter types, to keep the tokio_stream interface organized. Fixes: #6656

stream: fix documentation element

d827273

Removed redundant target in label. Fixes: #6656

stream: expose the streams gated by the time feature

1ac5d33

Added an export for the three last stream structures in the adapters mod. Timeout was already exposed in tokio_stream directly due to #4601 ; left it there. Fixes: #6656

stream: handle the situation with the previously exposed symbols

ff3dc84

Excluded the Timeout and its error struct exposed flush in tokio_stream. Added that exposed struct in the new adapters mod, so that it's still somewhere. Ref: #6656

stream: rewrite the adapters mod documentation

ad7aaa6

My previous phrasing was catastrophic. Ref: #6656

stream: add deprecation to hiding the doc

0ca4cc4

Deprecated the import on top of having it doc hidden. Ref: #6656

stream: removed Next from the interface

434c310

Removed Next as it's a Future, not a Stream, from the interface. Ref: #6656

stream: removed futures implementations

61bc618

Removed the various structures implementing Future and not Stream. Ref: #6656

stream: move Elapsed to the root

d82b6b2

Moved the Elapsed error to the root, un-deprecating it. Ref: #6656

Darksonn approved these changes Jul 2, 2024

View reviewed changes

Darksonn enabled auto-merge (squash) July 2, 2024 20:47

Darksonn merged commit c8f3539 into tokio-rs:master Jul 2, 2024
77 checks passed

Darksonn mentioned this pull request Sep 5, 2024

stream: prepare 0.1.16 release #6825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making the Stream adaptor structures public in tokio-stream #6658

Making the Stream adaptor structures public in tokio-stream #6658

sharpened-nacho commented Jun 25, 2024 •

edited by Darksonn

Loading

Darksonn Jun 25, 2024

sharpened-nacho Jun 25, 2024

Darksonn Jun 25, 2024

sharpened-nacho commented Jun 25, 2024

sharpened-nacho commented Jun 25, 2024

Darksonn left a comment

sharpened-nacho Jun 30, 2024

Darksonn Jun 30, 2024

sharpened-nacho Jun 30, 2024

sharpened-nacho Jun 30, 2024

Darksonn Jun 30, 2024

mox692 Jul 1, 2024

obi1kenobi Jul 1, 2024 •

edited

Loading

sharpened-nacho Jul 1, 2024

obi1kenobi Jul 1, 2024

Darksonn Jul 2, 2024

Darksonn Jul 2, 2024

sharpened-nacho Jul 2, 2024

Darksonn Jul 2, 2024

sharpened-nacho Jul 2, 2024

sharpened-nacho Jul 2, 2024

Darksonn left a comment

devanoneth commented Jul 3, 2024

Darksonn commented Jul 14, 2024

djc commented Sep 5, 2024

Making the Stream adaptor structures public in tokio-stream #6658

Making the Stream adaptor structures public in tokio-stream #6658

Conversation

sharpened-nacho commented Jun 25, 2024 • edited by Darksonn Loading

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharpened-nacho commented Jun 25, 2024

sharpened-nacho commented Jun 25, 2024

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

devanoneth commented Jul 3, 2024

Darksonn commented Jul 14, 2024

djc commented Sep 5, 2024

sharpened-nacho commented Jun 25, 2024 •

edited by Darksonn

Loading

obi1kenobi Jul 1, 2024 •

edited

Loading