Report access to abstract static and class methods on the class - #21806
Open
Endika wants to merge 1 commit into
Open
Report access to abstract static and class methods on the class#21806Endika wants to merge 1 commit into
Endika wants to merge 1 commit into
Conversation
Contributor
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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
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.
Fixes #14939.
This follows the implementation @JukkaL outlined on the issue: report accessing
an abstract static method, and a class method too, on a direct reference to the
type object, but not through
type[T], since there the runtime class may be asubclass which implements it. That is the same distinction the existing
instantiation check makes with
not callee.from_type_type.What is and is not reported
Foo.s(),Foo.c()on the abstract class itselfFoo.swithout calling ittype[Foo]Foo.mInstance methods are excluded for the reason
type[T]is: whatever is passed asselfmay implement the method. The check hangs offanalyze_type_callable_member_access, which is only reached for a direct classreference —
analyze_type_type_member_accesshandlestype[T]and is leftalone. It reuses the existing
[abstract]error code.Tests
Two cases in
check-abstract.test, both of which fail on master:testAccessAbstractStaticAndClassMethodOnClass— the static and class methodare reported on the class and on a bare reference, and not through
type[A],on an instance, or for an unbound instance method.
testAccessAbstractStaticAndClassMethodOnSubclass— a subclass implementingboth is silent, a subclass which implements only one is reported for the other
and named in the message.
I checked what the tests actually catch by breaking each exclusion in turn:
OnClasstype[T]path left aloneOnClassabstract_attributesconsultedOnSubclassThe first of those initially had no coverage: the test called the method on an
instance, which never reaches this code at all. Accessing the unbound method
through the class does, so
g = A.mwas added.Full suites locally on Python 3.14 / Linux, uncompiled:
testcheck8202 passed,testfinegrained/testmerge/testtransform/testdeps/testpythoneval1453 passed. Self-check clean.
Performance
The check runs on class attribute access, so it returns immediately unless the
class is abstract. Self-check with a cold cache, three runs each, uncompiled:
The ranges overlap, so I read this as noise, but the medians do put it 1% on the
slow side and I have not measured a compiled build.
Notes for review
the right shape. A diff there is not automatically a bug in this PR — it may be
exactly what the issue asks to catch — but I will work through anything it
reports rather than assume.
classmethods should not be able to be called on abstract classes #14062.@JelleZijlstra distinguished the two:
classmethods should not be able to be called on abstract classes #14062 was about non-abstractstatic/classmethods on abstract classes, whereas this is about abstract ones.
I have taken the issue staying open and labelled
good-second-issueas theresolution of that, but say if you would rather revisit it.
LLM disclosure
Per the contributing guidelines: this PR text was written with LLM assistance, and I am a
first-time contributor here, so I understand that may weigh against it — say so
and I will close it without any fuss. The design is @JukkaL's from the issue
rather than mine, and I can explain and defend every decision above.
One thing worth recording, since it came out of verification rather than the
first draft: the check first tested
isinstance(func, FuncBase), and self-checkcorrectly flagged everything after it as unreachable.
FuncBaseis not aSymbolNode—FuncDefreaches it throughFuncItem— so the narrowing gaveNever.nodes.pysays as much right above the class and points atSYMBOL_FUNCBASE_TYPES, which is what the code uses now.