[Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility #8182

nipunn1313 · 2021-01-05T13:10:33Z

This should provide a runtime alias, which can be
used in mypy stubs to provide better typing for enum values

This is one potential solution to #8175 - see discussion there.
Fixes #8175

google-cla · 2021-01-05T13:10:39Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

nipunn1313 · 2021-01-05T13:14:32Z

@googlebot I signed it!

nipunn1313 · 2021-03-31T19:33:55Z

Hi! Ping on this^

2m · 2021-04-29T13:17:35Z

This would allow us to cleanup some code as well. :)

StefPtrs · 2021-06-30T19:02:49Z

@nipunn1313 can you please add a label? It seems to be the only missing thing before this can be merged.

perezd · 2021-06-30T19:14:33Z

@nipunn1313 can you please add a label? It seems to be the only missing thing before this can be merged.

This is quite old, so I'm gonna run the tests to make sure it's still viable.

nipunn1313 · 2021-06-30T19:20:09Z

Hi! I'm not an official contributor on the project, so it was not clear to me how to add a label. I can rebase it to make sure things still work! Thanks for taking a look.

perezd · 2021-06-30T19:59:38Z

Hi! I'm not an official contributor on the project, so it was not clear to me how to add a label. I can rebase it to make sure things still work! Thanks for taking a look.

No prob! I am :)

Looks like all the builds failed, if you wanna fix up this PR I'll have someone take a look at it.

nipunn1313 · 2021-06-30T21:16:59Z

I believe it needs a label "release notes: yes" - so that it's automatically added to release notes. Can you run the tests once again (I rebased onto master and updated the PR).

I don't have the ability to approve running workflows:

First-time contributors need a maintainer to approve running workflows. Learn more.

nipunn1313 · 2021-07-03T21:23:39Z

hi - it appears that the Kokoro tests are failing on master. I spun up a dev environment on linux and confirmed that

bazel test //:build_files_updated_unittest

is failing

I submitted a separate patch to address that in #8789
As far as I can tell, I don't think the failures we're seeing here are related to my patch.

nipunn1313 · 2021-07-04T00:36:37Z

as far as I can tell from poking around there are at least a handful of other unrelated build issues that exist on master.

amauryfa · 2021-07-06T12:34:40Z

python/google/protobuf/internal/enum_type_wrapper.py

+  # to be typed with more constraint
+  # Eg.
+  # def Name(self, number: MyGeneratedEnum.V) -> str
+  V = int


I'd prefer a more explicit name like ValueType

Hi - since this is for compatibility with https://github.com/dropbox/mypy-protobuf - this is a bit of a large non-backward compatible suggestion unfortunately.

It has the ergonomic downside of having very lengthy type names that don't end up being much more useful (IMO). In either case, you get an error from mypy of the form:

incompatible type "MyGeneratedEnum"; expected "ValueType" incompatible type "MyGeneratedEnum"; expected "V"

both of which require some amount of looking inside the generated mypy-protobuf .pyi to understand. I believe there is a common confusion amongst protobuf users that MyGeneratedEnum is also an int, when protobuf generates it as an EnumTypeWrapper. I'm hoping that mypy-protobuf + this change helps avoid bugs related to that confusion.

Here's an example .proto enum (OuterEnum) https://github.com/dropbox/mypy-protobuf/blob/master/proto/testproto/test.proto#L11
And the corresponding generated .pyi in mypy-protobuf OuterEnum.V https://github.com/dropbox/mypy-protobuf/blob/master/test/generated/testproto/test_pb2.pyi.expected#L25

mypy-protobuf has this documented here https://github.com/dropbox/mypy-protobuf#types-enum-int-values-more-strongly.

If we do want to change the V to something else, it could be possible via a compatibility version supporting both V and ValueType types and eventually dropping support for V. I know Dropbox alone has thousands of instances of V already, as do other companies/codebases using mypy+protobuf. I'm hesitant to do such a change without some strong feedback + consensus - to avoid churning on name changes here.

This is an unfortunate situation... and the approach looks a bit backwards to me: mypy should not require changes to Python or common libraries.

But if I got this correctly, all current usages need quotes, as in: def f(x: 'MyEnum.V') and this V is provided by mypy-protobuf generated code.
Since removing the quotes is a change anyway, the recommended end state could be def f(x: MyEnum.ValueType) (no quotes, but longer name), and the V would be supported by mypy as an alias (shorter, but with quotes)

This is an unfortunate situation... and the approach looks a bit backwards to me: mypy should not require changes to Python or common libraries.

Agreed.

The reason this comes up is because mypy-protobuf is attempting to get stronger typing than what protobuf itself generates at runtime. Protobuf generates all enum values as int. The stronger typing has proven to catch bugs and is one of the value adds of mypy-protobuf, but in order to namespace the NewType("ValueType", int) into MyEnum, it requires this change.

Starting with python 3.10 there is a new feature for postponed evaluation of annotations
https://www.python.org/dev/peps/pep-0563/
(or starting in python 3.7 with from __future__ import annotations)
However, there are still cases where this is insufficient to avoid evaluation - notably

from typing import cast cast(MyEnum.ValueType, x)

Since removing the quotes is a change anyway, the recommended end state could be def f(x: MyEnum.ValueType) (no quotes, but longer name), and the V would be supported by mypy as an alias (shorter, but with quotes)

I'm happy with your suggestion for a backward-compatible way to move toward a more verbose name. I can work with that in mypy-protobuf. @amauryfa - are you a review authority here? I can make the change. Just want to make sure I avoid discussion churn on these sorts of points.

Side note: There is an alternate python generated code option that sidestepped this problem entirely by generating enum values as enums https://github.com/danielgtaylor/python-betterproto. I've never tried it.

Updated the diff with this idea of s/V/ValueType

@perezd can you approve running to workflow again?

This should provide a runtime alias, which can be used in mypy stubs to provide better typing for enum values Fixes protocolbuffers#8175

acozzette · 2021-10-21T22:10:10Z

@nipunn1313 Thanks!

nipunn1313 · 2021-10-21T23:07:10Z

Cool. Just wanted to double check before this releases - are you happy with the name MyEnum.ValueType - as it would be challenging to change this name in the future. (eg mypy-protobuf currently uses MyEnum.V and I'll have to make that change). I'm happy with this naming scheme for the type.

If people have preexisting proto enums with a field named ValueType, their existing python calling code might need to be modified - but I think that's a reasonable change to ask customers to make. Technically not backward compatible, but it's a pretty rare scenario.

Once the next major release of protobuf comes out with this, I'll modify mypy-protobuf to work with this!
Appreciate it.

acozzette · 2021-10-25T17:56:24Z

@anandolee and @haberman, does this naming sound ok?

See protocolbuffers/protobuf#8182 which will make it into protobuf 3.20.0 Have to use a `V = Union[ValueType]` hax, because `V = ValueType` is interpreted as a value rather than alias `V: TypeAlias = ValueType` appears buggy within nested scopes. Fixes #169

google-cla bot added the cla: no label Jan 5, 2021

google-cla bot added cla: yes and removed cla: no labels Jan 5, 2021

nipunn1313 changed the title ~~Provide a type alias V on EnumTypeWrapper~~ [Python] Provide a type alias V on EnumTypeWrapper Jan 5, 2021

This was referenced Jan 31, 2021

Better mypy typing for enum values in python #8175

Closed

Upstream V = int to protoc's EnumTypeWrapper runtime nipunn1313/mypy-protobuf#169

Closed

MHDante mentioned this pull request Mar 9, 2021

Unknown type when using a cyclical reference to an inner type in stub. microsoft/pyright#1602

Closed

nipunn1313 force-pushed the enum_type_wrapper_value_field branch from c55ee08 to c98029d Compare May 27, 2021 22:29

perezd added python kokoro:run labels Jun 30, 2021

protobuf-kokoro removed the kokoro:run label Jun 30, 2021

nipunn1313 force-pushed the enum_type_wrapper_value_field branch from c98029d to 4e45b46 Compare June 30, 2021 21:09

nipunn1313 changed the title ~~[Python] Provide a type alias V on EnumTypeWrapper~~ [Python] Provide a type alias V on EnumTypeWrapper for mypy-protobuf compatibility Jun 30, 2021

perezd added release notes: yes labels Jun 30, 2021

protobuf-kokoro removed the kokoro:run label Jun 30, 2021

amauryfa suggested changes Jul 6, 2021

View reviewed changes

Provide a type alias field ValueType on EnumTypeWrapper

428ef16

This should provide a runtime alias, which can be used in mypy stubs to provide better typing for enum values Fixes protocolbuffers#8175

nipunn1313 force-pushed the enum_type_wrapper_value_field branch from 4e45b46 to 428ef16 Compare July 21, 2021 21:51

haberman added the kokoro:run label Sep 1, 2021

protobuf-kokoro removed kokoro:run labels Sep 1, 2021

acozzette changed the title ~~[Python] Provide a type alias V on EnumTypeWrapper for mypy-protobuf compatibility~~ [Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility Oct 21, 2021

acozzette merged commit 0707f2e into protocolbuffers:master Oct 21, 2021

nipunn1313 mentioned this pull request Oct 30, 2021

Convert V to ValueType nipunn1313/mypy-protobuf#306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility #8182

[Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility #8182

nipunn1313 commented Jan 5, 2021

google-cla bot commented Jan 5, 2021

nipunn1313 commented Jan 5, 2021

nipunn1313 commented Mar 31, 2021

2m commented Apr 29, 2021

StefPtrs commented Jun 30, 2021

perezd commented Jun 30, 2021

nipunn1313 commented Jun 30, 2021

perezd commented Jun 30, 2021

nipunn1313 commented Jun 30, 2021 •

edited

Loading

nipunn1313 commented Jul 3, 2021

nipunn1313 commented Jul 4, 2021

amauryfa Jul 6, 2021

nipunn1313 Jul 6, 2021 •

edited

Loading

nipunn1313 Jul 7, 2021

amauryfa Jul 7, 2021

nipunn1313 Jul 7, 2021

nipunn1313 Jul 21, 2021

StefPtrs Jul 26, 2021

acozzette commented Oct 21, 2021

nipunn1313 commented Oct 21, 2021

acozzette commented Oct 25, 2021

[Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility #8182

[Python] Provide a type alias ValueType on EnumTypeWrapper for mypy-protobuf compatibility #8182

Conversation

nipunn1313 commented Jan 5, 2021

google-cla bot commented Jan 5, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

nipunn1313 commented Jan 5, 2021

nipunn1313 commented Mar 31, 2021

2m commented Apr 29, 2021

StefPtrs commented Jun 30, 2021

perezd commented Jun 30, 2021

nipunn1313 commented Jun 30, 2021

perezd commented Jun 30, 2021

nipunn1313 commented Jun 30, 2021 • edited Loading

nipunn1313 commented Jul 3, 2021

nipunn1313 commented Jul 4, 2021

amauryfa Jul 6, 2021

Choose a reason for hiding this comment

nipunn1313 Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

nipunn1313 Jul 7, 2021

Choose a reason for hiding this comment

amauryfa Jul 7, 2021

Choose a reason for hiding this comment

nipunn1313 Jul 7, 2021

Choose a reason for hiding this comment

nipunn1313 Jul 21, 2021

Choose a reason for hiding this comment

StefPtrs Jul 26, 2021

Choose a reason for hiding this comment

acozzette commented Oct 21, 2021

nipunn1313 commented Oct 21, 2021

acozzette commented Oct 25, 2021

nipunn1313 commented Jun 30, 2021 •

edited

Loading

nipunn1313 Jul 6, 2021 •

edited

Loading