Skip to content
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.

Sign up for GitHub

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

[red-knot] Improve the accuracy of the unresolved-import check #13055

Merged
merged 2 commits into from
Aug 27, 2024
Merged

[red-knot] Improve the accuracy of the unresolved-import check #13055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f04a3bf
[red-knot] Improve the accuracy of the unresolved-import check
AlexWaygood Aug 22, 2024
097e24d
Add back an error enum (but use it more narrowly)
AlexWaygood Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,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 @@ -457,7 +457,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 @@ -469,15 +469,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 @@ -490,10 +484,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
162 changes: 95 additions & 67 deletions crates/red_knot_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,31 +934,23 @@ impl<'db> TypeInferenceBuilder<'db> {
}
}

fn infer_import_definition(&mut self, alias: &ast::Alias, definition: Definition<'db>) {
fn infer_import_definition(&mut self, alias: &'db ast::Alias, definition: Definition<'db>) {
let ast::Alias {
range: _,
name,
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) => {
self.add_diagnostic(
AnyNodeRef::Alias(alias),
"unresolved-import",
format_args!("Import '{name}' could not be resolved."),
);
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.unresolved_module_diagnostic(alias, 0, Some(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 @@ -998,6 +990,23 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_optional_expression(cause.as_deref());
}

fn unresolved_module_diagnostic(
&mut self,
import_node: impl Into<AnyNodeRef<'db>>,
level: u32,
module: Option<&str>,
) {
self.add_diagnostic(
import_node.into(),
"unresolved-import",
format_args!(
"Cannot resolve import '{}{}'.",
".".repeat(level as usize),
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 @@ -1012,15 +1021,9 @@ impl<'db> TypeInferenceBuilder<'db> {
&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 Err(ModuleResolutionError::UnresolvedModule);
};
) -> Result<ModuleName, ModuleNameResolutionError> {
let module = file_to_module(self.db, self.file)
.ok_or(ModuleNameResolutionError::UnknownCurrentModule)?;
let mut level = level.get();
if module.kind().is_package() {
level -= 1;
Expand All @@ -1029,22 +1032,18 @@ impl<'db> TypeInferenceBuilder<'db> {
for _ in 0..level {
module_name = module_name
.parent()
.ok_or(ModuleResolutionError::UnresolvedModule)?;
.ok_or(ModuleNameResolutionError::TooManyDots)?;
}
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);
}
let tail = ModuleName::new(tail).ok_or(ModuleNameResolutionError::InvalidSyntax)?;
module_name.extend(&tail);
}
Ok(module_name)
}

fn infer_import_from_definition(
&mut self,
import_from: &ast::StmtImportFrom,
import_from: &'db ast::StmtImportFrom,
alias: &ast::Alias,
definition: Definition<'db>,
) {
Expand All @@ -1060,6 +1059,7 @@ impl<'db> TypeInferenceBuilder<'db> {
let ast::StmtImportFrom { module, level, .. } = import_from;
tracing::trace!("Resolving imported object {alias:?} from statement {import_from:?}");
let module = module.as_deref();

let module_name = if let Some(level) = NonZeroU32::new(*level) {
tracing::trace!(
"Resolving imported object '{}' from module '{}' relative to file '{}'",
Expand All @@ -1076,50 +1076,72 @@ impl<'db> TypeInferenceBuilder<'db> {
);
module
.and_then(ModuleName::new)
.ok_or(ModuleResolutionError::InvalidSyntax)
.ok_or(ModuleNameResolutionError::InvalidSyntax)
};

let module_ty = module_name.and_then(|module_name| self.module_ty_from_name(module_name));
let module_ty = match module_name {
Ok(name) => {
if let Some(ty) = self.module_ty_from_name(name) {
ty
} else {
self.unresolved_module_diagnostic(import_from, *level, module);
Type::Unknown
}
}
Err(ModuleNameResolutionError::InvalidSyntax) => {
tracing::debug!("Failed to resolve import due to invalid syntax");
// Invalid syntax diagnostics are emitted elsewhere.
Type::Unknown
}
Err(ModuleNameResolutionError::TooManyDots) => {
tracing::debug!(
"Relative module resolution '{}' failed: too many leading dots",
format_import_from_module(*level, module),
);
self.unresolved_module_diagnostic(import_from, *level, module);
Type::Unknown
}
Err(ModuleNameResolutionError::UnknownCurrentModule) => {
tracing::debug!(
"Relative module resolution '{}' failed; could not resolve file '{}' to a module",
format_import_from_module(*level, module),
self.file.path(self.db)
);
self.unresolved_module_diagnostic(import_from, *level, module);
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}'",
carljm marked this conversation as resolved.
Show resolved Hide resolved
".".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 @@ -1133,13 +1155,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 @@ -1915,10 +1932,21 @@ fn format_import_from_module(level: u32, module: Option<&str>) -> String {
)
}

/// Various ways in which resolving a [`ModuleName`]
/// from an [`ast::StmtImport`] or [`ast::StmtImportFrom`] node might fail
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum ModuleResolutionError {
enum ModuleNameResolutionError {
/// The import statement has invalid syntax
InvalidSyntax,
UnresolvedModule,

/// We couldn't resolve the file we're currently analyzing back to a module
/// (Only necessary for relative import statements)
UnknownCurrentModule,

/// The relative import statement seems to take us outside of the module search path
/// (e.g. our current module is `foo.bar`, and the relative import statement in `foo.bar`
/// is `from ....baz import spam`)
TooManyDots,
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion crates/red_knot_wasm/tests/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ fn check() {

assert_eq!(
result,
vec!["/test.py:1:8: Import 'random22' could not be resolved.",]
vec!["/test.py:1:8: Cannot resolve import 'random22'."]
);
}
13 changes: 2 additions & 11 deletions crates/ruff_benchmark/benches/red_knot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,9 @@ 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"
// The "unresolved import" is because we don't understand `*` imports yet.
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
Loading