[`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504` #17656

LaBatata101 · 2025-04-27T20:41:41Z

Summary

Fixes #14052.

Test Plan

Snapshot tests.

… functions in `RET504`

github-actions · 2025-04-27T20:53:57Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+6 -8 violations, +0 -0 fixes in 3 projects; 1 project error; 51 projects unchanged)

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

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

- performance/src/performance_dags/performance_dag/performance_dag_utils.py:464:12: RET504 Unnecessary assignment to `safe_dag_prefix` before `return` statement

apache/superset (+4 -4 violations, +0 -0 fixes)

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

+ scripts/erd/erd.py:195:5: D413 [*] Missing blank line after last section ("Parameters")
- scripts/erd/erd.py:195:5: D413 [*] Missing blank line after last section ("Parameters")
+ tests/integration_tests/base_tests.py:553:9: PLR0913 Too many arguments in function definition (12 > 5)
- tests/integration_tests/base_tests.py:553:9: PLR0913 Too many arguments in function definition (12 > 5)
+ tests/integration_tests/reports/utils.py:194:5: ANN201 Missing return type annotation for public function `create_dashboard_report`
- tests/integration_tests/reports/utils.py:194:5: ANN201 Missing return type annotation for public function `create_dashboard_report`
+ tests/unit_tests/fixtures/bash_mock.py:23:9: ANN205 Missing return type annotation for staticmethod `tag_latest_release`
- tests/unit_tests/fixtures/bash_mock.py:23:9: ANN205 Missing return type annotation for staticmethod `tag_latest_release`

zulip/zulip (+2 -3 violations, +0 -0 fixes)

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

- zerver/lib/generate_test_data.py:16:12: RET504 Unnecessary assignment to `config` before `return` statement
- zerver/lib/query_helpers.py:15:5: D404 First word of the docstring should not be "This"
+ zerver/lib/query_helpers.py:15:5: D404 First word of the docstring should not be "This"
+ zerver/lib/test_helpers.py:784:5: D400 First line should end with a period
- zerver/lib/test_helpers.py:784:5: D400 First line should end with a period

openai/openai-cookbook (error)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select A,E703,F704,B015,B018,D100

Failed to clone openai/openai-cookbook: error: 6635 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output

Changes by rule (7 rules affected)

code	total	+ violation	- violation
D413	2	1	1
PLR0913	2	1	1
ANN201	2	1	1
ANN205	2	1	1
D404	2	1	1
D400	2	1	1
RET504	2	0	2

Linter (preview)

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

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

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

- performance/src/performance_dags/performance_dag/performance_dag_utils.py:464:12: RET504 Unnecessary assignment to `safe_dag_prefix` before `return` statement

apache/superset (+1 -1 violations, +0 -0 fixes)

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

- superset/sql_lab.py:680:5: DOC201 `return` is not documented in docstring
+ superset/sql_lab.py:680:5: DOC201 `return` is not documented in docstring

zulip/zulip (+0 -1 violations, +0 -0 fixes)

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

- zerver/lib/generate_test_data.py:16:12: RET504 Unnecessary assignment to `config` before `return` statement

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
DOC201	2	1	1	0	0
RET504	2	0	2	0	0

MichaReiser

I'm not sure this is the right fix because it only works for directly nested functions but not functions e.g. nested inside a with statement.

Recursively visiting nested functions is also somewhat expensive because it requires traversing the entire body of the function (deep!) and we even do this multiple times if functions are nested, even if there are no violations.

I'd prefer if we could make use of the information already present in the SemanticModel (binding, scopes). However, this may require making this a deferred rule that runs after the entire semantic model is fully built.

LaBatata101 · 2025-05-03T14:20:56Z

I'd prefer if we could make use of the information already present in the SemanticModel (binding, scopes). However, this may require making this a deferred rule that runs after the entire semantic model is fully built.

Alright!

LaBatata101 · 2025-05-03T22:01:36Z

I've put the rule to execute in bindings.rs not sure if there's a better place to do it.

I didn't get it why the snapshot changed, but it seems correct??

LaBatata101 · 2025-05-03T22:02:16Z

crates/ruff_linter/src/rules/flake8_return/rules/function.rs

+        let Some(assigned_binding) = checker
+            .semantic()
+            .bindings
+            .iter()
+            .filter(|binding| binding.kind.is_assignment())
+            .find(|binding| binding.name(checker.source()) == assigned_id.as_str())
+        else {
+            continue;
+        };


Is there a better way of getting the binding for the assigned_id?

ntBre · 2025-05-05T20:03:57Z

I've put the rule to execute in bindings.rs not sure if there's a better place to do it.

I didn't get it why the snapshot changed, but it seems correct??

It looks like there is a duplicate diagnostic. If you view the whole file it's more obvious than in the diff:

ruff/crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET504_RET504.py.snap

Lines 84 to 122 in 16c055b

    
           RET504.py:269:12: RET504 [*] Unnecessary assignment to `val` before `return` statement 
        
               | 
        
           267 |         return val 
        
           268 |     val = 1 
        
           269 |     return val  # RET504 
        
               |            ^^^ RET504 
        
               | 
        
               = help: Remove unnecessary assignment 
        
           ℹ Unsafe fix 
        
           265 265 | def str_to_bool(val): 
        
           266 266 |     if isinstance(val, bool): 
        
           267 267 |         return val 
        
           268     |-    val = 1 
        
           269     |-    return val  # RET504 
        
               268 |+    return 1 
        
           270 269 |  
        
           271 270 |  
        
           272 271 | def str_to_bool(val): 
        
           RET504.py:269:12: RET504 [*] Unnecessary assignment to `val` before `return` statement 
        
               | 
        
           267 |         return val 
        
           268 |     val = 1 
        
           269 |     return val  # RET504 
        
               |            ^^^ RET504 
        
               | 
        
               = help: Remove unnecessary assignment 
        
           ℹ Unsafe fix 
        
           265 265 | def str_to_bool(val): 
        
           266 266 |     if isinstance(val, bool): 
        
           267 267 |         return val 
        
           268     |-    val = 1 
        
           269     |-    return val  # RET504 
        
               268 |+    return 1 
        
           270 269 |  
        
           271 270 |  
        
           272 271 | def str_to_bool(val):

These are both emitted on line 269. I'm guessing that's the cause of the numerous new ecosystem hits too.

LaBatata101 · 2025-05-05T20:08:55Z

I'm guessing that's the cause of the numerous new ecosystem hits too.

Wow, didn't see that until you mentioned. github-actions doesn't notify me when it edits the ecosystem comment.

LaBatata101 · 2025-05-06T20:55:53Z

@ntBre I fixed the duplicated diagnostics issue, but the ecosystem changes still looks weird.

ntBre · 2025-05-07T20:34:27Z

@ntBre I fixed the duplicated diagnostics issue, but the ecosystem changes still looks weird.

I think you can safely ignore the changes with non-RET504 codes because I don't think any of your changes should affect other rules. The two ecosystem hits for RET504 itself look correct to me. Is there anything else that looks weird?

I'll let Micha re-review this, but we might want to add a test for his suggestion about nested scopes to show off your binding-based implementation. Otherwise this looks reasonable to me based on a quick look. Happy to review in more detail if y'all want!

LaBatata101 · 2025-05-07T21:01:59Z

I think you can safely ignore the changes with non-RET504 codes because I don't think any of your changes should affect other rules. The two ecosystem hits for RET504 itself look correct to me. Is there anything else that looks weird?

Nope, not really.

I have this other open PR #17692 (which is not related to this one) that I think you guys may have missed, could you take a look?

Anyway, thanks for the help!

ntBre · 2025-06-02T16:25:06Z

Sorry for the delay reviewing this. Are you planning to reopen, or was there something wrong with the PR? Just checking before I clear it from my notifications.

LaBatata101 · 2025-06-02T16:53:03Z

Sorry for the delay reviewing this. Are you planning to reopen, or was there something wrong with the PR? Just checking before I clear it from my notifications.

I was fixing another issue for RET-504 and accidentally made a branch with the same name as this one. Git told me the branch already existed, so I just deleted it. I didn’t realize this PR was still open though 😅. That's why it got closed. Not sure how to fix this.

ntBre · 2025-06-02T17:30:40Z

Ahh, I see. I was going to say I'd just review the other PR and then we can try to resurrect this branch afterwards, but I wonder if it might resurrect the second one instead. It might be easiest to open a new PR if you can recover this branch locally. I think you can probably do

git branch new-branch-name 31f62a1eca7a4b9e17f75231510dd1e5db54c1a4  # looks like the most recent hash above

or something else with git reflog, if you need it. I usually have to double check the git commands I use less frequently 😆

https://stackoverflow.com/questions/3640764/can-i-recover-a-branch-after-its-deletion-in-git

LaBatata101 · 2025-06-02T19:43:24Z

git branch new-branch-name 31f62a1eca7a4b9e17f75231510dd1e5db54c1a4

This command worked like a charm. Here's the new PR.

Thanks!

…d functions in `RET504` (#18433)  ## 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

LaBatata101 added 2 commits April 27, 2025 17:40

[flake8_return] Fix false-positive for variables used inside nested…

5251666

… functions in `RET504`

Fix clippy warning

b1afafb

ntBre added the bug Something isn't working label Apr 28, 2025

LaBatata101 changed the title ~~[flake8_return] Fix false-positive for variables used inside nested functions in RET504~~ [flake8-return] Fix false-positive for variables used inside nested functions in RET504 May 1, 2025

MichaReiser reviewed May 3, 2025

View reviewed changes

LaBatata101 added 2 commits May 3, 2025 18:57

Trigger RET504 after the semantic model is fully built

f0254c8

Update snapshot

16c055b

LaBatata101 commented May 3, 2025

View reviewed changes

LaBatata101 requested a review from MichaReiser May 3, 2025 22:02

LaBatata101 added 2 commits May 6, 2025 17:02

Fix duplicated diagnostics

92d8387

Remove commented code

dccc22d

Add more tests

31f62a1

MichaReiser requested a review from ntBre May 26, 2025 09:37

LaBatata101 closed this Jun 2, 2025

LaBatata101 deleted the fix-RET504 branch June 2, 2025 14:05

LaBatata101 mentioned this pull request Jun 2, 2025

[flake8-return] Fix false-positive for variables used inside nested functions in RET504 #18433

Merged

[flake8-return] Fix false-positive for variables used inside nested functions in RET504 #17656

[flake8-return] Fix false-positive for variables used inside nested functions in RET504 #17656

Uh oh!

Conversation

LaBatata101 commented Apr 27, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

LaBatata101 commented May 3, 2025

Uh oh!

LaBatata101 commented May 3, 2025

Uh oh!

LaBatata101 May 3, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre commented May 5, 2025

Uh oh!

LaBatata101 commented May 5, 2025

Uh oh!

LaBatata101 commented May 6, 2025

Uh oh!

ntBre commented May 7, 2025

Uh oh!

LaBatata101 commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Jun 2, 2025

Uh oh!

LaBatata101 commented Jun 2, 2025

Uh oh!

ntBre commented Jun 2, 2025

Uh oh!

LaBatata101 commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504` #17656

[`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504` #17656

github-actions bot commented Apr 27, 2025 •

edited

Loading

`ruff-ecosystem` results

LaBatata101 commented May 7, 2025 •

edited

Loading

LaBatata101 commented Jun 2, 2025 •

edited

Loading