FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve the support for promotions inside unions. by tyralla · Pull Request #19245 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Improve the support for promotions inside unions. - #19245

Merged
ilevkivskyi merged 7 commits into
python:masterfrom
tyralla:fix/fix_narrow_promotions_in_unions
Jul 7, 2025
ConversationCommits (7)ChecksFiles changed
Merged

Improve the support for promotions inside unions.#19245
ilevkivskyi merged 7 commits into
python:masterfrom
tyralla:fix/fix_narrow_promotions_in_unions

Conversation

tyralla commented Jun 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Fixes #14987

I was puzzled as to why my previous attempts to avoid false unreachable warnings for loops failed for issue #14987. After some debugging, I realised that the underlying problem is that type narrowing does not work with promotions if both the declared type and the constraining type are unions:

x: float | None
y: int | None
x = y
reveal_type(x)  #  None !!!

The fix seems straightforward (but let's see what the Mypy primer says) and is checked by the test cases testNarrowPromotionsInsideUnions1 and testNarrowPromotionsInsideUnions2.

This comment has been minimized.

Sign in to view

tyralla commented Jun 6, 2025

Copy link
Copy Markdown
Collaborator Author

The test suite currently fails due to a previous merge conflict that will be resolved by #19118.

The Mypy primer differences are only due to different union item orders.

@ilevkivskyi: Would you like to review this simple change? It is a small extension of one of your code sections.

tyralla requested a review from ilevkivskyi June 6, 2025 22:52

This comment has been minimized.

Sign in to view
sterliakov reviewed Jun 7, 2025
View reviewed changes
Comment thread mypy/meet.py Outdated
narrow_declared_type(x, narrowed)
for x in declared.relevant_items()
narrow_declared_type(d, n)
for d, n in product(

sterliakov Jun 7, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks like the correct way of doing that, despite higher comp complexity. What's more mypyc-friendly - itertools.product or a plain listcomp with two for clauses?

A5rocks Jun 10, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I feel like plain listcomp would be faster (we're consuming all the items anyways), but I would be surprised if this is a hot path because there's an is_subtype check -- surely that's somewhat slow!

tyralla Jun 10, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the feedback!

I have no experience with Mypyc, so I can only guess, and would agree with @A5rocks that the possible overhead of itertools.product (if it exists) should be negligible compared to the cost of the additional calls to narrow_declared_type, is_overlapping_types, and is_subtype.

JukkaL Jun 13, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

itertools.product may not be a significant bottleneck here, but I'd like to avoid it so that new code is less likely to follow the example and add usages in locations where it will be a bottleneck. Also it might slow things down measurably in very simple cases at least, e.g. union of 2 items. I'd prefer to write this using list comprehensions for these reasons.

tyralla Jun 13, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, I removed itertools.product. The second-best readable solution that came to mind. No list comprehensions; I hope that's fine.

A5rocks approved these changes Jun 10, 2025
View reviewed changes
Comment thread mypy/meet.py Outdated
narrow_declared_type(x, narrowed)
for x in declared.relevant_items()
narrow_declared_type(d, n)
for d, n in product(

A5rocks Jun 10, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I feel like plain listcomp would be faster (we're consuming all the items anyways), but I would be surprised if this is a hot path because there's an is_subtype check -- surely that's somewhat slow!

This comment has been minimized.

Sign in to view
tyralla requested a review from JukkaL June 13, 2025 17:25

This comment has been minimized.

Sign in to view
sterliakov mentioned this pull request Jun 27, 2025
Closed
ilevkivskyi approved these changes Jul 7, 2025
View reviewed changes

ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a bit ugly, but since promotions are generally ugly, I think this is fine.

ilevkivskyi commented Jul 7, 2025

Copy link
Copy Markdown
Member

@tyralla Hm, one test fails because of the order in union after I merged master. Could you please check if this is expected and update as needed?

This comment has been minimized.

Sign in to view

github-actions Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

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

pandas (https://github.com/pandas-dev/pandas)
- pandas/core/arrays/_mixins.py:150: error: Argument "dtype" to "view" of "ndarray" has incompatible type "ExtensionDtype | dtype[Any]"; expected "dtype[Any] | _HasDType[dtype[Any]]"  [arg-type]
+ pandas/core/arrays/_mixins.py:150: error: Argument "dtype" to "view" of "ndarray" has incompatible type "dtype[Any] | ExtensionDtype"; expected "dtype[Any] | _HasDType[dtype[Any]]"  [arg-type]

ibis (https://github.com/ibis-project/ibis)
- ibis/backends/mssql/__init__.py:691: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/mssql/__init__.py:691: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/mssql/__init__.py:738: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]"; expected "Schema"  [arg-type]
+ ibis/backends/mssql/__init__.py:691: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/mssql/__init__.py:691: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/mssql/__init__.py:738: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any"; expected "Schema"  [arg-type]
- ibis/backends/databricks/__init__.py:215: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | None" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/databricks/__init__.py:215: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | None" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/databricks/__init__.py:215: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any | None" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/databricks/__init__.py:215: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any | None" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/databricks/__init__.py:215: error: Item "None" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | None" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/databricks/__init__.py:215: error: Item "None" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any | None" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/risingwave/__init__.py:564: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/risingwave/__init__.py:564: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/risingwave/__init__.py:602: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]"; expected "Schema"  [arg-type]
+ ibis/backends/risingwave/__init__.py:564: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/risingwave/__init__.py:564: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/risingwave/__init__.py:602: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any"; expected "Schema"  [arg-type]
- ibis/backends/postgres/__init__.py:634: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/postgres/__init__.py:634: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/postgres/__init__.py:666: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]"; expected "Schema"  [arg-type]
+ ibis/backends/postgres/__init__.py:634: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/postgres/__init__.py:634: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/postgres/__init__.py:666: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any"; expected "Schema"  [arg-type]
- ibis/backends/mysql/__init__.py:411: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/mysql/__init__.py:411: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/mysql/__init__.py:438: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]"; expected "Schema"  [arg-type]
+ ibis/backends/mysql/__init__.py:411: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/mysql/__init__.py:411: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/mysql/__init__.py:438: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any"; expected "Schema"  [arg-type]
- ibis/backends/exasol/__init__.py:385: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/exasol/__init__.py:385: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/exasol/__init__.py:410: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]"; expected "Schema"  [arg-type]
+ ibis/backends/exasol/__init__.py:385: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/exasol/__init__.py:385: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/exasol/__init__.py:410: error: Argument "schema" to "DatabaseTable" has incompatible type "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any"; expected "Schema"  [arg-type]
- ibis/backends/duckdb/__init__.py:190: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "null_fields"  [union-attr]
- ibis/backends/duckdb/__init__.py:190: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "null_fields"  [union-attr]
- ibis/backends/duckdb/__init__.py:203: error: Item "Mapping[str, str | DataType]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
- ibis/backends/duckdb/__init__.py:203: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Any | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]]" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/duckdb/__init__.py:190: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "null_fields"  [union-attr]
+ ibis/backends/duckdb/__init__.py:190: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "null_fields"  [union-attr]
+ ibis/backends/duckdb/__init__.py:203: error: Item "Mapping[str, str | DataType]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]
+ ibis/backends/duckdb/__init__.py:203: error: Item "Iterable[tuple[str, str | DataType]]" of "Schema | Mapping[str, str | DataType] | Iterable[tuple[str, str | DataType]] | Any" has no attribute "to_sqlglot"  [union-attr]

ilevkivskyi merged commit cb3bddd into python:master Jul 7, 2025

tyralla commented Jul 7, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks for the review!

This is a bit ugly, but since promotions are generally ugly, I think this is fine.

I agree.

Hm, one test fails because of the order in union after I merged master. Could you please check if this is expected and update as needed?

This is most likely a result of #19324 or its predecessors.

tyralla commented Jul 7, 2025

Copy link
Copy Markdown
Collaborator Author

Oh, you were faster with merging than I was with writing my last comment...

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ilevkivskyi ilevkivskyi approved these changes
JukkaL Awaiting requested review from JukkaL
+2 more reviewers
sterliakov sterliakov left review comments
A5rocks A5rocks approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive unreachable when variable is set and reset inside loop

5 participants

Back | FazBrowse Home | New Git URL