diff --git a/crates/ty/docs/rules.md b/crates/ty/docs/rules.md index 34e4227f11..0f4cc130ae 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_python_semantic/src/diagnostic.rs b/crates/ty_python_semantic/src/diagnostic.rs index a54e7b9083..b8ff2861ea 100644 --- a/crates/ty_python_semantic/src/diagnostic.rs +++ b/crates/ty_python_semantic/src/diagnostic.rs @@ -1,5 +1,11 @@ -use crate::{Db, Program, PythonVersionWithSource}; -use ruff_db::diagnostic::{Annotation, Diagnostic, SubDiagnostic, SubDiagnosticSeverity}; +use crate::{ + Db, Program, PythonVersionWithSource, lint::lint_documentation_url, types::TypeCheckDiagnostics, +}; +use ruff_db::{ + diagnostic::{Annotation, Diagnostic, DiagnosticId, SubDiagnostic, SubDiagnosticSeverity}, + files::File, +}; +use std::cell::RefCell; use std::fmt::Write; /// Suggest a name from `existing_names` that is similar to `wrong_name`. @@ -144,3 +150,129 @@ where buffer } + +/// An abstraction for mutating a diagnostic. +/// +/// Callers likely should use `LintDiagnosticGuard` via +/// `InferContext::report_lint` instead. This guard is only intended for use +/// with non-lint diagnostics or non-type checking diagnostics. It is fundamentally lower level and easier to +/// get things wrong by using it. +/// +/// Unlike `LintDiagnosticGuard`, this API does not guarantee that the +/// constructed `Diagnostic` not only has a primary annotation, but its +/// associated file is equivalent to the file being type checked. As a result, +/// if either is violated, then the `Drop` impl on `DiagnosticGuard` will +/// panic. +pub(super) struct DiagnosticGuard<'sink> { + /// The file of the primary span (to which file does this diagnostic belong). + file: File, + + /// The target where to emit the diagnostic to. + /// + /// We use a [`RefCell`] here over a `&mut TypeCheckDiagnostics` to ensure the fact that + /// `InferContext` (and other contexts with diagnostics) use a [`RefCell`] internally + /// remains abstracted away. Specifically, we want to ensure that calling `report_lint` on + /// `InferContext` twice doesn't result in a panic: + /// + /// ```ignore + /// let diag1 = context.report_lint(...); + /// + /// // would panic if using a `&mut TypeCheckDiagnostics` + /// // because of a second mutable borrow. + /// let diag2 = context.report_lint(...); + /// ``` + sink: &'sink RefCell, + + /// The diagnostic that we want to report. + /// + /// This is always `Some` until the `Drop` impl. + diag: Option, +} + +impl<'sink> DiagnosticGuard<'sink> { + pub(crate) fn new( + file: File, + sink: &'sink std::cell::RefCell, + diag: Diagnostic, + ) -> Self { + Self { + file, + sink, + diag: Some(diag), + } + } +} + +impl std::ops::Deref for DiagnosticGuard<'_> { + type Target = Diagnostic; + + fn deref(&self) -> &Diagnostic { + // OK because `self.diag` is only `None` within `Drop`. + self.diag.as_ref().unwrap() + } +} + +/// Return a mutable borrow of the diagnostic in this guard. +/// +/// Callers may mutate the diagnostic to add new sub-diagnostics +/// or annotations. +/// +/// The diagnostic is added to the typing context, if appropriate, +/// when this guard is dropped. +impl std::ops::DerefMut for DiagnosticGuard<'_> { + fn deref_mut(&mut self) -> &mut Diagnostic { + // OK because `self.diag` is only `None` within `Drop`. + self.diag.as_mut().unwrap() + } +} + +/// Finishes use of this guard. +/// +/// This will add the diagnostic to the typing context if appropriate. +/// +/// # Panics +/// +/// This panics when the underlying diagnostic lacks a primary +/// annotation, or if it has one and its file doesn't match the file +/// being type checked. +impl Drop for DiagnosticGuard<'_> { + fn drop(&mut self) { + if std::thread::panicking() { + // Don't submit diagnostics when panicking because they might be incomplete. + return; + } + + // OK because the only way `self.diag` is `None` + // is via this impl, which can only run at most + // once. + let mut diag = self.diag.take().unwrap(); + + let Some(ann) = diag.primary_annotation() else { + panic!( + "All diagnostics reported by `InferContext` must have a \ + primary annotation, but diagnostic {id} does not", + id = diag.id(), + ); + }; + + let expected_file = self.file; + let got_file = ann.get_span().expect_ty_file(); + assert_eq!( + expected_file, + got_file, + "All diagnostics reported by `InferContext` must have a \ + primary annotation whose file matches the file of the \ + current typing context, but diagnostic {id} has file \ + {got_file:?} and we expected {expected_file:?}", + id = diag.id(), + ); + + if let DiagnosticId::Lint(lint_name) = diag.id() + && diag.documentation_url().is_none() + { + diag.set_documentation_url(Some(lint_documentation_url(lint_name))); + } + + self.sink.borrow_mut().push(diag); + } +} diff --git a/crates/ty_python_semantic/src/suppression.rs b/crates/ty_python_semantic/src/suppression.rs index 6148e9744f..e337200f94 100644 --- a/crates/ty_python_semantic/src/suppression.rs +++ b/crates/ty_python_semantic/src/suppression.rs @@ -1,7 +1,10 @@ +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, Span}; +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; @@ -133,12 +136,18 @@ pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { builder.finish() } -pub(crate) fn check_suppressions(db: &dyn Db, file: File, diagnostics: &mut TypeCheckDiagnostics) { +pub(crate) fn check_suppressions( + db: &dyn Db, + file: File, + diagnostics: TypeCheckDiagnostics, +) -> Vec { let mut context = CheckSuppressionsContext::new(db, file, diagnostics); check_unknown_rule(&mut context); check_invalid_suppression(&mut context); check_unused_suppressions(&mut context); + + context.diagnostics.into_inner().into_diagnostics() } /// Checks for `ty: ignore` comments that reference unknown rules. @@ -148,7 +157,9 @@ fn check_unknown_rule(context: &mut CheckSuppressionsContext) { } for unknown in &context.suppressions.unknown { - context.report_lint(&IGNORE_COMMENT_UNKNOWN_RULE, unknown.range, &unknown.reason); + if let Some(diag) = context.report_lint(&IGNORE_COMMENT_UNKNOWN_RULE, unknown.range) { + diag.into_diagnostic(&unknown.reason); + } } } @@ -158,15 +169,13 @@ fn check_invalid_suppression(context: &mut CheckSuppressionsContext) { } for invalid in &context.suppressions.invalid { - context.report_lint( - &INVALID_IGNORE_COMMENT, - invalid.error.range, - format_args!( + if let Some(diag) = context.report_lint(&INVALID_IGNORE_COMMENT, invalid.error.range) { + diag.into_diagnostic(format_args!( "Invalid `{kind}` comment: {reason}", kind = invalid.kind, reason = &invalid.error - ), - ); + )); + } } } @@ -179,17 +188,19 @@ fn check_unused_suppressions(context: &mut CheckSuppressionsContext) { return; } + let diagnostics = context.diagnostics.get_mut(); + let all = context.suppressions; let mut unused = Vec::with_capacity( all.file .len() .saturating_add(all.line.len()) - .saturating_sub(context.diagnostics.used_len()), + .saturating_sub(diagnostics.used_len()), ); // Collect all suppressions that are unused after type-checking. for suppression in all { - if context.diagnostics.is_used(suppression.id()) { + if diagnostics.is_used(suppression.id()) { continue; } @@ -203,7 +214,7 @@ fn check_unused_suppressions(context: &mut CheckSuppressionsContext) { // A `unused-ignore-comment` suppression can't ignore itself. // It can only ignore other suppressions. if unused_suppression.id() != suppression.id() { - context.diagnostics.mark_used(unused_suppression.id()); + diagnostics.mark_used(unused_suppression.id()); continue; } } @@ -211,37 +222,52 @@ fn check_unused_suppressions(context: &mut CheckSuppressionsContext) { unused.push(suppression); } - for suppression in unused { + 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] // ``` - if context.diagnostics.is_used(suppression.id()) { - continue; - } + !context.is_suppression_used(suppression.id()) + }); + for suppression in unused { match suppression.target { - SuppressionTarget::All => context.report_unchecked( - &UNUSED_IGNORE_COMMENT, - suppression.range, - format_args!("Unused blanket `{}` directive", suppression.kind), - ), - SuppressionTarget::Lint(lint) => context.report_unchecked( - &UNUSED_IGNORE_COMMENT, - suppression.range, - format_args!( + 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) => { + let Some(diag) = + context.report_unchecked(&UNUSED_IGNORE_COMMENT, suppression.range) + else { + continue; + }; + diag.into_diagnostic(format_args!( "Unused `{kind}` directive: '{code}'", kind = suppression.kind, code = lint.name() - ), - ), - SuppressionTarget::Empty => context.report_unchecked( - &UNUSED_IGNORE_COMMENT, - suppression.range, - format_args!("Unused `{kind}` without a code", kind = suppression.kind), - ), - } + )) + } + SuppressionTarget::Empty => { + let Some(diag) = + context.report_unchecked(&UNUSED_IGNORE_COMMENT, suppression.range) + else { + continue; + }; + diag.into_diagnostic(format_args!( + "Unused `{kind}` without a code", + kind = suppression.kind + )) + } + }; } } @@ -249,17 +275,17 @@ struct CheckSuppressionsContext<'a> { db: &'a dyn Db, file: File, suppressions: &'a Suppressions, - diagnostics: &'a mut TypeCheckDiagnostics, + diagnostics: std::cell::RefCell, } impl<'a> CheckSuppressionsContext<'a> { - fn new(db: &'a dyn Db, file: File, diagnostics: &'a mut TypeCheckDiagnostics) -> Self { + fn new(db: &'a dyn Db, file: File, diagnostics: TypeCheckDiagnostics) -> Self { let suppressions = suppressions(db, file); Self { db, file, suppressions, - diagnostics, + diagnostics: diagnostics.into(), } } @@ -270,38 +296,77 @@ impl<'a> CheckSuppressionsContext<'a> { .is_enabled(LintId::of(lint)) } - fn report_lint( - &mut self, + fn is_suppression_used(&self, id: FileSuppressionId) -> bool { + self.diagnostics.borrow().is_used(id) + } + + fn report_lint<'ctx>( + &'ctx self, lint: &'static LintMetadata, range: TextRange, - message: impl IntoDiagnosticMessage, - ) { + ) -> Option> { if let Some(suppression) = self.suppressions.find_suppression(range, LintId::of(lint)) { - self.diagnostics.mark_used(suppression.id()); - return; + self.diagnostics.borrow_mut().mark_used(suppression.id()); + return None; } - self.report_unchecked(lint, range, message); + self.report_unchecked(lint, range) } /// Reports a diagnostic without checking if the lint at the given range is suppressed or marking /// the suppression as used. - fn report_unchecked( - &mut self, + fn report_unchecked<'ctx>( + &'ctx self, lint: &'static LintMetadata, range: TextRange, - message: impl IntoDiagnosticMessage, - ) { - let Some(severity) = self.db.rule_selection(self.file).severity(LintId::of(lint)) else { - return; - }; + ) -> Option> { + SuppressionDiagnosticGuardBuilder::new(self, lint, range) + } +} - let id = DiagnosticId::Lint(lint.name()); - let mut diag = Diagnostic::new(id, severity, message); - diag.set_documentation_url(Some(lint.documentation_url())); - let span = Span::from(self.file).with_range(range); - diag.annotate(Annotation::primary(span)); - self.diagnostics.push(diag); +/// A builder for constructing a diagnostic guard. +/// +/// This type exists to separate the phases of "check if a diagnostic should +/// be reported" and "build the actual diagnostic." +pub(crate) struct SuppressionDiagnosticGuardBuilder<'ctx, 'db> { + ctx: &'ctx CheckSuppressionsContext<'db>, + id: DiagnosticId, + range: TextRange, + severity: Severity, +} + +impl<'ctx, 'db> SuppressionDiagnosticGuardBuilder<'ctx, 'db> { + fn new( + ctx: &'ctx CheckSuppressionsContext<'db>, + lint: &'static LintMetadata, + range: TextRange, + ) -> Option { + let severity = ctx.db.rule_selection(ctx.file).severity(LintId::of(lint))?; + + Some(Self { + ctx, + id: DiagnosticId::Lint(lint.name()), + severity, + range, + }) + } + + /// Create a new guard. + /// + /// This initializes a new diagnostic using the given message along with + /// the ID and severity used to create this builder. + /// + /// The diagnostic can be further mutated on the guard via its `DerefMut` + /// impl to `Diagnostic`. + pub(crate) fn into_diagnostic( + self, + message: impl IntoDiagnosticMessage, + ) -> DiagnosticGuard<'ctx> { + let mut diag = Diagnostic::new(self.id, self.severity, message); + + let primary_span = Span::from(self.ctx.file).with_range(self.range); + diag.annotate(Annotation::primary(primary_span)); + DiagnosticGuard::new(self.ctx.file, &self.ctx.diagnostics, diag) } } diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index f90810d5e3..b90ac4b01a 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -142,7 +142,7 @@ pub fn check_types(db: &dyn Db, file: File) -> Vec { .map(|error| Diagnostic::invalid_syntax(file, error, error)), ); - check_suppressions(db, file, &mut diagnostics); + let diagnostics = check_suppressions(db, file, diagnostics); let elapsed = start.elapsed(); if elapsed >= Duration::from_millis(100) { @@ -152,7 +152,7 @@ pub fn check_types(db: &dyn Db, file: File) -> Vec { ); } - diagnostics.into_diagnostics() + diagnostics } /// Infer the type of a binding. diff --git a/crates/ty_python_semantic/src/types/context.rs b/crates/ty_python_semantic/src/types/context.rs index 6ef5afcdf5..7eb84e0d01 100644 --- a/crates/ty_python_semantic/src/types/context.rs +++ b/crates/ty_python_semantic/src/types/context.rs @@ -11,7 +11,8 @@ use ruff_text_size::{Ranged, TextRange}; use super::{Type, TypeCheckDiagnostics, binding_type}; -use crate::lint::{LintSource, lint_documentation_url}; +use crate::diagnostic::DiagnosticGuard; +use crate::lint::LintSource; use crate::semantic_index::scope::ScopeId; use crate::semantic_index::semantic_index; use crate::types::function::FunctionDecorators; @@ -398,7 +399,7 @@ pub(super) struct LintDiagnosticGuardBuilder<'db, 'ctx> { id: LintId, severity: Severity, source: LintSource, - primary_span: Span, + primary_range: TextRange, } impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { @@ -452,13 +453,12 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { return None; } - let primary_span = Span::from(ctx.file()).with_range(range); Some(LintDiagnosticGuardBuilder { ctx, id: lint_id, severity, source, - primary_span, + primary_range: range, }) } @@ -483,7 +483,8 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { // the optional message can be added later. We could accept it here // in this `build` method, but we already accept the main diagnostic // message. So the messages are likely to be quite confusable. - diag.annotate(Annotation::primary(self.primary_span.clone())); + let primary_span = Span::from(self.ctx.file()).with_range(self.primary_range); + diag.annotate(Annotation::primary(primary_span)); LintDiagnosticGuard { ctx: self.ctx, source: self.source, @@ -492,101 +493,6 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { } } -/// An abstraction for mutating a diagnostic. -/// -/// Callers can build this guard by starting with -/// `InferContext::report_diagnostic`. -/// -/// Callers likely should use `LintDiagnosticGuard` via -/// `InferContext::report_lint` instead. This guard is only intended for use -/// with non-lint diagnostics. It is fundamentally lower level and easier to -/// get things wrong by using it. -/// -/// Unlike `LintDiagnosticGuard`, this API does not guarantee that the -/// constructed `Diagnostic` not only has a primary annotation, but its -/// associated file is equivalent to the file being type checked. As a result, -/// if either is violated, then the `Drop` impl on `DiagnosticGuard` will -/// panic. -pub(super) struct DiagnosticGuard<'db, 'ctx> { - ctx: &'ctx InferContext<'db, 'ctx>, - /// The diagnostic that we want to report. - /// - /// This is always `Some` until the `Drop` impl. - diag: Option, -} - -impl std::ops::Deref for DiagnosticGuard<'_, '_> { - type Target = Diagnostic; - - fn deref(&self) -> &Diagnostic { - // OK because `self.diag` is only `None` within `Drop`. - self.diag.as_ref().unwrap() - } -} - -/// Return a mutable borrow of the diagnostic in this guard. -/// -/// Callers may mutate the diagnostic to add new sub-diagnostics -/// or annotations. -/// -/// The diagnostic is added to the typing context, if appropriate, -/// when this guard is dropped. -impl std::ops::DerefMut for DiagnosticGuard<'_, '_> { - fn deref_mut(&mut self) -> &mut Diagnostic { - // OK because `self.diag` is only `None` within `Drop`. - self.diag.as_mut().unwrap() - } -} - -/// Finishes use of this guard. -/// -/// This will add the diagnostic to the typing context if appropriate. -/// -/// # Panics -/// -/// This panics when the underlying diagnostic lacks a primary -/// annotation, or if it has one and its file doesn't match the file -/// being type checked. -impl Drop for DiagnosticGuard<'_, '_> { - fn drop(&mut self) { - if std::thread::panicking() { - // Don't submit diagnostics when panicking because they might be incomplete. - return; - } - - // OK because the only way `self.diag` is `None` - // is via this impl, which can only run at most - // once. - let diag = self.diag.take().unwrap(); - - if std::thread::panicking() { - // Don't submit diagnostics when panicking because they might be incomplete. - return; - } - - let Some(ann) = diag.primary_annotation() else { - panic!( - "All diagnostics reported by `InferContext` must have a \ - primary annotation, but diagnostic {id} does not", - id = diag.id(), - ); - }; - - let expected_file = self.ctx.file(); - let got_file = ann.get_span().expect_ty_file(); - assert_eq!( - expected_file, - got_file, - "All diagnostics reported by `InferContext` must have a \ - primary annotation whose file matches the file of the \ - current typing context, but diagnostic {id} has file \ - {got_file:?} and we expected {expected_file:?}", - id = diag.id(), - ); - self.ctx.diagnostics.borrow_mut().push(diag); - } -} - /// A builder for constructing a diagnostic guard. /// /// This type exists to separate the phases of "check if a diagnostic should @@ -625,19 +531,9 @@ impl<'db, 'ctx> DiagnosticGuardBuilder<'db, 'ctx> { /// /// The diagnostic can be further mutated on the guard via its `DerefMut` /// impl to `Diagnostic`. - pub(super) fn into_diagnostic( - self, - message: impl std::fmt::Display, - ) -> DiagnosticGuard<'db, 'ctx> { - let mut diag = Diagnostic::new(self.id, self.severity, message); + pub(super) fn into_diagnostic(self, message: impl std::fmt::Display) -> DiagnosticGuard<'ctx> { + let diag = Diagnostic::new(self.id, self.severity, message); - if let DiagnosticId::Lint(lint_name) = diag.id() { - diag.set_documentation_url(Some(lint_documentation_url(lint_name))); - } - - DiagnosticGuard { - ctx: self.ctx, - diag: Some(diag), - } + DiagnosticGuard::new(self.ctx.file, &self.ctx.diagnostics, diag) } }