-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix --strict-equality for iteratively visited code
#19635
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
99725c9
016eda6
2182e5d
edf63d7
6c3b489
ab4d567
9842064
1d69935
d3b9ebd
d1ba2c6
fc00f9c
b40c1e6
4e474cb
f741645
ee852fd
f00e86c
189f8b3
b4455ff
ef9f650
129ff47
02dfe62
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 |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| ErrorWatcher, | ||
| IterationDependentErrors, | ||
| IterationErrorWatcher, | ||
| NonOverlapErrorInfo, | ||
| ) | ||
| from mypy.nodes import ( | ||
| ARG_NAMED, | ||
|
|
@@ -1624,6 +1625,26 @@ def incompatible_typevar_value( | |
| ) | ||
|
|
||
| def dangerous_comparison(self, left: Type, right: Type, kind: str, ctx: Context) -> None: | ||
| # In loops (and similar cases), the same expression might be analysed multiple | ||
| # times and thereby confronted with different types. We only want to raise a | ||
| # `comparison-overlap` error if it occurs in all cases and therefore collect the | ||
| # respective types of the current iteration here so that we can report the error | ||
| # later if it is persistent over all iteration steps: | ||
| for watcher in self.errors.get_watchers(): | ||
| if watcher._filter: | ||
|
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. Could you move this logic into the watcher itself? This access to private followed by special-casing on watcher type is a bit painful to read, IMO this would be clearer as for watcher in self.errors.get_watchers():
if watcher.store_nonoverlapping_types(ctx, kind, left, right):
returnWhere
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. Ough, you really need both
Collaborator
Author
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. No need to excuse. I am obviously using things a little differently from how they were originally intended. Hence, I highly appreciate any thoughts on improving readability. |
||
| break | ||
| if isinstance(watcher, IterationErrorWatcher): | ||
| watcher.iteration_dependent_errors.nonoverlapping_types[-1][ | ||
| NonOverlapErrorInfo( | ||
| line=ctx.line, | ||
| column=ctx.column, | ||
| end_line=ctx.end_line, | ||
| end_column=ctx.end_column, | ||
| kind=kind, | ||
| ) | ||
| ] = (left, right) | ||
| return | ||
|
|
||
| left_str = "element" if kind == "container" else "left operand" | ||
| right_str = "container item" if kind == "container" else "right operand" | ||
| message = "Non-overlapping {} check ({} type: {}, {} type: {})" | ||
|
|
@@ -2513,8 +2534,11 @@ def match_statement_inexhaustive_match(self, typ: Type, context: Context) -> Non | |
| def iteration_dependent_errors(self, iter_errors: IterationDependentErrors) -> None: | ||
| for error_info in iter_errors.yield_uselessness_error_infos(): | ||
| self.fail(*error_info[:2], code=error_info[2]) | ||
| msu = mypy.typeops.make_simplified_union | ||
| for nonoverlaps, kind, context in iter_errors.yield_nonoverlapping_types(): | ||
| self.dangerous_comparison(msu(nonoverlaps[0]), msu(nonoverlaps[1]), kind, context) | ||
| for types, context in iter_errors.yield_revealed_type_infos(): | ||
| self.reveal_type(mypy.typeops.make_simplified_union(types), context) | ||
| self.reveal_type(msu(types), context) | ||
|
|
||
|
|
||
| def quote_type_string(type_string: str) -> str: | ||
|
|
||
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.
selectedis a strict subset ofnonoverlaps, right? How big is it usually, relative tononoverlapssize? If it normally includes a relatively small portion of those, it might be faster to swap these checks (iterate overselectedand doleft, right = nonoverlaps[candidate]for each).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.
selectedis only a subset of eachnonoverlapsitem if the relevant lines are reachable in all iterations. So, I am not really sure about the potential benefits of such a switch. However, maybe performance isn't overly critical here, since the loop's body is only executed if there is, in fact, an overlap issue to report?