Skip to content

Comments

[red-knot] avoid inferring types if unpacking fails#16530

Merged
carljm merged 2 commits intoastral-sh:mainfrom
mtshiba:fix-15199-invalid-unpacking
Mar 7, 2025
Merged

[red-knot] avoid inferring types if unpacking fails#16530
carljm merged 2 commits intoastral-sh:mainfrom
mtshiba:fix-15199-invalid-unpacking

Conversation

@mtshiba
Copy link
Collaborator

@mtshiba mtshiba commented Mar 6, 2025

Summary

This PR closes #15199.

The change I just made is to set all variables to type Unknown if unpacking fails, but in some cases this may be excessive.
For example:

a, b, c = "ab"
reveal_type(a)  # Unknown, but it would be reasonable to think of it as LiteralString
reveal_type(c)  # Unknown
# Failed to unpack before the starred expression
(a, b, *c, d, e) = (1,)
reveal_type(a)  # Unknown
reveal_type(b)  # Unknown
...
# Failed to unpack after the starred expression
(a, b, *c, d, e) = (1, 2, 3)
reveal_type(a)  # Unknown, but should it be Literal[1]?
reveal_type(b)  # Unknown, but should it be Literal[2]?
reveal_type(c)  # Todo
reveal_type(d)  # Unknown
reveal_type(e)  # Unknown

I will modify it if you think it would be better to make it a different type than just Unknown.

Test Plan

I have made appropriate modifications to the test cases affected by this change, and also added some more test cases.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Mar 6, 2025
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! There are couple of changes around simplification and using Unknown even in the case of starred unpacking but otherwise this looks good.

@mtshiba mtshiba force-pushed the fix-15199-invalid-unpacking branch from ce1bd07 to 576b63c Compare March 7, 2025 18:18
@mtshiba mtshiba requested a review from dhruvmanila March 7, 2025 18:19
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 good to me with the changes from Dhruv's review!

@carljm carljm merged commit 348c196 into astral-sh:main Mar 7, 2025
21 checks passed
dcreager added a commit that referenced this pull request Mar 8, 2025
* main:
  [red-knot] Understand `typing.Callable` (#16493)
  [red-knot] Support unpacking `with` target (#16469)
  [red-knot] Attribute access and the descriptor protocol (#16416)
  [`pep8-naming`] Add links to `ignore-names` options in various rules' documentation (#16557)
  [red-knot] avoid inferring types if unpacking fails (#16530)
@mtshiba mtshiba deleted the fix-15199-invalid-unpacking branch March 9, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[red-knot] Avoid inferring types in error case during unpacking

4 participants