From 10b574c25741038ad8e75c94224fa325d86048ef Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 22 Aug 2024 13:23:00 +0100 Subject: [PATCH] [red-knot] Improve the accuracy of the unresolved-import check --- crates/red_knot_python_semantic/src/types.rs | 17 +-- .../src/types/infer.rs | 129 +++++++++--------- crates/ruff_benchmark/benches/red_knot.rs | 11 +- 3 files changed, 72 insertions(+), 85 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 173c957d1a28ed..8cfc977106a78f 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -420,7 +420,7 @@ mod tests { 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."]); + assert_diagnostic_messages(&diagnostics, &["Cannot resolve import 'bar'."]); Ok(()) } @@ -433,7 +433,7 @@ mod tests { .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_diagnostic_messages(&diagnostics, &["Cannot resolve import 'bar'."]); } #[test] @@ -445,15 +445,9 @@ mod tests { 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'"], - ); + assert_diagnostic_messages(&b_file_diagnostics, &["Module 'a' has no member 'thing'"]); } - #[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(); @@ -466,10 +460,7 @@ despite the symbol existing in the symbol table for `a.py`"] 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."], - ); + assert_diagnostic_messages(&a_file_diagnostics, &["Cannot resolve import 'foo'."]); // Importing the unresolved import into a second first-party file should not trigger // an additional "unresolved import" violation diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 28fb0a002ccd03..7347b3d8b39786 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -898,24 +898,20 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - 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) => { + let module_ty = if let Some(module_name) = ModuleName::new(name) { + if let Some(module) = self.module_ty_from_name(module_name) { + module + } else { self.add_diagnostic( AnyNodeRef::Alias(alias), "unresolved-import", - format_args!("Import '{name}' could not be resolved."), + format_args!("Cannot resolve import '{name}'."), ); Type::Unknown } + } else { + tracing::debug!("Failed to resolve import due to invalid syntax"); + Type::Unknown }; self.types.definitions.insert(definition, module_ty); @@ -955,6 +951,23 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_optional_expression(cause.as_deref()); } + fn unresolved_module_name( + &mut self, + import_from: &ast::StmtImportFrom, + level: usize, + module: Option<&str>, + ) { + self.add_diagnostic( + AnyNodeRef::StmtImportFrom(import_from), + "unresolved-import", + format_args!( + "Cannot resolve import '{}{}'.", + ".".repeat(level), + module.unwrap_or_default() + ), + ); + } + /// Given a `from .foo import bar` relative import, resolve the relative module /// we're importing `bar` from into an absolute [`ModuleName`] /// using the name of the module we're currently analyzing. @@ -966,37 +979,41 @@ impl<'db> TypeInferenceBuilder<'db> { /// - `from .foo import bar` => `tail == "foo"` /// - `from ..foo.bar import baz` => `tail == "foo.bar"` fn relative_module_name( - &self, + &mut self, tail: Option<&str>, level: NonZeroU32, - ) -> Result { + import_from: &ast::StmtImportFrom, + ) -> Option { + let mut level = level.get(); 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), + format_import_from_module(level, tail), self.file.path(self.db) ); - return Err(ModuleResolutionError::UnresolvedModule); + self.unresolved_module_name(import_from, level as usize, tail); + return None; }; - let mut level = level.get(); if module.kind().is_package() { level -= 1; } let mut module_name = module.name().to_owned(); for _ in 0..level { - module_name = module_name - .parent() - .ok_or(ModuleResolutionError::UnresolvedModule)?; + let Some(parent) = module_name.parent() else { + self.unresolved_module_name(import_from, level as usize, tail); + return None; + }; + module_name = parent; } 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 Err(ModuleResolutionError::InvalidSyntax); + return None; } } - Ok(module_name) + Some(module_name) } fn infer_import_from_definition( @@ -1024,19 +1041,27 @@ impl<'db> TypeInferenceBuilder<'db> { format_import_from_module(level.get(), module), self.file.path(self.db), ); - self.relative_module_name(module, level) + self.relative_module_name(module, level, import_from) } else { tracing::trace!( "Resolving imported object '{}' from module '{}'", alias.name, format_import_from_module(*level, module), ); - module - .and_then(ModuleName::new) - .ok_or(ModuleResolutionError::InvalidSyntax) + module.and_then(ModuleName::new) }; - let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name)); + let module_ty = if let Some(module_name) = module_name { + if let Some(module_ty) = self.module_ty_from_name(module_name) { + module_ty + } else { + self.unresolved_module_name(import_from, *level as usize, module); + Type::Unknown + } + } else { + tracing::debug!("Failed to resolve import due to invalid syntax"); + Type::Unknown + }; let ast::Alias { range: _, @@ -1044,39 +1069,30 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - // If a symbol is unbound in the module the symbol was originally defined in, - // when we're trying to import the symbol from that module into "our" module, - // 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 member_ty = module_ty - .unwrap_or(Type::Unbound) - .member(self.db, &Name::new(&name.id)) - .replace_unbound_with(self.db, Type::Unknown); + let member_ty = module_ty.member(self.db, &Name::new(&name.id)); - 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() { + // TODO: What if it's a union where one of the elements is `Unbound`? + if member_ty.is_unbound() { self.add_diagnostic( AnyNodeRef::Alias(alias), "unresolved-import", format_args!( - "Could not resolve import of '{name}' from '{}{}'", + "Module '{}{}' has no member '{name}'", ".".repeat(*level as usize), module.unwrap_or_default() ), ); } - self.types.definitions.insert(definition, member_ty); + // If a symbol is unbound in the module the symbol was originally defined in, + // when we're trying to import the symbol from that module into "our" module, + // 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` + self.types.definitions.insert( + definition, + member_ty.replace_unbound_with(self.db, Type::Unknown), + ); } fn infer_return_statement(&mut self, ret: &ast::StmtReturn) { @@ -1090,13 +1106,8 @@ impl<'db> TypeInferenceBuilder<'db> { } } - fn module_ty_from_name( - &self, - module_name: ModuleName, - ) -> Result, ModuleResolutionError> { - resolve_module(self.db, module_name) - .map(|module| Type::Module(module.file())) - .ok_or(ModuleResolutionError::UnresolvedModule) + fn module_ty_from_name(&self, module_name: ModuleName) -> Option> { + resolve_module(self.db, module_name).map(|module| Type::Module(module.file())) } fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> { @@ -1862,12 +1873,6 @@ 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; diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 312b2b23103136..e3924a6e0df205 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -21,17 +21,8 @@ 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'", + "/src/tomllib/_parser.py:7:29: Module 'collections.abc' has no member 'Iterable'", "Line 69 is too long (89 characters)", "Use double quotes for strings", "Use double quotes for strings",