From f3d7ea97a581865e24e6cd3983be387bedbbd7d3 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 26 Apr 2024 14:06:06 +0200 Subject: [PATCH] [red-knot] Unresolved imports lint rule --- crates/red_knot/src/cancellation.rs | 2 +- crates/red_knot/src/db.rs | 28 +++--- crates/red_knot/src/lib.rs | 7 ++ crates/red_knot/src/lint.rs | 130 ++++++++++++++++++++++++++- crates/red_knot/src/program/check.rs | 9 +- crates/red_knot/src/program/mod.rs | 17 ++-- crates/red_knot/src/symbols.rs | 38 ++++++-- crates/red_knot/src/types.rs | 46 ++++++++-- crates/red_knot/src/types/infer.rs | 48 ++++++---- 9 files changed, 271 insertions(+), 54 deletions(-) diff --git a/crates/red_knot/src/cancellation.rs b/crates/red_knot/src/cancellation.rs index 05610ce279f2a1..0620d86ab5e761 100644 --- a/crates/red_knot/src/cancellation.rs +++ b/crates/red_knot/src/cancellation.rs @@ -12,7 +12,7 @@ impl CancellationTokenSource { } } - #[tracing::instrument(level = "trace")] + #[tracing::instrument(level = "trace", skip_all)] pub fn cancel(&self) { let (cancelled, condvar) = &*self.signal; diff --git a/crates/red_knot/src/db.rs b/crates/red_knot/src/db.rs index 9cea81808c3df3..21685a5734f687 100644 --- a/crates/red_knot/src/db.rs +++ b/crates/red_knot/src/db.rs @@ -2,7 +2,7 @@ use std::path::Path; use std::sync::Arc; use crate::files::FileId; -use crate::lint::{Diagnostics, LintSyntaxStorage}; +use crate::lint::{Diagnostics, LintSemanticStorage, LintSyntaxStorage}; use crate::module::{Module, ModuleData, ModuleName, ModuleResolver, ModuleSearchPath}; use crate::parse::{Parsed, ParsedStorage}; use crate::source::{Source, SourceStorage}; @@ -32,13 +32,15 @@ pub trait SemanticDb: SourceDb { fn symbol_table(&self, file_id: FileId) -> Arc; + fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type; + + fn lint_semantic(&self, file_id: FileId) -> Diagnostics; + // mutations fn add_module(&mut self, path: &Path) -> Option<(Module, Vec>)>; fn set_module_search_paths(&mut self, paths: Vec); - - fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type; } pub trait Db: SemanticDb {} @@ -55,6 +57,7 @@ pub struct SemanticJar { pub module_resolver: ModuleResolver, pub symbol_tables: SymbolTablesStorage, pub type_store: TypeStore, + pub lint_semantic: LintSemanticStorage, } /// Gives access to a specific jar in the database. @@ -79,9 +82,12 @@ pub trait HasJar { #[cfg(test)] pub(crate) mod tests { + use std::path::Path; + use std::sync::Arc; + use crate::db::{HasJar, SourceDb, SourceJar}; use crate::files::{FileId, Files}; - use crate::lint::{lint_syntax, Diagnostics}; + use crate::lint::{lint_semantic, lint_syntax, Diagnostics}; use crate::module::{ add_module, file_to_module, path_to_module, resolve_module, set_module_search_paths, Module, ModuleData, ModuleName, ModuleSearchPath, @@ -90,8 +96,6 @@ pub(crate) mod tests { use crate::source::{source_text, Source}; use crate::symbols::{symbol_table, SymbolId, SymbolTable}; use crate::types::{infer_symbol_type, Type}; - use std::path::Path; - use std::sync::Arc; use super::{SemanticDb, SemanticJar}; @@ -163,6 +167,14 @@ pub(crate) mod tests { symbol_table(self, file_id) } + fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type { + infer_symbol_type(self, file_id, symbol_id) + } + + fn lint_semantic(&self, file_id: FileId) -> Diagnostics { + lint_semantic(self, file_id) + } + fn add_module(&mut self, path: &Path) -> Option<(Module, Vec>)> { add_module(self, path) } @@ -170,9 +182,5 @@ pub(crate) mod tests { fn set_module_search_paths(&mut self, paths: Vec) { set_module_search_paths(self, paths); } - - fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type { - infer_symbol_type(self, file_id, symbol_id) - } } } diff --git a/crates/red_knot/src/lib.rs b/crates/red_knot/src/lib.rs index bf321f8e3b255c..574ab811412271 100644 --- a/crates/red_knot/src/lib.rs +++ b/crates/red_knot/src/lib.rs @@ -1,3 +1,4 @@ +use std::fmt::Formatter; use std::hash::BuildHasherDefault; use std::ops::Deref; use std::path::{Path, PathBuf}; @@ -100,3 +101,9 @@ where Self(value.into()) } } + +impl std::fmt::Display for Name { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} diff --git a/crates/red_knot/src/lint.rs b/crates/red_knot/src/lint.rs index d787e00d8cd1cd..e1534fde96e147 100644 --- a/crates/red_knot/src/lint.rs +++ b/crates/red_knot/src/lint.rs @@ -1,12 +1,17 @@ +use std::cell::RefCell; use std::ops::{Deref, DerefMut}; use std::sync::Arc; use ruff_python_ast::visitor::Visitor; -use ruff_python_ast::StringLiteral; +use ruff_python_ast::{ModModule, StringLiteral}; use crate::cache::KeyValueCache; -use crate::db::{HasJar, SourceDb, SourceJar}; +use crate::db::{HasJar, SemanticDb, SemanticJar, SourceDb, SourceJar}; use crate::files::FileId; +use crate::parse::Parsed; +use crate::source::Source; +use crate::symbols::{Definition, SymbolId, SymbolTable}; +use crate::types::Type; #[tracing::instrument(level = "debug", skip(db))] pub(crate) fn lint_syntax(db: &Db, file_id: FileId) -> Diagnostics @@ -40,7 +45,7 @@ where }) } -pub(crate) fn lint_lines(source: &str, diagnostics: &mut Vec) { +fn lint_lines(source: &str, diagnostics: &mut Vec) { for (line_number, line) in source.lines().enumerate() { if line.len() < 88 { continue; @@ -57,6 +62,108 @@ pub(crate) fn lint_lines(source: &str, diagnostics: &mut Vec) { } } +#[tracing::instrument(level = "debug", skip(db))] +pub(crate) fn lint_semantic(db: &Db, file_id: FileId) -> Diagnostics +where + Db: SemanticDb + HasJar, +{ + let storage = &db.jar().lint_semantic; + + storage.get(&file_id, |file_id| { + let source = db.source(*file_id); + let parsed = db.parse(*file_id); + let symbols = db.symbol_table(*file_id); + + let context = SemanticLintContext { + file_id: *file_id, + source, + parsed, + symbols, + db, + diagnostics: RefCell::new(Vec::new()), + }; + + lint_unresolved_imports(&context); + + Diagnostics::from(context.diagnostics.take()) + }) +} + +fn lint_unresolved_imports(context: &SemanticLintContext) { + for (symbol, definition) in context.symbols().all_definitions() { + match definition { + Definition::Import(import) => { + let ty = context.eval_symbol(symbol); + + if ty.is_unknown() { + context.push_diagnostic(format!("Unresolved module {}", import.module)); + } + } + Definition::ImportFrom(import) => { + let ty = context.eval_symbol(symbol); + + if ty.is_unknown() { + let message = if let Some(module) = import.module() { + format!( + "Unresolved relative import '{}' from '{}{module}'", + import.name(), + ".".repeat(import.level() as usize) + ) + } else { + format!( + "Unresolved import '{}' from {}", + import.name(), + ".".repeat(import.level() as usize) + ) + }; + + context.push_diagnostic(message); + } + } + _ => {} + } + } +} + +pub struct SemanticLintContext<'a> { + file_id: FileId, + source: Source, + parsed: Parsed, + symbols: Arc, + db: &'a dyn SemanticDb, + diagnostics: RefCell>, +} + +impl<'a> SemanticLintContext<'a> { + pub fn source_text(&self) -> &str { + self.source.text() + } + + pub fn file_id(&self) -> FileId { + self.file_id + } + + pub fn ast(&self) -> &ModModule { + self.parsed.ast() + } + + pub fn symbols(&self) -> &SymbolTable { + &self.symbols + } + + pub fn eval_symbol(&self, symbol_id: SymbolId) -> Type { + self.db.infer_symbol_type(self.file_id, symbol_id) + } + + pub fn push_diagnostic(&self, diagnostic: String) { + self.diagnostics.borrow_mut().push(diagnostic); + } + + pub fn extend_diagnostics(&mut self, diagnostics: impl IntoIterator) { + self.diagnostics.get_mut().extend(diagnostics); + } +} + #[derive(Debug)] struct SyntaxLintVisitor<'a> { diagnostics: Vec, @@ -123,3 +230,20 @@ impl DerefMut for LintSyntaxStorage { &mut self.0 } } + +#[derive(Default, Debug)] +pub struct LintSemanticStorage(KeyValueCache); + +impl Deref for LintSemanticStorage { + type Target = KeyValueCache; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for LintSemanticStorage { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} diff --git a/crates/red_knot/src/program/check.rs b/crates/red_knot/src/program/check.rs index 0154a17d615300..66a140e1d11ba7 100644 --- a/crates/red_knot/src/program/check.rs +++ b/crates/red_knot/src/program/check.rs @@ -1,11 +1,13 @@ +use std::num::NonZeroUsize; + +use rayon::max_num_threads; +use rustc_hash::FxHashSet; + use crate::cancellation::CancellationToken; use crate::db::{SemanticDb, SourceDb}; use crate::files::FileId; use crate::lint::Diagnostics; use crate::program::Program; -use rayon::max_num_threads; -use rustc_hash::FxHashSet; -use std::num::NonZeroUsize; impl Program { /// Checks all open files in the workspace and its dependencies. @@ -71,6 +73,7 @@ impl Program { if self.workspace().is_file_open(file) { diagnostics.extend_from_slice(&self.lint_syntax(file)); + diagnostics.extend_from_slice(&self.lint_semantic(file)); } Ok(Diagnostics::from(diagnostics)) diff --git a/crates/red_knot/src/program/mod.rs b/crates/red_knot/src/program/mod.rs index 0070d530869c79..63ea427065e7a9 100644 --- a/crates/red_knot/src/program/mod.rs +++ b/crates/red_knot/src/program/mod.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use crate::db::{Db, HasJar, SemanticDb, SemanticJar, SourceDb, SourceJar}; use crate::files::{FileId, Files}; -use crate::lint::{lint_syntax, Diagnostics, LintSyntaxStorage}; +use crate::lint::{lint_semantic, lint_syntax, Diagnostics, LintSyntaxStorage}; use crate::module::{ add_module, file_to_module, path_to_module, resolve_module, set_module_search_paths, Module, ModuleData, ModuleName, ModuleResolver, ModuleSearchPath, @@ -36,6 +36,7 @@ impl Program { module_resolver: ModuleResolver::new(module_search_paths), symbol_tables: SymbolTablesStorage::default(), type_store: TypeStore::default(), + lint_semantic: LintSemanticStorage::default(), }, files: Files::default(), workspace, @@ -56,6 +57,7 @@ impl Program { self.source.lint_syntax.remove(&change.id); // TODO: remove all dependent modules as well self.semantic.type_store.remove_module(change.id); + self.semantic.lint_semantic.remove(&change.id); } } @@ -111,8 +113,15 @@ impl SemanticDb for Program { symbol_table(self, file_id) } - // Mutations + fn infer_symbol_type(&self, file_id: FileId, symbol_id: SymbolId) -> Type { + infer_symbol_type(self, file_id, symbol_id) + } + + fn lint_semantic(&self, file_id: FileId) -> Diagnostics { + lint_semantic(self, file_id) + } + // Mutations fn add_module(&mut self, path: &Path) -> Option<(Module, Vec>)> { add_module(self, path) } @@ -120,10 +129,6 @@ impl SemanticDb for Program { fn set_module_search_paths(&mut self, paths: Vec) { set_module_search_paths(self, paths); } - - fn infer_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId) -> Type { - infer_symbol_type(self, file_id, symbol_id) - } } impl Db for Program {} diff --git a/crates/red_knot/src/symbols.rs b/crates/red_knot/src/symbols.rs index 6f82522d36f667..98eeb5d26618a2 100644 --- a/crates/red_knot/src/symbols.rs +++ b/crates/red_knot/src/symbols.rs @@ -121,6 +121,20 @@ pub(crate) struct ImportFromDefinition { pub(crate) level: u32, } +impl ImportFromDefinition { + pub(crate) fn module(&self) -> Option<&Name> { + self.module.as_ref() + } + + pub(crate) fn name(&self) -> &Name { + &self.name + } + + pub(crate) fn level(&self) -> u32 { + self.level + } +} + #[derive(Debug, Clone)] pub(crate) enum Dependency { Module(Name), @@ -253,13 +267,19 @@ impl SymbolTable { self.symbol_by_name(SymbolTable::root_scope_id(), name) } - pub(crate) fn defs(&self, symbol_id: SymbolId) -> &[Definition] { + pub(crate) fn definitions(&self, symbol_id: SymbolId) -> &[Definition] { self.defs .get(&symbol_id) .map(std::vec::Vec::as_slice) .unwrap_or_default() } + pub(crate) fn all_definitions(&self) -> impl Iterator + '_ { + self.defs + .iter() + .flat_map(|(sym_id, defs)| defs.iter().map(move |def| (*sym_id, def))) + } + fn add_symbol_to_scope(&mut self, scope_id: ScopeId, name: &str) -> SymbolId { let hash = SymbolTable::hash_name(name); let scope = &mut self.scopes_by_id[scope_id]; @@ -584,7 +604,9 @@ mod tests { let table = SymbolTable::from_ast(parsed.ast()); assert_eq!(names(table.root_symbols()), vec!["x"]); assert_eq!( - table.defs(table.root_symbol_id_by_name("x").unwrap()).len(), + table + .definitions(table.root_symbol_id_by_name("x").unwrap()) + .len(), 0 ); } @@ -604,7 +626,7 @@ mod tests { assert_eq!(names(table.root_symbols()), vec!["foo"]); assert_eq!( table - .defs(table.root_symbol_id_by_name("foo").unwrap()) + .definitions(table.root_symbol_id_by_name("foo").unwrap()) .len(), 1 ); @@ -631,7 +653,7 @@ mod tests { assert_eq!(names(table.root_symbols()), vec!["foo"]); assert_eq!( table - .defs(table.root_symbol_id_by_name("foo").unwrap()) + .definitions(table.root_symbol_id_by_name("foo").unwrap()) .len(), 1 ); @@ -655,7 +677,9 @@ mod tests { assert_eq!(c_scope.name(), "C"); assert_eq!(names(table.symbols_for_scope(scopes[0])), vec!["x"]); assert_eq!( - table.defs(table.root_symbol_id_by_name("C").unwrap()).len(), + table + .definitions(table.root_symbol_id_by_name("C").unwrap()) + .len(), 1 ); } @@ -679,7 +703,7 @@ mod tests { assert_eq!(names(table.symbols_for_scope(scopes[0])), vec!["x"]); assert_eq!( table - .defs(table.root_symbol_id_by_name("func").unwrap()) + .definitions(table.root_symbol_id_by_name("func").unwrap()) .len(), 1 ); @@ -709,7 +733,7 @@ mod tests { assert_eq!(names(table.symbols_for_scope(scopes[1])), vec!["y"]); assert_eq!( table - .defs(table.root_symbol_id_by_name("func").unwrap()) + .definitions(table.root_symbol_id_by_name("func").unwrap()) .len(), 2 ); diff --git a/crates/red_knot/src/types.rs b/crates/red_knot/src/types.rs index 7c084445fcea58..e6d2180057af00 100644 --- a/crates/red_knot/src/types.rs +++ b/crates/red_knot/src/types.rs @@ -35,6 +35,38 @@ impl Type { fn display<'a>(&'a self, store: &'a TypeStore) -> DisplayType<'a> { DisplayType { ty: self, store } } + + pub const fn is_unbound(&self) -> bool { + matches!(self, Type::Unbound) + } + + pub const fn is_unknown(&self) -> bool { + matches!(self, Type::Unknown) + } +} + +impl From for Type { + fn from(id: FunctionTypeId) -> Self { + Type::Function(id) + } +} + +impl From for Type { + fn from(id: ClassTypeId) -> Self { + Type::Class(id) + } +} + +impl From for Type { + fn from(id: UnionTypeId) -> Self { + Type::Union(id) + } +} + +impl From for Type { + fn from(id: IntersectionTypeId) -> Self { + Type::Intersection(id) + } } // TODO: currently calling `get_function` et al and holding on to the `FunctionTypeRef` will lock a @@ -51,13 +83,13 @@ impl TypeStore { self.modules.remove(&file_id); } - pub fn cache_symbol_type(&mut self, file_id: FileId, symbol_id: SymbolId, ty: Type) { + pub fn cache_symbol_type(&self, file_id: FileId, symbol_id: SymbolId, ty: Type) { self.add_or_get_module(file_id) .symbol_types .insert(symbol_id, ty); } - pub fn cache_node_type(&mut self, file_id: FileId, node_key: NodeKey, ty: Type) { + pub fn cache_node_type(&self, file_id: FileId, node_key: NodeKey, ty: Type) { self.add_or_get_module(file_id) .node_types .insert(node_key, ty); @@ -77,7 +109,7 @@ impl TypeStore { .copied() } - fn add_or_get_module(&mut self, file_id: FileId) -> ModuleStoreRefMut { + fn add_or_get_module(&self, file_id: FileId) -> ModuleStoreRefMut { self.modules .entry(file_id) .or_insert_with(|| ModuleTypeStore::new(file_id)) @@ -91,11 +123,11 @@ impl TypeStore { self.modules.get(&file_id) } - fn add_function(&mut self, file_id: FileId, name: &str) -> FunctionTypeId { + fn add_function(&self, file_id: FileId, name: &str) -> FunctionTypeId { self.add_or_get_module(file_id).add_function(name) } - fn add_class(&mut self, file_id: FileId, name: &str) -> ClassTypeId { + fn add_class(&self, file_id: FileId, name: &str) -> ClassTypeId { self.add_or_get_module(file_id).add_class(name) } @@ -454,7 +486,7 @@ mod tests { #[test] fn add_class() { - let mut store = TypeStore::default(); + let store = TypeStore::default(); let files = Files::default(); let file_id = files.intern(Path::new("/foo")); let id = store.add_class(file_id, "C"); @@ -465,7 +497,7 @@ mod tests { #[test] fn add_function() { - let mut store = TypeStore::default(); + let store = TypeStore::default(); let files = Files::default(); let file_id = files.intern(Path::new("/foo")); let id = store.add_function(file_id, "func"); diff --git a/crates/red_knot/src/types/infer.rs b/crates/red_knot/src/types/infer.rs index eb44dc9cb1b3c6..fb767ab9664d97 100644 --- a/crates/red_knot/src/types/infer.rs +++ b/crates/red_knot/src/types/infer.rs @@ -1,20 +1,22 @@ #![allow(dead_code)] + +use ruff_python_ast::AstNode; + use crate::db::{HasJar, SemanticDb, SemanticJar}; use crate::module::ModuleName; use crate::symbols::{Definition, ImportFromDefinition, SymbolId}; use crate::types::Type; use crate::FileId; -use ruff_python_ast::AstNode; // TODO this should not take a &mut db, it should be a query, not a mutation. This means we'll need // to use interior mutability in TypeStore instead, and avoid races in populating the cache. #[tracing::instrument(level = "trace", skip(db))] -pub fn infer_symbol_type(db: &mut Db, file_id: FileId, symbol_id: SymbolId) -> Type +pub fn infer_symbol_type(db: &Db, file_id: FileId, symbol_id: SymbolId) -> Type where Db: SemanticDb + HasJar, { let symbols = db.symbol_table(file_id); - let defs = symbols.defs(symbol_id); + let defs = symbols.definitions(symbol_id); if let Some(ty) = db .jar() @@ -48,28 +50,40 @@ where Type::Unknown } } - Definition::ClassDef(node_key) => { - if let Some(ty) = db - .jar() - .type_store - .get_cached_node_type(file_id, node_key.erased()) - { - ty - } else { + Definition::ClassDef(node_key) => db + .jar() + .type_store + .get_cached_node_type(file_id, node_key.erased()) + .unwrap_or_else(|| { let parsed = db.parse(file_id); let ast = parsed.ast(); let node = node_key.resolve_unwrap(ast.as_any_node_ref()); - let store = &mut db.jar_mut().type_store; - let ty = Type::Class(store.add_class(file_id, &node.name.id)); + let store = &db.jar().type_store; + let ty: Type = store.add_class(file_id, &node.name.id).into(); store.cache_node_type(file_id, *node_key.erased(), ty); ty - } - } + }), + Definition::FunctionDef(node_key) => db + .jar() + .type_store + .get_cached_node_type(file_id, node_key.erased()) + .unwrap_or_else(|| { + let parsed = db.parse(file_id); + let ast = parsed.ast(); + let node = node_key + .resolve(ast.as_any_node_ref()) + .expect("node key should resolve"); + + let store = &db.jar().type_store; + let ty = store.add_function(file_id, &node.name.id).into(); + store.cache_node_type(file_id, *node_key.erased(), ty); + ty + }), _ => todo!("other kinds of definitions"), }; - db.jar_mut() + db.jar() .type_store .cache_symbol_type(file_id, symbol_id, ty); // TODO record dependencies @@ -112,7 +126,7 @@ mod tests { fn follow_import_to_class() -> std::io::Result<()> { let TestCase { src, - mut db, + db, temp_dir: _temp_dir, } = create_test()?;