[red-knot] Silence unresolved-attribute in unreachable code (#17305)

## Summary

Basically just repeat the same thing that we did for
`unresolved-reference`, but now for attribute expressions.

We now also handle the case where the unresolved attribute (or the
unresolved reference) diagnostic originates from a stringified type
annotation.

And I made the evaluation of reachability constraints lazy (will only be
evaluated right before we are about to emit a diagnostic).

## Test Plan

New Markdown tests for stringified annotations.

crates/red_knot_python_semantic/resources/mdtest/unreachable.md

@@ -166,8 +166,6 @@ python-platform = "linux"
 import sys
 
 if sys.platform == "win32":
-    # TODO: we should not emit an error here
-    # error: [unresolved-attribute]
     sys.getwindowsversion()
 ```
 
@@ -381,8 +379,6 @@ import sys
 import builtins
 
 if sys.version_info >= (3, 11):
-    # TODO
-    # error: [unresolved-attribute]
     builtins.ExceptionGroup
 ```
 
@@ -430,6 +426,33 @@ if False:
             print(x)
 ```
 
+### Type annotations
+
+Silencing of diagnostics also works for type annotations, even if they are stringified:
+
+```py
+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:
+        def name_expr(self) -> Self:
+            return self
+
+        def name_expr_stringified(self) -> "Self":
+            return self
+
+        def attribute_expr(self) -> typing.Self:
+            return self
+
+        def attribute_expr_stringified(self) -> "typing.Self":
+            return self
+```
+
 ### Use of unreachable symbols in type annotations, or as class bases
 
 We should not show any diagnostics in type annotations inside unreachable sections.

crates/red_knot_python_semantic/src/semantic_index.rs

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

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -8,7 +8,7 @@ use ruff_db::parsed::ParsedModule;
 use ruff_index::IndexVec;
 use ruff_python_ast::name::Name;
 use ruff_python_ast::visitor::{walk_expr, walk_pattern, walk_stmt, Visitor};
-use ruff_python_ast::{self as ast, ExprContext};
+use ruff_python_ast::{self as ast};
 
 use crate::ast_node_ref::AstNodeRef;
 use crate::module_name::ModuleName;
@@ -1770,7 +1770,8 @@ where
                 if is_use {
                     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);
+                    self.current_use_def_map_mut()
+                        .record_use(symbol, use_id, expression_id);
                 }
 
                 if is_definition {
@@ -2011,24 +2012,39 @@ where
             ast::Expr::Attribute(ast::ExprAttribute {
                 value: object,
                 attr,
-                ctx: ExprContext::Store,
+                ctx,
                 range: _,
             }) => {
-                if let Some(unpack) = self
+                if ctx.is_store() {
-                    .current_assignment()
+                    if let Some(unpack) = self
-                    .as_ref()
+                        .current_assignment()
-                    .and_then(CurrentAssignment::unpack)
+                        .as_ref()
-                {
+                        .and_then(CurrentAssignment::unpack)
-                    self.register_attribute_assignment(
+                    {
-                        object,
+                        self.register_attribute_assignment(
-                        attr,
+                            object,
-                        AttributeAssignment::Unpack {
+                            attr,
-                            attribute_expression_id: expression_id,
+                            AttributeAssignment::Unpack {
-                            unpack,
+                                attribute_expression_id: expression_id,
-                        },
+                                unpack,
-                    );
+                            },
+                        );
+                    }
                 }
 
+                // Track reachability of attribute expressions to silence `unresolved-attribute`
+                // diagnostics in unreachable code.
+                self.current_use_def_map_mut()
+                    .record_expression_reachability(expression_id);
+
+                walk_expr(self, expr);
+            }
+            ast::Expr::StringLiteral(_) => {
+                // 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);
+
                 walk_expr(self, expr);
             }
             _ => {

crates/red_knot_python_semantic/src/semantic_index/use_def.rs

@@ -263,7 +263,7 @@ use self::symbol_state::{
     LiveBindingsIterator, LiveDeclaration, LiveDeclarationsIterator, ScopedDefinitionId,
     SymbolBindings, SymbolDeclarations, SymbolState,
 };
-use crate::semantic_index::ast_ids::ScopedUseId;
+use crate::semantic_index::ast_ids::{ScopedExpressionId, ScopedUseId};
 use crate::semantic_index::definition::Definition;
 use crate::semantic_index::narrowing_constraints::{
     NarrowingConstraints, NarrowingConstraintsBuilder, NarrowingConstraintsIterator,
@@ -297,8 +297,8 @@ pub(crate) struct UseDefMap<'db> {
     /// [`SymbolBindings`] reaching a [`ScopedUseId`].
     bindings_by_use: IndexVec<ScopedUseId, SymbolBindings>,
 
-    /// Tracks whether or not a given use of a symbol is reachable from the start of the scope.
+    /// Tracks whether or not a given expression is reachable from the start of the scope.
-    reachability_by_use: IndexVec<ScopedUseId, ScopedVisibilityConstraintId>,
+    expression_reachability: FxHashMap<ScopedExpressionId, 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.
@@ -359,8 +359,27 @@ impl<'db> UseDefMap<'db> {
             .is_always_false()
     }
 
-    pub(super) fn is_symbol_use_reachable(&self, db: &dyn crate::Db, use_id: ScopedUseId) -> bool {
+    /// Check whether or not a given expression is reachable from the start of the scope. This
-        self.is_reachable(db, self.reachability_by_use[use_id])
+    /// 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
+    /// analysis.
+    #[track_caller]
+    pub(super) fn is_expression_reachable(
+        &self,
+        db: &dyn crate::Db,
+        expression_id: ScopedExpressionId,
+    ) -> 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"),
+            )
+            .is_always_false()
     }
 
     pub(crate) fn public_bindings(
@@ -617,8 +636,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 use of a symbol is reachable from the start of the scope.
+    /// Tracks whether or not a given expression is reachable from the start of the scope.
-    reachability_by_use: IndexVec<ScopedUseId, ScopedVisibilityConstraintId>,
+    expression_reachability: FxHashMap<ScopedExpressionId, ScopedVisibilityConstraintId>,
 
     /// Live declarations for each so-far-recorded binding.
     declarations_by_binding: FxHashMap<Definition<'db>, SymbolDeclarations>,
@@ -644,7 +663,7 @@ impl Default for UseDefMapBuilder<'_> {
             scope_start_visibility: ScopedVisibilityConstraintId::ALWAYS_TRUE,
             bindings_by_use: IndexVec::new(),
             reachability: ScopedVisibilityConstraintId::ALWAYS_TRUE,
-            reachability_by_use: IndexVec::new(),
+            expression_reachability: FxHashMap::default(),
             declarations_by_binding: FxHashMap::default(),
             bindings_by_declaration: FxHashMap::default(),
             symbol_states: IndexVec::new(),
@@ -799,7 +818,12 @@ impl<'db> UseDefMapBuilder<'db> {
         symbol_state.record_binding(def_id, self.scope_start_visibility);
     }
 
-    pub(super) fn record_use(&mut self, symbol: ScopedSymbolId, use_id: ScopedUseId) {
+    pub(super) fn record_use(
+        &mut self,
+        symbol: ScopedSymbolId,
+        use_id: ScopedUseId,
+        expression_id: ScopedExpressionId,
+    ) {
         // We have a use of a symbol; clone the current bindings for that symbol, and record them
         // as the live bindings for this use.
         let new_use = self
@@ -807,8 +831,14 @@ impl<'db> UseDefMapBuilder<'db> {
             .push(self.symbol_states[symbol].bindings().clone());
         debug_assert_eq!(use_id, new_use);
 
-        let new_use = self.reachability_by_use.push(self.reachability);
+        // Track reachability of all uses of symbols to silence `unresolved-reference`
-        debug_assert_eq!(use_id, new_use);
+        // diagnostics in unreachable code.
+        self.record_expression_reachability(expression_id);
+    }
+
+    pub(super) fn record_expression_reachability(&mut self, expression_id: ScopedExpressionId) {
+        self.expression_reachability
+            .insert(expression_id, self.reachability);
     }
 
     pub(super) fn snapshot_eager_bindings(
@@ -905,7 +935,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.reachability_by_use.shrink_to_fit();
+        self.expression_reachability.shrink_to_fit();
         self.declarations_by_binding.shrink_to_fit();
         self.bindings_by_declaration.shrink_to_fit();
         self.eager_bindings.shrink_to_fit();
@@ -916,7 +946,7 @@ impl<'db> UseDefMapBuilder<'db> {
             narrowing_constraints: self.narrowing_constraints.build(),
             visibility_constraints: self.visibility_constraints.build(),
             bindings_by_use: self.bindings_by_use,
-            reachability_by_use: self.reachability_by_use,
+            expression_reachability: self.expression_reachability,
             public_symbols: self.symbol_states,
             declarations_by_binding: self.declarations_by_binding,
             bindings_by_declaration: self.bindings_by_declaration,

crates/red_knot_python_semantic/src/types/infer.rs

@@ -590,6 +590,17 @@ 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 {
+        match self.deferred_state {
+            DeferredExpressionState::InStringAnnotation(enclosing_string_expression) => {
+                enclosing_string_expression
+            }
+            _ => expr.scoped_expression_id(self.db(), self.scope()),
+        }
+    }
+
     fn in_stub(&self) -> bool {
         self.context.in_stub()
     }
@@ -4287,8 +4298,8 @@ impl<'db> TypeInferenceBuilder<'db> {
         let use_def = self.index.use_def_map(file_scope_id);
 
         // If we're inferring types of deferred expressions, always treat them as public symbols
-        let (local_scope_symbol, report_unresolved_usage) = if self.is_deferred() {
+        let local_scope_symbol = if self.is_deferred() {
-            let symbol = if let Some(symbol_id) = symbol_table.symbol_id_by_name(symbol_name) {
+            if let Some(symbol_id) = symbol_table.symbol_id_by_name(symbol_name) {
                 symbol_from_bindings(db, use_def.public_bindings(symbol_id))
             } else {
                 assert!(
@@ -4296,16 +4307,10 @@ impl<'db> TypeInferenceBuilder<'db> {
                     "Expected the symbol table to create a symbol for every Name node"
                 );
                 Symbol::Unbound
-            };
+            }
-
-            (symbol, true)
         } else {
             let use_id = name_node.scoped_use_id(db, scope);
-            let symbol = symbol_from_bindings(db, use_def.bindings_at_use(use_id));
+            symbol_from_bindings(db, use_def.bindings_at_use(use_id))
-            let report_unresolved_usage =
-                self.index
-                    .is_symbol_use_reachable(db, file_scope_id, use_id);
-            (symbol, report_unresolved_usage)
         };
 
         let symbol = SymbolAndQualifiers::from(local_scope_symbol).or_fall_back_to(db, || {
@@ -4452,16 +4457,24 @@ impl<'db> TypeInferenceBuilder<'db> {
                 })
         });
 
+        let report_unresolved_usage = || {
+            self.index.is_expression_reachable(
+                db,
+                file_scope_id,
+                self.enclosing_expression_id(name_node),
+            )
+        };
+
         symbol
             .unwrap_with_diagnostic(|lookup_error| match lookup_error {
                 LookupError::Unbound(qualifiers) => {
-                    if report_unresolved_usage {
+                    if report_unresolved_usage() {
                         report_unresolved_reference(&self.context, name_node);
                     }
                     TypeAndQualifiers::new(Type::unknown(), qualifiers)
                 }
                 LookupError::PossiblyUnbound(type_when_bound) => {
-                    if report_unresolved_usage {
+                    if report_unresolved_usage() {
                         report_possibly_unresolved_reference(&self.context, name_node);
                     }
                     type_when_bound
@@ -4494,43 +4507,53 @@ impl<'db> TypeInferenceBuilder<'db> {
             .member(db, &attr.id)
             .unwrap_with_diagnostic(|lookup_error| match lookup_error {
                 LookupError::Unbound(_) => {
-                    let bound_on_instance = match value_type {
+                    let report_unresolved_attribute = self
-                        Type::ClassLiteral(class) => {
+                        .index
-                            !class.instance_member(db, None, attr).symbol.is_unbound()
+                        .is_expression_reachable(
-                        }
+                            db,
-                        Type::SubclassOf(subclass_of @ SubclassOfType { .. }) => {
+                            self.scope().file_scope_id(db),
-                            match subclass_of.subclass_of() {
+                            self.enclosing_expression_id(attribute),
-                                ClassBase::Class(class) => {
+                        );
-                                    !class.instance_member(db, attr).symbol.is_unbound()
-                                }
-                                ClassBase::Dynamic(_) => unreachable!(
-                                    "Attribute lookup on a dynamic `SubclassOf` type should always return a bound symbol"
-                                ),
-                            }
-                        }
-                        _ => false,
-                    };
 
-                    if bound_on_instance {
+                    if report_unresolved_attribute {
-                        self.context.report_lint(
+                        let bound_on_instance = match value_type {
-                            &UNRESOLVED_ATTRIBUTE,
+                            Type::ClassLiteral(class) => {
-                            attribute,
+                                !class.instance_member(db, None, attr).symbol.is_unbound()
-                            format_args!(
+                            }
-                                "Attribute `{}` can only be accessed on instances, not on the class object `{}` itself.",
+                            Type::SubclassOf(subclass_of @ SubclassOfType { .. }) => {
-                                attr.id,
+                                match subclass_of.subclass_of() {
-                                value_type.display(db)
+                                    ClassBase::Class(class) => {
-                            ),
+                                        !class.instance_member(db, attr).symbol.is_unbound()
-                        );
+                                    }
-                    } else {
+                                    ClassBase::Dynamic(_) => unreachable!(
-                        self.context.report_lint(
+                                        "Attribute lookup on a dynamic `SubclassOf` type should always return a bound symbol"
-                            &UNRESOLVED_ATTRIBUTE,
+                                    ),
-                            attribute,
+                                }
-                            format_args!(
+                            }
-                                "Type `{}` has no attribute `{}`",
+                            _ => false,
-                                value_type.display(db),
+                        };
-                                attr.id
+
-                            ),
+                        if bound_on_instance {
-                        );
+                            self.context.report_lint(
+                                &UNRESOLVED_ATTRIBUTE,
+                                attribute,
+                                format_args!(
+                                    "Attribute `{}` can only be accessed on instances, not on the class object `{}` itself.",
+                                    attr.id,
+                                    value_type.display(db)
+                                ),
+                            );
+                        } else {
+                            self.context.report_lint(
+                                &UNRESOLVED_ATTRIBUTE,
+                                attribute,
+                                format_args!(
+                                    "Type `{}` has no attribute `{}`",
+                                    value_type.display(db),
+                                    attr.id
+                                ),
+                            );
+                        }
                     }
 
                     Type::unknown().into()
@@ -6368,7 +6391,9 @@ impl<'db> TypeInferenceBuilder<'db> {
                 // String annotations are always evaluated in the deferred context.
                 self.infer_annotation_expression(
                     parsed.expr(),
-                    DeferredExpressionState::InStringAnnotation,
+                    DeferredExpressionState::InStringAnnotation(
+                        self.enclosing_expression_id(string),
+                    ),
                 )
             }
             None => TypeAndQualifiers::unknown(),
@@ -6732,7 +6757,9 @@ impl<'db> TypeInferenceBuilder<'db> {
                 // String annotations are always evaluated in the deferred context.
                 self.infer_type_expression_with_state(
                     parsed.expr(),
-                    DeferredExpressionState::InStringAnnotation,
+                    DeferredExpressionState::InStringAnnotation(
+                        self.enclosing_expression_id(string),
+                    ),
                 )
             }
             None => Type::unknown(),
@@ -7449,19 +7476,22 @@ enum DeferredExpressionState {
     ///
     /// The annotation of `a` is completely inside a string while for `b`, it's only partially
     /// stringified.
-    InStringAnnotation,
+    ///
+    /// This variant wraps a [`ScopedExpressionId`] that allows us to retrieve
+    /// the original [`ast::ExprStringLiteral`] node which created the string annotation
+    InStringAnnotation(ScopedExpressionId),
 }
 
 impl DeferredExpressionState {
     const fn is_deferred(self) -> bool {
         matches!(
             self,
-            DeferredExpressionState::Deferred | DeferredExpressionState::InStringAnnotation
+            DeferredExpressionState::Deferred | DeferredExpressionState::InStringAnnotation(_)
         )
     }
 
     const fn in_string_annotation(self) -> bool {
-        matches!(self, DeferredExpressionState::InStringAnnotation)
+        matches!(self, DeferredExpressionState::InStringAnnotation(_))
     }
 }