Support `typing.NoReturn` and `typing.Never` by Glyphack · Pull Request #14559 · astral-sh/ruff

Glyphack · 2024-11-23T17:07:38Z

Summary

Add typing.NoReturn and typing.Never to known instances and infer them as Type::Never
Add is_assignable_to cases for Type::Never

I skipped emitting diagnostic for when a function is annotated as NoReturn but it actually returns.

Test Plan

Added tests from
https://github.com/python/typing/blob/main/conformance/tests/specialtypes_never.py
except from generics and checking if the return value of the function and the annotations match.

github-actions · 2024-11-23T17:14:18Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

AlexWaygood

Thanks! Not a full review yet, but one important thing I noticed:

panic was fixed

carljm

Also not a full review yet, just a couple things I noticed in passing.

sharkdp · 2024-11-25T08:15:52Z

+# revealed: NoReturn
+reveal_type(stop())


I think this should be a separate test that ends here. If you were to actually run this code, the program would never return from the stop() call.

Everything after this line can be considered unreachable code from the perspective of static analysis. Which also means we might not (should not) emit any diagnostics, including reveal_type-information. I guess it's even questionable as to whether or not the reveal_type in this line should emit a diagnostic. You would not see any result at runtime. Existing type checkers still reveal the type for the stop() call, but nothing beyond.

Similar reasoning applies to some of the tests below, I think.

My intention was to test the revealed type only. You're right this test will break when the behavior for Never is added.
I will move this call and other reveal_types that can stop the type checking to a function.

I think this should be a separate test that ends here.

Are you suggesting to implement this behavior in this PR? Otherwise I can add a todo comment here and work on it in a follow up PR.

Are you suggesting to implement this behavior in this PR?

Ah, no — sorry. I was merely suggesting that we split this large test into multiple smaller tests. And take care not to have any test assertions (# revealed: / # error: ) in lines following a call to a function that returns Never/NoReturn. To make sure this test will work in the future once we add unreachable-code analysis.

Writing smaller self-consistent tests would also be beneficial if we ever need to debug an assertion failure in this file.

carljm · 2024-11-25T16:01:07Z

I skipped emitting diagnostic for when a function is annotated as NoReturn but it actually returns.

This is correct to skip in this PR; there shouldn't be any special-casing of such a check, it will just fall out naturally from checking return type assignability in general, since nothing (except for Never) is assignable to Never.

carljm

Looks great, thank you! Just a few small tweaks which I'll push, and then merge.

carljm · 2024-11-25T18:39:22Z

+`NoReturn` to annotate functions that never return normally. `Never` represents the bottom type, a
+type that represents the empty set of Python objects. These two annotations can be used
+interchangeably.


Suggested change

`NoReturn` to annotate functions that never return normally. `Never` represents the bottom type, a

type that represents the empty set of Python objects. These two annotations can be used

interchangeably.

`NoReturn` is used to annotate functions that never return (they always raise an exception or exit the process).

`Never` is the bottom type, and represents the empty set of Python objects. These two annotations can be used

interchangeably.

carljm · 2024-11-25T21:22:48Z

                        },
                    )
            }
+            (Type::Never, Type::Never) => false,


I think this case is a) wrong (because Never should be assignable to Never) and b) never reached, due to the is_equivalent_to check that takes precedence above.

There is a test verifying that Never is actually assignable to Never.

Suggested change

(Type::Never, Type::Never) => false,

carljm · 2024-11-25T21:24:33Z

                    )
            }
+            (Type::Never, Type::Never) => false,
+            (_, Type::Never) => false,


And I think this case should also not be necessary, because we fallback to is_subtype_of, which already contains this same case for Never. Removing this line and the previous one, all tests still pass.

Suggested change

(_, Type::Never) => false,

Co-authored-by: Alex Waygood <[email protected]>

AlexWaygood added the ty Multi-file analysis & type inference label Nov 23, 2024

Glyphack commented Nov 24, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types.rs Outdated

Glyphack marked this pull request as ready for review November 24, 2024 13:54

Glyphack requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners November 24, 2024 13:54

AlexWaygood previously requested changes Nov 24, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types/infer.rs

carljm reviewed Nov 24, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/resources/mdtest/annotations/never.md Outdated

Comment thread crates/red_knot_python_semantic/src/types.rs Outdated

dhruvmanila reviewed Nov 25, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types/infer.rs Outdated

sharkdp reviewed Nov 25, 2024

View reviewed changes

Glyphack force-pushed the typing-no-return-never branch from 57c7764 to f8debee Compare November 25, 2024 18:09

AlexWaygood reviewed Nov 25, 2024

View reviewed changes

Comment thread crates/red_knot_python_semantic/src/types/infer.rs Outdated

Glyphack force-pushed the typing-no-return-never branch from 59d08ba to 7102da0 Compare November 25, 2024 18:32

carljm approved these changes Nov 25, 2024

View reviewed changes

Glyphack and others added 8 commits November 25, 2024 13:32

Support typing.NoReturn and typing.Never

2070d25

Add test case for assigning integer to Never

fe8b958

Emit diagnostic when Never or NoReturn has type parameters

653df25

Do not pass subscript and slice

2237a0b

Update crates/red_knot_python_semantic/src/types/infer.rs

6e0e884

Co-authored-by: Alex Waygood <[email protected]>

Split never test into smaller tests

9453591

tweaks

1187341

one more wording fix

a217be4

carljm force-pushed the typing-no-return-never branch from 1de7286 to a217be4 Compare November 25, 2024 21:33

carljm enabled auto-merge (squash) November 25, 2024 21:33

carljm merged commit 557d583 into astral-sh:main Nov 25, 2024

Glyphack deleted the typing-no-return-never branch November 26, 2024 09:49

MichaReiser mentioned this pull request Dec 2, 2024

Revert: [pyflakes] Avoid false positives in @no_type_check contexts (F821, F722) (#14615) #14726

Merged

Conversation

Glyphack commented Nov 23, 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 Nov 23, 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!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharkdp Nov 25, 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!

Glyphack Nov 25, 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!

sharkdp Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

carljm commented Nov 25, 2024

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

carljm Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

carljm Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Glyphack commented Nov 23, 2024 •

edited

Loading

github-actions Bot commented Nov 23, 2024 •

edited

Loading

`ruff-ecosystem` results

sharkdp Nov 25, 2024 •

edited

Loading

Glyphack Nov 25, 2024 •

edited

Loading