Skip to content

Avoid using get on a defaultdict #5766

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

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

tushar-deepsource
Copy link
Contributor

@tushar-deepsource tushar-deepsource commented Feb 3, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

I was poking around Pylint internals, trying to add new issue categories to the codebase, and stumbled upon this bug.
Since MessageDefinitionStore._msgs_by_category is a defaultdict(list), using .get() here can break the code on bad input, because it will return None, which is not iterable.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1789962783

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.917%

Totals Coverage Status
Change from base Build 1784689763: 0.0%
Covered Lines: 14837
Relevant Lines: 15798

πŸ’› - Coveralls

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.

But the proposed change would also result in a KeyError crash, right ?

@@ -1659,7 +1659,7 @@ def _get_messages_to_set(
else:
category_id_formatted = category_id
if category_id_formatted is not None:
for _msgid in self.msgs_store._msgs_by_category.get(category_id_formatted):
for _msgid in self.msgs_store._msgs_by_category[category_id_formatted]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for _msgid in self.msgs_store._msgs_by_category[category_id_formatted]:
for _msgid in self.msgs_store._msgs_by_category.get(category_id_formatted, []):

@tusharsadhwani
Copy link
Contributor

It won't, as it's a defaultdict. If the key doesn't exist it'll make one by the default factory (list() in this case).

@Pierre-Sassoulas Pierre-Sassoulas added Bug πŸͺ² Minor πŸ’… Polishing pylint is always nice labels Feb 3, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Feb 3, 2022
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.

I missed that it was a defaultdict, thank you for the fix !

@Pierre-Sassoulas Pierre-Sassoulas merged commit d7013f6 into pylint-dev:main Feb 3, 2022
@tushar-deepsource tushar-deepsource deleted the patch-1 branch February 4, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ² Minor πŸ’… Polishing pylint is always nice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants