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

[red-knot] Simplify tuples containing Never #14744

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Dec 2, 2024

Summary

Simplify tuples containing Never to Never:

from typing import Never

def never() -> Never: ...

reveal_type((1, never(), "foo"))  # revealed: Never

I should note that mypy and pyright do not perform this simplification. I don't know why.

There is only one place where we use TupleType::new directly (instead of Type::tuple, which changes behavior here). This appears when creating TypeVar constraints, and it looks to me like it should stay this way, because we're using TupleType to store a list of constraints there, instead of an actual type. We also store tuple[constraint1, constraint2, …] as the type for the constraint1, constraint2, … tuple expression. This would mean that we infer a type of tuple[str, Never] for the following type variable constraints, without simplifying it to Never. This seems like a weird edge case that's maybe not worth looking further into?!

from typing import Never

#         vvvvvvvvvv
def f[T: (str, Never)](x: T):
    pass

Test Plan

  • Added a new unit test. Did not add additional Markdown tests as that seems superfluous.
  • Tested the example above using red knot, mypy, pyright.
  • Verified that this allows us to remove contains_never from the property tests ([red-knot] Property tests #14178 (comment))

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Dec 2, 2024
Copy link
Contributor

github-actions bot commented Dec 2, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@sharkdp sharkdp force-pushed the david/simplify-tuple-containing-never branch from 9ccef1a to 9b22492 Compare December 2, 2024 22:18
@sharkdp sharkdp force-pushed the david/simplify-tuple-containing-never branch from 9b22492 to 310a4a9 Compare December 2, 2024 22:20
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great!

@sharkdp sharkdp merged commit a69dfd4 into main Dec 3, 2024
21 checks passed
@sharkdp sharkdp deleted the david/simplify-tuple-containing-never branch December 3, 2024 07:28
Comment on lines +577 to 582
pub fn tuple<T: Into<Type<'db>>>(
db: &'db dyn Db,
elements: impl IntoIterator<Item = T>,
) -> Self {
TupleType::from_elements(db, elements)
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: this method now feels a bit redundant. I'd probably have got rid of it and changed the callsites to use the new TupleType::from_elements() method

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 3, 2024

I should note that mypy and pyright do not perform this simplification. I don't know why.

Mypy in general appears not to do much eager simplification at all, e.g. it doesn't even simplify this union:

def f(x: int | int):
    reveal_type(x)  # mypy: revealed type is "Union[builtins.int, builtins.int]"

Pyright does a lot more eager simplification than mypy. But it's important to note, I think, that it's only relatively recently that we formalized the idea that Never/NoReturn constitute the bottom type in Python. Mypy has historically always treated it as such, but I think pyright only started treating it as such relatively recently. So the issue of simplification may just not have been considered. This is just speculation, though!

dcreager added a commit that referenced this pull request Dec 3, 2024
* main:
  [`ruff`] Extend unnecessary-regular-expression to non-literal strings (`RUF055`) (#14679)
  Minor followups to RUF052 (#14755)
  [red-knot] Property tests (#14178)
  [red-knot] `is_subtype_of` fix for `KnownInstance` types (#14750)
  Improve docs for flake8-use-pathlib rules (#14741)
  [`ruff`] Implemented `used-dummy-variable` (`RUF052`) (#14611)
  [red-knot] Simplify tuples containing `Never` (#14744)
  Possible fix for flaky file watching test (#14543)
  [`flake8-import-conventions`] Improve syntax check for aliases supplied in configuration for `unconventional-import-alias (ICN001)` (#14745)
  [red-knot] Deeper understanding of `LiteralString` (#14649)
  red-knot: support narrowing for bool(E) (#14668)
  [`refurb`] Handle non-finite decimals in `verbose-decimal-constructor (FURB157)` (#14596)
  [red-knot] Re-enable linter corpus tests (#14736)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants