Skip to content

lru_cache-wrapped function args must be Hashable #3944

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

Merged
merged 1 commit into from
May 28, 2020

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Apr 25, 2020

Per docs:
Since a dictionary is used to cache results, the positional and keyword arguments to the function must be hashable.

@hauntsaninja
Copy link
Collaborator

A little wary of this change; type checkers don't really understand Hashable: #3884 (comment)

@ikonst
Copy link
Contributor Author

ikonst commented Apr 26, 2020

Thanks, I learned something :) I've indeed wondered how Hashable worked in mypy since object is hashable.

I tried this approach of restricting arg types to Hashable, and mypy did catch (thanks to #3219) passing list, dict etc. which would otherwise raise in runtime. So it does seem to add some value without adding false negatives. no?

BTW, Another thing that irks me about this definition is that it doesn't maintain the wrapped function's signature, i.e. it's not

_T = TypeVar('_T', bound=Callable)

def lru_cache() -> Callable[[_T], _T]: ...

Of course defining it this way would do away with the wrapper's methods like cache_clear method, which I wouldn't want to do.

@JelleZijlstra
Copy link
Member

I might have been too pessimistic about Hashable in the past; mypy does understand it now. This PR looks like a step in the right direction.

@JelleZijlstra JelleZijlstra merged commit 1dc5853 into python:master May 28, 2020
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this pull request Jun 26, 2020
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.

3 participants