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

Proposal: don't simplify unions in expand_type() #14178

Merged
merged 3 commits into from
Nov 24, 2022

Conversation

ilevkivskyi
Copy link
Member

Fixes #6730

Currently expand_type() is inherently recursive, going through expand_type -> make_simplified_union -> is_proper_subtype -> map_instance_to_supertype -> expand_type. TBH I never liked this, so I propose that we don't do this. One one hand, this is a significant change in semantics, but on the other hand:

  • This fixes a crash (actually a whole class of crashes) that can happen even without recursive aliases
  • This removes an ugly import and simplifies an import cycle in mypy code
  • This makes mypy 2% faster (measured on self-check)

To make transition smoother, I propose to make trivial simplifications, like removing <nothing> (and None without strict optional), removing everything else if there is an object type, and remove strict duplicates. Notably, with these few things all existing tests pass (and even without it, only half a dozen tests fail on reveal_type()).

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

I like this! I think that we've been simplifying unions too aggressively. Not simplifying unions "should not" affect semantics, except for the details of revealed types (and bugs related to union types).

Accepting on the assumption that mypy_primer doesn't show meaningful regressions.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

The one in paasta looks like a bug. Probably we hash TypedDicts incorrectly.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

operator (https://github.com/canonical/operator)
- ops/testing.py:906: error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "Mapping[str, str]"  [arg-type]
+ ops/testing.py:906: error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "Union[Dict[str, str], Mapping[str, str]]"  [arg-type]

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/utilities/asyncutils.py:205: error: Argument 1 to "is_async_fn" has incompatible type "T"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], Awaitable[<nothing>]]"  [arg-type]
+ src/prefect/utilities/asyncutils.py:205: error: Argument 1 to "is_async_fn" has incompatible type "T"; expected "Union[Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>], Callable[[VarArg(<nothing>), KwArg(<nothing>)], Awaitable[<nothing>]]]"  [arg-type]

discord.py (https://github.com/Rapptz/discord.py)
- discord/automod.py:284: error: Argument "data" to "from_data" of "AutoModTrigger" has incompatible type "object"; expected "Optional[Empty]"  [arg-type]
+ discord/automod.py:284: error: Argument "data" to "from_data" of "AutoModTrigger" has incompatible type "object"; expected "Union[_AutoModerationTriggerMetadataKeyword, _AutoModerationTriggerMetadataKeywordPreset, _AutoModerationTriggerMetadataMentionLimit, Empty, None]"  [arg-type]

@ilevkivskyi
Copy link
Member Author

OK, now mypy_primer looks OK. I will merge this later today if there are no objections.

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.

segfault on recursive type
2 participants