Fix recursion issue with nested instances and unions #9663
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #9657
Test Plan
I have added three new tests to
test-data/unit/check-unions.testtestNestedProtocolUnionsThis is an exact copy of the MVCE I included in the issue.
testNestedProtocolGenericUnionsAt first I 'fixed' the issue by not running the
forifleftis a protocol inmypy.subtypes:is_protocol_implementation. I however noticed that this would still cause the same issue if we changed toIter = Union[Iterator[T], List[T]]. And all I'd have done is add a subtle bug. This is just the same astestNestedProtocolUnionsbut to account for non-protocols.testNestedProtocolGenericUnionsDeepThis tests that we can go 5
Iters deep. This is important as we can fix the bug at 3 levels by adding the following tomypy.sametypes:is_same_type.My changes pass all the tests ran via
./runtests.py.Origin of the Bug
The issue comes from assuming in
mypy.subtypes:is_protocol_implementation.The assumed type is an instance that has a union as its argument.
mypy.sametypes:is_same_typeworks fine for theInstance.We check the
Unions are the same type, again withmypy.sametypes:is_same_type.This falls apart as we then call
simplify_unionwhich goes on to callmypy.subtypes:is_protocol_implementation.In this second call to
is_protocol_implementationwhat we assume the type to be stays the same. This causes problems because we're constantly simplifying what we assume the type to be against itself.To circumvent this semi-infinite loop I've forced the left value to always be assumed to be a simplified union. This is risky as this is a new assumption. It seems to work, but I'm unsure if this is because of a lack of tests that'd put a non-simplified union on the left side.
Alternates
I can think of two different solutions that don't add an assumption to
mypy.sametypes:is_same_type.mypy.subtypes:is_protocol_implementationto correctly handle the recursion. I don't know how to implement this.