Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

red_knot_python_semantic: make TextRange required for reporting a lint diagnostic

Browse Source 
This commit shuffles the reporting API around a little bit such that a
range is required, up front, when reporting a lint diagnostic. This in
turn enables us to make suppression checking eager.

In order to avoid callers needing to provide the range twice, we create
a primary annotation *without* a message inside the `Diagnostic`
encapsulated by the guard. We do this instead of requiring the message
up front because we're concerned about API complexity and the effort
involved in creating the message.

In order to provide a means of attaching a message to the primary
annotation, we expose a convenience API on `LintDiagnosticGuard` for
setting the message. This isn't generally possible for a `Diagnostic`,
but since a `LintDiagnosticGuard` knows how the `Diagnostic` was
constructed, we can offer this API correctly.

It strikes me that it might be easy to forget to attach a primary
annotation message, btu I think this the "least" bad failure mode. And
in particular, it should be somewhat obvious that it's missing once one
adds a snapshot test for how the diagnostic renders.

Otherwise, this API gives us the ability to eagerly check whether a
diagnostic should be reported with nearly minimal information. It also
shouldn't have any footguns since it guarantees that the primary
annotation is tied to the file in the typing context. And it keeps
things pretty simple: callers only need to provide what is actually
strictly necessary to make a diagnostic.
This commit is contained in:
Andrew Gallant
2025-04-11 11:02:27 -04:00
committed by Andrew Gallant
parent b79d43a852
commit 7d11ef1564
4 changed files with 230 additions and 161 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
crates/red_knot_python_semantic/src/types/call/bind.rs
View File

@@ -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)
{

367
crates/red_knot_python_semantic/src/types/context.rs
View File

@@ -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<'ctx, 'db>> {
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<Diagnostic>,
/// 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<LintDiagnosticGuardBuilder<'db, 'ctx>> {
// 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<LintDiagnosticGuardBuilder<'db, 'ctx>> {
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,

9
crates/red_knot_python_semantic/src/types/diagnostic.rs
View File

@@ -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",