diff --git a/Cargo.lock b/Cargo.lock index 1a09eb3feb..1461aa0dab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2432,7 +2432,6 @@ dependencies = [ "salsa", "serde", "tempfile", - "thiserror", "tracing", "tracing-subscriber", "tracing-tree", diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 2244e5df2d..36a94ef505 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -5,6 +5,7 @@ use anyhow::{anyhow, Context}; use clap::Parser; use colored::Colorize; use crossbeam::channel as crossbeam_channel; +use ruff_db::diagnostic::CompileDiagnostic; use salsa::plumbing::ZalsaDatabase; use red_knot_python_semantic::SitePackages; @@ -318,8 +319,9 @@ impl MainLoop { } => { let has_diagnostics = !result.is_empty(); if check_revision == revision { + #[allow(clippy::print_stdout)] for diagnostic in result { - tracing::error!("{}", diagnostic); + println!("{}", diagnostic.display(db)); } } else { tracing::debug!( @@ -378,7 +380,10 @@ impl MainLoopCancellationToken { #[derive(Debug)] enum MainLoopMessage { CheckWorkspace, - CheckCompleted { result: Vec, revision: u64 }, + CheckCompleted { + result: Vec, + revision: u64, + }, ApplyChanges(Vec), Exit, } diff --git a/crates/red_knot_python_semantic/resources/mdtest/unpacking.md b/crates/red_knot_python_semantic/resources/mdtest/unpacking.md index b5f4b6f368..6781cbb438 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/unpacking.md +++ b/crates/red_knot_python_semantic/resources/mdtest/unpacking.md @@ -145,13 +145,8 @@ reveal_type(f) # revealed: Unknown ### Non-iterable unpacking -TODO: Remove duplicate diagnostics. This is happening because for a sequence-like assignment target, -multiple definitions are created and the inference engine runs on each of them which results in -duplicate diagnostics. - ```py # error: "Object of type `Literal[1]` is not iterable" -# error: "Object of type `Literal[1]` is not iterable" a, b = 1 reveal_type(a) # revealed: Unknown reveal_type(b) # revealed: Unknown diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 2cdc79d545..dcf16b9285 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1,8 +1,11 @@ +use diagnostic::{is_any_diagnostic_enabled, report_not_iterable, report_type_diagnostic}; use mro::{ClassBase, Mro, MroError, MroIterator}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic, Severity}; use ruff_db::files::File; use ruff_python_ast as ast; use itertools::Itertools; +use ruff_text_size::{Ranged as _, TextRange}; use crate::semantic_index::ast_ids::HasScopedAstId; use crate::semantic_index::definition::Definition; @@ -15,12 +18,11 @@ use crate::stdlib::{ builtins_symbol, types_symbol, typeshed_symbol, typing_extensions_symbol, typing_symbol, }; use crate::symbol::{Boundness, Symbol}; -use crate::types::diagnostic::TypeCheckDiagnosticsBuilder; use crate::types::narrow::narrowing_constraint; use crate::{Db, FxOrderSet, HasTy, Module, SemanticModel}; pub(crate) use self::builder::{IntersectionBuilder, UnionBuilder}; -pub use self::diagnostic::{TypeCheckDiagnostic, TypeCheckDiagnostics}; +pub use self::diagnostic::TypeCheckDiagnostic; pub(crate) use self::display::TypeArrayDisplay; pub(crate) use self::infer::{ infer_deferred_types, infer_definition_types, infer_expression_types, infer_scope_types, @@ -34,18 +36,15 @@ mod mro; mod narrow; mod unpacker; -pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { +#[salsa::tracked] +pub fn check_types(db: &dyn Db, file: File) { let _span = tracing::trace_span!("check_types", file=?file.path(db)).entered(); let index = semantic_index(db, file); - let mut diagnostics = TypeCheckDiagnostics::new(); for scope_id in index.scope_ids() { - let result = infer_scope_types(db, scope_id); - diagnostics.extend(result.diagnostics()); + let _ = infer_scope_types(db, scope_id); } - - diagnostics } /// Infer the public type of a symbol (its type as seen from outside its scope). @@ -1598,19 +1597,21 @@ impl<'db> CallOutcome<'db> { } /// Get the return type of the call, emitting default diagnostics if needed. - fn unwrap_with_diagnostic<'a>( + fn unwrap_with_diagnostic( &self, db: &'db dyn Db, node: ast::AnyNodeRef, - diagnostics: &'a mut TypeCheckDiagnosticsBuilder<'db>, + file: File, ) -> Type<'db> { - match self.return_ty_result(db, node, diagnostics) { + match self.return_ty_result(db, node, file) { Ok(return_ty) => return_ty, Err(NotCallableError::Type { not_callable_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -1625,7 +1626,9 @@ impl<'db> CallOutcome<'db> { called_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -1641,7 +1644,9 @@ impl<'db> CallOutcome<'db> { called_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -1656,11 +1661,11 @@ impl<'db> CallOutcome<'db> { } /// Get the return type of the call as a result. - fn return_ty_result<'a>( + fn return_ty_result( &self, db: &'db dyn Db, node: ast::AnyNodeRef, - diagnostics: &'a mut TypeCheckDiagnosticsBuilder<'db>, + file: File, ) -> Result, NotCallableError<'db>> { match self { Self::Callable { return_ty } => Ok(*return_ty), @@ -1668,11 +1673,17 @@ impl<'db> CallOutcome<'db> { return_ty, revealed_ty, } => { - diagnostics.add( - node, - "revealed-type", - format_args!("Revealed type is `{}`", revealed_ty.display(db)), - ); + if is_any_diagnostic_enabled(db, file) { + CompileDiagnostic::report( + db.upcast(), + RevealTypeDiagnostic { + file, + range: node.range(), + ty: revealed_ty.display(db).to_string(), + }, + ); + } + Ok(*return_ty) } Self::NotCallable { not_callable_ty } => Err(NotCallableError::Type { @@ -1700,10 +1711,10 @@ impl<'db> CallOutcome<'db> { *return_ty } else { revealed = true; - outcome.unwrap_with_diagnostic(db, node, diagnostics) + outcome.unwrap_with_diagnostic(db, node, file) } } - _ => outcome.unwrap_with_diagnostic(db, node, diagnostics), + _ => outcome.unwrap_with_diagnostic(db, node, file), }; union_builder = union_builder.add(return_ty); } @@ -1785,13 +1796,14 @@ enum IterationOutcome<'db> { impl<'db> IterationOutcome<'db> { fn unwrap_with_diagnostic( self, + db: &dyn Db, iterable_node: ast::AnyNodeRef, - diagnostics: &mut TypeCheckDiagnosticsBuilder<'db>, + file: File, ) -> Type<'db> { match self { Self::Iterable { element_ty } => element_ty, Self::NotIterable { not_iterable_ty } => { - diagnostics.add_not_iterable(iterable_node, not_iterable_ty); + report_not_iterable(db, file, iterable_node, not_iterable_ty); Type::Unknown } } @@ -2228,6 +2240,35 @@ impl<'db> TupleType<'db> { } } +#[derive(Debug)] +pub struct RevealTypeDiagnostic { + range: TextRange, + ty: String, + file: File, +} + +impl Diagnostic for RevealTypeDiagnostic { + fn rule(&self) -> &str { + "revealed-type" + } + + fn message(&self) -> std::borrow::Cow { + format!("Revealed type is `{}`", self.ty).into() + } + + fn file(&self) -> File { + self.file + } + + fn range(&self) -> Option { + Some(self.range) + } + + fn severity(&self) -> Severity { + Severity::Info + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index eb2ae8b2e7..7c42eff31c 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -1,13 +1,198 @@ +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic, Severity}; use ruff_db::files::File; use ruff_python_ast::{self as ast, AnyNodeRef}; use ruff_text_size::{Ranged, TextRange}; -use std::fmt::Formatter; -use std::ops::Deref; -use std::sync::Arc; +use std::borrow::Cow; use crate::types::{ClassLiteralType, Type}; use crate::Db; +/// Returns `true` if any diagnostic is enabled for this file. +pub(crate) fn is_any_diagnostic_enabled(db: &dyn Db, file: File) -> bool { + db.is_file_open(file) +} + +pub(crate) fn report_type_diagnostic( + db: &dyn Db, + file: File, + node: AnyNodeRef, + rule: &str, + message: std::fmt::Arguments, +) { + if !is_any_diagnostic_enabled(db, file) { + return; + } + + // TODO: Don't emit the diagnostic if: + // * The enclosing node contains any syntax errors + // * The rule is disabled for this file. We probably want to introduce a new query that + // 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. + + CompileDiagnostic::report( + db.upcast(), + TypeCheckDiagnostic { + file, + rule: rule.to_string(), + message: message.to_string(), + range: node.range(), + }, + ); +} + +/// Emit a diagnostic declaring that the object represented by `node` is not iterable +pub(super) fn report_not_iterable( + db: &dyn Db, + file: File, + node: AnyNodeRef, + not_iterable_ty: Type, +) { + report_type_diagnostic( + db, + file, + node, + "not-iterable", + format_args!( + "Object of type `{}` is not iterable", + not_iterable_ty.display(db) + ), + ); +} + +/// Emit a diagnostic declaring that an index is out of bounds for a tuple. +pub(super) fn report_index_out_of_bounds( + db: &dyn Db, + file: File, + kind: &'static str, + node: AnyNodeRef, + tuple_ty: Type, + length: usize, + index: i64, +) { + report_type_diagnostic( + db, + file, + node, + "index-out-of-bounds", + format_args!( + "Index {index} is out of bounds for {kind} `{}` with length {length}", + tuple_ty.display(db) + ), + ); +} + +/// Emit a diagnostic declaring that a type does not support subscripting. +pub(super) fn report_non_subscriptable( + db: &dyn Db, + file: File, + node: AnyNodeRef, + non_subscriptable_ty: Type, + method: &str, +) { + report_type_diagnostic( + db, + file, + node, + "non-subscriptable", + format_args!( + "Cannot subscript object of type `{}` with no `{method}` method", + non_subscriptable_ty.display(db) + ), + ); +} + +pub(super) fn report_unresolved_module<'a>( + db: &dyn Db, + file: File, + import_node: impl Into>, + level: u32, + module: Option<&str>, +) { + report_type_diagnostic( + db, + file, + import_node.into(), + "unresolved-import", + format_args!( + "Cannot resolve import `{}{}`", + ".".repeat(level as usize), + module.unwrap_or_default() + ), + ); +} + +pub(super) fn report_slice_step_size_zero(db: &dyn Db, file: File, node: AnyNodeRef) { + report_type_diagnostic( + db, + file, + node, + "zero-stepsize-in-slice", + format_args!("Slice step size can not be zero"), + ); +} + +pub(super) fn report_invalid_assignment( + db: &dyn Db, + file: File, + node: AnyNodeRef, + declared_ty: Type, + assigned_ty: Type, +) { + match declared_ty { + Type::ClassLiteral(ClassLiteralType { class }) => { + report_type_diagnostic(db, file, node, "invalid-assignment", format_args!( + "Implicit shadowing of class `{}`; annotate to make it explicit if this is intentional", + class.name(db))); + } + Type::FunctionLiteral(function) => { + report_type_diagnostic(db, file, node, "invalid-assignment", format_args!( + "Implicit shadowing of function `{}`; annotate to make it explicit if this is intentional", + function.name(db))); + } + _ => { + report_type_diagnostic( + db, + file, + node, + "invalid-assignment", + format_args!( + "Object of type `{}` is not assignable to `{}`", + assigned_ty.display(db), + declared_ty.display(db), + ), + ); + } + } +} + +pub(super) fn report_possibly_unresolved_reference( + db: &dyn Db, + file: File, + expr_name_node: &ast::ExprName, +) { + let ast::ExprName { id, .. } = expr_name_node; + + report_type_diagnostic( + db, + file, + expr_name_node.into(), + "possibly-unresolved-reference", + format_args!("Name `{id}` used when possibly not defined"), + ); +} + +pub(super) fn report_unresolved_reference(db: &dyn Db, file: File, expr_name_node: &ast::ExprName) { + let ast::ExprName { id, .. } = expr_name_node; + + report_type_diagnostic( + db, + file, + expr_name_node.into(), + "unresolved-reference", + format_args!("Name `{id}` used when not defined"), + ); +} + #[derive(Debug, Eq, PartialEq)] pub struct TypeCheckDiagnostic { // TODO: Don't use string keys for rules @@ -22,265 +207,29 @@ impl TypeCheckDiagnostic { &self.rule } - pub fn message(&self) -> &str { - &self.message - } - - pub fn file(&self) -> File { - self.file - } -} - -impl Ranged for TypeCheckDiagnostic { - fn range(&self) -> TextRange { + pub fn range(&self) -> TextRange { self.range } } -/// A collection of type check diagnostics. -/// -/// The diagnostics are wrapped in an `Arc` because they need to be cloned multiple times -/// when going from `infer_expression` to `check_file`. We could consider -/// making [`TypeCheckDiagnostic`] a Salsa struct to have them Arena-allocated (once the Tables refactor is done). -/// Using Salsa struct does have the downside that it leaks the Salsa dependency into diagnostics and -/// each Salsa-struct comes with an overhead. -#[derive(Default, Eq, PartialEq)] -pub struct TypeCheckDiagnostics { - inner: Vec>, -} - -impl TypeCheckDiagnostics { - pub fn new() -> Self { - Self { inner: Vec::new() } +impl Diagnostic for TypeCheckDiagnostic { + fn message(&self) -> std::borrow::Cow { + Cow::Borrowed(&self.message) } - pub(super) fn push(&mut self, diagnostic: TypeCheckDiagnostic) { - self.inner.push(Arc::new(diagnostic)); + fn file(&self) -> File { + self.file } - pub(crate) fn shrink_to_fit(&mut self) { - self.inner.shrink_to_fit(); - } -} - -impl Extend for TypeCheckDiagnostics { - fn extend>(&mut self, iter: T) { - self.inner.extend(iter.into_iter().map(std::sync::Arc::new)); - } -} - -impl Extend> for TypeCheckDiagnostics { - fn extend>>(&mut self, iter: T) { - self.inner.extend(iter); - } -} - -impl<'a> Extend<&'a std::sync::Arc> for TypeCheckDiagnostics { - fn extend>>(&mut self, iter: T) { - self.inner - .extend(iter.into_iter().map(std::sync::Arc::clone)); - } -} - -impl std::fmt::Debug for TypeCheckDiagnostics { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - self.inner.fmt(f) - } -} - -impl Deref for TypeCheckDiagnostics { - type Target = [std::sync::Arc]; - - fn deref(&self) -> &Self::Target { - &self.inner - } -} - -impl IntoIterator for TypeCheckDiagnostics { - type Item = Arc; - type IntoIter = std::vec::IntoIter>; - - fn into_iter(self) -> Self::IntoIter { - self.inner.into_iter() - } -} - -impl<'a> IntoIterator for &'a TypeCheckDiagnostics { - type Item = &'a Arc; - type IntoIter = std::slice::Iter<'a, std::sync::Arc>; - - fn into_iter(self) -> Self::IntoIter { - self.inner.iter() - } -} - -pub(super) struct TypeCheckDiagnosticsBuilder<'db> { - db: &'db dyn Db, - file: File, - diagnostics: TypeCheckDiagnostics, -} - -impl<'db> TypeCheckDiagnosticsBuilder<'db> { - pub(super) fn new(db: &'db dyn Db, file: File) -> Self { - Self { - db, - file, - diagnostics: TypeCheckDiagnostics::new(), - } - } - - /// Emit a diagnostic declaring that the object represented by `node` is not iterable - pub(super) fn add_not_iterable(&mut self, node: AnyNodeRef, not_iterable_ty: Type<'db>) { - self.add( - node, - "not-iterable", - format_args!( - "Object of type `{}` is not iterable", - not_iterable_ty.display(self.db) - ), - ); - } - - /// Emit a diagnostic declaring that an index is out of bounds for a tuple. - pub(super) fn add_index_out_of_bounds( - &mut self, - kind: &'static str, - node: AnyNodeRef, - tuple_ty: Type<'db>, - length: usize, - index: i64, - ) { - self.add( - node, - "index-out-of-bounds", - format_args!( - "Index {index} is out of bounds for {kind} `{}` with length {length}", - tuple_ty.display(self.db) - ), - ); - } - - /// Emit a diagnostic declaring that a type does not support subscripting. - pub(super) fn add_non_subscriptable( - &mut self, - node: AnyNodeRef, - non_subscriptable_ty: Type<'db>, - method: &str, - ) { - self.add( - node, - "non-subscriptable", - format_args!( - "Cannot subscript object of type `{}` with no `{method}` method", - non_subscriptable_ty.display(self.db) - ), - ); - } - - pub(super) fn add_unresolved_module( - &mut self, - import_node: impl Into>, - level: u32, - module: Option<&str>, - ) { - self.add( - import_node.into(), - "unresolved-import", - format_args!( - "Cannot resolve import `{}{}`", - ".".repeat(level as usize), - module.unwrap_or_default() - ), - ); - } - - pub(super) fn add_slice_step_size_zero(&mut self, node: AnyNodeRef) { - self.add( - node, - "zero-stepsize-in-slice", - format_args!("Slice step size can not be zero"), - ); - } - - pub(super) fn add_invalid_assignment( - &mut self, - node: AnyNodeRef, - declared_ty: Type<'db>, - assigned_ty: Type<'db>, - ) { - match declared_ty { - Type::ClassLiteral(ClassLiteralType { class }) => { - self.add(node, "invalid-assignment", format_args!( - "Implicit shadowing of class `{}`; annotate to make it explicit if this is intentional", - class.name(self.db))); - } - Type::FunctionLiteral(function) => { - self.add(node, "invalid-assignment", format_args!( - "Implicit shadowing of function `{}`; annotate to make it explicit if this is intentional", - function.name(self.db))); - } - _ => { - self.add( - node, - "invalid-assignment", - format_args!( - "Object of type `{}` is not assignable to `{}`", - assigned_ty.display(self.db), - declared_ty.display(self.db), - ), - ); - } - } - } - - pub(super) fn add_possibly_unresolved_reference(&mut self, expr_name_node: &ast::ExprName) { - let ast::ExprName { id, .. } = expr_name_node; - - self.add( - expr_name_node.into(), - "possibly-unresolved-reference", - format_args!("Name `{id}` used when possibly not defined"), - ); - } - - pub(super) fn add_unresolved_reference(&mut self, expr_name_node: &ast::ExprName) { - let ast::ExprName { id, .. } = expr_name_node; - - self.add( - expr_name_node.into(), - "unresolved-reference", - format_args!("Name `{id}` used when not defined"), - ); - } - - /// Adds a new diagnostic. - /// - /// The diagnostic does not get added if the rule isn't enabled for this file. - pub(super) fn add(&mut self, node: AnyNodeRef, rule: &str, message: std::fmt::Arguments) { - if !self.db.is_file_open(self.file) { - return; - } - - // TODO: Don't emit the diagnostic if: - // * The enclosing node contains any syntax errors - // * The rule is disabled for this file. We probably want to introduce a new query that - // 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. - - self.diagnostics.push(TypeCheckDiagnostic { - file: self.file, - rule: rule.to_string(), - message: message.to_string(), - range: node.range(), - }); - } - - pub(super) fn extend(&mut self, diagnostics: &TypeCheckDiagnostics) { - self.diagnostics.extend(diagnostics); - } - - pub(super) fn finish(mut self) -> TypeCheckDiagnostics { - self.diagnostics.shrink_to_fit(); - self.diagnostics + fn range(&self) -> Option { + Some(self.range) + } + + fn severity(&self) -> Severity { + Severity::Error + } + + fn rule(&self) -> &str { + &self.rule } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 9465542a87..7b7ae17b37 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -48,9 +48,6 @@ use crate::semantic_index::semantic_index; use crate::semantic_index::symbol::{NodeWithScopeKind, NodeWithScopeRef, ScopeId}; use crate::semantic_index::SemanticIndex; use crate::stdlib::builtins_module_scope; -use crate::types::diagnostic::{ - TypeCheckDiagnostic, TypeCheckDiagnostics, TypeCheckDiagnosticsBuilder, -}; use crate::types::mro::MroErrorKind; use crate::types::unpacker::{UnpackResult, Unpacker}; use crate::types::{ @@ -64,6 +61,12 @@ use crate::unpack::Unpack; use crate::util::subscript::{PyIndex, PySlice}; use crate::Db; +use super::diagnostic::{ + is_any_diagnostic_enabled, report_index_out_of_bounds, report_invalid_assignment, + report_non_subscriptable, report_possibly_unresolved_reference, report_slice_step_size_zero, + report_type_diagnostic, report_unresolved_module, report_unresolved_reference, +}; + /// Infer all types for a [`ScopeId`], including all definitions and expressions in that scope. /// Use when checking a scope, or needing to provide a type for an arbitrary expression in the /// scope. @@ -212,9 +215,6 @@ pub(crate) struct TypeInference<'db> { /// The types of every declaration in this region. declarations: FxHashMap, Type<'db>>, - /// The diagnostics for this region. - diagnostics: TypeCheckDiagnostics, - /// Are there deferred type expressions in this region? has_deferred: bool, @@ -228,7 +228,6 @@ impl<'db> TypeInference<'db> { expressions: FxHashMap::default(), bindings: FxHashMap::default(), declarations: FxHashMap::default(), - diagnostics: TypeCheckDiagnostics::default(), has_deferred: false, scope, } @@ -253,15 +252,10 @@ impl<'db> TypeInference<'db> { self.declarations[&definition] } - pub(crate) fn diagnostics(&self) -> &[std::sync::Arc] { - &self.diagnostics - } - fn shrink_to_fit(&mut self) { self.expressions.shrink_to_fit(); self.bindings.shrink_to_fit(); self.declarations.shrink_to_fit(); - self.diagnostics.shrink_to_fit(); } } @@ -321,8 +315,6 @@ pub(super) struct TypeInferenceBuilder<'db> { /// The type inference results types: TypeInference<'db>, - - diagnostics: TypeCheckDiagnosticsBuilder<'db>, } impl<'db> TypeInferenceBuilder<'db> { @@ -352,7 +344,6 @@ impl<'db> TypeInferenceBuilder<'db> { region, file, types: TypeInference::empty(scope), - diagnostics: TypeCheckDiagnosticsBuilder::new(db, file), } } @@ -364,7 +355,6 @@ impl<'db> TypeInferenceBuilder<'db> { .declarations .extend(inference.declarations.iter()); self.types.expressions.extend(inference.expressions.iter()); - self.diagnostics.extend(&inference.diagnostics); self.types.has_deferred |= inference.has_deferred; } @@ -439,13 +429,14 @@ impl<'db> TypeInferenceBuilder<'db> { if infer_definition_types(self.db, *definition).has_deferred { let deferred = infer_deferred_types(self.db, *definition); self.types.expressions.extend(&deferred.expressions); - self.diagnostics.extend(&deferred.diagnostics); } } } - // TODO: Only call this function when diagnostics are enabled. - self.check_class_definitions(); + // TODO: Test specifically for the check class definition rules + if is_any_diagnostic_enabled(self.db, self.file) { + self.check_class_definitions(); + } } /// Iterate over all class definitions to check that Python will be able to create a @@ -473,14 +464,18 @@ impl<'db> TypeInferenceBuilder<'db> { MroErrorKind::DuplicateBases(duplicates) => { let base_nodes = class.node(self.db).bases(); for (index, duplicate) in duplicates { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, (&base_nodes[*index]).into(), "duplicate-base", format_args!("Duplicate base class `{}`", duplicate.name(self.db)) ); } } - MroErrorKind::CyclicClassDefinition => self.diagnostics.add( + MroErrorKind::CyclicClassDefinition => report_type_diagnostic( + self.db, + self.file, class.node(self.db).into(), "cyclic-class-def", format_args!( @@ -492,7 +487,9 @@ impl<'db> TypeInferenceBuilder<'db> { MroErrorKind::InvalidBases(bases) => { let base_nodes = class.node(self.db).bases(); for (index, base_ty) in bases { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, (&base_nodes[*index]).into(), "invalid-base", format_args!( @@ -502,7 +499,9 @@ impl<'db> TypeInferenceBuilder<'db> { ); } }, - MroErrorKind::UnresolvableMro{bases_list} => self.diagnostics.add( + MroErrorKind::UnresolvableMro{bases_list} => report_type_diagnostic( + self.db, + self.file, class.node(self.db).into(), "inconsistent-mro", format_args!( @@ -627,7 +626,9 @@ impl<'db> TypeInferenceBuilder<'db> { _ => return, }; - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, expr.into(), "division-by-zero", format_args!( @@ -652,7 +653,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO point out the conflicting declarations in the diagnostic? let symbol_table = self.index.symbol_table(binding.file_scope(self.db)); let symbol_name = symbol_table.symbol(binding.symbol(self.db)).name(); - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "conflicting-declarations", format_args!( @@ -664,8 +667,7 @@ impl<'db> TypeInferenceBuilder<'db> { }, ); if !bound_ty.is_assignable_to(self.db, declared_ty) { - self.diagnostics - .add_invalid_assignment(node, declared_ty, bound_ty); + report_invalid_assignment(self.db, self.file, node, declared_ty, bound_ty); // allow declarations to override inference in case of invalid assignment bound_ty = declared_ty; }; @@ -682,7 +684,9 @@ impl<'db> TypeInferenceBuilder<'db> { let ty = if inferred_ty.is_assignable_to(self.db, ty) { ty } else { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "invalid-declaration", format_args!( @@ -708,8 +712,7 @@ impl<'db> TypeInferenceBuilder<'db> { let inferred_ty = if inferred_ty.is_assignable_to(self.db, declared_ty) { inferred_ty } else { - self.diagnostics - .add_invalid_assignment(node, declared_ty, inferred_ty); + report_invalid_assignment(self.db, self.file, node, declared_ty, inferred_ty); // if the assignment is invalid, fall back to assuming the annotation is correct declared_ty }; @@ -1150,7 +1153,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO: Make use of Protocols when we support it (the manager be assignable to `contextlib.AbstractContextManager`). match (enter, exit) { (Symbol::Unbound, Symbol::Unbound) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1161,7 +1166,9 @@ impl<'db> TypeInferenceBuilder<'db> { Type::Unknown } (Symbol::Unbound, _) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1173,7 +1180,9 @@ impl<'db> TypeInferenceBuilder<'db> { } (Symbol::Type(enter_ty, enter_boundness), exit) => { if enter_boundness == Boundness::MayBeUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1185,9 +1194,11 @@ impl<'db> TypeInferenceBuilder<'db> { let target_ty = enter_ty .call(self.db, &[context_expression_ty]) - .return_ty_result(self.db, context_expression.into(), &mut self.diagnostics) + .return_ty_result(self.db, context_expression.into(), self.file) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!(" @@ -1201,7 +1212,9 @@ impl<'db> TypeInferenceBuilder<'db> { match exit { Symbol::Unbound => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1214,7 +1227,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO: Use the `exit_ty` to determine if any raised exception is suppressed. if exit_boundness == Boundness::MayBeUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1234,14 +1249,12 @@ impl<'db> TypeInferenceBuilder<'db> { Type::none(self.db), ], ) - .return_ty_result( - self.db, - context_expression.into(), - &mut self.diagnostics, - ) + .return_ty_result(self.db, context_expression.into(), self.file) .is_err() { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1440,7 +1453,6 @@ impl<'db> TypeInferenceBuilder<'db> { let target_ty = match target { TargetKind::Sequence(unpack) => { let unpacked = infer_unpack_types(self.db, unpack); - self.diagnostics.extend(unpacked.diagnostics()); unpacked.get(name_ast_id).unwrap_or(Type::Unknown) } TargetKind::Name => value_ty, @@ -1545,11 +1557,13 @@ impl<'db> TypeInferenceBuilder<'db> { let augmented_return_ty = match call.return_ty_result( self.db, AnyNodeRef::StmtAugAssign(assignment), - &mut self.diagnostics, + self.file, ) { Ok(t) => t, Err(e) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1570,7 +1584,9 @@ impl<'db> TypeInferenceBuilder<'db> { let binary_return_ty = self.infer_binary_expression_type(left_ty, right_ty, op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1599,7 +1615,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_expression_type(left_ty, right_ty, op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1697,7 +1715,7 @@ impl<'db> TypeInferenceBuilder<'db> { } else { iterable_ty .iterate(self.db) - .unwrap_with_diagnostic(iterable.into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, iterable.into(), self.file) }; self.store_expression_type(target, loop_var_value_ty); @@ -1736,7 +1754,7 @@ impl<'db> TypeInferenceBuilder<'db> { if let Some(module) = self.module_ty_from_name(&module_name) { module } else { - self.diagnostics.add_unresolved_module(alias, 0, Some(name)); + report_unresolved_module(self.db, self.file, alias, 0, Some(name)); Type::Unknown } } else { @@ -1867,7 +1885,9 @@ impl<'db> TypeInferenceBuilder<'db> { .member(self.db, &ast::name::Name::new(&name.id)) .as_type() .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, AnyNodeRef::Alias(alias), "unresolved-import", format_args!("Module `{module_name}` has no member `{name}`",), @@ -1876,8 +1896,7 @@ impl<'db> TypeInferenceBuilder<'db> { Type::Unknown }) } else { - self.diagnostics - .add_unresolved_module(import_from, *level, module); + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } } @@ -1891,8 +1910,7 @@ impl<'db> TypeInferenceBuilder<'db> { "Relative module resolution `{}` failed: too many leading dots", format_import_from_module(*level, module), ); - self.diagnostics - .add_unresolved_module(import_from, *level, module); + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } Err(ModuleNameResolutionError::UnknownCurrentModule) => { @@ -1901,8 +1919,7 @@ impl<'db> TypeInferenceBuilder<'db> { format_import_from_module(*level, module), self.file.path(self.db) ); - self.diagnostics - .add_unresolved_module(import_from, *level, module); + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } }; @@ -2364,7 +2381,7 @@ impl<'db> TypeInferenceBuilder<'db> { } else { iterable_ty .iterate(self.db) - .unwrap_with_diagnostic(iterable.into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, iterable.into(), self.file) }; self.types @@ -2456,7 +2473,7 @@ impl<'db> TypeInferenceBuilder<'db> { let function_type = self.infer_expression(func); function_type .call(self.db, arg_types.as_slice()) - .unwrap_with_diagnostic(self.db, func.as_ref().into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, func.as_ref().into(), self.file) } fn infer_starred_expression(&mut self, starred: &ast::ExprStarred) -> Type<'db> { @@ -2467,9 +2484,11 @@ impl<'db> TypeInferenceBuilder<'db> { } = starred; let iterable_ty = self.infer_expression(value); - iterable_ty - .iterate(self.db) - .unwrap_with_diagnostic(value.as_ref().into(), &mut self.diagnostics); + iterable_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + value.as_ref().into(), + self.file, + ); // TODO Type::Todo @@ -2488,9 +2507,11 @@ impl<'db> TypeInferenceBuilder<'db> { let ast::ExprYieldFrom { range: _, value } = yield_from; let iterable_ty = self.infer_expression(value); - iterable_ty - .iterate(self.db) - .unwrap_with_diagnostic(value.as_ref().into(), &mut self.diagnostics); + iterable_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + value.as_ref().into(), + self.file, + ); // TODO get type from `ReturnType` of generator Type::Todo @@ -2558,7 +2579,9 @@ impl<'db> TypeInferenceBuilder<'db> { { let mut symbol = builtins_symbol(self.db, name); if symbol.is_unbound() && name == "reveal_type" { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, name_node.into(), "undefined-reveal", format_args!( @@ -2611,7 +2634,7 @@ impl<'db> TypeInferenceBuilder<'db> { match self.lookup_name(name) { Symbol::Type(looked_up_ty, looked_up_boundness) => { if looked_up_boundness == Boundness::MayBeUnbound { - self.diagnostics.add_possibly_unresolved_reference(name); + report_possibly_unresolved_reference(self.db, self.file, name); } bindings_ty @@ -2620,9 +2643,9 @@ impl<'db> TypeInferenceBuilder<'db> { } Symbol::Unbound => { if bindings_ty.is_some() { - self.diagnostics.add_possibly_unresolved_reference(name); + report_possibly_unresolved_reference(self.db, self.file, name); } else { - self.diagnostics.add_unresolved_reference(name); + report_unresolved_reference(self.db, self.file, name); } bindings_ty.unwrap_or(Type::Unknown) } @@ -2715,14 +2738,13 @@ impl<'db> TypeInferenceBuilder<'db> { { let call = class_member.call(self.db, &[operand_type]); - match call.return_ty_result( - self.db, - AnyNodeRef::ExprUnaryOp(unary), - &mut self.diagnostics, - ) { + match call.return_ty_result(self.db, AnyNodeRef::ExprUnaryOp(unary), self.file) + { Ok(t) => t, Err(e) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, unary.into(), "unsupported-operator", format_args!( @@ -2734,7 +2756,9 @@ impl<'db> TypeInferenceBuilder<'db> { } } } else { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, unary.into(), "unsupported-operator", format_args!( @@ -2775,7 +2799,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_expression_type(left_ty, right_ty, *op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, binary.into(), "unsupported-operator", format_args!( @@ -3105,7 +3131,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_type_comparison(left_ty, *op, right_ty) .unwrap_or_else(|error| { // Handle unsupported operators (diagnostic, `bool`/`Unknown` outcome) - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, AnyNodeRef::ExprCompare(compare), "unsupported-operator", format_args!( @@ -3495,7 +3523,9 @@ impl<'db> TypeInferenceBuilder<'db> { .py_index(i32::try_from(int).expect("checked in branch arm")) .copied() .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "tuple", value_node.into(), value_ty, @@ -3514,7 +3544,7 @@ impl<'db> TypeInferenceBuilder<'db> { let new_elements: Vec<_> = new_elements.copied().collect(); Type::Tuple(TupleType::new(self.db, new_elements.into_boxed_slice())) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown } } @@ -3533,7 +3563,9 @@ impl<'db> TypeInferenceBuilder<'db> { )) }) .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "string", value_node.into(), value_ty, @@ -3553,7 +3585,7 @@ impl<'db> TypeInferenceBuilder<'db> { let literal: String = new_chars.collect(); Type::StringLiteral(StringLiteralType::new(self.db, literal.into_boxed_str())) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown }; result @@ -3570,7 +3602,9 @@ impl<'db> TypeInferenceBuilder<'db> { Type::BytesLiteral(BytesLiteralType::new(self.db, [*byte].as_slice())) }) .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "bytes literal", value_node.into(), value_ty, @@ -3589,7 +3623,7 @@ impl<'db> TypeInferenceBuilder<'db> { let new_bytes: Vec = new_bytes.copied().collect(); Type::BytesLiteral(BytesLiteralType::new(self.db, new_bytes.into_boxed_slice())) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown } } @@ -3613,7 +3647,9 @@ impl<'db> TypeInferenceBuilder<'db> { Symbol::Unbound => {} Symbol::Type(dunder_getitem_method, boundness) => { if boundness == Boundness::MayBeUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-possibly-unbound-method", format_args!( @@ -3625,9 +3661,11 @@ impl<'db> TypeInferenceBuilder<'db> { return dunder_getitem_method .call(self.db, &[slice_ty]) - .return_ty_result(self.db, value_node.into(), &mut self.diagnostics) + .return_ty_result(self.db, value_node.into(), self.file) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-non-callable", format_args!( @@ -3657,7 +3695,9 @@ impl<'db> TypeInferenceBuilder<'db> { Symbol::Unbound => {} Symbol::Type(ty, boundness) => { if boundness == Boundness::MayBeUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-possibly-unbound-method", format_args!( @@ -3669,9 +3709,11 @@ impl<'db> TypeInferenceBuilder<'db> { return ty .call(self.db, &[slice_ty]) - .return_ty_result(self.db, value_node.into(), &mut self.diagnostics) + .return_ty_result(self.db, value_node.into(), self.file) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-non-callable", format_args!( @@ -3690,13 +3732,17 @@ impl<'db> TypeInferenceBuilder<'db> { return KnownClass::GenericAlias.to_instance(self.db); } - self.diagnostics.add_non_subscriptable( + report_non_subscriptable( + self.db, + self.file, value_node.into(), value_ty, "__class_getitem__", ); } else { - self.diagnostics.add_non_subscriptable( + report_non_subscriptable( + self.db, + self.file, value_node.into(), value_ty, "__getitem__", @@ -3791,7 +3837,6 @@ impl<'db> TypeInferenceBuilder<'db> { pub(super) fn finish(mut self) -> TypeInference<'db> { self.infer_region(); - self.types.diagnostics = self.diagnostics.finish(); self.types.shrink_to_fit(); self.types } @@ -4079,7 +4124,9 @@ impl<'db> TypeInferenceBuilder<'db> { Ok(ty) => ty, Err(nodes) => { for node in nodes { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node.into(), "invalid-literal-parameter", format_args!( @@ -4401,8 +4448,9 @@ fn perform_membership_test_comparison<'db>( #[cfg(test)] mod tests { - use anyhow::Context; + use ruff_db::diagnostic::CompileDiagnostic; + use ruff_db::diagnostic::Diagnostic; use crate::db::tests::TestDb; use crate::program::{Program, SearchPathSettings}; @@ -4503,22 +4551,18 @@ mod tests { } #[track_caller] - fn assert_diagnostic_messages(diagnostics: &TypeCheckDiagnostics, expected: &[&str]) { - let messages: Vec<&str> = diagnostics + fn assert_file_diagnostics(db: &TestDb, filename: &str, expected: &[&str]) { + let file = system_path_to_file(db, filename).unwrap(); + + let diagnostics = check_types::accumulated::(db, file); + + let messages: Vec<_> = diagnostics .iter() .map(|diagnostic| diagnostic.message()) .collect(); assert_eq!(&messages, expected); } - #[track_caller] - fn assert_file_diagnostics(db: &TestDb, filename: &str, expected: &[&str]) { - let file = system_path_to_file(db, filename).unwrap(); - let diagnostics = check_types(db, file); - - assert_diagnostic_messages(&diagnostics, expected); - } - #[test] fn from_import_with_no_module_name() -> anyhow::Result<()> { // This test checks that invalid syntax in a `StmtImportFrom` node @@ -5077,7 +5121,14 @@ mod tests { ", )?; - assert_file_diagnostics(&db, "src/a.py", &["Revealed type is `Unknown`"]); + assert_file_diagnostics( + &db, + "src/a.py", + &[ + "Expected one or more exception types", + "Revealed type is `Unknown`", + ], + ); Ok(()) } @@ -5337,8 +5388,15 @@ mod tests { assert_scope_ty(&db, "src/a.py", &["foo", ""], "z", "Unknown"); - // (There is a diagnostic for invalid syntax that's emitted, but it's not listed by `assert_file_diagnostics`) - assert_file_diagnostics(&db, "src/a.py", &["Name `z` used when not defined"]); + assert_file_diagnostics( + &db, + "src/a.py", + &[ + "Expected an identifier, but found a keyword 'in' that cannot be used here", + "Expected 'in', found name", + "Name `z` used when not defined", + ], + ); Ok(()) } @@ -5424,7 +5482,7 @@ mod tests { return 42 class Iterable: - def __iter__(self) -> Iterator: + def __iter__(self) -> Iterator: ... x = [*NotIterable()] y = [*Iterable()] diff --git a/crates/red_knot_python_semantic/src/types/unpacker.rs b/crates/red_knot_python_semantic/src/types/unpacker.rs index 9dd9466c05..ec12ed9547 100644 --- a/crates/red_knot_python_semantic/src/types/unpacker.rs +++ b/crates/red_knot_python_semantic/src/types/unpacker.rs @@ -6,14 +6,14 @@ use rustc_hash::FxHashMap; use crate::semantic_index::ast_ids::{HasScopedAstId, ScopedExpressionId}; use crate::semantic_index::symbol::ScopeId; -use crate::types::{TupleType, Type, TypeCheckDiagnostics, TypeCheckDiagnosticsBuilder}; +use crate::types::{TupleType, Type}; use crate::Db; /// Unpacks the value expression type to their respective targets. pub(crate) struct Unpacker<'db> { db: &'db dyn Db, targets: FxHashMap>, - diagnostics: TypeCheckDiagnosticsBuilder<'db>, + file: File, } impl<'db> Unpacker<'db> { @@ -21,7 +21,7 @@ impl<'db> Unpacker<'db> { Self { db, targets: FxHashMap::default(), - diagnostics: TypeCheckDiagnosticsBuilder::new(db, file), + file, } } @@ -104,9 +104,11 @@ impl<'db> Unpacker<'db> { let value_ty = if value_ty.is_literal_string() { Type::LiteralString } else { - value_ty - .iterate(self.db) - .unwrap_with_diagnostic(AnyNodeRef::from(target), &mut self.diagnostics) + value_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + AnyNodeRef::from(target), + self.file, + ) }; for element in elts { self.unpack(element, value_ty, scope); @@ -120,7 +122,6 @@ impl<'db> Unpacker<'db> { pub(crate) fn finish(mut self) -> UnpackResult<'db> { self.targets.shrink_to_fit(); UnpackResult { - diagnostics: self.diagnostics.finish(), targets: self.targets, } } @@ -129,15 +130,10 @@ impl<'db> Unpacker<'db> { #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct UnpackResult<'db> { targets: FxHashMap>, - diagnostics: TypeCheckDiagnostics, } impl<'db> UnpackResult<'db> { pub(crate) fn get(&self, expr_id: ScopedExpressionId) -> Option> { self.targets.get(&expr_id).copied() } - - pub(crate) fn diagnostics(&self) -> &TypeCheckDiagnostics { - &self.diagnostics - } } diff --git a/crates/red_knot_server/src/edit.rs b/crates/red_knot_server/src/edit.rs index 94cf84c282..0345f8aac5 100644 --- a/crates/red_knot_server/src/edit.rs +++ b/crates/red_knot_server/src/edit.rs @@ -6,7 +6,7 @@ mod text_document; use lsp_types::{PositionEncodingKind, Url}; pub use notebook::NotebookDocument; -pub(crate) use range::RangeExt; +pub(crate) use range::{RangeExt, ToRangeExt}; pub(crate) use text_document::DocumentVersion; pub use text_document::TextDocument; diff --git a/crates/red_knot_server/src/edit/range.rs b/crates/red_knot_server/src/edit/range.rs index a923b0023a..9ccef9e67d 100644 --- a/crates/red_knot_server/src/edit/range.rs +++ b/crates/red_knot_server/src/edit/range.rs @@ -1,13 +1,32 @@ +use super::notebook; use super::PositionEncoding; -use ruff_source_file::LineIndex; +use lsp_types as types; +use ruff_notebook::NotebookIndex; use ruff_source_file::OneIndexed; +use ruff_source_file::{LineIndex, SourceLocation}; use ruff_text_size::{TextRange, TextSize}; +pub(crate) struct NotebookRange { + pub(crate) cell: notebook::CellId, + pub(crate) range: types::Range, +} + pub(crate) trait RangeExt { fn to_text_range(&self, text: &str, index: &LineIndex, encoding: PositionEncoding) -> TextRange; } +pub(crate) trait ToRangeExt { + fn to_range(&self, text: &str, index: &LineIndex, encoding: PositionEncoding) -> types::Range; + fn to_notebook_range( + &self, + text: &str, + source_index: &LineIndex, + notebook_index: &NotebookIndex, + encoding: PositionEncoding, + ) -> NotebookRange; +} + fn u32_index_to_usize(index: u32) -> usize { usize::try_from(index).expect("u32 fits in usize") } @@ -75,6 +94,61 @@ impl RangeExt for lsp_types::Range { } } +impl ToRangeExt for TextRange { + fn to_range(&self, text: &str, index: &LineIndex, encoding: PositionEncoding) -> types::Range { + types::Range { + start: source_location_to_position(&offset_to_source_location( + self.start(), + text, + index, + encoding, + )), + end: source_location_to_position(&offset_to_source_location( + self.end(), + text, + index, + encoding, + )), + } + } + + fn to_notebook_range( + &self, + text: &str, + source_index: &LineIndex, + notebook_index: &NotebookIndex, + encoding: PositionEncoding, + ) -> NotebookRange { + let start = offset_to_source_location(self.start(), text, source_index, encoding); + let mut end = offset_to_source_location(self.end(), text, source_index, encoding); + let starting_cell = notebook_index.cell(start.row); + + // weird edge case here - if the end of the range is where the newline after the cell got added (making it 'out of bounds') + // we need to move it one character back (which should place it at the end of the last line). + // we test this by checking if the ending offset is in a different (or nonexistent) cell compared to the cell of the starting offset. + if notebook_index.cell(end.row) != starting_cell { + end.row = end.row.saturating_sub(1); + end.column = offset_to_source_location( + self.end().checked_sub(1.into()).unwrap_or_default(), + text, + source_index, + encoding, + ) + .column; + } + + let start = source_location_to_position(¬ebook_index.translate_location(&start)); + let end = source_location_to_position(¬ebook_index.translate_location(&end)); + + NotebookRange { + cell: starting_cell + .map(OneIndexed::to_zero_indexed) + .unwrap_or_default(), + range: types::Range { start, end }, + } + } +} + /// Converts a UTF-16 code unit offset for a given line into a UTF-8 column number. fn utf8_column_offset(utf16_code_unit_offset: u32, line: &str) -> TextSize { let mut utf8_code_unit_offset = TextSize::new(0); @@ -96,3 +170,46 @@ fn utf8_column_offset(utf16_code_unit_offset: u32, line: &str) -> TextSize { utf8_code_unit_offset } + +fn offset_to_source_location( + offset: TextSize, + text: &str, + index: &LineIndex, + encoding: PositionEncoding, +) -> SourceLocation { + match encoding { + PositionEncoding::UTF8 => { + let row = index.line_index(offset); + let column = offset - index.line_start(row, text); + + SourceLocation { + column: OneIndexed::from_zero_indexed(column.to_usize()), + row, + } + } + PositionEncoding::UTF16 => { + let row = index.line_index(offset); + + let column = if index.is_ascii() { + (offset - index.line_start(row, text)).to_usize() + } else { + let up_to_line = &text[TextRange::new(index.line_start(row, text), offset)]; + up_to_line.encode_utf16().count() + }; + + SourceLocation { + column: OneIndexed::from_zero_indexed(column), + row, + } + } + PositionEncoding::UTF32 => index.source_location(offset, text), + } +} + +fn source_location_to_position(location: &SourceLocation) -> types::Position { + types::Position { + line: u32::try_from(location.row.to_zero_indexed()).expect("row usize fits in u32"), + character: u32::try_from(location.column.to_zero_indexed()) + .expect("character usize fits in u32"), + } +} diff --git a/crates/red_knot_server/src/server/api/requests/diagnostic.rs b/crates/red_knot_server/src/server/api/requests/diagnostic.rs index 25622dc32b..4f9c6478b3 100644 --- a/crates/red_knot_server/src/server/api/requests/diagnostic.rs +++ b/crates/red_knot_server/src/server/api/requests/diagnostic.rs @@ -3,12 +3,15 @@ use std::borrow::Cow; use lsp_types::request::DocumentDiagnosticRequest; use lsp_types::{ Diagnostic, DiagnosticSeverity, DocumentDiagnosticParams, DocumentDiagnosticReport, - DocumentDiagnosticReportResult, FullDocumentDiagnosticReport, Position, Range, + DocumentDiagnosticReportResult, FullDocumentDiagnosticReport, NumberOrString, Range, RelatedFullDocumentDiagnosticReport, Url, }; -use red_knot_workspace::db::RootDatabase; +use red_knot_workspace::db::{Db, RootDatabase}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic as _, Severity}; +use ruff_db::source::{line_index, source_text}; +use crate::edit::ToRangeExt as _; use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler}; use crate::server::{client::Notifier, Result}; use crate::session::DocumentSnapshot; @@ -64,36 +67,37 @@ fn compute_diagnostics(snapshot: &DocumentSnapshot, db: &RootDatabase) -> Vec Diagnostic { - let words = message.split(':').collect::>(); +fn to_lsp_diagnostic( + db: &dyn Db, + diagnostic: &CompileDiagnostic, + encoding: crate::PositionEncoding, +) -> Diagnostic { + let range = if let Some(range) = diagnostic.range() { + let index = line_index(db.upcast(), diagnostic.file()); + let source = source_text(db.upcast(), diagnostic.file()); - let (range, message) = match words.as_slice() { - [_, _, line, column, message] | [_, line, column, message] => { - let line = line.parse::().unwrap_or_default().saturating_sub(1); - let column = column.parse::().unwrap_or_default(); - ( - Range::new( - Position::new(line, column.saturating_sub(1)), - Position::new(line, column), - ), - message.trim(), - ) - } - _ => (Range::default(), message), + range.to_range(&source, &index, encoding) + } else { + Range::default() + }; + + let severity = match diagnostic.severity() { + Severity::Info => DiagnosticSeverity::INFORMATION, + Severity::Error => DiagnosticSeverity::ERROR, }; Diagnostic { range, - severity: Some(DiagnosticSeverity::ERROR), + severity: Some(severity), tags: None, - code: None, + code: Some(NumberOrString::String(diagnostic.rule().to_string())), code_description: None, source: Some("red-knot".into()), - message: message.to_string(), + message: diagnostic.message().into_owned(), related_information: None, data: None, } diff --git a/crates/red_knot_test/src/diagnostic.rs b/crates/red_knot_test/src/diagnostic.rs index 56e4a87906..52087cf5bd 100644 --- a/crates/red_knot_test/src/diagnostic.rs +++ b/crates/red_knot_test/src/diagnostic.rs @@ -3,9 +3,10 @@ //! We don't assume that we will get the diagnostics in source order. use ruff_source_file::{LineIndex, OneIndexed}; -use ruff_text_size::Ranged; use std::ops::{Deref, Range}; +use crate::matcher::Diagnostic; + /// All diagnostics for one embedded Python file, sorted and grouped by start line number. /// /// The diagnostics are kept in a flat vector, sorted by line number. A separate vector of @@ -19,7 +20,7 @@ pub(crate) struct SortedDiagnostics { impl SortedDiagnostics where - T: Ranged + Clone, + T: Diagnostic, { pub(crate) fn new(diagnostics: impl IntoIterator, line_index: &LineIndex) -> Self { let mut diagnostics: Vec<_> = diagnostics @@ -94,7 +95,7 @@ pub(crate) struct LineDiagnosticsIterator<'a, T> { impl<'a, T> Iterator for LineDiagnosticsIterator<'a, T> where - T: Ranged + Clone, + T: Diagnostic, { type Item = LineDiagnostics<'a, T>; @@ -110,7 +111,7 @@ where } } -impl std::iter::FusedIterator for LineDiagnosticsIterator<'_, T> where T: Clone + Ranged {} +impl std::iter::FusedIterator for LineDiagnosticsIterator<'_, T> where T: Diagnostic {} /// All diagnostics that start on a single line of source code in one embedded Python file. #[derive(Debug)] @@ -139,6 +140,7 @@ struct DiagnosticWithLine { #[cfg(test)] mod tests { use crate::db::Db; + use crate::matcher::Diagnostic; use ruff_db::files::system_path_to_file; use ruff_db::source::line_index; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; @@ -152,13 +154,18 @@ mod tests { let file = system_path_to_file(&db, "/src/test.py").unwrap(); let lines = line_index(&db, file); - let ranges = vec![ + let ranges = [ TextRange::new(TextSize::new(0), TextSize::new(1)), TextRange::new(TextSize::new(5), TextSize::new(10)), TextRange::new(TextSize::new(1), TextSize::new(7)), ]; - let sorted = super::SortedDiagnostics::new(&ranges, &lines); + let diagnostics: Vec<_> = ranges + .into_iter() + .map(|range| DummyDiagnostic { range }) + .collect(); + + let sorted = super::SortedDiagnostics::new(diagnostics, &lines); let grouped = sorted.iter_lines().collect::>(); let [line1, line2] = &grouped[..] else { @@ -170,4 +177,22 @@ mod tests { assert_eq!(line2.line_number, OneIndexed::from_zero_indexed(1)); assert_eq!(line2.diagnostics.len(), 1); } + + struct DummyDiagnostic { + range: TextRange, + } + + impl Diagnostic for DummyDiagnostic { + fn rule(&self) -> &str { + "dummy" + } + + fn message(&self) -> std::borrow::Cow { + "Dummy error".into() + } + + fn range(&self) -> TextRange { + self.range + } + } } diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index cd49a98041..9f00042650 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -1,6 +1,7 @@ use colored::Colorize; use parser as test_parser; use red_knot_python_semantic::types::check_types; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic as _}; use ruff_db::files::{system_path_to_file, File, Files}; use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; @@ -96,7 +97,15 @@ fn run_test(db: &mut db::Db, test: &parser::MarkdownTest) -> Result<(), Failures parsed.errors() ); - match matcher::match_file(db, test_file.file, check_types(db, test_file.file)) { + let diagnostics: Vec<_> = + // The accumulator returns all diagnostics from all files. We're only interested into + // diagnostics from this file. + check_types::accumulated::(db, test_file.file) + .into_iter() + .filter(|diagnostic| diagnostic.file() == test_file.file) + .collect(); + + match matcher::match_file(db, test_file.file, diagnostics) { Ok(()) => None, Err(line_failures) => Some(FileFailures { backtick_offset: test_file.backtick_offset, diff --git a/crates/red_knot_test/src/matcher.rs b/crates/red_knot_test/src/matcher.rs index fbc3d9ba77..994a7c35ea 100644 --- a/crates/red_knot_test/src/matcher.rs +++ b/crates/red_knot_test/src/matcher.rs @@ -1,17 +1,16 @@ -//! Match [`TypeCheckDiagnostic`]s against [`Assertion`]s and produce test failure messages for any +//! Match [`Diagnostic`]s against [`Assertion`]s and produce test failure messages for any //! mismatches. use crate::assertion::{Assertion, ErrorAssertion, InlineFileAssertions}; use crate::db::Db; use crate::diagnostic::SortedDiagnostics; use colored::Colorize; -use red_knot_python_semantic::types::TypeCheckDiagnostic; use ruff_db::files::File; use ruff_db::source::{line_index, source_text, SourceText}; use ruff_source_file::{LineIndex, OneIndexed}; -use ruff_text_size::Ranged; +use ruff_text_size::{TextRange, TextSize}; +use std::borrow::Cow; use std::cmp::Ordering; use std::ops::Range; -use std::sync::Arc; #[derive(Debug, Default)] pub(super) struct FailuresByLine { @@ -126,19 +125,33 @@ where } } -pub(super) trait Diagnostic: Ranged { +pub(super) trait Diagnostic { + /// Returns the diagnostic's rule code. fn rule(&self) -> &str; - fn message(&self) -> &str; + fn message(&self) -> Cow; + + fn range(&self) -> TextRange; + + fn start(&self) -> TextSize { + self.range().start() + } } -impl Diagnostic for Arc { +impl Diagnostic for T +where + T: ruff_db::diagnostic::Diagnostic, +{ fn rule(&self) -> &str { - self.as_ref().rule() + ruff_db::diagnostic::Diagnostic::rule(self) } - fn message(&self) -> &str { - self.as_ref().message() + fn message(&self) -> Cow { + ruff_db::diagnostic::Diagnostic::message(self) + } + + fn range(&self) -> TextRange { + ruff_db::diagnostic::Diagnostic::range(self).unwrap_or_default() } } @@ -253,7 +266,7 @@ impl Matcher { } } - fn column(&self, ranged: &T) -> OneIndexed { + fn column(&self, ranged: &T) -> OneIndexed { self.line_index .source_location(ranged.start(), &self.source) .column @@ -322,12 +335,14 @@ impl Matcher { #[cfg(test)] mod tests { + use std::borrow::Cow; + use super::FailuresByLine; use ruff_db::files::system_path_to_file; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; use ruff_python_trivia::textwrap::dedent; use ruff_source_file::OneIndexed; - use ruff_text_size::{Ranged, TextRange}; + use ruff_text_size::TextRange; #[derive(Clone, Debug)] struct TestDiagnostic { @@ -352,12 +367,10 @@ mod tests { self.rule } - fn message(&self) -> &str { - self.message + fn message(&self) -> Cow { + Cow::Borrowed(self.message) } - } - impl Ranged for TestDiagnostic { fn range(&self) -> ruff_text_size::TextRange { self.range } @@ -373,6 +386,7 @@ mod tests { super::match_file(&db, file, diagnostics) } + #[track_caller] fn assert_fail(result: Result<(), FailuresByLine>, messages: &[(usize, &[&str])]) { let Err(failures) = result else { panic!("expected a failure"); @@ -395,6 +409,7 @@ mod tests { assert_eq!(failures, expected); } + #[track_caller] fn assert_ok(result: &Result<(), FailuresByLine>) { assert!(result.is_ok(), "{result:?}"); } diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index 2d70b3d684..ec6c708e7e 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -1,9 +1,10 @@ use std::any::Any; use js_sys::Error; +use ruff_db::diagnostic::CompileDiagnostic; use wasm_bindgen::prelude::*; -use red_knot_workspace::db::RootDatabase; +use red_knot_workspace::db::{Db, RootDatabase}; use red_knot_workspace::workspace::settings::Configuration; use red_knot_workspace::workspace::WorkspaceMetadata; use ruff_db::files::{system_path_to_file, File}; @@ -110,14 +111,14 @@ impl Workspace { pub fn check_file(&self, file_id: &FileHandle) -> Result, Error> { let result = self.db.check_file(file_id.file).map_err(into_error)?; - Ok(result) + Ok(map_diagnostics(&self.db, result)) } /// Checks all open files pub fn check(&self) -> Result, Error> { let result = self.db.check().map_err(into_error)?; - Ok(result) + Ok(map_diagnostics(&self.db, result)) } /// Returns the parsed AST for `path` @@ -142,6 +143,13 @@ impl Workspace { } } +fn map_diagnostics(db: &dyn Db, diagnostics: Vec) -> Vec { + diagnostics + .into_iter() + .map(|diagnostic| diagnostic.display(db.upcast()).to_string()) + .collect() +} + pub(crate) fn into_error(err: E) -> Error { Error::new(&err.to_string()) } diff --git a/crates/red_knot_wasm/tests/api.rs b/crates/red_knot_wasm/tests/api.rs index ed2ccd76ba..4735eca849 100644 --- a/crates/red_knot_wasm/tests/api.rs +++ b/crates/red_knot_wasm/tests/api.rs @@ -19,6 +19,6 @@ fn check() { assert_eq!( result, - vec!["/test.py:1:8: Cannot resolve import `random22`"] + vec!["error[unresolved-import] /test.py:1:8 Cannot resolve import `random22`"] ); } diff --git a/crates/red_knot_workspace/src/db.rs b/crates/red_knot_workspace/src/db.rs index 4d44036eca..3d24ac70be 100644 --- a/crates/red_knot_workspace/src/db.rs +++ b/crates/red_knot_workspace/src/db.rs @@ -1,6 +1,7 @@ use std::panic::RefUnwindSafe; use std::sync::Arc; +use ruff_db::diagnostic::CompileDiagnostic; use salsa::plumbing::ZalsaDatabase; use salsa::{Cancelled, Event}; @@ -10,7 +11,7 @@ use ruff_db::system::System; use ruff_db::vendored::VendoredFileSystem; use ruff_db::{Db as SourceDb, Upcast}; -use crate::workspace::{check_file, Workspace, WorkspaceMetadata}; +use crate::workspace::{Workspace, WorkspaceMetadata}; mod changes; @@ -51,14 +52,14 @@ impl RootDatabase { } /// Checks all open files in the workspace and its dependencies. - pub fn check(&self) -> Result, Cancelled> { + pub fn check(&self) -> Result, Cancelled> { self.with_db(|db| db.workspace().check(db)) } - pub fn check_file(&self, file: File) -> Result, Cancelled> { + pub fn check_file(&self, file: File) -> Result, Cancelled> { let _span = tracing::debug_span!("check_file", file=%file.path(self)).entered(); - self.with_db(|db| check_file(db, file)) + self.with_db(|db| db.workspace().check_file(db, file)) } /// Returns a mutable reference to the system. diff --git a/crates/red_knot_workspace/src/workspace.rs b/crates/red_knot_workspace/src/workspace.rs index 52e5f57a7a..c1afb69d3a 100644 --- a/crates/red_knot_workspace/src/workspace.rs +++ b/crates/red_knot_workspace/src/workspace.rs @@ -1,19 +1,18 @@ use std::{collections::BTreeMap, sync::Arc}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic}; use rustc_hash::{FxBuildHasher, FxHashSet}; use salsa::{Durability, Setter as _}; pub use metadata::{PackageMetadata, WorkspaceMetadata}; use red_knot_python_semantic::types::check_types; use red_knot_python_semantic::SearchPathSettings; -use ruff_db::parsed::parsed_module; -use ruff_db::source::{line_index, source_text, SourceDiagnostic}; +use ruff_db::source::source_text; use ruff_db::{ files::{system_path_to_file, File}, system::{walk_directory::WalkState, SystemPath, SystemPathBuf}, }; use ruff_python_ast::{name::Name, PySourceType}; -use ruff_text_size::Ranged; use crate::db::Db; use crate::db::RootDatabase; @@ -188,7 +187,7 @@ impl Workspace { } /// Checks all open files in the workspace and its dependencies. - pub fn check(self, db: &RootDatabase) -> Vec { + pub fn check(self, db: &RootDatabase) -> Vec { let workspace_span = tracing::debug_span!("check_workspace"); let _span = workspace_span.enter(); @@ -200,6 +199,13 @@ impl Workspace { let db = db.snapshot(); let workspace_span = workspace_span.clone(); + // TODO: Checking per-file and then filtering out the diagnostics from other files + // isn't the ideal solution but doing it "properly" requires support for + // ["parallel Salsa"](https://github.com/salsa-rs/salsa/pull/568). + // + // The solution with parallel Salsa is: + // * Create a new `check_workspace_impl` query similar to `check_file_impl` + // * Collect the workspace diagnostics using `check_workspace_impl::accumulated::(&db, file)` rayon::scope(move |scope| { for file in &files { let result = inner_result.clone(); @@ -210,8 +216,17 @@ impl Workspace { let check_file_span = tracing::debug_span!(parent: &workspace_span, "check_file", file=%file.path(&db)); let _entered = check_file_span.entered(); - let file_diagnostics = check_file(&db, file); - result.lock().unwrap().extend(file_diagnostics); + // Filter out the diagnostics from other files to avoid duplicates. + // This should no longer be necessary with parallel-salsa where + // it's possible to call the query for the entire workspace. + let mut file_diagnostics = + check_file_impl::accumulated::(&db, file); + + file_diagnostics.sort_unstable_by_key(|a| { + a.range().unwrap_or_default().start() + }); + + result.lock().unwrap().extend(file_diagnostics.into_iter().filter(|diagnostic| diagnostic.file() == file)); }); } }); @@ -219,6 +234,20 @@ impl Workspace { Arc::into_inner(result).unwrap().into_inner().unwrap() } + pub fn check_file(self, db: &dyn Db, file: File) -> Vec { + let diagnostics = check_file_impl::accumulated::(db, file); + + let mut file_diagnostics: Vec<_> = diagnostics + .into_iter() + .filter(|diagnostic| diagnostic.file() == file) + .collect(); + + file_diagnostics + .sort_unstable_by_key(|diagnostic| diagnostic.range().unwrap_or_default().start()); + + file_diagnostics + } + /// Opens a file in the workspace. /// /// This changes the behavior of `check` to only check the open files rather than all files in the workspace. @@ -378,49 +407,28 @@ impl Package { } } +/// Checks a single file +/// +/// This is a Salsa query so that [`Workspace::check_file`] can retrieve the accumulated diagnostics. #[salsa::tracked] -pub(super) fn check_file(db: &dyn Db, file: File) -> Vec { +pub(super) fn check_file_impl(db: &dyn Db, file: File) { tracing::debug!("Checking file '{path}'", path = file.path(db)); - let mut diagnostics = Vec::new(); - - let source_diagnostics = source_text::accumulated::(db.upcast(), file); - // TODO(micha): Consider using a single accumulator for all diagnostics - diagnostics.extend( - source_diagnostics - .iter() - .map(std::string::ToString::to_string), - ); - // Abort checking if there are IO errors. - let source = source_text(db.upcast(), file); - - if source.has_read_error() { - return diagnostics; + if source_text(db.upcast(), file).has_read_error() { + return; } - let parsed = parsed_module(db.upcast(), file); + check_types(db.upcast(), file); +} - if !parsed.errors().is_empty() { - let path = file.path(db); - let line_index = line_index(db.upcast(), file); - diagnostics.extend(parsed.errors().iter().map(|err| { - let source_location = line_index.source_location(err.location.start(), source.as_str()); - format!("{path}:{source_location}: {message}", message = err.error) - })); +#[salsa::tracked] +fn check_workspace_sync(db: &dyn Db, workspace: Workspace) { + let files = WorkspaceFiles::new(db, workspace); + + for file in &files { + check_file_impl(db, file); } - - for diagnostic in check_types(db.upcast(), file) { - let index = line_index(db.upcast(), diagnostic.file()); - let location = index.source_location(diagnostic.start(), source.as_str()); - diagnostics.push(format!( - "{path}:{location}: {message}", - path = file.path(db), - message = diagnostic.message() - )); - } - - diagnostics } fn discover_package_files(db: &dyn Db, path: &SystemPath) -> FxHashSet { @@ -536,17 +544,43 @@ impl Iterator for WorkspaceFilesIter<'_> { #[cfg(test)] mod tests { use red_knot_python_semantic::types::check_types; + use red_knot_python_semantic::{ + ProgramSettings, PythonVersion, SearchPathSettings, SitePackages, + }; + use ruff_db::diagnostic::Diagnostic; use ruff_db::files::system_path_to_file; use ruff_db::source::source_text; - use ruff_db::system::{DbWithTestSystem, SystemPath}; + use ruff_db::system::{DbWithTestSystem, SystemPath, SystemPathBuf}; use ruff_db::testing::assert_function_query_was_not_run; use crate::db::tests::TestDb; - use crate::workspace::check_file; + + use crate::workspace::settings::WorkspaceSettings; + use crate::workspace::{Workspace, WorkspaceMetadata}; #[test] fn check_file_skips_type_checking_when_file_cant_be_read() -> ruff_db::system::Result<()> { let mut db = TestDb::new(); + let root = SystemPathBuf::from("src"); + let workspace = Workspace::from_metadata( + &db, + WorkspaceMetadata { + root: root.clone(), + packages: vec![], + settings: WorkspaceSettings { + program: ProgramSettings { + target_version: PythonVersion::default(), + search_paths: SearchPathSettings { + extra_paths: vec![], + src_root: root, + custom_typeshed: None, + site_packages: SitePackages::Known(vec![]), + }, + }, + }, + }, + ); + let path = SystemPath::new("test.py"); db.write_file(path, "x = 10")?; @@ -556,9 +590,16 @@ mod tests { db.memory_file_system().remove_file(path)?; file.sync(&mut db); + let diagnostics: Vec<_> = workspace + .check_file(&db, file) + .into_iter() + .map(|diagnostic| diagnostic.message().to_string()) + .collect(); + assert_eq!(source_text(&db, file).as_str(), ""); + assert_eq!( - check_file(&db, file), + diagnostics, vec!["Failed to read file: No such file or directory".to_string()] ); @@ -569,8 +610,14 @@ mod tests { // content returned by `source_text` remains unchanged, but the diagnostics should get updated. db.write_file(path, "").unwrap(); + let diagnostics: Vec<_> = workspace + .check_file(&db, file) + .into_iter() + .map(|diagnostic| diagnostic.message().to_string()) + .collect(); + assert_eq!(source_text(&db, file).as_str(), ""); - assert_eq!(check_file(&db, file), vec![] as Vec); + assert_eq!(diagnostics, vec![] as Vec); Ok(()) } diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index cc1c4f9028..20603fa9b7 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -24,32 +24,32 @@ const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/ static EXPECTED_DIAGNOSTICS: &[&str] = &[ // We don't support `*` imports yet: - "/src/tomllib/_parser.py:7:29: Module `collections.abc` has no member `Iterable`", + "error[unresolved-import] /src/tomllib/_parser.py:7:29 Module `collections.abc` has no member `Iterable`", // We don't support terminal statements in control flow yet: - "/src/tomllib/_parser.py:246:15: Method `__class_getitem__` of type `Literal[frozenset]` is possibly unbound", - "/src/tomllib/_parser.py:692:8354: Invalid class base with type `GenericAlias` (all bases must be a class, `Any`, `Unknown` or `Todo`)", - "/src/tomllib/_parser.py:66:18: Name `s` used when possibly not defined", - "/src/tomllib/_parser.py:98:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:101:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:104:14: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:108:17: Conflicting declared types for `second_char`: Unknown, str | None", - "/src/tomllib/_parser.py:115:14: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:126:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:267:9: Conflicting declared types for `char`: Unknown, str | None", - "/src/tomllib/_parser.py:348:20: Name `nest` used when possibly not defined", - "/src/tomllib/_parser.py:353:5: Name `nest` used when possibly not defined", - "/src/tomllib/_parser.py:364:9: Conflicting declared types for `char`: Unknown, str | None", - "/src/tomllib/_parser.py:381:13: Conflicting declared types for `char`: Unknown, str | None", - "/src/tomllib/_parser.py:395:9: Conflicting declared types for `char`: Unknown, str | None", - "/src/tomllib/_parser.py:453:24: Name `nest` used when possibly not defined", - "/src/tomllib/_parser.py:455:9: Name `nest` used when possibly not defined", - "/src/tomllib/_parser.py:482:16: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:566:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:573:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:579:12: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:580:63: Name `char` used when possibly not defined", - "/src/tomllib/_parser.py:590:9: Conflicting declared types for `char`: Unknown, str | None", - "/src/tomllib/_parser.py:629:38: Name `datetime_obj` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:66:18 Name `s` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:98:12 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:101:12 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:104:14 Name `char` used when possibly not defined", + "error[conflicting-declarations] /src/tomllib/_parser.py:108:17 Conflicting declared types for `second_char`: Unknown, str | None", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:115:14 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:126:12 Name `char` used when possibly not defined", + "error[call-possibly-unbound-method] /src/tomllib/_parser.py:246:15 Method `__class_getitem__` of type `Literal[frozenset]` is possibly unbound", + "error[conflicting-declarations] /src/tomllib/_parser.py:267:9 Conflicting declared types for `char`: Unknown, str | None", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:348:20 Name `nest` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:353:5 Name `nest` used when possibly not defined", + "error[conflicting-declarations] /src/tomllib/_parser.py:364:9 Conflicting declared types for `char`: Unknown, str | None", + "error[conflicting-declarations] /src/tomllib/_parser.py:381:13 Conflicting declared types for `char`: Unknown, str | None", + "error[conflicting-declarations] /src/tomllib/_parser.py:395:9 Conflicting declared types for `char`: Unknown, str | None", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:453:24 Name `nest` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:455:9 Name `nest` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:482:16 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:566:12 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:573:12 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:579:12 Name `char` used when possibly not defined", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:580:63 Name `char` used when possibly not defined", + "error[conflicting-declarations] /src/tomllib/_parser.py:590:9 Conflicting declared types for `char`: Unknown, str | None", + "error[possibly-unresolved-reference] /src/tomllib/_parser.py:629:38 Name `datetime_obj` used when possibly not defined", + "error[invalid-base] /src/tomllib/_parser.py:692:8354 Invalid class base with type `GenericAlias` (all bases must be a class, `Any`, `Unknown` or `Todo`)", ]; fn get_test_file(name: &str) -> TestFile { @@ -123,7 +123,13 @@ fn setup_rayon() { fn benchmark_incremental(criterion: &mut Criterion) { fn setup() -> Case { let case = setup_case(); - let result = case.db.check().unwrap(); + let result: Vec<_> = case + .db + .check() + .unwrap() + .into_iter() + .map(|diagnostic| diagnostic.display(&case.db).to_string()) + .collect(); assert_eq!(result, EXPECTED_DIAGNOSTICS); @@ -150,7 +156,7 @@ fn benchmark_incremental(criterion: &mut Criterion) { let result = db.check().unwrap(); - assert_eq!(result, EXPECTED_DIAGNOSTICS); + assert_eq!(result.len(), EXPECTED_DIAGNOSTICS.len()); } setup_rayon(); @@ -168,7 +174,12 @@ fn benchmark_cold(criterion: &mut Criterion) { setup_case, |case| { let Case { db, .. } = case; - let result = db.check().unwrap(); + let result: Vec<_> = db + .check() + .unwrap() + .into_iter() + .map(|diagnostic| diagnostic.display(db).to_string()) + .collect(); assert_eq!(result, EXPECTED_DIAGNOSTICS); }, diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index 3410fe7449..d151ae5c60 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -28,7 +28,6 @@ matchit = { workspace = true } salsa = { workspace = true } serde = { workspace = true, optional = true } path-slash = { workspace = true } -thiserror = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true, optional = true } tracing-tree = { workspace = true, optional = true } diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs new file mode 100644 index 0000000000..e688aca4c6 --- /dev/null +++ b/crates/ruff_db/src/diagnostic.rs @@ -0,0 +1,155 @@ +use ruff_text_size::TextRange; +use salsa::Accumulator as _; + +use crate::{ + files::File, + source::{line_index, source_text}, + Db, +}; + +pub trait Diagnostic: Send + Sync + std::fmt::Debug { + fn rule(&self) -> &str; + + fn message(&self) -> std::borrow::Cow; + + fn file(&self) -> File; + + fn range(&self) -> Option; + + fn severity(&self) -> Severity; +} + +#[derive(Debug, Clone, Copy)] +pub enum Severity { + Info, + Error, +} + +#[salsa::accumulator] +pub struct CompileDiagnostic(std::sync::Arc); + +impl CompileDiagnostic { + pub fn report(db: &dyn Db, diagnostic: T) + where + T: Diagnostic + 'static, + { + Self(std::sync::Arc::new(diagnostic)).accumulate(db); + } + + pub fn display<'a>(&'a self, db: &'a dyn Db) -> DisplayDiagnostic<'a> { + DisplayDiagnostic { + db, + diagnostic: &*self.0, + } + } +} + +impl Diagnostic for CompileDiagnostic { + fn rule(&self) -> &str { + self.0.rule() + } + + fn message(&self) -> std::borrow::Cow { + self.0.message() + } + + fn file(&self) -> File { + self.0.file() + } + + fn range(&self) -> Option { + self.0.range() + } + + fn severity(&self) -> Severity { + self.0.severity() + } +} + +pub struct DisplayDiagnostic<'db> { + db: &'db dyn Db, + diagnostic: &'db dyn Diagnostic, +} + +impl<'db> DisplayDiagnostic<'db> { + pub fn new(db: &'db dyn Db, diagnostic: &'db dyn Diagnostic) -> Self { + Self { db, diagnostic } + } +} + +impl std::fmt::Display for DisplayDiagnostic<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.diagnostic.severity() { + Severity::Info => f.write_str("info")?, + Severity::Error => f.write_str("error")?, + } + + write!( + f, + "[{rule}] {path}", + rule = self.diagnostic.rule(), + path = self.diagnostic.file().path(self.db) + )?; + + if let Some(range) = self.diagnostic.range() { + let index = line_index(self.db, self.diagnostic.file()); + let source = source_text(self.db, self.diagnostic.file()); + + let start = index.source_location(range.start(), &source); + + write!(f, ":{line}:{col}", line = start.row, col = start.column)?; + } + + write!(f, " {message}", message = self.diagnostic.message()) + } +} + +impl Diagnostic for Box +where + T: Diagnostic, +{ + fn rule(&self) -> &str { + (**self).rule() + } + + fn message(&self) -> std::borrow::Cow { + (**self).message() + } + + fn file(&self) -> File { + (**self).file() + } + + fn range(&self) -> Option { + (**self).range() + } + + fn severity(&self) -> Severity { + (**self).severity() + } +} + +impl Diagnostic for std::sync::Arc +where + T: Diagnostic, +{ + fn rule(&self) -> &str { + (**self).rule() + } + + fn message(&self) -> std::borrow::Cow { + (**self).message() + } + + fn file(&self) -> File { + (**self).file() + } + + fn range(&self) -> Option { + (**self).range() + } + + fn severity(&self) -> Severity { + (**self).severity() + } +} diff --git a/crates/ruff_db/src/lib.rs b/crates/ruff_db/src/lib.rs index 63369d9fa1..adccefb7c4 100644 --- a/crates/ruff_db/src/lib.rs +++ b/crates/ruff_db/src/lib.rs @@ -6,6 +6,7 @@ use crate::files::Files; use crate::system::System; use crate::vendored::VendoredFileSystem; +pub mod diagnostic; pub mod display; pub mod file_revision; pub mod files; diff --git a/crates/ruff_db/src/parsed.rs b/crates/ruff_db/src/parsed.rs index e93d5e5517..d02dccf264 100644 --- a/crates/ruff_db/src/parsed.rs +++ b/crates/ruff_db/src/parsed.rs @@ -1,10 +1,13 @@ +use std::borrow::Cow; use std::fmt::Formatter; use std::ops::Deref; use std::sync::Arc; use ruff_python_ast::{ModModule, PySourceType}; use ruff_python_parser::{parse_unchecked_source, Parsed}; +use ruff_text_size::TextRange; +use crate::diagnostic::{CompileDiagnostic, Diagnostic}; use crate::files::{File, FilePath}; use crate::source::source_text; use crate::Db; @@ -37,7 +40,20 @@ pub fn parsed_module(db: &dyn Db, file: File) -> ParsedModule { .map_or(PySourceType::Python, PySourceType::from_extension), }; - ParsedModule::new(parse_unchecked_source(&source, ty)) + let parsed = parse_unchecked_source(&source, ty); + + for error in parsed.errors() { + CompileDiagnostic::report( + db, + ParseDiagnostic { + error: error.error.to_string(), + range: error.location, + file, + }, + ); + } + + ParsedModule::new(parsed) } /// Cheap cloneable wrapper around the parsed module. @@ -73,6 +89,35 @@ impl std::fmt::Debug for ParsedModule { } } +#[derive(Debug)] +pub struct ParseDiagnostic { + error: String, + range: TextRange, + file: File, +} + +impl Diagnostic for ParseDiagnostic { + fn rule(&self) -> &str { + "syntax-error" + } + + fn message(&self) -> std::borrow::Cow { + Cow::Borrowed(&self.error) + } + + fn file(&self) -> File { + self.file + } + + fn range(&self) -> Option { + Some(self.range) + } + + fn severity(&self) -> crate::diagnostic::Severity { + crate::diagnostic::Severity::Error + } +} + #[cfg(test)] mod tests { use crate::files::{system_path_to_file, vendored_path_to_file}; diff --git a/crates/ruff_db/src/source.rs b/crates/ruff_db/src/source.rs index 7605d9f30c..12b977f58d 100644 --- a/crates/ruff_db/src/source.rs +++ b/crates/ruff_db/src/source.rs @@ -1,14 +1,13 @@ -use std::fmt::Formatter; use std::ops::Deref; use std::sync::Arc; use countme::Count; -use salsa::Accumulator; use ruff_notebook::Notebook; use ruff_python_ast::PySourceType; use ruff_source_file::LineIndex; +use crate::diagnostic::{CompileDiagnostic, Diagnostic}; use crate::files::{File, FilePath}; use crate::Db; @@ -25,8 +24,14 @@ pub fn source_text(db: &dyn Db, file: File) -> SourceText { tracing::debug!("Failed to read notebook '{path}': {error}"); has_read_error = true; - SourceDiagnostic(Arc::new(SourceTextError::FailedToReadNotebook(error))) - .accumulate(db); + CompileDiagnostic::report( + db, + SourceTextDiagnostic { + error: SourceTextError::FailedToReadNotebook(error), + file, + }, + ); + Notebook::empty() }) .into() @@ -36,7 +41,14 @@ pub fn source_text(db: &dyn Db, file: File) -> SourceText { tracing::debug!("Failed to read file '{path}': {error}"); has_read_error = true; - SourceDiagnostic(Arc::new(SourceTextError::FailedToReadFile(error))).accumulate(db); + CompileDiagnostic::report( + db, + SourceTextDiagnostic { + error: SourceTextError::FailedToReadFile(error), + file, + }, + ); + String::new() }) .into() @@ -153,21 +165,45 @@ impl From for SourceTextKind { } } -#[salsa::accumulator] -pub struct SourceDiagnostic(Arc); +#[derive(Debug)] +pub struct SourceTextDiagnostic { + error: SourceTextError, + file: File, +} -impl std::fmt::Display for SourceDiagnostic { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - std::fmt::Display::fmt(&self.0, f) +impl Diagnostic for SourceTextDiagnostic { + fn message(&self) -> std::borrow::Cow { + match &self.error { + SourceTextError::FailedToReadNotebook(notebook_error) => { + format!("Failed to read notebook: {notebook_error}").into() + } + SourceTextError::FailedToReadFile(error) => { + format!("Failed to read file: {error}").into() + } + } + } + + fn rule(&self) -> &str { + "io-error" + } + + fn file(&self) -> File { + self.file + } + + fn severity(&self) -> crate::diagnostic::Severity { + crate::diagnostic::Severity::Error + } + + fn range(&self) -> Option { + None } } -#[derive(Debug, thiserror::Error)] +#[derive(Debug)] pub enum SourceTextError { - #[error("Failed to read notebook: {0}`")] - FailedToReadNotebook(#[from] ruff_notebook::NotebookError), - #[error("Failed to read file: {0}")] - FailedToReadFile(#[from] std::io::Error), + FailedToReadNotebook(ruff_notebook::NotebookError), + FailedToReadFile(std::io::Error), } /// Computes the [`LineIndex`] for `file`.