From 2efa4230d7a2a57de07433fc51d6f264cee2fd0c Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 20 Aug 2024 13:07:07 +0100 Subject: [PATCH 1/3] Add failing tests --- crates/red_knot_python_semantic/src/types.rs | 73 ++++++++++++++++--- .../src/types/infer.rs | 10 +++ crates/ruff_benchmark/benches/red_knot.rs | 1 + 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 272d0bfb0cdcb..ac09784d0a5ea 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -369,12 +369,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 +391,68 @@ 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 check_types() -> 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(()) + } - assert_eq!(diagnostics.len(), 1); - assert_eq!( - diagnostics[0].message(), - "Import 'bar' could not be resolved." + #[test] + fn 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").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."], ); - Ok(()) + // Importing the unresolved import into a second first-party file does 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, &[]); + } + + #[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").expect("Expected `b.py` to exist"); + let b_file_diagnostics = super::check_types(&db, b_file); + assert_diagnostic_messages( + &b_file_diagnostics, + &["Could not resolve import of 'thing' from 'a'"], + ); } } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 93c15bc015ab0..9c555933c18eb 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -2048,6 +2048,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(); diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 2aac42364eec4..1e035ebf5cbef 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -19,6 +19,7 @@ struct Case { const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/8e8a4baf652f6e1cee7acde9d78c4b6154539748/Lib/tomllib"; static EXPECTED_DIAGNOSTICS: &[&str] = &[ + "/src/tomllib/_parser.py:7:29: Could not resolve import of 'Iterable' from 'collections.abc'", "Line 69 is too long (89 characters)", "Use double quotes for strings", "Use double quotes for strings", From 29ab98d9a37d5d341ae422647a2bee0908fca306 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 20 Aug 2024 13:47:05 +0100 Subject: [PATCH 2/3] Fix tests as best we can Update crates/red_knot_python_semantic/src/types/infer.rs Co-authored-by: Micha Reiser --- crates/red_knot_python_semantic/src/types.rs | 8 +- .../src/types/infer.rs | 96 ++++++++++++------- crates/ruff_benchmark/benches/red_knot.rs | 11 +++ 3 files changed, 79 insertions(+), 36 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index ac09784d0a5ea..d41ed975e8a2b 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -434,11 +434,11 @@ mod tests { &["Import 'foo' could not be resolved."], ); - // Importing the unresolved import into a second first-party file does not trigger + // 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, &[]); + // 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, &[]); } #[test] diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 9c555933c18eb..9499cd7549923 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -866,7 +866,20 @@ impl<'db> TypeInferenceBuilder<'db> { asname: _, } = alias; - let module_ty = self.module_ty_from_name(ModuleName::new(name), alias.into()); + 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() { + self.add_diagnostic( + AnyNodeRef::Alias(alias), + "unresolved-import", + format_args!("Import '{module_name}' could not be resolved."), + ); + } + ty + } else { + tracing::debug!("Failed to resolve import due to invalid syntax"); + Type::Unknown + }; self.types.definitions.insert(definition, module_ty); } @@ -914,14 +927,18 @@ 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 { + fn relative_module_name( + &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 None; + return Err(ModuleResolutionError::UnresolvedModule); }; let mut level = level.get(); if module.kind().is_package() { @@ -929,17 +946,19 @@ impl<'db> TypeInferenceBuilder<'db> { } 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 +993,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.map(|module_name| self.module_ty_from_name(module_name)); let ast::Alias { range: _, @@ -992,11 +1011,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!( + "Unresolved import {}{}", + ".".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) { @@ -1010,26 +1052,10 @@ impl<'db> TypeInferenceBuilder<'db> { } } - fn module_ty_from_name( - &mut self, - module_name: Option, - 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 - } + fn module_ty_from_name(&self, module_name: ModuleName) -> Type<'db> { + resolve_module(self.db, module_name) + .map(|module| Type::Module(module.file())) + .unwrap_or(Type::Unknown) } fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> { @@ -1795,6 +1821,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; diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 1e035ebf5cbef..d920793337b07 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -18,8 +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", From 2c1fb1d2c1c3561f1706cead9d4b52b5b93c093d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 21 Aug 2024 14:09:36 +0100 Subject: [PATCH 3/3] 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> {