Repository navigation
Preserve narrowing in unreachable code - #20710
hauntsaninja merged 1 commit into
Conversation
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.
JukkaL
left a comment
There was a problem hiding this comment.
Not a full review, but I'm interested if this affects performance.
Sorry, something went wrong.
|
|
||
| # The second pass narrows down the types and type checks bodies. | ||
| unmatched_types: TypeMap = None | ||
| unmatched_types: TypeMap = {s.subject: UninhabitedType()} |
There was a problem hiding this comment.
Now we construct some extra temporary dictionaries and UninhabitedType objects. Can you measure if this has measurable performance impact?
Sorry, something went wrong.
|
There is maybe some perf impact, something like 0.3% on self check I think it's well worth the cost. Not checking unreachable code is one of the most surprising mypy behaviours and it's a thing that makes accurately improving narrowing right now quite scary (see e.g. #20660 (comment) or fixing #12535 ) I also do think I can maybe claw some of it back in future refactors |
Sorry, something went wrong.
A5rocks
left a comment
There was a problem hiding this comment.
Makes sense!
If perf really matters, maybe we can replace the dictionaries with an associative list or something like that. Or even a 2-tuple if a function is guaranteed to return at most 1 result! But IMO it's worth it to take a slight perf hit in order to provide new functionality.
Sorry, something went wrong.
|
Yes, I was experimenting with 2-tuples and this is promising (but a larger change). Esp since we actually probably want to switch to combining keyed by literal_hash anyway... I also recently landed several percentage point improvements, so I feel it is okay if I spend 0.3% :-) |
Sorry, something went wrong.
In the future, it would be nice to have an option to continue to check unreachable code. See #18707 from A5rocks. This PR lays some semantics preserving groundwork for that.
This is effectively a subset of #18707 , but I go just a little bit further and change the type of TypeMap to exclude None
Btw A5rocks if you are willing to rebase your PR, that would be great. I think --check-unreachable will be a very useful feature.
Co-authored-by: A5rocks