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

Use (simplified) unions instead of joins for tuple fallbacks #17408

Merged
merged 7 commits into from
Jun 22, 2024

Conversation

ilevkivskyi
Copy link
Member

Ref #12056

If mypy_primer will look good, I will add some logic to shorted unions in error messages.

cc @JukkaL

This comment has been minimized.

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 20, 2024

Overall this looks good. I didn't check all the mypy primer hits, but nothing looks particularly concerning there.

@ilevkivskyi
Copy link
Member Author

OK, it looks like all the new errors are because join(Any, X) = Any, but make_simplified_union(Any, X) = Any | X. I think this is correct.

This comment has been minimized.


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>]")
Copy link
Member

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.

Copy link
Member Author

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 some reveal_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.

Copy link
Member Author

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.

Copy link
Member

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 :-)

Copy link
Member Author

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.

@@ -1324,7 +1324,8 @@ class C10: ...

x: Union[C1, C2, C3, C4, C5, C6, C7, C8, C9, C10, int]
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"
Copy link
Member

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.

Copy link
Member Author

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 of Super | int, but now it seems to me I am overthinking it. I will change the message.

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

test-data/unit/check-newsemanal.test Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Contributor

Diff from mypy_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 = ...) -> None

pwndbg (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']]] = ...) -> DataframeState

poetry (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]

@ilevkivskyi ilevkivskyi merged commit abdaf6a into python:master Jun 22, 2024
18 checks passed
@ilevkivskyi ilevkivskyi deleted the union-fallback branch June 22, 2024 22:19
@cdce8p
Copy link
Collaborator

cdce8p commented Jun 23, 2024

This PR seem to cause new false positives, especially with narrowing after for ... in ....

Example 1

from typing import cast, Any

d1: dict[str, int]
d2: dict[str, str]

for data in (d1, d2):
    reveal_type(data)
    data = cast(dict[str, Any], data)
    reveal_type(data)  # should be dict[str, Any]
note: Revealed type is "Union[builtins.dict[builtins.str, builtins.int], builtins.dict[builtins.str, builtins.str]]"
note: Revealed type is "Union[builtins.dict[builtins.str, builtins.int], builtins.dict[builtins.str, builtins.str]]"

Example 2

# mypy: disallow_untyped_calls
from typing import Callable

def func() -> str | None: ...
var: str | None

factory: Callable[[], str | None]
for factory in (
    lambda: var,
    func,
):
    reveal_type(factory)
    var = factory()  # false-positive
note: Revealed type is "def () -> Union[builtins.str, None]"
error: Call to untyped function <lambda> in typed context

@ilevkivskyi
Copy link
Member Author

The first is just how type narrowing works with Any

x: int
x = 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).

@ilevkivskyi
Copy link
Member Author

Possible fix in #17430

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.

4 participants