Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[ty] Refactor `CheckSuppressionContext` to use `DiagnosticGuard` (#21587)

This commit is contained in:
Micha Reiser 2025-11-25 11:54:42 +01:00 committed by GitHub
parent b19ddca69b
commit eddb9ad38d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 269 additions and 176 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
crates/ty/docs/rules.md generated
View File

@ -2120,7 +2120,7 @@ old_func() # emits [deprecated] diagnostic
Default level: <a href="../rules.md#rule-levels" title="This lint has a default level of 'warn'."><code>warn</code></a> ·
Added in <a href="https://github.com/astral-sh/ty/releases/tag/0.0.1-alpha.1">0.0.1-alpha.1</a> ·
<a href="https://github.com/astral-sh/ty/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20ignore-comment-unknown-rule" target="_blank">Related issues</a> ·
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L40" target="_blank">View source</a>
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L43" target="_blank">View source</a>
</small>
@ -2151,7 +2151,7 @@ a = 20 / 0 # ty: ignore[division-by-zero]
Default level: <a href="../rules.md#rule-levels" title="This lint has a default level of 'warn'."><code>warn</code></a> ·
Added in <a href="https://github.com/astral-sh/ty/releases/tag/0.0.1-alpha.1">0.0.1-alpha.1</a> ·
<a href="https://github.com/astral-sh/ty/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20invalid-ignore-comment" target="_blank">Related issues</a> ·
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L65" target="_blank">View source</a>
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L68" target="_blank">View source</a>
</small>
@ -2536,7 +2536,7 @@ print(x) # NameError: name 'x' is not defined
Default level: <a href="../rules.md#rule-levels" title="This lint has a default level of 'ignore'."><code>ignore</code></a> ·
Added in <a href="https://github.com/astral-sh/ty/releases/tag/0.0.1-alpha.1">0.0.1-alpha.1</a> ·
<a href="https://github.com/astral-sh/ty/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20unused-ignore-comment" target="_blank">Related issues</a> ·
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L15" target="_blank">View source</a>
<a href="https://github.com/astral-sh/ruff/blob/main/crates%2Fty_python_semantic%2Fsrc%2Fsuppression.rs#L18" target="_blank">View source</a>
</small>

136
crates/ty_python_semantic/src/diagnostic.rs
View File

@ -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<TypeCheckDiagnostics>,
/// The diagnostic that we want to report.
///
/// This is always `Some` until the `Drop` impl.
diag: Option<Diagnostic>,
}
impl<'sink> DiagnosticGuard<'sink> {
pub(crate) fn new(
file: File,
sink: &'sink std::cell::RefCell<TypeCheckDiagnostics>,
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);
}
}

181
crates/ty_python_semantic/src/suppression.rs
View File

@ -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<Diagnostic> {
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<TypeCheckDiagnostics>,
}
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,
lint: &'static LintMetadata,
range: TextRange,
message: impl IntoDiagnosticMessage,
) {
if let Some(suppression) = self.suppressions.find_suppression(range, LintId::of(lint)) {
self.diagnostics.mark_used(suppression.id());
return;
fn is_suppression_used(&self, id: FileSuppressionId) -> bool {
self.diagnostics.borrow().is_used(id)
}
self.report_unchecked(lint, range, message);
fn report_lint<'ctx>(
&'ctx self,
lint: &'static LintMetadata,
range: TextRange,
) -> Option<SuppressionDiagnosticGuardBuilder<'ctx, 'a>> {
if let Some(suppression) = self.suppressions.find_suppression(range, LintId::of(lint)) {
self.diagnostics.borrow_mut().mark_used(suppression.id());
return None;
}
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<'ctx, 'a>> {
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<Self> {
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)
}
}

4
crates/ty_python_semantic/src/types.rs
View File

@ -142,7 +142,7 @@ pub fn check_types(db: &dyn Db, file: File) -> Vec<Diagnostic> {
.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<Diagnostic> {
);
}
diagnostics.into_diagnostics()
diagnostics
}
/// Infer the type of a binding.

122
crates/ty_python_semantic/src/types/context.rs
View File

@ -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<Diagnostic>,
}
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)
}
}