Fix crash on properties & inherited methods in implicit-booleaness

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #5646 and supersedes #5647.

The typing on this function was incorrect and never actually received classes. Instead it received bases.Instance which was also deductible from the parameter name. I've completely rewritten the function as I felt this was the best way to go.

[DanielNoord]

@Pierre-Sassoulas You seem to have written the test that is currently failing. Could it be that this is a non-sensical test? Seeing as that the functional tests all pass it seems like we don't ever pass ClassDef nodes to this function other than in this test.

I'll wait for the primer to pass, but this might be a non-real-world test.

[Pierre-Sassoulas]

I probably messed the test up by giving class def instead of instances but the code also work for instances (?)

[Pierre-Sassoulas]

I feel like we should have two functions, one for unbound method (base_classes_of_returned_instance) and one for instances (base_classes_of_instance). This new function is handling two cases.

[DanielNoord]

Not sure.. How would we then call the function that calls base_classes_of_returned_instance and base_classes_of_instance?

[Pierre-Sassoulas]

Do we need one ? It seems the code before was not handling returned instances ?

[DanielNoord]

Yeah, that's what the crash is about. We don't only get Instances here but also UnboundMethod whenever one of the compared values is a property.

This is basically three changes in one: fix method typing to actually reflect that is handling Instances + update method to also handle UnboundMethod + update method to also handle Uninsurable (and everything else that is not Instance or UnboundMethod. Perhaps I should have split this, but felt that this made more sense.

[Pierre-Sassoulas]

Ok, I understand now. Maybe the original name, "base class of node" is generic enough (and future proof if we encounter exotic codebase that will have other nodes) ?

[DanielNoord]

Renamed to base_names_of_base_node, let me know what you think!

[Pierre-Sassoulas]

Neat !

[DanielNoord]

This comment was outdated when you did your review. Are you also okay with the new new name?

[Pierre-Sassoulas]

Yes :)

[DanielNoord]

Then I'll merge 🎉

[DanielNoord]

I probably messed the test up by giving class def instead of instances but the code also work for instances (?)

Yes, since instances are derived from classes and have an ancestors method. It would be similar to testing with nodes.Name instead of nodes.FunctionDef and using their name attribute, both have them but they are not the same.

[coveralls]

Pull Request Test Coverage Report for Build 1694033305

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 93.843%

---

Totals
Change from base Build 1693266841:	0.01%
Covered Lines:	14648
Relevant Lines:	15609

---

💛 - Coveralls