wemake-services / wemake-python-styleguide

The strictest and most opinionated python linter ever!
https://wemake-python-styleguide.rtfd.io
MIT License
2.49k stars 380 forks source link

Forbid misrefactored assign op #790

Closed sobolevn closed 4 years ago

sobolevn commented 5 years ago

Rule request

Thesis

We need to forbid code like:

x += x + 1

It is most likely an error. Refactor it to:

  1. x += 1
  2. x *= 2; x += 1

https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op Related: #789

Reasoning

This is a refactoring rule.

dinolinjob commented 4 years ago

I would like to work on this issue

sobolevn commented 4 years ago

Awesome! @dinolinjob you are on!

dinolinjob commented 4 years ago

I understood that I have to create a violation in https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/violations/refactoring.py. Should I raise an error as soon as I come across some line consisting x += x + 1 exists? Does it come under Tokenize violation? What are the further steps to complete writing this rule?

sobolevn commented 4 years ago

@dinolinjob it is an ASTViolation. Then you should create a method inside visitors/ast to find these cases.

dinolinjob commented 4 years ago

@sobolevn should I write a new visitor?

sobolevn commented 4 years ago

Try to write a _check_* method first on an existing visitor to check the logic. Then we can decide on a new visitor.

dinolinjob commented 4 years ago

@sobolevn In which file under visitors/ast/ should I write my _check_* method?

sobolevn commented 4 years ago

visitors/ast/statements.py

viveksoundrapandi commented 4 years ago

@sobolevn I am trying to do a TDD, i have written a test in test_statements.py . But when i run it inside the docker image, it takes the installed wemake_python_styleguide. How do one configure the tests to run?

sobolevn commented 4 years ago

@viveksoundrapandi oh, I must document this. This Dockerfile is for GithubActions, not the local development.

viveksoundrapandi commented 4 years ago

Ahh! Thanks for the clarification

dinolinjob commented 4 years ago

@sobolevn Can you suggest me some ideas on how to approach while writing this rule. Is there a way to compare variables before and after assignment operators?

sobolevn commented 4 years ago

Yes, you need to visit ast.AugAssign and fetch variables from the right part. If the left variable is contained in the right part - then we raise a violation.

That's how ast.AugAssign looks like for this simple case:

» echo 'x += x + 1' > ex.py

» python scripts/parse.py ex.py

AugAssign(target=Name(id='x', ctx=Store(), lineno=1, col_offset=0), op=Add(), value=BinOp(left=Name(id='x', ctx=Load(), lineno=1, col_offset=5), op=Add(), right=Num(n=1, lineno=1, col_offset=9), lineno=1, col_offset=5), lineno=1, col_offset=0)