Skip to content

Add/fix types to a bunch of non-generated protobuf files - #7017

Merged
srittau merged 4 commits into
python:masterfrom
nipunn1313:type_more_protobuf_things
Feb 2, 2022
Merged

Add/fix types to a bunch of non-generated protobuf files#7017
srittau merged 4 commits into
python:masterfrom
nipunn1313:type_more_protobuf_things

Conversation

@nipunn1313

@nipunn1313nipunn1313 commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

Used stubtest warnings as guidance as well as the source
https://github.com/protocolbuffers/protobuf/tree/master/python/google/protobuf

@nipunn1313
nipunn1313force-pushed the type_more_protobuf_things branch from 5a46e44 to ce42fd7CompareJanuary 24, 2022 08:47
@AlexWaygoodAlexWaygood mentioned this pull request Jan 24, 2022
Closed
@AlexWaygood

AlexWaygood commented Jan 24, 2022

Copy link
Copy Markdown
Member

I've just filed #7019 for some of the unrelated CI errors.

@JelleZijlstra

JelleZijlstra commented Jan 24, 2022

Copy link
Copy Markdown
Member

A number of other CI failures are real though.

@nipunn1313
nipunn1313force-pushed the type_more_protobuf_things branch from bc540f9 to 02a81c7CompareJanuary 24, 2022 17:10
@nipunn1313

nipunn1313 commented Jan 24, 2022

Copy link
Copy Markdown
ContributorAuthor

Looks better now! Thanks for prompt fix on the other things.

srittau
srittau reviewed Jan 25, 2022
View reviewed changes
Comment threadstubs/protobuf/google/protobuf/internal/containers.pyi Outdated
srittau
srittau requested changes Jan 25, 2022
View reviewed changes

@srittausrittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits below and one question, but LGTM otherwise, although my protobuf knowledge is limited.

Comment threadstubs/protobuf/google/protobuf/internal/enum_type_wrapper.pyi Outdated
Comment threadstubs/protobuf/google/protobuf/message.pyi Outdated
nipunn1313and others added 2 commits January 25, 2022 09:54
@nipunn1313@srittau
Use union syntax
5278d37
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
@nipunn1313
Use _ValueChecker protocol
b5cbdb4
nipunn1313
nipunn1313 commented Jan 25, 2022
View reviewed changes
Comment threadstubs/protobuf/google/protobuf/internal/type_checkers.pyi

_T = TypeVar("_T")

class _ValueChecker(Protocol[_T]):

@nipunn1313nipunn1313Jan 25, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put an underscore here since the protocol is not an actual element exposed within this module.

nipunn1313
nipunn1313 commented Jan 25, 2022
View reviewed changes
Comment threadstubs/protobuf/google/protobuf/internal/type_checkers.pyi

class TypeCheckerWithDefault(TypeChecker[_T]):
def __init__(self, default_value: _T, *acceptable_types: _T): ...
def DefaultValue(self) -> _T: ...

@nipunn1313nipunn1313Jan 25, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could assert that TypeCheckerWithDefault[_T] implements the protocol here.
I know there was some discussion on typing-sig to have this be supported in typecheckers

For now I checked manually.

AlexWaygood
AlexWaygood reviewed Feb 2, 2022
View reviewed changes
Comment threadstubs/protobuf/google/protobuf/internal/enum_type_wrapper.pyi Outdated
@srittau@AlexWaygood
Remove unused import
45a2256
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@srittau
srittau merged commit 90f5422 into python:masterFeb 2, 2022
@neruson

neruson commented Feb 3, 2022

Copy link
Copy Markdown

Hi @nipunn1313 and others, thank you for your excellent work! I had a question about this PR because my CI builds started failing with the upgrade from types-protobuf 3.19.7 to 3.19.8.

Specifically, this line is causing issues, because my code is assuming that DESCRIPTOR will not be None. https://github.com/nipunn1313/typeshed/blob/45a2256f397f9810944e8fccff53dc36471cb3ce/stubs/protobuf/google/protobuf/message.pyi#L14

I do see that the Message abstract base class has DESCRIPTOR = None, but there's a comment stating that a descriptor is expected in subclasses. https://github.com/protocolbuffers/protobuf/blob/v3.19.4/python/google/protobuf/message.py#L77-L78

I don't think None is actually a valid option for message.DESCRIPTOR, so I think it would be best to change this back and remove the | None. Does that sound reasonable?

@nipunn1313
nipunn1313 deleted the type_more_protobuf_things branch February 3, 2022 17:42
@nipunn1313

nipunn1313 commented Feb 3, 2022

Copy link
Copy Markdown
ContributorAuthor

Yep that makes sense to me @neruson. Make a PR + Add a comment explaining the non-nullable bit and I'll take a look!
I think you'll probably have to add it back to the stubtest_allowlist.txt - since stubtest will compare the Message base class for types.

In practicality, you're never actually instantiating the base class.

@asyksasyks mentioned this pull request Feb 3, 2022
Merged
pcorpet
pcorpet reviewed Feb 7, 2022
View reviewed changes
Comment threadstubs/protobuf/google/protobuf/message.pyi
def FromString(cls: type[Self], s: bytes) -> Self: ...
# The TypeVar must be bound to `Message` or we get mypy errors, so we cannot use `Self` for `Extensions`
@property
def Extensions(self: _M) -> _ExtensionDict[_M]: ...

@pcorpetpcorpetFeb 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi all, my CI starts failing because of some my code trying to get the extensions:

extensions=field_descriptor.GetOptions().Extensions

Where did that go? Is it invalid to get the Extensions that way?

@nipunn1313nipunn1313Feb 7, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool. I can repro this with the mypy-protobuf tests, but not with these tests.

I think stubtest got confused because the Message base class doesn't obviously have Extensions field (per this comment)
https://github.com/protocolbuffers/protobuf/blob/master/python/google/protobuf/message.py#L67

Here's a repro
nipunn1313/mypy-protobuf#346
https://github.com/nipunn1313/mypy-protobuf/runs/5095806933?check_suite_focus=true

I think we can bring this back.
I'd also like to look into running mypy-protobuf's testsuite from PR's to typeshed. I'll also look into that.

@nipunn1313nipunn1313Feb 7, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is in #7154

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygoodAlexWaygoodAlexWaygood left review comments
@srittausrittausrittau approved these changes
+1 more reviewer
@pcorpetpcorpetpcorpet left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nipunn1313@AlexWaygood@JelleZijlstra@neruson@srittau@pcorpet