Repository navigation
Combine the revealed types of multiple iteration steps in a more robust manner. - #19324
JukkaL merged 9 commits into
Conversation
Sorry, something went wrong.
for more information, see https://pre-commit.ci
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!
Sorry, something went wrong.
This comment has been minimized.
for more information, see https://pre-commit.ci
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
done
Sorry, something went wrong.
| 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).
Sorry, something went wrong.
There was a problem hiding this comment.
done
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
done
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Sorry, something went wrong.
|
Thanks for the reviews! I adjusted everything as suggested, except for the tiny difference of avoiding UnionType.make_union (as mentioned above). |
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.
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.