nelfin / pylint-protobuf

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

Nested protobuf messages are not inspected by the plugin #4

Closed zapstar closed 5 years ago

zapstar commented 5 years ago

Consider the example:

# cat test.proto
syntax = "proto2";
package Test;

message inner {
    required string name = 1;
    optional uint32 number = 2;
}

message outer {
    required string name = 1;
    required inner inn = 2;
}

Here's the test harness:

# cat run.py
#!/usr/bin/env python
"""
Some module doc
"""
import test_pb2

def main():
    """
    Some function doc
    """
    x_a = test_pb2.outer()
    x_a.foo = 2  # Non-existent field in `outer`
    x_a.name = "Outer"
    x_a.inn.name1 = "Inner"  # Non-existent field in `inner`
    x_a.inn.number = 42
    print x_a

if __name__ == '__main__':
    main()

Here's the output of running PyLint

# pylint --load-plugins=pylint_protobuf run.py
No config file found, using default configuration
************* Module run
E: 12, 4: Field 'foo' does not appear in the declared fields of protobuf-generated class 'outer' and will raise AttributeError on access (protobuf-undefined-attribute)
E: 14, 4: Instance of 'outer' has no 'inn' member (no-member)
E: 15, 4: Instance of 'outer' has no 'inn' member (no-member)

--------------------------------------------------------------------
Your code has been rated at -5.00/10 (previous run: -5.00/10, +0.00)
nelfin commented 5 years ago

Hi @zapstar, thanks for the great report. I saw some similar behaviour with nested message definitions too:

message Outer {
  message Inner {
    required string value = 1;
  }
  required Inner inner = 1;
}

Both of these should be addressed by an upcoming refactor, but I'll add your test to make sure that this case gets caught.

zapstar commented 5 years ago

When are you planning to refactor? Is it already started? I'm trying to cook up something here on my own. Essentially, I plan to transform the module node itself with the needed fields. That way the no-member variable automatically goes away, so does the requirement of having a new error message.

nelfin commented 5 years ago

Halfway done, I just haven't been able to find as much time as I would have liked recently. I'll push up a branch if you'd like to follow along. Otherwise, your ideas definitely sound worthwhile, I look forward to hearing how you go.

zapstar commented 5 years ago

Please push them into a temporary branch, that'll be helpful. I'm building mostly on top of your parsing code. The core idea I have is to create an astroid ClassDef with fields attributes in it so that this gets parsed as a class in PyLint.

nelfin commented 5 years ago

Sorry about the wait @zapstar, you can find it at wip/vars-refactor.

seanwarren commented 5 years ago

Hello, I was wondering about the current status of this issue? I'm happy to help if that would be useful.

nelfin commented 5 years ago

Hey @seanwarren, I don't think I've made any progress on this. There's an xfail test in tests/test_checker.py::test_complex_field which you can work off. I'll take another stab at it over the weekend, but feel free to work on it as much as you'd like.

nelfin commented 5 years ago

A fix for this issue should be released as 0.6.dev1

nelfin commented 5 years ago

This issue should be fixed in 0.6