diff --git a/crates/red_knot_python_semantic/src/types/call/bind.rs b/crates/red_knot_python_semantic/src/types/call/bind.rs index 0e7f049116..8cbb2c2aa8 100644 --- a/crates/red_knot_python_semantic/src/types/call/bind.rs +++ b/crates/red_knot_python_semantic/src/types/call/bind.rs @@ -1248,18 +1248,18 @@ impl<'db> BindingError<'db> { expected_ty, provided_ty, } => { - let Some(builder) = context.report_lint(&INVALID_ARGUMENT_TYPE) else { + let range = Self::get_node(node, *argument_index); + let Some(builder) = context.report_lint(&INVALID_ARGUMENT_TYPE, range) else { return; }; let provided_ty_display = provided_ty.display(context.db()); let expected_ty_display = expected_ty.display(context.db()); - let mut diag = builder.build("Argument to this function is incorrect"); - let span = context.span(Self::get_node(node, *argument_index)); - diag.annotate(Annotation::primary(span).message(format_args!( + let mut diag = builder.into_diagnostic("Argument to this function is incorrect"); + diag.set_primary_message(format_args!( "Expected `{expected_ty_display}`, found `{provided_ty_display}`" - ))); + )); if let Some((name_span, parameter_span)) = Self::parameter_span_from_index(context.db(), callable_ty, parameter.index) { diff --git a/crates/red_knot_python_semantic/src/types/context.rs b/crates/red_knot_python_semantic/src/types/context.rs index bfbca28803..5f92be04ab 100644 --- a/crates/red_knot_python_semantic/src/types/context.rs +++ b/crates/red_knot_python_semantic/src/types/context.rs @@ -2,10 +2,10 @@ use std::fmt; use drop_bomb::DebugDropBomb; use ruff_db::{ - diagnostic::{Annotation, Diagnostic, DiagnosticId, Severity, Span}, + diagnostic::{Annotation, Diagnostic, DiagnosticId, IntoDiagnosticMessage, Severity, Span}, files::File, }; -use ruff_text_size::Ranged; +use ruff_text_size::{Ranged, TextRange}; use super::{binding_type, Type, TypeCheckDiagnostics}; @@ -82,51 +82,60 @@ impl<'db> InferContext<'db> { ) where T: Ranged, { - let Some(builder) = self.report_lint(lint) else { + let Some(builder) = self.report_lint(lint, ranged) else { return; }; - let mut diag = builder.build(""); - let span = Span::from(self.file).with_range(ranged.range()); - diag.annotate(Annotation::primary(span).message(message)); + let mut diag = builder.into_diagnostic(""); + diag.set_primary_message(message); } - /// Optionally return a reporter builder for adding a lint diagnostic. + /// Optionally return a builder for a lint diagnostic guard. /// - /// If the current context believes a diagnostic should be reported for the - /// given lint, then a reporter builder is returned that enables building - /// a diagnostic. The severity of the diagnostic returned is automatically - /// determined by the given lint and configuration. The message given is - /// used to construct the initial diagnostic and should be considered the - /// "primary message" of the diagnostic. (i.e., If nothing else about the - /// diagnostic is seen, aside from its identifier, the message is probably - /// the thing you'd pick to show.) + /// If the current context believes a diagnostic should be reported for + /// the given lint, then a builder is returned that enables building a + /// lint diagnostic guard. The guard can then be used, via its `DerefMut` + /// implementation, to directly mutate a `Diagnostic`. /// - /// After using the builder to make a reporter, once the reporter is - /// dropped, the diagnostic is added to the context, unless there is - /// something in the diagnostic that excludes it. For example, if a - /// diagnostic's primary span is covered by a suppression, then the - /// constructed diagnostic will be ignored. + /// The severity of the diagnostic returned is automatically determined + /// by the given lint and configuration. The message given to + /// `LintDiagnosticGuardBuilder::to_diagnostic` is used to construct the + /// initial diagnostic and should be considered the "top-level message" of + /// the diagnostic. (i.e., If nothing else about the diagnostic is seen, + /// aside from its identifier, the message is probably the thing you'd pick + /// to show.) /// - /// If callers need to create a non-lint diagnostic, you'll want to use - /// the lower level `InferContext::report_diagnostic` routine. - pub(super) fn report_lint<'ctx>( + /// The diagnostic constructed also includes a primary annotation with a + /// `Span` derived from the range given attached to the `File` in this + /// typing context. (That means the range given _must_ be valid for the + /// `File` currently being type checked.) This primary annotation does + /// not have a message attached to it, but callers can attach one via + /// `LintDiagnosticGuard::set_primary_message`. + /// + /// After using the builder to make a guard, once the guard is dropped, the + /// diagnostic is added to the context, unless there is something in the + /// diagnostic that excludes it. (Currently, no such conditions exist.) + /// + /// If callers need to create a non-lint diagnostic, you'll want to use the + /// lower level `InferContext::report_diagnostic` routine. + pub(super) fn report_lint<'ctx, T: Ranged>( &'ctx self, lint: &'static LintMetadata, + ranged: T, ) -> Option> { - LintDiagnosticGuardBuilder::new(self, lint) + LintDiagnosticGuardBuilder::new(self, lint, ranged.range()) } - /// Optionally return a reporter builder for adding a diagnostic. + /// Optionally return a builder for a diagnostic guard. /// - /// This only returns a reporter builder if the current context - /// allows a diagnostic with the given information to be added. - /// In general, the requirements here are quite a bit less than - /// for `InferContext::report_lint`, since this routine doesn't take rule - /// selection into account. + /// This only returns a builder if the current context allows a diagnostic + /// with the given information to be added. In general, the requirements + /// here are quite a bit less than for `InferContext::report_lint`, since + /// this routine doesn't take rule selection into account (among other + /// things). /// - /// After using the builder to make a reporter, once the reporter is - /// dropped, the diagnostic is added to the context, unless there is - /// something in the diagnostic that excludes it. + /// After using the builder to make a guard, once the guard is dropped, the + /// diagnostic is added to the context, unless there is something in the + /// diagnostic that excludes it. (Currently, no such conditions exist.) /// /// Callers should generally prefer adding a lint diagnostic via /// `InferContext::report_lint` whenever possible. @@ -202,30 +211,17 @@ pub(crate) enum InNoTypeCheck { Yes, } -/// An abstraction for reporting lints as diagnostics. +/// An abstraction for mutating a diagnostic through the lense of a lint. /// -/// Callers can build a reporter via `InferContext::report_lint`. +/// Callers can build this guard by starting with `InferContext::report_lint`. /// -/// A reporter encapsulates the logic for determining if a diagnostic *should* -/// be reported according to the environment, configuration, and any relevant -/// suppressions. The advantage of this reporter is that callers may avoid -/// doing extra work to populate a diagnostic if it is known that it would be -/// otherwise ignored. +/// There are two primary functions of this guard, which mutably derefs to +/// a `Diagnostic`: /// -/// The diagnostic is added to the typing context, if appropriate, when this -/// reporter is dropped. That is, there are two different filtering points: -/// -/// * Building the reporter may return `None` if the initial information given -/// is sufficient to determine that the diagnostic will not be shown to end -/// users. -/// * Dropping the reporter may ignore the diagnostic if information inside the -/// diagnostic itself (like its span positions) indicates that it should be -/// suppressed. -/// -/// If callers need to report a diagnostic with an identifier type other -/// than `DiagnosticId::Lint`, then they should use the more general -/// `InferContext::report_diagnostic` API. But note that this API will not take -/// rule selection or suppressions into account. +/// * On `Drop`, the underlying diagnostic is added to the typing context. +/// * Some convenience methods for mutating the underlying `Diagnostic` +/// in lint context. For example, `LintDiagnosticGuard::set_primary_message` +/// will attach a message to the primary span on the diagnostic. pub(super) struct LintDiagnosticGuard<'db, 'ctx> { /// The typing context. ctx: &'ctx InferContext<'db>, @@ -233,10 +229,44 @@ pub(super) struct LintDiagnosticGuard<'db, 'ctx> { /// /// This is always `Some` until the `Drop` impl. diag: Option, - /// The lint ID. Stored here because it doesn't - /// seem easily derivable from `DiagnosticId` and - /// because we need it for looking up suppressions. - lint_id: LintId, +} + +impl LintDiagnosticGuard<'_, '_> { + /// Set the message on the primary annotation for this diagnostic. + /// + /// If a message already exists on the primary annotation, then this + /// overwrites the existing message. + /// + /// This message is associated with the primary annotation created + /// for every `Diagnostic` that uses the `LintDiagnosticGuard` API. + /// Specifically, the annotation is derived from the `TextRange` given to + /// the `InferContext::report_lint` API. + /// + /// Callers can add additional primary or secondary annotations via the + /// `DerefMut` trait implementation to a `Diagnostic`. + pub(super) fn set_primary_message(&mut self, message: impl IntoDiagnosticMessage) { + // N.B. It is normally bad juju to define `self` methods + // on types that implement `Deref`. Instead, it's idiomatic + // to do `fn foo(this: &mut LintDiagnosticGuard)`, which in + // turn forces callers to use + // `LintDiagnosticGuard(&mut guard, message)`. But this is + // supremely annoying for what is expected to be a common + // case. + // + // Moreover, most of the downside that comes from these sorts + // of methods is a semver hazard. Because the deref target type + // could also define a method by the same name, and that leads + // to confusion. But we own all the code involved here and + // there is no semver boundary. So... ¯\_(ツ)_/¯ ---AG + + // OK because we know the diagnostic was constructed with a single + // primary annotation that will always come before any other annotation + // in the diagnostic. (This relies on the `Diagnostic` API not exposing + // any methods for removing annotations or re-ordering them, which is + // true as of 2025-04-11.) + let ann = self.primary_annotation_mut().unwrap(); + ann.set_message(message); + } } impl std::ops::Deref for LintDiagnosticGuard<'_, '_> { @@ -248,6 +278,13 @@ impl std::ops::Deref for LintDiagnosticGuard<'_, '_> { } } +/// 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 LintDiagnosticGuard<'_, '_> { fn deref_mut(&mut self) -> &mut Diagnostic { // OK because `self.diag` is only `None` within `Drop`. @@ -255,13 +292,60 @@ impl std::ops::DerefMut for LintDiagnosticGuard<'_, '_> { } } -/// Finishes use of this reporter. +/// Finishes use of this guard. /// /// This will add the lint as a diagnostic to the typing context if /// appropriate. The diagnostic may be skipped, for example, if there is a /// relevant suppression. impl Drop for LintDiagnosticGuard<'_, '_> { fn drop(&mut self) { + // 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(); + self.ctx.diagnostics.borrow_mut().push(diag); + } +} + +/// A builder for constructing a lint diagnostic guard. +/// +/// This type exists to separate the phases of "check if a diagnostic should +/// be reported" and "build the actual diagnostic." It's why, for example, +/// `InferContext::report_lint` only requires a `LintMetadata` (and a range), +/// but this builder further requires a message before one can mutate the +/// diagnostic. This is because the `LintMetadata` can be used to derive +/// the diagnostic ID and its severity (based on configuration). Combined +/// with a message you get the minimum amount of data required to build a +/// `Diagnostic`. +/// +/// Additionally, the range is used to construct a primary annotation (without +/// a message) using the file current being type checked. The range given to +/// `InferContext::report_lint` must be from the file currently being type +/// checked. +/// +/// If callers need to report a diagnostic with an identifier type other +/// than `DiagnosticId::Lint`, then they should use the more general +/// `InferContext::report_diagnostic` API. But note that this API will not take +/// rule selection or suppressions into account. +/// +/// # When is the diagnostic added? +/// +/// When a builder is not returned by `InferContext::report_lint`, then +/// it is known that the diagnostic should not be reported. This can happen +/// when the diagnostic is disabled or suppressed (among other reasons). +pub(super) struct LintDiagnosticGuardBuilder<'db, 'ctx> { + ctx: &'ctx InferContext<'db>, + id: DiagnosticId, + severity: Severity, + primary_span: Span, +} + +impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { + fn new( + ctx: &'ctx InferContext<'db>, + lint: &'static LintMetadata, + range: TextRange, + ) -> Option> { // The comment below was copied from the original // implementation of diagnostic reporting. The code // has been refactored, but this still kind of looked @@ -273,69 +357,6 @@ impl Drop for LintDiagnosticGuard<'_, '_> { // returns a rule selector for a given file that respects the package's settings, // any global pragma comments in the file, and any per-file-ignores. - // 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(); - let Some(span) = diag.primary_span() else { - self.ctx.diagnostics.borrow_mut().push(diag); - return; - }; - let Some(range) = span.range() else { - self.ctx.diagnostics.borrow_mut().push(diag); - return; - }; - // Two things to note here. - // - // First is that we use `span.file()` here to find suppressions - // and *not* self.reporter.ctx.file` (as this code used to do). - // The reasoning for this is that the suppression ought to be - // based on the primary span of the diagnostic itself, and not - // the file that happens to be checked currently. It seems likely - // that in most (all?) cases, these will be the same. But in a - // hypothetical case where it isn't, this seems like the more - // sensible option. - // - // Second is that we are only checking suppressions here when - // a range is present. But it seems like we could check - // suppressions even when a range isn't present, since they - // could be file-level suppressions. However, it's not clear, - // in practice, when this matters. In particular, it is generally - // expected that most (all?) lint diagnostics will come with at - // least one primary span. - let suppressions = suppressions(self.ctx.db, span.file()); - if let Some(suppression) = suppressions.find_suppression(range, self.lint_id) { - self.ctx - .diagnostics - .borrow_mut() - .mark_used(suppression.id()); - } else { - self.ctx.diagnostics.borrow_mut().push(diag); - } - } -} - -/// A builder for constructing a lint diagnostic reporter. -/// -/// This type exists to separate the phases of "check if a diagnostic should -/// be reported" and "build the actual diagnostic." It's why, for example, -/// `InferContext::report_lint` only requires a `LintMetadata`, but this builder -/// further requires a message before one can mutate the diagnostic. This is -/// because the `LintMetadata` can be used to derive the diagnostic ID and its -/// severity (based on configuration). Combined with a message you get the -/// minimum amount of data required to build a `Diagnostic`. -pub(super) struct LintDiagnosticGuardBuilder<'db, 'ctx> { - ctx: &'ctx InferContext<'db>, - id: DiagnosticId, - severity: Severity, - lint_id: LintId, -} - -impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { - fn new( - ctx: &'ctx InferContext<'db>, - lint: &'static LintMetadata, - ) -> Option> { if !ctx.db.is_file_open(ctx.file) { return None; } @@ -349,48 +370,66 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> { return None; } let id = DiagnosticId::Lint(lint.name()); + + let suppressions = suppressions(ctx.db(), ctx.file()); + if let Some(suppression) = suppressions.find_suppression(range, lint_id) { + ctx.diagnostics.borrow_mut().mark_used(suppression.id()); + return None; + } + + let primary_span = Span::from(ctx.file()).with_range(range); Some(LintDiagnosticGuardBuilder { ctx, id, severity, - lint_id, + primary_span, }) } - /// Create a new lint reporter. + /// Create a new lint diagnostic guard. /// /// This initializes a new diagnostic using the given message along with - /// the ID and severity derived from the `LintMetadata` used to create this - /// builder. + /// the ID and severity derived from the `LintMetadata` used to create + /// this builder. The diagnostic also includes a primary annotation + /// without a message. To add a message to this primary annotation, use + /// `LintDiagnosticGuard::set_primary_message`. /// - /// The diagnostic can be further mutated via - /// `LintReporter::diagnostic`. + /// The diagnostic can be further mutated on the guard via its `DerefMut` + /// impl to `Diagnostic`. #[must_use] - pub(super) fn build(self, message: impl std::fmt::Display) -> LintDiagnosticGuard<'db, 'ctx> { - let diag = Some(Diagnostic::new(self.id, self.severity, message)); + pub(super) fn into_diagnostic( + self, + message: impl std::fmt::Display, + ) -> LintDiagnosticGuard<'db, 'ctx> { + let mut diag = Diagnostic::new(self.id, self.severity, message); + // This is why `LintDiagnosticGuard::set_primary_message` exists. + // We add the primary annotation here (because it's required), but + // 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())); LintDiagnosticGuard { ctx: self.ctx, - diag, - lint_id: self.lint_id, + diag: Some(diag), } } } -/// An abstraction for reporting diagnostics. +/// An abstraction for mutating a diagnostic. /// -/// Callers can build a reporter via `InferContext::report_diagnostic`. +/// Callers can build this guard by starting with +/// `InferContext::report_diagnostic`. /// -/// A reporter encapsulates the logic for determining if a diagnostic *should* -/// be reported according to the environment, configuration, and any relevant -/// suppressions. The advantage of this reporter is that callers may avoid -/// doing extra work to populate a diagnostic if it is known that it would be -/// otherwise ignored. +/// 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. /// -/// The diagnostic is added to the typing context, if appropriate, when this -/// reporter is dropped. -/// -/// Callers likely should use `LintReporter` via `InferContext::report_lint` -/// instead. This reporter is only intended for use with non-lint diagnostics. +/// 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>, /// The diagnostic that we want to report. @@ -408,13 +447,13 @@ impl std::ops::Deref for DiagnosticGuard<'_, '_> { } } -/// Return a mutable borrow of the diagnostic on this reporter. +/// 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 reporter is dropped. +/// 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`. @@ -422,20 +461,46 @@ impl std::ops::DerefMut for DiagnosticGuard<'_, '_> { } } -/// Finishes use of this reporter. +/// Finishes use of this guard. /// /// This will add the diagnostic to the typing context if appropriate. +/// +/// # Panics +/// +/// This panics when the 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) { // 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(); + + 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().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 reporter. +/// 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." It's why, for example, @@ -461,13 +526,13 @@ impl<'db, 'ctx> DiagnosticGuardBuilder<'db, 'ctx> { Some(DiagnosticGuardBuilder { ctx, id, severity }) } - /// Create a new reporter. + /// 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 via - /// `DiagnosticReporter::diagnostic`. + /// The diagnostic can be further mutated on the guard via its `DerefMut` + /// impl to `Diagnostic`. #[must_use] pub(super) fn into_diagnostic( self, diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index e4d7ab4329..ccfa6d8095 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -1068,19 +1068,18 @@ pub(super) fn report_invalid_return_type( expected_ty: Type, actual_ty: Type, ) { - let Some(builder) = context.report_lint(&INVALID_RETURN_TYPE) else { + let Some(builder) = context.report_lint(&INVALID_RETURN_TYPE, object_range) else { return; }; - let object_span = Span::from(context.file()).with_range(object_range.range()); let return_type_span = Span::from(context.file()).with_range(return_type_range.range()); - let mut diag = builder.build("Return type does not match returned value"); - diag.annotate(Annotation::primary(object_span).message(format_args!( + let mut diag = builder.into_diagnostic("Return type does not match returned value"); + diag.set_primary_message(format_args!( "Expected `{expected_ty}`, found `{actual_ty}`", expected_ty = expected_ty.display(context.db()), actual_ty = actual_ty.display(context.db()), - ))); + )); diag.annotate( Annotation::secondary(return_type_span).message(format_args!( "Expected `{expected_ty}` because of return type", diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 1e2d1785b3..3bb2b24070 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -406,6 +406,11 @@ impl Annotation { pub fn get_message(&self) -> Option<&str> { self.message.as_ref().map(|m| m.as_str()) } + + /// Returns the `Span` associated with this annotation. + pub fn get_span(&self) -> &Span { + &self.span + } } /// A string identifier for a lint rule.