MagicStack / immutables

A high-performance immutable mapping type for Python.
Other
1.14k stars 57 forks source link

Typing overhaul #54

Closed decorator-factory closed 3 years ago

decorator-factory commented 3 years ago

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

  3. 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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()).

bryanforbes commented 3 years ago

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

elprans commented 3 years ago

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

That's right.

bryanforbes commented 3 years ago

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

decorator-factory commented 3 years ago

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