Make isinstance/issubclass generate ad-hoc intersections #8305
Conversation
|
Not really sure what's up with the tests though -- they seem to be crashing for unrelated reasons? |
|
@Michael0x2a I think #8308 fixes the failing tests. |
This diff makes `isinstance(...)` and `issubclass(...)` try
generating ad-hoc intersections of Instances when possible.
For example, we previously concluded the if-branch is unreachable
in the following program. This PR makes mypy infer an ad-hoc
intersection instead.
class A: pass
class B: pass
x: A
if isinstance(x, B):
reveal_type(x) # N: Revealed type is 'test.<subclass of "A" and "B">'
If you try doing an `isinstance(...)` that legitimately is impossible
due to conflicting method signatures or MRO issues, we continue to
declare the branch unreachable. Passing in the `--warn-unreachable`
flag will now also report an error about this:
# flags: --warn-unreachable
x: str
# E: Subclass of "str" and "bytes" cannot exist: would have
# incompatible method signatures
if isinstance(x, bytes):
reveal_type(x) # E: Statement is unreachable
This error message has the same limitations as the other
`--warn-unreachable` ones: we suppress them if the isinstance check
is inside a function using TypeVars with multiple values.
However, we *do* end up always inferring an intersection type when
possible -- that logic is never suppressed.
I initially thought we might have to suppress the new logic as well
(see python#3603 (comment)),
but it turns out this is a non-issue in practice once you add in
the check that disallows impossible intersections.
For example, when I tried running this PR on the larger of our two
internal codebases, I found about 25 distinct errors, all of which
were legitimate and unrelated to the problem discussed in the PR.
(And if we don't suppress the extra error message, we get about
100-120 errors, mostly due to tests repeatdly doing `result = blah()`
followed by `assert isinstance(result, X)` where X keeps changing.)
Michael0x2a
force-pushed
the
add-ad-hoc-intersections-v2
branch
from
658150d
to
f0d83f0
Compare
|
A quick question before I will do a full review: last time I was thinking about this my main worry was about how the fake intersections would interact with incremental mode. Did you try this in incremental mode? Maybe add few tests where generated |
|
I'm also interested in how this interacts with mypyc---though making it work with mypyc I don't think is actually a requirement to merge this, but would be great to tackle if you are interested. (These intersection issues can cause runtime errors under mypyc, which is very frustrating.) |
|
Ok, I added some incremental mode tests. Just as a disclaimer though, I'm pretty rusty on anything to do with incremental mode, so I'm not actually sure if the new tests are high quality. In particular:
If it's any consolation, I'm creating the ad-hoc intersection using the same code we were previously using in I can try looking into adding support for this in mypyc, sure. |
|
I know that Update: yeah the consistency checker is now completely broken |
|
I'm hacking on the consistency checker to get it back into shape and while there are some failures it looks like none of them are related to the |
This fixes some stuff in the mergechecker that breaks with current mypy's and also some bugs that the mergechecker caught. The entire fine-grained test suite now passes with the merge checker on, which I don't think has ever been true before. This means that we could turn it on by default, but it doubles the runtime of the fine-grained tests (from 90s CPU time to 180s CPU time on my laptop), so I've left it off for now. The motivation here is that I knew intersect_callable's creation of types during typechecking used to run afoul of the consistency checker and so I was nervous that #8305 would cause more problems by adding more logic of that kind. It no longer does, probably as a result of the semantic analyzer rewrite, so I think we are in the clear on that.
| type_ranges: Optional[List[TypeRange]], | ||
| ) -> Tuple[TypeMap, TypeMap]: | ||
| # For some reason, doing "yes_map, no_map = conditional_type_maps(...)" | ||
| # doesn't work: mypyc will decide that 'yes_map' is of type None if we try. |
This fixes some stuff in the mergechecker that breaks with current mypy's and also some bugs that the mergechecker caught. The entire fine-grained test suite now passes with the merge checker on, which I don't think has ever been true before. This means that we could turn it on by default, but it doubles the runtime of the fine-grained tests (from 90s CPU time to 180s CPU time on my laptop), so I've left it off for now. The motivation here is that I knew intersect_callable's creation of types during typechecking used to run afoul of the consistency checker and so I was nervous that #8305 would cause more problems by adding more logic of that kind. It no longer does, probably as a result of the semantic analyzer rewrite, so I think we are in the clear on that.
This diff makes
isinstance(...)andissubclass(...)try generating ad-hoc intersections of Instances when possible.For example, we previously concluded the if-branch is unreachable in the following program. This PR makes mypy infer an ad-hoc intersection instead.
If you try doing an
isinstance(...)that legitimately is impossible due to conflicting method signatures or MRO issues, we continue to declare the branch unreachable. Passing in the--warn-unreachableflag will now also report an error about this:This error message has the same limitations as the other
--warn-unreachableones: we suppress them if the isinstance check is inside a function using TypeVars with multiple values.However, we do end up always inferring an intersection type when possible -- that logic is never suppressed.
I initially thought we might have to suppress the new logic as well (see #3603 (comment)), but it turns out this is a non-issue in practice once you add in the check that disallows impossible intersections.
For example, when I tried running this PR on the larger of our two internal codebases, I found about 25 distinct errors, all of which were legitimate and unrelated to the problem discussed in the PR.
(And if we don't suppress the extra error message, we get about 100-120 errors, mostly due to tests repeatdly doing
result = blah()followed byassert isinstance(result, X)where X keeps changing.)