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

Fix a crash for Enum class decorated with dataclass #9104

Merged

Conversation

mbyrnepr2
Copy link
Member

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Description

Fix a crash when an enum class which is also decorated with a dataclasses.dataclass decorator is defined.

Closes #9100

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #9104 (fc62a9d) into main (f74200d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9104   +/-   ##
=======================================
  Coverage   95.75%   95.75%           
=======================================
  Files         173      173           
  Lines       18663    18665    +2     
=======================================
+ Hits        17871    17873    +2     
  Misses        792      792           
Files Coverage Δ
pylint/checkers/utils.py 95.94% <100.00%> (+<0.01%) ⬆️

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a 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 ?

@mbyrnepr2
Copy link
Member Author

Ok once I create the astroid issue I'll update the doc-string of the test class with the issue number.

@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review October 3, 2023 18:20
@github-actions

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a 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!

@@ -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
Copy link
Collaborator

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.

Copy link
Member Author

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

Copy link
Collaborator

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" 😄

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit fc62a9d

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 2c3425d into pylint-dev:main Oct 4, 2023
46 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 4, 2023
* 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)
@mbyrnepr2 mbyrnepr2 deleted the crash_dataclass_and_enums branch October 4, 2023 06:05
Pierre-Sassoulas added a commit that referenced this pull request Oct 4, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash with dataclass enums
3 participants