Improve the handling of "iteration dependent" errors and notes in finally clauses. #19270
Conversation
for more information, see https://pre-commit.ci
|
Oh, constrained type variables are indeed a problem, and I just realised this was very recently reported in #19256. |
tyralla
changed the title
This comment has been minimized.
This comment has been minimized.
# Conflicts: # mypy/checker.py
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks for the PR! Looks good overall, but I noticed some issues with how the union types are parsed and constructed. Can you fix the syntax of generated union types in this PR, since the fix seems simple? The error message parsing related issues are best fixed in a separate PR.
mypy/errors.py
Outdated
|
|
||
| for note_info, types in self.iteration_dependent_errors.revealed_types.items(): | ||
| sorted_ = sorted(types, key=lambda typ: typ.lower()) | ||
| revealed = sorted_[0] if len(types) == 1 else f"Union[{', '.join(sorted_)}]" |
There was a problem hiding this comment.
Use | union syntax if targeting 3.10 or later. Can you reuse the functionality already in MessageBuilder.reveal_type to have the type in the correct format?
There was a problem hiding this comment.
I see no direct way to use MessageBuilder.reveal_type/TypeStrVisitor here (for now, but see my general comment below), because they expect types instead of strings, and the types are (currently) not available here. So I just extended the current logic by using Options.use_or_syntax.
| (info.code, info.message, info.line, info.column, info.end_line, info.end_column) | ||
| ) | ||
| if info.code == codes.UNREACHABLE: | ||
| self.unreachable_lines.update(range(info.line, info.end_line + 1)) | ||
| iter_errors.unreachable_lines[-1].update(range(info.line, info.end_line + 1)) | ||
| return True | ||
|
|
||
| if info.code == codes.MISC and info.message.startswith("Revealed type is "): | ||
| key = info.function_or_member, info.line, info.column, info.end_line, info.end_column | ||
| types = info.message.split('"')[1] |
There was a problem hiding this comment.
This logic seems unreliable -- consider code like reveal_type('"\''). A better approach would be to keep anything between the first and last double quotes. Since this issue is pre-existing, it's best to fix it in a separate PR.
| return True | ||
|
|
||
| if info.code == codes.MISC and info.message.startswith("Revealed type is "): | ||
| key = info.function_or_member, info.line, info.column, info.end_line, info.end_column | ||
| types = info.message.split('"')[1] | ||
| if types.startswith("Union["): | ||
| self.revealed_types[key].update(types[6:-1].split(", ")) | ||
| iter_errors.revealed_types[key].update(types[6:-1].split(", ")) |
There was a problem hiding this comment.
Here's another issue, but since it's pre-existing, it's better to fix in a separate PR: I don't think that the split operation here works reliably -- consider a type like Union[dict[int, str], dict[str, int]], but there are other cases as well, such as nested unions, and unions using the new-style syntax (when targeting 3.10 or later). A correct way to do this would have to be able to skip string literals and keep track of the nesting of parentheses and square brackets at least. It may be easier to keep the original type object.
|
Thank you for the review, @JukkaL! It's great that you've found the existing limitations of my current "revealed type recombination" approach. Besides the simple fix requested for this PR, I played a little and tend to think that storing the original type objects in I could work on it soon, and it would be good to have this fixed in 1.17 as well. If you think this is not possible, I could also temporarily disable the "revealed type recombination" approach, as the (in my opinion, more relevant) new |
|
Diff from mypy_primer, showing the effect of this PR on open source code: xarray (https://github.com/pydata/xarray)
+ xarray/backends/api.py:1943: error: Unused "type: ignore" comment [unused-ignore]
|
Fixes #19269
This PR refactors the logic implemented in #19118 (which only targeted repeatedly checked loops) and applies it to repeatedly checked finally clauses.
I moved nearly all relevant code to the class
LoopErrorWatcher, which now has the more general nameIterationErrorWatcher, to avoid code duplication. However, one duplication is left, which concerns error reporting. It would be nice and easy to move this functionality toIterationErrorWatcher, too, but this would result in import cycles, and I am unsure if working withTYPE_CHECKINGand postponed importing is acceptable in such cases (both for Mypy and Mypyc).After the refactoring, it should not be much effort to apply the logic to other cases where code sections are analysed iteratively. However, the only thing that comes to my mind is the repeated checking of functions with arguments that contain constrained type variables. I will check it. If anyone finds a similar case and the solution is as simple as expected, we could add the fix to this PR, of course.