From 678b0c2d393733ec33574db94e21d3eb4bdf2396 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Feb 2025 14:28:45 +0000 Subject: [PATCH] [red-knot] Resolve `Options` to `Settings` (#16000) ## Summary This PR generalize the idea that we may want to emit diagnostics for invalid or incompatible configuration values similar to how we already do it for `rules`. This PR introduces a new `Settings` struct that is similar to `Options` but, unlike `Options`, are fields have their default values filled in and they use a representation optimized for reads. The diagnostics created during loading the `Settings` are stored on the `Project` so that we can emit them when calling `check`. The motivation for this work is that it simplifies adding new settings. That's also why I went ahead and added the `terminal.error-on-warning` setting to demonstrate how new settings are added. ## Test Plan Existing tests, new CLI test. --- crates/red_knot/src/args.rs | 9 ++-- crates/red_knot/src/main.rs | 29 ++++------ crates/red_knot/tests/cli.rs | 31 +++++++++++ crates/red_knot_project/src/db.rs | 10 ++-- crates/red_knot_project/src/lib.rs | 54 ++++++++++++------- crates/red_knot_project/src/metadata.rs | 1 + .../red_knot_project/src/metadata/options.rs | 32 ++++++++++- .../red_knot_project/src/metadata/settings.rs | 53 ++++++++++++++++++ crates/red_knot_python_semantic/src/db.rs | 8 +-- crates/red_knot_test/src/db.rs | 10 ++-- crates/ruff_graph/src/db.rs | 4 +- .../red_knot_check_invalid_syntax.rs | 12 ++--- knot.schema.json | 23 ++++++++ 13 files changed, 214 insertions(+), 62 deletions(-) create mode 100644 crates/red_knot_project/src/metadata/settings.rs diff --git a/crates/red_knot/src/args.rs b/crates/red_knot/src/args.rs index bd7d82e89d..33ec3595c4 100644 --- a/crates/red_knot/src/args.rs +++ b/crates/red_knot/src/args.rs @@ -1,7 +1,7 @@ use crate::logging::Verbosity; use crate::python_version::PythonVersion; use clap::{ArgAction, ArgMatches, Error, Parser}; -use red_knot_project::metadata::options::{EnvironmentOptions, Options}; +use red_knot_project::metadata::options::{EnvironmentOptions, Options, TerminalOptions}; use red_knot_project::metadata::value::{RangedValue, RelativePathBuf}; use red_knot_python_semantic::lint; use ruff_db::system::SystemPathBuf; @@ -67,8 +67,8 @@ pub(crate) struct CheckCommand { pub(crate) rules: RulesArg, /// Use exit code 1 if there are any warning-level diagnostics. - #[arg(long, conflicts_with = "exit_zero")] - pub(crate) error_on_warning: bool, + #[arg(long, conflicts_with = "exit_zero", default_missing_value = "true", num_args=0..1)] + pub(crate) error_on_warning: Option, /// Always use exit code 0, even when there are error-level diagnostics. #[arg(long)] @@ -107,6 +107,9 @@ impl CheckCommand { }), ..EnvironmentOptions::default() }), + terminal: Some(TerminalOptions { + error_on_warning: self.error_on_warning, + }), rules, ..Default::default() } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index fca4f70c04..3c1751e540 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -11,8 +11,8 @@ use clap::Parser; use colored::Colorize; use crossbeam::channel as crossbeam_channel; use red_knot_project::metadata::options::Options; -use red_knot_project::watch; use red_knot_project::watch::ProjectWatcher; +use red_knot_project::{watch, Db}; use red_knot_project::{ProjectDatabase, ProjectMetadata}; use red_knot_server::run_server; use ruff_db::diagnostic::{Diagnostic, Severity}; @@ -97,11 +97,6 @@ fn run_check(args: CheckCommand) -> anyhow::Result { let system = OsSystem::new(cwd); let watch = args.watch; let exit_zero = args.exit_zero; - let min_error_severity = if args.error_on_warning { - Severity::Warning - } else { - Severity::Error - }; let cli_options = args.into_options(); let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; @@ -109,7 +104,7 @@ fn run_check(args: CheckCommand) -> anyhow::Result { let mut db = ProjectDatabase::new(workspace_metadata, system)?; - let (main_loop, main_loop_cancellation_token) = MainLoop::new(cli_options, min_error_severity); + let (main_loop, main_loop_cancellation_token) = MainLoop::new(cli_options); // Listen to Ctrl+C and abort the watch mode. let main_loop_cancellation_token = Mutex::new(Some(main_loop_cancellation_token)); @@ -167,18 +162,10 @@ struct MainLoop { watcher: Option, cli_options: Options, - - /// The minimum severity to consider an error when deciding the exit status. - /// - /// TODO(micha): Get from the terminal settings. - min_error_severity: Severity, } impl MainLoop { - fn new( - cli_options: Options, - min_error_severity: Severity, - ) -> (Self, MainLoopCancellationToken) { + fn new(cli_options: Options) -> (Self, MainLoopCancellationToken) { let (sender, receiver) = crossbeam_channel::bounded(10); ( @@ -187,7 +174,6 @@ impl MainLoop { receiver, watcher: None, cli_options, - min_error_severity, }, MainLoopCancellationToken { sender }, ) @@ -245,9 +231,16 @@ impl MainLoop { result, revision: check_revision, } => { + let min_error_severity = + if db.project().settings(db).terminal().error_on_warning { + Severity::Warning + } else { + Severity::Error + }; + let failed = result .iter() - .any(|diagnostic| diagnostic.severity() >= self.min_error_severity); + .any(|diagnostic| diagnostic.severity() >= min_error_severity); if check_revision == revision { #[allow(clippy::print_stdout)] diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index 7f7e583a17..28360dfc87 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -575,6 +575,37 @@ fn exit_code_no_errors_but_error_on_warning_is_true() -> anyhow::Result<()> { Ok(()) } +#[test] +fn exit_code_no_errors_but_error_on_warning_is_enabled_in_configuration() -> anyhow::Result<()> { + let case = TestCase::with_files([ + ("test.py", r"print(x) # [unresolved-reference]"), + ( + "knot.toml", + r#" + [terminal] + error-on-warning = true + "#, + ), + ])?; + + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:1:7 + | + 1 | print(x) # [unresolved-reference] + | - Name `x` used when not defined + | + + + ----- stderr ----- + "###); + + Ok(()) +} + #[test] fn exit_code_both_warnings_and_errors() -> anyhow::Result<()> { let case = TestCase::with_file( diff --git a/crates/red_knot_project/src/db.rs b/crates/red_knot_project/src/db.rs index af491c5c3d..96ff04301c 100644 --- a/crates/red_knot_project/src/db.rs +++ b/crates/red_knot_project/src/db.rs @@ -114,8 +114,8 @@ impl SemanticDb for ProjectDatabase { project.is_file_open(self, file) } - fn rule_selection(&self) -> &RuleSelection { - self.project().rule_selection(self) + fn rule_selection(&self) -> Arc { + self.project().rules(self) } fn lint_registry(&self) -> &LintRegistry { @@ -186,7 +186,6 @@ pub(crate) mod tests { files: Files, system: TestSystem, vendored: VendoredFileSystem, - rule_selection: RuleSelection, project: Option, } @@ -198,7 +197,6 @@ pub(crate) mod tests { vendored: red_knot_vendored::file_system().clone(), files: Files::default(), events: Arc::default(), - rule_selection: RuleSelection::from_registry(&DEFAULT_LINT_REGISTRY), project: None, }; @@ -270,8 +268,8 @@ pub(crate) mod tests { !file.path(self).is_vendored_path() } - fn rule_selection(&self) -> &RuleSelection { - &self.rule_selection + fn rule_selection(&self) -> Arc { + self.project().rules(self) } fn lint_registry(&self) -> &LintRegistry { diff --git a/crates/red_knot_project/src/lib.rs b/crates/red_knot_project/src/lib.rs index 2967f63500..3d8ee4ae4c 100644 --- a/crates/red_knot_project/src/lib.rs +++ b/crates/red_knot_project/src/lib.rs @@ -3,6 +3,7 @@ use crate::metadata::options::OptionDiagnostic; pub use db::{Db, ProjectDatabase}; use files::{Index, Indexed, IndexedFiles}; +use metadata::settings::Settings; pub use metadata::{ProjectDiscoveryError, ProjectMetadata}; use red_knot_python_semantic::lint::{LintRegistry, LintRegistryBuilder, RuleSelection}; use red_knot_python_semantic::register_lints; @@ -66,12 +67,22 @@ pub struct Project { /// The metadata describing the project, including the unresolved options. #[return_ref] pub metadata: ProjectMetadata, + + /// The resolved project settings. + #[return_ref] + pub settings: Settings, + + /// Diagnostics that were generated when resolving the project settings. + #[return_ref] + settings_diagnostics: Vec, } #[salsa::tracked] impl Project { pub fn from_metadata(db: &dyn Db, metadata: ProjectMetadata) -> Self { - Project::builder(metadata) + let (settings, settings_diagnostics) = metadata.options().to_settings(db); + + Project::builder(metadata, settings, settings_diagnostics) .durability(Durability::MEDIUM) .open_fileset_durability(Durability::LOW) .file_set_durability(Durability::LOW) @@ -86,30 +97,37 @@ impl Project { self.metadata(db).name() } + /// Returns the resolved linter rules for the project. + /// + /// This is a salsa query to prevent re-computing queries if other, unrelated + /// settings change. For example, we don't want that changing the terminal settings + /// invalidates any type checking queries. + #[salsa::tracked] + pub fn rules(self, db: &dyn Db) -> Arc { + self.settings(db).to_rules() + } + pub fn reload(self, db: &mut dyn Db, metadata: ProjectMetadata) { tracing::debug!("Reloading project"); assert_eq!(self.root(db), metadata.root()); if &metadata != self.metadata(db) { + let (settings, settings_diagnostics) = metadata.options().to_settings(db); + + if self.settings(db) != &settings { + self.set_settings(db).to(settings); + } + + if self.settings_diagnostics(db) != &settings_diagnostics { + self.set_settings_diagnostics(db).to(settings_diagnostics); + } + self.set_metadata(db).to(metadata); } self.reload_files(db); } - pub fn rule_selection(self, db: &dyn Db) -> &RuleSelection { - let (selection, _) = self.rule_selection_with_diagnostics(db); - selection - } - - #[salsa::tracked(return_ref)] - fn rule_selection_with_diagnostics( - self, - db: &dyn Db, - ) -> (RuleSelection, Vec) { - self.metadata(db).options().to_rule_selection(db) - } - /// Checks all open files in the project and its dependencies. pub(crate) fn check(self, db: &ProjectDatabase) -> Vec> { let project_span = tracing::debug_span!("Project::check"); @@ -118,8 +136,7 @@ impl Project { tracing::debug!("Checking project '{name}'", name = self.name(db)); let mut diagnostics: Vec> = Vec::new(); - let (_, options_diagnostics) = self.rule_selection_with_diagnostics(db); - diagnostics.extend(options_diagnostics.iter().map(|diagnostic| { + diagnostics.extend(self.settings_diagnostics(db).iter().map(|diagnostic| { let diagnostic: Box = Box::new(diagnostic.clone()); diagnostic })); @@ -151,9 +168,8 @@ impl Project { } pub(crate) fn check_file(self, db: &dyn Db, file: File) -> Vec> { - let (_, options_diagnostics) = self.rule_selection_with_diagnostics(db); - - let mut file_diagnostics: Vec<_> = options_diagnostics + let mut file_diagnostics: Vec<_> = self + .settings_diagnostics(db) .iter() .map(|diagnostic| { let diagnostic: Box = Box::new(diagnostic.clone()); diff --git a/crates/red_knot_project/src/metadata.rs b/crates/red_knot_project/src/metadata.rs index 6fcd5f7a02..c7f2009c0e 100644 --- a/crates/red_knot_project/src/metadata.rs +++ b/crates/red_knot_project/src/metadata.rs @@ -12,6 +12,7 @@ use options::Options; pub mod options; pub mod pyproject; +pub mod settings; pub mod value; #[derive(Debug, PartialEq, Eq)] diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index 2188281221..401dcf1233 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -15,6 +15,8 @@ use std::borrow::Cow; use std::fmt::Debug; use thiserror::Error; +use super::settings::{Settings, TerminalSettings}; + /// The options for the project. #[derive(Debug, Default, Clone, PartialEq, Eq, Combine, Serialize, Deserialize)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] @@ -30,6 +32,9 @@ pub struct Options { /// Configures the enabled lints and their severity. #[serde(skip_serializing_if = "Option::is_none")] pub rules: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub terminal: Option, } impl Options { @@ -110,7 +115,22 @@ impl Options { } #[must_use] - pub(crate) fn to_rule_selection(&self, db: &dyn Db) -> (RuleSelection, Vec) { + pub(crate) fn to_settings(&self, db: &dyn Db) -> (Settings, Vec) { + let (rules, diagnostics) = self.to_rule_selection(db); + + let mut settings = Settings::new(rules); + + if let Some(terminal) = self.terminal.as_ref() { + settings.set_terminal(TerminalSettings { + error_on_warning: terminal.error_on_warning.unwrap_or_default(), + }); + } + + (settings, diagnostics) + } + + #[must_use] + fn to_rule_selection(&self, db: &dyn Db) -> (RuleSelection, Vec) { let registry = db.lint_registry(); let mut diagnostics = Vec::new(); @@ -244,6 +264,16 @@ impl FromIterator<(RangedValue, RangedValue)> for Rules { } } +#[derive(Debug, Default, Clone, Eq, PartialEq, Combine, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case", deny_unknown_fields)] +#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] +pub struct TerminalOptions { + /// Use exit code 1 if there are any warning-level diagnostics. + /// + /// Defaults to `false`. + pub error_on_warning: Option, +} + #[cfg(feature = "schemars")] mod schema { use crate::DEFAULT_LINT_REGISTRY; diff --git a/crates/red_knot_project/src/metadata/settings.rs b/crates/red_knot_project/src/metadata/settings.rs new file mode 100644 index 0000000000..c29d23ed61 --- /dev/null +++ b/crates/red_knot_project/src/metadata/settings.rs @@ -0,0 +1,53 @@ +use std::sync::Arc; + +use red_knot_python_semantic::lint::RuleSelection; + +/// The resolved [`super::Options`] for the project. +/// +/// Unlike [`super::Options`], the struct has default values filled in and +/// uses representations that are optimized for reads (instead of preserving the source representation). +/// It's also not required that this structure precisely resembles the TOML schema, although +/// it's encouraged to use a similar structure. +/// +/// It's worth considering to adding a salsa query for specific settings to +/// limit the blast radius when only some settings change. For example, +/// changing the terminal settings shouldn't invalidate any core type-checking queries. +/// This can be achieved by adding a salsa query for the type checking specific settings. +/// +/// Settings that are part of [`red_knot_python_semantic::ProgramSettings`] are not included here. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Settings { + rules: Arc, + + terminal: TerminalSettings, +} + +impl Settings { + pub fn new(rules: RuleSelection) -> Self { + Self { + rules: Arc::new(rules), + terminal: TerminalSettings::default(), + } + } + + pub fn rules(&self) -> &RuleSelection { + &self.rules + } + + pub fn to_rules(&self) -> Arc { + self.rules.clone() + } + + pub fn terminal(&self) -> &TerminalSettings { + &self.terminal + } + + pub fn set_terminal(&mut self, terminal: TerminalSettings) { + self.terminal = terminal; + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct TerminalSettings { + pub error_on_warning: bool, +} diff --git a/crates/red_knot_python_semantic/src/db.rs b/crates/red_knot_python_semantic/src/db.rs index be117fd0f4..8b94da20ff 100644 --- a/crates/red_knot_python_semantic/src/db.rs +++ b/crates/red_knot_python_semantic/src/db.rs @@ -1,3 +1,5 @@ +use std::sync::Arc; + use crate::lint::{LintRegistry, RuleSelection}; use ruff_db::files::File; use ruff_db::{Db as SourceDb, Upcast}; @@ -7,7 +9,7 @@ use ruff_db::{Db as SourceDb, Upcast}; pub trait Db: SourceDb + Upcast { fn is_file_open(&self, file: File) -> bool; - fn rule_selection(&self) -> &RuleSelection; + fn rule_selection(&self) -> Arc; fn lint_registry(&self) -> &LintRegistry; } @@ -111,8 +113,8 @@ pub(crate) mod tests { !file.path(self).is_vendored_path() } - fn rule_selection(&self) -> &RuleSelection { - &self.rule_selection + fn rule_selection(&self) -> Arc { + self.rule_selection.clone() } fn lint_registry(&self) -> &LintRegistry { diff --git a/crates/red_knot_test/src/db.rs b/crates/red_knot_test/src/db.rs index 1c7a9e84dd..12b34df4fa 100644 --- a/crates/red_knot_test/src/db.rs +++ b/crates/red_knot_test/src/db.rs @@ -1,3 +1,5 @@ +use std::sync::Arc; + use red_knot_python_semantic::lint::{LintRegistry, RuleSelection}; use red_knot_python_semantic::{ default_lint_registry, Db as SemanticDb, Program, ProgramSettings, PythonPlatform, @@ -16,7 +18,7 @@ pub(crate) struct Db { files: Files, system: TestSystem, vendored: VendoredFileSystem, - rule_selection: RuleSelection, + rule_selection: Arc, } impl Db { @@ -29,7 +31,7 @@ impl Db { system: TestSystem::default(), vendored: red_knot_vendored::file_system().clone(), files: Files::default(), - rule_selection, + rule_selection: Arc::new(rule_selection), }; db.memory_file_system() @@ -94,8 +96,8 @@ impl SemanticDb for Db { !file.path(self).is_vendored_path() } - fn rule_selection(&self) -> &RuleSelection { - &self.rule_selection + fn rule_selection(&self) -> Arc { + self.rule_selection.clone() } fn lint_registry(&self) -> &LintRegistry { diff --git a/crates/ruff_graph/src/db.rs b/crates/ruff_graph/src/db.rs index e08bfb5a6a..ac51abc8b2 100644 --- a/crates/ruff_graph/src/db.rs +++ b/crates/ruff_graph/src/db.rs @@ -79,8 +79,8 @@ impl Db for ModuleDb { !file.path(self).is_vendored_path() } - fn rule_selection(&self) -> &RuleSelection { - &self.rule_selection + fn rule_selection(&self) -> Arc { + self.rule_selection.clone() } fn lint_registry(&self) -> &LintRegistry { diff --git a/fuzz/fuzz_targets/red_knot_check_invalid_syntax.rs b/fuzz/fuzz_targets/red_knot_check_invalid_syntax.rs index efadfb7ffa..758746a5e0 100644 --- a/fuzz/fuzz_targets/red_knot_check_invalid_syntax.rs +++ b/fuzz/fuzz_targets/red_knot_check_invalid_syntax.rs @@ -3,7 +3,7 @@ #![no_main] -use std::sync::{Mutex, OnceLock}; +use std::sync::{Arc, Mutex, OnceLock}; use libfuzzer_sys::{fuzz_target, Corpus}; @@ -29,8 +29,8 @@ struct TestDb { files: Files, system: TestSystem, vendored: VendoredFileSystem, - events: std::sync::Arc>>, - rule_selection: std::sync::Arc, + events: Arc>>, + rule_selection: Arc, } impl TestDb { @@ -39,7 +39,7 @@ impl TestDb { storage: salsa::Storage::default(), system: TestSystem::default(), vendored: red_knot_vendored::file_system().clone(), - events: std::sync::Arc::default(), + events: Arc::default(), files: Files::default(), rule_selection: RuleSelection::from_registry(default_lint_registry()).into(), } @@ -86,8 +86,8 @@ impl SemanticDb for TestDb { !file.path(self).is_vendored_path() } - fn rule_selection(&self) -> &RuleSelection { - &self.rule_selection + fn rule_selection(&self) -> Arc { + self.rule_selection.clone() } fn lint_registry(&self) -> &LintRegistry { diff --git a/knot.schema.json b/knot.schema.json index f961e34643..08e2c57a18 100644 --- a/knot.schema.json +++ b/knot.schema.json @@ -35,6 +35,16 @@ "type": "null" } ] + }, + "terminal": { + "anyOf": [ + { + "$ref": "#/definitions/TerminalOptions" + }, + { + "type": "null" + } + ] } }, "additionalProperties": false, @@ -688,6 +698,19 @@ } }, "additionalProperties": false + }, + "TerminalOptions": { + "type": "object", + "properties": { + "error-on-warning": { + "description": "Use exit code 1 if there are any warning-level diagnostics.\n\nDefaults to `false`.", + "type": [ + "boolean", + "null" + ] + } + }, + "additionalProperties": false } } } \ No newline at end of file