Fix unsupported-binary-operation on classes that overload or

[timmartin]

Type of Changes

	Type
✓	🐛 Bug fix

Description

We have a specific checker looking for people using | with two types, apparently on the assumption that anyone doing that is mistakenly trying to use union syntax where it's not supported. This seems to overlook the fact that it is possible for class definitions to use a metaclass that implements __or__ or __ror__ as appropriate, which is the case in the permission classes for Django REST Framework, for example.

Closes #4951

[Pierre-Sassoulas]

Thank you for opening the merge request, this look promising :)

[timmartin]

I'm a bit confused about the functional tests. It looks like with my changes in place, we stop reporting this warning when run under Python 3.10 (because the type metaclass now defines __or__, and Astroid picks that up) but that's not how it's supposed to work.

If we're running on Python 3.10 but the .rc file sets py-version=3.8 should we report errors according to the py-version? If that's the case, then we're in trouble as we'll have to differentiate between a case where __or__ is coming from type (in which case we should only use it if we're configured for a version of Python that defines it) or whether it's in user-defined code, in which case we can use it on any version of Python.

Is that correct?

[Pierre-Sassoulas]

If we're running on Python 3.10 but the .rc file sets py-version=3.8 should we report errors according to the py-version?

Yes. I almost commented on this but did not because I thought the if not allowed_nested_syntax: condition might already be handling this. py-version is a valuable option but also relatively new in the codebase.

[timmartin]

It feels like the right way to solve this is for Astroid not to report the existence of an __or__ method on Python before v3.10, because it doesn't exist. If we solve this at the Astroid level, Pylint doesn't need any special rules and Astroid will be reporting slightly more correct data. But based on a quick search of the repo, it looks like Astroid doesn't support anything like a py-version setting.

Does this mean we need to have logic to deal with this in the Pylint checker? If so, I think it means that we can't benefit from implementing a hasattr, because we'll need to inspect the attribute to see what class it belongs to.

[Pierre-Sassoulas]

If we solve this at the Astroid level, Pylint doesn't need any special rules and Astroid will be reporting slightly more correct data. But based on a quick search of the repo, it looks like Astroid doesn't support anything like a py-version setting.

I think adding a py-version in astroid is very high effort because we'd have to know every specificity of each interpreter instead of just relying on the ast module for the interpreter (basically emulate a 3.8 parsing on a 3.10 interpreter). By contrast in pylint for a specific case we have to know what is the specificity of versions older than 3.x for this specific use case. What do you think @cdce8p ?

[timmartin]

I have what I think is a fix for this. It's not elegant, but I can see that making Astroid infer all the cases correctly based on the configured version is not going to be easy.

[coveralls]

Pull Request Test Coverage Report for Build 2545186714

• 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
• 260 unchanged lines in 25 files lost coverage.
• Overall coverage decreased (-0.2%) to 95.291%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/deprecated.py	1	98.91%
pylint/checkers/spelling.py	1	77.78%
pylint/extensions/comparetozero.py	1	97.06%
pylint/extensions/mccabe.py	1	99.1%
pylint/utils/docs.py	1	98.15%
pylint/checkers/modified_iterating_checker.py	2	97.56%
pylint/config/arguments_manager.py	2	98.22%
pylint/config/find_default_config_files.py	3	89.23%
pylint/extensions/docparams.py	3	98.43%
pylint/checkers/imports.py	4	94.42%

Totals
Change from base Build 2422297694:	-0.2%
Covered Lines:	16614
Relevant Lines:	17435

---

💛 - Coveralls

[Pierre-Sassoulas]

There's a small changelog issue (sorry about that, we're trying to get rid of the duplication in the next versions), but other than that LGTM. I'll wait for a review from someone else.

[Pierre-Sassoulas]

Unfortunately we need to still add the changelog in full.rst too until we release 2.14.0.

[timmartin]

Now I've missed the 2.14 cut-off I guess I have to move this. I've put the note in index.rst in the 2.15 section because that's where all the section headings are, but I don't know if this is going to get moved into separate files later?

[Pierre-Sassoulas]

Moving to 2.15 as we try to release asap :)

[timmartin]

I don't fully understand, but on Python 3.10 Astroid will infer two values for the attribute built-in on type, a function definition and a bound method. However, it seems simple enough to filter out both of these.

[jacobtylerwalls]

I'm not very fluent in metaclasses. Should this inheritance pattern lead to different results? Currently I get a test failure:

diff --git a/tests/functional/a/alternative/alternative_union_syntax_error.py b/tests/functional/a/alternative/alternative_union_syntax_error.py
index 3922e3c0d..4cb293d5c 100644
--- a/tests/functional/a/alternative/alternative_union_syntax_error.py
+++ b/tests/functional/a/alternative/alternative_union_syntax_error.py
@@ -98,10 +98,13 @@ class ReverseMetaclass(type):
     def __ror__(cls, other):
         return True
 
+class SecondLevelParent(ReverseMetaclass):
+    pass
+
 class WithForward(metaclass=ForwardMetaclass):
     pass
 
-class WithReverse(metaclass=ReverseMetaclass):
+class WithReverse(metaclass=SecondLevelParent):
     pass
 
 class DefaultMetaclass:

[timmartin]

I think you're right in your expectation that it should not report a warning with the second-level metaclass. I was assuming Astroid getattr would do the right thing here, but I guess it's more complicated than that. I'll have a closer look at this.

[jacobtylerwalls]

I think it might just be the assumption of direct parentage. Any time I see .parent I always go to try to violate that assumption somehow :D

[cdce8p]

If we solve this at the Astroid level, Pylint doesn't need any special rules and Astroid will be reporting slightly more correct data. But based on a quick search of the repo, it looks like Astroid doesn't support anything like a py-version setting.

I think adding a py-version in astroid is very high effort because we'd have to know every specificity of each interpreter instead of just relying on the ast module for the interpreter (basically emulate a 3.8 parsing on a 3.10 interpreter). By contrast in pylint for a specific case we have to know what is the specificity of versions older than 3.x for this specific use case. What do you think @cdce8p ?

It would be quite difficult to implement py-version in astroid unfortunately. True, we could do the AST parsing with a different version but that doesn't really change much. For instance, I believe the __or__ and __ror__ methods on the type metaclass are read directly from the interpreter. Thus we would need to manually update each and every change from Python at which point we're basically rebuilding CPython itself.

py-version was more designed for things which can easily be inferred by pylint, like recommendations for the use of new features (f-strings, ...).

The way you solved the issue here is quite clever! I left a few notes but after that I think it's ready.

[cdce8p]

This doesn't seem to work at the moment. I only spend a few minutes investigating it, but couldn't a quick fix. Maybe it's better to remove that test case here and open a followup issue for it to be addressed later. At least that way this PR could be merged.

[timmartin]

I was hoping to get to the bottom of it, because I didn't want to commit a half-working fix. But thinking about it, this failure case isn't a new false positive so I guess this fix will be an improvement overall. I'll pull this out and open another PR for it.

[github-actions]

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

This comment was generated for commit 8ec0215

[cdce8p]

Looks good! Thanks @timmartin 🐬

@jacobtylerwalls Small ping. Saw you left a review a while ago. Has it been resolved?

[jacobtylerwalls]

Yep, thanks for checking! I was the one that asked for that test case we haven't solved, but I agree we can postpone. I think it's just a matter of doing a recursive search on ancestors instead of relying on direct .parent-age, but we can postpone.