[ty] Implement typing.final for methods
#21646
Merged
+2,607
−159
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlexWaygood
force-pushed
the
alex/final-method
branch
2 times, most recently
from
b9916af to
830e084
Compare
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-11-28 15:14:58.574457152 +0000
+++ new-output.txt 2025-11-28 15:15:02.124465943 +0000
@@ -795,6 +795,7 @@
overloads_definitions.py:146:48: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int | str`
overloads_definitions.py:159:46: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int | str`
overloads_definitions.py:167:44: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int | str`
+overloads_definitions.py:181:9: error[override-of-final-method] Cannot override final member `final_method` from superclass `Base`
overloads_definitions.py:183:10: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int | str`
overloads_definitions.py:198:9: error[invalid-explicit-override] Method `bad_override` is decorated with `@override` but does not override anything
overloads_definitions.py:198:45: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int | str`
@@ -807,6 +808,7 @@
overloads_definitions_stub.pyi:44:9: error[invalid-overload] Overloaded function `func6` does not use the `@classmethod` decorator consistently
overloads_definitions_stub.pyi:76:9: error[invalid-overload] `@final` decorator should be applied only to the first overload
overloads_definitions_stub.pyi:86:9: error[invalid-overload] `@final` decorator should be applied only to the first overload
+overloads_definitions_stub.pyi:111:9: error[override-of-final-method] Cannot override final member `final_method` from superclass `Base`
overloads_definitions_stub.pyi:122:9: error[invalid-explicit-override] Method `bad_override` is decorated with `@override` but does not override anything
overloads_definitions_stub.pyi:147:9: error[invalid-overload] `@override` decorator should be applied only to the first overload
overloads_evaluation.py:38:1: error[no-matching-overload] No overload of function `example1_1` matches arguments
@@ -894,8 +896,15 @@
qualifiers_final_annotation.py:155:9: error[invalid-assignment] Reassignment of `Final` symbol `x` is not allowed: Symbol later reassigned here
qualifiers_final_annotation.py:166:1: error[invalid-assignment] Reassignment of `Final` symbol `TEN` is not allowed: Reassignment of `Final` symbol
qualifiers_final_decorator.py:21:16: error[subclass-of-final-class] Class `Derived1` cannot inherit from final class `Base1`
+qualifiers_final_decorator.py:56:9: error[override-of-final-method] Cannot override final member `method1` from superclass `Base2`
+qualifiers_final_decorator.py:60:9: error[override-of-final-method] Cannot override final member `method2` from superclass `Base2`
+qualifiers_final_decorator.py:64:9: error[override-of-final-method] Cannot override final member `method3` from superclass `Base2`
+qualifiers_final_decorator.py:75:9: error[override-of-final-method] Cannot override final member `method4` from superclass `Base2`
qualifiers_final_decorator.py:89:9: error[invalid-overload] `@final` decorator should be applied only to the overload implementation
+qualifiers_final_decorator.py:89:9: error[override-of-final-method] Cannot override final member `method` from superclass `Base3`
+qualifiers_final_decorator.py:102:9: error[override-of-final-method] Cannot override final member `method` from superclass `Base4`
qualifiers_final_decorator.py:118:9: error[invalid-method-override] Invalid override of method `method`: Definition is incompatible with `Base5_2.method`
+qualifiers_final_decorator.py:118:9: error[override-of-final-method] Cannot override final member `method` from superclass `Base5_2`
specialtypes_any.py:86:1: error[type-assertion-failure] Type `int` does not match asserted type `int | @Todo(instance attribute on class with dynamic base)`
specialtypes_never.py:19:22: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Never`
specialtypes_never.py:32:12: error[invalid-return-type] Return type does not match returned value: expected `int`, found `Literal["whatever works"]`
@@ -1029,4 +1038,4 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Unknown key "title" for TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions
-Found 1031 diagnostics
+Found 1040 diagnostics
|
|
Member
Author
|
All the changes to the typing conformance suite look good; the lines where the new diagnostics appear all have |
Member
Author
|
The primer report also all look correct! All of the new diagnostics already have suppression comments for other tools (pylint, pyright or mypy) that we either interpret as blanket |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
override-of-final-method |
4 | 0 | 0 |
unused-ignore-comment |
0 | 4 | 0 |
invalid-argument-type |
0 | 0 | 2 |
unsupported-base |
1 | 0 | 0 |
| Total | 5 | 4 | 2 |
AlexWaygood
force-pushed
the
alex/final-method
branch
from
830e084 to
070e2c5
Compare
CodSpeed Performance ReportMerging #21646 will degrade performances by 25.39%Comparing Summary
Benchmarks breakdown
Footnotes
|
AlexWaygood
force-pushed
the
alex/final-method
branch
from
070e2c5 to
7e739c6
Compare
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Member
Author
you hate to see it, but this regression is only manifesting on one (deliberately strange) microbenchmark, not on any of our real-world benchmarks. |
Member
Author
|
hmm I don't know why anything I've done in this PR would change the intersection/union order here. I'm seeing the same failures locally, though, so I guess I can just change the mdtest assertion. cc. @ibraheemdev -- could this be related to #21267 ? |
AlexWaygood
commented
Nov 26, 2025
Member
|
I think #21267 just means started seeing more intersection types in mdtests that we were previously hiding, not the source of the problem. I suppose it is possible that we are creating the intersection types in a non-deterministic way during multi-inference, but I'm not sure how that would be. |
Member
Author
|
I think our intersection builder is generally deterministic. There are a lot of intersection types in our ecosystem diagnostics, so if the order of elements in our intersections was nondeterministic, we would constantly get spurious mypy_primer diffs in CI -- but we don't. |
Contributor
|
It's deterministic as long as you add the types in a deterministic order; I suspect somewhere we are building an intersection from a non-deterministically-ordered iterator of types. I'm auditing intersection-building call sites to see if I can find it. |
Member
Author
yes, that's what I meant |
Member
Author
Well, okay, after fixing some edge-case bugs there are now regressions on other benchmarks too. |
Member
Author
|
I tried playing around with this optimisation locally: diff --git a/crates/ty_python_semantic/src/types/liskov.rs b/crates/ty_python_semantic/src/types/liskov.rs
index c377212831..0b8f354970 100644
--- a/crates/ty_python_semantic/src/types/liskov.rs
+++ b/crates/ty_python_semantic/src/types/liskov.rs
@@ -2,6 +2,8 @@
//!
//! [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle
+use std::cell::LazyCell;
+
use ruff_db::diagnostic::Annotation;
use rustc_hash::FxHashSet;
@@ -9,7 +11,7 @@ use crate::{
place::Place,
semantic_index::{place_table, use_def_map},
types::{
- ClassBase, ClassLiteral, ClassType, KnownClass, Type,
+ ClassBase, ClassLiteral, ClassType, KnownClass, SpecialFormType, Type,
class::CodeGeneratorKind,
context::InferContext,
diagnostic::{
@@ -56,7 +58,7 @@ fn check_class_declaration<'db>(
};
let (literal, specialization) = class.class_literal(db);
- let class_kind = CodeGeneratorKind::from_class(db, literal, specialization);
+ let class_kind = LazyCell::new(|| CodeGeneratorKind::from_class(db, literal, specialization));
let mut subclass_overrides_superclass_declaration = false;
let mut has_dynamic_superclass = false;
@@ -171,7 +173,7 @@ fn check_class_declaration<'db>(
// Synthesized `__replace__` methods on dataclasses are not checked
if &member.name == "__replace__"
- && matches!(class_kind, Some(CodeGeneratorKind::DataclassLike(_)))
+ && matches!(*class_kind, Some(CodeGeneratorKind::DataclassLike(_)))
{
continue;
}
@@ -209,7 +211,14 @@ fn check_class_declaration<'db>(
{
subclass_overrides_superclass_declaration = true;
}
- } else if class_kind == Some(CodeGeneratorKind::NamedTuple) {
+ // avoid checking `class_kind` here just to check if it's a `NamedTuple`:
+ // we can get that from the explicit bases directly, and dereferencing
+ // `class_kind` can result in unnecessary memory use due to it being a
+ // `LazyCell` wrapper around a Salsa-cached method.
+ } else if literal
+ .explicit_bases(db)
+ .contains(&Type::SpecialForm(SpecialFormType::NamedTuple))
+ {
if !KnownClass::NamedTupleFallback
.to_instance(db)
.member(db, &member.name)I can't measure any speedup locally, but it's possible it might also reduce memory usage... I could push it and see what happens in CI? It does add a little more complexity to the code. |
carljm
added a commit
that referenced
this pull request
Nov 27, 2025
## Summary This caused "deterministic but chaotic" ordering of some intersection types in diagnostics. When calling a union, we infer the argument type once per matching parameter type, intersecting the inferred types for the argument expression, and we did that in an unpredictable order. We do need a hashset here for de-duplication. Sometimes we call large unions where the type for a given parameter is the same across the union, we should infer the argument once per parameter type, not once per union element. So use an `FxIndexSet` instead of an `FxHashSet`. ## Test Plan With this change, switching between `main` and #21646 no longer changes the ordering of the intersection type in the test in cca3a80
MichaReiser
reviewed
Nov 27, 2025
MichaReiser
reviewed
Nov 27, 2025
AlexWaygood
force-pushed
the
alex/final-method
branch
from
56cd2aa to
623ca67
Compare
AlexWaygood
commented
Nov 27, 2025
AlexWaygood
force-pushed
the
alex/final-method
branch
from
623ca67 to
408895e
Compare
Member
Author
|
I ran ty on a large codebase locally with Results on this branchResults on main |
Member
Author
|
The memory reports indicate that the increased memory usage is because we're doing a lot more - `ClassLiteral < 'db >::implicit_attribute_inner_::interned_arguments` metadata=189.96MB fields=135.68MB count=3392059
+ `ClassLiteral < 'db >::implicit_attribute_inner_::interned_arguments` metadata=259.75MB fields=185.53MB count=4638323
- `Type < 'db >::class_member_with_policy_::interned_arguments` metadata=184.12MB fields=157.81MB count=3287789
+ `Type < 'db >::class_member_with_policy_::interned_arguments` metadata=210.21MB fields=180.18MB count=3753792
`ClassLiteral < 'db >::implicit_attribute_inner_ -> ty_python_semantic::types::member::Member<'_>`
- metadata=396.92MB fields=108.55MB count=3392059
+ metadata=516.93MB fields=148.43MB count=4638323
`Type < 'db >::class_member_with_policy_ -> ty_python_semantic::place::PlaceAndQualifiers<'_>`
- metadata=976.98MB fields=105.21MB count=3287789
+ metadata=1078.42MB fields=120.12MB count=3753792
`Type < 'db >::member_lookup_with_policy_ -> ty_python_semantic::place::PlaceAndQualifiers<'_>`
- metadata=887.72MB fields=80.30MB count=2509480
+ metadata=1071.66MB fields=92.52MB count=2891106That makes sense: previously, we only did the I'm not sure there's realistically any way to get around this. With the checks we have in place right now we don't really need to look up implicit instance attributes: we can just look at the type as declared/defined in the class body. So it's possible I could avoid the |
MichaReiser
approved these changes
Nov 28, 2025
...t/snapshots/final.md_-_Tests_for_the_`@typi…_-_Cannot_override_a_me…_(338615109711a91b).snap
Outdated
Show resolved
Hide resolved
AlexWaygood
force-pushed
the
alex/final-method
branch
from
3a7eb08 to
cb520d5
Compare
AlexWaygood
commented
Nov 28, 2025
Member
Author
There was a problem hiding this comment.
@MichaReiser, I feel like I remember Ruff having this issue in the past -- can you remember what the solution was? Ideally we'd mark the autofix as display-only if we can see that it's going to introduce invalid syntax. (Or use a range_replacement that replaces the definition with pass.)
Member
There was a problem hiding this comment.
We have a DisplayOnly Applicability. I don't think they are shown anywhere by default (ever?). The IDE also filters out manual only edits.
I don't think we should add fixes that introduce syntax errors. I'm also not convinced that it's the right fix. It's as likely that the proper fix is to remove the @final
Member
Author
There was a problem hiding this comment.
Right, I was more wondering if Ruff had a nice little routine somewhere to easily detect that this was the only statement in the if: branch (and therefore that a naive deletion would introduce an unsafe fix)
Member
There was a problem hiding this comment.
ruff/crates/ruff_linter/src/fix/edits.rs
Lines 25 to 63 in 096e539
Member
Author
There was a problem hiding this comment.
seems complicated, I'll leave this for a followup
Member
Author
|
Thanks for the great review @MichaReiser, it flagged a bunch of edge cases I missed! |
AlexWaygood
force-pushed
the
alex/final-method
branch
from
03e7236 to
33915c1
Compare
AlexWaygood
force-pushed
the
alex/final-method
branch
from
4ddd64e to
f8eb0d2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
A method or property on a superclass decorated with
@finalcannot be overridden on a subclass.Fixes astral-sh/ty#546.
This currently only implements the check for properties and function-literal types in the class body, not for methods that end up as having other types in the class body (e.g. those decorated with
@lru_cache). I'll open a followup issue tracking that, if this is merged.Test Plan
mdtests/snapshots