From c65e3310d516ed400e3743dad96d2930cb961eca Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 20 Aug 2024 09:22:30 +0200 Subject: [PATCH] Add API to emit type-checking diagnostics (#12988) Co-authored-by: Alex Waygood --- crates/red_knot_python_semantic/src/db.rs | 13 +- .../src/semantic_index.rs | 4 + crates/red_knot_python_semantic/src/types.rs | 71 ++++++++++- .../src/types/diagnostic.rs | 111 +++++++++++++++++ .../src/types/infer.rs | 67 ++++++++-- crates/red_knot_wasm/src/lib.rs | 2 +- crates/red_knot_wasm/tests/api.rs | 5 +- crates/red_knot_workspace/src/db.rs | 20 ++- crates/red_knot_workspace/src/lint.rs | 116 ++---------------- crates/red_knot_workspace/src/workspace.rs | 59 +++++++-- crates/ruff_benchmark/benches/red_knot.rs | 5 +- crates/ruff_source_file/src/lib.rs | 6 + 12 files changed, 337 insertions(+), 142 deletions(-) create mode 100644 crates/red_knot_python_semantic/src/types/diagnostic.rs diff --git a/crates/red_knot_python_semantic/src/db.rs b/crates/red_knot_python_semantic/src/db.rs index c773199572937..c358d3e1cc351 100644 --- a/crates/red_knot_python_semantic/src/db.rs +++ b/crates/red_knot_python_semantic/src/db.rs @@ -1,15 +1,18 @@ +use ruff_db::files::File; use ruff_db::{Db as SourceDb, Upcast}; /// Database giving access to semantic information about a Python program. #[salsa::db] -pub trait Db: SourceDb + Upcast {} +pub trait Db: SourceDb + Upcast { + fn is_file_open(&self, file: File) -> bool; +} #[cfg(test)] pub(crate) mod tests { use std::sync::Arc; use crate::module_resolver::vendored_typeshed_stubs; - use ruff_db::files::Files; + use ruff_db::files::{File, Files}; use ruff_db::system::{DbWithTestSystem, System, TestSystem}; use ruff_db::vendored::VendoredFileSystem; use ruff_db::{Db as SourceDb, Upcast}; @@ -91,7 +94,11 @@ pub(crate) mod tests { } #[salsa::db] - impl Db for TestDb {} + impl Db for TestDb { + fn is_file_open(&self, file: File) -> bool { + !file.path(self).is_vendored_path() + } + } #[salsa::db] impl salsa::Database for TestDb { diff --git a/crates/red_knot_python_semantic/src/semantic_index.rs b/crates/red_knot_python_semantic/src/semantic_index.rs index 4ac39470c7ae7..0c5942f05f4d8 100644 --- a/crates/red_knot_python_semantic/src/semantic_index.rs +++ b/crates/red_knot_python_semantic/src/semantic_index.rs @@ -154,6 +154,10 @@ impl<'db> SemanticIndex<'db> { &self.scopes[id] } + pub(crate) fn scope_ids(&self) -> impl Iterator { + self.scope_ids_by_scope.iter().copied() + } + /// Returns the id of the parent scope. pub(crate) fn parent_scope_id(&self, scope_id: FileScopeId) -> Option { let scope = self.scope(scope_id); diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index bf6230d50fcb0..07dd1c6e1d48e 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -5,21 +5,37 @@ use crate::builtins::builtins_scope; use crate::semantic_index::definition::Definition; use crate::semantic_index::symbol::{ScopeId, ScopedSymbolId}; use crate::semantic_index::{ - global_scope, symbol_table, use_def_map, DefinitionWithConstraints, + global_scope, semantic_index, symbol_table, use_def_map, DefinitionWithConstraints, DefinitionWithConstraintsIterator, }; use crate::types::narrow::narrowing_constraint; use crate::{Db, FxOrderSet}; +pub(crate) use self::builder::{IntersectionBuilder, UnionBuilder}; +pub(crate) use self::diagnostic::TypeCheckDiagnostics; +pub(crate) use self::infer::{ + infer_definition_types, infer_expression_types, infer_scope_types, TypeInference, +}; + mod builder; +mod diagnostic; mod display; mod infer; mod narrow; -pub(crate) use self::builder::{IntersectionBuilder, UnionBuilder}; -pub(crate) use self::infer::{ - infer_definition_types, infer_expression_types, infer_scope_types, TypeInference, -}; +pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { + 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()); + } + + diagnostics +} /// Infer the public type of a symbol (its type as seen from outside its scope). pub(crate) fn symbol_ty<'db>( @@ -333,3 +349,48 @@ pub struct IntersectionType<'db> { /// directly in intersections rather than as a separate type. negative: FxOrderSet>, } + +#[cfg(test)] +mod tests { + use anyhow::Context; + + use ruff_db::files::system_path_to_file; + use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; + + use crate::db::tests::TestDb; + use crate::{Program, ProgramSettings, PythonVersion, SearchPathSettings}; + + #[test] + fn check_types() -> anyhow::Result<()> { + let mut db = TestDb::new(); + + db.write_file("src/foo.py", "import bar\n") + .context("Failed to write foo.py")?; + + Program::from_settings( + &db, + ProgramSettings { + target_version: PythonVersion::default(), + search_paths: SearchPathSettings { + extra_paths: Vec::new(), + src_root: SystemPathBuf::from("/src"), + site_packages: vec![], + custom_typeshed: None, + }, + }, + ) + .expect("Valid search path settings"); + + let foo = system_path_to_file(&db, "src/foo.py").context("Failed to resolve foo.py")?; + + let diagnostics = super::check_types(&db, foo); + + assert_eq!(diagnostics.len(), 1); + assert_eq!( + diagnostics[0].message(), + "Import 'bar' could not be resolved." + ); + + Ok(()) + } +} diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs new file mode 100644 index 0000000000000..3da2373d23ab8 --- /dev/null +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -0,0 +1,111 @@ +use ruff_db::files::File; +use ruff_text_size::{Ranged, TextRange}; +use std::fmt::Formatter; +use std::ops::Deref; +use std::sync::Arc; + +#[derive(Debug, Eq, PartialEq)] +pub struct TypeCheckDiagnostic { + // TODO: Don't use string keys for rules + pub(super) rule: String, + pub(super) message: String, + pub(super) range: TextRange, + pub(super) file: File, +} + +impl TypeCheckDiagnostic { + pub fn rule(&self) -> &str { + &self.rule + } + + pub fn message(&self) -> &str { + &self.message + } + + pub fn file(&self) -> File { + self.file + } +} + +impl Ranged for TypeCheckDiagnostic { + 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() } + } + + pub(super) fn push(&mut self, diagnostic: TypeCheckDiagnostic) { + self.inner.push(Arc::new(diagnostic)); + } + + 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() + } +} diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 7cad269146171..9cf7c47b776cb 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -29,7 +29,8 @@ use salsa::plumbing::AsId; use ruff_db::files::File; use ruff_db::parsed::parsed_module; use ruff_python_ast as ast; -use ruff_python_ast::{Expr, ExprContext}; +use ruff_python_ast::{AnyNodeRef, ExprContext}; +use ruff_text_size::Ranged; use crate::builtins::builtins_scope; use crate::module_name::ModuleName; @@ -40,6 +41,7 @@ use crate::semantic_index::expression::Expression; use crate::semantic_index::semantic_index; use crate::semantic_index::symbol::{FileScopeId, NodeWithScopeKind, NodeWithScopeRef, ScopeId}; use crate::semantic_index::SemanticIndex; +use crate::types::diagnostic::{TypeCheckDiagnostic, TypeCheckDiagnostics}; use crate::types::{ builtins_symbol_ty_by_name, definitions_ty, global_symbol_ty_by_name, ClassType, FunctionType, Name, Type, UnionBuilder, @@ -123,13 +125,16 @@ pub(crate) enum InferenceRegion<'db> { } /// The inferred types for a single region. -#[derive(Debug, Eq, PartialEq, Default, Clone)] +#[derive(Debug, Eq, PartialEq, Default)] pub(crate) struct TypeInference<'db> { /// The types of every expression in this region. expressions: FxHashMap>, /// The types of every definition in this region. definitions: FxHashMap, Type<'db>>, + + /// The diagnostics for this region. + diagnostics: TypeCheckDiagnostics, } impl<'db> TypeInference<'db> { @@ -142,9 +147,14 @@ impl<'db> TypeInference<'db> { self.definitions[&definition] } + pub(crate) fn diagnostics(&self) -> &[std::sync::Arc] { + &self.diagnostics + } + fn shrink_to_fit(&mut self) { self.expressions.shrink_to_fit(); self.definitions.shrink_to_fit(); + self.diagnostics.shrink_to_fit(); } } @@ -235,6 +245,7 @@ impl<'db> TypeInferenceBuilder<'db> { fn extend(&mut self, inference: &TypeInference<'db>) { self.types.definitions.extend(inference.definitions.iter()); self.types.expressions.extend(inference.expressions.iter()); + self.types.diagnostics.extend(&inference.diagnostics); } /// Infers types in the given [`InferenceRegion`]. @@ -855,7 +866,7 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - let module_ty = self.module_ty_from_name(ModuleName::new(name)); + let module_ty = self.module_ty_from_name(ModuleName::new(name), alias.into()); self.types.definitions.insert(definition, module_ty); } @@ -953,7 +964,7 @@ impl<'db> TypeInferenceBuilder<'db> { ModuleName::new(module_name) }; - let module_ty = self.module_ty_from_name(module_name); + let module_ty = self.module_ty_from_name(module_name, import_from.into()); let ast::Alias { range: _, @@ -984,10 +995,26 @@ impl<'db> TypeInferenceBuilder<'db> { } } - fn module_ty_from_name(&self, module_name: Option) -> Type<'db> { - module_name - .and_then(|module_name| resolve_module(self.db, module_name)) - .map_or(Type::Unknown, |module| Type::Module(module.file())) + fn module_ty_from_name( + &mut self, + module_name: Option, + node: AnyNodeRef, + ) -> Type<'db> { + let Some(module_name) = module_name else { + return Type::Unknown; + }; + + if let Some(module) = resolve_module(self.db, module_name.clone()) { + Type::Module(module.file()) + } else { + self.add_diagnostic( + node, + "unresolved-import", + format_args!("Import '{module_name}' could not be resolved."), + ); + + Type::Unknown + } } fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> { @@ -1059,7 +1086,7 @@ impl<'db> TypeInferenceBuilder<'db> { ast::Expr::Yield(yield_expression) => self.infer_yield_expression(yield_expression), ast::Expr::YieldFrom(yield_from) => self.infer_yield_from_expression(yield_from), ast::Expr::Await(await_expression) => self.infer_await_expression(await_expression), - Expr::IpyEscapeCommand(_) => todo!("Implement Ipy escape command support"), + ast::Expr::IpyEscapeCommand(_) => todo!("Implement Ipy escape command support"), }; let expr_id = expression.scoped_ast_id(self.db, self.scope); @@ -1706,6 +1733,28 @@ impl<'db> TypeInferenceBuilder<'db> { } } + /// Adds a new diagnostic. + /// + /// The diagnostic does not get added if the rule isn't enabled for this file. + fn add_diagnostic(&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.types.diagnostics.push(TypeCheckDiagnostic { + file: self.file, + rule: rule.to_string(), + message: message.to_string(), + range: node.range(), + }); + } + pub(super) fn finish(mut self) -> TypeInference<'db> { self.infer_region(); self.types.shrink_to_fit(); diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index 4bdfd9c9b2a5d..b2ab78c4f4093 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -109,7 +109,7 @@ 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.to_vec()) + Ok(result.clone()) } /// Checks all open files diff --git a/crates/red_knot_wasm/tests/api.rs b/crates/red_knot_wasm/tests/api.rs index 36eda60f06ba1..f6073d3cc382c 100644 --- a/crates/red_knot_wasm/tests/api.rs +++ b/crates/red_knot_wasm/tests/api.rs @@ -17,5 +17,8 @@ fn check() { let result = workspace.check_file(&test).expect("Check to succeed"); - assert_eq!(result, vec!["/test.py:1:8: Unresolved import 'random22'"]); + assert_eq!( + result, + vec!["/test.py:1:8: Import 'random22' could not be resolved.",] + ); } diff --git a/crates/red_knot_workspace/src/db.rs b/crates/red_knot_workspace/src/db.rs index 216885caf3899..f172ee0f1a19c 100644 --- a/crates/red_knot_workspace/src/db.rs +++ b/crates/red_knot_workspace/src/db.rs @@ -11,7 +11,6 @@ use ruff_db::{Db as SourceDb, Upcast}; use salsa::plumbing::ZalsaDatabase; use salsa::{Cancelled, Event}; -use crate::lint::Diagnostics; use crate::workspace::{check_file, Workspace, WorkspaceMetadata}; mod changes; @@ -61,7 +60,7 @@ impl RootDatabase { self.with_db(|db| db.workspace().check(db)) } - pub fn check_file(&self, file: File) -> Result { + pub fn check_file(&self, file: File) -> Result, Cancelled> { self.with_db(|db| check_file(db, file)) } @@ -115,7 +114,15 @@ impl Upcast for RootDatabase { } #[salsa::db] -impl SemanticDb for RootDatabase {} +impl SemanticDb for RootDatabase { + fn is_file_open(&self, file: File) -> bool { + let Some(workspace) = &self.workspace else { + return false; + }; + + workspace.is_file_open(self, file) + } +} #[salsa::db] impl SourceDb for RootDatabase { @@ -242,7 +249,12 @@ pub(crate) mod tests { } #[salsa::db] - impl red_knot_python_semantic::Db for TestDb {} + impl red_knot_python_semantic::Db for TestDb { + fn is_file_open(&self, file: ruff_db::files::File) -> bool { + !file.path(self).is_vendored_path() + } + } + #[salsa::db] impl Db for TestDb {} diff --git a/crates/red_knot_workspace/src/lint.rs b/crates/red_knot_workspace/src/lint.rs index 79ae41ecd3a9c..8fee8dd96865b 100644 --- a/crates/red_knot_workspace/src/lint.rs +++ b/crates/red_knot_workspace/src/lint.rs @@ -1,5 +1,4 @@ use std::cell::RefCell; -use std::ops::Deref; use std::time::Duration; use tracing::debug_span; @@ -22,7 +21,7 @@ use crate::db::Db; pub(crate) fn unwind_if_cancelled(db: &dyn Db) {} #[salsa::tracked(return_ref)] -pub(crate) fn lint_syntax(db: &dyn Db, file_id: File) -> Diagnostics { +pub(crate) fn lint_syntax(db: &dyn Db, file_id: File) -> Vec { #[allow(clippy::print_stdout)] if std::env::var("RED_KNOT_SLOW_LINT").is_ok() { for i in 0..10 { @@ -64,7 +63,7 @@ pub(crate) fn lint_syntax(db: &dyn Db, file_id: File) -> Diagnostics { })); } - Diagnostics::from(diagnostics) + diagnostics } fn lint_lines(source: &str, diagnostics: &mut Vec) { @@ -86,7 +85,7 @@ fn lint_lines(source: &str, diagnostics: &mut Vec) { #[allow(unreachable_pub)] #[salsa::tracked(return_ref)] -pub fn lint_semantic(db: &dyn Db, file_id: File) -> Diagnostics { +pub fn lint_semantic(db: &dyn Db, file_id: File) -> Vec { let _span = debug_span!("lint_semantic", file=%file_id.path(db)).entered(); let source = source_text(db.upcast(), file_id); @@ -94,7 +93,7 @@ pub fn lint_semantic(db: &dyn Db, file_id: File) -> Diagnostics { let semantic = SemanticModel::new(db.upcast(), file_id); if !parsed.is_valid() { - return Diagnostics::Empty; + return vec![]; } let context = SemanticLintContext { @@ -106,7 +105,7 @@ pub fn lint_semantic(db: &dyn Db, file_id: File) -> Diagnostics { SemanticVisitor { context: &context }.visit_body(parsed.suite()); - Diagnostics::from(context.diagnostics.take()) + context.diagnostics.take() } fn format_diagnostic(context: &SemanticLintContext, message: &str, start: TextSize) -> String { @@ -116,48 +115,13 @@ fn format_diagnostic(context: &SemanticLintContext, message: &str, start: TextSi .source_location(start, context.source_text()); format!( "{}:{}:{}: {}", - context.semantic.file_path().as_str(), + context.semantic.file_path(), source_location.row, source_location.column, message, ) } -fn lint_unresolved_imports(context: &SemanticLintContext, import: AnyImportRef) { - // TODO: this treats any symbol with `Type::Unknown` as an unresolved import, - // which isn't really correct: if it exists but has `Type::Unknown` in the - // module we're importing it from, we shouldn't really emit a diagnostic here, - // but currently do. - match import { - AnyImportRef::Import(import) => { - for alias in &import.names { - let ty = alias.ty(&context.semantic); - - if ty.is_unknown() { - context.push_diagnostic(format_diagnostic( - context, - &format!("Unresolved import '{}'", &alias.name), - alias.start(), - )); - } - } - } - AnyImportRef::ImportFrom(import) => { - for alias in &import.names { - let ty = alias.ty(&context.semantic); - - if ty.is_unknown() { - context.push_diagnostic(format_diagnostic( - context, - &format!("Unresolved import '{}'", &alias.name), - alias.start(), - )); - } - } - } - } -} - fn lint_maybe_undefined(context: &SemanticLintContext, name: &ast::ExprName) { if !matches!(name.ctx, ast::ExprContext::Load) { return; @@ -280,17 +244,8 @@ struct SemanticVisitor<'a> { impl Visitor<'_> for SemanticVisitor<'_> { fn visit_stmt(&mut self, stmt: &ast::Stmt) { - match stmt { - ast::Stmt::ClassDef(class) => { - lint_bad_override(self.context, class); - } - ast::Stmt::Import(import) => { - lint_unresolved_imports(self.context, AnyImportRef::Import(import)); - } - ast::Stmt::ImportFrom(import) => { - lint_unresolved_imports(self.context, AnyImportRef::ImportFrom(import)); - } - _ => {} + if let ast::Stmt::ClassDef(class) = stmt { + lint_bad_override(self.context, class); } walk_stmt(self, stmt); @@ -308,53 +263,6 @@ impl Visitor<'_> for SemanticVisitor<'_> { } } -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum Diagnostics { - Empty, - List(Vec), -} - -impl Diagnostics { - pub fn as_slice(&self) -> &[String] { - match self { - Diagnostics::Empty => &[], - Diagnostics::List(list) => list.as_slice(), - } - } -} - -impl Deref for Diagnostics { - type Target = [String]; - fn deref(&self) -> &Self::Target { - self.as_slice() - } -} - -impl From> for Diagnostics { - fn from(value: Vec) -> Self { - if value.is_empty() { - Diagnostics::Empty - } else { - Diagnostics::List(value) - } - } -} - -#[derive(Copy, Clone, Debug)] -enum AnyImportRef<'a> { - Import(&'a ast::StmtImport), - ImportFrom(&'a ast::StmtImportFrom), -} - -impl Ranged for AnyImportRef<'_> { - fn range(&self) -> ruff_text_size::TextRange { - match self { - AnyImportRef::Import(import) => import.range(), - AnyImportRef::ImportFrom(import) => import.range(), - } - } -} - #[cfg(test)] mod tests { use red_knot_python_semantic::{Program, ProgramSettings, PythonVersion, SearchPathSettings}; @@ -363,7 +271,7 @@ mod tests { use crate::db::tests::TestDb; - use super::{lint_semantic, Diagnostics}; + use super::lint_semantic; fn setup_db() -> TestDb { setup_db_with_root(SystemPathBuf::from("/src")) @@ -409,9 +317,9 @@ mod tests { .unwrap(); let file = system_path_to_file(&db, "/src/a.py").expect("file to exist"); - let Diagnostics::List(messages) = lint_semantic(&db, file) else { - panic!("expected some diagnostics"); - }; + let messages = lint_semantic(&db, file); + + assert_ne!(messages, &[] as &[String], "expected some diagnostics"); assert_eq!( *messages, diff --git a/crates/red_knot_workspace/src/workspace.rs b/crates/red_knot_workspace/src/workspace.rs index d5a24b64a65a6..fdbbdcd6eb62a 100644 --- a/crates/red_knot_workspace/src/workspace.rs +++ b/crates/red_knot_workspace/src/workspace.rs @@ -4,17 +4,19 @@ use rustc_hash::{FxBuildHasher, FxHashSet}; use salsa::{Durability, Setter as _}; pub use metadata::{PackageMetadata, WorkspaceMetadata}; -use ruff_db::source::{source_text, SourceDiagnostic}; +use red_knot_python_semantic::types::check_types; +use ruff_db::source::{line_index, source_text, SourceDiagnostic}; 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::workspace::files::{Index, Indexed, PackageFiles}; use crate::{ db::Db, - lint::{lint_semantic, lint_syntax, Diagnostics}, + lint::{lint_semantic, lint_syntax}, }; mod files; @@ -92,8 +94,8 @@ pub struct Package { root_buf: SystemPathBuf, /// The files that are part of this package. - #[return_ref] #[default] + #[return_ref] file_set: PackageFiles, // TODO: Add the loaded settings. } @@ -249,6 +251,23 @@ impl Workspace { FxHashSet::default() } } + + /// Returns `true` if the file is open in the workspace. + /// + /// A file is considered open when: + /// * explicitly set as an open file using [`open_file`](Self::open_file) + /// * It has a [`SystemPath`] and belongs to a package's `src` files + /// * It has a [`SystemVirtualPath`](ruff_db::system::SystemVirtualPath) + pub fn is_file_open(self, db: &dyn Db, file: File) -> bool { + if let Some(open_files) = self.open_files(db) { + open_files.contains(&file) + } else if let Some(system_path) = file.path(db).as_system_path() { + self.package(db, system_path) + .map_or(false, |package| package.contains_file(db, file)) + } else { + file.path(db).is_system_virtual_path() + } + } } #[salsa::tracked] @@ -309,8 +328,12 @@ impl Package { let _entered = tracing::debug_span!("index_package_files", package = %self.name(db)).entered(); - tracing::debug!("Indexing files for package {}", self.name(db)); let files = discover_package_files(db, self.root(db)); + tracing::info!( + "Indexed {} files for package '{}'", + files.len(), + self.name(db) + ); vacant.set(files) } Index::Indexed(indexed) => indexed, @@ -348,7 +371,7 @@ impl Package { } #[salsa::tracked] -pub(super) fn check_file(db: &dyn Db, file: File) -> Diagnostics { +pub(super) fn check_file(db: &dyn Db, file: File) -> Vec { let path = file.path(db); let _span = tracing::debug_span!("check_file", file=%path).entered(); tracing::debug!("Checking file {path}"); @@ -364,13 +387,25 @@ pub(super) fn check_file(db: &dyn Db, file: File) -> Diagnostics { ); // Abort checking if there are IO errors. - if source_text(db.upcast(), file).has_read_error() { - return Diagnostics::from(diagnostics); + let source = source_text(db.upcast(), file); + + if source.has_read_error() { + return diagnostics; + } + + 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.extend_from_slice(lint_syntax(db, file)); diagnostics.extend_from_slice(lint_semantic(db, file)); - Diagnostics::from(diagnostics) + diagnostics } fn discover_package_files(db: &dyn Db, path: &SystemPath) -> FxHashSet { @@ -424,7 +459,7 @@ mod tests { use ruff_db::testing::assert_function_query_was_not_run; use crate::db::tests::TestDb; - use crate::lint::{lint_syntax, Diagnostics}; + use crate::lint::lint_syntax; use crate::workspace::check_file; #[test] @@ -442,9 +477,7 @@ mod tests { assert_eq!(source_text(&db, file).as_str(), ""); assert_eq!( check_file(&db, file), - Diagnostics::List(vec![ - "Failed to read file: No such file or directory".to_string() - ]) + vec!["Failed to read file: No such file or directory".to_string()] ); let events = db.take_salsa_events(); @@ -455,7 +488,7 @@ mod tests { db.write_file(path, "").unwrap(); assert_eq!(source_text(&db, file).as_str(), ""); - assert_eq!(check_file(&db, file), Diagnostics::Empty); + assert_eq!(check_file(&db, file), vec![] as Vec); Ok(()) } diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 4126dda09ecf4..7c9b6461b53ad 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -18,6 +18,7 @@ struct Case { } const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/8e8a4baf652f6e1cee7acde9d78c4b6154539748/Lib/tomllib"; +const EXPECTED_DIAGNOSTICS: usize = 27; fn get_test_file(name: &str) -> TestFile { let path = format!("tomllib/{name}"); @@ -89,7 +90,7 @@ fn benchmark_incremental(criterion: &mut Criterion) { let Case { db, parser, .. } = case; let result = db.check_file(*parser).unwrap(); - assert_eq!(result.len(), 34); + assert_eq!(result.len(), EXPECTED_DIAGNOSTICS); }, BatchSize::SmallInput, ); @@ -104,7 +105,7 @@ fn benchmark_cold(criterion: &mut Criterion) { let Case { db, parser, .. } = case; let result = db.check_file(*parser).unwrap(); - assert_eq!(result.len(), 34); + assert_eq!(result.len(), EXPECTED_DIAGNOSTICS); }, BatchSize::SmallInput, ); diff --git a/crates/ruff_source_file/src/lib.rs b/crates/ruff_source_file/src/lib.rs index b5c2b85bfd24b..078c50cdc21e8 100644 --- a/crates/ruff_source_file/src/lib.rs +++ b/crates/ruff_source_file/src/lib.rs @@ -254,6 +254,12 @@ impl Debug for SourceLocation { } } +impl std::fmt::Display for SourceLocation { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{row}:{column}", row = self.row, column = self.column) + } +} + #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] pub enum SourceRow { /// A row within a cell in a Jupyter Notebook.