-
Notifications
You must be signed in to change notification settings - Fork 0
Fix union inference of generic class and its generic type #5
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 all commits
b1d5b92
2314852
401eb22
133c49b
4929815
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -276,7 +276,11 @@ def infer_constraints_for_callable( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def infer_constraints( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template: Type, actual: Type, direction: int, skip_neg_op: bool = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template: Type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actual: Type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| direction: int, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| skip_neg_op: bool = False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| can_have_union_overlapping: bool = True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> list[Constraint]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Infer type constraints. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -316,11 +320,15 @@ def infer_constraints( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| res = _infer_constraints(template, actual, direction, skip_neg_op) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type_state.inferring.pop() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return res | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return _infer_constraints(template, actual, direction, skip_neg_op) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return _infer_constraints(template, actual, direction, skip_neg_op, can_have_union_overlapping) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _infer_constraints( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template: Type, actual: Type, direction: int, skip_neg_op: bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template: Type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actual: Type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| direction: int, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| skip_neg_op: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| can_have_union_overlapping: bool = True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> list[Constraint]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| orig_template = template | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template = get_proper_type(template) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -383,13 +391,46 @@ def _infer_constraints( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return res | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if direction == SUPERTYPE_OF and isinstance(actual, UnionType): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| res = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _can_have_overlapping(_item: Type, _actual: UnionType) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # There is a special overlapping case, where we have a Union of where two types | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # are the same, but one of them contains the other. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # For example, we have Union[Sequence[T], Sequence[Sequence[T]]] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # In this case, only the second one can have overlapping because it contains the other. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # So, in case of list[list[int]], second one would be chosen. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(p_item := get_proper_type(_item), Instance) and p_item.args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| other_items = [o_item for o_item in _actual.items if o_item is not a_item] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(other_items) == 1 and other_items[0] in p_item.args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+395
to
+406
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. NameError: undefined variable
- other_items = [o_item for o_item in _actual.items if o_item is not a_item]
+ other_items = [o_item for o_item in _actual.items if o_item is not _item]Without this fix the helper will raise at runtime and break constraint inference 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(other_items) > 1: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| union_args = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| p_arg | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for arg in p_item.args | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(p_arg := get_proper_type(arg), UnionType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for union_arg in union_args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if all(o_item in union_arg.items for o_item in other_items): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for a_item in actual.items: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
391
to
420
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. Potential Infinite Recursion Risk in Type Overlapping DetectionThe _can_have_overlapping function doesn't handle recursive type structures, which could lead to infinite recursion. If a type contains itself (directly or indirectly) in its type arguments, the function might enter an infinite loop when trying to determine if types overlap. This is particularly problematic for recursive generic types like linked lists or trees, where a type parameter might reference the containing type.
Suggested change
Rationale
References
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # `orig_template` has to be preserved intact in case it's recursive. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If we unwrapped ``type[...]`` previously, wrap the item back again, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # as ``type[...]`` can't be removed from `orig_template`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if type_type_unwrapped: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| a_item = TypeType.make_normalized(a_item) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| res.extend(infer_constraints(orig_template, a_item, direction)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| res.extend( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| infer_constraints( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| orig_template, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| a_item, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| direction, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| can_have_union_overlapping=_can_have_overlapping(a_item, actual), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return res | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Now the potential subtype is known not to be a Union or a type | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -410,10 +451,30 @@ def _infer_constraints( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # When the template is a union, we are okay with leaving some | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # type variables indeterminate. This helps with some special | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # cases, though this isn't very principled. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _is_item_overlapping_actual_type(_item: Type) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Overlapping occurs when we have a Union where two types are | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # compatible and the more generic one is chosen. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # For example, in Union[T, Sequence[T]], we have to choose | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Sequence[T] if actual type is list[int]. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This returns true if the item is an argument of other item | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # that is subtype of the actual type | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return any( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(p_item_to_compare := get_proper_type(item_to_compare), Instance) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and mypy.subtypes.is_subtype(actual, erase_typevars(p_item_to_compare)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and _item in p_item_to_compare.args | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for item_to_compare in template.items | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if _item is not item_to_compare | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
451
to
+467
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. Quadratic Complexity in Union Type Overlapping AnalysisThe implementation introduces a nested loop with O(n²) complexity when analyzing union types with overlapping type variables. For each item in a union type, the code iterates through all other items in the same union to check for overlapping relationships. This can become a performance bottleneck for complex unions with many type arguments, especially in large codebases with intricate type hierarchies.
Suggested change
Rationale
References
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = any_constraints( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| infer_constraints_if_possible(t_item, actual, direction) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for t_item in template.items | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for t_item in [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| item | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for item in template.items | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not (can_have_union_overlapping and _is_item_overlapping_actual_type(item)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| eager=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Missing Documentation for New Parameter
A new parameter 'can_have_union_overlapping' has been added to the infer_constraints function without any documentation explaining its purpose, behavior, or when it should be set to False. This lack of documentation makes the code harder to understand and maintain, potentially leading to incorrect usage of the parameter in the future. This violates ISO/IEC 25010 Maintainability requirements for proper documentation and understandability.
Rationale
References