Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Use (simplified) unions instead of joins for tuple fallbacks#17408
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
Collaborator
JukkaL commentedJun 20, 2024
Overall this looks good. I didn't check all the mypy primer hits, but nothing looks particularly concerning there. |
MemberAuthor
ilevkivskyi commentedJun 22, 2024
OK, it looks like all the new errors are because |
This comment has been minimized.
This comment has been minimized.
test-data/unit/check-unions.test Outdated
| x: Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, C11] | ||
| y: Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, C11, None] | ||
| x = y # E: Incompatible types in assignment (expression has type "Union[C1, C2, C3, C4, C5, <6 more items>, None]", variable has type "Union[C1, C2, C3, C4, C5, <6 more items>]") |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure I like this idea. I think I'd prefer to see the exact type mypy has inferred in an error message, even if it means the message is very verbose. I'd much prefer to have all the information I might need to debug the issue.
MemberAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For debugging you can still use somereveal_type()s. In any case, we already do this in similar scenarios for long overloads, long tuples, and large protocols (and maybe some other situations I am not aware of), so this makes it more consistent.
Actually now that I am writing this, one thing that may be worth considering is some kind of stable sort for union items and/or flattening nesting unions (we already flatten most unions, but not those that are targets of type aliases). I will play with this locally now.
MemberAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, actually it looks worse when sorted. It seems more natural to see the same order as in the definitions. I would propose to just move on. I will be happy to revert this, if people will actually start complaining.
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I agree having them in the order given in the source code is definitely better.
I can remember times when omitting some overloads in the error message given to the user has made some issues much harder for me to debug over at typeshed, so I'm still not sure about abbreviating the union here. But I can see that it's consistent with what mypy does in other areas, so I won't die on this hill :-)
MemberAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One last thing that I just tried locally and seems good is to add a specific note for long unions about which subtype item(s) cause problem.
This comment has been minimized.
This comment has been minimized.
test-data/unit/check-unions.test Outdated
| y: Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, str] | ||
| x = y # E: Incompatible types in assignment (expression has type "Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, str]", variable has type "Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, int]") | ||
| x = y # E: Incompatible types in assignment (expression has type "Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, str]", variable has type "Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, int]") \ | ||
| # N: Subtype items that may cause the mismatch: "str" |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is great, and allays my concern. Thanks! Though ideally I think it would be best if we could say something like "items in the first union not in the second" rather than "subtype items", which might not be familiar terminology to all of our users.
MemberAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I was thinking about this, but then I thought that it is not technically correct, e.g.Sub| int is still a subtype ofSuper | int, but now it seems to me I am overthinking it. I will change the message.
This comment has been minimized.
This comment has been minimized.
AlexWaygood approved these changesJun 22, 2024
Member
AlexWaygood left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Contributor
Diff frommypy_primer, showing the effect of this PR on open source code: dragonchain (https://github.com/dragonchain/dragonchain)- dragonchain/lib/database/redis_utest.py:102:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[None, float, timedelta], Union[None, float, timedelta], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]+ dragonchain/lib/database/redis_utest.py:102:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[float, timedelta, None], Union[float, timedelta, None], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]- dragonchain/lib/database/redis_utest.py:106:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[None, float, timedelta], Union[None, float, timedelta], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]+ dragonchain/lib/database/redis_utest.py:106:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[float, timedelta, None], Union[float, timedelta, None], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]- dragonchain/lib/database/redis_utest.py:162:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[None, float, timedelta], Union[None, float, timedelta], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]+ dragonchain/lib/database/redis_utest.py:162:9: error: "Callable[[Union[str, bytes], Union[bytes, float, int, str], Union[float, timedelta, None], Union[float, timedelta, None], bool, bool, bool, bool, Optional[Any], Optional[Any]], Optional[bool]]" has no attribute "assert_called_once_with" [attr-defined]koda-validate (https://github.com/keithasaurus/koda-validate)+ koda_validate/dictionary.py:894: error: Unused "type: ignore" comment [unused-ignore]+ koda_validate/dictionary.py:895: error: Unused "type: ignore" comment [unused-ignore]ppb-vector (https://github.com/ppb/ppb-vector)- tests/benchmark.py:16: error: Unused "type: ignore" comment [unused-ignore]- tests/benchmark.py:19: error: Unused "type: ignore" comment [unused-ignore]mkosi (https://github.com/systemd/mkosi)+ mkosi/config.py:3566:17: error: Incompatible types in assignment (expression has type "Union[Any, str]", variable has type "Path") [assignment]pandera (https://github.com/pandera-dev/pandera)+ tests/strategies/test_strategies.py:924: error: Unused "type: ignore" comment [unused-ignore]schemathesis (https://github.com/schemathesis/schemathesis)+ src/schemathesis/runner/__init__.py: note: In function "load_schema":+ src/schemathesis/runner/__init__.py:314: error: Argument 1 to "get_requests_auth" has incompatible type "Union[str, Any, List[str], Tuple[str], Set[str], NotSet, Tuple[DataGenerationMethod, ...]]"; expected "Optional[Tuple[str, str]]" [arg-type]+ src/schemathesis/runner/__init__.py:314: error: Argument 2 to "get_requests_auth" has incompatible type "Union[str, Any, List[str], Tuple[str], Set[str], NotSet, Tuple[DataGenerationMethod, ...]]"; expected "Optional[str]" [arg-type]pyinstrument (https://github.com/joerick/pyinstrument)- pyinstrument/renderers/base.py:99: error: Argument 1 to "remove" of "list" has incompatible type "function"; expected "Callable[..., Frame | None]" [arg-type]discord.py (https://github.com/Rapptz/discord.py)- discord/automod.py:521: error: Incompatible types in assignment (expression has type "int | int", target has type "list[dict[str, Any]]") [assignment]+ discord/automod.py:521: error: Incompatible types in assignment (expression has type "int", target has type "list[dict[str, Any]]") [assignment]- discord/scheduled_event.py:491: error: Incompatible types in assignment (expression has type "int | int | int | int", target has type "str") [assignment]+ discord/scheduled_event.py:491: error: Incompatible types in assignment (expression has type "int", target has type "str") [assignment]- discord/scheduled_event.py:510: error: Incompatible types in assignment (expression has type "int | int | int", target has type "str") [assignment]+ discord/scheduled_event.py:510: error: Incompatible types in assignment (expression has type "int", target has type "str") [assignment]- discord/guild.py:3196: error: Incompatible types in assignment (expression has type "int | int | int", target has type "str") [assignment]+ discord/guild.py:3196: error: Incompatible types in assignment (expression has type "int", target has type "str") [assignment]- discord/app_commands/transformers.py:139: error: Incompatible types in assignment (expression has type "list[int | int | int | int | int | int | int | int | int | int | int | int]", target has type "str | int") [assignment]+ discord/app_commands/transformers.py:139: error: Incompatible types in assignment (expression has type "list[int]", target has type "str | int") [assignment]- discord/ui/select.py:417: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread | Role | Member | Role | User]+ discord/ui/select.py:417: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread]- discord/ui/select.py:577: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread | Role | Member | Role | User]+ discord/ui/select.py:577: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread]- discord/ui/select.py:669: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread | Role | Member | Role | User]+ discord/ui/select.py:669: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread]- discord/ui/select.py:757: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread | Role | Member | Role | User]+ discord/ui/select.py:757: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread]- discord/ui/select.py:870: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread | Role | Member | Role | User]+ discord/ui/select.py:870: note: list[str | User | Member | Role | AppCommandChannel | AppCommandThread]- discord/ext/tasks/__init__.py:513: error: Incompatible types in assignment (expression has type "tuple[type, ...]", variable has type "tuple[type[OSError], type[GatewayNotFound], type[ConnectionClosed], type[ClientError], type[TimeoutError]]") [assignment]+ discord/ext/tasks/__init__.py:513: error: Incompatible types in assignment (expression has type "tuple[type[OSError] | type[GatewayNotFound] | type[ConnectionClosed] | type[ClientError] | type[TimeoutError], ...]", variable has type "tuple[type[OSError], type[GatewayNotFound], type[ConnectionClosed], type[ClientError], type[TimeoutError]]") [assignment]ibis (https://github.com/ibis-project/ibis)- ibis/expr/datatypes/value.py:131: error: Unsupported operand types for + ("tuple[UnsignedInteger, ...]" and "tuple[Int8, Int16, Int32, Int64]") [operator]+ ibis/expr/datatypes/value.py:131: error: Unsupported operand types for + ("tuple[UInt8 | UInt16 | UInt32 | UInt64, ...]" and "tuple[Int8, Int16, Int32, Int64]") [operator]- ibis/backends/bigquery/__init__.py:221: error: Argument "database" to "drop_table" of "Backend" has incompatible type "tuple[Any, Any]"; expected "tuple[str | str] | str | None" [arg-type]+ ibis/backends/bigquery/__init__.py:221: error: Argument "database" to "drop_table" of "Backend" has incompatible type "tuple[Any, Any]"; expected "tuple[str] | str | None" [arg-type]- ibis/backends/bigquery/__init__.py:1075: note: def drop_table(self, name: str, *, schema: str | None = ..., database: tuple[str | str] | str | None = ..., force: bool = ...) -> None+ ibis/backends/bigquery/__init__.py:1075: note: def drop_table(self, name: str, *, schema: str | None = ..., database: tuple[str] | str | None = ..., force: bool = ...) -> Nonepwndbg (https://github.com/pwndbg/pwndbg)+ pwndbg/gdblib/regs.py:200: error: Unused "type: ignore" comment [unused-ignore]werkzeug (https://github.com/pallets/werkzeug)+ src/werkzeug/routing/rules.py:104: error: Unused "type: ignore" comment [unused-ignore]- tests/test_wrappers.py:371: error: Argument 1 to "force_type" of "Response" has incompatible type "object"; expected "Response" [arg-type]+ tests/test_wrappers.py:371: error: Argument 1 to "force_type" of "Response" has incompatible type "Callable[[Any, Any], Any] | Response"; expected "Response" [arg-type]streamlit (https://github.com/streamlit/streamlit)- lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:13: error: Value of type "Union[ColumnConfig, None, str]" is not indexable [index]+ lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:13: error: Value of type "Union[ColumnConfig, str, None]" is not indexable [index]- lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:30: error: Invalid index type "str" for "Union[ColumnConfig, None, str]"; expected type "Union[SupportsIndex, slice]" [index]+ lib/tests/streamlit/elements/lib/column_config_utils_test.py:382:30: error: Invalid index type "str" for "Union[ColumnConfig, str, None]"; expected type "Union[SupportsIndex, slice]" [index]- lib/tests/streamlit/elements/arrow_dataframe_test.py:229:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, None, str]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Literal['ignore'], selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DeltaGenerator+ lib/tests/streamlit/elements/arrow_dataframe_test.py:229:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, str, None]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Literal['ignore'], selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DeltaGenerator- lib/tests/streamlit/elements/arrow_dataframe_test.py:229:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, None, str]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Union[Literal['rerun'], Callable[..., None]] = ..., selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DataframeState+ lib/tests/streamlit/elements/arrow_dataframe_test.py:229:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, str, None]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Union[Literal['rerun'], Callable[..., None]] = ..., selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DataframeState- lib/tests/streamlit/elements/arrow_dataframe_test.py:322:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, None, str]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Literal['ignore'], selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DeltaGenerator+ lib/tests/streamlit/elements/arrow_dataframe_test.py:322:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, str, None]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Literal['ignore'], selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DeltaGenerator- lib/tests/streamlit/elements/arrow_dataframe_test.py:322:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, None, str]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Union[Literal['rerun'], Callable[..., None]] = ..., selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DataframeState+ lib/tests/streamlit/elements/arrow_dataframe_test.py:322:13: note: def dataframe(self, data: Union[Any, Any, Any, Any, Any, Any, Iterable[Any], Dict[str, List[Any]], None] = ..., width: Optional[int] = ..., height: Optional[int] = ..., *, use_container_width: bool = ..., hide_index: Optional[bool] = ..., column_order: Optional[Iterable[str]] = ..., column_config: Optional[Mapping[Union[Literal['_index'], str], Union[ColumnConfig, str, None]]] = ..., key: Optional[Union[str, int]] = ..., on_select: Union[Literal['rerun'], Callable[..., None]] = ..., selection_mode: Union[Literal['single-row', 'multi-row', 'single-column', 'multi-column'], Iterable[Literal['single-row', 'multi-row', 'single-column', 'multi-column']]] = ...) -> DataframeStatepoetry (https://github.com/python-poetry/poetry)+ src/poetry/utils/env/site_packages.py:195: error: Value of type "Path | Any | tuple[Path, Any]" is not indexable [index]+ src/poetry/utils/env/site_packages.py:205: error: Value of type "Path | Any | tuple[Path, Any]" is not indexable [index]+ src/poetry/utils/env/site_packages.py:209: error: Value of type "Path | Any | tuple[Path, Any]" is not indexable [index] |
Collaborator
cdce8p commentedJun 23, 2024
This PR seem to cause new false positives, especially with narrowing after Example 1fromtypingimportcast,Anyd1:dict[str,int]d2:dict[str,str]fordatain (d1,d2):reveal_type(data)data=cast(dict[str,Any],data)reveal_type(data)# should be dict[str, Any] note:Revealedtypeis"Union[builtins.dict[builtins.str, builtins.int], builtins.dict[builtins.str, builtins.str]]"note:Revealedtypeis"Union[builtins.dict[builtins.str, builtins.int], builtins.dict[builtins.str, builtins.str]]" Example 2# mypy: disallow_untyped_callsfromtypingimportCallabledeffunc()->str|None: ...var:str|Nonefactory:Callable[[],str|None]forfactoryin (lambda:var,func,):reveal_type(factory)var=factory()# false-positive note:Revealedtypeis"def () -> Union[builtins.str, None]"error:Calltountypedfunction<lambda>intypedcontext |
MemberAuthor
ilevkivskyi commentedJun 23, 2024
The first is just how type narrowing works with x:intx=cast(Any,"whatever")reveal_type(x)# still int whether you like it or not. If you want something like "from now on this variable has this type", you will need to create a new variable. There were long discussions about enabling variable re-definitions in mypy (and more general SSA-style logic), but there are too many edge cases, so it is not clear when it will happen. The second is a bug, although unrelated. We were a bit worried that it may expose some bugs for callables, so I will probably fix this one (unless it is non-trivial). |
MemberAuthor
ilevkivskyi commentedJun 23, 2024
Possible fix in#17430 |
This was referencedJul 3, 2024
This was referencedDec 8, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref#12056
If
mypy_primerwill look good, I will add some logic to shorted unions in error messages.cc@JukkaL