Refactor RET504 to only enforce assignment-then-return pattern (#4997)

## Summary

The `RET504` rule, which looks for unnecessary assignments before return
statements, is a frequent source of issues (#4173, #4236, #4242, #1606,
#2950). Over time, we've tried to refine the logic to handle more cases.
For example, we now avoid analyzing any functions that contain any
function calls or attribute assignments, since those operations can
contain side effects (and so we mark them as a "read" on all variables
in the function -- we could do a better job with code graph analysis to
handle this limitation, but that'd be a more involved change.) We also
avoid flagging any variables that are the target of multiple
assignments. Ultimately, though, I'm not happy with the implementation
-- we just can't do sufficiently reliable analysis of arbitrary code
flow given the limited logic herein, and the existing logic is very hard
to reason about and maintain.

This PR refocuses the rule to only catch cases of the form:

```py
def f():
    x = 1
    return x
```

That is, we now only flag returns that are immediately preceded by an
assignment to the returned variable. While this is more limiting, in
some ways, it lets us flag more cases vis-a-vis the previous
implementation, since we no longer "fully eject" when functions contain
function calls and other effect-ful operations.

Closes #4173.

Closes #4236.

Closes #4242.
This commit is contained in:
Charlie Marsh 2023-06-10 00:05:01 -04:00 committed by GitHub
parent 5abb8ec0dc
commit 02b8ce82af
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 156 additions and 318 deletions

View file

@ -1202,10 +1202,8 @@ pub fn first_colon_range(range: TextRange, locator: &Locator) -> Option<TextRang
}
/// Return the `Range` of the first `Elif` or `Else` token in an `If` statement.
pub fn elif_else_range(stmt: &Stmt, locator: &Locator) -> Option<TextRange> {
let Stmt::If(ast::StmtIf { body, orelse, .. } )= stmt else {
return None;
};
pub fn elif_else_range(stmt: &ast::StmtIf, locator: &Locator) -> Option<TextRange> {
let ast::StmtIf { body, orelse, .. } = stmt;
let start = body.last().expect("Expected body to be non-empty").end();
@ -1619,7 +1617,7 @@ mod tests {
use anyhow::Result;
use ruff_text_size::{TextLen, TextRange, TextSize};
use rustpython_ast::Suite;
use rustpython_ast::{Stmt, Suite};
use rustpython_parser::ast::Cmpop;
use rustpython_parser::Parse;
@ -1819,6 +1817,7 @@ elif b:
.trim_start();
let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap();
let stmt = Stmt::as_if_stmt(stmt).unwrap();
let locator = Locator::new(contents);
let range = elif_else_range(stmt, &locator).unwrap();
assert_eq!(range.start(), TextSize::from(14));
@ -1833,6 +1832,7 @@ else:
.trim_start();
let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap();
let stmt = Stmt::as_if_stmt(stmt).unwrap();
let locator = Locator::new(contents);
let range = elif_else_range(stmt, &locator).unwrap();
assert_eq!(range.start(), TextSize::from(14));