-
Notifications
You must be signed in to change notification settings - Fork 0
Warn when abstract or protocol type is assigned to a callable #10
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
Changes from all commits
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 | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2426,16 +2426,7 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Special case: only non-abstract non-protocol classes can be assigned to | ||||||||||||||||||||||||||||||
| # variables with explicit type Type[A], where A is protocol or abstract. | ||||||||||||||||||||||||||||||
| rvalue_type = get_proper_type(rvalue_type) | ||||||||||||||||||||||||||||||
| lvalue_type = get_proper_type(lvalue_type) | ||||||||||||||||||||||||||||||
| if (isinstance(rvalue_type, CallableType) and rvalue_type.is_type_obj() and | ||||||||||||||||||||||||||||||
| (rvalue_type.type_object().is_abstract or | ||||||||||||||||||||||||||||||
| rvalue_type.type_object().is_protocol) and | ||||||||||||||||||||||||||||||
| isinstance(lvalue_type, TypeType) and | ||||||||||||||||||||||||||||||
| isinstance(lvalue_type.item, Instance) and | ||||||||||||||||||||||||||||||
| (lvalue_type.item.type.is_abstract or | ||||||||||||||||||||||||||||||
| lvalue_type.item.type.is_protocol)): | ||||||||||||||||||||||||||||||
| self.msg.concrete_only_assign(lvalue_type, rvalue) | ||||||||||||||||||||||||||||||
| if not self.check_concrete_only_assign(lvalue_type, lvalue, rvalue_type, rvalue): | ||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||
| if rvalue_type and infer_lvalue_type and not isinstance(lvalue_type, PartialType): | ||||||||||||||||||||||||||||||
| # Don't use type binder for definitions of special forms, like named tuples. | ||||||||||||||||||||||||||||||
|
|
@@ -2453,6 +2444,46 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type | |||||||||||||||||||||||||||||
| self.infer_variable_type(inferred, lvalue, rvalue_type, rvalue) | ||||||||||||||||||||||||||||||
| self.check_assignment_to_slots(lvalue) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def check_concrete_only_assign(self, | ||||||||||||||||||||||||||||||
| lvalue_type: Optional[Type], | ||||||||||||||||||||||||||||||
| lvalue: Expression, | ||||||||||||||||||||||||||||||
| rvalue_type: Type, | ||||||||||||||||||||||||||||||
| rvalue: Expression) -> bool: | ||||||||||||||||||||||||||||||
| if (isinstance(lvalue, NameExpr) and isinstance(rvalue, NameExpr) | ||||||||||||||||||||||||||||||
| and lvalue.node == rvalue.node): | ||||||||||||||||||||||||||||||
| # This means that we reassign abstract class to itself. Like `A = A` | ||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| rvalue_type = get_proper_type(rvalue_type) | ||||||||||||||||||||||||||||||
| lvalue_type = get_proper_type(lvalue_type) | ||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||
|
Comment on lines
+2459
to
+2464
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. Redundant Boolean ExpressionsThe 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
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| lvalue_is_a_type = ( | ||||||||||||||||||||||||||||||
| isinstance(lvalue_type, TypeType) and | ||||||||||||||||||||||||||||||
| isinstance(lvalue_type.item, Instance) and | ||||||||||||||||||||||||||||||
| (lvalue_type.item.type.is_abstract or | ||||||||||||||||||||||||||||||
| lvalue_type.item.type.is_protocol) | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| lvalue_is_a_callable = False | ||||||||||||||||||||||||||||||
| if isinstance(lvalue_type, CallableType): | ||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
visz11 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if lvalue_is_a_type or lvalue_is_a_callable: | ||||||||||||||||||||||||||||||
|
Comment on lines
+2475
to
+2481
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. Callable Return ValidationThe 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.
Suggested change
Standards
|
||||||||||||||||||||||||||||||
| # `lvalue_type` here is either `TypeType` or `CallableType`: | ||||||||||||||||||||||||||||||
| self.msg.concrete_only_assign(cast(Type, lvalue_type), rvalue) | ||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||
|
Comment on lines
+2481
to
+2485
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. Type Confusion RiskUnchecked 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.
Suggested change
Standards
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # (type, operator) tuples for augmented assignments supported with partial types | ||||||||||||||||||||||||||||||
| partial_type_augmented_ops: Final = { | ||||||||||||||||||||||||||||||
| ('builtins.list', '+'), | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
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.
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