Reject @runtime_checkable on non-protocol classes - #11646
Reject @runtime_checkable on non-protocol classes#11646Henry Su (hsusul) wants to merge 2 commits into
Conversation
CPython requires Protocol in the base list; a subclass of a protocol is a concrete class unless it also lists Protocol, and the decorator raises TypeError.
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
| DiagnosticRule.reportGeneralTypeIssues, | ||
| LocMessage.runtimeCheckableNotProtocol(), | ||
| decoratorNode.d.expr | ||
| ); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Class decorators are applied bottom-up, but this validates originalClassType rather than the type produced by decorators below @runtime_checkable. A class-replacing decorator could therefore allow a source Protocol even though runtime_checkable receives a non-protocol class at runtime. Validate the currently decorated class and add a composition regression test.
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocol54.py']); | ||
|
|
||
| TestUtils.validateResults(analysisResults, 2); | ||
| }); |
There was a problem hiding this comment.
Info · Optional note
The aggregate diagnostic count can pass if diagnostics move from P2 and C1 to either valid control. Assert diagnostic locations or identities so the test proves exactly the two invalid declarations are rejected.
| DiagnosticRule.reportGeneralTypeIssues, | ||
| LocMessage.runtimeCheckableNotProtocol(), | ||
| decoratorNode.d.expr | ||
| ); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
When vendoring this change into pyrx, mirror it in packages/pylance-internal/src/analyzer/decorators.ts, which still sets RuntimeCheckable unconditionally, and add async-mode regression coverage. Otherwise Pylance's async evaluator can retain the old behavior.
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
added
the
review-auto:approved
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocol54.py']); | ||
|
|
||
| TestUtils.validateResults(analysisResults, 2); | ||
| }); |
There was a problem hiding this comment.
Info · Optional note
This checks only the aggregate diagnostic count, so unrelated diagnostics could satisfy the test. Assert the diagnostic message and decorator ranges for both invalid classes if the harness supports it.
[verified]
Rich Chiodo (rchiodo)
added
review-auto:changes-requested
Class decorators are applied bottom-up, so check the class type that runtime_checkable actually receives rather than the original class, and assert the reported diagnostic locations in the sample test.
| } else { | ||
| originalClassType.shared.flags |= ClassTypeFlags.RuntimeCheckable; | ||
| } | ||
|
|
There was a problem hiding this comment.
Warning · Non-blocking recommendation
When a lower decorator returns a non-runtime-checkable protocol, this validates the replacement type but sets RuntimeCheckable on the discarded original class. Set the flag on the validated decorated class and add coverage using a replacement protocol that is not already runtime-checkable.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
2 participants
Summary
@runtime_checkablemay be applied only to protocol classes. A subclass of a protocol is a concrete class unlessProtocolis listed as a base, and CPython raisesTypeErrorfor the decorator in that case.Test plan
npx jest typeEvaluator7.test.ts -t 'Protocol54|Protocol7' --forceExit(frompackages/pyright-internal)@runtime_checkable class Child(ParentProtocol)raisesTypeError;class Child(ParentProtocol, Protocol)succeeds