Repository navigation
Improve the handling of "iteration dependent" errors and notes in finally clauses. - #19270
JukkaL merged 7 commits into
Conversation
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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.
JukkaL
left a comment
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.
Sorry, something went wrong.
|
|
||
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
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 IterationErrorWatcher.revealed_types might, in fact, be preferable. My initial idea is to define a method like Errors.get_current_iterationerrorwatcher, which could then be used by MessageBuilder.reveal_type to store the original type objects in IterationErrorWatcher.revealed_types instead of constructing a note that is filtered out anyway. Such an approach should be much cleaner and more efficient, I suppose. 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 unreachable functionality does not depend on it. |
Sorry, something went wrong.
|
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]
|
Sorry, something went wrong.
…st manner. (#19324) This PR fixes a regression introduced in #19118 and discussed in #19270. The combination of the revealed types of individual iteration steps now relies on collecting the original type objects instead of parts of preliminary `revealed_type` notes. As @JukkaL suspected, this approach is much more straightforward than introducing a sufficiently complete `revealed_type` note parser. Please note that I appended a commit that refactors already existing code. It is mainly code-moving, so I hope it does not complicate the review of this PR.
…ally clauses. (#19270) 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 name `IterationErrorWatcher`, to avoid code duplication. However, one duplication is left, which concerns error reporting. It would be nice and easy to move this functionality to `IterationErrorWatcher`, too, but this would result in import cycles, and I am unsure if working with `TYPE_CHECKING` and 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.
…st manner. (#19324) This PR fixes a regression introduced in #19118 and discussed in #19270. The combination of the revealed types of individual iteration steps now relies on collecting the original type objects instead of parts of preliminary `revealed_type` notes. As @JukkaL suspected, this approach is much more straightforward than introducing a sufficiently complete `revealed_type` note parser. Please note that I appended a commit that refactors already existing code. It is mainly code-moving, so I hope it does not complicate the review of this PR.
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 name IterationErrorWatcher, to avoid code duplication. However, one duplication is left, which concerns error reporting. It would be nice and easy to move this functionality to IterationErrorWatcher, too, but this would result in import cycles, and I am unsure if working with TYPE_CHECKING and 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.