Skip to content

Comments

[red-knot] Narrowing on in tuple[...] and in str#17059

Merged
carljm merged 12 commits intoastral-sh:mainfrom
MatthewMckee4:narrowing-in-tuples
Mar 31, 2025
Merged

[red-knot] Narrowing on in tuple[...] and in str#17059
carljm merged 12 commits intoastral-sh:mainfrom
MatthewMckee4:narrowing-in-tuples

Conversation

@MatthewMckee4
Copy link
Contributor

@MatthewMckee4 MatthewMckee4 commented Mar 29, 2025

Summary

Part of #13694

Seems there a bit more to cover regarding in and other types, but i can cover them in different PRs

Test Plan

Add in.md file in narrowing conditionals folder

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅

@MatthewMckee4 MatthewMckee4 marked this pull request as draft March 30, 2025 00:13
@MatthewMckee4 MatthewMckee4 changed the title [red-knot] Narrowing on in tuple[...] [red-knot] Narrowing on in tuple[...] and in str Mar 30, 2025
@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Mar 30, 2025 •
edited
Loading

Okay, i realise this is not right

from typing import Literal

def _(x: Literal["a", "b", "c", "d"]):
    if x in "abc":
        reveal_type(x)  # revealed: Literal["a", "b", "c"]
    else:
        reveal_type(x)  # revealed: Literal["d"]

The true type is Literal["a", "b", "c", "ab", "ac", "bc", "abc"]. So i should just infer as str right?

Edit: In fact im wrong in this instance, but if the argument x was str then i think this is the case?

@MatthewMckee4 MatthewMckee4 marked this pull request as ready for review March 30, 2025 00:50
@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Mar 30, 2025 •
edited
Loading

Im a bit unsure about if we should support other types in str. For example we have these right now:

def _(x: Literal["a"]) -> None:
    if x in "abc":
        reveal_type(x) # revealed: Literal["a"]
def _(x: Literal["a", "b"]) -> None:
    if x in "abc":
        reveal_type(x) # revealed: Literal["a", "b"]
def _(x: str) -> None:
    if x in "abc":
        reveal_type(x) # revealed: str

but we also have:

def _(x: object) -> None:
    if x in "abc":
        reveal_type(x) # revealed: Literal["a", "b", "c"]

*we also get [unsupported-operator] error here
but i feel like this should be str.

My thinking for all StringLiteral and Union containing only StringLiteral, we can narrow the type, but otherwise we can only infer as str? How does this sounds.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Mar 30, 2025
carljm
carljm requested changes Mar 30, 2025
View reviewed changes
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.

Haven't reviewed the code yet, just the tests, but there are some things we'll have to be a bit more conservative about here.

@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Mar 30, 2025

Okay thank you, I'll look over this again

@AlexWaygood AlexWaygood removed their request for review March 31, 2025 12:18
carljm
carljm reviewed Mar 31, 2025
View reviewed changes
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.

The behavior here looks good! I think we can simplify the implementation and make it a bit more general.

@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Mar 31, 2025

Thank you

@carljm carljm enabled auto-merge (squash) March 31, 2025 23:34
@carljm carljm merged commit 3ad123b into astral-sh:main Mar 31, 2025
22 checks passed
dcreager added a commit that referenced this pull request Apr 1, 2025
@dcreager
Merge branch 'main' into dcreager/typevar-type
e6b7d40 
* main:
  [red-knot] Add property tests for callable types (#17006)
  [red-knot] Disjointness for callable types (#17094)
  [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126)
  mdtest.py: do a full mdtest run immediately when the script is executed (#17128)
  [red-knot] Fix callable subtyping for standard parameters (#17125)
  [red-knot] Fix more `redundant-cast` false positives (#17119)
  Sync vendored typeshed stubs (#17106)
  [red-knot] support Any as a class in typeshed (#17107)
  Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110)
  [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116)
  CI: Run pre-commit on depot machine (#17120)
  Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054)
  Control flow graph: setup (#17064)
  [red-knot] Playground improvements (#17109)
  [red-knot] IDE crate (#17045)
  Update dependency vite to v6.2.4 (#17104)
  [red-knot] Add redundant-cast error (#17100)
  [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)
@MatthewMckee4 MatthewMckee4 deleted the narrowing-in-tuples branch April 1, 2025 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp

@dcreager dcreager Awaiting requested review from dcreager

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MatthewMckee4 @carljm @AlexWaygood