Fix nested overload merging

[cdce8p]

Description

from typing import overload

class A: ...
class B: ...
class C: ...
class D: ...

@overload
def f1(g: A) -> A: ...
if True:
    @overload
    def f1(g: B) -> B: ...
    if True:
        @overload
        def f1(g: C) -> C: ...
        @overload
        def f1(g: D) -> D: ...
def f1(g): ...

Closes #12606

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[cdce8p]

@JelleZijlstra I believe you reviewed my original PR which added conditional overloads (#10712). Would you be able to take a look at this one, too? I found this issue after you mentioned on typing-sig that performance for a lot of overloads will be slower in Python 3.11. Since I don't need runtime introspection, I figured wrapping the overloads in a TYPE_CHECKING block should do the trick.

[AlexWaygood]

Since I don't need runtime introspection, I figured wrapping the overloads in a TYPE_CHECKING block should do the trick.

You can also do this to get round the performance penalty, if it's an issue (though personally, I'd check whether it is before complicating my code 😉):

if TYPE_CHECKING:
    from typing import overload
else:
    def overload(func): return func

Also I think we managed to get the performance penalty down a bit after that email to typing-sig. Can't remember what the final numbers were.

[JelleZijlstra]

I added the PR to my requested reviews, hopefully I'll get to it later today.

Also I think we managed to get the performance penalty down a bit after that email to typing-sig. Can't remember what the final numbers were.

Yes, I think it was about 4x. Also note that that number is purely for the overload() call itself. The cost of creating the function object for each of those overloads is probably also significant at this scale, and you pay that regardless of whether you use runtime overload registration.

[JelleZijlstra]

I don't understand what's going on in this test case, could you explain? I see you have --always-true True, but why should we care about that? And why does it cause all of these errors?

[cdce8p]

The current implementation for overload merging requires that all conditions can be inferred and mypy knows which branch will be executed. I.e. we know what if True will do (with --always-true True). However, for if maybe_true we can't be sure the branch will actually be executed and thus we don't merge the overloads.

The errors then are a result of the default behavior if any of the if blocks contains a node other than overload / FuncDef. Since none of them are merge, we emit Single overload definition, multiple required and Name "f1" already defined errors.

[JelleZijlstra]

I see. It's weird though that there are so many errors; I feel like only the one on line 6459 should be emitted. Is it possible to make the "Single overload definition, multiple required" and "Name "f1" already defined on line 9" errors go away? Seeing those will be confusing for users who use an unsupported condition.

[cdce8p]

It's weird though that there are so many errors

I agree. The issue here is that both single overload definition and Name already defined on line are emitted during the analysis phase. At least currently, I've implemented the overload merging during the initial file parse. Basically, I'm rearranging the overloads if it's safe to do so, i.e. removing the nesting and IfExp nodes to leave just one OverloadedFuncDef in its place.

I've not (yet) touched the original overload analysis. It could be possible, but I fear that the logic to handle it will get complicated pretty fast. An especially challenge would be to exclude those cases where it's actually desirable to have all these messages.

I'm just not sure the effort is worth it. Do also consider that it's unlikely to encounter this issue in practice. The docs clearly state that only a limited number of conditions are supported. Most users will likely be just fine if they stick to if TYPE_CHECKING and if sys.version_info checks. The only other case would be if they add some other node between all the overloads (L-6483). That is unfortunate, but should happen normally. Anyway, it's good that mypy fails loudly in this case.

[JelleZijlstra]

Alright, thanks. We can merge this as is for now and revisit if users complain about the errors.