-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Reject @runtime_checkable on non-protocol classes #11646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -409,7 +409,15 @@ export function applyClassDecorator( | |
| } | ||
|
|
||
| if (FunctionType.isBuiltIn(decoratorType, 'runtime_checkable')) { | ||
| originalClassType.shared.flags |= ClassTypeFlags.RuntimeCheckable; | ||
| if (!ClassType.isProtocolClass(originalClassType)) { | ||
| evaluator.addDiagnostic( | ||
| DiagnosticRule.reportGeneralTypeIssues, | ||
| LocMessage.runtimeCheckableNotProtocol(), | ||
| decoratorNode.d.expr | ||
| ); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When vendoring this change into pyrx, mirror it in |
||
| } else { | ||
| originalClassType.shared.flags |= ClassTypeFlags.RuntimeCheckable; | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a lower decorator returns a non-runtime-checkable protocol, this validates the replacement type but sets |
||
| // Don't call getTypeOfDecorator for runtime_checkable. It appears | ||
| // frequently in stubs, and it's a waste of time to validate its | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # This sample tests that @runtime_checkable can be applied only to | ||
| # classes that are protocols (Protocol must appear in the base list). | ||
|
|
||
| from typing import Protocol, runtime_checkable | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class P1(Protocol): | ||
| def foo(self) -> int: ... | ||
|
|
||
|
|
||
| # This should generate an error because a subclass of a protocol is | ||
| # not itself a protocol unless Protocol is listed as a base class. | ||
| @runtime_checkable | ||
| class P2(P1): | ||
| def bar(self) -> str: ... | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class P3(P1, Protocol): | ||
| def bar(self) -> str: ... | ||
|
|
||
|
|
||
| # This should generate an error because C1 is not a protocol. | ||
| @runtime_checkable | ||
| class C1: | ||
| def foo(self) -> int: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -628,6 +628,12 @@ test('Protocol53', () => { | |
| TestUtils.validateResults(analysisResults2, 8); | ||
| }); | ||
|
|
||
| test('Protocol54', () => { | ||
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocol54.py']); | ||
|
|
||
| TestUtils.validateResults(analysisResults, 2); | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The aggregate diagnostic count can pass if diagnostics move from
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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] |
||
|
|
||
| test('ProtocolExplicit1', () => { | ||
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocolExplicit1.py']); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class decorators are applied bottom-up, but this validates
originalClassTyperather than the type produced by decorators below@runtime_checkable. A class-replacing decorator could therefore allow a sourceProtocoleven thoughruntime_checkablereceives a non-protocol class at runtime. Validate the currently decorated class and add a composition regression test.