[ruff] Detect unnecessary dict comprehensions for iterables (RUF025)
#9613
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
RemarkI'm getting the following error when running tests locally with ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE001_1.py.snap
Snapshot: EXE001_1.py
Source: crates/ruff_linter/src/rules/flake8_executable/mod.rs:43
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
0 │-EXE001_1.py:1:1: EXE001 Shebang is present but file is not executable␊
1 │- |␊
2 │-1 | #!/usr/bin/python␊
3 │- | ^^^^^^^^^^^^^^^^^ EXE001␊
4 │-2 | ␊
5 │-3 | if __name__ == '__main__':␊
6 │- |␊
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'rules::flake8_executable::tests::rules::path_new_exe001_1_py_expects' panicked at /home/mikko/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.34.0/src/runtime.rs:563:9:
snapshot assertion for 'EXE001_1.py' failed in line 43
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
rules::flake8_executable::tests::rules::path_new_exe001_1_py_expects
rules::flake8_executable::tests::rules::path_new_exe002_1_py_expectsIs this a known issue or should I do something differently? SuggestionHave you considered using cargo-make? It could be used to execute e.g. fmt, clippy and tests in one task. |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF025 | 28 | 28 | 0 | 0 | 0 |
charliermarsh
added
rule
…or key-value pair in comprehension is binded to a same name
dhruvmanila
approved these changes
Jan 23, 2024
| /// ``` | ||
| #[violation] | ||
| pub struct UnnecessaryDictComprehensionForIterable { | ||
| is_value_none_literal: bool, |
There was a problem hiding this comment.
nit: we could use an enum instead of a boolean. A boolean value is hard to read until it is linked with some context like a variable name. An enum can prove to be readable in such cases.
enum Value {
// A `None` literal
NoneLiteral,
// Any value other than `None`
Other
}There was a problem hiding this comment.
Yea, I thought about using an Enum but it kind of felt slightly over board for such a simple thing.
|
|
||
| impl AlwaysFixableViolation for UnnecessaryDictComprehensionForIterable { | ||
| #[derive_message_formats] | ||
| #[allow(clippy::match_bool)] |
There was a problem hiding this comment.
I believe this must be a leftover and can be removed now?
| } | ||
| } | ||
|
|
||
| #[allow(clippy::match_bool)] |
There was a problem hiding this comment.
I believe this must be a leftover and can be removed now?
Comment on lines
78
to
80
| // Don't suggest `dict.fromkeys` for async generator expressions, because `dict.fromkeys` is not async. | ||
| // Don't suggest `dict.fromkeys` for nested generator expressions, because `dict.fromkeys` might be error-prone option at least for fixing. | ||
| // Don't suggest `dict.fromkeys` for generator expressions with `if` clauses, because `dict.fromkeys` might not be valid option. |
There was a problem hiding this comment.
nit: can we combine these comments and use bullet points instead?
// Don't suggest `dict.fromkeys` for
// - ..., because ...
Comment on lines
84
to
86
| let Expr::Name(_) = &generator.target else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
nit: we can use the is_ method on the node: generator.target.is_name_expr
| /// Generate a [`Fix`] to replace `dict` comprehension with `dict.fromkeys`. | ||
| /// (RUF025) Convert `{n: None for n in [1,2,3]}` to `dict.fromkeys([1,2,3])` or | ||
| /// `{n: 1 for n in [1,2,3]}` to `dict.fromkeys([1,2,3], 1)`. | ||
| fn fix_unnecessary_dict_comprehension( |
There was a problem hiding this comment.
Is there a specific reason you choose to use libcst nodes instead of the AST nodes to construct the fixed node? I might be wrong but I think it might be easier to construct the node using the AST nodes with the Generator to generate the equivalent source code. I'm fine with either but just trying to understand this.
There was a problem hiding this comment.
No, specific reason. I was not quite sure what would be the correct way to construct the fixed node. Seems that using AST node with Generator would be more reasonable.
|
@dhruvmanila thanks for the good comments! 👍 |
| return; | ||
| } | ||
|
|
||
| if !is_constant(dict_comp.value.as_ref()) { |
There was a problem hiding this comment.
@mikeleppane - I ended up using is_constant here with doesn't include Expr::Name, so that changed some of the test case results.
For example, we no longer flag this:
def func():
values = ["a", "b", "c"]
[{n: values for n in [1,2,3]}] # RUF025But I think we're correct not to flag, since in that case, values is actually mutable.
There was a problem hiding this comment.
Oh wait, hmm, but the user is already sharing it between elements. What you had is probably correct then.
charliermarsh
changed the title
ruff] Detect unnecessary dict comprehensions for iterables (RUF025)
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
Checks for unnecessary
dictcomprehension when creating a new dictionary from iterable. Suggest to replace withdict.fromkeys(iterable)See: #9592
Test Plan
cargo test