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 Stdin::lines, Stdin::split forwarder methods #86412

Closed
wants to merge 1 commit into from

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jun 17, 2021

Add forwarder methods Stdin::lines and Stdin::split, which consume
and lock a Stdin handle, and forward on to the corresponding BufRead
methods. This should make it easier for beginners to use those iterator
constructors without explicitly dealing with locks or lifetimes.

Fixes #85383

@rustbot label +A-io +C-enhancement +D-newcomer-roadblock +T-libs

@rustbot rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 17, 2021
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 17, 2021
Add forwarder methods `Stdin::lines` and `Stdin::split`, which consume
and lock a `Stdin` handle, and forward on to the corresponding `BufRead`
methods. This should make it easier for beginners to use those iterator
constructors without explicitly dealing with locks or lifetimes.
@tlyu
Copy link
Contributor Author

tlyu commented Jun 19, 2021

@rustbot label -T-libs +T-libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 19, 2021
@Lokathor
Copy link
Contributor

I'm pretty good with Rust and I had quite some trouble the other day trying to get stdin working with simple "line at a time" input, so this sounds great.

@joshtriplett
Copy link
Member

I love the idea of making this easier.

I wonder, though, if we should just provide a method like into_locked() (or perhaps just locked()) publicly instead, for Stdin/Stdout/Stderr. That would solve the single biggest annoyance when working with stdin: the need to write something like:

let stdin = std::io::stdin();
let stdin = stdin.lock();

Writing those two lines as one line generates lifetime errors, because .lock() takes &self. Adding a method that consumes self would eliminate that issue, and allow writing things like for line in std;:io::stdin().into_locked().lines() { ... }.

I don't strongly object to the idea of adding lines and split directly on stdin; I might weakly object, perhaps, but those methods might be worth adding.

But I personally think the priority should be to fix this common cause of lifetime errors that prevents people from writing various one-liners, and in the process, make it easier to call any method that would be available on the locked versions.

@tlyu
Copy link
Contributor Author

tlyu commented Jun 30, 2021

I love the idea of making this easier.

Thanks for the feedback!

I wonder, though, if we should just provide a method like into_locked() (or perhaps just locked()) publicly instead, for Stdin/Stdout/Stderr.

I think locked is a little too close to the existing lock method and might cause confusion. We do have precedent for the into_ prefix for methods that consume the receiver.

That would solve the single biggest annoyance when working with stdin: the need to write something like:

let stdin = std::io::stdin();
let stdin = stdin.lock();

Writing those two lines as one line generates lifetime errors, because .lock() takes &self. Adding a method that consumes self would eliminate that issue, and allow writing things like for line in std;:io::stdin().into_locked().lines() { ... }.

I do think having something like Stdin::into_lock or Stdin::locked that consumes Stdin would be helpful. I didn't propose making it a part of the public API because I heard quite a few objections when I proposed making Stdin::lock return StdinLock<'static> or removing the lifetime parameter from StdinLock. So, I was trying to be conservative along the axis of making publicly visible changes to the locking API.

On the other hand, there is some discussion here https://internals.rust-lang.org/t/add-owned-guard-types-to-standard-library/14912 about owned guard types, so maybe there is some interest in doing similarly for the stdio handles? The naming for those methods also seems reasonable: lock_owned, etc.

We could even go a step further and make free functions like io::stdin_locked() available, possibly reducing the complexity of simple operations even further. (Though I understand how people might be more hesitant to add more free functions.)

I don't strongly object to the idea of adding lines and split directly on stdin; I might weakly object, perhaps, but those methods might be worth adding.

I think there's substantial benefit for adding those forwarders. We already have Stdin::read_line as precedent for an implicitly locking forwarder method, which probably exists for a similar reason: to make it easier for beginners to do common line-wise terminal input operations without needing to know about the nuances of locks and lifetimes sooner than is appropriate for them.

I think this is quite important: adding interactivity can feel like a substantial achievement beyond "hello world", and lowering the barriers to achieving this can make learning Rust a little friendlier.

Adding Stdin::into_lock alone is an incremental improvement, but still exposes the locking API to beginners who might simply want an iterator over input lines. The only reason I've needed to use the locking API on Stdin is that I wanted access to the lines method, which requires StdinLock, for reasons that aren't readily apparent until you know some details about the implementation.

But I personally think the priority should be to fix this common cause of lifetime errors that prevents people from writing various one-liners, and in the process, make it easier to call any method that would be available on the locked versions.

I'm happy to work on a pull request that consists only of adding into_lock, lock_owned, etc to Stdin, and maybe to the other stdio handles, if you think that will be more widely accepted than the forwarder methods. I still think that the forwarder methods have substantial benefits of their own.

I guess I'd like to ask: what benefits there are to having a public into_lock method besides gaining access to the BufRead methods lines and split? I would guess that explicit locking control over Stdin isn't necessary for a lot of use cases once we make those methods available, but I'd be interested to hear about more use cases.

@joshtriplett
Copy link
Member

what benefits there are to having a public into_lock method besides gaining access to the BufRead methods lines and split?

Locking for performance. If you're going to consume or produce a large amount of data in a loop, you can get a substantial performance improvement by locking once rather than once per iteration.

We could even go a step further and make free functions like io::stdin_locked() available, possibly reducing the complexity of simple operations even further.

Those helpers seem entirely reasonable to me as well. I would be happy to see them added. And those helpers also provide more brevity, making it more reasonable to use the verbose name into_locked as the method name for clarity.

I'm happy to work on a pull request that consists only of adding into_lock, lock_owned, etc to Stdin, and maybe to the other stdio handles, if you think that will be more widely accepted than the forwarder methods.

I would love to see a PR that adds an into_locked method to all three types, and corresponding stdin_locked/stdout_locked/stderr_locked free functions. (I'd also love to see that PR adjust the documentation of the existing lock methods and the existing unlocked free functions to suggest using these new methods and functions.)

That doesn't preclude adding helpers for the methods of BufRead as well. I just feel like this is a good incremental step to make Rust more learnable, and to give users a gentler introduction to one concept at a time. (Users would still have to understand that they need to lock stdin, but they wouldn't also simultaneously have to deal with a lifetime issue.)

@tlyu
Copy link
Contributor Author

tlyu commented Jun 30, 2021

Thanks! I'll work on that, then.

I know this is unlikely to be accepted because it's a breaking change, but what if the default stdio handles io::stdin(), etc were locked, and you had to use a different function like io::stdin_unlocked() to get unlocked ones? I wonder how different things would look. Multi-threading isn't exactly a beginner concept, so if the default stdio handles can only be used in a single thread, it's not a huge loss to beginners. The impact on the rest of the ecosystem might be too much, though.

@Lokathor
Copy link
Contributor

Much as that might be a good idea, breaking changes to the standard library are just not going to happen at this time.

@joshtriplett
Copy link
Member

I don't think we could change io::stdin() to be locked by default. We might, in theory, be able to change lock() to return StdinLock<'static> (changing the lifetime), without breaking anything, but that'd still be more disruptive for minimal benefit.

@tlyu tlyu marked this pull request as draft July 2, 2021 02:05
@tlyu
Copy link
Contributor Author

tlyu commented Jul 2, 2021

Deferring until #86799 gets some review.

@kennytm kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2021
add owned locked stdio handles

Add stderr_locked, stdin_locked, and stdout_locked free functions
to obtain owned locked stdio handles in a single step. Also add
into_lock methods to consume a stdio handle and return an owned
lock. These methods will make it easier to use locked stdio
handles without having to deal with lifetime problems or keeping
bindings to the unlocked handles around.

Fixes rust-lang#85383; enables rust-lang#86412.

r? `@joshtriplett`
`@rustbot` label +A-io +C-enhancement +D-newcomer-roadblock +T-libs-api
@bors
Copy link
Contributor

bors commented Jul 3, 2021

☔ The latest upstream changes (presumably #86799) made this pull request unmergeable. Please resolve the merge conflicts.

@tlyu
Copy link
Contributor Author

tlyu commented Jul 3, 2021

I'll resolve the merge conflicts (and try to rename the branch, because that name is no longer quite accurate).

@rustbot label -S-blocked +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 3, 2021
@tlyu tlyu closed this Jul 3, 2021
@tlyu tlyu deleted the stdin-into-lock branch July 3, 2021 15:21
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 21, 2021
add `Stdin::lines`, `Stdin::split` forwarder methods

Add forwarder methods `Stdin::lines` and `Stdin::split`, which consume
and lock a `Stdin` handle, and forward on to the corresponding `BufRead`
methods. This should make it easier for beginners to use those iterator
constructors without explicitly dealing with locks or lifetimes.

Replaces rust-lang#86412.
~~Based on rust-lang#86846 to get the tracking issue number for the `stdio_locked` feature.~~ Rebased after merge, so it's only one commit now.

r? `@joshtriplett`
`@rustbot` label +A-io +C-enhancement +D-newcomer-roadblock +T-libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E0716: confusing reference to borrow of Stdin value when there's no obvious reference within StdinLock
7 participants