Decide on color handling up front

CHANGELOG.md

@@ -32,6 +32,8 @@
 
     - `x$1` where the right-hand side is not an identifier
 
+- Air no longer emits colored output when stderr is not a terminal, such as when stderr is redirected to a file.
+
 # 0.12.0
 
 - New `--no-configuration` flag to format with Air's built-in defaults without loading a project or user configuration file (#479, @LeonidasZhak).

crates/air/Cargo.toml

@@ -24,7 +24,7 @@ lsp = { workspace = true }
 thiserror = { workspace = true }
 tokio = { workspace = true }
 tracing = { workspace = true }
-tracing-subscriber = { workspace = true }
+tracing-subscriber = { workspace = true, features = ["ansi"] }
 workspace = { workspace = true }
 
 [dev-dependencies]

crates/air/src/color.rs

@@ -0,0 +1,54 @@
+use std::ffi::OsStr;
+use std::io::IsTerminal;
+
+/// Decides whether the CLI colors its output
+///
+/// We use colored output in two places:
+/// - Level labels in `tracing-subscriber`, like `ERROR` , which shows up in red
+/// - Usage of the `colored` crate
+///
+/// Both respect the `NO_COLOR` environment variable if we don't specify
+/// anything, but to avoid potential differences between the two crates we do a
+/// one time resolution of user overrides and `NO_COLOR` ourselves, then
+/// forcibly set the option for both `colored` and `tracing-subscriber`.
+pub(crate) fn init_color(no_color: bool) -> bool {
+    let color =
+        // User override via `--no-color`
+        !no_color
+        // User override via `NO_COLOR` env var
+        && !no_color_env_var_is_set()
+        // Only emit colors if the target is a terminal. We only emit colored
+        // output to stderr, so we don't need to check stdout.
+        && std::io::stderr().is_terminal();
+
+    // Apply our decision globally to `colored`
+    colored::control::set_override(color);
+
+    // Return our decision so we can apply it to `tracing-subscriber`
+    color
+}
+
+/// Is `NO_COLOR` set to a non-empty, non-zero value?
+fn no_color_env_var_is_set() -> bool {
+    env_var_is_set(std::env::var_os("NO_COLOR").as_deref())
+}
+
+fn env_var_is_set(value: Option<&OsStr>) -> bool {
+    value.is_some_and(|value| !value.is_empty() && value != "0")
+}
+
+#[cfg(test)]
+mod test {
+    use std::ffi::OsStr;
+
+    use crate::color::env_var_is_set;
+
+    #[test]
+    fn test_env_var_is_set() {
+        assert!(!env_var_is_set(None));
+        assert!(!env_var_is_set(Some(OsStr::new(""))));
+        assert!(!env_var_is_set(Some(OsStr::new("0"))));
+        assert!(env_var_is_set(Some(OsStr::new("1"))));
+        assert!(env_var_is_set(Some(OsStr::new("true"))));
+    }
+}

crates/air/src/lib.rs

@@ -3,17 +3,16 @@ use crate::args::Command;
 use crate::status::ExitStatus;
 
 pub mod args;
+mod color;
 mod commands;
 mod logging;
 pub mod status;
 
 pub fn run(args: Args) -> anyhow::Result<ExitStatus> {
     if !matches!(args.command, Command::LanguageServer(_)) {
         // The language server sets up its own logging
-        logging::init_logging(
-            args.global_options.log_level.unwrap_or_default(),
-            args.global_options.no_color,
-        );
+        let color = color::init_color(args.global_options.no_color);
+        logging::init_logging(args.global_options.log_level.unwrap_or_default(), color);
     }
 
     match args.command {

crates/air/src/logging.rs

@@ -1,11 +1,10 @@
 use std::fmt::Display;
 use std::str::FromStr;
 use tracing_subscriber::Layer;
-use tracing_subscriber::Registry;
 use tracing_subscriber::filter;
 use tracing_subscriber::layer::SubscriberExt;
 
-pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) {
+pub(crate) fn init_logging(log_level: LogLevel, color: bool) {
     let log_level = log_level.tracing_level();
 
     // Apply the log level to each air crate.
@@ -15,13 +14,9 @@ pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) {
         filter = filter.with_target(*target, log_level);
     }
 
-    let mut layer = tracing_subscriber::fmt::layer();
-
-    if no_color {
-        layer = turn_off_colors(layer);
-    };
-
-    let layer = layer
+    let layer = tracing_subscriber::fmt::layer()
+        // Apply coloring decision
+        .with_ansi(color)
         // i.e. Displaying `ERROR` or `WARN`
         .with_level(true)
         // Don't show the module name, not useful in a cli
@@ -40,24 +35,6 @@ pub(crate) fn init_logging(log_level: LogLevel, no_color: bool) {
     tracing::trace!("Initialized logging");
 }
 
-/// Explicitly turn off ANSI colored / styled output
-///
-/// We use colored output in two places:
-/// - Level labels in tracing-subscriber, like `ERROR` , which shows up in red
-/// - Usage of the `colored` crate
-///
-/// Both respect the `NO_COLOR` environment variable if we don't specify anything.
-///
-/// We explicitly call `set_override()` and `with_ansi()` ONLY when turning colors off.
-/// `set_override(true)` and `with_ansi(true)` override the `NO_COLOR` environment
-/// variable, and we don't want to do that.
-fn turn_off_colors(
-    layer: tracing_subscriber::fmt::Layer<Registry>,
-) -> tracing_subscriber::fmt::Layer<Registry> {
-    colored::control::set_override(false);
-    layer.with_ansi(false)
-}
-
 #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, clap::ValueEnum)]
 pub(crate) enum LogLevel {
     Error,

crates/air/tests/integration/format.rs

@@ -347,6 +347,32 @@ fn test_check_output_format() {
     );
 }
 
+#[test]
+fn test_no_color_when_stderr_is_not_a_terminal() -> anyhow::Result<()> {
+    let directory = TempDir::new()?;
+    let directory = directory.path();
+
+    std::fs::write(directory.join("needs-formatting.R"), "1+1\n")?;
+    std::fs::write(directory.join("parse-error.R"), "1+/1\n")?;
+
+    // `run()` captures stderr through pipes so `is_terminal()` is `false`, just
+    // like a user doing `air format R/ 2>errors.log` to pipe stderr to a log
+    // file
+    let output = Command::new(binary_path())
+        .current_dir(directory)
+        // Defensive to be reproducible on all systems, just in case
+        .env_remove("NO_COLOR")
+        .arg("format")
+        .arg(".")
+        .arg("--check")
+        .run();
+
+    assert!(!output.stderr.contains('\x1b'));
+    insta::assert_snapshot!(output);
+
+    Ok(())
+}
+
 #[test]
 fn test_stdin_cant_supply_paths() -> anyhow::Result<()> {
     let directory = TempDir::new()?;

crates/air/tests/integration/snapshots/integration__format__no_color_when_stderr_is_not_a_terminal.snap

@@ -0,0 +1,14 @@
+---
+source: crates/air/tests/integration/format.rs
+expression: output
+---
+success: false
+exit_code: 255
+----- stdout -----
+
+----- stderr -----
+ERROR Failed to format parse-error.R: Failed to parse due to syntax errors.
+Would reformat: needs-formatting.R
+
+----- args -----
+format . --check