diff --git a/Cargo.lock b/Cargo.lock index f06613f0ba1fc1..5f0e34faede9c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2717,6 +2717,7 @@ name = "ruff_db" version = "0.0.0" dependencies = [ "camino", + "colored 3.0.0", "countme", "dashmap 6.1.0", "dunce", @@ -2726,6 +2727,7 @@ dependencies = [ "insta", "matchit", "path-slash", + "ruff_annotate_snippets", "ruff_cache", "ruff_notebook", "ruff_python_ast", diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index c114f24e3b7220..cd6188a825fbe3 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -28,14 +28,24 @@ fn config_override() -> anyhow::Result<()> { ), ])?; - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - error[lint:unresolved-attribute] /test.py:5:7 Type `` has no attribute `last_exc` + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + error: lint:unresolved-attribute + --> /test.py:5:7 + | + 1 | + 2 | import sys + 3 | + 4 | # Access `sys.last_exc` that was only added in Python 3.12 + 5 | print(sys.last_exc) + | ^^^^^^^^^^^^ Type `` has no attribute `last_exc` + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); assert_cmd_snapshot!(case.command().arg("--python-version").arg("3.12"), @r" success: true @@ -91,14 +101,23 @@ fn cli_arguments_are_relative_to_the_current_directory() -> anyhow::Result<()> { ])?; // Make sure that the CLI fails when the `libs` directory is not in the search path. - assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")), @r#" - success: false - exit_code: 1 - ----- stdout ----- - error[lint:unresolved-import] /child/test.py:2:1 Cannot resolve import `utils` + assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")), @r###" + success: false + exit_code: 1 + ----- stdout ----- + error: lint:unresolved-import + --> /child/test.py:2:1 + | + 1 | + 2 | from utils import add + | ^^^^^^^^^^^^^^^^^^^^^ Cannot resolve import `utils` + 3 | + 4 | stat = add(10, 15) + | - ----- stderr ----- - "#); + + ----- stderr ----- + "###); assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")).arg("--extra-search-path").arg("../libs"), @r" success: true @@ -180,15 +199,39 @@ fn configuration_rule_severity() -> anyhow::Result<()> { // Assert that there's a possibly unresolved reference diagnostic // and that division-by-zero has a severity of error by default. - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - error[lint:division-by-zero] /test.py:2:5 Cannot divide object of type `Literal[4]` by zero - warning[lint:possibly-unresolved-reference] /test.py:7:7 Name `x` used when possibly not defined + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + error: lint:division-by-zero + --> /test.py:2:5 + | + 1 | + 2 | y = 4 / 0 + | ^^^^^ Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | + + warning: lint:possibly-unresolved-reference + --> /test.py:7:7 + | + 1 | + 2 | y = 4 / 0 + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | - Name `x` used when possibly not defined + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); case.write_file( "pyproject.toml", @@ -199,14 +242,26 @@ fn configuration_rule_severity() -> anyhow::Result<()> { "#, )?; - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - warning[lint:division-by-zero] /test.py:2:5 Cannot divide object of type `Literal[4]` by zero + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:division-by-zero + --> /test.py:2:5 + | + 1 | + 2 | y = 4 / 0 + | ----- Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); Ok(()) } @@ -230,16 +285,58 @@ fn cli_rule_severity() -> anyhow::Result<()> { // Assert that there's a possibly unresolved reference diagnostic // and that division-by-zero has a severity of error by default. - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - error[lint:unresolved-import] /test.py:2:8 Cannot resolve import `does_not_exit` - error[lint:division-by-zero] /test.py:4:5 Cannot divide object of type `Literal[4]` by zero - warning[lint:possibly-unresolved-reference] /test.py:9:7 Name `x` used when possibly not defined + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + error: lint:unresolved-import + --> /test.py:2:8 + | + 1 | + 2 | import does_not_exit + | ^^^^^^^^^^^^^ Cannot resolve import `does_not_exit` + 3 | + 4 | y = 4 / 0 + 5 | + 6 | for a in range(0, y): + 7 | x = a + 8 | + 9 | print(x) # possibly-unresolved-reference + | + + error: lint:division-by-zero + --> /test.py:4:5 + | + 1 | + 2 | import does_not_exit + 3 | + 4 | y = 4 / 0 + | ^^^^^ Cannot divide object of type `Literal[4]` by zero + 5 | + 6 | for a in range(0, y): + 7 | x = a + 8 | + 9 | print(x) # possibly-unresolved-reference + | + + warning: lint:possibly-unresolved-reference + --> /test.py:9:7 + | + 1 | + 2 | import does_not_exit + 3 | + 4 | y = 4 / 0 + 5 | + 6 | for a in range(0, y): + 7 | x = a + 8 | + 9 | print(x) # possibly-unresolved-reference + | - Name `x` used when possibly not defined + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); assert_cmd_snapshot!( case @@ -250,15 +347,43 @@ fn cli_rule_severity() -> anyhow::Result<()> { .arg("division-by-zero") .arg("--warn") .arg("unresolved-import"), - @r" + @r###" success: false exit_code: 1 ----- stdout ----- - warning[lint:unresolved-import] /test.py:2:8 Cannot resolve import `does_not_exit` - warning[lint:division-by-zero] /test.py:4:5 Cannot divide object of type `Literal[4]` by zero + warning: lint:unresolved-import + --> /test.py:2:8 + | + 1 | + 2 | import does_not_exit + | ------------- Cannot resolve import `does_not_exit` + 3 | + 4 | y = 4 / 0 + 5 | + 6 | for a in range(0, y): + 7 | x = a + 8 | + 9 | print(x) # possibly-unresolved-reference + | + + warning: lint:division-by-zero + --> /test.py:4:5 + | + 1 | + 2 | import does_not_exit + 3 | + 4 | y = 4 / 0 + | ----- Cannot divide object of type `Literal[4]` by zero + 5 | + 6 | for a in range(0, y): + 7 | x = a + 8 | + 9 | print(x) # possibly-unresolved-reference + | + ----- stderr ----- - " + "### ); Ok(()) @@ -282,15 +407,39 @@ fn cli_rule_severity_precedence() -> anyhow::Result<()> { // Assert that there's a possibly unresolved reference diagnostic // and that division-by-zero has a severity of error by default. - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - error[lint:division-by-zero] /test.py:2:5 Cannot divide object of type `Literal[4]` by zero - warning[lint:possibly-unresolved-reference] /test.py:7:7 Name `x` used when possibly not defined + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + error: lint:division-by-zero + --> /test.py:2:5 + | + 1 | + 2 | y = 4 / 0 + | ^^^^^ Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | + + warning: lint:possibly-unresolved-reference + --> /test.py:7:7 + | + 1 | + 2 | y = 4 / 0 + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | - Name `x` used when possibly not defined + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); assert_cmd_snapshot!( case @@ -302,14 +451,26 @@ fn cli_rule_severity_precedence() -> anyhow::Result<()> { // Override the error severity with warning .arg("--ignore") .arg("possibly-unresolved-reference"), - @r" - success: false - exit_code: 1 - ----- stdout ----- - warning[lint:division-by-zero] /test.py:2:5 Cannot divide object of type `Literal[4]` by zero + @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:division-by-zero + --> /test.py:2:5 + | + 1 | + 2 | y = 4 / 0 + | ----- Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + 5 | x = a + 6 | + 7 | print(x) # possibly-unresolved-reference + | - ----- stderr ----- - " + + ----- stderr ----- + "### ); Ok(()) @@ -329,14 +490,22 @@ fn configuration_unknown_rules() -> anyhow::Result<()> { ("test.py", "print(10)"), ])?; - assert_cmd_snapshot!(case.command(), @r" - success: false - exit_code: 1 - ----- stdout ----- - warning[unknown-rule] /pyproject.toml:3:1 Unknown lint rule `division-by-zer` + assert_cmd_snapshot!(case.command(), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: unknown-rule + --> /pyproject.toml:3:1 + | + 1 | + 2 | [tool.knot.rules] + 3 | division-by-zer = "warn" # incorrect rule name + | --------------- Unknown lint rule `division-by-zer` + | - ----- stderr ----- - "); + + ----- stderr ----- + "###); Ok(()) } @@ -346,14 +515,15 @@ fn configuration_unknown_rules() -> anyhow::Result<()> { 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: false - exit_code: 1 - ----- stdout ----- - warning[unknown-rule] Unknown lint rule `division-by-zer` + assert_cmd_snapshot!(case.command().arg("--ignore").arg("division-by-zer"), @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: unknown-rule: Unknown lint rule `division-by-zer` - ----- stderr ----- - "); + + ----- stderr ----- + "###); Ok(()) } diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index cef0d11f8f3016..bb2ecf9702d70e 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -11,6 +11,7 @@ repository = { workspace = true } license = { workspace = true } [dependencies] +ruff_annotate_snippets = { workspace = true } ruff_cache = { workspace = true, optional = true } ruff_notebook = { workspace = true } ruff_python_ast = { workspace = true } @@ -20,6 +21,7 @@ ruff_source_file = { workspace = true } ruff_text_size = { workspace = true } camino = { workspace = true } +colored = { workspace = true } countme = { workspace = true } dashmap = { workspace = true } dunce = { workspace = true } diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs index 223879cdd08ca5..eb942349d13658 100644 --- a/crates/ruff_db/src/diagnostic.rs +++ b/crates/ruff_db/src/diagnostic.rs @@ -3,14 +3,11 @@ use std::fmt::Formatter; use thiserror::Error; +use ruff_annotate_snippets::{Level, Renderer, Snippet}; use ruff_python_parser::ParseError; use ruff_text_size::TextRange; -use crate::{ - files::File, - source::{line_index, source_text}, - Db, -}; +use crate::{files::File, source::source_text, Db}; /// A string identifier for a lint rule. /// @@ -210,29 +207,56 @@ impl<'db> DisplayDiagnostic<'db> { impl std::fmt::Display for DisplayDiagnostic<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self.diagnostic.severity() { - Severity::Info => f.write_str("info")?, - Severity::Warning => f.write_str("warning")?, - Severity::Error => f.write_str("error")?, - Severity::Fatal => f.write_str("fatal")?, - } - - write!(f, "[{rule}]", rule = self.diagnostic.id())?; - - if let Some(file) = self.diagnostic.file() { - write!(f, " {path}", path = file.path(self.db))?; - } - - if let (Some(file), Some(range)) = (self.diagnostic.file(), self.diagnostic.range()) { - let index = line_index(self.db, file); - let source = source_text(self.db, file); - - let start = index.source_location(range.start(), &source); - - write!(f, ":{line}:{col}", line = start.row, col = start.column)?; + let level = match self.diagnostic.severity() { + Severity::Info => Level::Info, + Severity::Warning => Level::Warning, + Severity::Error => Level::Error, + // NOTE: Should we really collapse this to "error"? + // + // After collapsing this, the snapshot tests seem to reveal that we + // don't currently have any *tests* with a `fatal` severity level. + // And maybe *rendering* this as just an `error` is fine. If we + // really do need different rendering, then I think we can add a + // `Level::Fatal`. ---AG + Severity::Fatal => Level::Error, + }; + + let render = |f: &mut std::fmt::Formatter, message| { + let renderer = if !cfg!(test) && colored::control::SHOULD_COLORIZE.should_colorize() { + Renderer::styled() + } else { + Renderer::plain() + } + .cut_indicator("…"); + let rendered = renderer.render(message); + writeln!(f, "{rendered}") + }; + match (self.diagnostic.file(), self.diagnostic.range()) { + (None, _) => { + // NOTE: This is pretty sub-optimal. It doesn't render well. We + // really want a snippet, but without a `File`, we can't really + // render anything. It looks like this case currently happens + // for configuration errors. It looks like we can probably + // produce a snippet for this if it comes from a file, but if + // it comes from the CLI, I'm not quite sure exactly what to + // do. ---AG + let msg = format!("{}: {}", self.diagnostic.id(), self.diagnostic.message()); + render(f, level.title(&msg)) + } + (Some(file), range) => { + let path = file.path(self.db).to_string(); + let source = source_text(self.db, file); + let title = self.diagnostic.id().to_string(); + let message = self.diagnostic.message(); + + let mut snippet = Snippet::source(source.as_str()).origin(&path).line_start(1); + if let Some(range) = range { + let annotation = level.span(range.into()).label(&message); + snippet = snippet.annotation(annotation); + } + render(f, level.title(&title).snippet(snippet)) + } } - - write!(f, " {message}", message = self.diagnostic.message()) } }