Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enum._generate_next_value_ should be static #103056

Closed
RomanSteinberg opened this issue Mar 27, 2023 · 6 comments
Closed

Enum._generate_next_value_ should be static #103056

RomanSteinberg opened this issue Mar 27, 2023 · 6 comments
Assignees
Labels
3.12 bugs and security fixes type-bug An unexpected behavior, bug, or error

Comments

@RomanSteinberg
Copy link

RomanSteinberg commented Mar 27, 2023

Bug report

Here and in two more places in the same file function _generate_next_value_ supposed to be called for instance. But it will not work properly in that case, because it behaves like static method. Consider staticmethod decorator.

As far as I understand there is no problem right now and all calls are correct. But in future it will be more robust if it would be static.

Linked PRs

@RomanSteinberg RomanSteinberg added the type-bug An unexpected behavior, bug, or error label Mar 27, 2023
@ethanfurman
Copy link
Member

_generate_next_value_ is not a static method, and should never be called on an instance. It is a plain function, called by the enum type machinery during value creation. If a user does use the staticmethod decorator, it will be stripped and the decorated function used instead during value creation.

Hmm, I see that the final function stored on the completed class is either a plain function or a static method depending on how it was written in the class -- it should probably always be one or the other; I think staticmethod since that's what I thought it was doing. ;-)

The check and possible staticmethod conversion should be at line 531.

@ethanfurman
Copy link
Member

Okay, so it will be a staticmethod, but specifying that when writing a class is optional.

@RomanSteinberg
Copy link
Author

@ethanfurman could you, please, explain the following phrase:

If a user does use the staticmethod decorator, it will be stripped and the decorated function used instead during value creation.

@ethanfurman
Copy link
Member

If the function was decorated with staticmethod, this line: _gnv = value.__func__ if isinstance(value, staticmethod) else value retrieves the function. When auto() is used, the actual enum doesn't yet exist and the members don't yet exist, so a static method doesn't work -- we need the plain function in order to generate the values.

The change implented by this patch is to wrap the _generate_next_value_ with staticmethod (if not already done) before storing it in the final enum class.

@RomanSteinberg
Copy link
Author

RomanSteinberg commented Mar 29, 2023

@ethanfurman thank you!
As far as I understand, the main reason not to make _generate_next_value_ static is not related to the code I linked, but to the code you linked, i.e. method around this line.

BTW, I do not understand how auto related to my question.

Ok, If I understood you correctly then I have one more question. Why not to decorate the definition of the _generate_next_value_ function with @staticmethod? The line you linked removes decorator for machinery purposes and it will still work fine, but the code becomes more clear for future developers and IDE will not complain.

@ethanfurman
Copy link
Member

@RomanSteinberg You make a good point. I have added the staticmethod decorator to the code and the docs.

Thank you for your input!

gaogaotiantian pushed a commit to gaogaotiantian/cpython that referenced this issue Apr 8, 2023
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants