biqqles / dataclassy

A fast and flexible reimplementation of data classes
https://pypi.org/project/dataclassy
Mozilla Public License 2.0
81 stars 9 forks source link

Type mismatch in strict mode on deferred field initialisation #34

Closed nolar closed 3 years ago

nolar commented 3 years ago

As suggested in the README, the non-copyable fields are populated in the constructor:

import asyncio
import dataclassy

@dataclassy.dataclass(slots=True)
class C:
    cnd: asyncio.Condition = None
    evt: asyncio.Event = None

    def __init__(self) -> None:
        self.cnd = asyncio.Condition()
        self.evt = asyncio.Event()

c = C()
print(repr(c.cnd))
print(repr(c.evt))

In that case, the strict mypy typing fails:

$ python --version
Python 3.9.1

$ mypy --version
mypy 0.800

$ mypy --strict _dcy_late_init.py 
_dcy_late_init.py:6: error: Incompatible types in assignment (expression has type "None", variable has type "Condition")
_dcy_late_init.py:7: error: Incompatible types in assignment (expression has type "None", variable has type "Event")
_dcy_late_init.py:10: error: Property "cnd" defined in "C" is read-only
_dcy_late_init.py:11: error: Property "evt" defined in "C" is read-only
# mypy.ini
[mypy]
warn_unused_configs = True
ignore_missing_imports = True
plugins = dataclassy.mypy

dataclassy is installed from git as of 127338786c7aa61ade2689f4217c3d5cde4eb63b.

Actually, two issues at once:

Is there any other way of initialising the non-copyable objects (a functional equivalent of dataclasses' default_factory)?

I would suggest auto-instantiating the objects if the default value is a class. The cases when a class is actually a value to be stored can be detected by having a field annotation Type[...] — in that case, no instantiation should happen. But I am not sure if this covers all the use-cases and does not break anything.

biqqles commented 3 years ago

I'll need to think about this. With mypy the lazy replacement for default_factory I envisaged falls down.

I would suggest auto-instantiating the objects if the default value is a class.

Really it should be for all callables, in case you need to provide arguments to the class (as you would with default_factory).

nolar commented 3 years ago

I'm not so sure about callables. I also use data classes for settings, and some fields are actual callables (with some protocols) — and are used as callables (but not as types/classes).

E.g.:

    some_callback: Callable[[str, int], int] = lambda a, b: ...

Besides, I'm not sure about the cases when an annotation type is a base class, while the default value is one of the derived classes:

    progress_storage: progress.ProgressStorage = dataclasses.field(default_factory=progress.SmartProgressStorage)
    diffbase_storage: diffbase.DiffBaseStorage = dataclasses.field(default_factory=diffbase.AnnotationsDiffBaseStorage)

So, on second thought, I call off my suggestion — it was not a good idea.

The implied question about an equivalent for the default_factory remains relevant though.

biqqles commented 3 years ago

Don't worry, I was thinking of something like field = factory(callable). This would need mypy plugin support though, so it wouldn't immediately solve the problem.

biqqles commented 3 years ago

Just realised you can add a functional Factory class in 5 lines so expect this pretty soon.

biqqles commented 3 years ago

@nolar and @hjalmarlucius - could you test the new Factory class (added in the referenced commit) to see if it does what you want? I know it won't work with mypy yet, before the plugin is updated, but apart from that.

Usage is like so:

from dataclassy import dataclass, Factory

class SomeRandomClass:
    pass

@dataclass
class Example:
    a: SomeRandomClass = Factory(SomeRandomClass)
    b: int = Factory(lambda: 2)

Example()  # Example(a=<__main__.SomeRandomClass object at 0x7f25a6139460>, b=2)
biqqles commented 3 years ago

Realised I could just test the example in the original comment!

import asyncio
import dataclassy

@dataclassy.dataclass(slots=True)
class C:
    cnd: asyncio.Condition = dataclassy.Factory(asyncio.Condition)
    evt: asyncio.Event = dataclassy.Factory(asyncio.Event)

c = C()
print(repr(c.cnd))
print(repr(c.evt))

# <asyncio.locks.Condition object at 0x7fe7ccbdcbe0 [unlocked]>
# <asyncio.locks.Event object at 0x7fe7ccbc3d00 [unset]>
nolar commented 3 years ago

I've checked — yes, it works at runtime.

For type-checking, I believe, it is better if there is a function dataclassy.factory(fct), not a class. That function can produce instances of Factory if needed while simulating that it produces instances of the target type — it is easier to fake types with a function than with a class:

from typing import TypeVar, Callable, cast
_T = TypeVar('_T')
FactoryProtocol = Callable[[], _T]
def factory(fct: FactoryProtocol[_T]) -> _T:
    return cast(_T, dataclassy.Factory(fct))  # we lie, but for a reason

@dataclassy.dataclass(slots=True)
class C:
    cnd: asyncio.Condition = factory(asyncio.Condition)
    evt: asyncio.Event = factory(asyncio.Event)

Not sure if this works for any results of such a function, e.g. when the factory is not a type but a lambda. Probably also doable.

biqqles commented 3 years ago

Usually I hate having functions with the same names as classes but it indeed seems like a great idea here. I would be surprised if it didn't work for any callable. We are using Callable[[], _T] after all. Should be easy to test at least.

nolar commented 3 years ago

In that case, you can actually hide the existence of the class Factory, as it is already a "clean hack" by design, and make it private/protected, so that the implementation can be changed later.

nolar commented 3 years ago

PS: I am not insisting. Just thinking out loud ;-)

biqqles commented 3 years ago

Yes, will do. Like DataClassMeta, I won't name it privately so that if people really want to import it for some reason, analysis tools won't complain, but I will remove it from __init__.py so it has to be explicitly imported from a different file.

biqqles commented 3 years ago

I made your suggested improvement. Since mypy likes it, I think all objectives of this issue are done? :tada: