Add use-set-for-membership check

[cdce8p]

Type of Changes

	Type
✓	✨ New feature

Description

Add new use-set-for-membership check following the discussion in #4776.

# old
x in (1, 2, 3)  # [use-set-for-membership]
x in [1, 2, 3]  # [use-set-for-membership]

# new
x in {1, 2, 3}


# only in-place defined lists and tuples will be marked
# I.e. the following would NOT result in an error
var = [1, 2, 3]
x in var

For the moment I've chosen to add it to the CodeStyleChecker, thus not enabling it by default.
However this is certainly something that might be worth having a discussion about.

Closes #4776

--
/CC: @Pierre-Sassoulas, @samueltardieu

[coveralls]

Pull Request Test Coverage Report for Build 1181150804

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

---

Totals
Change from base Build 1181148707:	0.01%
Covered Lines:	13563
Relevant Lines:	14618

---

💛 - Coveralls

[Pierre-Sassoulas]

We could make this a default checker I think. I'm not merging immediately so it can be discussed. It's going in before we release 2.10.0 either way.

[Pierre-Sassoulas]

The numerous warning in pylint's code are an opportunity to check the performance improvement that this checker really bring :D

[cdce8p]

While fixing the existing code, I came across something we haven't yet considered.

https://github.com/PyCQA/pylint/blob/f2cd986da07c12e81dd411e4b1190386a6cad750/pylint/checkers/refactoring/refactoring_checker.py#L1417-L1420

Here, pylint would currently print a warning for L1419. However, replacing the tuple with a set results in a TypeError:

TypeError: unhashable type: 'list'

--
It's more or less obvious that sets require all items to be hashable but that does also mean we can't just suggest use-set-for-membership for everything. If we don't account for that, we risk introducing difficult to find errors.

Suggestions going forward

We can only be certain if all items in a list / tuple are in-place defined constants (str, int, bool, ...). What about if a new item would be added later that isn't hashable?

As the performance benefit from this change would be minimal, I would suggest not to move forward with it.
Instead, we could consider extending consider-using-tuple once again. In contrast to #4768 however, this time only for lists.

Furthermore, we could add a new refactoring check consider-using-set to suggest the use of set / frozenset if the in-place defined list is quite long. Not sure though if this would truly be worth it.

[Pierre-Sassoulas]

Maybe we can check that the __hash__ and __cmp__ or __eq__ function is defined for each object and warn only if that's the case ? Adding object that are not of the same type in a set or in any container for that matter do not make a lot of sense. So I think there would be very few error caused by adding another type of object after pylint suggested to change to set (and in that case the user will go back to a container that does not need a hash and there would not be any false positive because this time not all object would be hashable).

[cdce8p]

Adding object that are not of the same type in a set or in any container for that matter do not make a lot of sense.

True, but it is possible. Python is a dynamic language after all.

So I think there would be very few error caused by adding another type of object after pylint suggested to change to set (and in that case the user will go back to a container that does not need a hash and there would not be any false positive because this time not all object would be hashable).

I don't think even a very few false positives are worth it considering that the change provides only a minimal performance benefit in practice (if any at all).

[samueltardieu]

Implementing it for collections containing string literals (probably a very common case) would bring a notable benefit as string hashes are cached in the string structure, especially in successive if in/elif in/... constructs.

[cdce8p]

Implementing it for collections containing string literals (probably a very common case) would bring a notable benefit as string hashes are cached in the string structure, especially in successive if in/elif in/... constructs.

As pylint doesn't support control flow inference, we can't reliably use infer for it. Thus this would be limited to in-place strings, ints, and so on.

Still believe the effort wouldn't be worth it here, unfortunately.

--
Just to clarify, I'm not suggesting we should recommend tuples instead of sets. (That change was reverted in #4832.) Only in cases that currently use lists (for consistency).

[Pierre-Sassoulas]

I moved that to 2.11 so we do not delay 2.10 while we discuss this.

[cdce8p]

I opened #4853 which extends consider-using-tuple for lists in in comparisons like I mentioned earlier. This would be my preferred way going forward.

[DanielNoord]

What about if a new item would be added later that isn't hashable?

Perhaps I am mistaken but isn't this impossible?
With x in set() if set() is not a variable but defined in place it is impossible to add something later, right? The check should therefore consider:

1. Is the comparable on the right defined-in-place?
2. Is the comparable comprised of only hashable objects?
3. Is the comparable a set?

Although the order might need to be switched, as I think performance-wise it will be better to perform check 3 as the first step.

This would mean that checking for pre-defined variables such as on line 4 of the tests (var = frozenset({1, 2, 3})) will become impossible, but I think adding the check is still useful; When I started learning Python checks such as these triggered me to search for the rationale behind these checkers and would eventually lead me to write more optimised code. Even if there is no real performance benefit for small sets (for example, x in (1, 2) vs. x in {1, 2}) enforcing such rules might lead to positive side-effects in other places.

[cdce8p]

What about if a new item would be added later that isn't hashable?

Perhaps I am mistaken but isn't this impossible?
With x in set() if set() is not a variable but defined in place it is impossible to add something later, right?

Of course at runtime no new items will be added. That doesn't however consider if you later change the code itself.

[DanielNoord]

Hmm, I would argue that that is no reason not to add this check.
We could even create a unhashable-in-set checker to accompany it (if nothing of the sort already exists) to catch these cases before runtime.
Pylint wouldn't suggest any incorrect code and if you later change it to a tuple/list again with an unhashable it will no longer complain. Warnings becoming irrelevant/false after code has been changed should in my opinion not be a reason not to add them. However, this is also something the maintainers should weigh in on.

[cdce8p]

We could even create a unhashable-in-set checker to accompany it (if nothing of the sort already exists) to catch these cases before runtime.

Of course that would be an option. I believe I even suggested it somewhere already.
Keep in mind though, and this is what my argument is all about, that you then end up with two separate checks that both use heuristics which will contain errors. I simply believe the performance improvement isn't worth the added maintenance cost.

[cdce8p]

I've moved the code to a new extension: SetMembershipChecker.
For those who really what to use it, they could. I still wouldn't recommend it though.

[Pierre-Sassoulas]

Nice 👌 Just waiting for the hot fix for 2.10 to dry before merging.

[cdce8p]

Nice 👌 Just waiting for the hot fix for 2.10 to dry before merging.

@Pierre-Sassoulas Should we create a new label for it? Something like blocked? To indicate PRs that have been accepted but we are holding off merging.

[Pierre-Sassoulas]

Do you think we should handle the following ?

class Hashable:
    pass


class NotHashable:
    def __eq__(self, other):
        return self == other


class HashableAgain:
    def __eq__(self, other):
        return self == other

    def __hash__(self):
        return id(self)

x in (1, Hashable, HashableAgain)  # [use-set-for-membership]
x in (1, NotHashable)  # List is not hashable

[cdce8p]

Do you think we should handle the following ?

If someone likes to contribute it, that would certainly be an option. I think this should go in another PR though. This one only adds a basic check for constants.