[flake8-bugbear] add autofix for B006 mutable_argument_default
#4880
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qdegraaf
commented
Jun 5, 2023
crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs
Outdated
Show resolved
Hide resolved
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+45, -45, 0 error(s)) airflow (+11, -11)
- airflow/models/dag.py:2125:30: B006 Do not use mutable data structures for argument defaults + airflow/models/dag.py:2125:30: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/hooks/emr.py:259:78: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/hooks/emr.py:259:78: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/operators/lambda_function.py:76:24: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/operators/lambda_function.py:76:24: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/sensors/lambda_function.py:59:31: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/sensors/lambda_function.py:59:31: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/transfers/azure_blob_to_s3.py:93:33: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/transfers/azure_blob_to_s3.py:93:33: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/transfers/azure_blob_to_s3.py:94:31: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/transfers/azure_blob_to_s3.py:94:31: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/transfers/redshift_to_s3.py:106:42: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/transfers/redshift_to_s3.py:106:42: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/providers/amazon/aws/transfers/s3_to_redshift.py:99:42: B006 Do not use mutable data structures for argument defaults + airflow/providers/amazon/aws/transfers/s3_to_redshift.py:99:42: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - airflow/utils/event_scheduler.py:32:16: B006 Do not use mutable data structures for argument defaults + airflow/utils/event_scheduler.py:32:16: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/api_connexion/endpoints/test_mapped_task_instance_endpoint.py:93:74: B006 Do not use mutable data structures for argument defaults + tests/api_connexion/endpoints/test_mapped_task_instance_endpoint.py:93:74: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/conftest.py:719:25: B006 Do not use mutable data structures for argument defaults + tests/conftest.py:719:25: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` bokeh (+34, -34)
- examples/interaction/js_callbacks/js_on_event.py:15:53: B006 Do not use mutable data structures for argument defaults + examples/interaction/js_callbacks/js_on_event.py:15:53: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - examples/server/app/events_app.py:16:35: B006 Do not use mutable data structures for argument defaults + examples/server/app/events_app.py:16:35: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - examples/server/app/events_app.py:38:28: B006 Do not use mutable data structures for argument defaults + examples/server/app/events_app.py:38:28: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/code.py:82:78: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/code.py:82:78: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/directory.py:110:65: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/directory.py:110:65: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/notebook.py:66:65: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/notebook.py:66:65: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/script.py:80:65: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/script.py:80:65: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/server_lifecycle.py:55:65: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/server_lifecycle.py:55:65: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/application/handlers/server_request_handler.py:57:65: B006 Do not use mutable data structures for argument defaults + src/bokeh/application/handlers/server_request_handler.py:57:65: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/json_encoder.py:178:51: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/json_encoder.py:178:51: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/container.py:123:82: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/container.py:123:82: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/container.py:150:82: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/container.py:150:82: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/container.py:189:32: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/container.py:189:32: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/container.py:297:32: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/container.py:297:32: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/container.py:310:66: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/container.py:310:66: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/visual.py:133:32: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/visual.py:133:32: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/visual.py:92:32: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/visual.py:92:32: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/property/wrappers.py:404:37: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/property/wrappers.py:404:37: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/core/serialization.py:217:52: B006 Do not use mutable data structures for argument defaults + src/bokeh/core/serialization.py:217:52: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/bundle.py:108:46: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/bundle.py:108:46: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/bundle.py:108:70: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/bundle.py:108:70: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/bundle.py:109:36: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/bundle.py:109:36: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/bundle.py:109:61: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/bundle.py:109:61: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/bundle.py:109:82: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/bundle.py:109:82: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/elements.py:84:46: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/elements.py:84:46: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/server.py:130:114: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/server.py:130:114: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/embed/standalone.py:299:52: B006 Do not use mutable data structures for argument defaults + src/bokeh/embed/standalone.py:299:52: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/layouts.py:317:26: B006 Do not use mutable data structures for argument defaults + src/bokeh/layouts.py:317:26: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/plotting/_renderer.py:144:56: B006 Do not use mutable data structures for argument defaults + src/bokeh/plotting/_renderer.py:144:56: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - src/bokeh/plotting/_renderer.py:144:78: B006 Do not use mutable data structures for argument defaults + src/bokeh/plotting/_renderer.py:144:78: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/support/util/examples.py:77:90: B006 Do not use mutable data structures for argument defaults + tests/support/util/examples.py:77:90: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/unit/bokeh/models/_util_models.py:87:127: B006 Do not use mutable data structures for argument defaults + tests/unit/bokeh/models/_util_models.py:87:127: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/unit/bokeh/server/_util_server.py:66:35: B006 Do not use mutable data structures for argument defaults + tests/unit/bokeh/server/_util_server.py:66:35: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None` - tests/unit/bokeh/test_events.py:43:35: B006 Do not use mutable data structures for argument defaults + tests/unit/bokeh/test_events.py:43:35: B006 [*] Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None`
BenchmarkLinuxWindows |
qdegraaf
changed the title
[flake8-bugbear] add autofix for B006 mutable_argument_default[flake8-bugbear] add autofix for B006 mutable_argument_default
konstin
approved these changes
Jul 7, 2023
| @staticmethod | ||
| def this_is_also_wrong_and_more_indented(value={}): | ||
| pass | ||
|
|
There was a problem hiding this comment.
another test case i'd like to see is a multi-line default value
There was a problem hiding this comment.
We also need to handle same-line functions (we should just avoid trying to fix these):
def f(value = {}): ...There was a problem hiding this comment.
I'll add both and have already set fix to manual. Can also change to not suggest at all for same-line funcs.
There was a problem hiding this comment.
So multiline arg seems to do fine, but same-line func creates defunct Python. It's best to not fix these indeed. Is there a quick way to check if a function is one line, or its body is on the same line as its def?
There was a problem hiding this comment.
locator.contains_line_break between the colon (:) and the first body statement should work
There was a problem hiding this comment.
Added a check for single-line func and changed from AlwaysAutoFixableViolation to Violation and sometimes fixable (also already changed the fixes to manual as per request in another comment so unsure what type of Violation the rule should now be to be fair, as so far it appears to be the only rule using manual_edit(s))
There was a problem hiding this comment.
Hmm my heuristic is not covering all cases in the end.
checker
.locator
.contains_line_break(TextRange::new(arguments.range().end(), body[0].start()))Allows weird things like
def single_line_func_wrong(value = {}
)\
: ...To slip through. Is there a way to efficiently locate the range value of the :? I have found the lexer::lex but that would require getting a string representation of the function def. Is that the way to go, or is there a better way to locate single characters in parts of a StmtFunctionDef? Maybe a find_x_in_range like util or something along those lines?
There was a problem hiding this comment.
Using the lexer when creating a diagnostic can be useful, but it is preferred not to when possible. But I don't see how you can get away with not using the lexer here (we wrote a more lightweight lexer in the formatter, because getting to tokens is such a common task, maybe it's time to make it available to ruff too). You can get the string representation of the function def by using locator.slice(def.range()). You can then start lexing after the last argument, find the colon. Then use locator.contains_line_break(TextRange::new(colon.end(), first_statement.start()
Which raises an interesting question. Is this valid? Is this a suite or a single statement body
def single_line_func_wrong(value = {}):\
...Unrelated: @konstin a use case where StmtSuite would be handy... It would tell us right away whether it is a single statement or a suite.
There was a problem hiding this comment.
Thanks for the hyperclear instructions and context. I'll see if I can spot and autofix the
def single_line_func_wrong(value = {}):\
...scenario without too much hassle and, if so, include it and only leave out the
def single_line_func_wrong(value = {}): ...scenario.
There was a problem hiding this comment.
So I've managed to exclude def single_line_func_wrong(value = {}): ... based on your instructions. The other scenario you raise is tricky though. The lexer does not do anything with the \ and it counts as a line break for contains_line_break() so current implementation still tries to make:
def single_line_func_wrong(value = None):\
if value is None:
value = {}of it, which is broken. Is there a way to discern \ line breaks from other line breaks? Or do we want to go a different route here?
crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs
Outdated
Show resolved
Hide resolved
crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs
Outdated
Show resolved
Hide resolved
konstin
reviewed
Jul 8, 2023
...rc/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B006_B006_B008.py.snap
Show resolved
Hide resolved
dhruvmanila
reviewed
Jul 9, 2023
...rc/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B006_B006_B008.py.snap
Show resolved
Hide resolved
...rc/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B006_B006_B008.py.snap
Show resolved
Hide resolved
MichaReiser
reviewed
Jul 10, 2023
crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs
Outdated
Show resolved
Hide resolved
crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs
Outdated
Show resolved
Hide resolved
...rc/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B006_B006_B008.py.snap
Show resolved
Hide resolved
| check_lines.push_str(indentation); | ||
| let check_edit = Edit::insertion(check_lines, body[0].start()); | ||
|
|
||
| diagnostic.set_fix(Fix::manual_edits(arg_edit, [check_edit])); |
There was a problem hiding this comment.
Manual edits won't be applied when running --fix in the future and are intended to be used for fixes that are known to contain syntax errors. Do you expect the fixes to contain syntax errors? If not, consider using suggested instead.
There was a problem hiding this comment.
Currently only syntax errors I am aware of that can occur is in the case of weirdly formatted single line funcs. See also: #4880 (comment) if that can be fixed it can go to suggested AFAIAC.
|
Sorry I didn't realize that merging #5776 would create merge conflicts here. Edit: I've resolved the conflicts :) |
dhruvmanila
reviewed
Jul 20, 2023
Comment on lines
-55
to
+69
| format!("Do not use mutable data structures for argument defaults") | ||
| format!("Replace mutable data structure with `None` in argument default and replace it with data structure inside the function if still `None`") | ||
| } | ||
| fn autofix_title(&self) -> Option<String> { | ||
| Some(format!( | ||
| "Do not use mutable data structures for argument defaults" | ||
| )) |
There was a problem hiding this comment.
Should this be reversed as in the new message (the one with "Replace...") would be the autofix title?
There was a problem hiding this comment.
Yeah that was the case until: #4880 (comment) but that was more of a length argument. Agree that the phrasing now is potentially confusing. Can find a way to get the best of both worlds.
There was a problem hiding this comment.
Interesting! I think editors would be using the violation message to display instead of the autofix title and I believe that's the top level key in the diagnostic data sent by the LSP server as well. \cc @MichaReiser
|
(This PR is on me to do final review + merge.) |
konstin
added a commit
that referenced
this pull request
Aug 10, 2023
## Summary Reopening of #4880 One open TODO as described in: #4880 (comment) FYI @charliermarsh seeing as you commented you wanted to do final review and merge. @konstin @dhruvmanila @MichaReiser as previous reviewers. # Old Description ## Summary Adds an autofix for B006 turning mutable argument defaults into None and setting their original value back in the function body if still `None` at runtime like so: ```python def before(x=[]): pass def after(x=None): if x is None: x = [] pass ``` ## Test Plan Added an extra test case to existing fixture with more indentation. Checked results for all old examples. NOTE: Also adapted the jupyter notebook test as this checked for B006 as well. ## Issue link Closes: #4693 --------- Co-authored-by: konstin <[email protected]>
durumu
pushed a commit
to durumu/ruff
that referenced
this pull request
Aug 12, 2023
## Summary Reopening of astral-sh#4880 One open TODO as described in: astral-sh#4880 (comment) FYI @charliermarsh seeing as you commented you wanted to do final review and merge. @konstin @dhruvmanila @MichaReiser as previous reviewers. # Old Description ## Summary Adds an autofix for B006 turning mutable argument defaults into None and setting their original value back in the function body if still `None` at runtime like so: ```python def before(x=[]): pass def after(x=None): if x is None: x = [] pass ``` ## Test Plan Added an extra test case to existing fixture with more indentation. Checked results for all old examples. NOTE: Also adapted the jupyter notebook test as this checked for B006 as well. ## Issue link Closes: astral-sh#4693 --------- Co-authored-by: konstin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adds an autofix for B006 turning mutable argument defaults into None and setting their original value back in the function body if still
Noneat runtime like so:Test Plan
Added an extra test case to existing fixture with more indentation. Checked results for all old examples.
NOTE: Also adapted the jupyter notebook test as this checked for B006 as well.
Issue link
Closes: #4693