Skip to content

Typing overhaul - #54

Closed
decorator-factory wants to merge 3 commits into
MagicStack:masterfrom
decorator-factory:typing-fix
Closed

decorator-factory wants to merge 3 commits into
MagicStack:masterfrom
decorator-factory:typing-fix

Conversation

@decorator-factory

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

Copy link
Copy Markdown

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
Copy Markdown
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

elprans commented Feb 9, 2021

Copy link
Copy Markdown
Member

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

elprans commented Feb 9, 2021

Copy link
Copy Markdown
Member

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

bryanforbes commented Feb 9, 2021 •

Copy link
Copy Markdown
Contributor

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

@elprans

elprans commented Feb 9, 2021

Copy link
Copy Markdown
Member

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

That's right.

@bryanforbes

Copy link
Copy Markdown
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

decorator-factory commented Feb 13, 2021 •

Copy link
Copy Markdown
Author

@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.

3 participants