Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstreaming type annotations #407

Open
henribru opened this issue Nov 15, 2023 · 2 comments
Open

Upstreaming type annotations #407

henribru opened this issue Nov 15, 2023 · 2 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@henribru
Copy link

Hi. I'm the maintainer of a stubs-only package providing type annotations for this library: https://github.com/henribru/proto-plus-stubs

The most important part of the annotations consists of a bunch of overloads on the Field constructor and a descriptor method: https://github.com/henribru/proto-plus-stubs/blob/master/proto-stubs/fields.pyi#L21-L427

The result of this is that type checkers like Mypy and Pyright can correctly infer the types of attributes on instances of proto-plus classes (as well as improving autocomplete in some editors):

from typing import reveal_type

import proto

class Bar(proto.Message):
    pass

class Baz(proto.Enum):
    BAZ = 1

class Foo(proto.Message):
    double = proto.Field(proto.DOUBLE, number=3)
    int = proto.Field(proto.INT32, number=3)
    bool = proto.Field(proto.BOOL, number=3)
    string = proto.Field(proto.STRING, number=3)
    bytes = proto.Field(proto.BYTES, number=3)
    bar_message_1 = proto.Field(proto.MESSAGE, number=3, message=Bar)
    bar_message_2 = proto.Field(Bar, number=3)
    baz_enum_1 = proto.Field(proto.ENUM, number=3, enum=Baz)
    baz_enum_2 = proto.Field(Baz, number=3)

    double_repeated = proto.RepeatedField(proto.DOUBLE, number=3)
    int_repeated = proto.RepeatedField(proto.INT32, number=3)
    bool_repeated = proto.RepeatedField(proto.BOOL, number=3)
    string_repeated = proto.RepeatedField(proto.STRING, number=3)
    bytes_repeated = proto.RepeatedField(proto.BYTES, number=3)
    bar_message_repeated_1 = proto.RepeatedField(proto.MESSAGE, number=3, message=Bar)
    bar_message_repeated_2 = proto.RepeatedField(Bar, number=3)
    baz_enum_repeated_1 = proto.RepeatedField(proto.ENUM, number=3, enum=Baz)
    baz_enum_repeated_2 = proto.RepeatedField(Baz, number=3)

    string_double_map = proto.MapField(ProtoType.STRING, ProtoType.DOUBLE, number=3)
    string_int_map = proto.MapField(ProtoType.STRING, ProtoType.INT32, number=3)
    string_bool_map = proto.MapField(ProtoType.STRING, ProtoType.BOOL, number=3)
    string_string_map = proto.MapField(ProtoType.STRING, ProtoType.STRING, number=3)
    string_bytes_map = proto.MapField(ProtoType.STRING, ProtoType.BYTES, number=3)
    string_bar_message_map_1 = proto.MapField(ProtoType.STRING, ProtoType.MESSAGE, number=3, message=Bar)
    string_bar_message_map_2 = proto.MapField(ProtoType.STRING, Bar, number=3)
    string_baz_enum_map_1 = proto.MapField(ProtoType.STRING, ProtoType.ENUM, number=3, enum=Baz)
    string_baz_enum_map_2 = proto.MapField(ProtoType.STRING, Baz, number=3)
    int_double_map = proto.MapField(ProtoType.INT32, ProtoType.DOUBLE, number=3)
    int_int_map = proto.MapField(ProtoType.INT32, ProtoType.INT32, number=3)
    int_bool_map = proto.MapField(ProtoType.INT32, ProtoType.BOOL, number=3)
    int_string_map = proto.MapField(ProtoType.INT32, ProtoType.STRING, number=3)
    int_bytes_map = proto.MapField(ProtoType.INT32, ProtoType.BYTES, number=3)
    int_bar_message_map_1 = proto.MapField(ProtoType.INT32, ProtoType.MESSAGE, number=3, message=Bar)
    int_bar_message_map_2 = proto.MapField(ProtoType.INT32, Bar, number=3)
    int_baz_enum_map_1 = proto.MapField(ProtoType.INT32, ProtoType.ENUM, number=3, enum=Baz)
    int_baz_enum_map_2 = proto.MapField(ProtoType.INT32, Baz, number=3)

foo = Foo()
reveal_type(foo.double)  # float
reveal_type(foo.int)  # int
reveal_type(foo.bool)  # bool
reveal_type(foo.string)  # str
reveal_type(foo.bytes)  # bytes
reveal_type(foo.bar_message_1)  # Bar
reveal_type(foo.bar_message_2)  # Bar
reveal_type(foo.baz_enum_1)  # Baz
reveal_type(foo.baz_enum_1)  # Baz
reveal_type(foo.double_repeated)  # list[float]
reveal_type(foo.int_repeated)  # list[int] 
reveal_type(foo.bool_repeated)  # list[bool]
reveal_type(foo.string_repeated)  # list[str]
reveal_type(foo.bytes_repeated)  # list[bytes]
reveal_type(foo.bar_message_repeated_1)  # list[Bar]
reveal_type(foo.bar_message_repeated_2)  # list[Bar]
reveal_type(foo.baz_enum_repeated_1)  # list[Baz]
reveal_type(foo.baz_enum_repeated_2)  # list[Baz]
reveal_type(foo.string_double_map)  # dict[str, float]
reveal_type(foo.string_int_map)  # dict[str, int]
reveal_type(foo.string_bool_map)  # dict[string, bool]
reveal_type(foo.string_string_map)  # dict[str, str]
reveal_type(foo.string_bytes_map)  # dict[str, bytes]
reveal_type(foo.string_bar_message_map_1)  # dict[str, Bar]
reveal_type(foo.string_bar_message_map_2)  # dict[str, Bar]
reveal_type(foo.string_baz_enum_map_1)  # dict[str, Baz]
reveal_type(foo.string_baz_enum_map_2)  # dict[str, Baz]
reveal_type(foo.int_double_map)  # dict[int, float]
reveal_type(foo.int_int_map)  # dict[int, int]
reveal_type(foo.int_bool_map)  # dict[int, bool]
reveal_type(foo.int_string_map)  # dict[int, str]
reveal_type(foo.int_bytes_map)  # dict[int, bytes]
reveal_type(foo.int_bar_message_map_1)  # dict[int, Bar]
reveal_type(foo.int_bar_message_map_2)  # dict[int, Bar]
reveal_type(foo.int_baz_enum_map_1)  # dict[int, Baz]
reveal_type(foo.int_baz_enum_map_2)  # dict[int, Baz]

I'm wondering if there would be any interest in me upstreaming these annotations? I think they provide a decent bit of value for consumers of the library and upstreaming would bring that benefit to a lot more users. If there is, I'd be happy to open up a PR and work with you all to land this.

@henribru henribru added priority: p3 Desirable enhancement or fix. May not be included in next release. type: question Request for information or clarification. Not an issue. labels Nov 15, 2023
@parthea
Copy link
Contributor

parthea commented Nov 21, 2023

Hi @henribru,

These changes would be helpful. A PR is welcome!

@parthea parthea added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed type: question Request for information or clarification. Not an issue. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Nov 21, 2023
@henribru
Copy link
Author

Finally got around to this, but I've discovered a slight snag. https://github.com/googleapis/gapic-generator-python generates type annotations for fields. like property: str = proto.Field(proto.STRING, number=1). That means that this is present in the messages of all the packages in https://github.com/googleapis/google-cloud-python, as well as in https://github.com/googleads/google-ads-python.

The issue with this is that in my stubs, proto.Field(proto.STRING, number=1) returns a Field[str], and then there's some descriptor magic to make sure that it's a str when you access it on a message instance. It wouldn't be possible for it to directly return str unless I trick the typechecker to think Field is a function, typecheckers don't allow class constructors to return arbitrary types. But Field[str] and str are incompatible, which means property: str = proto.Field(proto.STRING, number=1) is technically a type error. This isn't actually a problem for consumers of these libraries, because the type annotation will take precedence and the typechecker won't produce an error about 3rd party code. But if you run the typechecker directly on the library, you would see a type error on that assignment. The fix would be to annotate it with Field[str] or drop the annotation entirely (since it's not really necessary with proper stubs for proto-plus), but that would require changing gapic-generator-python. Though given that these libraries are generated, I'm not sure if anyone would actually run a typechecker on them directly? But I suppose there could be handwritten code out there which does the same kind of annotation and if they use a typechecker for development it would be a breaking change for them, typing-wise.

Not sure if this is a deal-breaker. In the current situation where there's a separate stub package this issue is opt-in, but it wouldn't be if it was integrated in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants