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

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

Closed
pcorpet opened this issue Jul 13, 2021 · 11 comments · Fixed by #245 or #248
Closed

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

pcorpet opened this issue Jul 13, 2021 · 11 comments · Fixed by #245 or #248

Comments

@pcorpet
Copy link
Contributor

pcorpet commented Jul 13, 2021

I'm trying the new versions of mypy-protobuf 2.6 together with types-protobuf 3.17.0 and I'm getting some errors when using extensions.

syntax = "proto3";

extend google.protobuf.FieldOptions {
  // Give a bit more semantics to a field so that we can programatically
  // identify sensitive fields to handle with care.
  FieldUsage field_usage = 50000;
}

enum FieldUsage {
  // Many fields are not tagged, this is either because they have not been
  // tagged yet or because they define the user's project and as such do not
  // need special care yet. In the future we might add a tag for those if there
  // is a need.
  UNKNOWN_FIELD_USAGE = 0;

  // A field that may uniquely identify a user.
  PERSONAL_IDENTIFIER = 1;

  // A field that is only used for the app's UX or UI: it does not contain
  // actual user data.
  APP_ONLY = 2;

  // A field whose value is provided by the user as a feedback.
  USER_FEEDBACK = 3;

  // A field populated by the app as a result of an algorithm.
  ALGORITHM_RESULT = 4;
}
        extensions = field_descriptor.GetOptions().Extensions
        field_usage = extensions[options_pb2.field_usage]

Here's what mypy tells me for line 1 and 2 (respectively)

Need type annotation for "field_usage"
Invalid index type "FieldDescriptor" for "_ExtensionDict[FieldOptions]"; expected type "_ExtensionFieldDescriptor[FieldOptions, <nothing>]"

I've dug a bit and it seems that mypy-protobuf types options_pb2.field_usage as a generic google.protobuf.descriptor.FieldDescriptor instead of having a _ExtensionFieldDescriptor[FieldOptions, options_pb2.FieldUsage>].

Does someone else have this issue? Should I change the generated type?

@nipunn1313
Copy link
Owner

nipunn1313 commented Jul 13, 2021 via email

@nipunn1313
Copy link
Owner

@EPronovost added some support for better typing on extension fields in proto2, but it seems like it may have broken proto3 - I can see the issue you're describing.

The extensions change landed in mypy-protobuf 1.24 (see CHANGELOG.md)

See #145 and #154
If you happen to know what version things break for you, that sounds helpful.

Trying to write some tests for the issue you're seeing and coming up w/ some kind of patch

@nipunn1313
Copy link
Owner

ok I think I've isolated the issue
Appears to work with types-protobuf 0.1.14 https://pypi.org/project/types-protobuf/#history but not 3.17.0

I think python/typeshed#5754 may have been premature, as it appears that there is still use cases for indexing by FieldOptions rather than via _ExtensionFieldDescriptor

I believe the issue is that extensions scoped within messages are supported, but ones at global scope were overlooked

message Extensions1 {
  // works
  extend Simple1 { optional Extensions1 ext = 1000; }

  optional string ext1_string = 1;
}

// doesn't work - still typed as FieldDescriptor
extend google.protobuf.FieldOptions {
  string test_field_extension = 50000;
}

My plan is to spend a few minutes trying to fix forward the extension issue you're describing.
My backup plan if this isn't tractable quickly is to try and revert typeshed 5754, and longer term will look into ensuring full support for _ExtensionFieldDescriptor based indexing

quick workaround is to use types-protobuf 0.1.14

nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 14, 2021
Proto itself supports primitives, not just messages.
See nipunn1313/mypy-protobuf#244 for an example
motivating this change.

Test Plan: I was able to use MYPYPATH to test an updated version of
mypy-protobuf with this change.
@nipunn1313 nipunn1313 changed the title proto3 Extensions stopped working proto3 Extensions defined at module level produce bad mypy ExtensionDict types Jul 14, 2021
nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 14, 2021
Proto itself supports primitives, not just messages.
See nipunn1313/mypy-protobuf#244 for an example
motivating this change.

Test Plan: I was able to use MYPYPATH to test an updated version of
mypy-protobuf with this change.
nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 14, 2021
Proto itself supports primitives, not just messages.
See nipunn1313/mypy-protobuf#244 for an example
motivating this change.

Test Plan: I was able to use MYPYPATH to test an updated version of
mypy-protobuf with this change.
nipunn1313 added a commit that referenced this issue Jul 14, 2021
Update types-protobuf pin from typeshed

Depends on python/typeshed#5774
Fixes #244
@nipunn1313
Copy link
Owner

I was able to whip up fixes with python/typeshed#5774 and #245.
Going to see if we can quickly get this converted. With modular typeshed, https://mypy-lang.blogspot.com/2021/05/the-upcoming-switch-to-modular-typeshed.html - there are no longer big delays in this sort of thing!

JelleZijlstra pushed a commit to python/typeshed that referenced this issue Jul 14, 2021
Proto itself supports primitives, not just messages.
See nipunn1313/mypy-protobuf#244 for an example
motivating this change.

Test Plan: I was able to use MYPYPATH to test an updated version of
mypy-protobuf with this change.
@pcorpet
Copy link
Contributor Author

pcorpet commented Jul 14, 2021

Wow, thanks so much for the quick fix

nipunn1313 added a commit that referenced this issue Jul 14, 2021
Update types-protobuf pin from typeshed

Depends on python/typeshed#5774
Fixes #244
nipunn1313 added a commit that referenced this issue Jul 14, 2021
Update types-protobuf pin from typeshed

Depends on python/typeshed#5774
Fixes #244
nipunn1313 added a commit that referenced this issue Jul 14, 2021
Update types-protobuf pin from typeshed

Depends on python/typeshed#5774
Fixes #244
@nipunn1313
Copy link
Owner

Should be better now @pcorpet - you can try out the unreleased mypy-protobuf (instructions on README) to see if the issue is resolved for you. LMK - would be nice to get some validation before release that it's solving the issue for you.
I'm on slack (instructions on README) if you/your project/your company use/prefer it. Can do cross-workspace DMs there. I'm trying to build up a little community of folks using this project. Replying here is also fine.

@pcorpet
Copy link
Contributor Author

pcorpet commented Jul 15, 2021

It works way better thank you.

One last remaining factor is when the option is a repeated option:

syntax = "proto3";

extend google.protobuf.FieldOptions {
  // Give more information about the format of a string field.
  repeated StringFormat string_format = 50001;
}

enum StringFormat {
  // Many fields are not tagged, this is either because they have not been
  // tagged yet or because they do not have a specific format.
  UNKNOWN_STRING_FORMAT = 0;

  // A field whose value(s) are in natural language.
  NATURAL_LANGUAGE = 1;

  // A field whose value is a URL.
  URL_FORMAT = 2;

  // A field whose value is only a partial sentence, and as such should never start with an
  // uppercase letter or end with a punctuation mark.
  PARTIAL_SENTENCE = 3;
}

The the type of the extension shouldn't be StringFormat.V but a list of values.

error: "V" has no attribute "__iter__" (not iterable)

@nipunn1313
Copy link
Owner

Ah let me reopen and fix that issue as well.

@nipunn1313 nipunn1313 reopened this Jul 15, 2021
nipunn1313 added a commit that referenced this issue Jul 16, 2021
This should not change any of the generated code - simply refactoring
into a helper in order to unblock progress on #244 - which wants
to apply this to extensions.
nipunn1313 added a commit that referenced this issue Jul 16, 2021
This should not change any of the generated code - simply refactoring
into a helper in order to unblock progress on #244 - which wants
to apply this to extensions.
nipunn1313 added a commit that referenced this issue Jul 16, 2021
This should not change any of the generated code - simply refactoring
into a helper in order to unblock progress on #244 - which wants
to apply this to extensions.
nipunn1313 added a commit that referenced this issue Jul 16, 2021
This should not change any of the generated code - simply refactoring
into a helper in order to unblock progress on #244 - which wants
to apply this to extensions.
nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 16, 2021
See nipunn1313/mypy-protobuf#244 for the
inspiration for this. Repeated extensions are allowed by protobuf,
and they generate to extension values with repeated fields.

Notably map fields (ScalarMap and MessageMap) are NOT allowed to be
extension values - producing errors as such - so those are omitted.

testproto/test_extensions3.proto:19:6: Map fields are not allowed to be extensions.
nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 16, 2021
See nipunn1313/mypy-protobuf#244 for the
inspiration for this. Repeated extensions are allowed by protobuf,
and they generate to extension values with repeated fields.

Notably map fields (ScalarMap and MessageMap) are NOT allowed to be
extension values - producing errors as such - so those are omitted.

testproto/test_extensions3.proto:19:6: Map fields are not allowed to be extensions.
nipunn1313 added a commit to nipunn1313/typeshed that referenced this issue Jul 16, 2021
See nipunn1313/mypy-protobuf#244 for the
inspiration for this. Repeated extensions are allowed by protobuf,
and they generate to extension values with repeated fields.

Notably map fields (ScalarMap and MessageMap) are NOT allowed to be
extension values - producing errors as such - so those are omitted.

testproto/test_extensions3.proto:19:6: Map fields are not allowed to be extensions.
JelleZijlstra pushed a commit to python/typeshed that referenced this issue Jul 16, 2021
See nipunn1313/mypy-protobuf#244 for the
inspiration for this. Repeated extensions are allowed by protobuf,
and they generate to extension values with repeated fields.

Notably map fields (ScalarMap and MessageMap) are NOT allowed to be
extension values - producing errors as such - so those are omitted.

testproto/test_extensions3.proto:19:6: Map fields are not allowed to be extensions.
@pcorpet
Copy link
Contributor Author

pcorpet commented Jul 17, 2021

I confirm, that this is now working as expected for my codebase (using types-protobuf 3.17.3). Thanks @nipunn1313 !

@pcorpet
Copy link
Contributor Author

pcorpet commented Jul 20, 2021

Can you publish a new version of mypy-protobuf with this change please?

@nipunn1313
Copy link
Owner

Working on #227 - will try to complete that and then produce a new version. It is coming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants