uniffi-rs
uniffi-rs copied to clipboard
mozilla
Python mypy errors when record member names match type names
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.
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.
@crazytonyli @joe-p @akhramov any ideas here? Not a big deal but slightly annoying.
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
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.
huh - we also use the field as a placeholder for the field docstring :( https://github.com/mozilla/uniffi-rs/blob/ccc97b5582f84b02e02c892dc803d756b03f0246/uniffi_bindgen/src/bindings/python/templates/RecordTemplate.py#L5
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 """
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 - https://github.com/mozilla/uniffi-rs/blob/731ba3386087f78d1cccc3b99675bf5fade24189/fixtures/docstring/tests/bindings/test_docstring.py#L53. 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?
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
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
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.
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.
(I mean I guess we probably could use literals everywhere other than where they cause problems, but that still leaves lists, maps and objects)
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.