[flake8-return] Only add return None at end of function (RET503) by JonathanPlasse · Pull Request #11074 · astral-sh/ruff

JonathanPlasse · 2024-04-21T16:27:48Z

Summary

Fix Autofix for RET503 breaks RET505 (in one case) #5765

In RET503 documentation, it is said that it only adds return None at the end of functions.
The current behavior can add return None in multiple place in the function, but not at the end of the function.
This behavior does not match the documentation and goes against other flake8-return rules like RET505 and caused people to open #5765.
The new behavior only adds return None at the end of functions like in the documentation and results in more readable code.
The new behavior is only available in preview mode.
The range of RET503 is changed to be the entire function, as it will be more clear it concerns the entire function.

Before

def main(value: str) -> int | None:
    if value == "one":
        return 1

    if value.startswith("do_stuff"):
        print("doing stuff")

        if value.endswith("haha"):
            print("haha")
            return None
        else:
            print("Instruction unclear.")
            return None
    else:
        print("Doing nothing.")
        return None

After

def main(value: str) -> int | None:
    if value == "one":
        return 1

    if value.startswith("do_stuff"):
        print("doing stuff")

        if value.endswith("haha"):
            print("haha")
        else:
            print("Instruction unclear.")
    else:
        print("Doing nothing.")
    return None

Test Plan

I added a preview snapshot for RET503.
I compared the diff between the RET503 stable and preview snapshot.
I checked the ecosystem reports and fixed one regression.

github-actions · 2024-04-21T16:40:12Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+124 -134 violations, +0 -0 fixes in 6 projects; 48 projects unchanged)

apache/airflow (+70 -78 violations, +0 -0 fixes)

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

+ airflow/io/path.py:370:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/io/path.py:390:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/jobs/triggerer_job_runner.py:128:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/jobs/triggerer_job_runner.py:129:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/metrics/otel_logger.py:181:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/metrics/otel_logger.py:202:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/metrics/otel_logger.py:207:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/metrics/otel_logger.py:228:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/models/dag.py:3958:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/models/dag.py:3959:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/models/param.py:268:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/models/param.py:271:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/amazon/aws/executors/ecs/ecs_executor.py:273:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/amazon/aws/executors/ecs/ecs_executor.py:284:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/amazon/aws/executors/ecs/ecs_executor.py:285:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/amazon/aws/operators/emr.py:1412:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/amazon/aws/operators/emr.py:1415:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/amazon/aws/operators/eventbridge.py:61:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/amazon/aws/operators/eventbridge.py:82:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/apache/beam/operators/beam.py:347:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/apache/beam/operators/beam.py:365:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/apache/beam/operators/beam.py:369:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/apache/beam/operators/beam.py:403:17: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/apache/beam/operators/beam.py:540:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/apache/beam/operators/beam.py:553:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/apache/beam/operators/beam.py:557:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- airflow/providers/apache/beam/operators/beam.py:605:17: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ airflow/providers/apache/beam/operators/beam.py:734:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
... 120 additional changes omitted for project

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

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

+ superset/migrations/versions/2018-07-05_15-19_3dda56f1c4c6_migrate_num_period_compare_and_period_.py:112:1: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- superset/migrations/versions/2018-07-05_15-19_3dda56f1c4c6_migrate_num_period_compare_and_period_.py:129:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ tests/integration_tests/utils/hashing_tests.py:59:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- tests/integration_tests/utils/hashing_tests.py:60:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ tests/unit_tests/queries/query_object_test.py:103:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- tests/unit_tests/queries/query_object_test.py:104:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value

bokeh/bokeh (+43 -48 violations, +0 -0 fixes)

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

+ examples/server/app/clustering/main.py:82:1: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- examples/server/app/clustering/main.py:83:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/has_props.py:314:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/has_props.py:340:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/has_props.py:342:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/has_props.py:367:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/serialization.py:386:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:409:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/serialization.py:438:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:469:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/serialization.py:530:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:562:21: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/serialization.py:572:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:578:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ src/bokeh/core/serialization.py:710:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:717:17: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- src/bokeh/core/serialization.py:723:17: RET503 Missing explicit `return` at the end of function able to return non-`None` value
... 74 additional changes omitted for project

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

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

+ ibis/backends/duckdb/__init__.py:520:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ ibis/backends/duckdb/__init__.py:578:39: RUF100 Unused `noqa` directive (unused: `RET503`)
+ ibis/backends/pyspark/__init__.py:864:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ ibis/backends/pyspark/__init__.py:910:39: RUF100 Unused `noqa` directive (unused: `RET503`)

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

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

+ _version_helper.py:41:1: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- _version_helper.py:42:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value

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

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

- zerver/lib/addressee.py:137:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ zerver/lib/addressee.py:96:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ zerver/tests/test_import_export.py:1606:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- zerver/tests/test_import_export.py:1609:17: RET503 Missing explicit `return` at the end of function able to return non-`None` value
+ zerver/tests/test_openapi.py:456:5: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- zerver/tests/test_openapi.py:535:9: RET503 Missing explicit `return` at the end of function able to return non-`None` value
- zerver/tests/test_openapi.py:536:13: RET503 Missing explicit `return` at the end of function able to return non-`None` value

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RET503	256	122	134	0	0
RUF100	2	2	0	0	0

JonathanPlasse · 2024-04-24T16:32:37Z

+                    if checker.settings.preview.is_enabled() {
+                        return result;
+                    }


Early return if in preview mode because we only need to know if the function has at least one implicit return to add a single return None at the end of the function.
The stable version can add multiple return so it cannot return early.
The code should be simplified once the preview version become stable.

JonathanPlasse · 2024-04-24T16:33:49Z

A comment should be added that when the preview version becomes stable the code can be simplified with early return everywhere.

JonathanPlasse · 2024-06-27T13:03:06Z

Will it be included in Ruff 0.5.0?

AlexWaygood · 2024-06-27T15:05:13Z

Will it be included in Ruff 0.5.0?

Just seen this -- unfortunately I just set the release workflow in motion, so unfortunately not. Sorry :-(

JonathanPlasse · 2024-06-27T15:16:48Z

No worries.

JonathanPlasse · 2024-08-13T12:15:03Z

Will it be included in Ruff 0.6.0?

codspeed-hq · 2024-08-13T15:34:49Z

CodSpeed Performance Report

Merging #11074 will not alter performance

_{Comparing JonathanPlasse:ret503-only-add-return-at-end-of-function (4e575bc) with main (2520ebb)}

Summary

✅ 32 untouched benchmarks

MichaReiser · 2024-08-13T15:44:40Z

I don't think this requires a minor release, considering that the change is gated behind preview mode.

MichaReiser · 2024-08-13T15:47:41Z


+/// RET503
+fn implicit_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef, stmt: &Stmt) {
+    if has_implicit_return(checker, stmt) && checker.settings.preview.is_enabled() {


I don't have a great solution but I do find it a bit surprising that has_implicit_return adds diagnostics in the non preview mode.

I'm somewhat inclined to duplicate the logic between preview and non-preview mode. That should also make it much easier when stabilizing the new behavior (and it's not that much code that needs copying). Unless you see a way of how we can avoid the side-effects of has_implicit_return in stable (e.g. could it return a Vec with all the statements for which diagnostics should be added?)

I don't mind going ahead with the PR as is but I'm interested to hear what you think of duplicating the logic OR maybe you have another idea to make this a bit clearer

I used the Vec solution to remove the side effects.
The code is nicer.
Thank you for suggesting it.

Thanks. Will merge now and sorry for the long wait

You are welcome

charliermarsh self-assigned this Apr 22, 2024

JonathanPlasse commented Apr 24, 2024

View reviewed changes

MichaReiser added preview Related to preview mode features fixes Related to suggested fixes for violations labels Aug 13, 2024

MichaReiser reviewed Aug 13, 2024

View reviewed changes

JonathanPlasse and others added 7 commits August 14, 2024 07:12

[flake8-return] Only add return None at end of function (RET503)

60e37a0

Enable new behavior only in preview mode

22e0002

Fix ecosystem regression

66dfbd8

Set diagnostic range to be the entire function

da58a6b

Remove redundant arguments

687f697

Remove side effects for has_implicit_returns()

5d1fc6c

Add note about how to change the code when stabilizing the change

4e575bc

MichaReiser enabled auto-merge (squash) August 14, 2024 07:43

MichaReiser merged commit 7fc39ad into astral-sh:main Aug 14, 2024

JonathanPlasse deleted the ret503-only-add-return-at-end-of-function branch August 14, 2024 09:57

BrewTestBot mentioned this pull request Aug 15, 2024

ruff 0.6.0 Homebrew/homebrew-core#181268

Merged

Conversation

JonathanPlasse commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 21, 2024 • 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!

JonathanPlasse Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

JonathanPlasse commented Apr 24, 2024

Uh oh!

JonathanPlasse commented Jun 27, 2024

Uh oh!

AlexWaygood commented Jun 27, 2024

Uh oh!

JonathanPlasse commented Jun 27, 2024

Uh oh!

JonathanPlasse commented Aug 13, 2024

Uh oh!

codspeed-hq bot commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #11074 will not alter performance

Summary

Uh oh!

MichaReiser commented Aug 13, 2024

Uh oh!

MichaReiser Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

JonathanPlasse Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanPlasse Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JonathanPlasse commented Apr 21, 2024 •

edited

Loading

github-actions bot commented Apr 21, 2024 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented Aug 13, 2024 •

edited

Loading

JonathanPlasse Aug 14, 2024 •

edited

Loading

MichaReiser Aug 14, 2024 •

edited

Loading