[ty] Enforce typing.Final (#19178)

## Summary

Emit a diagnostic when a `Final`-qualified symbol is modified. This
first iteration only works for name targets. Tests with TODO comments
were added for attribute assignments as well.

related ticket: https://github.com/astral-sh/ty/issues/158

## Ecosystem impact

Correctly identified [modification of a `Final`
symbol](7b4164a5f2/sphinx/__init__.py (L44))
(behind a `# type: ignore`):
```diff
- warning[unused-ignore-comment] sphinx/__init__.py:44:56: Unused blanket `type: ignore` directive
```
And the same
[here](5471a37e82/src/trio/_core/_run.py (L128)):
```diff
- warning[unused-ignore-comment] src/trio/_core/_run.py:128:45: Unused blanket `type: ignore` directive
```

## Test Plan

New Markdown tests

crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_`typing.Final`_-_Full_diagnostics_(174fdd8134fb325b).snap

@@ -0,0 +1,42 @@
+---
+source: crates/ty_test/src/lib.rs
+expression: snapshot
+---
+---
+mdtest name: final.md - `typing.Final` - Full diagnostics
+mdtest path: crates/ty_python_semantic/resources/mdtest/type_qualifiers/final.md
+---
+
+# Python source files
+
+## mdtest_snippet.py
+
+```
+1 | from typing import Final
+2 | 
+3 | MY_CONSTANT: Final[int] = 1
+4 | 
+5 | # more code
+6 | 
+7 | MY_CONSTANT = 2  # error: [invalid-assignment]
+```
+
+# Diagnostics
+
+```
+error[invalid-assignment]: Reassignment of `Final` symbol `MY_CONSTANT` is not allowed
+ --> src/mdtest_snippet.py:3:1
+  |
+1 | from typing import Final
+2 |
+3 | MY_CONSTANT: Final[int] = 1
+  | ----------- Original definition
+4 |
+5 | # more code
+6 |
+7 | MY_CONSTANT = 2  # error: [invalid-assignment]
+  | ^^^^^^^^^^^ Reassignment of `Final` symbol
+  |
+info: rule `invalid-assignment` is enabled by default
+
+```

crates/ty_python_semantic/resources/mdtest/type_qualifiers/final.md

@@ -100,9 +100,13 @@ reveal_type(C().FINAL_D)  # revealed: Unknown
 
 ## Not modifiable
 
+### Names
+
 Symbols qualified with `Final` cannot be reassigned, and attempting to do so will result in an
 error:
 
+`mod.py`:
+
 ```py
 from typing import Final, Annotated
 
@@ -114,13 +118,97 @@ FINAL_E: Final[int]
 FINAL_E = 1
 FINAL_F: Final = 1
 
-# TODO: all of these should be errors
+FINAL_A = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_A` is not allowed"
-FINAL_A = 2
+FINAL_B = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_B` is not allowed"
-FINAL_B = 2
+FINAL_C = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_C` is not allowed"
-FINAL_C = 2
+FINAL_D = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_D` is not allowed"
-FINAL_D = 2
+FINAL_E = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_E` is not allowed"
-FINAL_E = 2
+FINAL_F = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_F` is not allowed"
-FINAL_F = 2
+
+def global_use():
+    global FINAL_A, FINAL_B, FINAL_C, FINAL_D, FINAL_E, FINAL_F
+    FINAL_A = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_A` is not allowed"
+    FINAL_B = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_B` is not allowed"
+    FINAL_C = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_C` is not allowed"
+    FINAL_D = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_D` is not allowed"
+    FINAL_E = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_E` is not allowed"
+    FINAL_F = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_F` is not allowed"
+
+def local_use():
+    # These are not errors, because they refer to local variables
+    FINAL_A = 2
+    FINAL_B = 2
+    FINAL_C = 2
+    FINAL_D = 2
+    FINAL_E = 2
+    FINAL_F = 2
+
+def nonlocal_use():
+    X: Final[int] = 1
+    def inner():
+        nonlocal X
+        # TODO: this should be an error
+        X = 2
+```
+
+`main.py`:
+
+```py
+from mod import FINAL_A, FINAL_B, FINAL_C, FINAL_D, FINAL_E, FINAL_F
+
+FINAL_A = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_A` is not allowed"
+FINAL_B = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_B` is not allowed"
+FINAL_C = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_C` is not allowed"
+FINAL_D = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_D` is not allowed"
+FINAL_E = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_E` is not allowed"
+FINAL_F = 2  # error: [invalid-assignment] "Reassignment of `Final` symbol `FINAL_F` is not allowed"
+```
+
+### Attributes
+
+Assignments to attributes qualified with `Final` are also not allowed:
+
+```py
+from typing import Final
+
+class C:
+    FINAL_A: Final[int] = 1
+    FINAL_B: Final = 1
+
+    def __init__(self):
+        self.FINAL_C: Final[int] = 1
+        self.FINAL_D: Final = 1
+
+# TODO: these should be errors (that mention `Final`)
+C.FINAL_A = 2
+# error: [invalid-assignment] "Object of type `Literal[2]` is not assignable to attribute `FINAL_B` of type `Literal[1]`"
+C.FINAL_B = 2
+
+# TODO: these should be errors (that mention `Final`)
+c = C()
+c.FINAL_A = 2
+# error: [invalid-assignment] "Object of type `Literal[2]` is not assignable to attribute `FINAL_B` of type `Literal[1]`"
+c.FINAL_B = 2
+c.FINAL_C = 2
+c.FINAL_D = 2
+```
+
+## Mutability
+
+Objects qualified with `Final` *can be modified*. `Final` represents a constant reference to an
+object, but that object itself may still be mutable:
+
+```py
+from typing import Final
+
+class C:
+    x: int = 1
+
+FINAL_C_INSTANCE: Final[C] = C()
+FINAL_C_INSTANCE.x = 2
+
+FINAL_LIST: Final[list[int]] = [1, 2, 3]
+FINAL_LIST[0] = 4
 ```
 
 ## Too many arguments
@@ -168,4 +256,18 @@ class C:
     NO_RHS: Final
 ```
 
+## Full diagnostics
+
+<!-- snapshot-diagnostics -->
+
+```py
+from typing import Final
+
+MY_CONSTANT: Final[int] = 1
+
+# more code
+
+MY_CONSTANT = 2  # error: [invalid-assignment]
+```
+
 [`typing.final`]: https://docs.python.org/3/library/typing.html#typing.Final

crates/ty_python_semantic/src/semantic_index/definition.rs

@@ -618,6 +618,15 @@ impl DefinitionKind<'_> {
         }
     }
 
+    pub(crate) fn is_import(&self) -> bool {
+        matches!(
+            self,
+            DefinitionKind::Import(_)
+                | DefinitionKind::ImportFrom(_)
+                | DefinitionKind::StarImport(_)
+        )
+    }
+
     /// Returns the [`TextRange`] of the definition target.
     ///
     /// A definition target would mainly be the node representing the place being defined i.e.,

crates/ty_python_semantic/src/semantic_index/use_def.rs

@@ -306,7 +306,10 @@ pub(crate) struct UseDefMap<'db> {
     /// If the definition is both a declaration and a binding -- `x: int = 1` for example -- then
     /// we don't actually need anything here, all we'll need to validate is that our own RHS is a
     /// valid assignment to our own annotation.
-    bindings_by_declaration: FxHashMap<Definition<'db>, Bindings>,
+    ///
+    /// If we see a binding to a `Final`-qualified symbol, we also need this map to find previous
+    /// bindings to that symbol. If there are any, the assignment is invalid.
+    bindings_by_definition: FxHashMap<Definition<'db>, Bindings>,
 
     /// [`PlaceState`] visible at end of scope for each place.
     end_of_scope_places: IndexVec<ScopedPlaceId, PlaceState>,
@@ -448,12 +451,12 @@ impl<'db> UseDefMap<'db> {
         }
     }
 
-    pub(crate) fn bindings_at_declaration(
+    pub(crate) fn bindings_at_definition(
         &self,
-        declaration: Definition<'db>,
+        definition: Definition<'db>,
     ) -> BindingWithConstraintsIterator<'_, 'db> {
         self.bindings_iterator(
-            &self.bindings_by_declaration[&declaration],
+            &self.bindings_by_definition[&definition],
             BoundnessAnalysis::BasedOnUnboundVisibility,
         )
     }
@@ -744,8 +747,8 @@ pub(super) struct UseDefMapBuilder<'db> {
     /// Live declarations for each so-far-recorded binding.
     declarations_by_binding: FxHashMap<Definition<'db>, Declarations>,
 
-    /// Live bindings for each so-far-recorded declaration.
+    /// Live bindings for each so-far-recorded definition.
-    bindings_by_declaration: FxHashMap<Definition<'db>, Bindings>,
+    bindings_by_definition: FxHashMap<Definition<'db>, Bindings>,
 
     /// Currently live bindings and declarations for each place.
     place_states: IndexVec<ScopedPlaceId, PlaceState>,
@@ -772,7 +775,7 @@ impl<'db> UseDefMapBuilder<'db> {
             reachability: ScopedReachabilityConstraintId::ALWAYS_TRUE,
             node_reachability: FxHashMap::default(),
             declarations_by_binding: FxHashMap::default(),
-            bindings_by_declaration: FxHashMap::default(),
+            bindings_by_definition: FxHashMap::default(),
             place_states: IndexVec::new(),
             reachable_definitions: IndexVec::new(),
             eager_snapshots: EagerSnapshots::default(),
@@ -808,6 +811,9 @@ impl<'db> UseDefMapBuilder<'db> {
         binding: Definition<'db>,
         is_place_name: bool,
     ) {
+        self.bindings_by_definition
+            .insert(binding, self.place_states[place].bindings().clone());
+
         let def_id = self.all_definitions.push(DefinitionState::Defined(binding));
         let place_state = &mut self.place_states[place];
         self.declarations_by_binding
@@ -942,7 +948,7 @@ impl<'db> UseDefMapBuilder<'db> {
             .all_definitions
             .push(DefinitionState::Defined(declaration));
         let place_state = &mut self.place_states[place];
-        self.bindings_by_declaration
+        self.bindings_by_definition
             .insert(declaration, place_state.bindings().clone());
         place_state.record_declaration(def_id, self.reachability);
 
@@ -1119,7 +1125,7 @@ impl<'db> UseDefMapBuilder<'db> {
         self.bindings_by_use.shrink_to_fit();
         self.node_reachability.shrink_to_fit();
         self.declarations_by_binding.shrink_to_fit();
-        self.bindings_by_declaration.shrink_to_fit();
+        self.bindings_by_definition.shrink_to_fit();
         self.eager_snapshots.shrink_to_fit();
 
         UseDefMap {
@@ -1132,7 +1138,7 @@ impl<'db> UseDefMapBuilder<'db> {
             end_of_scope_places: self.place_states,
             reachable_definitions: self.reachable_definitions,
             declarations_by_binding: self.declarations_by_binding,
-            bindings_by_declaration: self.bindings_by_declaration,
+            bindings_by_definition: self.bindings_by_definition,
             eager_snapshots: self.eager_snapshots,
             end_of_scope_reachability: self.reachability,
         }

crates/ty_python_semantic/src/types.rs

@@ -2613,7 +2613,7 @@ impl<'db> Type<'db> {
     /// See also: [`Type::member`]
     fn static_member(&self, db: &'db dyn Db, name: &str) -> Place<'db> {
         if let Type::ModuleLiteral(module) = self {
-            module.static_member(db, name)
+            module.static_member(db, name).place
         } else if let place @ Place::Type(_, _) = self.class_member(db, name.into()).place {
             place
         } else if let Some(place @ Place::Type(_, _)) =
@@ -3067,7 +3067,7 @@ impl<'db> Type<'db> {
                 Place::bound(Type::IntLiteral(i64::from(bool_value))).into()
             }
 
-            Type::ModuleLiteral(module) => module.static_member(db, name_str).into(),
+            Type::ModuleLiteral(module) => module.static_member(db, name_str),
 
             _ if policy.no_instance_fallback() => self.invoke_descriptor_protocol(
                 db,
@@ -7511,15 +7511,14 @@ pub struct ModuleLiteralType<'db> {
 impl get_size2::GetSize for ModuleLiteralType<'_> {}
 
 impl<'db> ModuleLiteralType<'db> {
-    fn static_member(self, db: &'db dyn Db, name: &str) -> Place<'db> {
+    fn static_member(self, db: &'db dyn Db, name: &str) -> PlaceAndQualifiers<'db> {
         // `__dict__` is a very special member that is never overridden by module globals;
         // we should always look it up directly as an attribute on `types.ModuleType`,
         // never in the global scope of the module.
         if name == "__dict__" {
             return KnownClass::ModuleType
                 .to_instance(db)
-                .member(db, "__dict__")
+                .member(db, "__dict__");
-                .place;
         }
 
         // If the file that originally imported the module has also imported a submodule
@@ -7538,7 +7537,8 @@ impl<'db> ModuleLiteralType<'db> {
             full_submodule_name.extend(&submodule_name);
             if imported_submodules.contains(&full_submodule_name) {
                 if let Some(submodule) = resolve_module(db, &full_submodule_name) {
-                    return Place::bound(Type::module_literal(db, importing_file, &submodule));
+                    return Place::bound(Type::module_literal(db, importing_file, &submodule))
+                        .into();
                 }
             }
         }
@@ -7547,7 +7547,6 @@ impl<'db> ModuleLiteralType<'db> {
             .file()
             .map(|file| imported_symbol(db, file, name, None))
             .unwrap_or_default()
-            .place
     }
 }
 

crates/ty_python_semantic/src/types/infer.rs

@@ -1567,21 +1567,21 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
         let place_id = binding.place(self.db());
         let place = place_table.place_expr(place_id);
         let skip_non_global_scopes = self.skip_non_global_scopes(file_scope_id, place_id);
-        let declarations = if skip_non_global_scopes {
+        let (declarations, is_local) = if skip_non_global_scopes {
             match self
                 .index
                 .place_table(FileScopeId::global())
                 .place_id_by_expr(&place.expr)
             {
-                Some(id) => global_use_def_map.end_of_scope_declarations(id),
+                Some(id) => (global_use_def_map.end_of_scope_declarations(id), false),
                 // This case is a syntax error (load before global declaration) but ignore that here
-                None => use_def.declarations_at_binding(binding),
+                None => (use_def.declarations_at_binding(binding), true),
             }
         } else {
-            use_def.declarations_at_binding(binding)
+            (use_def.declarations_at_binding(binding), true)
         };
 
-        let declared_ty = place_from_declarations(self.db(), declarations)
+        let (declared_ty, is_modifiable) = place_from_declarations(self.db(), declarations)
             .and_then(|place_and_quals| {
                 Ok(
                     if matches!(place_and_quals.place, Place::Type(_, Boundness::Bound)) {
@@ -1600,8 +1600,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
             .map(
                 |PlaceAndQualifiers {
                      place: resolved_place,
-                     ..
+                     qualifiers,
                  }| {
+                    let is_modifiable = !qualifiers.contains(TypeQualifiers::FINAL);
+
                     if resolved_place.is_unbound() && !place_table.place_expr(place_id).is_name() {
                         if let AnyNodeRef::ExprAttribute(ast::ExprAttribute {
                             value, attr, ..
@@ -1611,7 +1613,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                             if let Place::Type(ty, Boundness::Bound) =
                                 value_type.member(db, attr).place
                             {
-                                return ty;
+                                // TODO: also consider qualifiers on the attribute
+                                return (ty, is_modifiable);
                             }
                         } else if let AnyNodeRef::ExprSubscript(ast::ExprSubscript {
                             value,
@@ -1623,12 +1626,15 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                             let slice_ty = self.infer_expression(slice);
                             let result_ty =
                                 self.infer_subscript_expression_types(value, value_ty, slice_ty);
-                            return result_ty;
+                            return (result_ty, is_modifiable);
                         }
                     }
+                    (
                         resolved_place
                             .ignore_possibly_unbound()
-                        .unwrap_or(Type::unknown())
+                            .unwrap_or(Type::unknown()),
+                        is_modifiable,
+                    )
                 },
             )
             .unwrap_or_else(|(ty, conflicting)| {
@@ -1640,8 +1646,46 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                         format_enumeration(conflicting.iter().map(|ty| ty.display(db)))
                     ));
                 }
-                ty.inner_type()
+                (
+                    ty.inner_type(),
+                    !ty.qualifiers.contains(TypeQualifiers::FINAL),
+                )
             });
+
+        if !is_modifiable {
+            let mut previous_bindings = use_def.bindings_at_definition(binding);
+
+            // An assignment to a local `Final`-qualified symbol is only an error if there are prior bindings
+
+            let previous_definition = previous_bindings
+                .next()
+                .and_then(|r| r.binding.definition());
+
+            if !is_local || previous_definition.is_some() {
+                let place = place_table.place_expr(binding.place(db));
+                if let Some(builder) = self.context.report_lint(&INVALID_ASSIGNMENT, node) {
+                    let mut diagnostic = builder.into_diagnostic(format_args!(
+                        "Reassignment of `Final` symbol `{place}` is not allowed"
+                    ));
+
+                    diagnostic.set_primary_message("Reassignment of `Final` symbol");
+
+                    if let Some(previous_definition) = previous_definition {
+                        // It is not very helpful to show the previous definition if it results from
+                        // an import. Ideally, we would show the original definition in the external
+                        // module, but that information is currently not threaded through attribute
+                        // lookup.
+                        if !previous_definition.kind(db).is_import() {
+                            let range = previous_definition.full_range(self.db(), self.module());
+                            diagnostic.annotate(
+                                self.context.secondary(range).message("Original definition"),
+                            );
+                        }
+                    }
+                }
+            }
+        }
+
         if !bound_ty.is_assignable_to(db, declared_ty) {
             report_invalid_assignment(&self.context, node, declared_ty, bound_ty);
             // allow declarations to override inference in case of invalid assignment
@@ -1721,7 +1765,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 .is_declaration()
         );
         let use_def = self.index.use_def_map(declaration.file_scope(self.db()));
-        let prior_bindings = use_def.bindings_at_declaration(declaration);
+        let prior_bindings = use_def.bindings_at_definition(declaration);
         // unbound_ty is Never because for this check we don't care about unbound
         let inferred_ty = place_from_bindings(self.db(), prior_bindings)
             .with_qualifiers(TypeQualifiers::empty())
@@ -3680,7 +3724,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
             }
 
             Type::ModuleLiteral(module) => {
-                if let Place::Type(attr_ty, _) = module.static_member(db, attribute) {
+                if let Place::Type(attr_ty, _) = module.static_member(db, attribute).place {
                     let assignable = value_ty.is_assignable_to(db, attr_ty);
                     if assignable {
                         true
@@ -4427,7 +4471,11 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
 
         // First try loading the requested attribute from the module.
         if !import_is_self_referential {
-            if let Place::Type(ty, boundness) = module_ty.member(self.db(), name).place {
+            if let PlaceAndQualifiers {
+                place: Place::Type(ty, boundness),
+                qualifiers,
+            } = module_ty.member(self.db(), name)
+            {
                 if &alias.name != "*" && boundness == Boundness::PossiblyUnbound {
                     // TODO: Consider loading _both_ the attribute and any submodule and unioning them
                     // together if the attribute exists but is possibly-unbound.
@@ -4443,7 +4491,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 self.add_declaration_with_binding(
                     alias.into(),
                     definition,
-                    &DeclaredAndInferredType::AreTheSame(ty),
+                    &DeclaredAndInferredType::MightBeDifferent {
+                        declared_ty: TypeAndQualifiers {
+                            inner: ty,
+                            qualifiers,
+                        },
+                        inferred_ty: ty,
+                    },
                 );
                 return;
             }