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] Narrowing for type(x) is C checks #14432

Merged
merged 9 commits into from
Nov 18, 2024
Merged

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Nov 18, 2024

Summary

Add type narrowing for type(x) is C conditions (and else clauses of type(x) is not C conditionals):

if type(x) is A:
    reveal_type(x)  # revealed: A
else:
    reveal_type(x)  # revealed: A | B

closes: #14431, part of: #13694

Test Plan

New Markdown-based tests.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Nov 18, 2024
Add type narrowing for `type(x) is C` conditions:

  ```py
  if type(x) is A:
      reveal_type(x)  # revealed: A
  else:
      reveal_type(x)  # revealed: A | B
  ```
@sharkdp sharkdp force-pushed the david/type-x-is-c-narrowing branch from ce53505 to 1143efd Compare November 18, 2024 12:29
x = get_a_or_b()

if type(x) == A:
reveal_type(x) # revealed: A | B
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both pyright and mypy are less strict here and infer A instead of A | B. This is unsound, as can be demonstrated by returning B() from get_a_or_b(), and observing that this branch is taken.

Copy link

codspeed-hq bot commented Nov 18, 2024

CodSpeed Performance Report

Merging #14432 will not alter performance

Comparing david/type-x-is-c-narrowing (2e7cd69) with main (3642381)

Summary

✅ 32 untouched benchmarks

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

github-actions bot commented Nov 18, 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/type-x-is-c-narrowing branch from 1f888e2 to fefd47a Compare November 18, 2024 13:15
@sharkdp
Copy link
Contributor Author

sharkdp commented Nov 18, 2024

Ah, the performance regression is probably from removing this (now insufficient) check:

        if !left.is_name_expr() && comparators.iter().all(|c| !c.is_name_expr()) {
            // If none of the comparators are name expressions,
            // we have no symbol to narrow down the type of.
            return None;
        }

I'll look into it.

Edit: fixed now by introducing a similar check.

@sharkdp sharkdp merged commit d8538d8 into main Nov 18, 2024
20 checks passed
@sharkdp sharkdp deleted the david/type-x-is-c-narrowing branch November 18, 2024 15:21
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.

[red-knot] Narrowing for type(x) is C (where C is a class)
2 participants