-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Improve the unresolved-import
check
#13007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I think describing it as a failed attribute access would actually be much clearer than the vague "Could not resolve" which doesn't really communicate anything! This message also doesn't currently clarify that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do people really think of
|
||
); | ||
} | ||
|
||
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`"] | ||
Comment on lines
+460
to
+461
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think when we issue the error on the import in |
||
#[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, &[]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just emit the diagnostics inside this function, return an |
||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still find it odd that we have to do the "unbound" check everywhere where we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do, but it will need to be different error codes. It would be very counterintuitive for users if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. We might just want to parameterize the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure exactly what you mean; could you clarify?
Mypy has a very different architecture to the one we're building. It resolves all imports eagerly in a first pass before it does any other type checking; I'm not as familiar with pyright's codebase but I can dig in now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... it seems both mypy and pyright in fact do what you'd prefer here -- they both use "attribute-access" error codes for
Pyright's error code is
whereas mypy has
I'm somewhat surprised by this. But given this precedent, I'm okay with emitting the diagnostic from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I agree that that makes a lot of sense! I can make that change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... if the inferred type of a member access is a union, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This might be doable, but after looking at it for a little bit I think it would still be quite a large refactor and design change. I think it's out of the scope of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced there is any problem with reporting an error on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should return an We could design our own representation for "Type plus definitely-bound/maybe-unbound/definitely-unbound state" and use that instead of I was thinking that we would push diagnostics inside |
||
.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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should be emitting a diagnostic at all if the imported symbol is of type I think just removing this clause would fix the ignored test; what would it break? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the only reason we have to check for Unknown here is that we intentionally replaced Unbound with Unknown just a few lines above. If we wait to do it after, the bug in the ignored test goes away, and all tests pass:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I think you're right that this fixes the tests I added (and skipped) in this PR. Here's an interesting edge case that you might not have considered, though. What should we do for something like this? # foo.py
for x in range(0):
pass
# bar.py
from foo import x Running So then what if we have something like this, where the symbol definitely exists in # foo.py
x: int
# bar.py
from foo import x These are somewhat obscure edge cases, so I think what you propose here is a strict improvement on what I implemented in this PR; I'll make a fixup PR to address your suggestions. But my point is that we may still at some point need some context propagated regarding the cause of the error -- whether that error is represented as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, great points and great questions! I had the I agree that maybe-unbound is a different error class than definitely-unbound (and arguable whether maybe-unbound should trigger an error on import at all.) I think we do want a representation of "definitely unbound and typed" that we use for imports, so e.g. we can error if you import from What's less clear to me is if we need a representation of "definitely unbound and typed" for inferred types. It would mean that within a scope we could also check code following Like I mentioned above in a different comment, I'm pretty open to exploring other representations of unbound to make it orthogonal to types; the main question for me is if we can avoid it regressing in perf, and if not, do we gain enough from it to be worth the regression? |
||
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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the advantage of returning a |
||
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(); | ||
|
Uh oh!
There was an error while loading. Please reload this page.