io/os: Implement IsTerminal trait on Stdio #91121

ghost · 2021-11-22T02:50:48Z

Supersedes #81807
Fixes #80937

I've reimplemented the changes off of the latest master as the rebase on the old PR was causing issues.

This PR adds a IsTerminal trait to Stdin, Stdout, and Stderr for the Unix, Windows, and Hermit platforms. This allows the stdlib's CLI tests to make use of this trait instead of a custom isatty implementation contained only within the testing crate.

I also incorporated @r00ster91's feedback from the original PR on using STD_INPUT_HANDLE and such directly instead of importing and renaming them.

rust-highfive · 2021-11-22T02:50:50Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

Urgau

I was passing by so I thought I would give my impression of your PR.

PS: I'm not a Rust reviewer or maintainer, just a random guy. 😄

Urgau · 2021-11-22T09:16:32Z

library/std/src/io/stdio.rs

@@ -1202,6 +1202,59 @@ where
    }
 }

+/// Trait to determine if stdio stream (Stdin, Stdout, Stderr) is a terminal/tty.
+#[unstable(feature = "is_terminal", issue = "80937")]
+pub trait IsTerminal {


Why are you using a trait ? Are you expecting that user code will want to implement that trait ? (if not it must be sealed with a private module).

I don't see a scenario in which user code would need to implement this trait, so it could just be converted to regular impls if that is easier. I originally created it as a trait after a recommendation from @bjorn3: #81807 (comment)

I do think this should start out as a sealed trait. I can imagine reasons why people might want to implement it in the future, but we should be able to make the decision about exposing the trait and its method independently of the decision about letting people implement it outside the standard library.

Urgau · 2021-11-22T09:16:35Z

library/std/src/io/stdio.rs

+#[unstable(feature = "is_terminal", issue = "80937")]
+pub trait IsTerminal {
+    /// returns true if stdio stream (Stdin, Stdout, Stderr) is a terminal/tty.
+    fn is_terminal() -> bool;


This should maybe return an Option of bool where None will be returned when we are unable to determine if it's either a terminal or not.

Suggested change

fn is_terminal() -> bool;

fn is_terminal() -> Option<bool>;

This could be a solution to the Windows hack problem. Instead of falling back to the msys_tty_on hack, we just return None? Although I'm not sure how useful this would be to the end user, as I'm assuming that any Nones would have to be treated the same as Some(false) in the calling code.

I think it'd be appropriate to return io::Result<bool> here. If we get the expected error that indicates "not a terminal" we should return false, but if we get an unexpected error we should return that error

bjorn3 · 2021-11-22T09:20:32Z

library/std/src/sys/unix/io.rs

+#[unstable(feature = "is_terminal", issue = "80937")]
+impl io::IsTerminal for sys::stdio::Stdin {
+    fn is_terminal() -> bool {
+        unsafe { libc::isatty(libc::STDIN_FILENO) != 0 }


Won't this return true in case of an error?

The man page indicates that this function can only return 0 or 1: https://linux.die.net/man/3/isatty

I could make it libc::isatty(libc::STDIN_FILENO) == 1 to be more explicit?

ChrisDenton · 2021-11-22T09:40:47Z

I'm not sure I understand the intent of the Windows code. It returns true if any handle is a console handle? That doesn't seem right? What if, say, Stdout has been redirected? Or is null?

I'm also somewhat concerned about having the msys hack in the standard library as it looks like it's purely based on the file name. Though I'm not sure how much of a problem this will be in practice.

bjorn3 · 2021-11-22T12:29:45Z

I'm not sure I understand the intent of the Windows code. It returns true if any handle is a console handle? That doesn't seem right? What if, say, Stdout has been redirected? Or is null?

I think the intent is that if any handle is a console handle, then we are running inside a non-msys terminal, so if say stdout is not a console handle, then we know for a fact that it is redirected. If all handles are not console handles, then we may be running inside an msys terminal, so we need to use the filename hack to check if this is the case. Stdin and stdout and stderr are rarely redirected at the same time, right? So the filename hack should rarely be used.

ChrisDenton · 2021-11-22T12:41:36Z

Ah, thanks I get it now. I guess what I'm struggling with is how reliable these heuristics are and in what situations they're needed.

ChrisDenton · 2021-11-22T12:56:30Z

For prior art, git uses GetFileType instead to check if the handle is a pipe. Only then does it do a test for special names. Source.

ghost · 2021-11-22T15:04:35Z

Ah, thanks I get it now. I guess what I'm struggling with is how reliable these heuristics are and in what situations they're needed.

The code for this is basically lifted from the atty crate: https://github.com/softprops/atty/blob/master/src/lib.rs.

I'm not an expert on the ins and outs of Windows terminals unfortunately, so i'll have to defer to those who know better.

@BurntSushi was the original author of the Windows logic, and explains the reasoning here #81807 (comment). He also mentions how git does it.

I suppose the biggest issue is if we are OK with having a heuristic-y detection inside the standard library. But considering git does something very similar, there doesn't seem to be another option besides just not having this functionality at all, or at least not on Windows.

BurntSushi · 2021-11-22T15:25:23Z

@BurntSushi was the original author of the Windows logic, and explains the reasoning here #81807 (comment). He also mentions how git does it.

Right. I got the logic straight from git.

Folks are right to be skeptical of it. It's quite precarious looking, and it certainly is not bullet proof. But, it works well enough in practice. I do think it's probably tied to whether we expose this sort of functionality at all in the first place. If we expose something that doesn't work in some particularly common cases on Windows, then I think that just wouldn't be good because probably folks wouldn't use it or have to invent their own work-arounds.

ChrisDenton · 2021-11-22T15:27:48Z

He also mentions how git does it.

Huh, I guess the git code has changed slightly. Or I'm looking at something different.

Side note: I've been testing this a bit. It seems that the version of msys used by "Git Bash" on Windows uses winpty so that non-msys applications run as expected (or at least on my install of it does). This confused me at first because I couldn't initially reproduce the issue until I switched to a clean install of msys2.

BurntSushi · 2021-11-22T15:44:44Z

Oh interesting. I remember learning about the high level logic from git and porting it. But that was some number of years ago. I'll try to give a look later and see if I can clarify.

…

On Mon, Nov 22, 2021, 10:27 Chris Denton ***@***.***> wrote: He also mentions how git does it. Huh, I guess the git code has changed slightly. Or I'm looking at something different. Side note: I've been testing this a bit. It seems that the version of msys used by "Git Bash" on Windows uses winpty <https://github.com/rprichard/winpty> so that non-msys applications run as expected (or at least on my install of it does). This confused me at first because I couldn't initially reproduce the issue until I switched to a clean install of msys2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#91121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADPPYWTTPR36IITHIDYLIDUNJOP7ANCNFSM5IP5RDQA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

ChrisDenton · 2021-11-22T15:57:52Z

No worries. The core part of testing the name is the same in any case. I'm trying to see if there is any precedent in other language standard libraries to see if they are of any help.

ghost · 2021-12-02T15:18:37Z

@m-ou-se @bjorn3 etc: Did you have any comments on this PR? Not sure what the next steps are for this.

ChrisDenton · 2021-12-02T15:34:41Z

If it helps, I don't think my reservations should be a blocker to adding this unstably. It could be mentioned as a concern in the tracking issue or the msys heuristics could be put in a separate PR and discussed independently. I'd personally be ok with either.

ghost · 2021-12-10T01:06:05Z

r? @Amanieu

joshtriplett · 2021-12-10T06:58:09Z

library/std/src/io/stdio.rs

+#[unstable(feature = "is_terminal", issue = "80937")]
+pub trait IsTerminal {
+    /// returns true if stdio stream (Stdin, Stdout, Stderr) is a terminal/tty.
+    fn is_terminal() -> bool;


Suggested change

fn is_terminal() -> bool;

fn is_terminal() -> io::Result<bool>;

This would allow for implementations that might return errors when attempting to determine this.

joshtriplett · 2021-12-10T07:02:22Z

I do think this trait needs to be sealed before we add it.

I also personally think it should return an io::Result<bool>, to allow for implementations where that determination could return an error. In particular, that could happen if called on a bad file descriptor.

In the future, I'd love to see implementations of this trait for BorrowedFd and OwnedFd, but that doesn't need to happen in the initial PR.

Amanieu · 2021-12-13T02:31:20Z

library/std/src/sys/windows/io.rs

+
+#[unstable(feature = "is_terminal", issue = "80937")]
+impl io::IsTerminal for sys::stdio::Stdin {
+    fn is_terminal() -> bool {


There's a lot of duplication here, this should be factored out into a method that takes fd and others as arguments.

Amanieu · 2021-12-13T02:52:20Z

library/std/src/sys/hermit/stdio.rs

@@ -118,3 +118,23 @@ pub fn is_ebadf(_err: &io::Error) -> bool {
 pub fn panic_output() -> Option<impl io::Write> {
    Some(Stderr::new())
 }
+
+#[unstable(feature = "is_terminal", issue = "80937")]
+impl io::IsTerminal for sys::stdio::Stdin {


The types in sys are private, so there is no need to use a trait here. These can just be inherent methods on the private type.

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

… r=thomcc Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

… r=thomcc Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

Dylan-DPC · 2022-09-10T12:05:29Z

Closing this due to inactivity

… r=thomcc Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

… r=thomcc Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…=thomcc Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang#91121 Co-authored-by: Matt Wilkinson <[email protected]>

…rminal The UNIX and WASI implementations use `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and their locked counterparts on all platforms. On UNIX and WASI, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang/rust#91121 Co-authored-by: Matt Wilkinson <[email protected]>

Add `IsTerminal` trait to determine if a descriptor or handle is a terminal The UNIX implementation uses `isatty`. The Windows implementation uses the same logic the `atty` crate uses, including the hack needed to detect msys terminals. Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for `BorrowedHandle`/`OwnedHandle`. Based on rust-lang/rust#91121 Co-authored-by: Matt Wilkinson <[email protected]>

rust-highfive assigned m-ou-se Nov 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2021

ghost mentioned this pull request Nov 22, 2021

io/os: Implement IsTerminal trait on Stdio #81807

Closed

This comment has been minimized.

Sign in to view

Urgau reviewed Nov 22, 2021

View reviewed changes

bjorn3 reviewed Nov 22, 2021

View reviewed changes

apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 22, 2021

rust-highfive assigned Amanieu and unassigned m-ou-se Dec 10, 2021

dtolnay 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 Dec 10, 2021

joshtriplett reviewed Dec 10, 2021

View reviewed changes

Amanieu reviewed Dec 13, 2021

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2022

Dylan-DPC closed this Sep 10, 2022

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2022

kwi-dk mentioned this pull request Oct 17, 2022

Tracking Issue for IsTerminal / is_terminal #98070

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io/os: Implement IsTerminal trait on Stdio #91121

io/os: Implement IsTerminal trait on Stdio #91121

ghost commented Nov 22, 2021 •

edited by ghost

Loading

rust-highfive commented Nov 22, 2021

This comment has been minimized.

This comment has been minimized.

Urgau left a comment

Urgau Nov 22, 2021 •

edited

Loading

ghost Nov 22, 2021

joshtriplett Dec 10, 2021

Urgau Nov 22, 2021

ghost Nov 22, 2021

joshtriplett Mar 21, 2022

bjorn3 Nov 22, 2021

ghost Nov 22, 2021 •

edited by ghost

Loading

ChrisDenton commented Nov 22, 2021

bjorn3 commented Nov 22, 2021

ChrisDenton commented Nov 22, 2021

ChrisDenton commented Nov 22, 2021 •

edited

Loading

ghost commented Nov 22, 2021

BurntSushi commented Nov 22, 2021 •

edited

Loading

ChrisDenton commented Nov 22, 2021

BurntSushi commented Nov 22, 2021 via email

ChrisDenton commented Nov 22, 2021

ghost commented Dec 2, 2021

ChrisDenton commented Dec 2, 2021

ghost commented Dec 10, 2021

joshtriplett Dec 10, 2021

joshtriplett commented Dec 10, 2021

Amanieu Dec 13, 2021

Amanieu Dec 13, 2021

Dylan-DPC commented Sep 10, 2022

	fn is_terminal() -> bool;
	fn is_terminal() -> io::Result<bool>;

io/os: Implement IsTerminal trait on Stdio #91121

io/os: Implement IsTerminal trait on Stdio #91121

Conversation

ghost commented Nov 22, 2021 • edited by ghost Loading

rust-highfive commented Nov 22, 2021

This comment has been minimized.

This comment has been minimized.

Urgau left a comment

Choose a reason for hiding this comment

Urgau Nov 22, 2021 • 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

ghost Nov 22, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ChrisDenton commented Nov 22, 2021

bjorn3 commented Nov 22, 2021

ChrisDenton commented Nov 22, 2021

ChrisDenton commented Nov 22, 2021 • edited Loading

ghost commented Nov 22, 2021

BurntSushi commented Nov 22, 2021 • edited Loading

ChrisDenton commented Nov 22, 2021

BurntSushi commented Nov 22, 2021 via email

ChrisDenton commented Nov 22, 2021

ghost commented Dec 2, 2021

ChrisDenton commented Dec 2, 2021

ghost commented Dec 10, 2021

Choose a reason for hiding this comment

joshtriplett commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-DPC commented Sep 10, 2022

ghost commented Nov 22, 2021 •

edited by ghost

Loading

Urgau Nov 22, 2021 •

edited

Loading

ghost Nov 22, 2021 •

edited by ghost

Loading

ChrisDenton commented Nov 22, 2021 •

edited

Loading

BurntSushi commented Nov 22, 2021 •

edited

Loading