fix error that tuple with star expr #8827
Conversation
LiuYuHui
force-pushed
the
TupleWithStarExpr
branch
2 times, most recently
from
0d44146 to
bc8a3ad
Compare
mypy/checker.py
Outdated
| if isinstance(typs, TupleType): | ||
| rvalues.extend([TempNode(typ) for typ in typs.items]) | ||
| else: | ||
| rvalues.append(TempNode(typs)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
| [case testTupleWithStarExpr] | ||
| from typing import Tuple | ||
| points = (1, 2) # type: Tuple[int, int] | ||
| x, y, z = *points, 0 |
There was a problem hiding this comment.
Also reveal the types of x, y, and z to check that we inferred the correct types.
test-data/unit/check-tuples.test
Outdated
|
|
||
| [case testTupleWithStarExpr] | ||
| from typing import Tuple | ||
| points = (1, 2) # type: Tuple[int, int] |
There was a problem hiding this comment.
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.
LiuYuHui
force-pushed
the
TupleWithStarExpr
branch
2 times, most recently
from
295c8e3 to
276705f
Compare
mypy/checker.py
Outdated
| 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) |
There was a problem hiding this comment.
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
*argsrvalues 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
mypy/checker.py
Outdated
| 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) |
There was a problem hiding this comment.
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.
mypy/checker.py
Outdated
| 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)] |
There was a problem hiding this comment.
I think that the indices can become out of date as this modifies the length of rvalues?
| 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' |
There was a problem hiding this comment.
Add test case for two *expr that are both variable-length.
LiuYuHui
force-pushed
the
TupleWithStarExpr
branch
3 times, most recently
from
08fb8d1 to
266e6da
Compare
LiuYuHui
force-pushed
the
TupleWithStarExpr
branch
from
266e6da to
f884bc8
Compare
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)
fixes #7779