[red-knot] Improve the accuracy of the unresolved-import check (#13055)

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(
+        assert_diagnostic_messages(&b_file_diagnostics, &["Module 'a' has no member 'thing'"]);
-            &b_file_diagnostics,
-            &["Could not resolve import of 'thing' from 'a'"],
-        );
     }
 
-    #[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(
+        assert_diagnostic_messages(&a_file_diagnostics, &["Cannot resolve import 'foo'."]);
-            &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

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)
+        let module_ty = if let Some(module_name) = ModuleName::new(name) {
-            .ok_or(ModuleResolutionError::InvalidSyntax)
+            if let Some(module) = self.module_ty_from_name(module_name) {
-            .and_then(|module_name| self.module_ty_from_name(module_name));
+                module
-
+            } else {
-        let module_ty = match module_ty {
+                self.unresolved_module_diagnostic(alias, 0, Some(name));
-            Ok(ty) => ty,
+                Type::Unknown
-            Err(ModuleResolutionError::InvalidSyntax) => {
+            }
+        } else {
             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);
@@ -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<AnyNodeRef<'db>>,
+        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<ModuleName, ModuleResolutionError> {
+    ) -> Result<ModuleName, ModuleNameResolutionError> {
-        let Some(module) = file_to_module(self.db, self.file) else {
+        let module = file_to_module(self.db, self.file)
-            tracing::debug!(
+            .ok_or(ModuleNameResolutionError::UnknownCurrentModule)?;
-                "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);
-        };
         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) {
+            let tail = ModuleName::new(tail).ok_or(ModuleNameResolutionError::InvalidSyntax)?;
-                module_name.extend(&valid_tail);
+            module_name.extend(&tail);
-            } else {
-                tracing::debug!("Relative module resolution failed: invalid syntax");
-                return Err(ModuleResolutionError::InvalidSyntax);
-            }
         }
         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,
+        let member_ty = module_ty.member(self.db, &Name::new(&name.id));
-        // 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);
 
-        if matches!(module_ty, Err(ModuleResolutionError::UnresolvedModule)) {
+        // TODO: What if it's a union where one of the elements is `Unbound`?
-            self.add_diagnostic(
+        if member_ty.is_unbound() {
-                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 '{}{}'",
+                    "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(
+    fn module_ty_from_name(&self, module_name: ModuleName) -> Option<Type<'db>> {
-        &self,
+        resolve_module(self.db, module_name).map(|module| Type::Module(module.file()))
-        module_name: ModuleName,
-    ) -> Result<Type<'db>, 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> {
@@ -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)]

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'."]
     );
 }

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 "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:7:29: Module 'collections.abc' has no member 'Iterable'",
-    "/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",