fix error that tuple with star expr by LiuYuHui · Pull Request #8827 · python/mypy

LiuYuHui · 2020-05-15T00:44:15Z

JukkaL

Thanks for the PR! Looks pretty good, left some comments about how to improve this further.

JukkaL · 2020-05-19T13:24:32Z

+                    if isinstance(typs, TupleType):
+                        rvalues.extend([TempNode(typ) for typ in typs.items])
+                    else:
+                        rvalues.append(TempNode(typs))


If the type is an iterable, for example, the correct thing to do here would be to expand the right number of iterable item types, depending on the number of lvalues. It's okay if you don't want to implement that in this PR, however. The alternative would be to give an error about not being able to check the number of rvalues. The current behavior propagates an incorrect type, which I'd prefer to avoid.

thanks for your review, i would like to implement the correct thing, so the basic idea is that just get the right number of iterable item types, instead of constructing a new TempNode type, right?

The more correct thing would be figure out the number of rvalues needed here so that the number of rvalues matches the number of lvalues. Then assuming the rvalue type is an iterable, look at the iterable item type and append a suitable number of TempNode instances with the iterable item type. Here we can assume that the length of the iterable is correct, as we don't know how many items there will actually be.

There will be some interesting edge cases, I assume. If the type is not iterable and not a tuple, you can probably fall back to AnyType (make sure that an error gets reported either here or elsewhere if the rvalue type is bogus).

JukkaL · 2020-05-19T13:27:47Z

+[case testTupleWithStarExpr]
+from typing import Tuple
+points = (1, 2)  # type: Tuple[int, int]
+x, y, z = *points, 0


Also reveal the types of x, y, and z to check that we inferred the correct types.

JukkaL · 2020-05-19T13:29:04Z

+
+[case testTupleWithStarExpr]
+from typing import Tuple
+points = (1, 2)  # type: Tuple[int, int]


Instead of using all int types, use different types so that each of x, y and z will get distinct types. This way we can test that the types are propagated in the correct order.

msullivan

We can merge this if Jukka's suggestions are implemented

JukkaL

Thanks for the updates! There are still a few edge cases that aren't quite right, but this is very close.

JukkaL · 2020-06-08T13:00:17Z

+            num_last_iterable = 0
+            if len(idx_of_iterable):
+                num_every_iterable = int(lhs_len / len(idx_of_iterable))
+                num_last_iterable = lhs_len - (len(idx_of_iterable) - 1) * int(num_every_iterable)


This seems kind of arbitrary. We assume that the items from multiple *args rvalues are split between the values in a specific way, but we can't really predict this. Instead, I'd suggest this:

If all the item types of iterable *args rvalues are the same and next to each other, treat them as a single iterable.

If the item types vary or they aren't next to each other generate an error and fall back to some conservative approximation, such as assuming all their types are Any.

I think that the current behavior can be unsafe. Consider this:

a = [1] b = ['x', 'y', 'z'] x1, x2, x3, x4 = *a, *b x2 + 5 # Runtime error

JukkaL · 2020-06-08T13:03:01Z

+                        item_type_of_iterable.append(self.iterable_item_type(typs))
+                        idx_of_iterable.append(idx)
+                    else:
+                        self.fail("StarExpr should not be a '{}'".format(typs), context)


Nits: StarExpr is an internal name that we'd rather not use in an error message. It would be better to use something more descriptive, such as Invalid type 'x' for *expr (iterable expected)

If you pass rval as the context to fail, the error message should point to the relevant subexpression, making it easier to figure out what it going on.

It would be good to have a test case for this as well.

JukkaL · 2020-06-08T13:05:30Z

+                if i == (len(idx_of_iterable) - 1):
+                    rvalues[idx:idx] = [TempNode(item_type) for _ in range(num_last_iterable)]
+                else:
+                    rvalues[idx:idx] = [TempNode(item_type) for _ in range(num_every_iterable)]


I think that the indices can become out of date as this modifies the length of rvalues?

JukkaL · 2020-06-08T13:06:06Z

+reveal_type(x4) # N: Revealed type is 'builtins.str'
+reveal_type(y3) # N: Revealed type is 'builtins.int*'
+reveal_type(y4) # N: Revealed type is 'builtins.int*'
+reveal_type(z3) # N: Revealed type is 'builtins.str'


Add test case for two *expr that are both variable-length.

JukkaL

Thanks for the updates and your persistence! Looks great now.

In general, mypy doesn't promise to be able to check the remainder of your code in the presence of syntax errors, so just make this a blocking error. Fixes python#9137 Fixes python#3825 (most of the reports in this issue were fixed by python#8827)

In general, mypy doesn't promise to be able to check the remainder of your code in the presence of syntax errors, so just make this a blocking error. Fixes #9137 Fixes #3825 (most of the reports in this issue were fixed by #8827) Co-authored-by: hauntsaninja <>

LiuYuHui force-pushed the TupleWithStarExpr branch 2 times, most recently from 0d44146 to bc8a3ad Compare May 15, 2020 02:46

JukkaL reviewed May 19, 2020

View reviewed changes

msullivan requested changes May 20, 2020

View reviewed changes

LiuYuHui force-pushed the TupleWithStarExpr branch 2 times, most recently from 295c8e3 to 276705f Compare May 26, 2020 05:13

LiuYuHui requested a review from JukkaL May 29, 2020 08:26

JukkaL reviewed Jun 8, 2020

View reviewed changes

LiuYuHui added 2 commits June 26, 2020 21:49

fix error that tuple with star expr

7d27f5c

add support for iterable

fcd5855

LiuYuHui force-pushed the TupleWithStarExpr branch 3 times, most recently from 08fb8d1 to 266e6da Compare June 28, 2020 01:25

fix some potential errors and add more unit test

f884bc8

LiuYuHui force-pushed the TupleWithStarExpr branch from 266e6da to f884bc8 Compare June 28, 2020 03:16

LiuYuHui requested a review from JukkaL June 28, 2020 11:17

JukkaL approved these changes Jul 3, 2020

View reviewed changes

JukkaL merged commit 17b1f9c into python:master Jul 3, 2020

hauntsaninja mentioned this pull request Oct 27, 2020

Internal error on a simple destructuring assignment #8938

Closed

hauntsaninja mentioned this pull request Nov 8, 2021

Fix crashes with unpacking SyntaxError #11499

Merged

hauntsaninja mentioned this pull request Feb 13, 2022

Tuple assignment and list * unpacking crashes mypy #3825

Closed

Uh oh!

Conversation

LiuYuHui commented May 15, 2020

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msullivan left a comment

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants