remove metaclass check for protected access in properties

[temyurchenko]

1. it was incorrect when the property wasn't inside a class and the enclosing class didn't have an explicit metaclass

2. The conjugated test is full with errors and I can't understand the intention. (the errors: MC doesn't inherit type; _nargs isn't defined; obj isn't defined, etc). What is the pointed of testing protected access on an undefined variable?..

Description

The PR is intentionally extreme (removing the whole test), and I wouldn't be surprised if I missed something and my change is incorrect. I'd be happy to adapt based on the feedback.

The primary reason for this PR is marked as 1. in the commit description (or above).

One situation where this happens is the following. In new astroid "function-call" style properties don't have the enclosing class as the parent. The reason is that the properties themselves are not defined within the class, they are defined within some builtin module. It's just the name that they are assigned to that is defined within the class. So, for example in:

class Foo:
  x = property(lambda self: 5)

x is defined within Foo, but the property itself isn't. You can read more about it here: pylint-dev/astroid#2553

[DanielNoord]

Can you just remove the property and see if the tests still passes?

[temyurchenko]

If I comment out @property, it does pass

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (fa64425) to head (f2591c7).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9966      +/-   ##
==========================================
- Coverage   95.80%   95.80%   -0.01%     
==========================================
  Files         174      174              
  Lines       18937    18933       -4     
==========================================
- Hits        18143    18139       -4     
  Misses        794      794              

Files with missing lines	Coverage Δ
pylint/checkers/classes/class_checker.py	93.73% <100.00%> (-0.03%)	⬇️

[temyurchenko]

I believe we should «skip news» on this one.

[Pierre-Sassoulas]

Thank you for the PR. Great fan of simplifying the code, not a great fan of removing tests when simplifying the code.

[temyurchenko]

Thank you for the PR. Great fan of simplifying the code, not a great fan of removing tests when simplifying the code.

I'd be happy to fix the test instead, but I genuinely don't understand what it's supposed to be doing. It seems to be broken in too many ways to be useful and the intent is unclear as well. I'm not even sure if the behaviour it's testing is correct.

[Pierre-Sassoulas]

It's been introduced in #2393, there was a crash when an attribute was uninferable, we need to adapt the test to keep checking for that maybe ?

[temyurchenko]

It's been introduced in #2393, there was a crash when an attribute was uninferable, we need to adapt the test to keep checking for that maybe ?

I've spent a lot of time trying to find a single path that would lead to an exception described in that issue. I actually kind of began to understand why the test in question is so complicated. Hitting the unhappy path is incredibly involved (and vulnerable to even slight changes in the codebase). So, even when I've removed the fix from that PR, every single test is still passing, including the test in question.

So, I think it really makes sense to remove the test altogether. It was testing the quirks of a database from 6 years ago, and it doesn't apply nowadays.

[DanielNoord]

@Pierre-Sassoulas Okay to merge this?

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit f2591c7