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

Fix checking multiple assignments based on tuple unpacking involving partially initialised variables (Fixes #12915). #14423

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tyralla
Copy link
Contributor

@tyralla tyralla commented Jan 10, 2023

Fix checking multiple assignments based on tuple unpacking involving partially initialised variables (Fixes #12915).

This commit introduces two changes:

  • It converts unions of tuples to tuples of unions during multi-assignment checking when all (original) tuples have the same length. This fixes Incorrect type narrowing when unpacking union of tuple on an existing variable #12915.
  • It removes the undefined_rvalue logic completely. This logic bothered me because it introduced the necessity to make the mentioned conversion twice (which was not always successful due to reasons I do not fully understand).

Test new case testDefinePartiallyInitialisedVariableDuringTupleUnpacking covers issue #12915.

I had to adjust some other test cases. In my opinion, the error messages are now more transparent, consistent, and complete. Removing the undefined_rvalue logic also seems to fix one more bug. In the test cases testInferenceWithTypeVariableTwiceInReturnType and testInferenceWithTypeVariableTwiceInReturnTypeAndMultipleVariables, there were three assignments for which Mypy did not report errors but, in my opinion, should.

So, for the current test data set, removing the undefined_rvalue logic seems to introduce mere benefits than drawbacks (meaning, different error messages than before). I hope it does not break any Mypy functionalities not covered by the tests.

…partially initialised variables (Fixes python#12915).

This commit introduces two changes:
 * It converts unions of tuples to tuples of unions during multi-assignment checking when all (original) tuples have the same length.  This fixes issue python#12915.
 * It removes the `undefined_rvalue` logic completely.  This logic bothered me because it introduced the necessity to make the mentioned conversion twice (which was not always successful due to reasons I do not fully understand).

Test new case `testDefinePartiallyInitialisedVariableDuringTupleUnpacking` covers issue python#12915.

I had to adjust some other test cases.  In my opinion, the error messages are now more transparent, consistent, and complete.  Removing the `undefined_rvalue` logic also seems to fix one more bug.  In the test cases `testInferenceWithTypeVariableTwiceInReturnType` and `testInferenceWithTypeVariableTwiceInReturnTypeAndMultipleVariables`, there were three assignments for which Mypy did not report errors but, in my opinion, should.

So, for the current test data set, removing the `undefined_rvalue` logic seems to introduce mere benefits than drawbacks (meaning, different error messages than before).  I hope it does not break any Mypy functionalities not covered by the tests.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

tyralla added a commit to tyralla/mypy that referenced this pull request Jan 12, 2023
…partially initialised variables (Fixes python#12915).

This proposal is an alternative to python#14423.  Similar to python#14423, the main idea is to convert unions of tuples to tuples of (simplified) unions during multi-assignment checks.  In addition, it extends this idea to other iterable types, which allows removing the `undefined_rvalue` logic and the `no_partial_types` logic.  Hence, the problem reported in python#12915 with partially initialised variables should be fixed for unions that combine, for example, tuples and lists, as well.

Besides the new test case also provided by python#14423 (`testDefinePartiallyInitialisedVariableDuringTupleUnpacking`), this commit also adds the test cases `testUnionUnpackingIncludingListPackingSameItemTypes`, `testUnionUnpackingIncludingListPackingDifferentItemTypes`, and `testUnionUnpackingIncludingListPackingForVariousItemTypes`.
@tyralla
Copy link
Contributor Author

tyralla commented Jan 12, 2023

After rethinking my approach, I decided to extend it to cover unions containing other iterable items. The result is #14440, which involves even more changes than this pull request. I consider #14440 better but leave this one open if I am wrong.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect type narrowing when unpacking union of tuple on an existing variable
1 participant