diff --git a/Cargo.lock b/Cargo.lock index e4715cac60..3a1cfd39ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4474,6 +4474,7 @@ dependencies = [ "quickcheck_macros", "ruff_annotate_snippets", "ruff_db", + "ruff_diagnostics", "ruff_index", "ruff_macros", "ruff_memory_usage", @@ -4519,6 +4520,7 @@ dependencies = [ "lsp-types", "regex", "ruff_db", + "ruff_diagnostics", "ruff_macros", "ruff_notebook", "ruff_python_ast", diff --git a/crates/ty/docs/rules.md b/crates/ty/docs/rules.md index 0f4cc130ae..fcbab4402a 100644 --- a/crates/ty/docs/rules.md +++ b/crates/ty/docs/rules.md @@ -2120,7 +2120,7 @@ old_func() # emits [deprecated] diagnostic Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2151,7 +2151,7 @@ a = 20 / 0 # ty: ignore[division-by-zero] Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2536,7 +2536,7 @@ print(x) # NameError: name 'x' is not defined Default level: ignore · Added in 0.0.1-alpha.1 · Related issues · -View source +View source diff --git a/crates/ty/src/lib.rs b/crates/ty/src/lib.rs index e93292936c..96e85fea6b 100644 --- a/crates/ty/src/lib.rs +++ b/crates/ty/src/lib.rs @@ -313,7 +313,8 @@ impl MainLoop { let terminal_settings = db.project().settings(db).terminal(); let display_config = DisplayDiagnosticConfig::default() .format(terminal_settings.output_format.into()) - .color(colored::control::SHOULD_COLORIZE.should_colorize()); + .color(colored::control::SHOULD_COLORIZE.should_colorize()) + .show_fix_diff(true); if check_revision == revision { if db.project().files(db).is_empty() { diff --git a/crates/ty_python_semantic/Cargo.toml b/crates/ty_python_semantic/Cargo.toml index 4d4ece53eb..140eec33be 100644 --- a/crates/ty_python_semantic/Cargo.toml +++ b/crates/ty_python_semantic/Cargo.toml @@ -13,6 +13,7 @@ license = { workspace = true } [dependencies] ruff_db = { workspace = true } ruff_annotate_snippets = { workspace = true } +ruff_diagnostics = { workspace = true } ruff_index = { workspace = true, features = ["salsa"] } ruff_macros = { workspace = true } ruff_memory_usage = { workspace = true } diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/mro.md_-_Method_Resolution_Or…_-_`__bases__`_lists_wi…_(ea7ebc83ec359b54).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/mro.md_-_Method_Resolution_Or…_-_`__bases__`_lists_wi…_(ea7ebc83ec359b54).snap index 99d646c1b3..e67424b4fb 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/mro.md_-_Method_Resolution_Or…_-_`__bases__`_lists_wi…_(ea7ebc83ec359b54).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/mro.md_-_Method_Resolution_Or…_-_`__bases__`_lists_wi…_(ea7ebc83ec359b54).snap @@ -312,6 +312,15 @@ info[unused-ignore-comment]: Unused blanket `type: ignore` directive | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 73 | ): ... | +help: Remove the unused suppression comment +69 | class D( +70 | A, +71 | # error: [unused-ignore-comment] + - A, # type: ignore[duplicate-base] +72 + A, +73 | ): ... +74 | +75 | # error: [duplicate-base] ``` @@ -356,5 +365,13 @@ info[unused-ignore-comment]: Unused blanket `type: ignore` directive 82 | 83 | # fmt: on | +help: Remove the unused suppression comment +78 | A +79 | ): +80 | # error: [unused-ignore-comment] + - x: int # type: ignore[duplicate-base] +81 + x: int +82 | +83 | # fmt: on ``` diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/ty_ignore.md_-_Suppressing_errors_w…_-_Multiple_unused_comm…_(7cbe4a1d9893a05).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/ty_ignore.md_-_Suppressing_errors_w…_-_Multiple_unused_comm…_(7cbe4a1d9893a05).snap new file mode 100644 index 0000000000..f5d6cc1663 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/ty_ignore.md_-_Suppressing_errors_w…_-_Multiple_unused_comm…_(7cbe4a1d9893a05).snap @@ -0,0 +1,109 @@ +--- +source: crates/ty_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: ty_ignore.md - Suppressing errors with `ty: ignore` - Multiple unused comments +mdtest path: crates/ty_python_semantic/resources/mdtest/suppressions/ty_ignore.md +--- + +# Python source files + +## mdtest_snippet.py + +``` +1 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive" +2 | a = 10 / 2 # ty: ignore[division-by-zero, unresolved-reference] +3 | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" +6 | a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" +9 | a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] +``` + +# Diagnostics + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive + --> src/mdtest_snippet.py:2:13 + | +1 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive" +2 | a = 10 / 2 # ty: ignore[division-by-zero, unresolved-reference] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +3 | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" + | +help: Remove the unused suppression comment +1 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive" + - a = 10 / 2 # ty: ignore[division-by-zero, unresolved-reference] +2 + a = 10 / 2 +3 | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" + +``` + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive: 'invalid-assignment' + --> src/mdtest_snippet.py:6:26 + | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" +6 | a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] + | ^^^^^^^^^^^^^^^^^^ +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" + | +help: Remove the unused suppression code +3 | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" + - a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] +6 + a = 10 / 0 # ty: ignore[division-by-zero, unresolved-reference] +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" +9 | a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] + +``` + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive: 'unresolved-reference' + --> src/mdtest_snippet.py:6:64 + | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" +6 | a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] + | ^^^^^^^^^^^^^^^^^^^^ +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" + | +help: Remove the unused suppression code +3 | +4 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" +5 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" + - a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] +6 + a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero] +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" +9 | a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] + +``` + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference' + --> src/mdtest_snippet.py:9:26 + | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" +9 | a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Remove the unused suppression codes +6 | a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] +7 | +8 | # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" + - a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] +9 + a = 10 / 0 # ty: ignore[division-by-zero] + +``` diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/type_ignore.md_-_Suppressing_errors_w…_-_Nested_comments_(6e4dc67270e388d2).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/type_ignore.md_-_Suppressing_errors_w…_-_Nested_comments_(6e4dc67270e388d2).snap new file mode 100644 index 0000000000..79d97064f2 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/type_ignore.md_-_Suppressing_errors_w…_-_Nested_comments_(6e4dc67270e388d2).snap @@ -0,0 +1,72 @@ +--- +source: crates/ty_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: type_ignore.md - Suppressing errors with `type: ignore` - Nested comments +mdtest path: crates/ty_python_semantic/resources/mdtest/suppressions/type_ignore.md +--- + +# Python source files + +## mdtest_snippet.py + +``` + 1 | # fmt: off + 2 | a = test / + 3 | + 2 # fmt: skip # type: ignore + 4 | + 5 | a = test / + 6 | + 2 # type: ignore # fmt: skip + 7 | + 8 | a = (3 + 9 | # error: [unused-ignore-comment] +10 | + 2) # ty:ignore[division-by-zero] # fmt: skip +11 | +12 | a = (3 +13 | # error: [unused-ignore-comment] +14 | + 2) # fmt: skip # ty:ignore[division-by-zero] +``` + +# Diagnostics + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive + --> src/mdtest_snippet.py:10:9 + | + 8 | a = (3 + 9 | # error: [unused-ignore-comment] +10 | + 2) # ty:ignore[division-by-zero] # fmt: skip + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +11 | +12 | a = (3 + | +help: Remove the unused suppression comment +7 | +8 | a = (3 +9 | # error: [unused-ignore-comment] + - + 2) # ty:ignore[division-by-zero] # fmt: skip +10 + + 2) # fmt: skip +11 | +12 | a = (3 +13 | # error: [unused-ignore-comment] + +``` + +``` +info[unused-ignore-comment]: Unused `ty: ignore` directive + --> src/mdtest_snippet.py:14:21 + | +12 | a = (3 +13 | # error: [unused-ignore-comment] +14 | + 2) # fmt: skip # ty:ignore[division-by-zero] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Remove the unused suppression comment +11 | +12 | a = (3 +13 | # error: [unused-ignore-comment] + - + 2) # fmt: skip # ty:ignore[division-by-zero] +14 + + 2) # fmt: skip + +``` diff --git a/crates/ty_python_semantic/resources/mdtest/suppressions/no_type_check.md b/crates/ty_python_semantic/resources/mdtest/suppressions/no_type_check.md index a658031ed3..262d287f97 100644 --- a/crates/ty_python_semantic/resources/mdtest/suppressions/no_type_check.md +++ b/crates/ty_python_semantic/resources/mdtest/suppressions/no_type_check.md @@ -117,6 +117,6 @@ from typing import no_type_check @no_type_check def test(): - # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" + # error: [unused-ignore-comment] "Unused `ty: ignore` directive" return x + 5 # ty: ignore[unresolved-reference] ``` diff --git a/crates/ty_python_semantic/resources/mdtest/suppressions/ty_ignore.md b/crates/ty_python_semantic/resources/mdtest/suppressions/ty_ignore.md index f9c2f9b5d2..0fc7d1e568 100644 --- a/crates/ty_python_semantic/resources/mdtest/suppressions/ty_ignore.md +++ b/crates/ty_python_semantic/resources/mdtest/suppressions/ty_ignore.md @@ -18,7 +18,7 @@ a = 4 + test # ty: ignore[unresolved-reference] ```py test = 10 -# error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'possibly-unresolved-reference'" +# error: [unused-ignore-comment] "Unused `ty: ignore` directive" a = test + 3 # ty: ignore[possibly-unresolved-reference] ``` @@ -26,7 +26,7 @@ a = test + 3 # ty: ignore[possibly-unresolved-reference] ```py # error: [unresolved-reference] -# error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'possibly-unresolved-reference'" +# error: [unused-ignore-comment] "Unused `ty: ignore` directive" a = test + 3 # ty: ignore[possibly-unresolved-reference] ``` @@ -50,17 +50,20 @@ a = 10 / 0 # ty: ignore[division-by-zero, unused-ignore-comment] ## Multiple unused comments -Today, ty emits a diagnostic for every unused code. We might want to group the codes by comment at -some point in the future. +ty groups unused codes that are next to each other. + + ```py -# error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'division-by-zero'" -# error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" +# error: [unused-ignore-comment] "Unused `ty: ignore` directive" a = 10 / 2 # ty: ignore[division-by-zero, unresolved-reference] # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment'" # error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'unresolved-reference'" a = 10 / 0 # ty: ignore[invalid-assignment, division-by-zero, unresolved-reference] + +# error: [unused-ignore-comment] "Unused `ty: ignore` directive: 'invalid-assignment', 'unresolved-reference'" +a = 10 / 0 # ty: ignore[invalid-assignment, unresolved-reference, division-by-zero] ``` ## Multiple suppressions diff --git a/crates/ty_python_semantic/resources/mdtest/suppressions/type_ignore.md b/crates/ty_python_semantic/resources/mdtest/suppressions/type_ignore.md index 43e9eedcc2..60f0f349de 100644 --- a/crates/ty_python_semantic/resources/mdtest/suppressions/type_ignore.md +++ b/crates/ty_python_semantic/resources/mdtest/suppressions/type_ignore.md @@ -96,6 +96,8 @@ a = test # type: ignore[name-defined] ## Nested comments + + ```py # fmt: off a = test \ @@ -103,6 +105,14 @@ a = test \ a = test \ + 2 # type: ignore # fmt: skip + +a = (3 + # error: [unused-ignore-comment] + + 2) # ty:ignore[division-by-zero] # fmt: skip + +a = (3 + # error: [unused-ignore-comment] + + 2) # fmt: skip # ty:ignore[division-by-zero] ``` ## Misspelled `type: ignore` diff --git a/crates/ty_python_semantic/src/suppression.rs b/crates/ty_python_semantic/src/suppression.rs index e337200f94..ab246c1b0a 100644 --- a/crates/ty_python_semantic/src/suppression.rs +++ b/crates/ty_python_semantic/src/suppression.rs @@ -1,20 +1,24 @@ -use crate::diagnostic::DiagnosticGuard; -use crate::lint::{GetLintError, Level, LintMetadata, LintRegistry, LintStatus}; -use crate::types::TypeCheckDiagnostics; -use crate::{Db, declare_lint, lint::LintId}; -use ruff_db::diagnostic::{ - Annotation, Diagnostic, DiagnosticId, IntoDiagnosticMessage, Severity, Span, -}; -use ruff_db::{files::File, parsed::parsed_module, source::source_text}; -use ruff_python_parser::TokenKind; -use ruff_python_trivia::Cursor; -use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use smallvec::{SmallVec, smallvec}; use std::error::Error; use std::fmt; use std::fmt::Formatter; +use std::fmt::Write as _; use thiserror::Error; +use crate::diagnostic::DiagnosticGuard; +use crate::lint::{GetLintError, Level, LintMetadata, LintRegistry, LintStatus}; +use crate::types::TypeCheckDiagnostics; +use crate::{Db, declare_lint, lint::LintId}; + +use ruff_db::diagnostic::{ + Annotation, Diagnostic, DiagnosticId, IntoDiagnosticMessage, Severity, Span, +}; +use ruff_db::{files::File, parsed::parsed_module, source::source_text}; +use ruff_diagnostics::{Edit, Fix}; +use ruff_python_parser::TokenKind; +use ruff_python_trivia::Cursor; +use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; + declare_lint! { /// ## What it does /// Checks for `type: ignore` or `ty: ignore` directives that are no longer applicable. @@ -109,7 +113,7 @@ pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { for comment in parser { match comment { Ok(comment) => { - builder.add_comment(&comment, TextRange::new(line_start, token.end())); + builder.add_comment(comment, TextRange::new(line_start, token.end())); } Err(error) => match error.kind { ParseErrorKind::NotASuppression @@ -222,38 +226,132 @@ fn check_unused_suppressions(context: &mut CheckSuppressionsContext) { unused.push(suppression); } - let unused = unused.iter().filter(|suppression| { - // This looks silly but it's necessary to check again if a `unused-ignore-comment` is indeed unused - // in case the "unused" directive comes after it: - // ```py - // a = 10 / 2 # ty: ignore[unused-ignore-comment, division-by-zero] - // ``` - !context.is_suppression_used(suppression.id()) - }); + let mut unused_iter = unused + .iter() + .filter(|suppression| { + // This looks silly but it's necessary to check again if a `unused-ignore-comment` is indeed unused + // in case the "unused" directive comes after it: + // ```py + // a = 10 / 2 # ty: ignore[unused-ignore-comment, division-by-zero] + // ``` + !context.is_suppression_used(suppression.id()) + }) + .peekable(); - for suppression in unused { - match suppression.target { + let source = source_text(context.db, context.file); + + while let Some(suppression) = unused_iter.next() { + let mut diag = match suppression.target { SuppressionTarget::All => { let Some(diag) = context.report_unchecked(&UNUSED_IGNORE_COMMENT, suppression.range) else { continue; }; + diag.into_diagnostic(format_args!( "Unused blanket `{}` directive", suppression.kind )) } SuppressionTarget::Lint(lint) => { + // A single code in a `ty: ignore[, , ...]` directive + + // Is this the first code directly after the `[`? + let includes_first_code = source[..suppression.range.start().to_usize()] + .trim_end() + .ends_with('['); + + let mut current = suppression; + let mut unused_codes = Vec::new(); + + // Group successive codes together into a single diagnostic, + // or report the entire directive if all codes are unused. + while let Some(next) = unused_iter.peek() { + if let SuppressionTarget::Lint(next_lint) = next.target + && next.comment_range == current.comment_range + && source[TextRange::new(current.range.end(), next.range.start())] + .chars() + .all(|c| c.is_whitespace() || c == ',') + { + unused_codes.push(next_lint); + current = *next; + unused_iter.next(); + } else { + break; + } + } + + // Is the last suppression code the last code before the closing `]`. + let includes_last_code = source[current.range.end().to_usize()..] + .trim_start() + .starts_with(']'); + + // If only some codes are unused + if !includes_first_code || !includes_last_code { + let mut codes = format!("'{}'", lint.name()); + for code in &unused_codes { + let _ = write!(&mut codes, ", '{code}'", code = code.name()); + } + + if let Some(diag) = context.report_unchecked( + &UNUSED_IGNORE_COMMENT, + TextRange::new(suppression.range.start(), current.range.end()), + ) { + let mut diag = diag.into_diagnostic(format_args!( + "Unused `{kind}` directive: {codes}", + kind = suppression.kind + )); + + diag.primary_annotation_mut() + .unwrap() + .push_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); + + // Delete everything up to the start of the next code. + let trailing_len: TextSize = source[current.range.end().to_usize()..] + .chars() + .take_while(|c: &char| c.is_whitespace() || *c == ',') + .map(TextLen::text_len) + .sum(); + + // If we delete the last codes before `]`, ensure we delete any trailing comma + let leading_len: TextSize = if includes_last_code { + source[..suppression.range.start().to_usize()] + .chars() + .rev() + .take_while(|c: &char| c.is_whitespace() || *c == ',') + .map(TextLen::text_len) + .sum() + } else { + TextSize::default() + }; + + let fix_range = TextRange::new( + suppression.range.start() - leading_len, + current.range.end() + trailing_len, + ); + diag.set_fix(Fix::safe_edit(Edit::range_deletion(fix_range))); + + if unused_codes.is_empty() { + diag.help("Remove the unused suppression code"); + } else { + diag.help("Remove the unused suppression codes"); + } + } + + continue; + } + + // All codes are unused let Some(diag) = - context.report_unchecked(&UNUSED_IGNORE_COMMENT, suppression.range) + context.report_unchecked(&UNUSED_IGNORE_COMMENT, suppression.comment_range) else { continue; }; + diag.into_diagnostic(format_args!( - "Unused `{kind}` directive: '{code}'", - kind = suppression.kind, - code = lint.name() + "Unused `{kind}` directive", + kind = suppression.kind )) } SuppressionTarget::Empty => { @@ -268,6 +366,12 @@ fn check_unused_suppressions(context: &mut CheckSuppressionsContext) { )) } }; + + diag.primary_annotation_mut() + .unwrap() + .push_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); + diag.set_fix(remove_comment_fix(suppression, &source)); + diag.help("Remove the unused suppression comment"); } } @@ -469,12 +573,22 @@ pub(crate) struct Suppression { target: SuppressionTarget, kind: SuppressionKind, - /// The range of this specific suppression. - /// This is the same as `comment_range` except for suppression comments that suppress multiple - /// codes. For those, the range is limited to the specific code. + /// The range of the code in this suppression. + /// + /// This is the same as the `comment_range` for the + /// targets [`SuppressionTarget::All`] and [`SuppressionTarget::Empty`]. range: TextRange, /// The range of the suppression comment. + /// + /// This isn't the range of the entire comment if this is a nested comment: + /// + /// ```py + /// a # ty: ignore # fmt: off + /// ^^^^^^^^^^^^^ + /// ``` + /// + /// It doesn't include the range of the nested `# fmt: off` comment. comment_range: TextRange, /// The range for which this suppression applies. @@ -572,7 +686,7 @@ impl<'a> SuppressionsBuilder<'a> { } } - fn add_comment(&mut self, comment: &SuppressionComment, line_range: TextRange) { + fn add_comment(&mut self, comment: SuppressionComment, line_range: TextRange) { // `type: ignore` comments at the start of the file apply to the entire range. // > A # type: ignore comment on a line by itself at the top of a file, before any docstrings, // > imports, or other executable code, silences all errors in the file. @@ -595,7 +709,7 @@ impl<'a> SuppressionsBuilder<'a> { } }; - match comment.codes.as_deref() { + match comment.codes { // `type: ignore` None => { push_type_ignore_suppression(Suppression { @@ -621,7 +735,7 @@ impl<'a> SuppressionsBuilder<'a> { } // `ty: ignore[]` - Some([]) => { + Some(codes) if codes.is_empty() => { self.line.push(Suppression { target: SuppressionTarget::Empty, kind: comment.kind, @@ -634,25 +748,20 @@ impl<'a> SuppressionsBuilder<'a> { // `ty: ignore[a, b]` Some(codes) => { for code_range in codes { - let code = &self.source[*code_range]; - let range = if codes.len() == 1 { - comment.range - } else { - *code_range - }; + let code = &self.source[code_range]; match self.lint_registry.get(code) { Ok(lint) => { self.line.push(Suppression { target: SuppressionTarget::Lint(lint), kind: comment.kind, - range, + range: code_range, comment_range: comment.range, suppressed_range, }); } Err(error) => self.unknown.push(UnknownSuppression { - range, + range: code_range, comment_range: comment.range, reason: error, }), @@ -795,8 +904,6 @@ impl<'src> SuppressionParser<'src> { self.eat_whitespace(); if !self.cursor.eat_char(',') { - self.eat_whitespace(); - if self.cursor.eat_char(']') { break Ok(Some(codes)); } @@ -963,6 +1070,37 @@ enum ParseErrorKind { CodesMissingClosingBracket(SuppressionKind), } +fn remove_comment_fix(suppression: &Suppression, source: &str) -> Fix { + let comment_end = suppression.comment_range.end(); + let comment_start = suppression.comment_range.start(); + let after_comment = &source[comment_end.to_usize()..]; + + if !after_comment.starts_with(['\n', '\r']) { + // For example: `# ty: ignore # fmt: off` + // Don't remove the trailing whitespace up to the `ty: ignore` comment + return Fix::safe_edit(Edit::range_deletion(suppression.comment_range)); + } + + // Remove any leading whitespace before the comment + // to avoid unnecessary trailing whitespace once the comment is removed + let before_comment = &source[..comment_start.to_usize()]; + + let mut leading_len = TextSize::default(); + + for c in before_comment.chars().rev() { + match c { + '\n' | '\r' => break, + c if c.is_whitespace() => leading_len += c.text_len(), + _ => break, + } + } + + Fix::safe_edit(Edit::range_deletion(TextRange::new( + comment_start - leading_len, + comment_end, + ))) +} + #[cfg(test)] mod tests { use crate::suppression::{SuppressionComment, SuppressionParser}; diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 6660f331f1..6744169241 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -12,6 +12,7 @@ license = { workspace = true } [dependencies] ruff_db = { workspace = true, features = ["os"] } +ruff_diagnostics = { workspace = true } ruff_macros = { workspace = true } ruff_notebook = { workspace = true } ruff_python_ast = { workspace = true } diff --git a/crates/ty_server/src/capabilities.rs b/crates/ty_server/src/capabilities.rs index 972e57c09f..82837ec026 100644 --- a/crates/ty_server/src/capabilities.rs +++ b/crates/ty_server/src/capabilities.rs @@ -1,11 +1,12 @@ use lsp_types::{ - ClientCapabilities, CompletionOptions, DeclarationCapability, DiagnosticOptions, - DiagnosticServerCapabilities, HoverProviderCapability, InlayHintOptions, - InlayHintServerCapabilities, MarkupKind, NotebookCellSelector, NotebookSelector, OneOf, - RenameOptions, SelectionRangeProviderCapability, SemanticTokensFullOptions, - SemanticTokensLegend, SemanticTokensOptions, SemanticTokensServerCapabilities, - ServerCapabilities, SignatureHelpOptions, TextDocumentSyncCapability, TextDocumentSyncKind, - TextDocumentSyncOptions, TypeDefinitionProviderCapability, WorkDoneProgressOptions, + ClientCapabilities, CodeActionKind, CodeActionOptions, CompletionOptions, + DeclarationCapability, DiagnosticOptions, DiagnosticServerCapabilities, + HoverProviderCapability, InlayHintOptions, InlayHintServerCapabilities, MarkupKind, + NotebookCellSelector, NotebookSelector, OneOf, RenameOptions, SelectionRangeProviderCapability, + SemanticTokensFullOptions, SemanticTokensLegend, SemanticTokensOptions, + SemanticTokensServerCapabilities, ServerCapabilities, SignatureHelpOptions, + TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, + TypeDefinitionProviderCapability, WorkDoneProgressOptions, }; use crate::PositionEncoding; @@ -376,6 +377,13 @@ pub(crate) fn server_capabilities( ServerCapabilities { position_encoding: Some(position_encoding.into()), + code_action_provider: Some(types::CodeActionProviderCapability::Options( + CodeActionOptions { + code_action_kinds: Some(vec![CodeActionKind::QUICKFIX]), + ..CodeActionOptions::default() + }, + )), + execute_command_provider: Some(types::ExecuteCommandOptions { commands: SupportedCommand::all() .map(|command| command.identifier().to_string()) diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index 326c3eeef9..a5c54e4518 100644 --- a/crates/ty_server/src/server/api.rs +++ b/crates/ty_server/src/server/api.rs @@ -32,6 +32,9 @@ pub(super) fn request(req: server::Request) -> Task { match req.method.as_str() { requests::ExecuteCommand::METHOD => sync_request_task::(req), + requests::CodeActionRequestHandler::METHOD => background_document_request_task::< + requests::CodeActionRequestHandler, + >(req, BackgroundSchedule::Worker), requests::DocumentDiagnosticRequestHandler::METHOD => background_document_request_task::< requests::DocumentDiagnosticRequestHandler, >( diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 9d228ec386..d25e6f5243 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::hash::{DefaultHasher, Hash as _, Hasher as _}; use lsp_types::notification::PublishDiagnostics; @@ -5,18 +6,21 @@ use lsp_types::{ CodeDescription, Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag, NumberOrString, PublishDiagnosticsParams, Url, }; +use ruff_diagnostics::Applicability; +use ruff_text_size::Ranged; use rustc_hash::FxHashMap; use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic}; use ruff_db::files::{File, FileRange}; use ruff_db::system::SystemPathBuf; +use serde::{Deserialize, Serialize}; use ty_project::{Db as _, ProjectDatabase}; -use crate::Db; use crate::document::{FileRangeExt, ToRangeExt}; use crate::session::DocumentHandle; use crate::session::client::Client; use crate::system::{AnySystemPath, file_to_url}; +use crate::{DIAGNOSTIC_NAME, Db}; use crate::{PositionEncoding, Session}; pub(super) struct Diagnostics { @@ -351,6 +355,8 @@ pub(super) fn to_lsp_diagnostic( ); } + let data = DiagnosticData::try_from_diagnostic(db, diagnostic, encoding); + ( url, Diagnostic { @@ -359,10 +365,10 @@ pub(super) fn to_lsp_diagnostic( tags, code: Some(NumberOrString::String(diagnostic.id().to_string())), code_description, - source: Some("ty".into()), + source: Some(DIAGNOSTIC_NAME.into()), message: diagnostic.concise_message().to_string(), related_information: Some(related_information), - data: None, + data: serde_json::to_value(data).ok(), }, ) } @@ -402,3 +408,49 @@ fn sub_diagnostic_to_related_information( message: diagnostic.concise_message().to_string(), }) } + +#[derive(Serialize, Deserialize)] +pub(crate) struct DiagnosticData { + pub(crate) fix_title: String, + pub(crate) edits: HashMap>, +} + +impl DiagnosticData { + fn try_from_diagnostic( + db: &dyn Db, + diagnostic: &ruff_db::diagnostic::Diagnostic, + encoding: PositionEncoding, + ) -> Option { + let fix = diagnostic + .fix() + .filter(|fix| fix.applies(Applicability::Unsafe))?; + + let primary_span = diagnostic.primary_span()?; + let file = primary_span.expect_ty_file(); + + let mut lsp_edits: HashMap> = HashMap::new(); + + for edit in fix.edits() { + let location = edit + .range() + .to_lsp_range(db, file, encoding)? + .to_location()?; + + lsp_edits + .entry(location.uri) + .or_default() + .push(lsp_types::TextEdit { + range: location.range, + new_text: edit.content().unwrap_or_default().to_string(), + }); + } + + Some(Self { + fix_title: diagnostic + .first_help_text() + .map(ToString::to_string) + .unwrap_or_else(|| format!("Fix {}", diagnostic.id())), + edits: lsp_edits, + }) + } +} diff --git a/crates/ty_server/src/server/api/requests.rs b/crates/ty_server/src/server/api/requests.rs index 3d76a55c00..355b0fd859 100644 --- a/crates/ty_server/src/server/api/requests.rs +++ b/crates/ty_server/src/server/api/requests.rs @@ -1,3 +1,4 @@ +mod code_action; mod completion; mod diagnostic; mod doc_highlights; @@ -19,6 +20,7 @@ mod signature_help; mod workspace_diagnostic; mod workspace_symbols; +pub(super) use code_action::CodeActionRequestHandler; pub(super) use completion::CompletionRequestHandler; pub(super) use diagnostic::DocumentDiagnosticRequestHandler; pub(super) use doc_highlights::DocumentHighlightRequestHandler; diff --git a/crates/ty_server/src/server/api/requests/code_action.rs b/crates/ty_server/src/server/api/requests/code_action.rs new file mode 100644 index 0000000000..fd7826d82a --- /dev/null +++ b/crates/ty_server/src/server/api/requests/code_action.rs @@ -0,0 +1,82 @@ +use std::borrow::Cow; + +use lsp_types::{self as types, Url, request as req}; +use ty_project::ProjectDatabase; +use types::{CodeActionKind, CodeActionOrCommand}; + +use crate::DIAGNOSTIC_NAME; +use crate::server::Result; +use crate::server::api::RequestHandler; +use crate::server::api::diagnostics::DiagnosticData; +use crate::server::api::traits::{BackgroundDocumentRequestHandler, RetriableRequestHandler}; +use crate::session::DocumentSnapshot; +use crate::session::client::Client; + +pub(crate) struct CodeActionRequestHandler; + +impl RequestHandler for CodeActionRequestHandler { + type RequestType = req::CodeActionRequest; +} + +impl BackgroundDocumentRequestHandler for CodeActionRequestHandler { + fn document_url(params: &types::CodeActionParams) -> Cow<'_, Url> { + Cow::Borrowed(¶ms.text_document.uri) + } + + fn run_with_snapshot( + _db: &ProjectDatabase, + _snapshot: &DocumentSnapshot, + _client: &Client, + params: types::CodeActionParams, + ) -> Result> { + let diagnostics = params.context.diagnostics; + + let mut actions = Vec::new(); + + for mut diagnostic in diagnostics.into_iter().filter(|diagnostic| { + diagnostic.source.as_deref() == Some(DIAGNOSTIC_NAME) + && range_intersect(&diagnostic.range, ¶ms.range) + }) { + let Some(data) = diagnostic.data.take() else { + continue; + }; + + let data: DiagnosticData = match serde_json::from_value(data) { + Ok(data) => data, + Err(err) => { + tracing::warn!("Failed to deserialize diagnostic data: {err}"); + continue; + } + }; + + actions.push(CodeActionOrCommand::CodeAction(lsp_types::CodeAction { + title: data.fix_title, + kind: Some(CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic]), + edit: Some(lsp_types::WorkspaceEdit { + changes: Some(data.edits), + document_changes: None, + change_annotations: None, + }), + is_preferred: Some(true), + command: None, + disabled: None, + data: None, + })); + } + + if actions.is_empty() { + return Ok(None); + } + + Ok(Some(actions)) + } +} + +fn range_intersect(range: &lsp_types::Range, other: &lsp_types::Range) -> bool { + let start = range.start.max(other.start); + let end = range.end.min(other.end); + end >= start +} + +impl RetriableRequestHandler for CodeActionRequestHandler {} diff --git a/crates/ty_server/tests/e2e/code_actions.rs b/crates/ty_server/tests/e2e/code_actions.rs new file mode 100644 index 0000000000..b33a5eb935 --- /dev/null +++ b/crates/ty_server/tests/e2e/code_actions.rs @@ -0,0 +1,117 @@ +use anyhow::Result; +use lsp_types::{Position, Range, request::CodeActionRequest}; +use ruff_db::system::SystemPath; + +use crate::TestServerBuilder; + +#[test] +fn code_action() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +x = 20 / 2 # ty: ignore[division-by-zero] +"; + + let ty_toml = SystemPath::new("ty.toml"); + let ty_toml_content = "\ +[rules] +unused-ignore-comment = \"warn\" +"; + + let mut server = TestServerBuilder::new()? + .with_workspace(workspace_root, None)? + .with_file(ty_toml, ty_toml_content)? + .with_file(foo, foo_content)? + .enable_pull_diagnostics(true) + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, &foo_content, 1); + + // Wait for diagnostics to be computed. + let diagnostics = server.document_diagnostic_request(foo, None); + + // Get code actions for the line with the unused ignore comment. + let code_action_params = lsp_types::CodeActionParams { + text_document: lsp_types::TextDocumentIdentifier { + uri: server.file_uri(foo), + }, + range: Range::new(Position::new(0, 0), Position::new(0, 43)), + context: lsp_types::CodeActionContext { + diagnostics: match diagnostics { + lsp_types::DocumentDiagnosticReportResult::Report( + lsp_types::DocumentDiagnosticReport::Full(report), + ) => report.full_document_diagnostic_report.items, + _ => panic!("Expected full diagnostic report"), + }, + only: None, + trigger_kind: None, + }, + work_done_progress_params: lsp_types::WorkDoneProgressParams::default(), + partial_result_params: lsp_types::PartialResultParams::default(), + }; + + let code_action_id = server.send_request::(code_action_params); + let code_actions = server.await_response::(&code_action_id); + + insta::assert_json_snapshot!(code_actions); + + Ok(()) +} + +#[test] +fn no_code_action_for_non_overlapping_range_on_same_line() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +x = 20 / 2 # ty: ignore[division-by-zero] +"; + + let ty_toml = SystemPath::new("ty.toml"); + let ty_toml_content = "\ +[rules] +unused-ignore-comment = \"warn\" +"; + + let mut server = TestServerBuilder::new()? + .with_workspace(workspace_root, None)? + .with_file(ty_toml, ty_toml_content)? + .with_file(foo, foo_content)? + .enable_pull_diagnostics(true) + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, &foo_content, 1); + + // Wait for diagnostics to be computed. + let diagnostics = server.document_diagnostic_request(foo, None); + + // Get code actions for a range that doesn't overlap with the diagnostic. + // The diagnostic is at characters 12-42, so we request actions for characters 0-10. + let code_action_params = lsp_types::CodeActionParams { + text_document: lsp_types::TextDocumentIdentifier { + uri: server.file_uri(foo), + }, + range: Range::new(Position::new(0, 0), Position::new(0, 10)), + context: lsp_types::CodeActionContext { + diagnostics: match diagnostics { + lsp_types::DocumentDiagnosticReportResult::Report( + lsp_types::DocumentDiagnosticReport::Full(report), + ) => report.full_document_diagnostic_report.items, + _ => panic!("Expected full diagnostic report"), + }, + only: None, + trigger_kind: None, + }, + work_done_progress_params: lsp_types::WorkDoneProgressParams::default(), + partial_result_params: lsp_types::PartialResultParams::default(), + }; + + let code_action_id = server.send_request::(code_action_params); + let code_actions = server.await_response::(&code_action_id); + + // Should return None because the range doesn't overlap with the diagnostic. + assert_eq!(code_actions, None); + + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/main.rs b/crates/ty_server/tests/e2e/main.rs index 086d24dcc5..8e416d11a3 100644 --- a/crates/ty_server/tests/e2e/main.rs +++ b/crates/ty_server/tests/e2e/main.rs @@ -27,6 +27,7 @@ //! [`await_request`]: TestServer::await_request //! [`await_notification`]: TestServer::await_notification +mod code_actions; mod commands; mod initialize; mod inlay_hints; @@ -740,7 +741,7 @@ impl TestServer { self.initialize_response.as_ref() } - fn file_uri(&self, path: impl AsRef) -> Url { + pub(crate) fn file_uri(&self, path: impl AsRef) -> Url { Url::from_file_path(self.test_context.root().join(path.as_ref()).as_std_path()) .expect("Path must be a valid URL") } diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action.snap b/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action.snap new file mode 100644 index 0000000000..af1fc0a86f --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action.snap @@ -0,0 +1,55 @@ +--- +source: crates/ty_server/tests/e2e/code_actions.rs +expression: code_actions +--- +[ + { + "title": "Remove the unused suppression comment", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 0, + "character": 12 + }, + "end": { + "line": 0, + "character": 42 + } + }, + "severity": 2, + "code": "unused-ignore-comment", + "codeDescription": { + "href": "https://ty.dev/rules#unused-ignore-comment" + }, + "source": "ty", + "message": "Unused `ty: ignore` directive", + "relatedInformation": [], + "tags": [ + 1 + ] + } + ], + "edit": { + "changes": { + "file:///src/foo.py": [ + { + "range": { + "start": { + "line": 0, + "character": 10 + }, + "end": { + "line": 0, + "character": 42 + } + }, + "newText": "" + } + ] + } + }, + "isPreferred": true + } +] diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap index c88a7fa513..79d71626b2 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap @@ -43,6 +43,11 @@ expression: initialization_result "documentHighlightProvider": true, "documentSymbolProvider": true, "workspaceSymbolProvider": true, + "codeActionProvider": { + "codeActionKinds": [ + "quickfix" + ] + }, "declarationProvider": true, "executeCommandProvider": { "commands": [ diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap index c88a7fa513..79d71626b2 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap @@ -43,6 +43,11 @@ expression: initialization_result "documentHighlightProvider": true, "documentSymbolProvider": true, "workspaceSymbolProvider": true, + "codeActionProvider": { + "codeActionKinds": [ + "quickfix" + ] + }, "declarationProvider": true, "executeCommandProvider": { "commands": [ diff --git a/crates/ty_test/src/lib.rs b/crates/ty_test/src/lib.rs index 04eda62a2f..182d4b206e 100644 --- a/crates/ty_test/src/lib.rs +++ b/crates/ty_test/src/lib.rs @@ -641,7 +641,9 @@ fn create_diagnostic_snapshot( test: &parser::MarkdownTest, diagnostics: impl IntoIterator, ) -> String { - let display_config = DisplayDiagnosticConfig::default().color(false); + let display_config = DisplayDiagnosticConfig::default() + .color(false) + .show_fix_diff(true); let mut snapshot = String::new(); writeln!(snapshot).unwrap();