New check `dict-init-mutate` #7794

clavedeluna · 2022-11-18T12:03:10Z

Type of Changes

	Type

| ✓ | ✨ New feature |

Description

Taking over #5765

Add new check to detect mutating dict after having immediately created it.

Closes #2876

clavedeluna · 2022-11-18T12:05:06Z

@nickdrozd I apologize for not being able to retain your commits as they were. I tried lots of things but got enough permission errors when trying to push directly to the original PR that I gave up.

coveralls · 2022-11-18T12:30:04Z

Pull Request Test Coverage Report for Build 3533781397

29 of 30 (96.67%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 95.427%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/extensions/dict_init_mutate.py	29	30	96.67%

Totals
Change from base Build 3533578986:	0.002%
Covered Lines:	17550
Relevant Lines:	18391

💛 - Coveralls

clavedeluna · 2022-11-18T14:21:06Z

Primer output looks really good, in fact this suggestion is just what we would've wanted to suggest.

nickdrozd · 2022-11-18T15:09:02Z

doc/user_guide/checkers/extensions.rst

+Dict-Init-Mutate checker Messages
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+:dict-init-mutate (W3301): *Dictionary mutated immediately after initialization*
+  Dictionaries can be initialized with a single statementusing dictionary


Suggested change

Dictionaries can be initialized with a single statementusing dictionary

Dictionaries can be initialized with a single statement using dictionary

nickdrozd · 2022-11-18T15:15:08Z

This looks great! Thanks for picking it up. The primer results are exactly right.

I noticed that there is a typo in the PR title ("mudate"). I didn't see it anywhere else, but you should double check just to be sure 😄

doc/data/messages/d/dict-init-mutate/bad.py

pylint/extensions/dict_init_mutate.py

Pierre-Sassoulas

The change look pretty refined already, let's discuss how we can make the name / message clearer/better.

Pierre-Sassoulas · 2022-11-18T17:13:57Z

pylint/extensions/dict_init_mutate.py

+    name = "dict-init-mutate"
+    msgs = {
+        "W3301": (
+            "Dictionary mutated immediately after initialization",


I think this message could be more explicit about the solution and the problem, but I'm not sure. We added a function to print big suggestions of dict recently, so maybe it's possible to do something like

Suggested change

"Dictionary mutated immediately after initialization",

"Declare all known values directly in the constructor with '%s'",

Ie. result would be Declare all known values directly in the constructor with ' {"apple": 1, "banana": 10}' Declare all known values directly in the constructor with ' {"apple": 1, "melon" : 4, ... "banana": 10}'

I like the updated language, but the suggestion with %s may be tricky because it may be multiple lines after. I'll see what I can do...

jacobtylerwalls · 2022-11-19T03:24:07Z

pylint/extensions/dict_init_mutate.py

+class DictInitMutateChecker(BaseChecker):
+    name = "dict-init-mutate"
+    msgs = {
+        "W3301": (


Throughout pylint we have some warning inflation where things that should be Convention(C) are Warning(W). This seems like something that could be demoted to convention. What do you think?

yeah I agree!

tests/functional/ext/dict_init_mutate.py

pylint/extensions/dict_init_mutate.py

Pierre-Sassoulas · 2022-11-20T20:28:42Z

pylint/extensions/dict_init_mutate.py

+def stringify_dict_items(node: nodes.NodeNG) -> str:
+    try:
+        key = node.targets[0].slice.value
+        value = node.value.value
+    except AttributeError:
+        # More complex cases will be handled later on
+        return ""
+
+    if isinstance(value, str):
+        value = f"'{value}'"
+    return f"'{key}': {value}"


Suggested change

def stringify_dict_items(node: nodes.NodeNG) -> str:

try:

key = node.targets[0].slice.value

value = node.value.value

except AttributeError:

# More complex cases will be handled later on

return ""

if isinstance(value, str):

value = f"'{value}'"

return f"'{key}': {value}"

We can use node.as_string(). Check RefactoringChecker._dict_literal_suggestion and the result in use-dict-literal functional tests. I think it would be better to move this function to pylint.util and reuse it, it's pretty polished already. The only thing it could do better is to take care of really long terminating value when the last element is close to 60 chars.

nickdrozd · 2022-11-21T15:57:42Z

pylint/extensions/dict_init_mutate.py

+        if not isinstance(sibling_name, nodes.Name):
+            return [node] + all_siblings
+
+        return [node] + self._get_dict_mutating_siblings(sibling, all_siblings)


Much as I love recursion, this will crash Pylint if the dictionary is big enough:

config = {} config[0] = None config[1] = None ... # 2 ... 981 config[982] = None config[983] = None

Hopefully nobody would write something like that by hand! But it could be an issue with generated files.

Maybe it would be best to limit the suggested replacement to checking only a few items (say, three). Then if there are more than that, add an elipsis: Declare all known key/values when initializing the dictionary with 'config = {'dir': 'bin', 'user': 'me', 'workers': 5, ...}'

This could be done in a later PR.

very good point, I actually I'm really not loving the additional suggestion. I would vote to simply Declare all known key/values when initializing the dictionary and not include the constructing dictionary help. It adds difficult to read and maintain code that doesn't add THAT much value. @Pierre-Sassoulas what do you think?

With the example shows in another comment
pylint/extensions/bad_builtin.py:21:0: C3301: Declare all known key/values when initializing the dictionary with 'BUILTIN_HINTS = {'filter': Name.BUILTIN_HINTS(name='BUILTIN_HINTS')}' (dict-init-mutate)

I'm even all for keeping the msg simple.

RefactoringChecker._dict_literal_suggestion (and the function around it) can probably handle that complexity for us. I'm okay with doing it later on.

That would be awesome. I'll update the PR to leave the general suggestion and open a new issue to add the improvement suggestion.

nickdrozd · 2022-11-21T16:06:50Z

pylint/extensions/dict_init_mutate.py

+            )
+
+    def _get_dict_mutating_siblings(
+        self, node: nodes.Assign, all_siblings: List[nodes.Assign] | None = None


Can't use optional-bar syntax in older Python versions 😢

We have python > 3.7.2 we can do it as long as we use from __future__ import annotations

AHA that's the magic line!!! I couldn't figure it out, thanks Pierre!

nickdrozd · 2022-11-21T18:46:15Z

Running this checker against Pylint itself yields a funny edge case:

BUILTIN_HINTS = {"map": "Using a list comprehension can be clearer."}
BUILTIN_HINTS["filter"] = BUILTIN_HINTS["map"]

This gives a confusing error message:

pylint/extensions/bad_builtin.py:21:0: C3301: Declare all known key/values when initializing the dictionary with 'BUILTIN_HINTS = {'filter': Name.BUILTIN_HINTS(name='BUILTIN_HINTS')}' (dict-init-mutate)

Considering the difficulty in catching these edge cases and how much complexity it adds to the checker, it might be worth keeping the error message simple for now and thinking it over further. Perhaps that could be done in conjunction with figuring out how to catch nested dictionaries. The big challenge is determining what exactly goes in the dictionary.

Pierre-Sassoulas

This looks pretty good at this point, the continuous integration needs to be fixed but we can merge otherwise 👍

Pierre-Sassoulas

It's possible to rebase now that #7833 is merged.

Pierre-Sassoulas · 2022-11-23T16:03:20Z

doc/data/messages/d/dict-init-mutate/bad.py

@@ -0,0 +1,3 @@
+fruits = {}  # [dict-init-mutate]


Suggested change

fruits = {} # [dict-init-mutate]

fruit_prices = {} # [dict-init-mutate]

Pierre-Sassoulas · 2022-11-23T16:03:30Z

doc/data/messages/d/dict-init-mutate/good.py

@@ -0,0 +1 @@
+fruits = {"apple": 1, "banana": 10}


Suggested change

fruits = {"apple": 1, "banana": 10}

fruit_prices = {"apple": 1, "banana": 10}

github-actions · 2022-11-23T16:54:58Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21:
The following messages are now emitted:

dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/cuthbertLab/music21/blob/b274aa58d44b89484273071703d17dc2450016eb/music21/stream/base.py#L8655
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/cuthbertLab/music21/blob/b274aa58d44b89484273071703d17dc2450016eb/music21/analysis/floatingKey.py#L131

Effect on pandas:
The following messages are now emitted:

dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/_version.py#L276
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/core/indexes/numeric.py#L311
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/reshape/test_get_dummies.py#L110
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/frame/test_query_eval.py#L464
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/frame/test_constructors.py#L1483
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/io/test_stata.py#L1292
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/io/generate_legacy_storage_files.py#L139
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/io/formats/test_to_excel.py#L295
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/io/pytables.py#L2065
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/io/pytables.py#L4150

Effect on pytest:
The following messages are now emitted:

dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/pytest-dev/pytest/blob/56544c11b563c1e0e3d3796a124a3f2570376e73/src/_pytest/junitxml.py#L77

Effect on sentry:
The following messages are now emitted:

dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/utils/snowflake.py#L78
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/api/endpoints/authenticator_index.py#L30
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/api/serializers/models/plugin.py#L132
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/plugins/sentry_webhooks/plugin.py#L100
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/testutils/factories.py#L981
dict-init-mutate:
Declare all known key/values when initializing the dictionary.
https://github.com/getsentry/sentry/blob/33066f46c1fcf8459f8ba9194a526243a090e631/src/sentry/testutils/cases.py#L1239

This comment was generated for commit 1f2cdd8

uzzell · 2022-12-28T15:20:08Z

I'm concerned about the warning for https://github.com/pandas-dev/pandas/blob/d1ecf63e2040daecad6e0a8485a35e8a23393795/pandas/tests/reshape/test_get_dummies.py#L110 (item 3 in the Pandas list in this github-actions comment):

expected_counts = {"int64": 1, "object": 1}
expected_counts[dtype_name] = 3 + expected_counts.get(dtype_name, 0)

(Here, dtype_name depends on an argument of the method that contains this snippet.)

How would one rewrite the snippet? Doing expected_counts = {"int64": 1, "object": 1, dtype_name: 3 + expected_counts.get(dtype_name, 0)} would throw an error.

clavedeluna · 2022-12-28T15:32:21Z

#7997

clavedeluna force-pushed the dict-init branch from e20ceb9 to 6bd578f Compare November 18, 2022 12:21

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Nov 18, 2022

clavedeluna force-pushed the dict-init branch from eaa26af to 34c4fa8 Compare November 18, 2022 13:07

This comment has been minimized.

Sign in to view

clavedeluna force-pushed the dict-init branch from 77e0b53 to eb5e2a6 Compare November 18, 2022 14:24

This comment has been minimized.

Sign in to view

nickdrozd reviewed Nov 18, 2022

View reviewed changes

doc/data/messages/d/dict-init-mutate/bad.py Outdated Show resolved Hide resolved

clavedeluna changed the title ~~New check dict-init-mudate~~ New check dict-init-mutate Nov 18, 2022

nickdrozd reviewed Nov 18, 2022

View reviewed changes

pylint/extensions/dict_init_mutate.py Show resolved Hide resolved

clavedeluna force-pushed the dict-init branch from eb5e2a6 to c3e3836 Compare November 18, 2022 16:21

This comment has been minimized.

Sign in to view

clavedeluna requested a review from Pierre-Sassoulas November 18, 2022 16:56

Pierre-Sassoulas reviewed Nov 18, 2022

View reviewed changes

jacobtylerwalls reviewed Nov 19, 2022

View reviewed changes

nickdrozd reviewed Nov 19, 2022

View reviewed changes

tests/functional/ext/dict_init_mutate.py Show resolved Hide resolved

nickdrozd reviewed Nov 19, 2022

View reviewed changes

tests/functional/ext/dict_init_mutate.py Show resolved Hide resolved

clavedeluna force-pushed the dict-init branch from c3e3836 to 8944b53 Compare November 20, 2022 19:47

clavedeluna commented Nov 20, 2022

View reviewed changes

pylint/extensions/dict_init_mutate.py Show resolved Hide resolved

Pierre-Sassoulas reviewed Nov 20, 2022

View reviewed changes

nickdrozd reviewed Nov 21, 2022

View reviewed changes

clavedeluna force-pushed the dict-init branch from 70dddf8 to 6c7e0e3 Compare November 22, 2022 12:34

clavedeluna requested a review from Pierre-Sassoulas November 22, 2022 12:34

clavedeluna mentioned this pull request Nov 22, 2022

Improve dict-init-mutate msg suggestion #7819

Open

clavedeluna force-pushed the dict-init branch from 6c7e0e3 to 3cac8cc Compare November 22, 2022 12:37

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested changes Nov 22, 2022

View reviewed changes

clavedeluna force-pushed the dict-init branch from 5c9a34f to 9fcccfc Compare November 23, 2022 11:58

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested changes Nov 23, 2022

View reviewed changes

clavedeluna added 7 commits November 23, 2022 13:13

update code and remove walrus

989f44a

add confidence

3478c1f

update docs

b115c1d

update msg id

40e2dfa

update copyright and docs

4ae95cf

provide dict suggestion

d7808ed

update msg code

1f2cdd8

clavedeluna force-pushed the dict-init branch from 9fcccfc to 1f2cdd8 Compare November 23, 2022 16:13

Pierre-Sassoulas approved these changes Nov 23, 2022

View reviewed changes

Pierre-Sassoulas merged commit f7d681b into pylint-dev:main Nov 23, 2022

Pierre-Sassoulas added this to the 2.16.0 milestone Nov 23, 2022

clavedeluna mentioned this pull request Dec 28, 2022

False Positive dict-init-mutate when dict assignment references dict #7997

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New check `dict-init-mutate` #7794

New check `dict-init-mutate` #7794

clavedeluna commented Nov 18, 2022

clavedeluna commented Nov 18, 2022

coveralls commented Nov 18, 2022 •

edited

Loading

This comment has been minimized.

clavedeluna commented Nov 18, 2022

This comment has been minimized.

nickdrozd Nov 18, 2022

nickdrozd commented Nov 18, 2022

This comment has been minimized.

Pierre-Sassoulas left a comment

Pierre-Sassoulas Nov 18, 2022

clavedeluna Nov 19, 2022

jacobtylerwalls Nov 19, 2022

clavedeluna Nov 19, 2022

Pierre-Sassoulas Nov 20, 2022

nickdrozd Nov 21, 2022

clavedeluna Nov 21, 2022 •

edited

Loading

Pierre-Sassoulas Nov 21, 2022

clavedeluna Nov 21, 2022

nickdrozd Nov 21, 2022

Pierre-Sassoulas Nov 21, 2022

clavedeluna Nov 21, 2022

nickdrozd commented Nov 21, 2022

This comment has been minimized.

Pierre-Sassoulas left a comment

This comment has been minimized.

Pierre-Sassoulas left a comment

Pierre-Sassoulas Nov 23, 2022

Pierre-Sassoulas Nov 23, 2022

github-actions bot commented Nov 23, 2022

uzzell commented Dec 28, 2022 •

edited

Loading

clavedeluna commented Dec 28, 2022

	Dictionaries can be initialized with a single statementusing dictionary
	Dictionaries can be initialized with a single statement using dictionary

	"Dictionary mutated immediately after initialization",
	"Declare all known values directly in the constructor with '%s'",

	fruits = {} # [dict-init-mutate]
	fruit_prices = {} # [dict-init-mutate]

	fruits = {"apple": 1, "banana": 10}
	fruit_prices = {"apple": 1, "banana": 10}

New check dict-init-mutate #7794

New check dict-init-mutate #7794

Conversation

clavedeluna commented Nov 18, 2022

Type of Changes

Description

clavedeluna commented Nov 18, 2022

coveralls commented Nov 18, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3533781397

💛 - Coveralls

This comment has been minimized.

clavedeluna commented Nov 18, 2022

This comment has been minimized.

Choose a reason for hiding this comment

nickdrozd commented Nov 18, 2022

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clavedeluna Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdrozd commented Nov 21, 2022

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2022

uzzell commented Dec 28, 2022 • edited Loading

clavedeluna commented Dec 28, 2022

New check `dict-init-mutate` #7794

New check `dict-init-mutate` #7794

coveralls commented Nov 18, 2022 •

edited

Loading

clavedeluna Nov 21, 2022 •

edited

Loading

uzzell commented Dec 28, 2022 •

edited

Loading