Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-return] Recognize functions returning Never as non-returning (RET503) #15298

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

viccie30
Copy link
Contributor

@viccie30 viccie30 commented Jan 6, 2025

Summary

This pull request makes flake8-return recognize functions annotated to return typing.Never as non-returning.

Test Plan

I have duplicated the existing tests for functions returning NoReturn.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

376 376 | if x == 5:
377 377 | return 5
378 378 | bar()
379 |+ return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code changes look correct, but they seem to have no effect. I'd expect that the rule wouldn't flag this specific line because there's no implicit return (it never returns). I added a ton of debug statements to is_noreturn_func and I suspect the problem is that the semantic model hasn't seen bar at the moment we try to resolve its name. My suspicious gets confirmed when changing the example to

import typing

def bar() -> typing.Never:
    abort()

def foo(x: int) -> int:
    if x == 5:
        return 5
    bar()

which raises a violation before your change but now doesn't.

I suggest that you change your examples (including the existing NoReturn) to not use a nested function. We can add a dedicated test with a nested function with a doc comment explaining that this is currently not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the current tests to no longer use nested functions and have added a currently ignored test checking nested functions.

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jan 6, 2025
…and add explanation why it isn't supported yet
@MichaReiser MichaReiser enabled auto-merge (squash) January 7, 2025 07:53
@MichaReiser MichaReiser merged commit 1e948f7 into astral-sh:main Jan 7, 2025
20 checks passed
@viccie30 viccie30 deleted the ret503-never branch January 7, 2025 08:36
dcreager added a commit that referenced this pull request Jan 7, 2025
* main:
  Use uv consistently throughout the documentation (#15302)
  [red-knot] Eagerly normalize `type[]` types (#15272)
  [`pyupgrade`] Split `UP007` to two individual rules for `Union` and `Optional` (`UP007`, `UP045`) (#15313)
  [red-knot] Improve symbol-lookup tracing (#14907)
  [red-knot] improve type shrinking coverage in red-knot property tests (#15297)
  [`flake8-return`] Recognize functions returning `Never` as non-returning (`RET503`) (#15298)
  [`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601)
  Avoid treating newline-separated sections as sub-sections (#15311)
  Remove call when removing final argument from `format` (#15309)
  Don't enforce `object-without-hash-method` in stubs (#15310)
  Don't special-case class instances in binary expression inference (#15161)
  Upgrade zizmor to the latest version in CI (#15300)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants