[red-knot] Improve the unresolved-import check (#13007)

Co-authored-by: Micha Reiser <micha@reiser.io>

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(<Bar>)`,
+    /// `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, &[]);
     }
 }

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,32 +933,38 @@ 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<ModuleName> {
+    fn relative_module_name(
+        &self,
+        tail: Option<&str>,
+        level: NonZeroU32,
+    ) -> Result<ModuleName, ModuleResolutionError> {
         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() {
             level -= 1;
         }
         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<ModuleName>,
-        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<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> {
@@ -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();

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",