Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure TypeVarIds are unique #11657

Closed
wants to merge 3 commits into from
Closed

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Dec 3, 2021

Description

Closes #11605

This PR makes it so that TypeVarIds have to be unique among the class / superclasses. Other parts of mypy use this as a unique identifier (for instance expandtype.py has a visitor that has said assumption). This change makes this work!

Test Plan

I added the case that didn't work for me, but I would recommend looking at the code closely as this fix seems really... icky.

Also, mypy-primer's output is going to be terribly long because of the difference in generic representation :(

@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 3, 2021

Analysis of mypy primer:

(Also I didn't get any of the differences in generic formatting things I expected due to the tests, neat)

rich's LRUCache

I believe these errors are because OrderedDict never gets type parameters, and since now the type vars are unique, the key and the value don't necessarily get merged with KT and VT.

I think this is the correct behaviour: if the typevars in the generic were flipped, I believe the mypy without this PR would complain which is not good behaviour. (Not at a computer to check)

Edits needed to make work:

index b7bf2ce..ef87d05 100644
--- a/rich/_lru_cache.py
+++ b/rich/_lru_cache.py
@@ -6,7 +6,7 @@ CacheKey = TypeVar("CacheKey")
 CacheValue = TypeVar("CacheValue")
 
 
-class LRUCache(Generic[CacheKey, CacheValue], OrderedDict):  # type: ignore # https://github.com/python/mypy/issues/6904
+class LRUCache(Generic[CacheKey, CacheValue], OrderedDict[CacheKey, CacheValue]):  # https://github.com/python/mypy/issues/6904
     """
     A dictionary-like container that stores a given maximum items.
 
@@ -24,11 +24,11 @@ class LRUCache(Generic[CacheKey, CacheValue], OrderedDict):  # type: ignore # ht
         if key not in self:
             if len(self) >= self.cache_size:
                 self.popitem(last=False)
-        OrderedDict.__setitem__(self, key, value)
+        super().__setitem__(key, value)
 
-    def __getitem__(self: Dict[CacheKey, CacheValue], key: CacheKey) -> CacheValue:
+    def __getitem__(self, key: CacheKey) -> CacheValue:
         """Gets the item, but also makes it most recent."""
-        value: CacheValue = OrderedDict.__getitem__(self, key)
-        OrderedDict.__delitem__(self, key)
-        OrderedDict.__setitem__(self, key, value)
+        value: CacheValue = super().__getitem__(key)
+        super().__delitem__(key)
+        super().__setitem__(key, value)
         return value

I believe this is OK.


websockets

My instinct is that this is because type(...) is erasing the type variables... But again, not at a computer :P

The error message sucks though. I think it should have the typevar ids? But out of scope...

--- a/src/websockets/legacy/protocol.py
+++ b/src/websockets/legacy/protocol.py
@@ -679,13 +679,11 @@ class WebSocketCommonProtocol(asyncio.Protocol):
         elif isinstance(message, AsyncIterable):
             # aiter_message = aiter(message) without aiter
             # https://github.com/python/mypy/issues/5738
-            aiter_message = type(message).__aiter__(message)  # type: ignore
+            aiter_message = message.__aiter__()
             try:
                 # message_chunk = anext(aiter_message) without anext
                 # https://github.com/python/mypy/issues/5738
-                message_chunk = await type(aiter_message).__anext__(  # type: ignore
-                    aiter_message
-                )
+                message_chunk = await aiter_message.__anext__()
             except StopAsyncIteration:
                 return
             opcode, data = prepare_data(message_chunk)

I... Don't know why the code was doing type(message).__aiter__(message) in the first place, but that was making the generic types unbound (also, I believe the mypy issues mentioned are not actually the problem, just that the error message was confusing like I thought earlier)/


Tanjun: nice to see the project I'm trying to make compatible with mypy shows up here, the issue was extracted from it :-)

mypy/semanal.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

rich (https://github.com/willmcgugan/rich)
+ rich/_lru_cache.py:27: error: Incompatible types in assignment (expression has type "CacheKey", target has type "_KT")
+ rich/_lru_cache.py:27: error: Incompatible types in assignment (expression has type "CacheValue", target has type "_VT")
+ rich/_lru_cache.py:31: error: Incompatible types in assignment (expression has type "_VT", variable has type "CacheValue")
+ rich/_lru_cache.py:31: error: Invalid index type "Dict[CacheKey, CacheValue]" for "dict"; expected type "Dict[_KT, _VT]"
+ rich/_lru_cache.py:31: error: Invalid index type "CacheKey" for "dict"; expected type "_KT"
+ rich/_lru_cache.py:32: error: Argument 1 to "__delitem__" of "dict" has incompatible type "Dict[CacheKey, CacheValue]"; expected "Dict[_KT, _VT]"
+ rich/_lru_cache.py:32: error: Argument 2 to "__delitem__" of "dict" has incompatible type "CacheKey"; expected "_KT"
+ rich/_lru_cache.py:33: error: Invalid index type "Dict[CacheKey, CacheValue]" for "dict"; expected type "Dict[_KT, _VT]"
+ rich/_lru_cache.py:33: error: Incompatible types in assignment (expression has type "CacheKey", target has type "_KT")
+ rich/_lru_cache.py:33: error: Incompatible types in assignment (expression has type "CacheValue", target has type "_VT")

Tanjun (https://github.com/FasterSpeeding/Tanjun)
- tanjun/abc.py:2268: error: Signature of "copy" incompatible with supertype "ExecutableCommand"
- tanjun/abc.py:2268: note:      Superclass:
- tanjun/abc.py:2268: note:          def copy(self) -> MessageCommand[MessageContext]
- tanjun/abc.py:2268: note:      Subclass:
- tanjun/abc.py:2268: note:          def copy(self, *, parent: Optional[MessageCommandGroup[Any]] = ...) -> MessageCommand[CommandCallbackSigT]
- tanjun/commands.py:1143: error: Return type "SlashCommand[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "SlashCommand[SlashContext]" in supertype "PartialCommand"
- tanjun/commands.py:1143: error: Return type "SlashCommand[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "SlashCommand[SlashContext]" in supertype "ExecutableCommand"
- tanjun/commands.py:1945: error: Signature of "copy" incompatible with supertype "PartialCommand"
- tanjun/commands.py:1945: note:      Superclass:
- tanjun/commands.py:1945: note:          def copy(self, *, _new: bool = ...) -> SlashCommand[SlashContext]
- tanjun/commands.py:1945: note:      Subclass:
- tanjun/commands.py:1945: note:          def copy(self, *, _new: bool = ..., parent: Optional[SlashCommandGroup] = ...) -> SlashCommand[abc.CommandCallbackSigT]
- tanjun/commands.py:1945: error: Signature of "copy" incompatible with supertype "ExecutableCommand"
- tanjun/commands.py:1945: note:          def copy(self) -> SlashCommand[SlashContext]
- tanjun/commands.py:1945: note:          def copy(self, *, _new: bool = ..., parent: Optional[SlashCommandGroup] = ...) -> SlashCommand[abc.CommandCallbackSigT]
- tanjun/commands.py:2080: error: Return type "MessageCommand[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "MessageCommand[MessageContext]" in supertype "PartialCommand"
- tanjun/commands.py:2080: error: Return type "MessageCommand[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "MessageCommand[MessageContext]" in supertype "ExecutableCommand"
- tanjun/commands.py:2088: error: Return type "MessageCommand[abc.CommandCallbackSigT]" of "bind_component" incompatible with return type "MessageCommand[MessageContext]" in supertype "PartialCommand"
- tanjun/commands.py:2088: error: Return type "MessageCommand[abc.CommandCallbackSigT]" of "bind_component" incompatible with return type "MessageCommand[MessageContext]" in supertype "ExecutableCommand"
- tanjun/commands.py:2096: error: Signature of "copy" incompatible with supertype "PartialCommand"
- tanjun/commands.py:2096: note:      Superclass:
- tanjun/commands.py:2096: note:          def copy(self, *, _new: bool = ...) -> MessageCommand[MessageContext]
- tanjun/commands.py:2096: note:      Subclass:
- tanjun/commands.py:2096: note:          def copy(self, *, parent: Optional[MessageCommandGroup[Any]] = ..., _new: bool = ...) -> MessageCommand[abc.CommandCallbackSigT]
- tanjun/commands.py:2096: error: Signature of "copy" incompatible with supertype "ExecutableCommand"
- tanjun/commands.py:2096: note:          def copy(self) -> MessageCommand[MessageContext]
- tanjun/commands.py:2096: note:          def copy(self, *, parent: Optional[MessageCommandGroup[Any]] = ..., _new: bool = ...) -> MessageCommand[abc.CommandCallbackSigT]
- tanjun/commands.py:2221: error: Signature of "copy" incompatible with supertype "PartialCommand"
- tanjun/commands.py:2221: note:      Superclass:
- tanjun/commands.py:2221: note:          def copy(self, *, _new: bool = ...) -> MessageCommandGroup[MessageContext]
- tanjun/commands.py:2221: note:      Subclass:
- tanjun/commands.py:2221: note:          def copy(self, *, parent: Optional[MessageCommandGroup[Any]] = ..., _new: bool = ...) -> MessageCommandGroup[abc.CommandCallbackSigT]
- tanjun/commands.py:2221: error: Signature of "copy" incompatible with supertype "ExecutableCommand"
- tanjun/commands.py:2221: note:          def copy(self) -> MessageCommandGroup[MessageContext]
- tanjun/commands.py:2221: note:          def copy(self, *, parent: Optional[MessageCommandGroup[Any]] = ..., _new: bool = ...) -> MessageCommandGroup[abc.CommandCallbackSigT]
- tanjun/commands.py:2291: error: Return type "MessageCommandGroup[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "MessageCommandGroup[MessageContext]" in supertype "PartialCommand"
- tanjun/commands.py:2291: error: Return type "MessageCommandGroup[abc.CommandCallbackSigT]" of "bind_client" incompatible with return type "MessageCommandGroup[MessageContext]" in supertype "ExecutableCommand"
- tanjun/commands.py:2299: error: Return type "MessageCommandGroup[abc.CommandCallbackSigT]" of "bind_component" incompatible with return type "MessageCommandGroup[MessageContext]" in supertype "PartialCommand"
- tanjun/commands.py:2299: error: Return type "MessageCommandGroup[abc.CommandCallbackSigT]" of "bind_component" incompatible with return type "MessageCommandGroup[MessageContext]" in supertype "ExecutableCommand"

websockets (https://github.com/aaugustin/websockets)
+ src/websockets/legacy/protocol.py:687: error: Argument 1 to "__anext__" of "AsyncIterator" has incompatible type "AsyncIterator[_T_co]"; expected "AsyncIterator[_T_co]"

JukkaL added a commit that referenced this pull request Apr 19, 2022
This was originally written by @A5rocks in #11657.

Related to #12590.
@JukkaL
Copy link
Collaborator

JukkaL commented Apr 19, 2022

Closing since #12590 fixes the underlying issue in a bit cleaner way. I extracted the new test case as #12623.

@A5rocks Thank you for the effort anyway! TypeVarIds overlap is a nasty issue and has caused a lot of confusion over time.

@JukkaL JukkaL closed this Apr 19, 2022
hauntsaninja pushed a commit that referenced this pull request Apr 20, 2022
This was originally written by @A5rocks in #11657.

Related to #12590.
@A5rocks A5rocks deleted the unique-typevarids branch May 15, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does replacing a type variable make a subclass incompatible?
3 participants