-
Notifications
You must be signed in to change notification settings - Fork 0
Clone issue 13171 #13
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: master
Are you sure you want to change the base?
Changes from 4 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -100,10 +100,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FineGrainedDeferredNodeType: _TypeAlias = Union[FuncDef, MypyFile, OverloadedFuncDef] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # A node which is postponed to be processed during the next pass. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # In normal mode one can defer functions and methods (also decorated and/or overloaded) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # and lambda expressions. Nested functions can't be deferred -- only top-level functions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # and methods of classes not defined within a function can be deferred. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class DeferredNode(NamedTuple): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| node: DeferredNodeType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # And its TypeInfo (for semantic analysis self type handling | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2426,16 +2423,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 +2441,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: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2444
to
+2448
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. Parameter Organization InconsistencyParameter ordering mixes types and expressions inconsistently reducing method signature readability. Grouping related parameters together would improve comprehension and follow consistent organization patterns. Current ordering makes parameter relationships unclear. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2449
to
+2451
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. Self Assignment DetectionSelf-assignment detection relies on node identity comparison which may not handle all edge cases like aliased references or complex expressions. While the current implementation prevents false positives for direct self-assignment, it could miss security-relevant reassignments through aliases. Consider expanding validation to handle indirect references that could bypass abstract class assignment restrictions. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rvalue_type = get_proper_type(rvalue_type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lvalue_type = get_proper_type(lvalue_type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
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 Type ProcessingDuplicate get_proper_type() calls on same variables that were already processed at lines 2429. This creates redundant type processing overhead in assignment checking hot path. The refactored method performs the same operations twice for identical inputs. Standards
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. Code Duplication OpportunityThe get_proper_type() calls are duplicated from the original code that was refactored. This creates maintenance overhead where the same type processing logic exists in both locations. Consider extracting this common pattern into a shared utility method to follow DRY principles. Standards
visz11 marked this conversation as resolved.
Outdated
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 Type ProcessingDuplicate get_proper_type() calls violate organization guideline requiring code reuse and redundancy avoidance. The same type processing operations were already performed at lines 2426 in the original code. This creates unnecessary computational overhead in the assignment checking hot path and maintenance burden through code duplication. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2472
to
+2478
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 block can be slightly refactored to improve readability by nesting the
Suggested change
Comment on lines
+2472
to
+2478
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 Logic EnhancementCallable validation logic only checks return type for abstract/protocol status but ignores callable parameter types. Complete callable validation should verify all parameter types for abstract/protocol constraints. This creates incomplete validation coverage for callable type assignments. Standards
Comment on lines
+2472
to
+2478
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. Incomplete Callable ValidationCallable validation logic only examines return type for abstract/protocol constraints but ignores parameter types. Complete validation requires checking all parameter types for abstract/protocol status. This creates logical gaps where callable assignments with abstract parameter types bypass validation checks. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2472
to
+2479
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. Incomplete Validation LogicCallable validation only checks return type for abstract/protocol status but ignores parameter types. Complete validation should verify all parameter types for abstract/protocol constraints. This creates incomplete coverage for callable type assignments. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 Cast SafetyType cast to Type without runtime validation could cause ClassCastException if lvalue_type is not actually a Type instance. The cast assumes type safety based on conditional checks but doesn't handle edge cases where type system assumptions fail. Adding runtime type validation before cast would prevent potential runtime failures. Standards
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. Unsafe Type CastType cast to Type without runtime validation assumes type safety based on conditional checks but doesn't handle edge cases where type system assumptions fail. The cast could cause runtime failures if lvalue_type is not actually a Type instance despite passing the conditional guards. Standards
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. Unsafe Type CastType cast to Type without runtime validation assumes type safety based on conditional checks but doesn't handle edge cases where type system assumptions fail. The cast could cause runtime failures if lvalue_type is not actually a Type instance despite passing conditional guards. Commitable Suggestion
Suggested change
Standards
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. Unsafe Type CastType cast to Type without runtime validation assumes type safety based on conditional checks. The cast could cause runtime failures if lvalue_type is not actually a Type instance despite passing conditional guards. Logic assumes perfect type system alignment without defensive validation. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2444
to
+2484
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. Restore alias handling before concrete-only enforcement Factoring the concrete-only check into from typing import Type, TypeAlias
from abc import ABC
class Base(ABC): ...
Concrete = type("Concrete", (Base,), {})
AbstractType: TypeAlias = Type[Base]
x: AbstractType = Base # used to error, now passesTo keep the previous behaviour, normalize both sides after the PartialType guard so aliases (and other wrappers) are unwrapped before the if isinstance(lvalue_type, PartialType):
return True
- 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)):
+ proper_rvalue_type = get_proper_type(rvalue_type)
+ if not (
+ isinstance(proper_rvalue_type, CallableType) and
+ proper_rvalue_type.is_type_obj() and
+ (proper_rvalue_type.type_object().is_abstract or
+ proper_rvalue_type.type_object().is_protocol)):
return True
- 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)
+ proper_lvalue_type = get_proper_type(lvalue_type)
+
+ lvalue_is_a_type = (
+ isinstance(proper_lvalue_type, TypeType) and
+ isinstance(proper_lvalue_type.item, Instance) and
+ (proper_lvalue_type.item.type.is_abstract or
+ proper_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)
+ if isinstance(proper_lvalue_type, CallableType):
+ ret_type = get_proper_type(proper_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_type` here is either `TypeType` or `CallableType`:
- self.msg.concrete_only_assign(cast(Type, lvalue_type), rvalue)
+ self.msg.concrete_only_assign(cast(Type, proper_lvalue_type), rvalue)
return FalseThat reinstates the original guard for alias-backed annotations. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # (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.
Method Parameter Ordering
Parameter ordering mixes types and expressions inconsistently (lvalue_type, lvalue, rvalue_type, rvalue). Grouping related parameters together (types together, expressions together) would improve method signature readability and follow consistent parameter organization patterns.
Standards