From ecd9e6a650ef428be67bb0e28cb0c52d27eb2895 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 21 Aug 2024 14:44:49 +0100 Subject: [PATCH] [red-knot] Improve the `unresolved-import` check (#13007) Co-authored-by: Micha Reiser --- crates/red_knot_python_semantic/src/types.rs | 100 ++++++++++++++-- .../src/types/infer.rs | 113 +++++++++++++----- crates/ruff_benchmark/benches/red_knot.rs | 12 ++ 3 files changed, 184 insertions(+), 41 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 272d0bfb0cdcb..50109fd19488c 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -222,6 +222,19 @@ impl<'db> Type<'db> { } } + /// Resolve a member access of a type. + /// + /// For example, if `foo` is `Type::Instance()`, + /// `foo.member(&db, "baz")` returns the type of `baz` attributes + /// as accessed from instances of the `Bar` class. + /// + /// TODO: use of this method currently requires manually checking + /// whether the returned type is `Unknown`/`Unbound` + /// (or a union with `Unknown`/`Unbound`) in many places. + /// Ideally we'd use a more type-safe pattern, such as returning + /// an `Option` or a `Result` from this method, which would force + /// us to explicitly consider whether to handle an error or propagate + /// it up the call stack. #[must_use] pub fn member(&self, db: &'db dyn Db, name: &Name) -> Type<'db> { match self { @@ -369,12 +382,13 @@ mod tests { use crate::db::tests::TestDb; use crate::{Program, ProgramSettings, PythonVersion, SearchPathSettings}; - #[test] - fn check_types() -> anyhow::Result<()> { - let mut db = TestDb::new(); + use super::TypeCheckDiagnostics; - db.write_file("src/foo.py", "import bar\n") - .context("Failed to write foo.py")?; + fn setup_db() -> TestDb { + let db = TestDb::new(); + db.memory_file_system() + .create_directory_all("/src") + .unwrap(); Program::from_settings( &db, @@ -390,16 +404,82 @@ mod tests { ) .expect("Valid search path settings"); + db + } + + fn assert_diagnostic_messages(diagnostics: &TypeCheckDiagnostics, expected: &[&str]) { + let messages: Vec<&str> = diagnostics + .iter() + .map(|diagnostic| diagnostic.message()) + .collect(); + assert_eq!(&messages, expected); + } + + #[test] + fn unresolved_import_statement() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_file("src/foo.py", "import bar\n") + .context("Failed to write foo.py")?; + let foo = system_path_to_file(&db, "src/foo.py").context("Failed to resolve foo.py")?; let diagnostics = super::check_types(&db, foo); + assert_diagnostic_messages(&diagnostics, &["Import 'bar' could not be resolved."]); + + Ok(()) + } + + #[test] + fn unresolved_import_from_statement() { + let mut db = setup_db(); + + db.write_file("src/foo.py", "from bar import baz\n") + .unwrap(); + let foo = system_path_to_file(&db, "src/foo.py").unwrap(); + let diagnostics = super::check_types(&db, foo); + assert_diagnostic_messages(&diagnostics, &["Import 'bar' could not be resolved."]); + } - assert_eq!(diagnostics.len(), 1); - assert_eq!( - diagnostics[0].message(), - "Import 'bar' could not be resolved." + #[test] + fn unresolved_import_from_resolved_module() { + let mut db = setup_db(); + + db.write_files([("/src/a.py", ""), ("/src/b.py", "from a import thing")]) + .unwrap(); + + let b_file = system_path_to_file(&db, "/src/b.py").unwrap(); + let b_file_diagnostics = super::check_types(&db, b_file); + assert_diagnostic_messages( + &b_file_diagnostics, + &["Could not resolve import of 'thing' from 'a'"], ); + } - Ok(()) + #[ignore = "\ +A spurious second 'Unresolved import' diagnostic message is emitted on `b.py`, \ +despite the symbol existing in the symbol table for `a.py`"] + #[test] + fn resolved_import_of_symbol_from_unresolved_import() { + let mut db = setup_db(); + + db.write_files([ + ("/src/a.py", "import foo as foo"), + ("/src/b.py", "from a import foo"), + ]) + .unwrap(); + + let a_file = system_path_to_file(&db, "/src/a.py").unwrap(); + let a_file_diagnostics = super::check_types(&db, a_file); + assert_diagnostic_messages( + &a_file_diagnostics, + &["Import 'foo' could not be resolved."], + ); + + // Importing the unresolved import into a second first-party file should not trigger + // an additional "unresolved import" violation + let b_file = system_path_to_file(&db, "/src/b.py").unwrap(); + let b_file_diagnostics = super::check_types(&db, b_file); + assert_eq!(&*b_file_diagnostics, &[]); } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 93c15bc015ab0..138bc73372176 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -866,7 +866,26 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - let module_ty = self.module_ty_from_name(ModuleName::new(name), alias.into()); + let module_ty = ModuleName::new(name) + .ok_or(ModuleResolutionError::InvalidSyntax) + .and_then(|module_name| self.module_ty_from_name(module_name)); + + let module_ty = match module_ty { + Ok(ty) => ty, + Err(ModuleResolutionError::InvalidSyntax) => { + tracing::debug!("Failed to resolve import due to invalid syntax"); + Type::Unknown + } + Err(ModuleResolutionError::UnresolvedModule) => { + self.add_diagnostic( + AnyNodeRef::Alias(alias), + "unresolved-import", + format_args!("Import '{name}' could not be resolved."), + ); + Type::Unknown + } + }; + self.types.definitions.insert(definition, module_ty); } @@ -914,14 +933,18 @@ impl<'db> TypeInferenceBuilder<'db> { /// - `tail` is the relative module name stripped of all leading dots: /// - `from .foo import bar` => `tail == "foo"` /// - `from ..foo.bar import baz` => `tail == "foo.bar"` - fn relative_module_name(&self, tail: Option<&str>, level: NonZeroU32) -> Option { + fn relative_module_name( + &self, + tail: Option<&str>, + level: NonZeroU32, + ) -> Result { let Some(module) = file_to_module(self.db, self.file) else { tracing::debug!( "Relative module resolution '{}' failed; could not resolve file '{}' to a module", format_import_from_module(level.get(), tail), self.file.path(self.db) ); - return None; + return Err(ModuleResolutionError::UnresolvedModule); }; let mut level = level.get(); if module.kind().is_package() { @@ -929,17 +952,19 @@ impl<'db> TypeInferenceBuilder<'db> { } let mut module_name = module.name().to_owned(); for _ in 0..level { - module_name = module_name.parent()?; + module_name = module_name + .parent() + .ok_or(ModuleResolutionError::UnresolvedModule)?; } if let Some(tail) = tail { if let Some(valid_tail) = ModuleName::new(tail) { module_name.extend(&valid_tail); } else { tracing::debug!("Relative module resolution failed: invalid syntax"); - return None; + return Err(ModuleResolutionError::InvalidSyntax); } } - Some(module_name) + Ok(module_name) } fn infer_import_from_definition( @@ -974,12 +999,12 @@ impl<'db> TypeInferenceBuilder<'db> { alias.name, format_import_from_module(*level, module), ); - let module_name = - module.expect("Non-relative import should always have a non-None `module`!"); - ModuleName::new(module_name) + module + .and_then(ModuleName::new) + .ok_or(ModuleResolutionError::InvalidSyntax) }; - let module_ty = self.module_ty_from_name(module_name, import_from.into()); + let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name)); let ast::Alias { range: _, @@ -992,11 +1017,34 @@ impl<'db> TypeInferenceBuilder<'db> { // the runtime error will occur immediately (rather than when the symbol is *used*, // as would be the case for a symbol with type `Unbound`), so it's appropriate to // think of the type of the imported symbol as `Unknown` rather than `Unbound` - let ty = module_ty + let member_ty = module_ty + .unwrap_or(Type::Unbound) .member(self.db, &Name::new(&name.id)) .replace_unbound_with(self.db, Type::Unknown); - self.types.definitions.insert(definition, ty); + if matches!(module_ty, Err(ModuleResolutionError::UnresolvedModule)) { + self.add_diagnostic( + AnyNodeRef::StmtImportFrom(import_from), + "unresolved-import", + format_args!( + "Import '{}{}' could not be resolved.", + ".".repeat(*level as usize), + module.unwrap_or_default() + ), + ); + } else if module_ty.is_ok() && member_ty.is_unknown() { + self.add_diagnostic( + AnyNodeRef::Alias(alias), + "unresolved-import", + format_args!( + "Could not resolve import of '{name}' from '{}{}'", + ".".repeat(*level as usize), + module.unwrap_or_default() + ), + ); + } + + self.types.definitions.insert(definition, member_ty); } fn infer_return_statement(&mut self, ret: &ast::StmtReturn) { @@ -1011,25 +1059,12 @@ impl<'db> TypeInferenceBuilder<'db> { } 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 - } + &self, + module_name: ModuleName, + ) -> Result, ModuleResolutionError> { + resolve_module(self.db, module_name) + .map(|module| Type::Module(module.file())) + .ok_or(ModuleResolutionError::UnresolvedModule) } fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> { @@ -1795,6 +1830,12 @@ fn format_import_from_module(level: u32, module: Option<&str>) -> String { ) } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum ModuleResolutionError { + InvalidSyntax, + UnresolvedModule, +} + #[cfg(test)] mod tests { use anyhow::Context; @@ -2048,6 +2089,16 @@ mod tests { Ok(()) } + #[test] + fn from_import_with_no_module_name() -> anyhow::Result<()> { + // This test checks that invalid syntax in a `StmtImportFrom` node + // leads to the type being inferred as `Unknown` + let mut db = setup_db(); + db.write_file("src/foo.py", "from import bar")?; + assert_public_ty(&db, "src/foo.py", "bar", "Unknown"); + Ok(()) + } + #[test] fn resolve_base_class_by_name() -> anyhow::Result<()> { let mut db = setup_db(); diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 2aac42364eec4..d920793337b07 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -18,7 +18,19 @@ struct Case { } const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/8e8a4baf652f6e1cee7acde9d78c4b6154539748/Lib/tomllib"; + +// This first "unresolved import" is because we don't understand `*` imports yet. +// The following "unresolved import" violations are because we can't distinguish currently from +// "Symbol exists in the module but its type is unknown" and +// "Symbol does not exist in the module" static EXPECTED_DIAGNOSTICS: &[&str] = &[ + "/src/tomllib/_parser.py:7:29: Could not resolve import of 'Iterable' from 'collections.abc'", + "/src/tomllib/_parser.py:10:20: Could not resolve import of 'Any' from 'typing'", + "/src/tomllib/_parser.py:13:5: Could not resolve import of 'RE_DATETIME' from '._re'", + "/src/tomllib/_parser.py:14:5: Could not resolve import of 'RE_LOCALTIME' from '._re'", + "/src/tomllib/_parser.py:15:5: Could not resolve import of 'RE_NUMBER' from '._re'", + "/src/tomllib/_parser.py:20:21: Could not resolve import of 'Key' from '._types'", + "/src/tomllib/_parser.py:20:26: Could not resolve import of 'ParseFloat' from '._types'", "Line 69 is too long (89 characters)", "Use double quotes for strings", "Use double quotes for strings",