Make str unpacking check opt-in #15511
Conversation
Fixes python#13823. See also python#6406
for more information, see https://pre-commit.ci
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
- tests/test_frame.py:2001: error: Unpacking a string is disallowed [misc]
- tests/test_frame.py:2007: error: Unpacking a string is disallowed [misc]
- tests/test_series.py:606: error: Unpacking a string is disallowed [misc]
- tests/test_series.py:612: error: Unpacking a string is disallowed [misc]
operator (https://github.com/canonical/operator)
- ops/framework.py:166: error: Unpacking a string is disallowed [misc]
aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client_reqrep.py:350: error: Unused "type: ignore" comment [unused-ignore]
+ aiohttp/web_log.py:206: error: Unused "type: ignore" comment [unused-ignore]
+ aiohttp/web_log.py:207: error: Unused "type: ignore[has-type]" comment [unused-ignore]
+ aiohttp/web_log.py:208: error: Unused "type: ignore[has-type]" comment [unused-ignore]
+ aiohttp/web_log.py:209: error: Unused "type: ignore[has-type]" comment [unused-ignore]
|
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
I agree that it makes sense to make this an opt-in check, since it can generate false positives. Left some minor comments, looks good overall.
| .. _code-str-unpacking: | ||
|
|
||
| Check that ``str`` is explicitly unpacked [str-unpacking] |
There was a problem hiding this comment.
The title is a bit unclear. What about something like "Check that 'str' is not unpacked?". Or "... is not unpacked implicitly", though I'm not sure what's the difference between explicit and implicit unpacking here.
Bikeshedding: what about renaming the error code to str-unpack, which would be a bit shorter?
| Check that ``str`` is explicitly unpacked [str-unpacking] | ||
| --------------------------------------------------------- | ||
|
|
||
| It can sometimes be surprising that ``str`` is iterable, especially when unpacking. |
There was a problem hiding this comment.
Explicitly enumerate the contexts where this matters (for loop, assignment)?
| ) | ||
| elif isinstance(rvalue_type, Instance) and rvalue_type.type.fullname == "builtins.str": | ||
| elif ( | ||
| self.msg.errors.is_error_code_enabled(codes.STR_UNPACKING) |
There was a problem hiding this comment.
Is this included in --strict, and should it be?
| elif ( | ||
| self.msg.errors.is_error_code_enabled(codes.STR_UNPACKING) | ||
| and isinstance(rvalue_type, Instance) | ||
| and rvalue_type.type.fullname == "builtins.str" |
There was a problem hiding this comment.
Will this check unpacking on Literal strings? Can you add a testcase for that please?
|
Any idea when this could be merged? |
|
This is superseded by #20325 , which also fixes type propagation The new PR keeps this enabled by default and addresses Jukka's feedback. Emma's feedback relates to preexisting behaviour in mypy. We do issue a warning there, but there is an unrelated improvement to be made to unpacking of Literals, which I will make in a separate PR |
The extra condition that excluded LiteralType was introduced in python#14827 I see no particular reason to have an instance check at all I was looking at this because of this comment from Emma python#15511 (comment) Previously we errored with `"Literal['xy']" object is not iterable` which is of course totally false Now I issue the same error as in the str case, but restrict to cases where the unpack length does not match
4 participants
Fixes #13823. See also #6406