Support inference of Enum subclasses.

[david-yz-liu]

Steps

• For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• Write a good description on what the PR does.

Description

Currently, the infer_enum_class inference tip is only applies to classes that have Enum/IntEnum/IntFlag as direct bases, but not when they have one of these classes higher up in an inheritance hierarchy, like

from enum import Enum

class A(Enum):
    pass

class B(A):  # Is an Enum, but doesn't have infer_enum_class activated
    attr = 1

I addressed this by:

1. Ensuring Enum would correctly appear when ClassDef.mro is called on a subclass of Enum.

   This required a change in ClassDef._inferred_bases to ignore any Const.None inference values, because Enum is first initialized to None in enum.py:

https://github.com/python/cpython/blob/0eec6276fdcdde5221370d92b50ea95851760c72/Lib/enum.py#L18-L21

2. Setting infer_enum_class to be called whenever one of the Enum built-in classes appear the mro. However, this required calling ClassDef.mro on every ClassDef node, which was a pretty significant change and potentially performance hit, compared to the previous (constant-time) check. So I put in a guard to only do this when we detect that enum of one of the built-in enum types has been imported, though this causes a false negative (see docstring of the new predicate function).

   I'm not sure there's a better way to solve this problem than searching through the mro, at least using what astroid already provides, but definitely open to ideas.

Additional comments

1. I also added a check for the module name, so that a user-defined Enum/IntEnum/IntFlag class won't trigger this inference tip. In principle this causes a false negative if the user writes their own Enum class based on the EnumMeta metaclass, but I figured this would be very rare.

2. This change causes an existing test in pylint to fail, on this line:

   https://github.com/PyCQA/pylint/blob/a379cc4df1ad303021628eca90e34b98c36c1b2b/tests/functional/a/arguments_renamed.py#L20

   The reason is Condiments() should just be Condiments; since Condiments is defined as an Enum, its constructor requires a value argument. To be honest I'm not sure why this error wasn't caught before, but it failed now when I tested these changes out locally.

3. I put the Changelog entry under 2.7.0 since the mro check seemed like a more substantial change. But I could easily move this to 2.6.7.

Type of Changes

	Type
✓	✨ New feature

Related Issue

Closes pylint-dev/pylint#533
Closes pylint-dev/pylint#2224
Closes pylint-dev/pylint#2626

As an aside, I found two related issues that I think no longer reproduce on master: pylint-dev/pylint#2263 and pylint-dev/pylint#3649. You might want to check those out and close the issues if you can't reproduce them either?

[Pierre-Sassoulas]

This MR is 🔥 ! Also thank you for signaling those 2 issues to close, I appreciate that very much. I have a concern with the limitation of the check if enum is not imported in the file. I think it's a reasonable choice to do because enums will probably be grouped together in a file most of the time (source required)... But this is going to create confusing false positive for those who import an enum subclass. Let me know what you think :)

Also opened an issue in pylint with the example from the issue this fix.

[Pierre-Sassoulas]

I think we need this kind of compromise if we want to keep pylint reasonably fast. But on the other hand I think that what pylint bring is a lot of checks in a lot more time than what flake8 does for example. I guess if we try to make pylint's fast and compromise on correctness pylint will stay slow and only be a little less wrong. So I'm a little torn here. How slow would the check be is we check the ancestors of the class (Knowing that we're going to cache the ancestors in #1120) ?

[david-yz-liu]

Yeah, looking at this a bit more I think this is a case of premature optimization on my part. I ran pylint on this repository with astroid==2.6.6, then with this version, and then with this version but the early return commented out, and didn't see a noticeable difference, they were all in the ballpark for 19-20 seconds. In general it seems there are more important performance issues with pylint that your team is working on.

Especially with the ancestors being cached (very cool!), and the fact that mro is almost certainly going to be called at some point when using pylint since it's essential for E1101 (no-member), I don't think the extra check is worth having. And certainly not at the expense of weird false positives!

So if you agree, I'll update this PR to remove the check from the code.

[Pierre-Sassoulas]

Thank you for taking the time to do the comparison. Let's use the ancestors then :) We do have major performance issue to tackle. I'm even thinking of a new checker freeze to be able to focus on performance, because it's hard to find time for that.

[david-yz-liu]

@Pierre-Sassoulas this test failed after I removed the additional check: len(inferred.slots()) is 1 at this line. It seems the caching behaviour being tested no longer applies, since mro is called earlier, so the bases of the ClassDef are inferred earlier as well.

I'm not sure if this is good or bad, but it's definitely a change in behaviour. This functionality was introduced in #931, so you might want to check that PR and with the author?

[Pierre-Sassoulas]

@cdce8p was the slots length supposed to be zero in the following code ? Do you remember what could be the problem if it's not ?

from typing import Generic, TypeVar

T = TypeVar('T')


class A(Generic[T]):
    __slots__ = ['value']
    
     def __init__(self, value):
         self.value = value

[cdce8p]

If I remember correctly, it's safe to remove that part of the check. As @david-yz-liu already mentioned, it was testing the, in that case invalid, caching of slots. Previously, slots would have been empty as the base wouldn't have been inferred as Generic just yet. What's important is that slots eventually contains the correct value, as tested a few lines below.

It seems as this behavior change is a side effect of this PR.

[Pierre-Sassoulas]

Nice work as always ! 👍