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

Typing overhaul #54

Closed
wants to merge 3 commits into from
Closed

Typing overhaul #54

wants to merge 3 commits into from

Conversation

decorator-factory
Copy link

@decorator-factory decorator-factory commented Jan 11, 2021

This PR aims to do three things:

  1. Make use of type annotations
    Before, only _map was annotated, so the type checker (Pylance in my case) wasn't picking up on it. This is done using TYPE_CHECKING, which is available since Python 3.5.2.

  2. Additional constraints when using **kwargs:
    Specifically, this is wrong:

m: "Map[int, int]" = Map(answer=42)
m: "Map[int, int]" = Map({1: 41}).update(answer=42)

so if **kwargs are used, the key type should be a subtype of str

  1. Allowing more valid updates
    With dict or any other mutable mapping, this is wrong:
d: dict[int, int] = {1: 2}
d["answer"] = 42

However, with Map, this is fine, it just produces a map of different type:

m1: "Map[int, int]" = Map({1: 2})
m2: "Map[int | str, int]" = m1.update({"answer": 42})
m3: "Map[int | str, int]" = m1.update(answer=42)
m4: "Map[int, int | str]" = m1.update({4: "four"})
m5: "Map[int | str, int | str]" = m1.update({"four": "four"})
m6: "Map[int | str, int | str]" = m1.update(four="four")

@decorator-factory
Copy link
Author

One issue, though: Mapping expects the key type to be invariant and the value type to be covariant.
Not only that, but Map[K, V]'s key and value type are covariant, because it's equivalent to tuple[tuple[K, V], ...] or frozenset[tuple[K, V]].
I'm not sure how this should be approached.

@elprans
Copy link
Member

elprans commented Feb 9, 2021

However, with Map, this is fine, it just produces a map of different type:

I don't think this is a good idea. Map.update() is semantically equivalent to dict mutation and the original type constraint is still valuable in most cases to prevent erroneous use. This change effectively makes Map to behave as if it was Any in the key for all mutations.

@elprans
Copy link
Member

elprans commented Feb 9, 2021

One issue, though: Mapping expects the key type to be invariant and the value type to be covariant.
Not only that, but Map[K, V]'s key and value type are covariant [...]
I'm not sure how this should be approached.

This is exactly why Mapping is invariant in key: it's the only way to soundly satisfy key contravariance in __getitem__ and its covariance in various key-returning methods (keys(), values()).

@1st1 1st1 mentioned this pull request Feb 9, 2021
@bryanforbes
Copy link
Contributor

bryanforbes commented Feb 9, 2021

@elprans Am I understanding you correct that Map should be invariant in key and covariant in value?

@elprans
Copy link
Member

elprans commented Feb 9, 2021

Am I understanding you correct that Map should be invariant in key and covariant in value?

That's right.

@bryanforbes
Copy link
Contributor

@elprans After playing around with this locally, I think the types need to be updated a bit more substantially than this PR addresses. I'll be submitting a new PR soon to address what I've found.

@bryanforbes bryanforbes mentioned this pull request Feb 10, 2021
@decorator-factory
Copy link
Author

decorator-factory commented Feb 13, 2021

@bryanforbes @elprans Thanks for the review. I guess this PR is made obsolete by #62, so I can close it?

I think restricting the type of update is a good idea (because it will prevent likely mistakes and not cause long and confusing error messages), and if someone wants unionized updates, they can make a separate function with the required signature.

@elprans elprans closed this in #62 Aug 4, 2021
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.

None yet

3 participants