ruff_db: add a vector for configuring diagnostic output (#16118)

For now, the only thing one can configure is whether color is enabled or
not. This avoids needing to ask the `colored` crate whether colors have
been globally enabled or disabled. And, more crucially, avoids the need
to _set_ this global flag for testing diagnostic output. Doing so can
have unintended consequences, as outlined in #16115.

Fixes #16115
This commit is contained in:
Andrew Gallant 2025-02-12 09:38:05 -05:00 committed by GitHub
parent 03f08283ad
commit a9671e7008
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 38 additions and 21 deletions

1
Cargo.lock generated
View File

@ -2755,7 +2755,6 @@ name = "ruff_db"
version = "0.0.0" version = "0.0.0"
dependencies = [ dependencies = [
"camino", "camino",
"colored 3.0.0",
"countme", "countme",
"dashmap 6.1.0", "dashmap 6.1.0",
"dunce", "dunce",

View File

@ -15,7 +15,7 @@ use red_knot_project::watch::ProjectWatcher;
use red_knot_project::{watch, Db}; use red_knot_project::{watch, Db};
use red_knot_project::{ProjectDatabase, ProjectMetadata}; use red_knot_project::{ProjectDatabase, ProjectMetadata};
use red_knot_server::run_server; use red_knot_server::run_server;
use ruff_db::diagnostic::{Diagnostic, Severity}; use ruff_db::diagnostic::{Diagnostic, DisplayDiagnosticConfig, Severity};
use ruff_db::system::{OsSystem, System, SystemPath, SystemPathBuf}; use ruff_db::system::{OsSystem, System, SystemPath, SystemPathBuf};
use salsa::plumbing::ZalsaDatabase; use salsa::plumbing::ZalsaDatabase;
@ -231,6 +231,9 @@ impl MainLoop {
result, result,
revision: check_revision, revision: check_revision,
} => { } => {
let display_config = DisplayDiagnosticConfig::default()
.color(colored::control::SHOULD_COLORIZE.should_colorize());
let min_error_severity = let min_error_severity =
if db.project().settings(db).terminal().error_on_warning { if db.project().settings(db).terminal().error_on_warning {
Severity::Warning Severity::Warning
@ -245,7 +248,7 @@ impl MainLoop {
if check_revision == revision { if check_revision == revision {
#[allow(clippy::print_stdout)] #[allow(clippy::print_stdout)]
for diagnostic in result { for diagnostic in result {
println!("{}", diagnostic.display(db)); println!("{}", diagnostic.display(db, &display_config));
} }
} else { } else {
tracing::debug!( tracing::debug!(

View File

@ -5,7 +5,7 @@ use colored::Colorize;
use parser as test_parser; use parser as test_parser;
use red_knot_python_semantic::types::check_types; use red_knot_python_semantic::types::check_types;
use red_knot_python_semantic::{Program, ProgramSettings, SearchPathSettings, SitePackages}; use red_knot_python_semantic::{Program, ProgramSettings, SearchPathSettings, SitePackages};
use ruff_db::diagnostic::{Diagnostic, ParseDiagnostic}; use ruff_db::diagnostic::{Diagnostic, DisplayDiagnosticConfig, ParseDiagnostic};
use ruff_db::files::{system_path_to_file, File, Files}; use ruff_db::files::{system_path_to_file, File, Files};
use ruff_db::panic::catch_unwind; use ruff_db::panic::catch_unwind;
use ruff_db::parsed::parsed_module; use ruff_db::parsed::parsed_module;
@ -300,9 +300,7 @@ fn create_diagnostic_snapshot<D: Diagnostic>(
test: &parser::MarkdownTest, test: &parser::MarkdownTest,
diagnostics: impl IntoIterator<Item = D>, diagnostics: impl IntoIterator<Item = D>,
) -> String { ) -> String {
// TODO(ag): Do something better than requiring this let display_config = DisplayDiagnosticConfig::default().color(false);
// global state to be twiddled everywhere.
colored::control::set_override(false);
let mut snapshot = String::new(); let mut snapshot = String::new();
writeln!(snapshot).unwrap(); writeln!(snapshot).unwrap();
@ -340,7 +338,7 @@ fn create_diagnostic_snapshot<D: Diagnostic>(
writeln!(snapshot).unwrap(); writeln!(snapshot).unwrap();
} }
writeln!(snapshot, "```").unwrap(); writeln!(snapshot, "```").unwrap();
writeln!(snapshot, "{}", diag.display(db)).unwrap(); writeln!(snapshot, "{}", diag.display(db, &display_config)).unwrap();
writeln!(snapshot, "```").unwrap(); writeln!(snapshot, "```").unwrap();
} }
snapshot snapshot

View File

@ -7,7 +7,7 @@ use red_knot_project::metadata::options::{EnvironmentOptions, Options};
use red_knot_project::metadata::value::RangedValue; use red_knot_project::metadata::value::RangedValue;
use red_knot_project::ProjectMetadata; use red_knot_project::ProjectMetadata;
use red_knot_project::{Db, ProjectDatabase}; use red_knot_project::{Db, ProjectDatabase};
use ruff_db::diagnostic::Diagnostic; use ruff_db::diagnostic::{Diagnostic, DisplayDiagnosticConfig};
use ruff_db::files::{system_path_to_file, File}; use ruff_db::files::{system_path_to_file, File};
use ruff_db::system::walk_directory::WalkDirectoryBuilder; use ruff_db::system::walk_directory::WalkDirectoryBuilder;
use ruff_db::system::{ use ruff_db::system::{
@ -114,9 +114,10 @@ impl Workspace {
pub fn check_file(&self, file_id: &FileHandle) -> Result<Vec<String>, Error> { pub fn check_file(&self, file_id: &FileHandle) -> Result<Vec<String>, Error> {
let result = self.db.check_file(file_id.file).map_err(into_error)?; let result = self.db.check_file(file_id.file).map_err(into_error)?;
let display_config = DisplayDiagnosticConfig::default().color(false);
Ok(result Ok(result
.into_iter() .into_iter()
.map(|diagnostic| diagnostic.display(&self.db).to_string()) .map(|diagnostic| diagnostic.display(&self.db, &display_config).to_string())
.collect()) .collect())
} }
@ -124,9 +125,10 @@ impl Workspace {
pub fn check(&self) -> Result<Vec<String>, Error> { pub fn check(&self) -> Result<Vec<String>, Error> {
let result = self.db.check().map_err(into_error)?; let result = self.db.check().map_err(into_error)?;
let display_config = DisplayDiagnosticConfig::default().color(false);
Ok(result Ok(result
.into_iter() .into_iter()
.map(|diagnostic| diagnostic.display(&self.db).to_string()) .map(|diagnostic| diagnostic.display(&self.db, &display_config).to_string())
.collect()) .collect())
} }

View File

@ -21,7 +21,6 @@ ruff_source_file = { workspace = true }
ruff_text_size = { workspace = true } ruff_text_size = { workspace = true }
camino = { workspace = true } camino = { workspace = true }
colored = { workspace = true }
countme = { workspace = true } countme = { workspace = true }
dashmap = { workspace = true } dashmap = { workspace = true }
dunce = { workspace = true } dunce = { workspace = true }

View File

@ -172,13 +172,18 @@ pub trait Diagnostic: Send + Sync + std::fmt::Debug {
fn severity(&self) -> Severity; fn severity(&self) -> Severity;
fn display<'a>(&'a self, db: &'a dyn Db) -> DisplayDiagnostic<'a> fn display<'db, 'diag, 'config>(
&'diag self,
db: &'db dyn Db,
config: &'config DisplayDiagnosticConfig,
) -> DisplayDiagnostic<'db, 'diag, 'config>
where where
Self: Sized, Self: Sized,
{ {
DisplayDiagnostic { DisplayDiagnostic {
db, db,
diagnostic: self, diagnostic: self,
config,
} }
} }
} }
@ -232,18 +237,29 @@ pub enum Severity {
Fatal, Fatal,
} }
pub struct DisplayDiagnostic<'db> { /// Configuration for rendering diagnostics.
db: &'db dyn Db, #[derive(Clone, Debug, Default)]
diagnostic: &'db dyn Diagnostic, pub struct DisplayDiagnosticConfig {
/// Whether to enable colors or not.
///
/// Disabled by default.
color: bool,
} }
impl<'db> DisplayDiagnostic<'db> { impl DisplayDiagnosticConfig {
pub fn new(db: &'db dyn Db, diagnostic: &'db dyn Diagnostic) -> Self { /// Whether to enable colors or not.
Self { db, diagnostic } pub fn color(self, yes: bool) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig { color: yes }
} }
} }
impl std::fmt::Display for DisplayDiagnostic<'_> { pub struct DisplayDiagnostic<'db, 'diag, 'config> {
db: &'db dyn Db,
diagnostic: &'diag dyn Diagnostic,
config: &'config DisplayDiagnosticConfig,
}
impl std::fmt::Display for DisplayDiagnostic<'_, '_, '_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let level = match self.diagnostic.severity() { let level = match self.diagnostic.severity() {
Severity::Info => Level::Info, Severity::Info => Level::Info,
@ -260,7 +276,7 @@ impl std::fmt::Display for DisplayDiagnostic<'_> {
}; };
let render = |f: &mut std::fmt::Formatter, message| { let render = |f: &mut std::fmt::Formatter, message| {
let renderer = if !cfg!(test) && colored::control::SHOULD_COLORIZE.should_colorize() { let renderer = if self.config.color {
Renderer::styled() Renderer::styled()
} else { } else {
Renderer::plain() Renderer::plain()