From 149350bf39f7cc7bb99cd4d8e031ce4a369321b3 Mon Sep 17 00:00:00 2001 From: David Peter Date: Tue, 8 Jul 2025 16:26:09 +0200 Subject: [PATCH] [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](https://github.com/sphinx-doc/sphinx/blob/7b4164a5f2b64781475e64daf2d05cce2a0261d8/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](https://github.com/python-trio/trio/blob/5471a37e82b36f556e0d26b36cb95a6b05afbef1/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 --- ...-_Full_diagnostics_(174fdd8134fb325b).snap | 42 +++++++ .../resources/mdtest/type_qualifiers/final.md | 116 ++++++++++++++++-- .../src/semantic_index/definition.rs | 9 ++ .../src/semantic_index/use_def.rs | 26 ++-- crates/ty_python_semantic/src/types.rs | 13 +- crates/ty_python_semantic/src/types/infer.rs | 86 ++++++++++--- 6 files changed, 252 insertions(+), 40 deletions(-) create mode 100644 crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_`typing.Final`_-_Full_diagnostics_(174fdd8134fb325b).snap diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_`typing.Final`_-_Full_diagnostics_(174fdd8134fb325b).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_`typing.Final`_-_Full_diagnostics_(174fdd8134fb325b).snap new file mode 100644 index 0000000000..516444f9aa --- /dev/null +++ b/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 + +``` diff --git a/crates/ty_python_semantic/resources/mdtest/type_qualifiers/final.md b/crates/ty_python_semantic/resources/mdtest/type_qualifiers/final.md index 224115f942..0b75bb85e6 100644 --- a/crates/ty_python_semantic/resources/mdtest/type_qualifiers/final.md +++ b/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 -FINAL_B = 2 -FINAL_C = 2 -FINAL_D = 2 -FINAL_E = 2 -FINAL_F = 2 +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 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 + + + +```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 diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index e070dce353..8b4a679a5c 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/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., diff --git a/crates/ty_python_semantic/src/semantic_index/use_def.rs b/crates/ty_python_semantic/src/semantic_index/use_def.rs index 671e68db31..33dc7d8989 100644 --- a/crates/ty_python_semantic/src/semantic_index/use_def.rs +++ b/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, 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, Bindings>, /// [`PlaceState`] visible at end of scope for each place. end_of_scope_places: IndexVec, @@ -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, Declarations>, - /// Live bindings for each so-far-recorded declaration. - bindings_by_declaration: FxHashMap, Bindings>, + /// Live bindings for each so-far-recorded definition. + bindings_by_definition: FxHashMap, Bindings>, /// Currently live bindings and declarations for each place. place_states: IndexVec, @@ -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, } diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 4a834fe38c..21246a75d7 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/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__") - .place; + .member(db, "__dict__"); } // 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 } } diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs index b0ac65de25..9575ea738c 100644 --- a/crates/ty_python_semantic/src/types/infer.rs +++ b/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()) + ( + resolved_place + .ignore_possibly_unbound() + .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; }