pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.32k stars 1.14k forks source link

remove metaclass check for protected access in properties #9966

Closed temyurchenko closed 1 month ago

temyurchenko commented 1 month ago
  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: https://github.com/pylint-dev/astroid/pull/2553

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pylint-dev/pylint/pull/9966/graphs/tree.svg?width=650&height=150&src=pr&token=ZETEzayrfk&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev)](https://app.codecov.io/gh/pylint-dev/pylint/pull/9966?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev) ```diff @@ 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](https://app.codecov.io/gh/pylint-dev/pylint/pull/9966?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev) | Coverage Δ | | |---|---|---| | [pylint/checkers/classes/class\_checker.py](https://app.codecov.io/gh/pylint-dev/pylint/pull/9966?src=pr&el=tree&filepath=pylint%2Fcheckers%2Fclasses%2Fclass_checker.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev#diff-cHlsaW50L2NoZWNrZXJzL2NsYXNzZXMvY2xhc3NfY2hlY2tlci5weQ==) | `93.73% <100.00%> (-0.03%)` | :arrow_down: |
github-actions[bot] commented 1 month ago

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

This comment was generated for commit 4f936d3e867a428acf8a7a6d223585f5574eb025

temyurchenko commented 1 month ago

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

temyurchenko commented 1 month ago

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 commented 1 month ago

It's been introduced in https://github.com/pylint-dev/pylint/pull/2393, there was a crash when an attribute was uninferable, we need to adapt the test to keep checking for that maybe ?

temyurchenko commented 1 month ago

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.

github-actions[bot] commented 1 month ago

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

This comment was generated for commit f2591c7a42a6adc2d4f532cb9a656e2e6f3d0cc0