MagicStack / immutables

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

Refactor typings #62

Closed bryanforbes closed 3 years ago

bryanforbes commented 3 years ago

fixes #54

I've taken the work in the typing overhaul PR and improved it as detailed above. The following passes using mypy in strict mode:

from immutables import Map
from typing import Union

m: Map[int, int] = Map()

m1 = Map[int, int]({1: 2})
m2: Map[Union[int, str], int] = m1.update({"answer": 2})
m3: Map[Union[int, str], int] = m1.update(answer=2)
m4: Map[int, Union[int, str]] = m1.update({4: "four"})
m5: Map[Union[int, str], Union[int, str]] = m1.update({"four": "four"})
m6: Map[Union[int, str], Union[int, str]] = m1.update(four="four")
m7: Map[Union[int, str], Union[int, str, float]] = m1.update(m5, five=5.0)

reveal_type(m1.update(m5, four=(4.0,)).update(five=5))  # Revealed type is 'immutables._map.Map[Union[builtins.int, builtins.str], Union[builtins.int, Tuple[builtins.float], builtins.str]]'
reveal_type(m1.set('four', 'four').set((4.0,), (5.0,)))  # Revealed type is 'immutables._map.Map[Union[builtins.int, builtins.str, Tuple[builtins.float]], Union[builtins.int, builtins.str, Tuple[builtins.float]]]'

There was discussion in #54 about restricting update() and other Map-producing functions to the key and value types of the original Map. I'm not exactly sure which is the best approach since the implementation allows update() to take different types for the key and value, however typing.Dict disallows this for dict even though dict.update() allows similar functionality as Map.update(). For now, I've left Map.update() similar to what was done in #54, but that can certainly be changed before merging.

bryanforbes commented 3 years ago

I thought about this a bit more and one advantage of disallowing different key and value types is that Map.mutate() returns a MapMutation that will type check correctly and won't need a cast() (or a change to the library code to support syntax like map.mutate[Union[int, str], str]() to use MapMutation.update() with different key and value types.

elprans commented 3 years ago

I'm strongly in favor of restricting update to original types.

bryanforbes commented 3 years ago

I'm strongly in favor of restricting update to original types.

I think I've convinced myself of restriction as well. I'll update the PR.

bryanforbes commented 3 years ago

I've updated the PR making update() and other mutators restricted to the Map type. Here's a run-down of what happens now:

from immutables import Map
from typing import Dict, Union, cast

def init() -> None:
    def thing(m: Map[str, Union[str, int]]) -> None:
        ...

    thing(Map(foo=1))
    thing(Map(foo='bar', baz=1))
    thing(Map([('foo', 'bar'), ('bar', 1)]))
    thing(Map(Map(foo=1), bar='foo'))
    m = Map({1: 2})
    thing(m)  # Incompatible type

def assignments() -> None:
    m_int__str = Map[int, str]()
    m_str__str = Map[str, str]()
    m_int_str__str = Map[Union[int, str], str]()
    m_str__int_str = Map[str, Union[int, str]]()

    m_int__str = m_str__str  # Incompatible types
    m_int__str = m_int_str__str  # Incompatible types
    m_int__str = m_str__int_str  # Incompatible types

    m_str__str = m_int__str  # Incompatible types
    m_str__str = m_int_str__str  # Incompatible types
    m_str__str = m_str__int_str  # Incompatible types

    m_int_str__str = m_int__str  # Incompatible types
    m_int_str__str = m_str__str  # Incompatible types
    m_int_str__str = m_str__int_str  # Incompatible types

    m_str__int_str = m_int__str  # Incompatible types
    m_str__int_str = m_int_str__str  # Incompatible types
    m_str__int_str = m_str__str

def update() -> None:
    m_int__str: Map[int, str] = Map()
    m_str__str: Map[str, str] = Map()
    m_int_str__str: Map[Union[int, str], str] = Map()
    m_str__int_str: Map[str, Union[int, str]] = Map()

    m_int__str.update({1: '2'})
    m_int__str.update({1: '2'}, three='4')  # Unexpected keyword argument
    m_int__str.update({1: 2})  # Argument 1 has incompatible type

    m_str__str.update({'1': '2'})
    m_str__str.update({'1': '2'}, three='4')
    m_str__str.update({'1': 2})  # Argument 1 has incompatible type

    m_int_str__str.update(cast(Dict[Union[int, str], str], {1: '2', '3': '4'}))
    m_int_str__str.update({1: '2'}, three='4')
    m_int_str__str.update({'1': 2})  # Argument 1 has incompatible type

    m_str__int_str.update({'1': 2, '2': 3})
    m_str__int_str.update({'1': 2, '2': 3}, four='5')
    m_str__int_str.update({1: 2})  # Argument 1 has incompatible type

def mutate() -> None:
    m = Map[str, str]()

    with m.mutate() as mm:
        mm[0] = '1'  # Invalid index type
        mm['1'] = 0  # Incompatible types
        mm['1'] = '2'
        del mm['1']
        mm.set('3', '4')
        m2 = mm.finish()

    reveal_type(m2)  # Revealed type is 'immutables._map.Map[builtins.str*, builtins.str*]'
JanTkacik commented 3 years ago

Hi, can I somehow help to get this to master ?