RFC: Add std::io::inputln() by not-my-profile · Pull Request #3196 · rust-lang/r...

Copy link

I think that this might be a better idea. In my RFC I proposed a more advanced approach which ultimately evolved to have formatting, however that is a lot more complex to implement and might be better to leave to crates. I think the standard library would benefit more from a simpler implementation like the one suggested here.

clarfonthey commented 4 days ago

I'm personally all for this method, but I think that the method should be called read_line because it matches the corresponding Stdin method. All of the related FS and IO methods that have standalone equivalents do the same, and the change of name here implies that there's something different happening. This also saves inputln as a name for later if we want a more advanced macro approach.

I'm personally all for this method, but I think that the method should be called read_line because it matches the corresponding Stdin method. All of the related FS and IO methods that have standalone equivalents do the same, and the change of name here implies that there's something different happening. This also saves inputln as a name for later if we want a more advanced macro approach.

Since there is already a read_line function, wouldn't it be confusing or even cause conflicts to name this function the same way? Or are you implying that this version replaces the existing read_line implementation?

not-my-profile commented 4 days ago •

std::io::read_line and std::io::Stdin::read_line can both exist perfectly fine without conflicting. Nobody is suggesting to replace any existing function: Rust guarantees backwards compatibility!

clarfonthey commented 4 days ago

Since there is already a read_line function, wouldn't it be confusing or even cause conflicts to name this function the same way? Or are you implying that this version replaces the existing read_line implementation?

I think a specific example might help: https://doc.rust-lang.org/std/fs/fn.read_to_string.html

One read_to_string is a method on File which accepts a buffer, whereas the other is a standalone function which takes in a filename and allocates a new string.

Note that print!() and println!() are "specialised" variant of write!() and writeln!(), where all of them are based on io::Write but the first two specifically writes to stdout.

So analogously, we should have something that provides a more convenient interface to io::Read first, then to provide a more specific one such as inputln().

Introducing inputln() without something like read() or readln() just feels weird logically.

not-my-profile commented 4 days ago •

Thanks Clar, that is a really good observation! I however see a different consistency axis: for any function named read I expect it to take an argument to specify from where you want to read:

• the std::io::Read::read* methods read from the passed &mut self
• std::io::read_to_string reads from the passed reader
• std::fs::read_to_string reads from the passed file path

So having a std::io::read_line function that does not let you choose where to read from just feels wrong to me.

undersquire commented 4 days ago •

I am not sure this is a big deal, but I tried using your function in a test project and noticed that it doesn't work with parsing to other values. For example:

let age: i32 = io::inputln()?.parse().expect("Invalid age!");

This will always fail here, because when it reads it uses the std::io::stdin().readline function, which also includes the \n (newline) in the resulting string. This isn't a huge deal if you expect the user to manually remove the newline, however I think it might be better to do that for them:

pub fn read_line() -> Result<String, std::io::Error> {
    let mut line = String::new();

    std::io::stdin().read_line(&mut line)?;

    Ok(line.trim_end_matches('\n').to_string())
}

I might be wrong, but I see no reason to include the newline character at the end.

EDIT: Why does std::io::stdin().read_line even include the newline? I feel like that should be changed because you really don't need that newline it reads.

not-my-profile commented 4 days ago

Good observation @undersquire! I however don't think that this is necessary. You can just do:

let age: i32 = io::inputln()?.trim().parse().expect("Invalid age!");

which also has the benefit that it does not fail when the user accidentally enters other whitespace before or after their input.

Why even does std::io::stdin().read_line include the newline? I feel like that should be changed because you really don't need that newline it reads.

Please stop talking about breaking backwards compatability

Diggsey commented 4 days ago

read_line includes the newline because it's intended to be a lower-level building block, and it may be important for the caller to know how the line was terminated.

Given that the entire purpose of inputln would be as a convenience function, then it shouldn't include the newline.

This is the same reason that read_line is passed a buffer to read into, whereas inputln should just allocate and return a stirng.

not-my-profile commented 3 days ago

That's a very good point! I updated the RFC to include the same newline trimming as std::io::BufRead::lines has.

m-ou-se commented 3 days ago

What does this function do when it hits EOF or reads a non-newline-terminated line?

Python's input() throws an exception on a zero read() with no input, but running inputln()? in a loop will run forever when hitting the end of a file that's redirected to stdin:

fn main() -> std::io::Result<()> {
    loop {
        dbg!(inputln()?);
    }
}

$ cargo r < /dev/null
[src/main.rs:16] inputln()? = ""
[src/main.rs:16] inputln()? = ""
[src/main.rs:16] inputln()? = ""
[src/main.rs:16] inputln()? = ""
[...never stops...]

Diggsey commented 3 days ago

I would expect inputln to return an error if used again after EOF was already hit. eg.
So:

<EOF> => Ok(""), Err
foo<EOF> => Ok("foo"), Err
foo\n<EOF> => Ok("foo"), Ok(""), Err

undersquire commented 3 days ago •

What does this function do when it hits EOF or reads a non-newline-terminated line?

The proposed inputln function simply uses std::io::stdin().read_line under the hood, so however std::io::stdin().read_line handles EOF is how inputln will also handle EOF. If it doesn't then maybe it needs to be added to the proposed function.

@not-my-profile Is the function being renamed to std::io::read_line or are you just keeping it as std::io::inputln?

not-my-profile commented 2 days ago •

Thanks @m-ou-se, you're of course right, when the function performs newline trimming it also has to convert zero reads to UnexpectedEof errors, or otherwise users have no chance of detecting EOF. I added EOF handling to the RFC (along with rationales for both newline trimming and EOF handling).

Is the function being renamed to std::io::read_line or are you just keeping it as std::io::inputln?

If the function would just allocate a string, call std::io::Stdin::read_line and return the string/error unchanged, then I would agree that it should also be called read_line. In particular because as @clarfonthey remarked there is precedent for that with std::fs::read_to_string and std::io::read_to_string both just allocating a new string, calling Read::read_to_string and returning the resulting string/error.

But now that the function is additionally trimming newlines and handling EOF, I am thinking that a distinct function name might be a better choice, so that API users aren't mislead into thinking that the function returns the string/error unchanged, as is the case with the read_to_string functions.

Do we want two functions in the same standard library module with the same name but subtly different behavior? Is there precedent for that? Is that desirable?

joshtriplett commented 2 days ago

Rather than returning an error on EOF, I personally would expect this to be an expected possibility to handle: Result<Option<String>>, where Ok(None) indicates an EOF with no available data.

jsimonss commented 2 days ago

The RFC notes that contrary to println! and friends, that are macros, inputln is a function and that printing a prompt with print doesn't flush. Couldn't both be addressed by instead of making inputln a function it would be a macro.

inputln!() would function like the proposed function, inputln!(...) would do a print!(...), flush, and read line.

This would add to the namespace but the RFC seems to propose that the function is available in prelude in any case.

ijackson commented 2 days ago

I like this idea. I have some comments on details:

eof handling

I think throwing an error is correct. For someone who wants to read a number of lines, BufRead::lines is perfect. This new function is for if you think you are expecting to get an actual line.

Ie, this is supposed to be a convenience function. That means it should be as convenient for the usual use case as possible. I think that means it should return io::Result<String> not io::Result<Option<String>>.

Providing Option is particularly unhelpful because you can't just use ? - you have to write something like

   let v: i32 = io::get_line()?
       .ok_or(io::ErrorKind::UnexpectedEOF)?
       .parse()?;

or something similarly clumsy (or worse). Having a function that Just Does The Thing Or Fails is IMO definitely warranted here.

In the rare cases where you want to do something special about EOF, you can open-code the whole convenience function, or check the ErrorKind on the error.

trimming behaviour

This is going to be the second place where BufReads precise notion of newline trimming is used, without exposing it separately. According to the principle that functionality provided as part of a "bundle", should also be available separately, I think this should be made available as a method on str and String.

stdout flushing, and prompting

The lack of stdout flushing by print!() is a footgun for the "prompt and read a thing" use case. Is there some reason why the proposed new function shouldn't unconditionally flush stdout before it reads? It's already implicitly using stdin so having it implicity use stdout too seems fine to me.

The version (eg proposed inputln!() macro) which has the prompting built in is more complicated and makes the program less legible IMO. Often people who write shell scripts prefer to use printf (or echo) followed by plain read, rather than read -p, just because it makes the script clearer.

undersquire commented 2 days ago •

Personally I don't think it makes sense for it to flush stdout every time you call it, especially since this function doesn't use stdout in any other way and isn't designed to be a prompt system. I think if you wanted to do the "prompt and read" style, you would either manually flush it or use println! instead. We could also propose a separate convenience function around print! that also flushed for you.

EDIT: Just curious, is there a reason for why print! doesn't automatically flush stdout?

Personally I don't think it makes sense for it to flush stdout every time you call it,

What would the downside be? Flush is a cheap no-op if there is no buffered data.

especially since this function ... isn't designed to be a prompt system.

It is the thing we are giving to people who want to write a simple prompt thing. It has many other uses too of course, but flushing stdout is unlikely to make a big impact on those either.

I think if you wanted to do the "prompt and read" style, you would either manually flush it or use println! instead. We could also propose a separate convenience function around print! that also flushed for you.

Omitting the manual flush is an easy mistake. We normally try to save the programmer from easy mistakes. The same applies to a separate convenience function.

EDIT: Just curious, is there a reason for why print! doesn't automatically flush stdout?

It can make some programs much much slower.

undersquire commented yesterday •

Wouldn't it be better just to make this a prompting input function then? Now that I think about it, I have never taken console input without a prompt before, so I think it might be better just to simplify that aspect of it as well.

The implementation could look something like this:

pub fn inputln(prompt: &str) -> std::io::Result<String> {
    print!("{}", prompt);
    std::io::stdout().flush()?;

    ... rest of the function implementation here ...
}

This way prompting could be a lot simpler:

let name = inputln("Enter your name: ")?;

If you didn't want to prompt, just passing a "" (empty prompt string) into the function would take input without prompting anything. Performance shouldn't be an issue here either.

let name = inputln("")?;

This might be beyond the scope of this RFC but if it is going to flush stdout for you then I don't see any reason not to just tag on the prompting to it too.

Diggsey commented yesterday

If we go with the version that takes a prompt, I would probably call the function prompt instead though.

not-my-profile commented yesterday •

@jsimonss wrote:

Couldn't both be addressed by instead of making inputln a function it would be a macro.

Since the function returns a result I think keeping it a function is preferable because functions have a signature as opposed to macros and thus way better documentation of their return type.

@ijackson I agree with your suggestions. I guess str::trim_end_newline and String::trim_end_newline would be fitting names. Though I guess these should be proposed in a separate RFC? ... could somebody from the library team comment on that? I can definitely add it to Future possibilities.

Flush is a cheap no-op if there is no buffered data.

In that case adding the flush to the function seems reasonable to me. Any other opinions on this?

@undersquire:

If you didn't want to prompt, just passing a "" (empty prompt string) into the function would take input without prompting anything. Performance shouldn't be an issue here either.

Aesthetically passing an empty string looks really weird to me.

tmccombs commented 18 hours ago •

How about adding two functions, an inputln that reads a line of input without flushing stdout, and a prompt function that prints a prompt and flushes stdout before reading input?

Given inputln as described in the RFC, prompt could be implemented like:

fn prompt(prompt: &str) -> std::io::Result<String> {
    print!(prompt);
    io::stdout().flush()?;
    inputln()?
}

actually, it would probably be better for prompt to be a macro:

macro_rules! prompt {
  ($args:tt) => {
      print!($args);
      io::stdout().flush().and_then(|_| inputln())
  }
}

so that you can use format args.

Diggsey commented 18 hours ago •

Yeah, there could be a single macro, prompt!, which can be used with zero or more arguments:

let x = prompt!(); // No message
let y = prompt!("Enter your name: "); // Print a message, flush, and then wait for input
let z = prompt!("Is your name {}? ", y); // Print a message, flush, and then wait for input

It would be justified as a macro because it does the same formatting as print! and println!.

not-my-profile commented 6 hours ago •

I proposed this RFC to make it easier to explain to complete beginners how to build an interactive console game in Rust, since that's a fun exercise to learn a new programming language.

let guess = io::inputln().expect("Failed to read line");

is easier to explain than

let mut guess = String::new();

io::stdin()
    .read_line(&mut guess)
    .expect("Failed to read line");

since you don't have to deal with mutability and borrowing straight away and can introduce those a bit later. I think for teaching it's always great if you can introduce one concept after the other instead of having to say "don't worry about that for now".

For the same reason I would like this function to be as beginner-friendly as possible ... which entails implementing it as an actual function because then it has a clear signature:

pub fn inputln() -> std::io::Result<String>

As opposed to the suggested macro because for that rustdoc would show the significantly less clear:

macro_rules! prompt {
    () => { ... };
    ($($args : tt) +) => { ... };
}

I think the flusing issue can be adequately addressed by either making the function always flush beforehand (@ijackson mentioned that being a cheap no-op if there's nothing buffered) or by introducing a respective Clippy lint that detects print!(...); let x = io::inputln(); and suggests you to insert a io::stdout().flush(); in between the statements.

tmccombs commented 3 hours ago •

Personally, I think that

let guess = prompt!("Enter a number: ")?;

is a also more clear than,

print!("Enter a number: ");
std::io::stdout().flush().unwrap();
let guess = io::inputln().expect("Failed to read line");

And if documenting the return type for a macro is really that much of a problem than maybe something needs to be done with rustdoc to make it easier?

@ijackson mentioned that being a cheap no-op if there's nothing buffered

Is it guaranteed to be cheap? It also just feels wrong and non-obvious to me for inputln to flush stdout. What if the prompt is written to stderr instead of stdout? (for that matter, would a prompt! macro write the prompt to stdout or stderr, or should there be a prompt! and an eprompt! macro to mirror print! and eprint!?) What if you don't have a prompt, are reading from a pipe, and stdout has already been closed? Then inputln might unexpectedly error because it can't write to stdout (or maybe not, because it doesn't actually flush if the buffer is empty, but that seems like an implementation detail).

I also don't love the idea of adding something to the standard library solely to cater to specific examples used to teach rust. If it isn't a function that will be used in "real" programs, why teach it to new users only to later tell them that isn't how they should do it?

not-my-profile commented 3 hours ago •

I think it's a bit unfair of you to say the latter is less clear when you don't enable syntax highlighting for it^^

I however agree that it "feels wrong and non-obvious for inputln to flush stdout". You raise some good questions regarding that.

adding something to the standard library solely to cater to specific examples used to teach rust

It's of course not solely meant for that, all I am saying is that we should design it with beginners in mind.

why teach it to new users only to later tell them that isn't how they should do it?

This is not about teaching the function; this is about being able to teach basic control flow before getting into borrowing. The best way to teach control flow is with a small interactive example program. The proposed function is of course also meant to be used in "real" programs (whenever you want to trade the precise control of std::io::Stdin::read_line for more convenience).

The proposed function is likely to be the first time a Rust beginner encounters a Result, so I think having a function signature with an explicitly defined return type would really help making that encounter go over smoothly.

I think your question: what if you want to print the prompt to stderr really shows that we should not introduce a prompt macro. What if you want to always print your prompts in color? Should the macro also account for that? No of course not. My point is that a "real" Rust program doing many prompts is likely to wanting to format its prompts in an application-specific manner. For that it can just define the prompt macro itself, allowing it to be customized to the application.

If std::io::inputln is introduced defining the suggested prompt macro would also become very simple:

macro_rules! prompt {
    () => {
        io::inputln()?
    };
    ($($args : tt) +) => {
        // app-specific format string (maybe even with color if you're feeling fancy)
        print!("[{}]: ", format_args!($($args)+));
        std::io::stdout().flush()?;
        io::inputln()?
    }
}

I think it's a bit unfair of you to say the latter is less clear when you don't enable syntax highlighting for it^^

Sorry about that, it was not intentional.