Warn when abstract or protocol type is assigned to a callable #10
Conversation
|
Diff from mypy_primer, showing the effect of this PR on open source code: ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/param_scheduler.py:44: error: Can only assign concrete classes to a variable of type "Type[ParamScheduler]" [misc]
+ ignite/contrib/handlers/param_scheduler.py:46: error: Can only assign concrete classes to a variable of type "Type[CyclicalScheduler]" [misc]
|
|
This is interesting 🤔 from ignite.handlers.param_scheduler import (
ParamScheduler,
)
ParamScheduler = ParamSchedulerWhere I think that this should be fine. It does not look like an issue to me. I am also ok if others are fine with ignoring it. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/param_scheduler.py:44: error: Can only assign concrete classes to a variable of type "Type[ParamScheduler]" [misc]
+ ignite/contrib/handlers/param_scheduler.py:46: error: Can only assign concrete classes to a variable of type "Type[CyclicalScheduler]" [misc]
|
Yeah, I think that this should be allowed. Maybe check if the lvalue and rvalue refer to the same node, and skip the concreteness check in this case? The assignment will be a no-op. |
WalkthroughThis change refactors the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant TypeChecker as TypeChecker
participant check_concrete_only_assign as check_concrete_only_assign
User->>TypeChecker: check_assignment(lvalue_type, lvalue, rvalue_type, rvalue)
TypeChecker->>check_concrete_only_assign: check_concrete_only_assign(lvalue_type, lvalue, rvalue_type, rvalue)
check_concrete_only_assign-->>TypeChecker: (True/False)
alt Assignment not allowed
TypeChecker-->>User: Return early (error emitted)
else Assignment allowed
TypeChecker->>User: Continue with assignment check
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Done! |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue python#13171 by adding a warning when an abstract or protocol type is assigned to a callable. The changes modify mypy/checker.py to include a check that emits an error when this type of assignment occurs. Additionally, new test cases are added to test-data/unit/check-abstract.test and test-data/unit/check-protocols.test to verify the new warning.
Highlights
- New Warning: Introduces a warning when assigning an abstract or protocol type to a callable, mirroring the behavior of
Type[AbstractOrProtocol]. - Code Refactoring: Refactors the
check_assignmentfunction inmypy/checker.pyto use a new helper functioncheck_concrete_only_assignfor checking concrete assignments. - New Test Cases: Adds new test cases to
test-data/unit/check-abstract.testandtest-data/unit/check-protocols.testto cover the new warning scenario.
Changelog
- mypy/checker.py
- Refactors
check_assignmentto extract the logic for checking concrete-only assignments into a new functioncheck_concrete_only_assign. - Adds a check within
check_concrete_only_assignto warn when an abstract or protocol type is assigned to a callable. - Adds a condition to allow reassigning an abstract class to itself (e.g.,
A = A).
- Refactors
- test-data/unit/check-abstract.test
- Adds a new test case
testInstantiationAbstractsWithCallablesto verify the warning for abstract types assigned to callables.
- Adds a new test case
- test-data/unit/check-protocols.test
- Adds a new test case
testInstantiationProtocolWithCallablesto verify the warning for protocol types assigned to callables.
- Adds a new test case
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A callable's plight,
Abstract types cause a fright,
Concrete's the key,
For functions to be,
Error's gone, code shines bright.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a new check in mypy to warn when an abstract or protocol type is assigned to a callable, aligning the semantics with Type[AbstractOrProtocol]. The changes include modifications to mypy/checker.py and additions to test-data/unit/check-abstract.test and test-data/unit/check-protocols.test to incorporate the new functionality and test cases. Overall, the changes seem well-structured and address the intended issue.
Summary of Findings
- Code Duplication: The logic for checking concrete-only assignments is duplicated within the
check_assignmentmethod. Extracting this logic into a separate, reusable function would improve maintainability and reduce redundancy. - Clarity of Conditionals: The conditional logic in
check_concrete_only_assigncould be simplified for better readability. Consider using intermediate variables with descriptive names to clarify the conditions being checked. - Missing Error Code: The error message
concrete_only_assignincheck_concrete_only_assignis missing an associated error code. Adding an error code would improve the ability to filter and manage these errors.
Merge Readiness
The pull request introduces a valuable new check. However, addressing the identified issues related to code duplication, conditional clarity, and the missing error code would further enhance the quality and maintainability of the code. I recommend addressing these points before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Security Review: Type Assignment👍 Well Done
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| if lvalue_is_a_type or lvalue_is_a_callable: | ||
| # `lvalue_type` here is either `TypeType` or `CallableType`: | ||
| self.msg.concrete_only_assign(cast(Type, lvalue_type), rvalue) | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Type Confusion Risk
Unchecked type cast could lead to runtime errors if lvalue_type is None. If None is passed to cast(), a TypeError would be raised, potentially causing denial of service.
| if lvalue_is_a_type or lvalue_is_a_callable: | |
| # `lvalue_type` here is either `TypeType` or `CallableType`: | |
| self.msg.concrete_only_assign(cast(Type, lvalue_type), rvalue) | |
| return False | |
| return True | |
| if lvalue_is_a_type or lvalue_is_a_callable: | |
| # `lvalue_type` here is either `TypeType` or `CallableType`: | |
| if lvalue_type is not None: | |
| self.msg.concrete_only_assign(cast(Type, lvalue_type), rvalue) | |
| return False | |
| return True |
Standards
- CWE-476
- CWE-704
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Type Checking for Abstract/Protocol Assignments👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| ret_type = get_proper_type(lvalue_type.ret_type) | ||
| lvalue_is_a_callable = ( | ||
| isinstance(ret_type, Instance) and | ||
| (ret_type.type.is_abstract or ret_type.type.is_protocol) | ||
| ) | ||
|
|
||
| if lvalue_is_a_type or lvalue_is_a_callable: |
There was a problem hiding this comment.
Callable Return Validation
The code only checks if the return type is an Instance, but misses other possible abstract or protocol types that could be wrapped in other type constructs. This creates a logical gap where abstract/protocol types in more complex return type structures won't trigger the warning.
| ret_type = get_proper_type(lvalue_type.ret_type) | |
| lvalue_is_a_callable = ( | |
| isinstance(ret_type, Instance) and | |
| (ret_type.type.is_abstract or ret_type.type.is_protocol) | |
| ) | |
| if lvalue_is_a_type or lvalue_is_a_callable: | |
| lvalue_is_a_callable = False | |
| if isinstance(lvalue_type, CallableType): | |
| ret_type = get_proper_type(lvalue_type.ret_type) | |
| if isinstance(ret_type, Instance): | |
| lvalue_is_a_callable = (ret_type.type.is_abstract or ret_type.type.is_protocol) | |
| elif isinstance(ret_type, TypeType) and isinstance(ret_type.item, Instance): | |
| lvalue_is_a_callable = (ret_type.item.type.is_abstract or ret_type.item.type.is_protocol) |
Standards
- Algorithm-Correctness-Pattern-Matching
- Logic-Verification-Type-Checking
| if not ( | ||
| isinstance(rvalue_type, CallableType) and | ||
| rvalue_type.is_type_obj() and | ||
| (rvalue_type.type_object().is_abstract or | ||
| rvalue_type.type_object().is_protocol)): | ||
| return True |
There was a problem hiding this comment.
Incomplete Type Check
The check only verifies rvalue_type properties but doesn't verify that lvalue_type is a callable type before proceeding to further checks. This could cause AttributeError if lvalue_type is None or a non-callable type, reducing reliability.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| if not ( | ||
| isinstance(rvalue_type, CallableType) and | ||
| rvalue_type.is_type_obj() and | ||
| (rvalue_type.type_object().is_abstract or | ||
| rvalue_type.type_object().is_protocol)): | ||
| return True |
There was a problem hiding this comment.
Redundant Boolean Expressions
The boolean expression is structured as 'if not (condition)' followed by 'return True', which creates unnecessary cognitive load. Inverting the condition and returning False directly would improve readability and maintainability.
Standards
- Clean-Code-Boolean-Logic
- Maintainability-Quality-Readability
Now
Callable[[], AbstractOrProtocol]has the same semantics asType[AbstractOrProtocol].I don't think we need to test for
@overloadhere, because you cannot have inline@overloadannotation.Closes python#13171
Summary by CodeRabbit
Refactor
Tests