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

[pylint] Implement rule to prefer augmented assignment (PLR6104) #9932

Merged

Conversation

lshi18
Copy link
Contributor

@lshi18 lshi18 commented Feb 11, 2024

Summary

Implement new rule: Prefer augmented assignment (#8877). It checks for the assignment statement with the form of <expr> = <expr> <binary-operator> … with a unsafe fix to use augmented assignment instead.

Test Plan

  1. Snapshot test is included in the PR.
  2. Manually test with playground.

Copy link
Contributor

github-actions bot commented Feb 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+507 -1 violations, +0 -0 fixes in 12 projects; 32 projects unchanged)

aiven/aiven-client (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ aiven/client/client.py:159:9: PLR6104 Use `+=` to perform an augmented assignment directly

PlasmaPy/PlasmaPy (+33 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ plasmapy/analysis/tests/test_fit_functions.py:241:13: PLR6104 Use `*=` to perform an augmented assignment directly
+ plasmapy/analysis/tests/test_fit_functions.py:263:13: PLR6104 Use `*=` to perform an augmented assignment directly
+ plasmapy/analysis/time_series/running_moments.py:61:5: PLR6104 Use `-=` to perform an augmented assignment directly
+ plasmapy/diagnostics/charged_particle_radiography/synthetic_radiography.py:413:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ plasmapy/diagnostics/charged_particle_radiography/synthetic_radiography.py:419:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ plasmapy/diagnostics/charged_particle_radiography/synthetic_radiography.py:830:9: PLR6104 Use `+=` to perform an augmented assignment directly
... 27 additional changes omitted for rule PLR6104
+ plasmapy/formulary/collisions/helio/collisional_analysis.py:238:9: PLR0914 Too many local variables (17/15)
- plasmapy/formulary/collisions/helio/collisional_analysis.py:238:9: PLR0914 Too many local variables (17/15)
... 26 additional changes omitted for project

apache/airflow (+47 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/io/path.py:252:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ airflow/io/path.py:254:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ airflow/kubernetes/pre_7_4_0_compatibility/kube_client.py:89:5: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/kubernetes/pre_7_4_0_compatibility/kube_client.py:90:5: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/models/taskinstance.py:1635:21: PLR6104 Use `/=` to perform an augmented assignment directly
+ airflow/providers/amazon/aws/hooks/eks.py:546:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/providers/amazon/aws/hooks/eks.py:549:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/providers/apache/hive/hooks/hive.py:346:21: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/providers/apache/hive/hooks/hive.py:348:21: PLR6104 Use `+=` to perform an augmented assignment directly
+ airflow/providers/cncf/kubernetes/kube_client.py:90:5: PLR6104 Use `+=` to perform an augmented assignment directly
... 37 additional changes omitted for project

aws/aws-sam-cli (+128 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ samcli/commands/_utils/table_print.py:46:9: PLR6104 Use `-=` to perform an augmented assignment directly
+ samcli/commands/init/interactive_event_bridge_flow.py:174:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/build/bundler.py:178:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/deploy/deployer.py:468:17: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/deploy/deployer.py:814:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/cli_paginator.py:27:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/cli_paginator.py:34:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/cli_paginator.py:38:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/cli_paginator.py:42:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/cli_paginator.py:59:9: PLR6104 Use `-=` to perform an augmented assignment directly
+ samcli/lib/schemas/schemas_aws_config.py:52:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ samcli/lib/schemas/schemas_directory_hierarchy_builder.py:14:9: PLR6104 Use `+=` to perform an augmented assignment directly
... 116 additional changes omitted for project

bokeh/bokeh (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ src/bokeh/embed/server.py:294:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ src/bokeh/resources.py:379:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ src/bokeh/server/tornado.py:307:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ src/bokeh/server/tornado.py:422:17: PLR6104 Use `+=` to perform an augmented assignment directly
+ src/bokeh/server/util.py:96:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ src/bokeh/util/token.py:308:9: PLR6104 Use `+=` to perform an augmented assignment directly

freedomofpress/securedrop (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ securedrop/pretty_bad_protocol/_util.py:462:5: PLR6104 Use `%=` to perform an augmented assignment directly
+ securedrop/tests/test_journalist.py:1864:9: PLR6104 Use `+=` to perform an augmented assignment directly

ibis-project/ibis (+10 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ ibis/backends/bigquery/tests/unit/udf/test_usage.py:25:5: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/backends/druid/__init__.py:61:9: PLR6104 Use `|=` to perform an augmented assignment directly
+ ibis/backends/exasol/__init__.py:100:9: PLR6104 Use `|=` to perform an augmented assignment directly
+ ibis/backends/pandas/executor.py:630:17: PLR6104 Use `&=` to perform an augmented assignment directly
+ ibis/common/annotations.py:287:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/common/annotations.py:289:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/common/tests/test_numeric.py:63:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/common/tests/test_numeric.py:68:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/common/tests/test_numeric.py:75:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ ibis/expr/tests/test_api.py:156:9: PLR6104 Use `+=` to perform an augmented assignment directly

milvus-io/pymilvus (+30 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ examples/example_bulkinsert_json.py:142:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:224:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:247:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:249:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:251:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:253:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_json.py:255:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_numpy.py:132:5: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_numpy.py:226:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ examples/example_bulkinsert_numpy.py:249:13: PLR6104 Use `+=` to perform an augmented assignment directly
... 20 additional changes omitted for project

pandas-dev/pandas (+131 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ asv_bench/benchmarks/algos/isin.py:136:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ asv_bench/benchmarks/arithmetic.py:141:13: PLR6104 Use `//=` to perform an augmented assignment directly
+ pandas/_testing/contexts.py:113:5: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/compat/numpy/function.py:107:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/compat/numpy/function.py:168:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/compat/numpy/function.py:200:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/compat/numpy/function.py:229:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/core/algorithms.py:900:9: PLR6104 Use `/=` to perform an augmented assignment directly
+ pandas/core/apply.py:1833:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/core/arrays/arrow/array.py:2544:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ pandas/core/arrays/boolean.py:229:17: PLR6104 Use `|=` to perform an augmented assignment directly
+ pandas/core/arrays/boolean.py:236:17: PLR6104 Use `|=` to perform an augmented assignment directly
... 119 additional changes omitted for project

rotki/rotki (+45 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ package.py:1061:17: PLR6104 Use `/=` to perform an augmented assignment directly
+ package.py:1063:17: PLR6104 Use `/=` to perform an augmented assignment directly
+ package.py:306:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ package.py:339:13: PLR6104 Use `/=` to perform an augmented assignment directly
+ rotkehlchen/chain/aggregator.py:1313:17: PLR6104 Use `*=` to perform an augmented assignment directly
+ rotkehlchen/chain/bitcoin/bch/utils.py:132:13: PLR6104 Use `+=` to perform an augmented assignment directly
+ rotkehlchen/chain/bitcoin/bch/utils.py:58:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ rotkehlchen/chain/ethereum/graph.py:71:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ rotkehlchen/chain/ethereum/modules/eth2/beacon.py:76:9: PLR6104 Use `+=` to perform an augmented assignment directly
+ rotkehlchen/chain/ethereum/modules/makerdao/dsr.py:377:9: PLR6104 Use `*=` to perform an augmented assignment directly
... 35 additional changes omitted for project

... Truncated remaining completed project reports due to GitHub comment length restrictions

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
PLR6104 506 506 0 0 0
PLR0914 2 1 1 0 0

@lshi18 lshi18 force-pushed the 8877-new-rule-prefer-in-place-operators branch from c3255c5 to 2241be2 Compare February 11, 2024 17:28
Copy link

codspeed-hq bot commented Feb 11, 2024

CodSpeed Performance Report

Merging #9932 will not alter performance

Comparing lshi18:8877-new-rule-prefer-in-place-operators (5afd7de) with main (563daa8)

Summary

✅ 30 untouched benchmarks

@lshi18 lshi18 force-pushed the 8877-new-rule-prefer-in-place-operators branch 4 times, most recently from 0bc73ac to 6fbedf6 Compare February 11, 2024 19:04
@Avasam
Copy link

Avasam commented Feb 11, 2024

Not a rust dev, so I won't comment on your implementation, but I think the tests should also check for proper handling of order of operation:

>>> test = 2
>>> test = test * test + 10  # OK
>>> test
14
>>> test = 2
>>> test *= test + 10       
>>> test
24
>>> test = 2
>>> test = test * (test + 10)  # RUF028
>>> test     
24

@lshi18
Copy link
Contributor Author

lshi18 commented Feb 11, 2024

Thanks for the quick reply. I added the suggested test cases. I had these two test cases in my earlier draft, but wasn’t sure and removed them. It is nice that you brought them up and have them back.

@sbrugman
Copy link
Contributor

sbrugman commented Feb 13, 2024

Related pylint rule:
https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/consider-using-augmented-assign.html

Related issue: #970

@lshi18 instead of assigning it to RUF028, this rule should be placed under PyLint R6104.

@lshi18 lshi18 force-pushed the 8877-new-rule-prefer-in-place-operators branch from 8bd617a to 6e57b2c Compare February 18, 2024 11:19
@lshi18 lshi18 changed the title Implement new rule: Prefer augmented assignment (#8877) Implement new rule PLR6104: Prefer augmented assignment (#8877) Feb 18, 2024
@lshi18
Copy link
Contributor Author

lshi18 commented Feb 18, 2024

The new rule has been moved under PLR6104 instead of RUF028.

@lshi18
Copy link
Contributor Author

lshi18 commented Feb 18, 2024

The CI/ecosystem check failed with an error. I am not able to figure out what I did wrong immediately. It'll be of great help if you could point me to the right direction which could effect an efficient fix.

checker: &mut Checker,
assign @ ast::StmtAssign { value, targets, .. }: &ast::StmtAssign,
) {
if targets.len() != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using let-else here to make this check a bit nicer:

let target = Some(targets.first()) else {
    return;
}

Copy link
Contributor Author

@lshi18 lshi18 Feb 18, 2024

Choose a reason for hiding this comment

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

Please correct me if I am wrong, but do you mean

let Some(target) = target.first() else {
    return;
}

I agree that this looks nicer if it could be changed to it.

I don't think, in this case, that they are semantically equal. In particular, the following test case which is marked as OK fails after the suggested change.

a_number = index = a_number + 1 # OK

I think a further question would be whether we should detect the above case and, in the same gist, the one below.

index = a_number = a_number + 1 # OK

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I hadn't tested. Since targets is a vector, this should work:

let targets = vec![1];
// let targets = vec![1, 2];
// let targets: Vec<i32> = vec![];

let &[target] = targets.as_slice() else{
    return;
};

Copy link
Contributor Author

@lshi18 lshi18 Feb 18, 2024

Choose a reason for hiding this comment

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

I had a quick test with pylint 3.0.3, whose R6104 rule detects this

a_number = index = a_number + 1 # OK

but not this,

index = a_number = a_number + 1 # OK

Do you think we should implement to reproduce this effect?

BTW, the test cases for this rule in the pylint project include neither of the above cases, so I think the current behaviour is a side-effect of implementation.

Meanwhile, my current implementation is incorrect regarding handling more complex subsript / attribute expressions. I'll reimplement it while also taking into consideration of your other suggestions .

}
let target = targets.first().unwrap();

let rhs_expr = value
Copy link
Contributor

@sbrugman sbrugman Feb 18, 2024

Choose a reason for hiding this comment

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

 let (left_operand, operator, right_operand) = Some(
      value
       .as_bin_op_expr()
       .map(|e| (e.left.as_ref(), e.op, e.right.as_ref()))
      ) else {
       return;
}

See let

}
let (left_operand, operator, right_operand) = rhs_expr.unwrap();

if name_expr(target, left_operand)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of testing these conditions, you could match the target/left_operand type with match.

There may be some other rules to serve as inspiration e.g.

fn is_constant(key: &Expr, names: &FxHashMap<&str, &ast::ExprName>) -> bool {

@charliermarsh charliermarsh changed the title Implement new rule PLR6104: Prefer augmented assignment (#8877) [pylint] Implement rule to prefer augmented assignment (PLR6104) Apr 12, 2024
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Apr 12, 2024
@charliermarsh charliermarsh force-pushed the 8877-new-rule-prefer-in-place-operators branch from 840ef33 to 5afd7de Compare April 12, 2024 02:44
@charliermarsh charliermarsh merged commit a9e4393 into astral-sh:main Apr 12, 2024
17 checks passed
@DetachHead DetachHead mentioned this pull request Apr 12, 2024
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
…stral-sh#9932)

## Summary

Implement new rule: Prefer augmented assignment (astral-sh#8877). It checks for
the assignment statement with the form of `<expr> = <expr>
<binary-operator> …` with a unsafe fix to use augmented assignment
instead.

## Test Plan

1. Snapshot test is included in the PR.
2. Manually test with playground.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants