Skip to content

Commit

Permalink
[red-knot] Improve the accuracy of the unresolved-import check
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWaygood committed Aug 22, 2024
1 parent d1d0678 commit 10b574c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 85 deletions.
17 changes: 4 additions & 13 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,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(())
}
Expand All @@ -433,7 +433,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]
Expand All @@ -445,15 +445,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(
&b_file_diagnostics,
&["Could not resolve import of 'thing' from 'a'"],
);
assert_diagnostic_messages(&b_file_diagnostics, &["Module 'a' has no member 'thing'"]);
}

#[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();
Expand All @@ -466,10 +460,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(
&a_file_diagnostics,
&["Import 'foo' could not be resolved."],
);
assert_diagnostic_messages(&a_file_diagnostics, &["Cannot resolve import 'foo'."]);

// Importing the unresolved import into a second first-party file should not trigger
// an additional "unresolved import" violation
Expand Down
129 changes: 67 additions & 62 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -898,24 +898,20 @@ impl<'db> TypeInferenceBuilder<'db> {
asname: _,
} = alias;

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) => {
let module_ty = if let Some(module_name) = ModuleName::new(name) {
if let Some(module) = self.module_ty_from_name(module_name) {
module
} else {
self.add_diagnostic(
AnyNodeRef::Alias(alias),
"unresolved-import",
format_args!("Import '{name}' could not be resolved."),
format_args!("Cannot resolve import '{name}'."),
);
Type::Unknown
}
} else {
tracing::debug!("Failed to resolve import due to invalid syntax");
Type::Unknown
};

self.types.definitions.insert(definition, module_ty);
Expand Down Expand Up @@ -955,6 +951,23 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_optional_expression(cause.as_deref());
}

fn unresolved_module_name(
&mut self,
import_from: &ast::StmtImportFrom,
level: usize,
module: Option<&str>,
) {
self.add_diagnostic(
AnyNodeRef::StmtImportFrom(import_from),
"unresolved-import",
format_args!(
"Cannot resolve import '{}{}'.",
".".repeat(level),
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.
Expand All @@ -966,37 +979,41 @@ impl<'db> TypeInferenceBuilder<'db> {
/// - `from .foo import bar` => `tail == "foo"`
/// - `from ..foo.bar import baz` => `tail == "foo.bar"`
fn relative_module_name(
&self,
&mut self,
tail: Option<&str>,
level: NonZeroU32,
) -> Result<ModuleName, ModuleResolutionError> {
import_from: &ast::StmtImportFrom,
) -> Option<ModuleName> {
let mut level = level.get();
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),
format_import_from_module(level, tail),
self.file.path(self.db)
);
return Err(ModuleResolutionError::UnresolvedModule);
self.unresolved_module_name(import_from, level as usize, tail);
return None;
};
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()
.ok_or(ModuleResolutionError::UnresolvedModule)?;
let Some(parent) = module_name.parent() else {
self.unresolved_module_name(import_from, level as usize, tail);
return None;
};
module_name = parent;
}
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 Err(ModuleResolutionError::InvalidSyntax);
return None;
}
}
Ok(module_name)
Some(module_name)
}

fn infer_import_from_definition(
Expand Down Expand Up @@ -1024,59 +1041,58 @@ impl<'db> TypeInferenceBuilder<'db> {
format_import_from_module(level.get(), module),
self.file.path(self.db),
);
self.relative_module_name(module, level)
self.relative_module_name(module, level, import_from)
} else {
tracing::trace!(
"Resolving imported object '{}' from module '{}'",
alias.name,
format_import_from_module(*level, module),
);
module
.and_then(ModuleName::new)
.ok_or(ModuleResolutionError::InvalidSyntax)
module.and_then(ModuleName::new)
};

let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name));
let module_ty = if let Some(module_name) = module_name {
if let Some(module_ty) = self.module_ty_from_name(module_name) {
module_ty
} else {
self.unresolved_module_name(import_from, *level as usize, module);
Type::Unknown
}
} else {
tracing::debug!("Failed to resolve import due to invalid syntax");
Type::Unknown
};

let ast::Alias {
range: _,
name,
asname: _,
} = alias;

// 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`
let member_ty = module_ty
.unwrap_or(Type::Unbound)
.member(self.db, &Name::new(&name.id))
.replace_unbound_with(self.db, Type::Unknown);
let member_ty = module_ty.member(self.db, &Name::new(&name.id));

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() {
// TODO: What if it's a union where one of the elements is `Unbound`?
if member_ty.is_unbound() {
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) {
Expand All @@ -1090,13 +1106,8 @@ impl<'db> TypeInferenceBuilder<'db> {
}
}

fn module_ty_from_name(
&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 module_ty_from_name(&self, module_name: ModuleName) -> Option<Type<'db>> {
resolve_module(self.db, module_name).map(|module| Type::Module(module.file()))
}

fn infer_decorator(&mut self, decorator: &ast::Decorator) -> Type<'db> {
Expand Down Expand Up @@ -1862,12 +1873,6 @@ 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;
Expand Down
11 changes: 1 addition & 10 deletions crates/ruff_benchmark/benches/red_knot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,8 @@ 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'",
"/src/tomllib/_parser.py:7:29: Module 'collections.abc' has no member 'Iterable'",
"Line 69 is too long (89 characters)",
"Use double quotes for strings",
"Use double quotes for strings",
Expand Down

0 comments on commit 10b574c

Please sign in to comment.