Add support for proto2 extensions #145

EPronovost · 2020-09-26T02:19:50Z

Adds support for message extensions. Now if there is a message

message Bar {
  extend Foo { optional Bar ext = 2020; }
  
  optional bool flag = 1;
}

then Bar.ext will no longer raise a mypy error. Furthermore, we are able to correctly type extension messages, so for something like

foo = Foo()
bar = foo.Extensions[Bar.ext]

the inferred type for bar is Bar.

nipunn1313

This looks great! Couple of comments around the testing, then we can go with it.

Trying to think of how we can add a negative test here - to confirm that this mypy typing catches bugs - has better typing than Any. Perhaps in test_negative.py we can add something of the sort

x = 5
x = Bar.ext   # E

Where we'd expect a type error of a FieldDescriptor being assigned to an int.

test/test_generated_mypy.py

nipunn1313 · 2020-09-30T04:47:32Z

Thank you so much for the contribution!
Must mention that Dropbox requires that you agree to the CLA before it can be accepted https://opensource.dropbox.com/cla/

Thank you!

nipunn1313 · 2020-09-30T04:52:21Z

Would also ask that you update CHANGELOG.md with the new change in the "Upcoming" section

EPronovost · 2020-10-01T18:02:47Z

The PR got more complicated, but we now have correct type inference for extensions.

nipunn1313 · 2020-10-04T02:34:32Z

Hmm - looks like this diff has developed some additional complexity for typing the extensions dict - and potentially has some points up for debate. If you were to split the PR into uncontroversial part (w/o typing extensions dict) - we can move forward with that easily, so we can focus discussion on this more controversial complexity and find the right way to solve it.

I don't want to hold up this entire diff on this discussion - since it's clearly adding value!
Thank you so much for doing this work!

EPronovost · 2020-10-04T03:57:00Z

@nipunn1313 I made a more minimal PR here: #149
What would you like to discuss about this PR?

nipunn1313

Sorry Forgot to hit submit on the comments! Here they are.

nipunn1313 · 2020-10-04T02:28:54Z

generated/_typeshed_mypy_protobuf.pyi.expected

+class _ExtensionDict(typing___Generic[_ContainerMessageT]):
+    def __getitem__(self, extension_handle: _ExtensionFieldDescriptor[_ContainerMessageT, _ExtenderMessageT]) -> _ExtenderMessageT: ...
+
+    def __setitem__(self, extension_handle: _ExtensionFieldDescriptor[_ContainerMessageT, _ExtenderMessageT], value: _ExtenderMessageT) -> None: ...


Can you explain what's going on here in this file?

1 - Why does this need to be autogenerated? Seems like it potentially should belong either directly in typeshed itself, or inside the mypy_protobuf library as something to be imported

2 - Can you explain what value this is adding over a Dict[ContainerMessage, ExtenderMessage]?

This file does indeed appear to add complexity, and I'm not sure I understand what value the complexity adds over a simple dict.

1 - Why does this need to be autogenerated? Seems like it potentially should belong either directly in typeshed itself, or inside the mypy_protobuf library as something to be imported

Yes I agree the autogeneration is not the best solution. I can think of 3 possibilities:

Generate a file as part of the protoc plugin (i.e. what we have currently). I changed it to use a template file instead of having python code to write the file on the fly. The benefit of this is that we can ensure these stubs will be present whenever mypy is run and don't add any new package requirements.

As part of the mypy_protobuf library. This simplifies the generated code and still ensures that the same version of the base classes used to generate stubs is used to evaluate them (assuming the user doesn't change library versions in between). However, I think it also means that mypy_protobuf would need to be present whenever mypy is run, which is not currently a requirement.

As part of the typeshed. This addresses the issue of needing mypy_protobuf present when running mypy. However, it means that we'd require the new mypy version to have these stubs be present. It would also pose difficulties if we want to change anything about this file in the future.

There may be a way to do either of the latter two options in a "soft" way; i.e. fallback to the minimal extension support already in master if the respective dependency is not present. I don't have a ton of experience publishing to the typeshed to know if/how they support API changes across package versions.

2 - Can you explain what value this is adding over a Dict[ContainerMessage, ExtenderMessage]

Two reasons. Message.Extensions is not a dictionary, and does not support the various other methods associated with one (e.g. you can't do foo.Extensions.values()). Additionally, I don't think using a static typing would allow for the polymorphism we currently have (i.e. _ExtensionDict is a class parametrized by _ContainerMessageT and __{gs}estitem__ is a method of that class parametrized by _ExtenderMessageT). The actual source code is here. I'm not sure why the typeshed implementation does not include the other methods; I added all the public ones here.

Ah! We can totally add these stubs to typeshed - as long as the stubs match the source code.
We have lots of precedent doing that (much of the google/protobuf typeshed stubs were contributed by dropboxers / folks who work on mypy-protobuf)

Source: https://github.com/protocolbuffers/protobuf/blob/e667bf6eaaa2fb1ba2987c6538df81f88500d030/python/google/protobuf/internal/extension_dict.py#L63

Typeshed: https://github.com/python/typeshed/tree/master/third_party/2and3/google/protobuf/internal
https://github.com/python/typeshed/tree/master/third_party/2and3/google/protobuf/internal

You can also test your typeshed change + mypy-protobuf change within mypy-protobuf by doing

CUSTOM_TYPESHED_DIR=~/path/to/your/typeshed/fork ./run_test.sh

Though one gotcha - when developing you should run the typeshed tests first - because errors within typeshed get suppressed when you use CUSTOM_TYPESHED_DIR.

It does mean that we'll have to wait for another release of mypy before we launch the next version of mypy-protobuf, but I think that's ok! #140 is similarly waiting on a typeshed change (you can look at that as an example)

Interesting... I didn't realize the typeshed stubs for protobuf came from this same group (thank you!). Are there any further comments you have on the current code before moving typeshed.pyi to the official typeshed?

python/mypy_protobuf.py

test_negative/negative.py

python/setup.py

nipunn1313 · 2020-10-05T21:19:18Z

Here are some examples of updates to typeshed for sake of mypy-protobuf
https://github.com/python/typeshed/pull/4265/files
https://github.com/python/typeshed/pull/3996/files
https://github.com/python/typeshed/pull/3995/files

EPronovost · 2020-10-06T05:02:13Z

This will be blocked on python/typeshed#4619

nipunn1313 · 2020-10-07T16:16:46Z

the typeshed change landed, and I landed a mypy->typeshed pin bump afterward!

I spoke with the mypy maintainers (they're also at Dropbox), and they mentioned that it may be until around November before there's a mypy release w/ your change.
Feel free to update https://github.com/dropbox/mypy-protobuf/blob/master/run_test.sh#L36 to a git revision to unblock your change. In November (or whenever), I'll change it back before releasing mypy-protobuf

From

python3 -m pip install git+https://github.com/python/[email protected]

to

python3 -m pip install git+https://github.com/python/mypy.git@7273e9ab1664b59a74d9bd1d2361bbeb9864b7ab

referencing python/mypy@7273e9a

nipunn1313 · 2020-10-08T14:04:45Z

test/test_generated_mypy.py

-    # correct base model. This fails at runtime but passes mypy.
+    for x in s1.Extensions:
+        assert isinstance(x, FieldDescriptor)
+        y = s1.Extensions[x]  # y should be typed as an AnyMessage


This comment might leave some confusion as to the why, I think more specifically, explaining what "x" should be typed as will help understand why "y" is an Any here.

Specifically, to add clarity that y is Any because x came out of an iterator, which can't know the value type.

nipunn1313 · 2020-10-08T14:05:54Z

This is awesome!!! Thank you so much for the contributions here.

I left one message about a comment in the tests. Going to go ahead and merge it - feel free to add another PR to update the comment if you're excited.

Add support for proto2 extensions

14f0e0e

nipunn1313 requested changes Sep 30, 2020

View reviewed changes

test/test_generated_mypy.py Outdated Show resolved Hide resolved

test/test_generated_mypy.py Outdated Show resolved Hide resolved

EPronovost and others added 4 commits October 1, 2020 10:50

Make extensions a generic class

f853ab7

Add info to mypy output from run_test.sh

c372c80

Update docs

9695409

Merge branch 'master' into epronovost/extensions

7e23057

EPronovost mentioned this pull request Oct 4, 2020

Minimal implementation of message extensions #149

Merged

nipunn1313 reviewed Oct 4, 2020

View reviewed changes

EPronovost added 3 commits October 4, 2020 08:48

Merge branch 'master' into epronovost/extensions

4f80c31

Move typeshed to separate file

861b7d7

Move typeshed to separate file

4d1e91e

EPronovost commented Oct 5, 2020

View reviewed changes

python/setup.py Outdated Show resolved Hide resolved

Update to use new typeshed

799ca82

EPronovost mentioned this pull request Oct 6, 2020

Update protobuf ExtensionDict python/typeshed#4619

Merged

Update mypy version

d3e7379

nipunn1313 reviewed Oct 8, 2020

View reviewed changes

nipunn1313 merged commit 0b71cbe into nipunn1313:master Oct 8, 2020

EPronovost mentioned this pull request Oct 9, 2020

Update test comment #150

Merged

nipunn1313 mentioned this pull request Jul 14, 2021

proto3 Extensions defined at module level produce bad mypy ExtensionDict types #244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for proto2 extensions #145

Add support for proto2 extensions #145

EPronovost commented Sep 26, 2020 •

edited

Loading

nipunn1313 left a comment

nipunn1313 commented Sep 30, 2020

nipunn1313 commented Sep 30, 2020

EPronovost commented Oct 1, 2020

nipunn1313 commented Oct 4, 2020

EPronovost commented Oct 4, 2020

nipunn1313 left a comment

nipunn1313 Oct 4, 2020

EPronovost Oct 5, 2020

nipunn1313 Oct 5, 2020

EPronovost Oct 6, 2020

nipunn1313 commented Oct 5, 2020

EPronovost commented Oct 6, 2020

nipunn1313 commented Oct 7, 2020

nipunn1313 Oct 8, 2020

nipunn1313 commented Oct 8, 2020

Add support for proto2 extensions #145

Add support for proto2 extensions #145

Conversation

EPronovost commented Sep 26, 2020 • edited Loading

nipunn1313 left a comment

Choose a reason for hiding this comment

nipunn1313 commented Sep 30, 2020

nipunn1313 commented Sep 30, 2020

EPronovost commented Oct 1, 2020

nipunn1313 commented Oct 4, 2020

EPronovost commented Oct 4, 2020

nipunn1313 left a comment

Choose a reason for hiding this comment

nipunn1313 Oct 4, 2020

Choose a reason for hiding this comment

EPronovost Oct 5, 2020

Choose a reason for hiding this comment

nipunn1313 Oct 5, 2020

Choose a reason for hiding this comment

EPronovost Oct 6, 2020

Choose a reason for hiding this comment

nipunn1313 commented Oct 5, 2020

EPronovost commented Oct 6, 2020

nipunn1313 commented Oct 7, 2020

nipunn1313 Oct 8, 2020

Choose a reason for hiding this comment

nipunn1313 commented Oct 8, 2020

EPronovost commented Sep 26, 2020 •

edited

Loading