Uh oh!
There was an error while loading. Please reload this page.
Preserve narrowing in unreachable code - #20710
Conversation
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL
left a comment
There was a problem hiding this comment.
Not a full review, but I'm interested if this affects performance.
| # 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?
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 |
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.
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 I also recently landed several percentage point improvements, so I feel it is okay if I spend 0.3% :-) |
Uh oh!
There was an error while loading. Please reload this page.
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-unreachablewill be a very useful feature.Co-authored-by: A5rocks