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

Open
1 of 3 tasks
joshtriplett opened this issue Jun 13, 2022 · 12 comments
Open
1 of 3 tasks

Tracking Issue for IsTerminal / is_terminal #98070

joshtriplett opened this issue Jun 13, 2022 · 12 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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

joshtriplett commented Oct 11, 2022

@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

BurntSushi commented Oct 17, 2022

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

joshtriplett commented Oct 19, 2022

@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

BurntSushi commented Oct 19, 2022

@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

joshtriplett commented Oct 19, 2022

@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

joshtriplett commented Oct 22, 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.

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.

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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants