Fix is_overlapping_types(NoneTyp, AnyType) for strict Optional checking

[benkuhn]

This fixes #2970. However, this is my first contribution to mypy, so I don't really know if this is the right place to fix it. (I'm mostly going off of following the control flow back from visit_assert_stmt and noticing that the comment near here seemed wrong.)

Note: I defaulted to a conservative change (only to the if experiments.STRICT_OPTIONAL branch of the function). However, I noticed that the function doesn't explicitly check whether either side is Any, instead leaving this path to be caught by the catch-all at the end. This seems like it could lead to other similar bugs in the future if people forgot about Any, and so an alternative fix might be to check if either argument is Any before doing anything else. I've left that change commented out so you can see what I'm talking about.

Let me know which is preferable! And sorry if I've missed something basic.

[gvanrossum]

This needs to be reviewed by @ddfisher.

[ddfisher]

Thanks for your contribution! Your alternative fix looks good to me -- it's theoretically correct, and it doesn't cause any problems when run over a large quantity of existing annotated code. That said, the fact that we didn't previously check for Any there gives me pause as well.

@JukkaL: do you know if there's any particular reason we don't currently check for Any in is_overlapping_types?

[JukkaL]

I suspect that the reason we don't check for Any is that the intention was to only report errors related to overlapping types when they don't involve Any -- if you use Any, mypy generally tries to do something reasonable but it should generally try not to complain about your code. Theoretically, whether any type is overlapping with Any is unknown, since Any doesn't mean "can be anything" but "mypy has no idea what this is".

Example: int should perhaps not be considered to be overlapping with Any, since Any can actually mean in this context "something vaguely string-like", which doesn't overlap with int.

However, it's possible that in some contexts we want a different definition of overlapping. What I described above could be called "known to overlap"; however, we might also want to determine whether two types "might overlap", and in the latter case Any would overlap with int. We shouldn't just blindly modify is_overlapping_types to use the latter definition unless we are certain that this is the right thing to do. It's plausible that we should have two different overlapping tests, but I don't know yet if that's the case here.

I can look at this more carefully if the above explanation doesn't help with this PR.

[benkuhn]

@JukkaL Ah, interesting!

It looks like, according to the docstring, is_overlapping_types says that its definition is, "Can a value of type t be a value of type s, or vice versa?" and that it's "based on runtime behavior." I don't really know what "based on runtime behavior" means, especially for Any--is that closer to the "known to overlap" definition you mentioned, the "might overlap" definition, or neither? Since it already erases type variables, it seems that the current implementation is defaulting to conservatism (in the sense of returning True more often). Another piece of evidence for this is the comment at the end saying "We conservatively assume that non-instance, non-union, and non-TypeType types can overlap any other types."

[JukkaL]

The docstring may not tell the whole story. Any is a very special type and often requires special logic.

Anyway, there seems to be no reason to make this change only for strict optional. If it's safe to do, it will be safe to do it for non-strict-optional code as well. There are only a small number of test cases that use strict optional checking so only having your change enabled for strict optional bypasses almost all tests, unfortunately. If doing it unconditionally causes test failures, it's likely that the change is not correct and we need to find a different fix. If tests still pass, it might be good, but I'd still like to have a look at all uses of is_overlapping_types before merging.

[JukkaL]

Note that even if is_overlapping_types is not fully implemented we'd want to move it be closer to spec rather than deviating further from spec. But I can't actually remember what it's supposed to do exactly so I'd need to do some research to determine that.

[JukkaL]

Okay I did some experimentation and your idea seems correct -- Any should overlap with everything. However, is_overlapping_types currently doesn't implement this logic consistently. Can you move the AnyType check to the beginning of the function? It shouldn't break any tests.

[benkuhn]

OK, done!

[ddfisher]

Thanks!