[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

@@ -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<bool>,
 
     /// 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()
         }

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<ExitStatus> {
     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<ExitStatus> {
 
     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<ProjectWatcher>,
 
     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)]

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
+     --> <temp_dir>/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(

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<RuleSelection> {
+        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<Project>,
     }
 
@@ -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<RuleSelection> {
+            self.project().rules(self)
         }
 
         fn lint_registry(&self) -> &LintRegistry {

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<OptionDiagnostic>,
 }
 
 #[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<RuleSelection> {
+        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<OptionDiagnostic>) {
-        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<Box<dyn Diagnostic>> {
         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<Box<dyn Diagnostic>> = 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<dyn Diagnostic> = Box::new(diagnostic.clone());
             diagnostic
         }));
@@ -151,9 +168,8 @@ impl Project {
     }
 
     pub(crate) fn check_file(self, db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
-        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<dyn Diagnostic> = Box::new(diagnostic.clone());

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

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<Rules>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub terminal: Option<TerminalOptions>,
 }
 
 impl Options {
@@ -110,7 +115,22 @@ impl Options {
     }
 
     #[must_use]
-    pub(crate) fn to_rule_selection(&self, db: &dyn Db) -> (RuleSelection, Vec<OptionDiagnostic>) {
+    pub(crate) fn to_settings(&self, db: &dyn Db) -> (Settings, Vec<OptionDiagnostic>) {
+        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<OptionDiagnostic>) {
         let registry = db.lint_registry();
         let mut diagnostics = Vec::new();
 
@@ -244,6 +264,16 @@ impl FromIterator<(RangedValue<String>, RangedValue<Level>)> 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<bool>,
+}
+
 #[cfg(feature = "schemars")]
 mod schema {
     use crate::DEFAULT_LINT_REGISTRY;

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<RuleSelection>,
+
+    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<RuleSelection> {
+        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,
+}

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<dyn SourceDb> {
     fn is_file_open(&self, file: File) -> bool;
 
-    fn rule_selection(&self) -> &RuleSelection;
+    fn rule_selection(&self) -> Arc<RuleSelection>;
 
     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<RuleSelection> {
+            self.rule_selection.clone()
         }
 
         fn lint_registry(&self) -> &LintRegistry {

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<RuleSelection>,
 }
 
 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<RuleSelection> {
+        self.rule_selection.clone()
     }
 
     fn lint_registry(&self) -> &LintRegistry {

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<RuleSelection> {
+        self.rule_selection.clone()
     }
 
     fn lint_registry(&self) -> &LintRegistry {

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<Mutex<Vec<salsa::Event>>>,
-    rule_selection: std::sync::Arc<RuleSelection>,
+    events: Arc<Mutex<Vec<salsa::Event>>>,
+    rule_selection: Arc<RuleSelection>,
 }
 
 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<RuleSelection> {
+        self.rule_selection.clone()
     }
 
     fn lint_registry(&self) -> &LintRegistry {

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