nelfin / pylint-protobuf

A plugin for making Pylint aware of the fields of protobuf-generated classes
MIT License
29 stars 12 forks source link

Fix nested messages #28

Closed nelfin closed 4 years ago

nelfin commented 4 years ago

@diana-infinitus-ai wrote in the comments of issue #21:

I recently updated the grpcio & grpcio-tools modules which caused nested enums and messages not being recognized anymore. I was able to trace down where it is failing (and a possible fix)

The new generated _pb2.py files have replaced:

_reflection.GeneratedProtocolMessageType('ProtoMessage', (_message.Message,), dict(
  DESCRIPTOR = _PROTOMESSAGE,
  __module__ = 'proto_message_pb2'
))

with

_reflection.GeneratedProtocolMessageType('ProtoMessage', (_message.Message,), {
  'DESCRIPTOR': _PROTOMESSAGE,
  '__module__': 'proto_message_pb2'
})

which seems innocuous, but because of how inner types are detected here https://github.com/nelfin/pylint-protobuf/blob/master/pylint_protobuf/parse_pb2.py#L193 (where it skips over all arguments that is not a Call type, but {} way of declaring dicts doesn't fall inside this) inner message types are no longer detected which causes incorrect protobuf-undefined-attribute errors.

This can be fixed by updating add_inner_types function to something like:

for arg in rhs.args:
    if type(arg) is not astroid.Call and type(arg) is not astroid.Dict:
        continue

    keywords = arg.keywords if type(arg) is astroid.Call else arg.items
    for kw in keywords:
        kwarg = kw.arg if hasattr(kw, 'arg') else kw[0].value
        if kwarg == 'DESCRIPTOR':
            try:
                name = kw.value.name if hasattr(
                    kw, 'value') else kw[1].name
...

(but maybe cleaner than what I hacked together during debugging?)

diana-infinitus-ai commented 4 years ago

All the tests continue to pass, but none of the tests that were previously failing now pass (tests are marked as strictly XFAIL in setup.cfg,, in short if a test marked XFAIL succeeds then it'll break the build to remind me that it's now working, see https://docs.pytest.org/en/stable/skipping.html#strict-parameter for more details).

What this means isn't "nothing changed", but nothing that was covered by the existing test suite changed, since you said that you experienced a bug, it would be good to include a new test that covers that case so we can check against regressions in the future.

sounds good, I'll write one up!

diana-infinitus-ai commented 4 years ago

Added a test for inner enum declaration with new generated pb2!

nelfin commented 4 years ago

I also checked that this test fails without the fix by cherry-picking it onto the latest release. Good work, LGTM.