false negative for protected-access in pylint 2.4.0 (new from 2.3.1)

[jazelenk]

protected-access violations previously detected by 2.3.1 are not detected by 2.4.0

Steps to reproduce

Run pylint versus sample.py, which is:

$ cat sample.py
'''
Example that demonstrates a protected access incorrectly not flagged
'''

class Object1():
    def __init__(self):
        self._private = 0

    def __str__(self):
        return str(self._private)

class Object2():
    def __init__(self, example):
        self.public = example._private
        self._private = example._private

    def __str__(self):
        return str(self.public) + str(self._private)

def main():
    object1 = Object1()
    print(object1)
    object2 = Object2(object1)
    print(object2)

if __name__ == '__main__':
    main()
    raise SystemExit(0)

pylint 2.3.1 detects this:

$ pylint --version
pylint 2.3.1
astroid 2.3.0
Python 3.7.3 (default, Apr  3 2019, 19:16:38)
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]]
(CLFSLoad_venv) atz5d8a2db350-linux-1:/jazelenk/src/clfsload.10$ pylint sample.py
************* Module sample
sample.py:14:22: W0212: Access to a protected member _private of a client class (protected-access)
sample.py:15:24: W0212: Access to a protected member _private of a client class (protected-access)

------------------------------------------------------------------
Your code has been rated at 8.95/10 (previous run: 8.95/10, +0.00)

pylint 2.4.0 does not:

$ pylint --version
pylint 2.4.0
astroid 2.3.0
Python 3.7.3 (default, Apr  3 2019, 19:16:38)
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]]
(CLFSLoad_venv) atz5d8a2db350-linux-1:/jazelenk/src/clfsload.10$ pylint sample.py

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 8.95/10, +1.05)

I'm not sure if this is related to #2959

[AWhetter]

Looks like this was caused by a change made in 5309e1d to address #1802. It reads like this message was to be disabled only when we knew the type of the object with the attribute being accessed as the same as the current class, but it doesn't look like that's the case.

Perhaps @hippo91 knows more?

[hippo91]

@jazelenk thanks for the report.
In fact pylint doesn't emit protected-access when the protected attribute is accessed inside special methods as it is the case here. This is to prevent false positive when using for example __eq__ where you may need to access protected member of the object you compare to.
However if the protected access is not inside special method, then pylint emit the message. For example consider the code below:

#pylint: disable=missing-docstring, too-few-public-methods
'''
Example that demonstrates a protected access incorrectly not flagged
'''

class Object1():
    def __init__(self):
        self._private = 0

    def __str__(self):
        return str(self._private)

class Object2():
    def __init__(self, example):
        self.public = example._private
        self._private = example._private

    def __str__(self):
        return str(self.public) + str(self._private)

    def _fake_special(self, example):
        self.public = example._private

def main():
    object1 = Object1()
    print(object1)
    object2 = Object2(object1)
    print(object2)

if __name__ == '__main__':
    main()
    raise SystemExit(0)

the when we run pylint against it we got :

bug_pylint_3120.py:22:22: W0212: Access to a protected member _private of a client class (protected-access)

As it seems there is no issue here, i close this issue. If i'm wrong or if those explanations are not clear feel free to reopen it.

[jazelenk]

I agree that you are describing the current behavior, which is new in 2.4.0. That was not the behavior in older versions. I understand the goal, but this happens to work against my use cases. Perhaps allowing private access should be limited to double-underscore names, rather than any private name - that would allow access to __eq__ et al without allowing access to private methods/atttributes like _private.

[hippo91]

@jazelenk i'm not sure i understand you.
To compare one object to the other you may need to have access to all of its member. Those prefixed with zero, one or two underscores. That means that, for example, in __eq__ methods pylint cannot filter access based on the number of underscores that prefixed the attribute. That's why in all special methods pylint doesn't warn about accessing private members.

[jazelenk]

I agree, but I think that the exceptions carved out right now are overly broad. In the example I show, Object1 and Object2 are sibling classes, yet in the __init__ for Object2, it accesses a private attribute of Object1. Older versions of pylint correctly flagged this, but newer versions do not.

[hippo91]

@jazelenk there have been a long discussion on #1802 and current pylint's behavior is the result of such discussion.
From what i can understand the behavior you desired (i.e the old one) is in contradiction with #1802.
To be honest i don't see how we can improve it on this particular point and have pylint fullfill requirement of #1802 and this one. However i will be happy to review a PR if you are interested to handle this.
Maybe @AWhetter or @PCManticore will have better understanding.

[AWhetter]

@hippo91 Do you think that we can add a new option to the checker to choose between the old and the new behaviour?

The original discussion called for disabling the message when in a magic method and when we know that the object with the private attribute being accessed has the same type as the object with the magic method being run. I don't think our current implementation does this, so should we create an issue to add this improvement in the future?

[hippo91]

@AWhetter the idea to have an option is interesting. I'll try to look at this ASAP.

In the PR #2959 i tried to detect if there was a isinstance test before accessing private attribute. At that time @PCManticore told the following:
The implementation would be a lot simpler if we would just skip emitting protected-access inside a special method, assuming the person that wrote the code already handled the case of accessing the attribute on a same instance.
I found it made sense. And to be honest i still find it make sense.

[AWhetter]

The implementation would be a lot simpler if we would just skip emitting protected-access inside a special method, assuming the person that wrote the code already handled the case of accessing the attribute on a same instance.

That's true but I think the point of pylint is to highlight areas of code that could cause problems either because someone is not aware of the problem or someone knows about it but might have missed it when writing this code.
Either way it probably won't help solve this issue really and allowing users to choose between behaviours with an option seems like the better way to go 👍

[hippo91]

@Whetter i reopen the issue. I'll try to work on this ASAP.