[red-knot] Silence unresolved-import in unreachable code (#17336)
Some checks are pending
CI / cargo build (msrv) (push) Blocked by required conditions
CI / cargo fuzz build (push) Blocked by required conditions
CI / Determine changes (push) Waiting to run
CI / cargo fmt (push) Waiting to run
CI / cargo clippy (push) Blocked by required conditions
CI / cargo test (linux) (push) Blocked by required conditions
CI / cargo test (linux, release) (push) Blocked by required conditions
CI / cargo test (windows) (push) Blocked by required conditions
CI / cargo test (wasm) (push) Blocked by required conditions
CI / cargo build (release) (push) Waiting to run
CI / fuzz parser (push) Blocked by required conditions
CI / test scripts (push) Blocked by required conditions
CI / ecosystem (push) Blocked by required conditions
CI / cargo shear (push) Blocked by required conditions
CI / python package (push) Waiting to run
CI / pre-commit (push) Waiting to run
CI / mkdocs (push) Waiting to run
CI / formatter instabilities and black similarity (push) Blocked by required conditions
CI / test ruff-lsp (push) Blocked by required conditions
CI / check playground (push) Blocked by required conditions
CI / benchmarks (push) Blocked by required conditions
[Knot Playground] Release / publish (push) Waiting to run

## Summary

Similar to what we did for `unresolved-reference` and
`unresolved-attribute`, we now also silence `unresolved-import`
diagnostics if the corresponding `import` statement is unreachable.

This addresses the (already closed) issue #17049.

## Test Plan

Adapted Markdown tests.
This commit is contained in:
David Peter 2025-04-10 21:13:28 +02:00 committed by GitHub
parent 410aa4b8fd
commit 8b2727cf67
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 131 additions and 103 deletions

View file

@ -117,8 +117,6 @@ python-version = "3.10"
import sys
if sys.version_info >= (3, 11):
# TODO: we should not emit an error here
# error: [unresolved-import]
from typing import Self
```
@ -391,22 +389,14 @@ diagnostics:
import sys
if sys.version_info >= (3, 11):
# TODO
# error: [unresolved-import]
from builtins import ExceptionGroup
# TODO
# error: [unresolved-import]
import builtins.ExceptionGroup
# See https://docs.python.org/3/whatsnew/3.11.html#new-modules
# TODO
# error: [unresolved-import]
import tomllib
# TODO
# error: [unresolved-import]
import wsgiref.types
```
@ -435,8 +425,6 @@ import sys
import typing
if sys.version_info >= (3, 11):
# TODO (silence diagnostics for imports, see above)
# error: [unresolved-import]
from typing import Self
class C:

View file

@ -10,8 +10,9 @@ use salsa::plumbing::AsId;
use salsa::Update;
use crate::module_name::ModuleName;
use crate::node_key::NodeKey;
use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey;
use crate::semantic_index::ast_ids::{AstIds, ScopedExpressionId};
use crate::semantic_index::ast_ids::AstIds;
use crate::semantic_index::attribute_assignment::AttributeAssignments;
use crate::semantic_index::builder::SemanticIndexBuilder;
use crate::semantic_index::definition::{Definition, DefinitionNodeKey, Definitions};
@ -254,7 +255,7 @@ impl<'db> SemanticIndex<'db> {
})
}
/// Returns true if a given expression is reachable from the start of the scope. For example,
/// Returns true if a given AST node is reachable from the start of the scope. For example,
/// in the following code, expression `2` is reachable, but expressions `1` and `3` are not:
/// ```py
/// def f():
@ -265,16 +266,14 @@ impl<'db> SemanticIndex<'db> {
/// return
/// x # 3
/// ```
pub(crate) fn is_expression_reachable(
pub(crate) fn is_node_reachable(
&self,
db: &'db dyn crate::Db,
scope_id: FileScopeId,
expression_id: ScopedExpressionId,
node_key: NodeKey,
) -> bool {
self.is_scope_reachable(db, scope_id)
&& self
.use_def_map(scope_id)
.is_expression_reachable(db, expression_id)
&& self.use_def_map(scope_id).is_node_reachable(db, node_key)
}
/// Returns an iterator over the descendent scopes of `scope`.

View file

@ -13,6 +13,7 @@ use ruff_python_ast::{self as ast};
use crate::ast_node_ref::AstNodeRef;
use crate::module_name::ModuleName;
use crate::module_resolver::resolve_module;
use crate::node_key::NodeKey;
use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey;
use crate::semantic_index::ast_ids::AstIdsBuilder;
use crate::semantic_index::attribute_assignment::{AttributeAssignment, AttributeAssignments};
@ -1139,7 +1140,10 @@ where
);
}
ast::Stmt::Import(node) => {
for alias in &node.names {
self.current_use_def_map_mut()
.record_node_reachability(NodeKey::from_node(node));
for (alias_index, alias) in node.names.iter().enumerate() {
// Mark the imported module, and all of its parents, as being imported in this
// file.
if let Some(module_name) = ModuleName::new(&alias.name) {
@ -1156,13 +1160,17 @@ where
self.add_definition(
symbol,
ImportDefinitionNodeRef {
alias,
node,
alias_index,
is_reexported,
},
);
}
}
ast::Stmt::ImportFrom(node) => {
self.current_use_def_map_mut()
.record_node_reachability(NodeKey::from_node(node));
let mut found_star = false;
for (alias_index, alias) in node.names.iter().enumerate() {
if &alias.name == "*" {
@ -1753,6 +1761,8 @@ where
.insert(expr.into(), self.current_scope());
let expression_id = self.current_ast_ids().record_expression(expr);
let node_key = NodeKey::from_node(expr);
match expr {
ast::Expr::Name(name_node @ ast::ExprName { id, ctx, .. }) => {
let (is_use, is_definition) = match (ctx, self.current_assignment()) {
@ -1771,7 +1781,7 @@ where
self.mark_symbol_used(symbol);
let use_id = self.current_ast_ids().record_use(expr);
self.current_use_def_map_mut()
.record_use(symbol, use_id, expression_id);
.record_use(symbol, use_id, node_key);
}
if is_definition {
@ -2035,7 +2045,7 @@ where
// Track reachability of attribute expressions to silence `unresolved-attribute`
// diagnostics in unreachable code.
self.current_use_def_map_mut()
.record_expression_reachability(expression_id);
.record_node_reachability(node_key);
walk_expr(self, expr);
}
@ -2043,7 +2053,7 @@ where
// Track reachability of string literals, as they could be a stringified annotation
// with child expressions whose reachability we are interested in.
self.current_use_def_map_mut()
.record_expression_reachability(expression_id);
.record_node_reachability(node_key);
walk_expr(self, expr);
}

View file

@ -224,7 +224,8 @@ impl<'a> From<StarImportDefinitionNodeRef<'a>> for DefinitionNodeRef<'a> {
#[derive(Copy, Clone, Debug)]
pub(crate) struct ImportDefinitionNodeRef<'a> {
pub(crate) alias: &'a ast::Alias,
pub(crate) node: &'a ast::StmtImport,
pub(crate) alias_index: usize,
pub(crate) is_reexported: bool,
}
@ -294,10 +295,12 @@ impl<'db> DefinitionNodeRef<'db> {
pub(super) unsafe fn into_owned(self, parsed: ParsedModule) -> DefinitionKind<'db> {
match self {
DefinitionNodeRef::Import(ImportDefinitionNodeRef {
alias,
node,
alias_index,
is_reexported,
}) => DefinitionKind::Import(ImportDefinitionKind {
alias: AstNodeRef::new(parsed, alias),
node: AstNodeRef::new(parsed, node),
alias_index,
is_reexported,
}),
@ -417,9 +420,10 @@ impl<'db> DefinitionNodeRef<'db> {
pub(super) fn key(self) -> DefinitionNodeKey {
match self {
Self::Import(ImportDefinitionNodeRef {
alias,
node,
alias_index,
is_reexported: _,
}) => alias.into(),
}) => (&node.names[alias_index]).into(),
Self::ImportFrom(ImportFromDefinitionNodeRef {
node,
alias_index,
@ -757,13 +761,18 @@ impl ComprehensionDefinitionKind {
#[derive(Clone, Debug)]
pub struct ImportDefinitionKind {
alias: AstNodeRef<ast::Alias>,
node: AstNodeRef<ast::StmtImport>,
alias_index: usize,
is_reexported: bool,
}
impl ImportDefinitionKind {
pub(crate) fn import(&self) -> &ast::StmtImport {
self.node.node()
}
pub(crate) fn alias(&self) -> &ast::Alias {
self.alias.node()
&self.node.node().names[self.alias_index]
}
pub(crate) fn is_reexported(&self) -> bool {

View file

@ -263,7 +263,8 @@ use self::symbol_state::{
LiveBindingsIterator, LiveDeclaration, LiveDeclarationsIterator, ScopedDefinitionId,
SymbolBindings, SymbolDeclarations, SymbolState,
};
use crate::semantic_index::ast_ids::{ScopedExpressionId, ScopedUseId};
use crate::node_key::NodeKey;
use crate::semantic_index::ast_ids::ScopedUseId;
use crate::semantic_index::definition::Definition;
use crate::semantic_index::narrowing_constraints::{
NarrowingConstraints, NarrowingConstraintsBuilder, NarrowingConstraintsIterator,
@ -297,8 +298,8 @@ pub(crate) struct UseDefMap<'db> {
/// [`SymbolBindings`] reaching a [`ScopedUseId`].
bindings_by_use: IndexVec<ScopedUseId, SymbolBindings>,
/// Tracks whether or not a given expression is reachable from the start of the scope.
expression_reachability: FxHashMap<ScopedExpressionId, ScopedVisibilityConstraintId>,
/// Tracks whether or not a given AST node is reachable from the start of the scope.
node_reachability: FxHashMap<NodeKey, ScopedVisibilityConstraintId>,
/// If the definition is a binding (only) -- `x = 1` for example -- then we need
/// [`SymbolDeclarations`] to know whether this binding is permitted by the live declarations.
@ -361,23 +362,19 @@ impl<'db> UseDefMap<'db> {
/// Check whether or not a given expression is reachable from the start of the scope. This
/// is a local analysis which does not capture the possibility that the entire scope might
/// be unreachable. Use [`super::SemanticIndex::is_expression_reachable`] for the global
/// be unreachable. Use [`super::SemanticIndex::is_node_reachable`] for the global
/// analysis.
#[track_caller]
pub(super) fn is_expression_reachable(
&self,
db: &dyn crate::Db,
expression_id: ScopedExpressionId,
) -> bool {
pub(super) fn is_node_reachable(&self, db: &dyn crate::Db, node_key: NodeKey) -> bool {
!self
.visibility_constraints
.evaluate(
db,
&self.predicates,
*self
.expression_reachability
.get(&expression_id)
.expect("`is_expression_reachable` should only be called on expressions with recorded reachability"),
.node_reachability
.get(&node_key)
.expect("`is_node_reachable` should only be called on AST nodes with recorded reachability"),
)
.is_always_false()
}
@ -636,8 +633,8 @@ pub(super) struct UseDefMapBuilder<'db> {
/// The use of `x` is recorded with a reachability constraint of `[test]`.
pub(super) reachability: ScopedVisibilityConstraintId,
/// Tracks whether or not a given expression is reachable from the start of the scope.
expression_reachability: FxHashMap<ScopedExpressionId, ScopedVisibilityConstraintId>,
/// Tracks whether or not a given AST node is reachable from the start of the scope.
node_reachability: FxHashMap<NodeKey, ScopedVisibilityConstraintId>,
/// Live declarations for each so-far-recorded binding.
declarations_by_binding: FxHashMap<Definition<'db>, SymbolDeclarations>,
@ -663,7 +660,7 @@ impl Default for UseDefMapBuilder<'_> {
scope_start_visibility: ScopedVisibilityConstraintId::ALWAYS_TRUE,
bindings_by_use: IndexVec::new(),
reachability: ScopedVisibilityConstraintId::ALWAYS_TRUE,
expression_reachability: FxHashMap::default(),
node_reachability: FxHashMap::default(),
declarations_by_binding: FxHashMap::default(),
bindings_by_declaration: FxHashMap::default(),
symbol_states: IndexVec::new(),
@ -822,7 +819,7 @@ impl<'db> UseDefMapBuilder<'db> {
&mut self,
symbol: ScopedSymbolId,
use_id: ScopedUseId,
expression_id: ScopedExpressionId,
node_key: NodeKey,
) {
// We have a use of a symbol; clone the current bindings for that symbol, and record them
// as the live bindings for this use.
@ -833,12 +830,11 @@ impl<'db> UseDefMapBuilder<'db> {
// Track reachability of all uses of symbols to silence `unresolved-reference`
// diagnostics in unreachable code.
self.record_expression_reachability(expression_id);
self.record_node_reachability(node_key);
}
pub(super) fn record_expression_reachability(&mut self, expression_id: ScopedExpressionId) {
self.expression_reachability
.insert(expression_id, self.reachability);
pub(super) fn record_node_reachability(&mut self, node_key: NodeKey) {
self.node_reachability.insert(node_key, self.reachability);
}
pub(super) fn snapshot_eager_bindings(
@ -935,7 +931,7 @@ impl<'db> UseDefMapBuilder<'db> {
self.all_definitions.shrink_to_fit();
self.symbol_states.shrink_to_fit();
self.bindings_by_use.shrink_to_fit();
self.expression_reachability.shrink_to_fit();
self.node_reachability.shrink_to_fit();
self.declarations_by_binding.shrink_to_fit();
self.bindings_by_declaration.shrink_to_fit();
self.eager_bindings.shrink_to_fit();
@ -946,7 +942,7 @@ impl<'db> UseDefMapBuilder<'db> {
narrowing_constraints: self.narrowing_constraints.build(),
visibility_constraints: self.visibility_constraints.build(),
bindings_by_use: self.bindings_by_use,
expression_reachability: self.expression_reachability,
node_reachability: self.node_reachability,
public_symbols: self.symbol_states,
declarations_by_binding: self.declarations_by_binding,
bindings_by_declaration: self.bindings_by_declaration,

View file

@ -993,23 +993,6 @@ pub(super) fn report_non_subscriptable(
);
}
pub(super) fn report_unresolved_module<'db>(
context: &InferContext,
import_node: impl Into<AnyNodeRef<'db>>,
level: u32,
module: Option<&str>,
) {
context.report_lint_old(
&UNRESOLVED_IMPORT,
import_node.into(),
format_args!(
"Cannot resolve import `{}{}`",
".".repeat(level as usize),
module.unwrap_or_default()
),
);
}
pub(super) fn report_slice_step_size_zero(context: &InferContext, node: AnyNodeRef) {
context.report_lint_old(
&ZERO_STEPSIZE_IN_SLICE,

View file

@ -46,6 +46,7 @@ use salsa::plumbing::AsId;
use crate::module_name::{ModuleName, ModuleNameResolutionError};
use crate::module_resolver::resolve_module;
use crate::node_key::NodeKey;
use crate::semantic_index::ast_ids::{HasScopedExpressionId, HasScopedUseId, ScopedExpressionId};
use crate::semantic_index::definition::{
AssignmentDefinitionKind, Definition, DefinitionKind, DefinitionNodeKey,
@ -67,13 +68,13 @@ use crate::types::diagnostic::{
report_implicit_return_type, report_invalid_arguments_to_annotated,
report_invalid_arguments_to_callable, report_invalid_assignment,
report_invalid_attribute_assignment, report_invalid_return_type,
report_possibly_unbound_attribute, report_unresolved_module, TypeCheckDiagnostics,
CALL_NON_CALLABLE, CALL_POSSIBLY_UNBOUND_METHOD, CONFLICTING_DECLARATIONS,
CONFLICTING_METACLASS, CYCLIC_CLASS_DEFINITION, DIVISION_BY_ZERO, DUPLICATE_BASE,
INCONSISTENT_MRO, INVALID_ASSIGNMENT, INVALID_ATTRIBUTE_ACCESS, INVALID_BASE,
INVALID_DECLARATION, INVALID_PARAMETER_DEFAULT, INVALID_TYPE_FORM,
INVALID_TYPE_VARIABLE_CONSTRAINTS, POSSIBLY_UNBOUND_IMPORT, UNDEFINED_REVEAL,
UNRESOLVED_ATTRIBUTE, UNRESOLVED_IMPORT, UNSUPPORTED_OPERATOR,
report_possibly_unbound_attribute, TypeCheckDiagnostics, CALL_NON_CALLABLE,
CALL_POSSIBLY_UNBOUND_METHOD, CONFLICTING_DECLARATIONS, CONFLICTING_METACLASS,
CYCLIC_CLASS_DEFINITION, DIVISION_BY_ZERO, DUPLICATE_BASE, INCONSISTENT_MRO,
INVALID_ASSIGNMENT, INVALID_ATTRIBUTE_ACCESS, INVALID_BASE, INVALID_DECLARATION,
INVALID_PARAMETER_DEFAULT, INVALID_TYPE_FORM, INVALID_TYPE_VARIABLE_CONSTRAINTS,
POSSIBLY_UNBOUND_IMPORT, UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE, UNRESOLVED_IMPORT,
UNSUPPORTED_OPERATOR,
};
use crate::types::generics::GenericContext;
use crate::types::mro::MroErrorKind;
@ -590,14 +591,12 @@ impl<'db> TypeInferenceBuilder<'db> {
matches!(self.region, InferenceRegion::Deferred(_)) || self.deferred_state.is_deferred()
}
/// Return the ID of the given expression, or the ID of the outermost enclosing string literal,
/// if the expression originates from a stringified annotation.
fn enclosing_expression_id(&self, expr: &impl HasScopedExpressionId) -> ScopedExpressionId {
/// Return the node key of the given AST node, or the key of the outermost enclosing string
/// literal, if the node originates from inside a stringified annotation.
fn enclosing_node_key(&self, node: AnyNodeRef<'_>) -> NodeKey {
match self.deferred_state {
DeferredExpressionState::InStringAnnotation(enclosing_string_expression) => {
enclosing_string_expression
}
_ => expr.scoped_expression_id(self.db(), self.scope()),
DeferredExpressionState::InStringAnnotation(enclosing_node_key) => enclosing_node_key,
_ => NodeKey::from_node(node),
}
}
@ -881,7 +880,7 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_type_alias_definition(type_alias.node(), definition);
}
DefinitionKind::Import(import) => {
self.infer_import_definition(import.alias(), definition);
self.infer_import_definition(import.import(), import.alias(), definition);
}
DefinitionKind::ImportFrom(import_from) => {
self.infer_import_from_definition(
@ -3119,7 +3118,39 @@ impl<'db> TypeInferenceBuilder<'db> {
}
}
fn infer_import_definition(&mut self, alias: &'db ast::Alias, definition: Definition<'db>) {
fn report_unresolved_import(
&self,
import_node: NodeKey,
range: TextRange,
level: u32,
module: Option<&str>,
) {
let file_scope_id = self.scope().file_scope_id(self.db());
let is_import_reachable =
self.index
.is_node_reachable(self.db(), file_scope_id, import_node);
if !is_import_reachable {
return;
}
self.context.report_lint_old(
&UNRESOLVED_IMPORT,
range,
format_args!(
"Cannot resolve import `{}{}`",
".".repeat(level as usize),
module.unwrap_or_default()
),
);
}
fn infer_import_definition(
&mut self,
node: &ast::StmtImport,
alias: &'db ast::Alias,
definition: Definition<'db>,
) {
let ast::Alias {
range: _,
name,
@ -3135,7 +3166,7 @@ impl<'db> TypeInferenceBuilder<'db> {
// Resolve the module being imported.
let Some(full_module_ty) = self.module_type_from_name(&full_module_name) else {
report_unresolved_module(&self.context, alias, 0, Some(name));
self.report_unresolved_import(NodeKey::from_node(node), alias.range(), 0, Some(name));
self.add_unknown_declaration_with_binding(alias.into(), definition);
return;
};
@ -3248,6 +3279,9 @@ impl<'db> TypeInferenceBuilder<'db> {
alias: &ast::Alias,
) -> Option<(ModuleName, Type<'db>)> {
let ast::StmtImportFrom { module, level, .. } = import_from;
let node_key = NodeKey::from_node(import_from);
// For diagnostics, we want to highlight the unresolvable
// module and not the entire `from ... import ...` statement.
let module_ref = module
@ -3276,7 +3310,7 @@ impl<'db> TypeInferenceBuilder<'db> {
"Relative module resolution `{}` failed: too many leading dots",
format_import_from_module(*level, module),
);
report_unresolved_module(&self.context, module_ref, *level, module);
self.report_unresolved_import(node_key, module_ref.range(), *level, module);
return None;
}
Err(ModuleNameResolutionError::UnknownCurrentModule) => {
@ -3285,13 +3319,13 @@ impl<'db> TypeInferenceBuilder<'db> {
format_import_from_module(*level, module),
self.file().path(self.db())
);
report_unresolved_module(&self.context, module_ref, *level, module);
self.report_unresolved_import(node_key, module_ref.range(), *level, module);
return None;
}
};
let Some(module_ty) = self.module_type_from_name(&module_name) else {
report_unresolved_module(&self.context, module_ref, *level, module);
self.report_unresolved_import(node_key, module_ref.range(), *level, module);
return None;
};
@ -3376,11 +3410,20 @@ impl<'db> TypeInferenceBuilder<'db> {
}
if &alias.name != "*" {
self.context.report_lint_old(
&UNRESOLVED_IMPORT,
AnyNodeRef::Alias(alias),
format_args!("Module `{module_name}` has no member `{name}`",),
let file_scope_id = self.scope().file_scope_id(self.db());
let is_import_reachable = self.index.is_node_reachable(
self.db(),
file_scope_id,
NodeKey::from_node(import_from),
);
if is_import_reachable {
self.context.report_lint_old(
&UNRESOLVED_IMPORT,
AnyNodeRef::Alias(alias),
format_args!("Module `{module_name}` has no member `{name}`",),
);
}
}
self.add_unknown_declaration_with_binding(alias.into(), definition);
@ -4461,10 +4504,10 @@ impl<'db> TypeInferenceBuilder<'db> {
});
let report_unresolved_usage = || {
self.index.is_expression_reachable(
self.index.is_node_reachable(
db,
file_scope_id,
self.enclosing_expression_id(name_node),
self.enclosing_node_key(name_node.into()),
)
};
@ -4512,10 +4555,10 @@ impl<'db> TypeInferenceBuilder<'db> {
LookupError::Unbound(_) => {
let report_unresolved_attribute = self
.index
.is_expression_reachable(
.is_node_reachable(
db,
self.scope().file_scope_id(db),
self.enclosing_expression_id(attribute),
self.enclosing_node_key(attribute.into()),
);
if report_unresolved_attribute {
@ -6395,7 +6438,7 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_annotation_expression(
parsed.expr(),
DeferredExpressionState::InStringAnnotation(
self.enclosing_expression_id(string),
self.enclosing_node_key(string.into()),
),
)
}
@ -6761,7 +6804,7 @@ impl<'db> TypeInferenceBuilder<'db> {
self.infer_type_expression_with_state(
parsed.expr(),
DeferredExpressionState::InStringAnnotation(
self.enclosing_expression_id(string),
self.enclosing_node_key(string.into()),
),
)
}
@ -7480,9 +7523,9 @@ enum DeferredExpressionState {
/// The annotation of `a` is completely inside a string while for `b`, it's only partially
/// stringified.
///
/// This variant wraps a [`ScopedExpressionId`] that allows us to retrieve
/// the original [`ast::ExprStringLiteral`] node which created the string annotation
InStringAnnotation(ScopedExpressionId),
/// This variant wraps a [`NodeKey`] that allows us to retrieve the original
/// [`ast::ExprStringLiteral`] node which created the string annotation.
InStringAnnotation(NodeKey),
}
impl DeferredExpressionState {