From 2c1fb1d2c1c3561f1706cead9d4b52b5b93c093d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 21 Aug 2024 14:09:36 +0100 Subject: [PATCH] Address review --- crates/red_knot_python_semantic/src/types.rs | 69 +++++++++++++------ .../src/types/infer.rs | 33 +++++---- 2 files changed, 69 insertions(+), 33 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index d41ed975e8a2b..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 { @@ -403,7 +416,7 @@ mod tests { } #[test] - fn check_types() -> anyhow::Result<()> { + fn unresolved_import_statement() -> anyhow::Result<()> { let mut db = setup_db(); db.write_file("src/foo.py", "import bar\n") @@ -418,27 +431,14 @@ mod tests { } #[test] - fn unresolved_import() { + fn unresolved_import_from_statement() { 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").expect("Expected `a.py` to exist!"); - let a_file_diagnostics = super::check_types(&db, a_file); - assert_diagnostic_messages( - &a_file_diagnostics, - &["Import 'foo' could not be resolved."], - ); - - // TODO: 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").expect("Expected `by.py` to exist!"); - // let b_file_diagnostics = super::check_types(&db, b_file); - // assert_eq!(&*b_file_diagnostics, &[]); + 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."]); } #[test] @@ -448,11 +448,38 @@ mod tests { 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").expect("Expected `b.py` to exist"); + 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'"], ); } + + #[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 9499cd7549923..138bc73372176 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -866,20 +866,26 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - let module_ty = if let Some(module_name) = ModuleName::new(name) { - let ty = self.module_ty_from_name(module_name.clone()); - if ty.is_unknown() { + 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 '{module_name}' could not be resolved."), + format_args!("Import '{name}' could not be resolved."), ); + Type::Unknown } - ty - } else { - tracing::debug!("Failed to resolve import due to invalid syntax"); - Type::Unknown }; + self.types.definitions.insert(definition, module_ty); } @@ -998,7 +1004,7 @@ impl<'db> TypeInferenceBuilder<'db> { .ok_or(ModuleResolutionError::InvalidSyntax) }; - let module_ty = module_name.map(|module_name| self.module_ty_from_name(module_name)); + let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name)); let ast::Alias { range: _, @@ -1021,7 +1027,7 @@ impl<'db> TypeInferenceBuilder<'db> { AnyNodeRef::StmtImportFrom(import_from), "unresolved-import", format_args!( - "Unresolved import {}{}", + "Import '{}{}' could not be resolved.", ".".repeat(*level as usize), module.unwrap_or_default() ), @@ -1052,10 +1058,13 @@ impl<'db> TypeInferenceBuilder<'db> { } } - fn module_ty_from_name(&self, module_name: ModuleName) -> Type<'db> { + fn module_ty_from_name( + &self, + module_name: ModuleName, + ) -> Result, ModuleResolutionError> { resolve_module(self.db, module_name) .map(|module| Type::Module(module.file())) - .unwrap_or(Type::Unknown) + .ok_or(ModuleResolutionError::UnresolvedModule) } fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> {