From d7f6a123a45ef9544e82c578be920509b89910cf Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 26 Jan 2025 00:48:33 +0000 Subject: [PATCH 1/3] [red-knot] Support `--exit-zero` and `--error-on-warning` --- crates/red_knot/src/args.rs | 9 + crates/red_knot/src/main.rs | 41 +++- crates/red_knot/tests/cli.rs | 231 +++++++++++++++++- .../red_knot_project/src/metadata/options.rs | 3 + 4 files changed, 268 insertions(+), 16 deletions(-) diff --git a/crates/red_knot/src/args.rs b/crates/red_knot/src/args.rs index 9e8d0d66a5435..ad60966e466cf 100644 --- a/crates/red_knot/src/args.rs +++ b/crates/red_knot/src/args.rs @@ -63,6 +63,14 @@ pub(crate) struct CheckCommand { #[clap(flatten)] 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, + + /// Always use exit code 0, even when there are error-level diagnostics. + #[arg(long)] + pub(crate) exit_zero: bool, + /// Run in watch mode by re-running whenever files change. #[arg(long, short = 'W')] pub(crate) watch: bool, @@ -97,6 +105,7 @@ impl CheckCommand { ..EnvironmentOptions::default() }), rules, + error_on_warning: Some(self.error_on_warning), ..Default::default() } } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 3f717434f42fe..dfe2d0f71abd6 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -12,7 +12,7 @@ use red_knot_project::watch; use red_knot_project::watch::ProjectWatcher; use red_knot_project::{ProjectDatabase, ProjectMetadata}; use red_knot_server::run_server; -use ruff_db::diagnostic::Diagnostic; +use ruff_db::diagnostic::{Diagnostic, Severity}; use ruff_db::system::{OsSystem, System, SystemPath, SystemPathBuf}; use salsa::plumbing::ZalsaDatabase; @@ -84,6 +84,7 @@ fn run_check(args: CheckCommand) -> anyhow::Result { let system = OsSystem::new(cwd); let watch = args.watch; + let exit_zero = args.exit_zero; let cli_options = args.into_options(); let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; workspace_metadata.apply_cli_options(cli_options.clone()); @@ -112,7 +113,11 @@ fn run_check(args: CheckCommand) -> anyhow::Result { std::mem::forget(db); - Ok(exit_status) + if exit_zero { + Ok(ExitStatus::Success) + } else { + Ok(exit_status) + } } #[derive(Copy, Clone)] @@ -213,7 +218,18 @@ impl MainLoop { result, revision: check_revision, } => { - let has_diagnostics = !result.is_empty(); + let (has_warnings, has_errors) = result.iter().fold( + (false, false), + |(has_warnings, has_errors), diagnostic| { + let severity = diagnostic.severity(); + + ( + has_warnings || severity == Severity::Warning, + has_errors || severity >= Severity::Error, + ) + }, + ); + if check_revision == revision { #[allow(clippy::print_stdout)] for diagnostic in result { @@ -226,10 +242,21 @@ impl MainLoop { } if self.watcher.is_none() { - return if has_diagnostics { - ExitStatus::Failure - } else { - ExitStatus::Success + let error_on_warning = + self.cli_options.error_on_warning.unwrap_or_default(); + + return match (has_warnings, has_errors) { + (false, false) => ExitStatus::Success, + + (true, false) => { + if error_on_warning { + ExitStatus::Failure + } else { + ExitStatus::Success + } + } + + (_, true) => ExitStatus::Failure, }; } diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index 61a904d9f85fc..5454308565463 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -231,8 +231,8 @@ fn configuration_rule_severity() -> anyhow::Result<()> { )?; assert_cmd_snapshot!(case.command(), @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- warning: lint:division-by-zero --> /test.py:2:5 @@ -316,8 +316,8 @@ fn cli_rule_severity() -> anyhow::Result<()> { .arg("--warn") .arg("unresolved-import"), @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- warning: lint:unresolved-import --> /test.py:2:8 @@ -402,8 +402,8 @@ fn cli_rule_severity_precedence() -> anyhow::Result<()> { .arg("--ignore") .arg("possibly-unresolved-reference"), @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- warning: lint:division-by-zero --> /test.py:2:5 @@ -437,8 +437,8 @@ fn configuration_unknown_rules() -> anyhow::Result<()> { ])?; assert_cmd_snapshot!(case.command(), @r###" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- warning: unknown-rule --> /pyproject.toml:3:1 @@ -461,10 +461,223 @@ fn cli_unknown_rules() -> anyhow::Result<()> { let case = TestCase::with_file("test.py", "print(10)")?; assert_cmd_snapshot!(case.command().arg("--ignore").arg("division-by-zer"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + warning: unknown-rule: Unknown lint rule `division-by-zer` + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_only_warnings() -> anyhow::Result<()> { + let case = TestCase::with_file("test.py", r"print(x) # [unresolved-reference]")?; + + assert_cmd_snapshot!(case.command(), @r###" + success: true + exit_code: 0 + ----- 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_only_info() -> anyhow::Result<()> { + let case = TestCase::with_file( + "test.py", + r#" + from typing_extensions import reveal_type + reveal_type(1) + "#, + )?; + + assert_cmd_snapshot!(case.command(), @r###" + success: true + exit_code: 0 + ----- stdout ----- + info: revealed-type + --> /test.py:3:1 + | + 2 | from typing_extensions import reveal_type + 3 | reveal_type(1) + | -------------- info: Revealed type is `Literal[1]` + | + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_only_info_and_error_on_warning_is_true() -> anyhow::Result<()> { + let case = TestCase::with_file( + "test.py", + r#" + from typing_extensions import reveal_type + reveal_type(1) + "#, + )?; + + assert_cmd_snapshot!(case.command().arg("--error-on-warning"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + info: revealed-type + --> /test.py:3:1 + | + 2 | from typing_extensions import reveal_type + 3 | reveal_type(1) + | -------------- info: Revealed type is `Literal[1]` + | + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_no_errors_but_error_on_warning_is_true() -> anyhow::Result<()> { + let case = TestCase::with_file("test.py", r"print(x) # [unresolved-reference]")?; + + assert_cmd_snapshot!(case.command().arg("--error-on-warning"), @r###" success: false exit_code: 1 ----- stdout ----- - warning: unknown-rule: Unknown lint rule `division-by-zer` + 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( + "test.py", + r#" + print(x) # [unresolved-reference] + print(4[1]) # [non-subscriptable] + "#, + )?; + + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:2:7 + | + 2 | print(x) # [unresolved-reference] + | - Name `x` used when not defined + 3 | print(4[1]) # [non-subscriptable] + | + + error: lint:non-subscriptable + --> /test.py:3:7 + | + 2 | print(x) # [unresolved-reference] + 3 | print(4[1]) # [non-subscriptable] + | ^ Cannot subscript object of type `Literal[4]` with no `__getitem__` method + | + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_both_warnings_and_errors_and_error_on_warning_is_true() -> anyhow::Result<()> { + let case = TestCase::with_file( + "test.py", + r###" + print(x) # [unresolved-reference] + print(4[1]) # [non-subscriptable] + "###, + )?; + + assert_cmd_snapshot!(case.command().arg("--error-on-warning"), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:2:7 + | + 2 | print(x) # [unresolved-reference] + | - Name `x` used when not defined + 3 | print(4[1]) # [non-subscriptable] + | + + error: lint:non-subscriptable + --> /test.py:3:7 + | + 2 | print(x) # [unresolved-reference] + 3 | print(4[1]) # [non-subscriptable] + | ^ Cannot subscript object of type `Literal[4]` with no `__getitem__` method + | + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_exit_zero_is_true() -> anyhow::Result<()> { + let case = TestCase::with_file( + "test.py", + r#" + print(x) # [unresolved-reference] + print(4[1]) # [non-subscriptable] + "#, + )?; + + assert_cmd_snapshot!(case.command().arg("--exit-zero"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:2:7 + | + 2 | print(x) # [unresolved-reference] + | - Name `x` used when not defined + 3 | print(4[1]) # [non-subscriptable] + | + + error: lint:non-subscriptable + --> /test.py:3:7 + | + 2 | print(x) # [unresolved-reference] + 3 | print(4[1]) # [non-subscriptable] + | ^ Cannot subscript object of type `Literal[4]` with no `__getitem__` method + | ----- stderr ----- diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index 13c27837c75f6..a2da43ce80e11 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -27,6 +27,9 @@ pub struct Options { #[serde(skip_serializing_if = "Option::is_none")] pub rules: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub error_on_warning: Option, } impl Options { From 010d6e8e646d189017020a0a33576d6d9e949ced Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 2 Feb 2025 20:46:02 +0000 Subject: [PATCH 2/3] Per review --- crates/red_knot/src/args.rs | 6 +- crates/red_knot/src/main.rs | 50 ++++++------- crates/red_knot/tests/cli.rs | 72 +++++++++++++++++++ .../red_knot_project/src/metadata/options.rs | 8 ++- 4 files changed, 106 insertions(+), 30 deletions(-) diff --git a/crates/red_knot/src/args.rs b/crates/red_knot/src/args.rs index ad60966e466cf..cbdd799fb670f 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; @@ -105,7 +105,9 @@ impl CheckCommand { ..EnvironmentOptions::default() }), rules, - error_on_warning: Some(self.error_on_warning), + terminal: Some(TerminalOptions { + error_on_warning: self.error_on_warning.then_some(true), + }), ..Default::default() } } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index dfe2d0f71abd6..cf5ec4fd9952c 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -8,8 +8,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}; @@ -218,17 +218,24 @@ impl MainLoop { result, revision: check_revision, } => { - let (has_warnings, has_errors) = result.iter().fold( - (false, false), - |(has_warnings, has_errors), diagnostic| { - let severity = diagnostic.severity(); - - ( - has_warnings || severity == Severity::Warning, - has_errors || severity >= Severity::Error, - ) - }, - ); + let error_on_warnings = db + .project() + .metadata(db) + .options() + .terminal + .as_ref() + .and_then(|it| it.error_on_warning) + .unwrap_or_default(); + + let failure_threshold = if error_on_warnings { + Severity::Warning + } else { + Severity::Error + }; + + let failed = result + .iter() + .any(|diagnostic| diagnostic.severity() >= failure_threshold); if check_revision == revision { #[allow(clippy::print_stdout)] @@ -242,21 +249,10 @@ impl MainLoop { } if self.watcher.is_none() { - let error_on_warning = - self.cli_options.error_on_warning.unwrap_or_default(); - - return match (has_warnings, has_errors) { - (false, false) => ExitStatus::Success, - - (true, false) => { - if error_on_warning { - ExitStatus::Failure - } else { - ExitStatus::Success - } - } - - (_, true) => ExitStatus::Failure, + return if failed { + ExitStatus::Failure + } else { + ExitStatus::Success }; } diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index 5454308565463..045225c3e305d 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -649,6 +649,78 @@ fn exit_code_both_warnings_and_errors_and_error_on_warning_is_true() -> anyhow:: Ok(()) } +#[test] +fn exit_code_error_on_warning_from_config_file() -> anyhow::Result<()> { + let case = TestCase::with_files([ + ( + "pyproject.toml", + r#" + [tool.knot.terminal] + error-on-warning = true + "#, + ), + ( + "test.py", + r#" + print(x) + "#, + ), + ])?; + + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:2:7 + | + 2 | print(x) + | - Name `x` used when not defined + | + + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn exit_code_error_on_warning_from_config_file_overridden_by_cli() -> anyhow::Result<()> { + let case = TestCase::with_files([ + ( + "pyproject.toml", + r#" + [tool.knot.terminal] + error-on-warning = false + "#, + ), + ( + "test.py", + r#" + print(x) + "#, + ), + ])?; + + assert_cmd_snapshot!(case.command().arg("--error-on-warning"), @r" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:unresolved-reference + --> /test.py:2:7 + | + 2 | print(x) + | - Name `x` used when not defined + | + + + ----- stderr ----- + "); + + Ok(()) +} + #[test] fn exit_code_exit_zero_is_true() -> anyhow::Result<()> { let case = TestCase::with_file( diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index a2da43ce80e11..2549abe8a5b4c 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -29,7 +29,7 @@ pub struct Options { pub rules: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub error_on_warning: Option, + pub terminal: Option, } impl Options { @@ -229,6 +229,12 @@ impl FromIterator<(RangedValue, RangedValue)> for Rules { } } +#[derive(Debug, Default, Clone, Eq, PartialEq, Combine, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case", deny_unknown_fields)] +pub struct TerminalOptions { + pub error_on_warning: Option, +} + #[derive(Error, Debug)] pub enum KnotTomlError { #[error(transparent)] From 01ef0b9e0ef482f4cb65d6ccae25d97fe7487cf4 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 3 Feb 2025 08:27:12 +0100 Subject: [PATCH 3/3] Remove `terminal` options --- crates/red_knot/src/args.rs | 5 +- crates/red_knot/src/main.rs | 38 +++++----- crates/red_knot/tests/cli.rs | 72 ------------------- .../red_knot_project/src/metadata/options.rs | 9 --- 4 files changed, 20 insertions(+), 104 deletions(-) diff --git a/crates/red_knot/src/args.rs b/crates/red_knot/src/args.rs index cbdd799fb670f..7873a5d4336a1 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, TerminalOptions}; +use red_knot_project::metadata::options::{EnvironmentOptions, Options}; use red_knot_project::metadata::value::{RangedValue, RelativePathBuf}; use red_knot_python_semantic::lint; use ruff_db::system::SystemPathBuf; @@ -105,9 +105,6 @@ impl CheckCommand { ..EnvironmentOptions::default() }), rules, - terminal: Some(TerminalOptions { - error_on_warning: self.error_on_warning.then_some(true), - }), ..Default::default() } } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index cf5ec4fd9952c..d1c90c3a1c29c 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -8,8 +8,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}; @@ -85,13 +85,19 @@ 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)?; workspace_metadata.apply_cli_options(cli_options.clone()); let mut db = ProjectDatabase::new(workspace_metadata, system)?; - let (main_loop, main_loop_cancellation_token) = MainLoop::new(cli_options); + let (main_loop, main_loop_cancellation_token) = MainLoop::new(cli_options, min_error_severity); // Listen to Ctrl+C and abort the watch mode. let main_loop_cancellation_token = Mutex::new(Some(main_loop_cancellation_token)); @@ -149,10 +155,18 @@ 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) -> (Self, MainLoopCancellationToken) { + fn new( + cli_options: Options, + min_error_severity: Severity, + ) -> (Self, MainLoopCancellationToken) { let (sender, receiver) = crossbeam_channel::bounded(10); ( @@ -161,6 +175,7 @@ impl MainLoop { receiver, watcher: None, cli_options, + min_error_severity, }, MainLoopCancellationToken { sender }, ) @@ -218,24 +233,9 @@ impl MainLoop { result, revision: check_revision, } => { - let error_on_warnings = db - .project() - .metadata(db) - .options() - .terminal - .as_ref() - .and_then(|it| it.error_on_warning) - .unwrap_or_default(); - - let failure_threshold = if error_on_warnings { - Severity::Warning - } else { - Severity::Error - }; - let failed = result .iter() - .any(|diagnostic| diagnostic.severity() >= failure_threshold); + .any(|diagnostic| diagnostic.severity() >= self.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 045225c3e305d..5454308565463 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -649,78 +649,6 @@ fn exit_code_both_warnings_and_errors_and_error_on_warning_is_true() -> anyhow:: Ok(()) } -#[test] -fn exit_code_error_on_warning_from_config_file() -> anyhow::Result<()> { - let case = TestCase::with_files([ - ( - "pyproject.toml", - r#" - [tool.knot.terminal] - error-on-warning = true - "#, - ), - ( - "test.py", - r#" - print(x) - "#, - ), - ])?; - - assert_cmd_snapshot!(case.command(), @r###" - success: false - exit_code: 1 - ----- stdout ----- - warning: lint:unresolved-reference - --> /test.py:2:7 - | - 2 | print(x) - | - Name `x` used when not defined - | - - - ----- stderr ----- - "###); - - Ok(()) -} - -#[test] -fn exit_code_error_on_warning_from_config_file_overridden_by_cli() -> anyhow::Result<()> { - let case = TestCase::with_files([ - ( - "pyproject.toml", - r#" - [tool.knot.terminal] - error-on-warning = false - "#, - ), - ( - "test.py", - r#" - print(x) - "#, - ), - ])?; - - assert_cmd_snapshot!(case.command().arg("--error-on-warning"), @r" - success: false - exit_code: 1 - ----- stdout ----- - warning: lint:unresolved-reference - --> /test.py:2:7 - | - 2 | print(x) - | - Name `x` used when not defined - | - - - ----- stderr ----- - "); - - Ok(()) -} - #[test] fn exit_code_exit_zero_is_true() -> anyhow::Result<()> { let case = TestCase::with_file( diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index 2549abe8a5b4c..13c27837c75f6 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -27,9 +27,6 @@ pub struct Options { #[serde(skip_serializing_if = "Option::is_none")] pub rules: Option, - - #[serde(skip_serializing_if = "Option::is_none")] - pub terminal: Option, } impl Options { @@ -229,12 +226,6 @@ impl FromIterator<(RangedValue, RangedValue)> for Rules { } } -#[derive(Debug, Default, Clone, Eq, PartialEq, Combine, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case", deny_unknown_fields)] -pub struct TerminalOptions { - pub error_on_warning: Option, -} - #[derive(Error, Debug)] pub enum KnotTomlError { #[error(transparent)]