Skip to content

Fix assigning-non-slot false positive with setattr#5457

Merged
Pierre-Sassoulas merged 6 commits into
pylint-dev:mainfrom
jakelishman:fix/false-positive-assigning-non-slot
Dec 15, 2021
Merged

Fix assigning-non-slot false positive with setattr#5457
Pierre-Sassoulas merged 6 commits into
pylint-dev:mainfrom
jakelishman:fix/false-positive-assigning-non-slot

Conversation

@jakelishman
Copy link
Copy Markdown
Contributor

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • [N/A] If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix

Description

Previously, if a class was slotted and overrode __setattr__,
assigning-non-slot would be issued when assigning to attributes. With
__setattr__ defined, we cannot infer if it is an error to assign to an
attribute, so we suppress the error.

Fix #3793

I came across that same behaviour in Qiskit/qiskit#7347, and found the old issue #3793 describing it. That's marked "high effort" whereas this is a very simple patch, so apologies if I've missed some complicating circumstances here.

Previously, if a class was slotted and overrode `__setattr__`,
`assigning-non-slot` would be issued when assigning to attributes.  With
`__setattr__` defined, we cannot infer if it is an error to assign to an
attribute, so we suppress the error.

Fix pylint-dev#3793
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 2, 2021

Pull Request Test Coverage Report for Build 1583001355

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

Totals Coverage Status
Change from base Build 1582953346: 0.0008%
Covered Lines: 14232
Relevant Lines: 15197

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component High effort 🏋 Difficult solution or problem to solve Control flow Requires control flow understanding labels Dec 3, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 3, 2021
@Pierre-Sassoulas Pierre-Sassoulas removed the High effort 🏋 Difficult solution or problem to solve label Dec 3, 2021
@jakelishman
Copy link
Copy Markdown
Contributor Author

I've fixed the merge conflict in CONTRIBUTORS.rst, and moved the change log entry I added into the 2.13.0 release section, to match the label applied to this PR.

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Pierre-Sassoulas commented Dec 3, 2021

Thank you ! There will be a lot of conflicts with the first other MR being merged in main (there's only one place to put the changelog right now so conflicts are inevitable), please be mindful of your time and wait for a first review before rebasing again 😄 (also it helps with saving runner so other MR get checked faster)

@Pierre-Sassoulas Pierre-Sassoulas self-requested a review December 3, 2021 18:29
@jakelishman
Copy link
Copy Markdown
Contributor Author

Ah sure, no problem!

Copy link
Copy Markdown
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.

It look like the fix was indeed simpler than anticipated at first. Thank you for the very straightforward fix ! Small change-set, well tested, and a proper changelog 👌

Copy link
Copy Markdown
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.

Small change to fix the changelog. Rest looks good to me, thanks!

Comment thread ChangeLog
@DanielNoord
Copy link
Copy Markdown
Collaborator

Thanks for the quick turnaround @jakelishman!

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Congratulation on becoming a contributor too 😉

@jakelishman
Copy link
Copy Markdown
Contributor Author

Thanks! I've used pylint a lot through general development, so it's nice I finally had a chance to give something back to it, no matter how small!

@Pierre-Sassoulas Pierre-Sassoulas merged commit 7b79386 into pylint-dev:main Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Control flow Requires control flow understanding Enhancement ✨ Improvement to a component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive assigning-non-slot (E0237) in a class with __slots__ + __setattr__

4 participants