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

io/os: Implement IsTerminal trait on Stdio #91121

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion library/std/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,8 @@ pub use self::buffered::WriterPanicked;
#[unstable(feature = "internal_output_capture", issue = "none")]
#[doc(no_inline, hidden)]
pub use self::stdio::set_output_capture;
#[unstable(feature = "is_terminal", issue = "80937")]
pub use self::stdio::IsTerminal;
#[unstable(feature = "print_internals", issue = "none")]
pub use self::stdio::{_eprint, _print};
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -2379,7 +2381,11 @@ impl<T: BufRead, U: BufRead> BufRead for Chain<T, U> {
}

fn consume(&mut self, amt: usize) {
if !self.done_first { self.first.consume(amt) } else { self.second.consume(amt) }
if !self.done_first {
self.first.consume(amt)
} else {
self.second.consume(amt)
}
}
}

Expand Down
53 changes: 53 additions & 0 deletions library/std/src/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,59 @@ where
}
}

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

@Urgau Urgau Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub trait IsTerminal {
pub trait IsTerminal: Sealed {

/// returns true if stdio stream (Stdin, Stdout, Stderr) is a terminal/tty.
fn is_terminal() -> bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

cfg_if::cfg_if! {
if #[cfg(any(unix, windows))] {
#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stdin {
fn is_terminal() -> bool {
stdio::Stdin::is_terminal()
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stdout {
fn is_terminal() -> bool {
stdio::Stdout::is_terminal()
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stderr {
fn is_terminal() -> bool {
stdio::Stderr::is_terminal()
}
}
} else {
#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stdin {
fn is_terminal() -> bool {
false
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stdout {
fn is_terminal() -> bool {
false
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl IsTerminal for Stderr {
fn is_terminal() -> bool {
false
}
}
}
}

#[unstable(
feature = "print_internals",
reason = "implementation detail which may disappear or be replaced at any time",
Expand Down
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@
#![feature(hashmap_internals)]
#![feature(int_error_internals)]
#![feature(intra_doc_pointers)]
#![feature(is_terminal)]
#![feature(lang_items)]
#![feature(linkage)]
#![feature(log_syntax)]
Expand Down
20 changes: 20 additions & 0 deletions library/std/src/sys/hermit/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn is_terminal() -> bool {
abi::isatty(abi::STDIN_FILENO)
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stdout {
fn is_terminal() -> bool {
abi::isatty(abi::STDOUT_FILENO)
}
}
#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stderr {
fn is_terminal() -> bool {
abi::isatty(abi::STDERR_FILENO)
}
}
23 changes: 22 additions & 1 deletion library/std/src/sys/unix/io.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::io;
use crate::marker::PhantomData;
use crate::slice;

use crate::sys;
use libc::{c_void, iovec};

#[derive(Copy, Clone)]
Expand Down Expand Up @@ -74,3 +75,23 @@ impl<'a> IoSliceMut<'a> {
unsafe { slice::from_raw_parts_mut(self.vec.iov_base as *mut u8, self.vec.iov_len) }
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mattwilkinsonn mattwilkinsonn Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stdout {
fn is_terminal() -> bool {
unsafe { libc::isatty(libc::STDOUT_FILENO) != 0 }
}
}
#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stderr {
fn is_terminal() -> bool {
unsafe { libc::isatty(libc::STDERR_FILENO) != 0 }
}
}
8 changes: 8 additions & 0 deletions library/std/src/sys/windows/c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ pub const SECURITY_SQOS_PRESENT: DWORD = 0x00100000;

pub const FIONBIO: c_ulong = 0x8004667e;

pub const MAX_PATH: usize = 260;

#[repr(C)]
#[derive(Copy)]
pub struct WIN32_FIND_DATAW {
Expand Down Expand Up @@ -484,6 +486,12 @@ pub struct SYMBOLIC_LINK_REPARSE_BUFFER {
pub PathBuffer: WCHAR,
}

#[repr(C)]
pub struct FILE_NAME_INFO {
pub FileNameLength: DWORD,
pub FileName: [WCHAR; 1],
}

#[repr(C)]
pub struct MOUNT_POINT_REPARSE_BUFFER {
pub SubstituteNameOffset: c_ushort,
Expand Down
121 changes: 121 additions & 0 deletions library/std/src/sys/windows/io.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
use crate::io;
use crate::marker::PhantomData;
use crate::slice;
use crate::sys;
use crate::sys::c;
use core;
use libc;

#[derive(Copy, Clone)]
#[repr(transparent)]
Expand Down Expand Up @@ -78,3 +82,120 @@ impl<'a> IoSliceMut<'a> {
unsafe { slice::from_raw_parts_mut(self.vec.buf as *mut u8, self.vec.len as usize) }
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let fd = c::STD_INPUT_HANDLE;
let others = [c::STD_ERROR_HANDLE, c::STD_OUTPUT_HANDLE];

if unsafe { console_on_any(&[fd]) } {
// False positives aren't possible. If we got a console then
// we definitely have a tty on stdin.
return true;
}

// At this point, we *could* have a false negative. We can determine that
// this is true negative if we can detect the presence of a console on
// any of the other streams. If another stream has a console, then we know
// we're in a Windows console and can therefore trust the negative.
if unsafe { console_on_any(&others) } {
return false;
}

// Otherwise, we fall back to a very strange msys hack to see if we can
// sneakily detect the presence of a tty.
unsafe { msys_tty_on(fd) }
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stdout {
fn is_terminal() -> bool {
let fd = c::STD_OUTPUT_HANDLE;
let others = [c::STD_INPUT_HANDLE, c::STD_ERROR_HANDLE];

if unsafe { console_on_any(&[fd]) } {
// False positives aren't possible. If we got a console then
// we definitely have a tty on stdout.
return true;
}

// At this point, we *could* have a false negative. We can determine that
// this is true negative if we can detect the presence of a console on
// any of the other streams. If another stream has a console, then we know
// we're in a Windows console and can therefore trust the negative.
if unsafe { console_on_any(&others) } {
return false;
}

// Otherwise, we fall back to a very strange msys hack to see if we can
// sneakily detect the presence of a tty.
unsafe { msys_tty_on(fd) }
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
impl io::IsTerminal for sys::stdio::Stderr {
fn is_terminal() -> bool {
let fd = c::STD_ERROR_HANDLE;
let others = [c::STD_INPUT_HANDLE, c::STD_OUTPUT_HANDLE];

if unsafe { console_on_any(&[fd]) } {
// False positives aren't possible. If we got a console then
// we definitely have a tty on stderr.
return true;
}

// At this point, we *could* have a false negative. We can determine that
// this is true negative if we can detect the presence of a console on
// any of the other streams. If another stream has a console, then we know
// we're in a Windows console and can therefore trust the negative.
if unsafe { console_on_any(&others) } {
return false;
}

// Otherwise, we fall back to a very strange msys hack to see if we can
// sneakily detect the presence of a tty.
unsafe { msys_tty_on(fd) }
}
}

#[unstable(feature = "is_terminal", issue = "80937")]
unsafe fn console_on_any(fds: &[c::DWORD]) -> bool {
for &fd in fds {
let mut out = 0;
let handle = c::GetStdHandle(fd);
if c::GetConsoleMode(handle, &mut out) != 0 {
return true;
}
}
false
}
#[unstable(feature = "is_terminal", issue = "80937")]
unsafe fn msys_tty_on(fd: c::DWORD) -> bool {
let size = core::mem::size_of::<c::FILE_NAME_INFO>();
let mut name_info_bytes = vec![0u8; size + c::MAX_PATH * core::mem::size_of::<c::WCHAR>()];
let res = c::GetFileInformationByHandleEx(
c::GetStdHandle(fd),
c::FileNameInfo,
&mut *name_info_bytes as *mut _ as *mut libc::c_void,
name_info_bytes.len() as u32,
);
if res == 0 {
return false;
}
let name_info: &c::FILE_NAME_INFO = &*(name_info_bytes.as_ptr() as *const c::FILE_NAME_INFO);
let s = core::slice::from_raw_parts(
name_info.FileName.as_ptr(),
name_info.FileNameLength as usize / 2,
);
let name = String::from_utf16_lossy(s);
// This checks whether 'pty' exists in the file name, which indicates that
// a pseudo-terminal is attached. To mitigate against false positives
// (e.g., an actual file name that contains 'pty'), we also require that
// either the strings 'msys-' or 'cygwin-' are in the file name as well.)
let is_msys = name.contains("msys-") || name.contains("cygwin-");
let is_pty = name.contains("-pty");
is_msys && is_pty
}
4 changes: 2 additions & 2 deletions library/test/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
use std::env;
use std::path::PathBuf;

use super::helpers::isatty;
use super::options::{ColorConfig, Options, OutputFormat, RunIgnored};
use super::time::TestTimeOptions;
use std::io::{IsTerminal, Stdout};

#[derive(Debug)]
pub struct TestOpts {
Expand All @@ -32,7 +32,7 @@ pub struct TestOpts {
impl TestOpts {
pub fn use_color(&self) -> bool {
match self.color {
ColorConfig::AutoColor => !self.nocapture && isatty::stdout_isatty(),
ColorConfig::AutoColor => !self.nocapture && Stdout::is_terminal(),
ColorConfig::AlwaysColor => true,
ColorConfig::NeverColor => false,
}
Expand Down
32 changes: 0 additions & 32 deletions library/test/src/helpers/isatty.rs

This file was deleted.

1 change: 0 additions & 1 deletion library/test/src/helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,5 @@

pub mod concurrency;
pub mod exit_code;
pub mod isatty;
pub mod metrics;
pub mod shuffle;
8 changes: 7 additions & 1 deletion library/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#![feature(nll)]
#![feature(bench_black_box)]
#![feature(internal_output_capture)]
#![feature(is_terminal)]
#![feature(panic_unwind)]
#![feature(staged_api)]
#![feature(termination_trait_lib)]
#![feature(process_exitcode_placeholder)]
Expand Down Expand Up @@ -292,7 +294,11 @@ where
fn calc_timeout(timeout_queue: &VecDeque<TimeoutEntry>) -> Option<Duration> {
timeout_queue.front().map(|&TimeoutEntry { timeout: next_timeout, .. }| {
let now = Instant::now();
if next_timeout >= now { next_timeout - now } else { Duration::new(0, 0) }
if next_timeout >= now {
next_timeout - now
} else {
Duration::new(0, 0)
}
})
}

Expand Down