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

Tracking Issue for IsTerminal / is_terminal #98070

Closed
1 of 3 tasks
joshtriplett opened this issue Jun 13, 2022 · 21 comments · Fixed by #110072
Closed
1 of 3 tasks

Tracking Issue for IsTerminal / is_terminal #98070

joshtriplett opened this issue Jun 13, 2022 · 21 comments · Fixed by #110072
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jun 13, 2022

Feature gate: #![feature(is_terminal)]

This is a tracking issue for the IsTerminal trait and its is_terminal method, to determine if a descriptor/handle refers to a terminal.

Public API

// std::io

pub trait IsTerminal {
    fn is_terminal(&self) -> bool;
}

impl IsTerminal for Stdin { /* ... */ }
impl IsTerminal for StdinLock<'_> { /* ... */ }
impl IsTerminal for Stdout { /* ... */ }
impl IsTerminal for StdoutLock<'_> { /* ... */ }
impl IsTerminal for Stderr { /* ... */ }
impl IsTerminal for StderrLock<'_> { /* ... */ }
impl IsTerminal for File { /* ... */ }

#[cfg(unix)]
impl IsTerminal for BorrowedFd { /* ... */ }
#[cfg(unix)]
impl IsTerminal for OwnedFd { /* ... */ }

#[cfg(windows)]
impl IsTerminal for BorrowedHandle { /* ... */ }
#[cfg(windows)]
impl IsTerminal for OwnedHandle { /* ... */ }

Steps / History

@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jun 13, 2022
@nagisa
Copy link
Member

nagisa commented Oct 9, 2022

Not a strong objection, but I wonder if there has been any discussion about making this universally available vs. it being a OS-specific Ext trait? I guess on std-systems that don’t have a concept of a terminal at all this can be implemented to always just return Ok(false), but that doesn’t sound obviously superior over telling callers that, “no, terminal is not a thing here.”

@joshtriplett
Copy link
Member Author

@nagisa I've updated the API (and now the documentation in this tracking issue): it now just returns bool, and returns false (not a terminal) if something goes wrong.

I personally think that there's value in being able to call it universally and just get a "false", because "treat as a non-terminal" is almost always the correct behavior in such cases: don't try to do fancy formatting, don't try to invoke a pager, don't do progress bars, just output text.

@BurntSushi
Copy link
Member

I personally think that there's value in being able to call it universally and just get a "false", because "treat as a non-terminal" is almost always the correct behavior in such cases: don't try to do fancy formatting, don't try to invoke a pager, don't do progress bars, just output text.

I don't feel strongly here either I think, but an alternative and perhaps slightly more idiosyncratic API would be Result<(), SomeErrorTypeName>. If the result is Ok, then it's a terminal, and otherwise there is an error explaining why it is believed to not be a terminal. AIUI, for example, the detection logic on Windows (assuming it was copied/derived from my patch to atty) is somewhat of a heuristic and in theory could return false results. In that sense, it could be nice for failure modes to document why is_terminal returned false. Then applications could either bubble up the error or log it as a warning and tree the error as "not a terminal."

On the other hand, it might be just as interesting to report why is_terminal thinks the file descriptor is a terminal too.

So maybe just a bool is simplest then. But it might be worth updating the docs to note that heuristics may be used. (Again, assuming we are doing what atty does. And if we aren't... well, I'd want to talk about that.)

@kwi-dk
Copy link

kwi-dk commented Oct 17, 2022

This is a nice addition to std! But... oof, that return value. Combining Ok(false) and Err(_) into a single false return value seems to me like a mistake (a repeat of Path::exists / #83186, not to mention a hundred broken APIs in a dozen other languages). Looking through the issue history, I realize I'm beating a horse that was already dying back in #91121, but there is inherent complexity here, and I really don't think std should try to paper over something like a failing syscall.

It's trivial for the caller to write is_terminal().unwrap_or(false) if that's the logic they want. Even if that ends up being what every single user does, returning a Result still seems preferable from an API design perspective. Friends don't let friends silently ignore errors.

(As to the error type, I'd say any io::Error will do... if people have more exact needs than that, talking directly to the OS is almost certainly the way to go.)

@ErichDonGubler
Copy link
Contributor

ErichDonGubler commented Oct 18, 2022

I'm not sure if this has been discussed, but I'm actually surprised that this is in std on nightly. I know that until now, the isatty crate and others have served to expose this functionality. As a Rustacean, I'm accustomed to the notion of std taking on a very small scope. In this way, this new surface seems to change from that historical precedent?

Out of curiosity, what's the impetus for experiment with standardizing this functionality in std now, instead of "blessing" a crate?

@joshtriplett
Copy link
Member Author

@BurntSushi @kwi-dk I originally wrote this to return an error, but it turns out in practice that there aren't any interesting error cases on existing platforms. For instance, on UNIX, apart from "success, this is a terminal" or "this is not a terminal", the only error is "you passed something that isn't a file descriptor", and that isn't a useful case for any of the types you can call is_terminal on. (It would imply having a File or OwnedFd or BorrowedFd or stdio type whose file descriptor was closed out from under it, and whether it's a terminal or not, you won't be able to do anything else with it either without discovering it isn't open.)

@BurntSushi
Copy link
Member

@joshtriplett For Unix, yeah, I probably agree. All of the interesting error cases would be on the Windows side. And there is the question of not just "did a syscall fail somewhere" but also "why is it thought that this is a terminal" and "why is it thought that this is not a terminal."

@joshtriplett
Copy link
Member Author

@BurntSushi I don't think in practice "why is it thought that this is a terminal" has a very useful answer. And if you're looking for Windows-specific info like "did this have one of the magic names", that seems like something we shouldn't expose in any stable fashion, because eventually we hope it'll go away.

@joshtriplett
Copy link
Member Author

joshtriplett commented Oct 19, 2022

@ErichDonGubler There are multiple crates providing this functionality, such as atty. But this is something widely useful, for which differentiation of any kind between implementations doesn't provide substantive value, and it isn't expected to ever become obsolete. We don't, in general, "bless" crates in the ecosystem; that tends to lead to multiple problems, not least of which if the implementation we point to has maintenance issues. (Case in point: the isatty crate you mention is deprecated, and points to atty.) The only way we can really vouch for the maintenance status of a crate is if we maintain it.

I don't think this is a change from historical precedent. We're not going to add GUIs or XML parsers to the standard library. The general reaction to this change from users of atty has, in general, seemed to be various degrees of elation.

@BurntSushi
Copy link
Member

BurntSushi commented Oct 19, 2022

I'm not advocating (or even suggesting hypothetically) that we expose "did this have one of the magic names" in any stable fashion. :-)

I'm not quite sure where the disconnect is here. I'll try again. Broadly, I have two points to bring up.

The first point are the docs. Today, the docs don't mention that heuristics may be used in determining whether something is attached to a terminal or not. I think we should mention that, even if we hope that the heuristics used on Windows will one day go away. (Looking for those magic names is the heuristic I'm thinking about specifically.)

The second point is that because we use heuristics (and I don't see those going away any time soon), the bool returned by this API can be very lossy. The case I'm thinking about here is when someone calls is_terminal and gets false but expects true. (Or even the inverse.) Someone who is paying attention will notice that the API returns false in case of an error, and so might know enough to look at a recording of the syscalls made by their process. But then they see that no syscalls failed. The question now is: why was false returned? Since they're on Windows and they know they've seen true returned, they know this API is supported on Windows.

Thus, it could be sensible to return something richer than just a bool. We do not have to stabilize any strange heuristic. For example, a helpful error message could be "not a terminal because the file name associated with the handle does not match internal heuristic rules." An application could log that in a way that is available to an end user. (I would do this in ripgrep when the --debug flag is given for example.)


To be clear, for my second point above, I am not saying "this is definitely what we should do." But rather, "it is perhaps something we should consider." I am myself not convinced that the extra ceremony is worth the improvement in failure modes, but it could be. It is perhaps possible that I feel this a little more strongly because, in the course of adding the heuristic to atty many moons ago, the heuristic was much more fragile than it was today and incorrect results happened and were very difficult to debug.

It is plausible that the heuristic is good enough that it simply will almost never do something unexpected. In which case, the value of better failure modes decreases.


To be doubly clear, I would rather have this API return a bool then not have it at all.

@joshtriplett
Copy link
Member Author

I'm not advocating (or even suggesting hypothetically) that we expose "did this have one of the magic names" in any stable fashion. :-)

I'm not quite sure where the disconnect is here. I'll try again. Broadly, I have two points to bring up.

I misunderstood, thank you for clarifying.

The first point are the docs. Today, the docs don't mention that heuristics may be used in determining whether something is attached to a terminal or not. I think we should mention that, even if we hope that the heuristics used on Windows will one day go away. (Looking for those magic names is the heuristic I'm thinking about specifically.)

I completely agree that the documentation should explain the details of the heuristics.

The second point is that because we use heuristics (and I don't see those going away any time soon), the bool returned by this API can be very lossy. The case I'm thinking about here is when someone calls is_terminal and gets false but expects true. (Or even the inverse.) Someone who is paying attention will notice that the API returns false in case of an error, and so might know enough to look at a recording of the syscalls made by their process. But then they see that no syscalls failed. The question now is: why was false returned? Since they're on Windows and they know they've seen true returned, they know this API is supported on Windows.

Thus, it could be sensible to return something richer than just a bool. We do not have to stabilize any strange heuristic. For example, a helpful error message could be "not a terminal because the file name associated with the handle does not match internal heuristic rules." An application could log that in a way that is available to an end user. (I would do this in ripgrep when the --debug flag is given for example.)

To be clear, for my second point above, I am not saying "this is definitely what we should do." But rather, "it is perhaps something we should consider." I am myself not convinced that the extra ceremony is worth the improvement in failure modes, but it could be. It is perhaps possible that I feel this a little more strongly because, in the course of adding the heuristic to atty many moons ago, the heuristic was much more fragile than it was today and incorrect results happened and were very difficult to debug.

It is plausible that the heuristic is good enough that it simply will almost never do something unexpected. In which case, the value of better failure modes decreases.

I personally think that attempting to return an error indicating the reason for a false would prove awkward to use in the common case, and I would expect that the vast majority of programs would not want that information. If you're trying to debug the heuristic, it makes sense to have this information. If you're running a random CLI program on Windows, I don't think the program should ever report "assuming you don't have a terminal because your stderr is a pipe but it isn't named XYZ".

I feel like that's a really good argument for having a debug version of the standard library where it's possible to enable all sorts of tracing via environment variables.

@kwi-dk
Copy link

kwi-dk commented Oct 24, 2022

on UNIX, apart from "success, this is a terminal" or "this is not a terminal", the only error is "you passed something that isn't a file descriptor" [which] would imply having a File or OwnedFd or BorrowedFd or stdio type whose file descriptor was closed out from under it

Right, and I realize now that that would require unsoundness. So barring kernel bugs or OS-level shenanigans (like LD_PRELOAD, ptrace or seccomp-bpf, which are all out of scope in the same manner as /proc/self/mem), the call is literally infallible on Linux (and presumably other POSIX systems). And WASI is modeled on POSIX, and while the Wasm host can make the "syscall" return whatever error it wants, that too can probably be filed under "kernel bugs or OS-level shenanigans".

On Windows I can certainly manufacture odd situations where a console handle has only GENERIC_WRITE access (and not the GENERIC_READ needed to query terminal data), leading to permission error and a false negative, but having reread and poked the code I agree that's probably not interesting. (As for the cygwin/msys heuristic, that is by definition best-effort only.)

What quibbles remain then comes down to documentation. I withdraw my concern, and look forward to using this in my CLI tools.

@Xiretza
Copy link
Contributor

Xiretza commented Nov 19, 2022

Since I haven't seen this suggested before: how about returning Option<bool>? That alleviates the need to create a (probably useless) error type, but still allows the "we don't know" case to be handled if needed.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 8, 2022
…nce, r=thomcc

kmc-solid: `std::sys` code maintenance

Includes a set of changes to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets and make some other improvements.

- Address `fuzzy_provenance_casts` by using `expose_addr` and `from_exposed_addr` for pointer-integer casts
- Add a stub implementation of `is_terminal` (rust-lang#98070)
- Address `unused_imports` and `unused_unsafe`
- Stop doing `Box::from_raw(&*(x: Box<T>) as *const T as *mut T)`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 8, 2022
…nce, r=thomcc

kmc-solid: `std::sys` code maintenance

Includes a set of changes to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets and make some other improvements.

- Address `fuzzy_provenance_casts` by using `expose_addr` and `from_exposed_addr` for pointer-integer casts
- Add a stub implementation of `is_terminal` (rust-lang#98070)
- Address `unused_imports` and `unused_unsafe`
- Stop doing `Box::from_raw(&*(x: Box<T>) as *const T as *mut T)`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 8, 2022
…nce, r=thomcc

kmc-solid: `std::sys` code maintenance

Includes a set of changes to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets and make some other improvements.

- Address `fuzzy_provenance_casts` by using `expose_addr` and `from_exposed_addr` for pointer-integer casts
- Add a stub implementation of `is_terminal` (rust-lang#98070)
- Address `unused_imports` and `unused_unsafe`
- Stop doing `Box::from_raw(&*(x: Box<T>) as *const T as *mut T)`
etehtsea added a commit to etehtsea/spin that referenced this issue Dec 11, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
…nce, r=thomcc

kmc-solid: `std::sys` code maintenance

Includes a set of changes to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets and make some other improvements.

- Address `fuzzy_provenance_casts` by using `expose_addr` and `from_exposed_addr` for pointer-integer casts
- Add a stub implementation of `is_terminal` (rust-lang#98070)
- Address `unused_imports` and `unused_unsafe`
- Stop doing `Box::from_raw(&*(x: Box<T>) as *const T as *mut T)`
@joshtriplett
Copy link
Member Author

@BurntSushi I've posted #109687 to add the heuristics to the documentation. Sorry for the delay in doing so.

@joshtriplett
Copy link
Member Author

I'd like to propose stabilizing this. I'd like to find out if we have consensus in doing so.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 28, 2023

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 28, 2023
@rfcbot
Copy link

rfcbot commented Mar 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@ranile
Copy link
Contributor

ranile commented Mar 31, 2023

There are multiple crates providing this functionality, such as atty. But this is something widely useful, for which differentiation of any kind between implementations doesn't provide substantive value, and it isn't expected to ever become obsolete.

I don't quite agree with this. While it's true this can safely be added to std, I don't see why it should be. It has lived fine in crates and I'm not able to think of any problems that are solved by bringing it in std

@faern
Copy link
Contributor

faern commented Mar 31, 2023

It has lived fine in crates and I'm not able to think of any problems that are solved by bringing it in std

I don't agree with it being fine in other crates. atty has been the de facto standard crate for this for years. But it has known problems and is unmaintained. So it has been a pretty long road trying to make various crates migrate to is-terminal instead. That job is still not done for us. Many semi-unmaintained crates that we rely on still depend on atty. Crates on crates.io always has the risk of becoming unmaintained while std hopefully never will become unmaintained. While std should not be batteries included and not have stuff that might risk being obsolete in a few years, the check for terminal is not one of these things. It will be relevant many years from now, and it's better to have it be well implemented rather than spread out in various crates with potential bugs in the implementation.

@Amanieu
Copy link
Member

Amanieu commented Mar 31, 2023

There is also a desire to use this in std to determine whether to line-buffer or block-buffer stdout (#60673). In this case we would want to expose the logic that std uses to avoid users relying on third party crates that might determine is_terminal differently.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 7, 2023
@rfcbot
Copy link

rfcbot commented Apr 7, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 13, 2023
…, r=Mark-Simulacrum

Stabilize IsTerminal

FCP completed in rust-lang#98070 .

closes: rust-lang#98070
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 13, 2023
…, r=Mark-Simulacrum

Stabilize IsTerminal

FCP completed in rust-lang#98070 .

closes: rust-lang#98070
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 13, 2023
…, r=Mark-Simulacrum

Stabilize IsTerminal

FCP completed in rust-lang#98070 .

closes: rust-lang#98070
@bors bors closed this as completed in afd45c2 Apr 13, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 13, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Apr 14, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Apr 14, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jun 1, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.