8000 Fix for dict_keys being in _collections.abc by mattdavis90 · Pull Request #6888 · python/typeshed · GitHub
[go: up one dir, main page]
Skip to content

Fix for dict_keys being in _collections.abc #6888

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix for dict_keys being in _collections.abc #6888

wants to merge 3 commits into from

Conversation

mattdavis90
Copy link

This is my attempt at fixing the issue I opened (#6837) that requires the user to import _collections_abc.dict_keys as the return type of {}.keys() - I believe that it still satisfies the requirements of #6039 whilst maintaining backward compatibility to versions of mypy <v0.930

@mattdavis90 mattdavis90 mentioned this pull request Jan 10, 2022
Closed
@github-actions

This comment has been minimized.

Sign in to view
@mattdavis90
Copy link
Author

Unable to work out how to fix Python 3.10 as this requires the generic MappingProxyType which takes two TypeVars, not a single one like each of KeysView and ValuesView expect. Maybe this is why the approach was taken in the original PR?

@github-actions
Copy link
Contributor

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

@AlexWaygood
Copy link
Member
AlexWaygood commented Jan 10, 2022 •
edited
Loading

It's a tricky problem to solve ☹

dict_keys and typing.KeysView have always been typed as separate classes in typeshed, which reflects the reality at runtime. It wasn't moving the dict_keys class from builtins to _collections_abc that broke your code using KeysView as an annotation; it was the addition of the mapping attribute to the dict_keys class in 3.10. That was a change made by CPython, not typeshed — all typeshed did was reflect the change made to the runtime in the stubs.

Mypy previously didn't object to you annotating your code with KeysView (even if you strictly "should" have been annotating your code with dict_keys), because dict_keys didn't have any attributes that KeysView didn't. But now, dict_keys does have attributes that KeysView doesn't have, meaning mypy no longer considers KeysView to be compatible with dict_keys.

Arguably, since CPython caused this problem, CPython should fix it: either by properly exposing the dict_keys class somewhere (probably the types module), so that users don't have to import the class from from a private module in order to annotate their code; or by adding the mapping attribute to collections.abc.KeysView, so that the keys() of non-dict mappings such as shelve.Shelf and os.environ also have the mapping attribute.

@AlexWaygood
Copy link
Member

I'm happy to file a BPO issue tomorrow about this, FWIW. It seems like a pretty serious problem.

@mattdavis90
Copy link
Author

Hi @AlexWaygood, thanks for the reply and explanation. I looked around a fair bit in the CPython code for the "true" return type of a dict but couldn't find it. I saw how dict_keys was defined but hadn't quite understood how it worked until I read your explanation.

Thanks for offering to raise a BPO that would be great.

@AlexWaygood
Copy link
Member

Three alternative solutions that wouldn't require changes in CPython would be:

  1. Leave dict_keys in _collections_abc, but delete the mapping attribute from the class. This would mean users would get a false-positive error from mypy if they ever tried to access the mapping attribute of dict.keys(). A bit of a shame.
  2. Leave dict_keys in _collections_abc, and add the mapping attribute to typing.KeysView. This would mean mypy would raise no errors if a user tried to do shelve.Shelf().keys().mapping, even though it should. It would also probably be quite disruptive: we'd have to change typing.KeysView so that it accepts two TypeVar parameters instead of one, which would break a lot of people's code.
  3. Somehow add special-casing logic to mypy and other type-checkers so that they recognise dict_keys and KeysView as being compatible, despite the difference in interfaces.

I like the idea of a CPython fix much better than any of those three, but I think those are our options 🙂

@hauntsaninja
Copy link
Collaborator
hauntsaninja commented Jan 11, 2022 •
edited
Loading

[what broke your code] was the addition of the mapping attribute to the dict_keys class in 3.10

This isn't exactly true. What broke OP's code was changing the return types of dict.whatever to concrete classes: https://github.com/python/typeshed/pull/6039/files#diff-1580ddf7dc331234b0845ffb953b6f7a5f1e9afa15048ebc0762226a08f194b4R826-R828

This would have broken OP's code regardless of whether the mapping attribute existed, although of course, the motivation to return concrete classes was the introduction of the mapping attribute.

Getting slightly off topic, there is a sort of general problem that can happen with Liskov's substitution principle:

class A: ...
class B(A): ...
class C(B): ...

class X:
    def f(self) -> A: ...

class Y(X):
    def f(self) -> B: ...

# where Z inherits from Y to inherit specific functionality, but users of Z will always treat it as an instance of X
# if so, the error is just noise, but users rightfully find it scary to type ignore -> it's very easy to treat Z as a Y if you ever use Y in your program
class Z(Y):
    def f(self) -> A: ...  # here type checkers complain about liskov substitution

Here, X is MutableMapping, Y is Dict, Z is user's custom class.

LSP often causes tension for typeshed. We often make changes that allow type checkers to infer more specific types (which is good), but users who inherit from those types get broken. When users don't care about exact types and especially when those exact types don't exist or aren't obvious at runtime, usability suffers. I recently walked someone through LSP issues involving a custom IO type + _typeshed.ReadableBuffer and it wasn't a great user experience for them. But hard to make things better :-)

Anyway, a possible better solution for the code OP has in their issue report could be to inherit their custom class from MutableMapping.
Another valid option is continue to use KeysView + type: ignore. However, this is a little unsafe: if a) OP later changes the actual returned value of keys to something that isn't dict_keys, and b) users ever do d.keys().mapping thinking they have a dict when actually they have OP's class, there will be errors.
Finally, as Alex suggests, exposing the types at runtime from a better place than _collections_abc.

@AlexWaygood
Copy link
Member

It's a tricky problem to solve ☹

Hey, at least I got this part right 😉

@hauntsaninja
Copy link
Collaborator

It is tricky! Thanks for helping talk through it. I led OP astray with my unworkable suggestion in #6837 (comment) , which you very kindly explain the problems with in point 2 of #6888 (comment)

@AlexWaygood
Copy link
Member

BPO issue here: https://bugs.python.org/issue46399

@AlexWaygood
Copy link
Member

FYI, I think my proposed solution (to expose dict_keys in the types module) is pretty close to being rejected by the core devs. If you have any thoughts — either for or against my suggested solution! — I'd encourage you to chime in on the BPO issue 🙂 (in a friendly and constructive way — obviously please don't start a pile-on!)

@Akuli
Copy link
Collaborator
Akuli commented Jan 20, 2022

How about adding dict_keys and dict_values to typing_extensions? It's not the right place for them, but:

  • It would more likely get accepted, because typing_extensions maintainers are more aware of subtle typing difficulties than CPython developers.
  • If you are doing advanced typing tricks, you likely need typing_extensions installed anyway, or you are used to doing if TYPE_CHECKING:.
  • typing_extensions is available at runtime, unlike e.g. _typeshed.

8000
@AlexWaygood
Copy link
Member
AlexWaygood commented Jan 20, 2022 •
edited
Loading

How about adding dict_keys and dict_values to typing_extensions?

Ooh, I like this idea. Let's wait until adding them to types has been definitively rejected, but if it is, I say let's run with this.

@JelleZijlstra
Copy link
Member

As I mentioned on the bug, I don't think exposing dict_keys and friends at runtime actually solves that much, since you can't inherit from or instantiate them.

How would a user write type-clean code if we exposed those types?

@AlexWaygood
Copy link
Member

As I mentioned on the bug, I don't think exposing dict_keys and friends at runtime actually solves that much, since you can't inherit from or instantiate them.

How would a user write type-clean code if we exposed those types?

Yeah. It's still not ideal.

Here's yet another idea for fixing this issue:

Define a custom protocol in typing_extensions (available at runtime and subclassable), that inherits from KeysView, but is parameterised by two TypeVars and, in addition to the KeysView interface, also has an abstract mapping property. Then have dict and OrderedDict return this protocol, rather than returning a concrete class.

@Akuli
Copy link
Collaborator
Akuli commented Jan 20, 2022

I wonder if a protocol can inherit from a concrete class.

@AlexWaygood
Copy link
Member

I wonder if a protocol can inherit from a concrete class.

No, it can't. And typing.KeysView is a concrete class. So yeah, it can't be a protocol.

But tbh, probably doesn't need to be? Just a generic ABC that's subclassable might work?

@JelleZijlstra
Copy link
Member

As discussed, this change isn't correct, and there isn't much we can do in typeshed. Users may just need to # type: ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mattdavis90 @AlexWaygood @hauntsaninja @Akuli @JelleZijlstra
0