From e64d77278830954a323d227e8f9f714c1d0e4c57 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 16 Oct 2025 11:56:32 -0400 Subject: [PATCH] Standardize syntax error construction (#20903) Summary -- This PR unifies the two different ways Ruff and ty construct syntax errors. Ruff has been storing the primary message in the diagnostic itself, while ty attached the message to the primary annotation: ``` > ruff check try.py invalid-syntax: name capture `x` makes remaining patterns unreachable --> try.py:2:10 | 1 | match 42: 2 | case x: ... | ^ 3 | case y: ... | Found 1 error. > uvx ty check try.py WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors. Checking ------------------------------------------------------------ 1/1 files error[invalid-syntax] --> try.py:2:10 | 1 | match 42: 2 | case x: ... | ^ name capture `x` makes remaining patterns unreachable 3 | case y: ... | Found 1 diagnostic ``` I think there are benefits to both approaches, and I do like ty's version, but I feel like we should pick one (and it might help with #20901 eventually). I slightly prefer Ruff's version, so I went with that. Hopefully this isn't too controversial, but I'm happy to close this if it is. Note that this shouldn't change any other diagnostic formats in ty because [`Diagnostic::primary_message`](https://github.com/astral-sh/ruff/blob/98d27c412810e157f8a65ea75726d66676628225/crates/ruff_db/src/diagnostic/mod.rs#L177) was already falling back to the primary annotation message if the diagnostic message was empty. As a result, I think this change will partially resolve the FIXME therein. Test Plan -- Existing tests with updated snapshots --- crates/ruff/src/diagnostics.rs | 7 +--- crates/ruff_db/src/diagnostic/mod.rs | 7 +--- crates/ruff_linter/src/linter.rs | 7 ++-- crates/ruff_linter/src/message/mod.rs | 24 +---------- crates/ruff_linter/src/test.rs | 6 +-- crates/ty/tests/cli/main.rs | 40 +++++++++---------- crates/ty/tests/cli/python_environment.rs | 16 ++++---- ...ehensio…_-_Python_3.10_(96aa8ec77d46553d).snap | 4 +- ...tatement_-_Before_3.10_(2545eaa83b635b8b).snap | 4 +- 9 files changed, 43 insertions(+), 72 deletions(-) diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index 3376133ed7..ef7623145a 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -13,7 +13,6 @@ use log::{debug, warn}; use ruff_db::diagnostic::Diagnostic; use ruff_linter::codes::Rule; use ruff_linter::linter::{FixTable, FixerResult, LinterResult, ParseSource, lint_fix, lint_only}; -use ruff_linter::message::create_syntax_error_diagnostic; use ruff_linter::package::PackageRoot; use ruff_linter::pyproject_toml::lint_pyproject_toml; use ruff_linter::settings::types::UnsafeFixes; @@ -103,11 +102,7 @@ impl Diagnostics { let name = path.map_or_else(|| "-".into(), Path::to_string_lossy); let dummy = SourceFileBuilder::new(name, "").finish(); Self::new( - vec![create_syntax_error_diagnostic( - dummy, - err, - TextRange::default(), - )], + vec![Diagnostic::invalid_syntax(dummy, err, TextRange::default())], FxHashMap::default(), ) } diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 230cda7a84..9d9ff2dbc3 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -84,17 +84,14 @@ impl Diagnostic { /// at time of writing, `ruff_db` depends on `ruff_python_parser` instead of /// the other way around. And since we want to do this conversion in a couple /// places, it makes sense to centralize it _somewhere_. So it's here for now. - /// - /// Note that `message` is stored in the primary annotation, _not_ in the primary diagnostic - /// message. pub fn invalid_syntax( span: impl Into, message: impl IntoDiagnosticMessage, range: impl Ranged, ) -> Diagnostic { - let mut diag = Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, ""); + let mut diag = Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, message); let span = span.into().with_range(range.range()); - diag.annotate(Annotation::primary(span).message(message)); + diag.annotate(Annotation::primary(span)); diag } diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 56e180bfab..1386704920 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -24,7 +24,6 @@ use crate::checkers::tokens::check_tokens; use crate::directives::Directives; use crate::doc_lines::{doc_lines_from_ast, doc_lines_from_tokens}; use crate::fix::{FixResult, fix_file}; -use crate::message::create_syntax_error_diagnostic; use crate::noqa::add_noqa; use crate::package::PackageRoot; use crate::registry::Rule; @@ -496,15 +495,15 @@ fn diagnostics_to_messages( parse_errors .iter() .map(|parse_error| { - create_syntax_error_diagnostic(source_file.clone(), &parse_error.error, parse_error) + Diagnostic::invalid_syntax(source_file.clone(), &parse_error.error, parse_error) }) .chain(unsupported_syntax_errors.iter().map(|syntax_error| { - create_syntax_error_diagnostic(source_file.clone(), syntax_error, syntax_error) + Diagnostic::invalid_syntax(source_file.clone(), syntax_error, syntax_error) })) .chain( semantic_syntax_errors .iter() - .map(|error| create_syntax_error_diagnostic(source_file.clone(), error, error)), + .map(|error| Diagnostic::invalid_syntax(source_file.clone(), error, error)), ) .chain(diagnostics.into_iter().map(|mut diagnostic| { if let Some(range) = diagnostic.range() { diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index f552d0df51..2525322fd9 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -16,7 +16,7 @@ use ruff_db::files::File; pub use grouped::GroupedEmitter; use ruff_notebook::NotebookIndex; use ruff_source_file::{SourceFile, SourceFileBuilder}; -use ruff_text_size::{Ranged, TextRange, TextSize}; +use ruff_text_size::{TextRange, TextSize}; pub use sarif::SarifEmitter; use crate::Fix; @@ -26,24 +26,6 @@ use crate::settings::types::{OutputFormat, RuffOutputFormat}; mod grouped; mod sarif; -/// Creates a `Diagnostic` from a syntax error, with the format expected by Ruff. -/// -/// This is almost identical to `ruff_db::diagnostic::create_syntax_error_diagnostic`, except the -/// `message` is stored as the primary diagnostic message instead of on the primary annotation. -/// -/// TODO(brent) These should be unified at some point, but we keep them separate for now to avoid a -/// ton of snapshot changes while combining ruff's diagnostic type with `Diagnostic`. -pub fn create_syntax_error_diagnostic( - span: impl Into, - message: impl std::fmt::Display, - range: impl Ranged, -) -> Diagnostic { - let mut diag = Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, message); - let span = span.into().with_range(range.range()); - diag.annotate(Annotation::primary(span)); - diag -} - /// Create a `Diagnostic` from a panic. pub fn create_panic_diagnostic(error: &PanicError, path: Option<&Path>) -> Diagnostic { let mut diagnostic = Diagnostic::new( @@ -260,8 +242,6 @@ mod tests { use crate::message::{Emitter, EmitterContext, create_lint_diagnostic}; use crate::{Edit, Fix}; - use super::create_syntax_error_diagnostic; - pub(super) fn create_syntax_error_diagnostics() -> Vec { let source = r"from os import @@ -274,7 +254,7 @@ if call(foo .errors() .iter() .map(|parse_error| { - create_syntax_error_diagnostic(source_file.clone(), &parse_error.error, parse_error) + Diagnostic::invalid_syntax(source_file.clone(), &parse_error.error, parse_error) }) .collect() } diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index 01eb02705e..67a6728404 100644 --- a/crates/ruff_linter/src/test.rs +++ b/crates/ruff_linter/src/test.rs @@ -26,7 +26,7 @@ use ruff_source_file::SourceFileBuilder; use crate::codes::Rule; use crate::fix::{FixResult, fix_file}; use crate::linter::check_path; -use crate::message::{EmitterContext, create_syntax_error_diagnostic}; +use crate::message::EmitterContext; use crate::package::PackageRoot; use crate::packaging::detect_package_root; use crate::settings::types::UnsafeFixes; @@ -405,7 +405,7 @@ Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to e diagnostic }) .chain(parsed.errors().iter().map(|parse_error| { - create_syntax_error_diagnostic(source_code.clone(), &parse_error.error, parse_error) + Diagnostic::invalid_syntax(source_code.clone(), &parse_error.error, parse_error) })) .sorted_by(Diagnostic::ruff_start_ordering) .collect(); @@ -419,7 +419,7 @@ fn print_syntax_errors(errors: &[ParseError], path: &Path, source: &SourceKind) let messages: Vec<_> = errors .iter() .map(|parse_error| { - create_syntax_error_diagnostic(source_file.clone(), &parse_error.error, parse_error) + Diagnostic::invalid_syntax(source_file.clone(), &parse_error.error, parse_error) }) .collect(); diff --git a/crates/ty/tests/cli/main.rs b/crates/ty/tests/cli/main.rs index 45d7304be5..e911e300c0 100644 --- a/crates/ty/tests/cli/main.rs +++ b/crates/ty/tests/cli/main.rs @@ -92,42 +92,42 @@ fn test_quiet_output() -> anyhow::Result<()> { #[test] fn test_run_in_sub_directory() -> anyhow::Result<()> { let case = CliTest::with_files([("test.py", "~"), ("subdir/nothing", "")])?; - assert_cmd_snapshot!(case.command().current_dir(case.root().join("subdir")).arg(".."), @r###" + assert_cmd_snapshot!(case.command().current_dir(case.root().join("subdir")).arg(".."), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Expected an expression --> /test.py:1:2 | 1 | ~ - | ^ Expected an expression + | ^ | Found 1 diagnostic ----- stderr ----- - "###); + "); Ok(()) } #[test] fn test_include_hidden_files_by_default() -> anyhow::Result<()> { let case = CliTest::with_files([(".test.py", "~")])?; - assert_cmd_snapshot!(case.command(), @r###" + assert_cmd_snapshot!(case.command(), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Expected an expression --> .test.py:1:2 | 1 | ~ - | ^ Expected an expression + | ^ | Found 1 diagnostic ----- stderr ----- - "###); + "); Ok(()) } @@ -146,57 +146,57 @@ fn test_respect_ignore_files() -> anyhow::Result<()> { "###); // Test that we can set to false via CLI - assert_cmd_snapshot!(case.command().arg("--no-respect-ignore-files"), @r###" + assert_cmd_snapshot!(case.command().arg("--no-respect-ignore-files"), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Expected an expression --> test.py:1:2 | 1 | ~ - | ^ Expected an expression + | ^ | Found 1 diagnostic ----- stderr ----- - "###); + "); // Test that we can set to false via config file case.write_file("ty.toml", "src.respect-ignore-files = false")?; - assert_cmd_snapshot!(case.command(), @r###" + assert_cmd_snapshot!(case.command(), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Expected an expression --> test.py:1:2 | 1 | ~ - | ^ Expected an expression + | ^ | Found 1 diagnostic ----- stderr ----- - "###); + "); // Ensure CLI takes precedence case.write_file("ty.toml", "src.respect-ignore-files = true")?; - assert_cmd_snapshot!(case.command().arg("--no-respect-ignore-files"), @r###" + assert_cmd_snapshot!(case.command().arg("--no-respect-ignore-files"), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Expected an expression --> test.py:1:2 | 1 | ~ - | ^ Expected an expression + | ^ | Found 1 diagnostic ----- stderr ----- - "###); + "); Ok(()) } diff --git a/crates/ty/tests/cli/python_environment.rs b/crates/ty/tests/cli/python_environment.rs index 4168da8e4c..4f3e442473 100644 --- a/crates/ty/tests/cli/python_environment.rs +++ b/crates/ty/tests/cli/python_environment.rs @@ -655,15 +655,15 @@ fn config_file_annotation_showing_where_python_version_set_syntax_error() -> any ), ])?; - assert_cmd_snapshot!(case.command(), @r###" + assert_cmd_snapshot!(case.command(), @r#" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Cannot use `match` statement on Python 3.8 (syntax was added in Python 3.10) --> test.py:2:1 | 2 | match object(): - | ^^^^^ Cannot use `match` statement on Python 3.8 (syntax was added in Python 3.10) + | ^^^^^ 3 | case int(): 4 | pass | @@ -678,17 +678,17 @@ fn config_file_annotation_showing_where_python_version_set_syntax_error() -> any Found 1 diagnostic ----- stderr ----- - "###); + "#); - assert_cmd_snapshot!(case.command().arg("--python-version=3.9"), @r###" + assert_cmd_snapshot!(case.command().arg("--python-version=3.9"), @r" success: false exit_code: 1 ----- stdout ----- - error[invalid-syntax] + error[invalid-syntax]: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) --> test.py:2:1 | 2 | match object(): - | ^^^^^ Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) + | ^^^^^ 3 | case int(): 4 | pass | @@ -697,7 +697,7 @@ fn config_file_annotation_showing_where_python_version_set_syntax_error() -> any Found 1 diagnostic ----- stderr ----- - "###); + "); Ok(()) } diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/semantic_syntax_erro…_-_Semantic_syntax_erro…_-_`async`_comprehensio…_-_Python_3.10_(96aa8ec77d46553d).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/semantic_syntax_erro…_-_Semantic_syntax_erro…_-_`async`_comprehensio…_-_Python_3.10_(96aa8ec77d46553d).snap index f25e7b1bac..975be8520f 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/semantic_syntax_erro…_-_Semantic_syntax_erro…_-_`async`_comprehensio…_-_Python_3.10_(96aa8ec77d46553d).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/semantic_syntax_erro…_-_Semantic_syntax_erro…_-_`async`_comprehensio…_-_Python_3.10_(96aa8ec77d46553d).snap @@ -33,13 +33,13 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syn # Diagnostics ``` -error[invalid-syntax] +error[invalid-syntax]: cannot use an asynchronous comprehension inside of a synchronous comprehension on Python 3.10 (syntax was added in 3.11) --> src/mdtest_snippet.py:6:19 | 4 | async def f(): 5 | # error: 19 [invalid-syntax] "cannot use an asynchronous comprehension inside of a synchronous comprehension on Python 3.10 (syntax… 6 | return {n: [x async for x in elements(n)] for n in range(3)} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use an asynchronous comprehension inside of a synchronous comprehension on Python 3.10 (syntax was added in 3.11) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ 7 | async def test(): 8 | # error: [not-iterable] "Object of type `range` is not async-iterable" | diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/version_related_synt…_-_Version-related_synt…_-_`match`_statement_-_Before_3.10_(2545eaa83b635b8b).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/version_related_synt…_-_Version-related_synt…_-_`match`_statement_-_Before_3.10_(2545eaa83b635b8b).snap index 2500cc4544..a2cea52ff9 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/version_related_synt…_-_Version-related_synt…_-_`match`_statement_-_Before_3.10_(2545eaa83b635b8b).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/version_related_synt…_-_Version-related_synt…_-_`match`_statement_-_Before_3.10_(2545eaa83b635b8b).snap @@ -20,11 +20,11 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/diagnostics/version_rela # Diagnostics ``` -error[invalid-syntax] +error[invalid-syntax]: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) --> src/mdtest_snippet.py:1:1 | 1 | match 2: # error: 1 [invalid-syntax] "Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)" - | ^^^^^ Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) + | ^^^^^ 2 | case 1: 3 | print("it's one") |