[ty] Emit diagnostic for unimplemented abstract method on @final class#22753
Merged
charliermarsh merged 12 commits intomainfrom Jan 21, 2026
Merged
[ty] Emit diagnostic for unimplemented abstract method on @final class#22753charliermarsh merged 12 commits intomainfrom
charliermarsh merged 12 commits intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
charliermarsh
force-pushed
the
charlie/final
branch
from
aa9e238 to
3cb5b97
Compare
|
charliermarsh
force-pushed
the
charlie/final
branch
from
3cb5b97 to
b518c4d
Compare
charliermarsh
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-await |
2 | 0 | 6 |
invalid-parameter-default |
0 | 0 | 7 |
invalid-return-type |
0 | 5 | 2 |
possibly-missing-attribute |
3 | 0 | 1 |
invalid-argument-type |
1 | 0 | 1 |
unresolved-attribute |
0 | 0 | 2 |
unused-ignore-comment |
0 | 1 | 0 |
| Total | 6 | 6 | 19 |
MichaReiser
reviewed
Jan 20, 2026
...ests_for_the_`@typi…_-_A_`@final`_class_mus…_-_Multiple_abstract_me…_(feafee9a4abbe8d1).snap
Outdated
Show resolved
Hide resolved
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
charliermarsh
force-pushed
the
charlie/final
branch
from
b930909 to
8e17e63
Compare
charliermarsh
force-pushed
the
charlie/final
branch
from
8e17e63 to
248ab2a
Compare
Member
|
now you're emitting false-positive errors on classes like this, which can be instantiated at runtime 😄 from abc import abstractmethod, ABC
from typing import final
class Base(ABC):
@property
@abstractmethod
def f(self) -> int: ...
@final
class Child(Base):
f = 42The difference between @final
class BadChild(Base):
f: intis that |
Member
Author
|
Oh gosh. |
charliermarsh
force-pushed
the
charlie/final
branch
from
9273cf0 to
6e68821
Compare
charliermarsh
commented
Jan 20, 2026
| .with_qualifiers(type_and_qualifiers.qualifiers()), | ||
| Ok(type_and_qualifiers) => Place::Defined( | ||
| DefinedPlace::new(type_and_qualifiers.inner_type()) | ||
| .with_origin(type_and_qualifiers.origin()), |
Member
Author
There was a problem hiding this comment.
I think we were losing the origin here (unintentionally?).
Member
There was a problem hiding this comment.
yes, this looks correct! good catch
charliermarsh
force-pushed
the
charlie/final
branch
from
6e68821 to
8adff03
Compare
AlexWaygood
reviewed
Jan 20, 2026
Comment on lines
+752
to
+753
| class Derived(Base): # No error - not final, can be subclassed | ||
| pass |
Member
There was a problem hiding this comment.
(but we should emit an error when such a class is instantiated, of course -- which is astral-sh/ty#1877; it doesn't need to be implemented here)
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
AlexWaygood
reviewed
Jan 20, 2026
| /// Only returns methods that are still abstract on `self` (i.e., have not been overridden | ||
| /// with a concrete implementation anywhere in the MRO). | ||
| #[salsa::tracked(heap_size=ruff_memory_usage::heap_size)] | ||
| pub(crate) fn abstract_methods(self, db: &'db dyn Db) -> FxHashMap<Name, ClassType<'db>> { |
Member
There was a problem hiding this comment.
I think there are probably marginally more efficient ways of doing this with a single MRO iteration (if we iterated through the MRO in reverse order), but this also seems fine for a first pass
Comment on lines
1300
to
1306
| for (method_name, defining_class) in class_type.abstract_methods(db) { | ||
| let Some(builder) = self | ||
| .context | ||
| .report_lint(&UNIMPLEMENTED_ABSTRACT_METHOD, &class_node.name) | ||
| else { | ||
| continue; | ||
| }; |
Member
There was a problem hiding this comment.
if there are multiple unimplemented abstract methods, do we really want to emit a separate diagnostic for each one? Wouldn't it be better to collect a list of all the unimplemented abstract methods and then emit a single diagnostic saying "Methods foo, bar, baz are all abstract"? (But I think it would be fine to only add a subdiagnostic pointing to the definition of the first one; too many subdiagnostics would be noisy.)
Member
Author
There was a problem hiding this comment.
I personally find this more useful + consistent with our other patterns, but I don't feel strongly.
Member
There was a problem hiding this comment.
I know there's still a bunch of rules where we emit a separate diagnostic for each element in cases like this, but in general I think we aspire to group them together where possible -- e.g. here we only emit a single diagnostic, even though multiple elements of the union have the incorrect signature:
% uvx ty check foo.py
error[unknown-argument]: Argument `x` does not match any known parameter of function `f`
--> foo.py:7:10
|
5 | def i(coinflip: bool):
6 | func = f if coinflip else g if coinflip else h
7 | func(x=42)
| ^^^^
|
info: Union variant `def f() -> Unknown` is incompatible with this call site
info: Attempted to call union type `(def f() -> Unknown) | (def h(x) -> Unknown)`
info: rule `unknown-argument` is enabled by default
Found 1 diagnostic
% cat foo.py
def f(): ...
def g(): ...
def h(x): ...
def i(coinflip: bool):
func = f if coinflip else g if coinflip else h
func(x=42)
d6ab7ac to
c2a575c
Compare
AlexWaygood
approved these changes
Jan 21, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
This looks great, thank you
Comment on lines
668
to
681
| ### Protocol with abstract methods | ||
|
|
||
| ```py | ||
| from abc import abstractmethod | ||
| from typing import Protocol, final | ||
|
|
||
| class MyProtocol(Protocol): | ||
| @abstractmethod | ||
| def method(self) -> int: ... | ||
|
|
||
| @final | ||
| class Implementer(MyProtocol): # error: [unimplemented-abstract-method] | ||
| pass | ||
| ``` |
Member
There was a problem hiding this comment.
(this can be a followup): we also need to implement the rule where all methods on Protocol classes that have "trivial bodies" (either ..., pass, a docstring, or a combination of those three elements) and have an explicit non-None return type are treated as implicitly abstract even if they are not decorated with @abstractmethod
charliermarsh
force-pushed
the
charlie/final
branch
from
c2a575c to
977155e
Compare
charliermarsh
force-pushed
the
charlie/final
branch
from
977155e to
edb828c
Compare
AlexWaygood
approved these changes
Jan 21, 2026
This was referenced Jan 21, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Closes astral-sh/ty#2525.