diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index b649a6bfb238b..0dae3d286ef0f 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -444,7 +444,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(()) } @@ -457,7 +457,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] @@ -469,15 +469,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(); @@ -490,10 +484,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 69ae1e7cc4dec..cd317bc8d2eef 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -934,31 +934,23 @@ impl<'db> TypeInferenceBuilder<'db> { } } - fn infer_import_definition(&mut self, alias: &ast::Alias, definition: Definition<'db>) { + fn infer_import_definition(&mut self, alias: &'db ast::Alias, definition: Definition<'db>) { let ast::Alias { range: _, name, 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) => { - self.add_diagnostic( - AnyNodeRef::Alias(alias), - "unresolved-import", - format_args!("Import '{name}' could not be resolved."), - ); + 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.unresolved_module_diagnostic(alias, 0, Some(name)); Type::Unknown } + } else { + tracing::debug!("Failed to resolve import due to invalid syntax"); + Type::Unknown }; self.types.definitions.insert(definition, module_ty); @@ -998,6 +990,23 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_optional_expression(cause.as_deref()); } + fn unresolved_module_diagnostic( + &mut self, + import_node: impl Into>, + level: u32, + module: Option<&str>, + ) { + self.add_diagnostic( + import_node.into(), + "unresolved-import", + format_args!( + "Cannot resolve import '{}{}'.", + ".".repeat(level as usize), + 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. @@ -1012,15 +1021,9 @@ impl<'db> TypeInferenceBuilder<'db> { &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 Err(ModuleResolutionError::UnresolvedModule); - }; + ) -> Result { + let module = file_to_module(self.db, self.file) + .ok_or(ModuleNameResolutionError::UnknownCurrentModule)?; let mut level = level.get(); if module.kind().is_package() { level -= 1; @@ -1029,22 +1032,18 @@ impl<'db> TypeInferenceBuilder<'db> { for _ in 0..level { module_name = module_name .parent() - .ok_or(ModuleResolutionError::UnresolvedModule)?; + .ok_or(ModuleNameResolutionError::TooManyDots)?; } 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); - } + let tail = ModuleName::new(tail).ok_or(ModuleNameResolutionError::InvalidSyntax)?; + module_name.extend(&tail); } Ok(module_name) } fn infer_import_from_definition( &mut self, - import_from: &ast::StmtImportFrom, + import_from: &'db ast::StmtImportFrom, alias: &ast::Alias, definition: Definition<'db>, ) { @@ -1060,6 +1059,7 @@ impl<'db> TypeInferenceBuilder<'db> { let ast::StmtImportFrom { module, level, .. } = import_from; tracing::trace!("Resolving imported object {alias:?} from statement {import_from:?}"); let module = module.as_deref(); + let module_name = if let Some(level) = NonZeroU32::new(*level) { tracing::trace!( "Resolving imported object '{}' from module '{}' relative to file '{}'", @@ -1076,10 +1076,41 @@ impl<'db> TypeInferenceBuilder<'db> { ); module .and_then(ModuleName::new) - .ok_or(ModuleResolutionError::InvalidSyntax) + .ok_or(ModuleNameResolutionError::InvalidSyntax) }; - let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name)); + let module_ty = match module_name { + Ok(name) => { + if let Some(ty) = self.module_ty_from_name(name) { + ty + } else { + self.unresolved_module_diagnostic(import_from, *level, module); + Type::Unknown + } + } + Err(ModuleNameResolutionError::InvalidSyntax) => { + tracing::debug!("Failed to resolve import due to invalid syntax"); + // Invalid syntax diagnostics are emitted elsewhere. + Type::Unknown + } + Err(ModuleNameResolutionError::TooManyDots) => { + tracing::debug!( + "Relative module resolution '{}' failed: too many leading dots", + format_import_from_module(*level, module), + ); + self.unresolved_module_diagnostic(import_from, *level, module); + Type::Unknown + } + Err(ModuleNameResolutionError::UnknownCurrentModule) => { + tracing::debug!( + "Relative module resolution '{}' failed; could not resolve file '{}' to a module", + format_import_from_module(*level, module), + self.file.path(self.db) + ); + self.unresolved_module_diagnostic(import_from, *level, module); + Type::Unknown + } + }; let ast::Alias { range: _, @@ -1087,39 +1118,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) { @@ -1133,13 +1155,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> { @@ -1915,10 +1932,21 @@ fn format_import_from_module(level: u32, module: Option<&str>) -> String { ) } +/// Various ways in which resolving a [`ModuleName`] +/// from an [`ast::StmtImport`] or [`ast::StmtImportFrom`] node might fail #[derive(Debug, Copy, Clone, PartialEq, Eq)] -enum ModuleResolutionError { +enum ModuleNameResolutionError { + /// The import statement has invalid syntax InvalidSyntax, - UnresolvedModule, + + /// We couldn't resolve the file we're currently analyzing back to a module + /// (Only necessary for relative import statements) + UnknownCurrentModule, + + /// The relative import statement seems to take us outside of the module search path + /// (e.g. our current module is `foo.bar`, and the relative import statement in `foo.bar` + /// is `from ....baz import spam`) + TooManyDots, } #[cfg(test)] diff --git a/crates/red_knot_wasm/tests/api.rs b/crates/red_knot_wasm/tests/api.rs index edf76c0d1b71e..6a8dfd4eac850 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: Import 'random22' could not be resolved.",] + vec!["/test.py:1:8: Cannot resolve import 'random22'."] ); } diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index debcd6c2459d2..6996c42fadca7 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -21,18 +21,9 @@ 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" +// The "unresolved import" is because we don't understand `*` imports yet. 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",