-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix a crash for Enum class decorated with dataclass #9104
Fix a crash for Enum class decorated with dataclass #9104
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #9104 +/- ##
=======================================
Coverage 95.75% 95.75%
=======================================
Files 173 173
Lines 18663 18665 +2
=======================================
+ Hits 17871 17873 +2
Misses 792 792
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add the functional test for the false negative we create (something that should return true ?) and a todo that we'll fix in 3.1 ?
Ok once I create the astroid issue I'll update the doc-string of the test class with the issue number. |
…lasses.dataclass`` decorator is defined. Closes pylint-dev#9100
1c1d6ba
to
aedcb3a
Compare
This comment has been minimized.
This comment has been minimized.
… ``__members__`` object.
aedcb3a
to
8ce3897
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up!
pylint/checkers/utils.py
Outdated
@@ -2281,5 +2281,8 @@ def is_enum_member(node: nodes.AssignName) -> bool: | |||
): | |||
return False | |||
|
|||
enum_member_objects = frame.locals.get("__members__")[0].items | |||
try: | |||
enum_member_objects = frame.locals.get("__members__")[0].items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we do a check if the get
returns None
? In the future I think we will ask ourselves what kind of error we are catching here whereas seeing an is not None
check is very explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for try/except since in the majority of cases this code is run, there should be a __members__
present; the only case I know of where it breaks currently, is when the dataclass decorator is used.
You make a good point though - at first glance it would be a bit difficult to see where the cause of the exception is. We could simplify it:
+ members = frame.locals.get("__members__")
try:
- enum_member_objects = frame.locals.get("__members__")[0].items
+ enum_member_objects = members[0].items
except TypeError:
return False
return node.name in [name_obj.name for value, name_obj in enum_member_objects]
Or your suggestion:
- try:
- enum_member_objects = frame.locals.get("__members__")[0].items
- except TypeError:
- return False
- return node.name in [name_obj.name for value, name_obj in enum_member_objects]
+ members = frame.locals.get("__members__")
+ if members is not None:
+ enum_member_objects = members[0].items
+ return node.name in [name_obj.name for value, name_obj in enum_member_objects]
+ return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the second diff, but perhaps with an early exit for if members is None
and then doing the rest of the checks.
We can just add a comment saying that "due to unforeseen circumstances members is not always present" 😄
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit fc62a9d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR !
* Fix a crash when an enum class which is also decorated with a ``dataclasses.dataclass`` decorator is defined. Closes #9100 * Update test to mention the `astroid` issue that addresses the missing ``__members__`` object. * Use an `if` instead of the `try/except` block. * Update pylint/checkers/utils.py Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> --------- Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> (cherry picked from commit 2c3425d)
* Fix a crash when an enum class which is also decorated with a ``dataclasses.dataclass`` decorator is defined. Closes #9100 * Update test to mention the `astroid` issue that addresses the missing ``__members__`` object. * Use an `if` instead of the `try/except` block. * Update pylint/checkers/utils.py Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> --------- Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> (cherry picked from commit 2c3425d) Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Type of Changes
Description
Fix a crash when an enum class which is also decorated with a
dataclasses.dataclass
decorator is defined.Closes #9100