[flake8-return] Fix false-positive for variables used inside nested functions in RET504
#18433
Conversation
… functions in `RET504`
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| D413 | 2 | 1 | 1 | 0 | 0 |
| D202 | 2 | 1 | 1 | 0 | 0 |
| PLR0913 | 2 | 1 | 1 | 0 | 0 |
| D102 | 2 | 1 | 1 | 0 | 0 |
| ANN201 | 2 | 1 | 1 | 0 | 0 |
| ANN205 | 2 | 1 | 1 | 0 | 0 |
| D404 | 2 | 1 | 1 | 0 | 0 |
Linter (preview)
✅ ecosystem check detected no linter changes.
|
I should have time to review this this week. Could you resolve the merge conflicts? |
Done! |
f90619e to
e5fc4a7
Compare
|
I'll take a closer look at the implementation too, but I think there might be two new false negatives in the ecosystem check:
The second one especially looks like a pretty clear example of what the rule is meant to catch, but maybe I'm missing something here. |
I've tested here, and it seems to be working. Unless there are other parts of the code causing some interference. def get_dag_prefix(performance_dag_conf: dict[str, str]) -> str:
"""
Return DAG prefix.
Returns prefix that will be assigned to DAGs created with given performance DAG configuration.
:param performance_dag_conf: dict with environment variables as keys and their values as values
:return: final form of prefix after substituting inappropriate characters
:rtype: str
"""
dag_prefix = get_performance_dag_environment_variable(performance_dag_conf, "PERF_DAG_PREFIX")
safe_dag_prefix = safe_dag_id(dag_prefix)
return safe_dag_prefix
def load_config() -> dict[str, Any]:
with open("zerver/tests/fixtures/config.generate_data.json", "rb") as infile:
config = orjson.loads(infile.read())
return config$ cargo run -p ruff -- check sample2.py --preview --no-cache --select RET504
sample2.py:16:12: RET504 Unnecessary assignment to `safe_dag_prefix` before `return` statement
|
14 | safe_dag_prefix = safe_dag_id(dag_prefix)
15 |
16 | return safe_dag_prefix
| ^^^^^^^^^^^^^^^ RET504
|
= help: Remove unnecessary assignment
sample2.py:23:12: RET504 Unnecessary assignment to `config` before `return` statement
|
21 | config = orjson.loads(infile.read())
22 |
23 | return config
| ^^^^^^ RET504
|
= help: Remove unnecessary assignment
Found 2 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option). |
| if assigned_binding | ||
| .references() | ||
| .map(|reference_id| checker.semantic().reference(reference_id)) | ||
| .any(|reference| reference.scope_id() != assigned_binding.scope) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm not sure about this, but I suspect that this is the cause of the new false negatives. Could this be checking other scopes not nested within the current function? Both of the false negative cases have instances of the same variable names in other functions in the same file. I don't know of an API off the top of my head, but we may need to restrict the search to children of the current scope.
There was a problem hiding this comment.
This might actually be corroborated by your comment. I think other functions in the file might be leaking into the current check, but I could still be wrong.
There was a problem hiding this comment.
I think the error is in the code above, where the search for assigned_binding is done.
There was a problem hiding this comment.
I've come up with this solution where it tries to find the scope of the function by looking up its name. Then it looks up assigned_binding in the function scope. It solves the false negatives, but the only problem is that it doesn't work if we have more than one function with the same name (the test file has multiple functions with the same name). I've run out of ideas for how to solve this problem.
diff --git a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs
index 299acbc6c..29d759dcd 100644
--- a/crates/ruff_linter/src/rules/flake8_return/rules/function.rs
+++ b/crates/ruff_linter/src/rules/flake8_return/rules/function.rs
@@ -7,8 +7,8 @@ use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::whitespace::indentation;
use ruff_python_ast::{self as ast, Decorator, ElifElseClause, Expr, Stmt};
use ruff_python_parser::TokenKind;
-use ruff_python_semantic::SemanticModel;
use ruff_python_semantic::analyze::visibility::is_property;
+use ruff_python_semantic::{BindingKind, SemanticModel};
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer, is_python_whitespace};
use ruff_source_file::LineRanges;
use ruff_text_size::{Ranged, TextRange, TextSize};
@@ -568,6 +568,19 @@ pub(crate) fn unnecessary_assign(checker: &Checker, function_stmt: &Stmt) {
return;
}
+ let Some(function_scope) = checker
+ .semantic()
+ .lookup_symbol(&function_def.name)
+ .map(|binding_id| checker.semantic().binding(binding_id))
+ .and_then(|binding| {
+ let BindingKind::FunctionDefinition(scope_id) = &binding.kind else {
+ return None;
+ };
+ Some(&checker.semantic().scopes[*scope_id])
+ })
+ else {
+ return;
+ };
for (assign, return_, stmt) in &stack.assignment_return {
// Identify, e.g., `return x`.
let Some(value) = return_.value.as_ref() else {
@@ -611,12 +624,9 @@ pub(crate) fn unnecessary_assign(checker: &Checker, function_stmt: &Stmt) {
continue;
}
- let Some(assigned_binding) = checker
- .semantic()
- .bindings
- .iter()
- .filter(|binding| binding.kind.is_assignment())
- .find(|binding| binding.name(checker.source()) == assigned_id.as_str())
+ let Some(assigned_binding) = function_scope
+ .get(assigned_id)
+ .map(|binding_id| checker.semantic().binding(binding_id))
else {
continue;
};There was a problem hiding this comment.
I'm not sure how helpful this is, but I spent some time minimizing the ecosystem hit that was causing problems:
# minimized from an ecosystem hit in
# https://github.com/astral-sh/ruff/pull/18433#issuecomment-2932216413
def foo():
safe_dag_prefix = 1
[safe_dag_prefix for _ in range(5)]
def bar():
safe_dag_prefix = 2
return safe_dag_prefix # Supposed to trigger RET504It seems that it might have something to do with the shared variable name appearing in a list comprehension. The Airflow example has it in a position like [... for x in ... if some_condition(safe_dag_prefix)] but it happens with the variable in the comprehension element position too.
I think you're on the right track with looking up the scope. Is there not a way to look them up by something other than their name? SemanticModel::current_scope sounds promising, but maybe it doesn't work for deferred checks like function bodies. It feels like there should be a way to get the scope from function_stmt, but I could be wrong.
crates/ruff_linter/resources/test/fixtures/flake8_return/RET504.py
Outdated
Show resolved
Hide resolved
b015dc8 to
927178d
Compare
|
@ntBre I think it's fixed now |
…re_help * 'main' of https://github.com/astral-sh/ruff: Add simple integration tests for all output formats (astral-sh#19265) [`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504` (astral-sh#18433) [ty] Add a `--quiet` mode (astral-sh#19233) Treat form feed as valid whitespace before a line continuation (astral-sh#19220) [ty] Make `check_file` a salsa query (astral-sh#19255) [ty] Consolidate submodule resolving code between `types.rs` and `ide_support.rs` (astral-sh#19256) [ty] Remove countme from salsa-structs (astral-sh#19257) [ty] Improve and document equivalence for module-literal types (astral-sh#19243) [ty] Optimize protocol subtyping by removing expensive and unnecessary equivalence check from the top of `Type::has_relation_to()` (astral-sh#19230) [ty] Ecosystem analyzer: parallelize, fix race condition (astral-sh#19252) [ty] Add completion kind to playground (astral-sh#19251) [ty] Deploy ecosystem diff to Cloudflare pages (astral-sh#19234) [ty] Add semantic token provider to playground (astral-sh#19232)
Summary
This PR is the same as #17656.
I accidentally deleted the branch of that PR, so I'm creating a new one.
Fixes #14052
Test Plan
Add regression tests