Combine the revealed types of multiple iteration steps in a more robust manner. #19324
Conversation
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.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
mypy/errors.py
Outdated
| @@ -15,6 +15,8 @@ | |||
| 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.
mypy/errors.py
Outdated
| @@ -596,18 +585,20 @@ def _add_error_info(self, file: str, info: ErrorInfo) -> None: | |||
| if info.code in (IMPORT, IMPORT_UNTYPED, IMPORT_NOT_FOUND): | |||
| 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).
mypy/messages.py
Outdated
| 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.
mypy/errors.py
Outdated
| 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. ✅ |
|
Thanks for the reviews! I adjusted everything as suggested, except for the tiny difference of avoiding |
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.