Uh oh!
There was an error while loading. Please reload this page.
Combine the revealed types of multiple iteration steps in a more robust manner. - #19324
Conversation
This comment has been minimized.
This comment has been minimized.
CoolCat467
left a comment
There was a problem hiding this comment.
Can't say I completely understand the code, but functionality shown from the test changes looks great!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for fixing error reporting! Left a few comments about style/structural issues, but otherwise looks good.
| from mypy.nodes import Context | ||
| from mypy.options import Options | ||
| from mypy.scope import Scope | ||
| from mypy.typeops import make_simplified_union |
There was a problem hiding this comment.
This causes a problematic import dependency and makes import cycles worse in the mypy codebase. You should be able to give the responsibility for calling this to the caller, or move the relevant code to another module.
| self.seen_import_error = True | ||
| @property | ||
| def watchers(self) -> Iterator[ErrorWatcher]: |
There was a problem hiding this comment.
Style nit: It's a bit unusual to have a generator that is a property. I think it would be better to make this a normal method. It may also help performance when compiled with mypyc (assuming this is a performance critical).
| for error_info in iter_errors.yield_uselessness_error_infos(): | ||
| self.fail(*error_info[:2], code=error_info[2]) | ||
| for note_info, context in iter_errors.yield_revealed_type_infos(): | ||
| self.reveal_type(note_info, context) |
There was a problem hiding this comment.
It's reasonable to perform the make_simplified_union here, since mypy.messages already depends on type ops.
| context = Context(line=note_info[0], column=note_info[1]) | ||
| context.end_line = note_info[2] | ||
| context.end_column = note_info[3] | ||
| yield make_simplified_union(types), context |
There was a problem hiding this comment.
Call UnionType.make_union here to avoid make_simplified_union dependency here.
There was a problem hiding this comment.
This seems unnecessary, as we now use make_simplified_union elsewhere, so I decided to return the original list of types here simply. I hope that's okay.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
tyralla
commented
Jul 4, 2025
Thanks for the reviews! I adjusted everything as suggested, except for the tiny difference of avoiding |
Uh oh!
There was an error while loading. Please reload this page.
…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.
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_typenotes. As @JukkaL suspected, this approach is much more straightforward than introducing a sufficiently completerevealed_typenote 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.