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(b909): Fix false positive affecting containers of mutables #469

Merged
merged 3 commits into from Apr 26, 2024

Conversation

mimre25
Copy link
Contributor

@mimre25 mimre25 commented Apr 25, 2024

The false positives occurred when trying to edit a dictionary while iterating over a list of dictionaries:

lst: list[dict] = [{}, {}, {}]
for dic in lst:
    dic["key"] = False # was false positive - fixed now

The false positives occurred when trying to edit a dictionary while
iterating over a list of dictionaries:

```
lst: list[dict] = [{}, {}, {}]
for dic in lst:
    dic["key"] = False # was false positive - fixed now
```
@henzef
Copy link
Contributor

henzef commented Apr 25, 2024

With this patch flake8 complains about the following construct in my code:

for key in my_dictionary:
    my_dictionary[key] = True

which is safe to do, because it doesn't add or remove any keys. At least as far as I understand, please correct me if I am wrong.

@cooperlees
Copy link
Collaborator

Thanks for this. What's the plan, did you want to add this fix or do you have more plans for this PR?

@mimre25
Copy link
Contributor Author

mimre25 commented Apr 26, 2024

I'd be happy to add the fix for

for key in my_dictionary:
    my_dictionary[key] = True

at the time I opened the draft PR I wasn't 100% sure this is desired, but judging from the discussions in #467 and the 👍 on henzef comment it seems like this is something we want.

I'll hopefully get around to adding this later today.

These changes allow the following:
```
some_dict = {"foo": "bar"}
for key in some_dict:
    some_dict[key] = 3 # no error (previously error'd)
```
Turns out, that the slice type was changed in python 3.9.

> Changed in version 3.9: Simple indices are represented by their value,
> extended slices are represented as tuples.
from https://docs.python.org/3/library/ast.html#module-ast
@mimre25 mimre25 marked this pull request as ready for review April 26, 2024 08:00
@mimre25
Copy link
Contributor Author

mimre25 commented Apr 26, 2024

This is ready now 🙂

FYI: tested it with both black and MONAI and there were no false positives. Test were done with py38 and py312

@henzef
Copy link
Contributor

henzef commented Apr 26, 2024

No false positives (or true positives :D) in my codebase either. Thanks for the quick fix!

Copy link
Collaborator

@cooperlees cooperlees 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 the quick fix with great test coverage! I really appreciate great contributors like yourself.

@@ -1603,6 +1605,46 @@ def compose_call_path(node):
yield node.id


def _tansform_slice_to_py39(slice: ast.Slice) -> ast.Slice | ast.Name:
"""Transform a py38 style slice to a py39 style slice.
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL - Thanks for this.

@cooperlees cooperlees merged commit b4986aa into PyCQA:main Apr 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants