Add conversions from File and Child* handles to Stdio

[cuviper]

Stdio now implements From<ChildStdin>, From<ChildStdout>,
From<ChildStderr>, and From<File>.

The Command::stdin/stdout/stderr methods now take any type that
implements Into<Stdio>.

This makes it much easier to write shell-like command chains, piping to
one another and redirecting to and from files. Otherwise one would need
to use the unsafe and OS-specific from_raw_fd or from_raw_handle.

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[cuviper]

The idea came from this question:
https://users.rust-lang.org/t/why-no-portable-way-to-redirect-output-to-a-file-in-process-command/10830

[droundy]

Based on d0810ad, it looks like this is not a backwards-compatible change, since it changes the type signature of stdin, etc. Isn't that a problem?

[alexcrichton]

This sounds like a great idea to me! Now I'm surprised we didn't do this earlier...

@droundy you're right that this is technically backwards-incompatible. For changes like this we typically run them through crater/cargobomb, automated systems to determine the amount of breakage in the ecosystem for a proposed change. I'll both tag this as needs-crater and also...

@rfcbot fcp merge

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @dtolnay
• @sfackler

Concerns:

• into (Add conversions from File and Child* handles to Stdio #42133 (comment))

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[cuviper]

Right, it should be considered a minor change according to RFC1105 here. Note the fix changing FromRawFd::from_raw_fd to Stdio::from_raw_fd doesn't change the actual types involved. It's using <Stdio as FromRawFd>::from_raw_fd(...) either way, but with the more generic methods the compiler couldn't infer that.

Crater will tell us if this is widespread. If so, we can consider more explicit alternatives for the new functionality. e.g. We could revert the generic methods so you'd call .stdout(output_file.into()), or go all the way to something like .stdout(Stdio::from_file(output_file)).

[BurntSushi]

Even if the crater run passes, I feel like the added generics are not pulling their weight. They make the signature more complex.

@rfcbot concern into

[cuviper]

They do make the signature more complex, but that concern should be balanced against making the calling code simpler. Writing just .stdout(output_file) looks to me like a very natural way to specify file redirection.

[brson]

I've started builds for a crater run

• crater Add conversions from File and Child* handles to Stdio #42133
  • https://github.com/cuviper/rust
  • before 7ac844f
  • https://tools.taskcluster.net/task-inspector/#TWTnc5GkQQ-ZC3wWU9d5Ug
  • after d0810ad060e468847a76f937debe0ca0f6cbd9dc
  • https://tools.taskcluster.net/task-inspector/#fdgawqnCQKuAMDut-HSRXQ

[brson]

I've started building crates for crater.

[brson]

https://gist.github.com/anonymous/731133fe802610f1e4a9212bcb69994d

[brson]

I see a lot of false positives...

[cuviper]

The first few I sampled look like infrastructure trouble, right?

curl: (56) GnuTLS recv error (-54): Error in the pull function.

[Mark-Simulacrum]

All actual root regressions noted by crater appear to be network failures... so no root regressions?

[alexcrichton]

@BurntSushi mind elaborating a bit on your thoughts here? I personally feel like these are pretty nice signatures and our usage of AsRef<Path> and such throughout the rest of libstd I think doesn't necessarily raise the cognitive burden on these methods too much. That, coupled with documentation, I think can mitigate problems with generics (in my mind at least)

[BurntSushi]

To clarify: I don't feel that strongly about it and I'm fine removing my concern, but I did at least want to bring it up in case others felt similarly.

I'm mostly just bleating about an issue that I think has problems in the aggregate, but is hard to nail down when talking about any one specific instance. For the most part, I just think that generic signatures add cognitive burden that makes them harder to understand. Basically, I would rather judge each instance on 1) its consistency with other uses of generics and 2) its bang-to-buck ratio. I think Into<Stdio> passes (1) but (2) is more questionable. I feel like AsRef<Path> and the like are used so much that they get a lot of utility. On the other hand, for Into<Stdio>, what exactly are we getting in return? I can't imagine its use would rival that of AsRef<Path>. Do we really think stdout(file) instead of stdout(Stdio::from(file)) is enough of a win to justify the more complicated signature? Of course, the argument can go the other way as well...

[cuviper]

Do we really think stdout(file) instead of stdout(Stdio::from(file)) is enough of a win to justify the more complicated signature?

I do think the slight complication in the signature is outweighed by the simplification in every place that will call it. I find stdout(Stdio::from(file)) rather verbose -- explicitness that doesn't really add any clarity.

But I worry more about folks writing stdout(From::from(file)) or stdout(file.into()), and I'd probably choose the latter just for brevity. Once either of those becomes commonplace, we can't later choose to make these methods generic without making those calls ambiguous. So I think it's now or never.

[BurntSushi]

@rfcbot resolve into

[sfackler]

I am mildly concerned about the use of From as well - the argument seems to be that it makes the API more convenient to use, but isn't that true for literally every argument in every function in the entire library?

[cuviper]

Stdio is already basically just a wrapper to make those methods more generic, so we don't need separate stdout_piped(), stdout_inherited(), etc. You can't actually do anything with a value of Stdio on its own, which is why I think it's not valuable to make the user explicitly create the Stdio wrapper from a File.

[alexcrichton]

ping @brson, checkbox!

[sfackler]

Can we split the function signature change out? I really like the ability to get a Stdio from a File, but am still kind of concerned about the Into bound, which would be insta-stable.

[cuviper]

If that's the consensus, I'll split it. My concern (as noted before) is that it will be hard to add later once people are manually converting. e.g. stdout(file.into()) is not compatible with stdout<T: Into<Stdio>>.

[alexcrichton]

I personally agree with @cuviper that we need to decide this now or basically break everyone when we do make it generic. Currenly though everyone's signed off but @brson?

[alexcrichton]

er sorry didn't mean to close

[cuviper]

Note to self, these will need to become 1.20.0 if we don't make it before the upcoming branch.

[cuviper]

Rebased, squashed, and bumped to 1.20.0.

It looks like everyone has checked off -- does @rfcbot need a nudge?

[alexcrichton]

Ah no I think it just polls on a longer interval, in any case thanks again @cuviper!

@bors: r+

[bors]

📌 Commit 9debe91 has been approved by alexcrichton

[bors]

⌛ Testing commit 9debe91 with merge 21d0f91...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 21d0f91 to master...