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). #14440

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

Conversation

tyralla
Copy link
Contributor

@tyralla tyralla commented Jan 12, 2023

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

This proposal is an alternative to #14423. Similar to #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 #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 #14423 (testDefinePartiallyInitialisedVariableDuringTupleUnpacking), this commit also adds the test cases testUnionUnpackingIncludingListPackingSameItemTypes, testUnionUnpackingIncludingListPackingDifferentItemTypes, and testUnionUnpackingIncludingListPackingForVariousItemTypes.

…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`.
@github-actions

This comment has been minimized.

@tyralla
Copy link
Contributor Author

tyralla commented Jan 12, 2023

I checked the primer diff for pylint and simplified the relevant code. The error happens in the last line:

MessageDefinitionTuple = Union[
    Tuple[str, str, str],
    Tuple[str, str, str, ExtraMessageOptions],
]

options: ExtraMessageOptions = {}
if len(msg_tuple) == 4:
    (msg, symbol, descr, options) = msg_tuple  # type: ignore[misc]
elif len(msg_tuple) == 3:
    (msg, symbol, descr) = msg_tuple  # type: ignore[misc]
else:
    raise InvalidMessageError("invalid error message")
return MessageDefinition(self, msgid, msg, descr, symbol, **options)

So, the new error message seems valid.

@tyralla
Copy link
Contributor Author

tyralla commented Jan 12, 2023

The differences reported for steam are also related to an ignored tuple error:

_raw: tuple[Query[Any], Operator, T_co] | tuple[str]

self._raw = raw  # type: ignore  # can't tell if this is a pyright bug

query_1, op, query_2 = self._raw

return op.format(
    query_1.query,
    query_2.query if isinstance(query_2, Query) else query_1._callback(query_2),
)

However, this reminds me that I forgot to consider ellipsis cases, e.g. Tuple[int, ...].

@tyralla
Copy link
Contributor Author

tyralla commented Jan 12, 2023

However, this reminds me that I forgot to consider ellipsis cases, e.g. Tuple[int, ...].

It's okay. Mypy seems to handle Tuple[int, ...] as Instance, so this case is already covered by isinstance(item, Instance) and self.type_is_iterable(item) for handling iterables like List[int]. Only fixed-size tuples seem to be translated to TupleType. I will adjust a test case nevertheless.

@tyralla
Copy link
Contributor Author

tyralla commented Jan 12, 2023

Last example pydantic:

HslColorTuple = Union[Tuple[float, float, float], Tuple[float, float, float, float]]

def as_hsl_tuple(self, *, alpha: Optional[bool] = None) -> HslColorTuple:
    ...

def as_hsl(self) -> str:
    if self._rgba.alpha is None:
        h, s, li = self.as_hsl_tuple(alpha=False)  # type: ignore
        return f'hsl({h * 360:0.0f}, {s:0.0%}, {li:0.0%})'
    else:
        h, s, li, a = self.as_hsl_tuple(alpha=True)  # type: ignore
        return f'hsl({h * 360:0.0f}, {s:0.0%}, {li:0.0%}, {round(a, 2)})'

Still the same reason.

@github-actions

This comment has been minimized.

@tyralla
Copy link
Contributor Author

tyralla commented Jan 14, 2023

Maybe it's better to prevent such follow-up error reports by analysing all possible union members instead of quitting when a single union member is problematic.

The intermediate construction of tuples is unnecessary. Checking each lvalue directly against the union of its relevant rvalue types is sufficient and avoids the hard-to-understand convert_star_rvalue_type argument.

I will change both aspects in the next commit.

@github-actions

This comment has been minimized.

@tyralla
Copy link
Contributor Author

tyralla commented Jan 15, 2023

Seems, Mypy primer tells me I forgot to handle other kinds of not iterable types like None and Literal.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

@tyralla
Copy link
Contributor Author

tyralla commented Apr 20, 2023

@ilevkivskyi: Would you like to review this one? I am asking you because it changes code you worked on. (And because this pull request grows old. So far, resolving conflicts should be simple.)

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