Python: improve default values and use dataclasses to keep mypy happy.

[mhammond]

pub struct Record {
    bool: bool, // sad mypy
}

cargo test -p uniffi-fixture-keywords-rust works, but:

% mypy target/tmp/uniffi_keywords_rust-xxx/keywords_rust.py
.../keywords_rust.py:1161: error: Variable "keywords_rust.RecordWithTypeNames.bool" is not valid as a type  [valid-type]

This just demonstrates the error but makes no attempt to fix it yet.

[mhammond]

"Rust and Foreign Language tests" are failing with the mypy errors.

[mhammond]

This isn't just builtins:

class foo: # unlikely, but looking at the general case...
    pass

class Test:
    bool: bool
    int: int
    foo: foo
    def __init__(self, bool: bool, foo: foo):
        pass

t = Test(True, foo())

mypy complains:

/tmp/t.py:8: error: Variable "t.Test.bool" is not valid as a type  [valid-type]
/tmp/t.py:8: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
/tmp/t.py:8: error: Variable "t.Test.foo" is not valid as a type  [valid-type]

(not clear why int isn't also an error)

So my initial naive idea of just special-casing builtin types and using a fully-qualified name from the builtins module isn't going to work in general (although may actually be fine in practice?) I can't see this addressed in the mypy docs.

[mhammond]

@crazytonyli @joe-p @akhramov any ideas here? Not a big deal but slightly annoying.

[joe-p]

If the __init__ args have type hints then MyPy can infer the types of the instance variables without needing explicit type hints. This is probably the simplest path forward.

from dataclasses import dataclass


class foo: 
    pass

@dataclass(init=False)
class Test: 
    def __init__(self, bool: bool, foo: foo) -> None:
        self.bool = bool
        self.foo = foo


test = Test(bool=True, foo=foo())
foo_val = test.foo

In this case MyPy knows foo_val is of type foo.

If explicit type hints on the instance vars are desired you could use type aliases, but that was added in 3.10. To be clear I'm not sure why this would be required. I've confirmed both mypy and basedpyright are able to infer the types properly with the above solution.

BoolType: TypeAlias = bool
FooType: TypeAlias = foo

@dataclass
class Test:
    bool: BoolType
    foo: FooType

Edit: I realized you could also just not use the TypeAlias type hint and it would still work

BoolType = bool
FooType = foo

@dataclass
class Test:
    bool: BoolType
    foo: FooType

Although I still think the first solution is preferable because it avoid the alias type being shown to the user

[mhammond]

dataclass is interesting - it looks like it will generate __eq__ etc too? We manually implement that, but a builtin one seems even better. #2671 just today touched the same concept in swift - it replaced manual implementations with a builtin one. We should maybe pursue that in another issue? But that makes sense to me for this issue, thanks.

[mhammond]

huh - we also use the field as a placeholder for the field docstring :(

uniffi-rs/uniffi_bindgen/src/bindings/python/templates/RecordTemplate.py

Line 5 in ccc97b5

{{ rec.docstring|docstring(4) -}}

[joe-p]

Oh sorry for some reason I thought uniffi already used dataclass. But yeah the fix for this specific issue still works without dataclasses.

huh - we also use the field as a placeholder for the field docstring :(

Looks like we can define them like this according to PEP 258. I just noticed that that PEP is rejected but I have confirmed locally that these style docstrings are picked up by basedpyright and sphinx, so probably safe to assume most tooling supports them.

class Test: 
    def __init__(self, bool: bool, foo: foo) -> None:
        self.bool = bool
        """ This is a boolean field """

        self.foo = foo
        """ This is a foo field """

[mhammond]

huh - line 2 of that file is identical - so we are just repeating the record docstring for every field, which can be seen in the docstring test:

class RecordTest:
    """
    <docstring-record>
"""
    test: int
    """
    <docstring-record>
"""
    
    def __init__(self, *, test:int):
        self.test = test

and the test is checking only the class -

uniffi-rs/fixtures/docstring/tests/bindings/test_docstring.py

Line 53 in 731ba33

assert RecordTest.__doc__.strip() == "<docstring-record>"

. As written, it seems like those "extra" docstrings aren't ever going to appear in __doc__ and doesn't appear in help(). The basedpyright playground doesn't seem to show anything about docs, although it is upset at our signatures for __str__ etc. The right thing to do for now is probably to just omit those field docs entirely and tackle that in another issue?

[mhammond]

@joe-p what do you think about this?

[joe-p]

Oh interesting... yeah for now type inference in __init__ solves the typing problem this PR is addressing and I agree that fixing the docstrings is an orthogonal issue. At the very least we know there is a solution that would fit with __init__ inference and common pyhton tooling. Deeper implications can be explored in other issues/PRs

[mhammond]

This is failing with:

    def __init__(self, *, no_default_string:str, boolean:bool = _DEFAULT, integer:int = _DEFAULT, float_var:float = _DEFAULT, vec:typing.List[bool] = _DEFAULT, opt_vec:typing.Optional[typing.List[bool]] = _DEFAULT, opt_integer:typing.Optional[int] = _DEFAULT, custom_integer:CustomInteger = _DEFAULT, boolean_default:bool = _DEFAULT, string_default:str = _DEFAULT, opt_default:typing.Optional[str] = _DEFAULT, sub:RecordWithImplicitDefaults = _DEFAULT):
        self.no_default_string = no_default_string
...
        if opt_integer is _DEFAULT:
            self.opt_integer = 42
        else:
            self.opt_integer = opt_integer

with error:

target/tmp/uniffi_proc_macro-b83c1fa2d45093c5/proc_macro.py:2810: error: Incompatible types in assignment (expression has type "int | None", variable has type "int") [assignment] pointing at the last line of that snippet

[joe-p]

What is _DEFAULT? I think this is a problem with type narrowing, so conditionally checking against None should work

       if opt_integer is None:
            self.opt_integer = 42
        else:
            self.opt_integer = opt_integer

This should also work:

       if opt_integer is None or is _DEFAULT:
            self.opt_integer = 42
        else:
            self.opt_integer = opt_integer

Alternatively if self.opt_integer being defined is an invariant then you could cast self.opt_integer = cast(int, opt_integer), but I think using is None in the conditional is properly the preferable solution for optional types.

[mhammond]

It's a placeholder - mainly to handle the "default value" case rather than null. We can't unconditionally use literals due to list etc.

But even in the None case it's a little problematic - integer:int = None seems more confusing than the current _DEFAULT, which is awkward but not really confusing. It only makes sense for optional.

[mhammond]

(I mean I guess we probably could use literals everywhere other than where they cause problems, but that still leaves lists, maps and objects)

[mhammond]

I mean I guess we probably could use literals everywhere other than where they cause problems

That actually worked out really well and made the generated code for default values much much better.

[bendk]

I don't understand all the mypy stuff, but I trust you and joe-p on that. Behavior-wise, I can't see anything that's changed so thumbs up from me.