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

Support narrowing literals and enums using the in operator in combination with list, set, and tuple expressions. #17044

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

tyralla
Copy link
Collaborator

@tyralla tyralla commented Mar 17, 2024

Note: This PR initially dealt only with tuple expressions.


The general idea is to transform expressions like

(x is None) and (x in (1, 2)) and (x not in (3, 4))

into

(x is None) and (x == 1 or x == 2) and (x != 3 and x != 4)

This transformation circumvents the need to extend the (already complicated) narrowing logic further.

See the test testNarrowTypeAfterInTuple, which shows a slight change in the narrowing behaviour. I don't know if this is an improvement, but at least it seems to increase consistency.

The only downside of the transformation approach I am aware of is that it might cause some hard-too-understand error messages. The only such problem I encountered is the Right operand of "and" is never evaluated warning in the testNarrowLiteralsNotInTupleExpression test.

tyralla and others added 2 commits March 17, 2024 18:06
…tion with tuple expressions.

The general idea is to transform expressions like

(x is None) and (x in (1, 2)) and (x not in (3, 4))

into

(x is None) and (x == 1 or x == 2) and (x != 3 and x != 4)

This transformation circumvents the need to extend the (already complicated) narrowing logic further.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@davidhalter
Copy link

davidhalter commented Mar 17, 2024

I generally like what you are trying to achieve.

But it feels a bit weird that this is the way you approach this. I have recently done a lot of research into the Mypy narrowing code and it feels like the most convoluted and complicated part of the whole Mypy code base. (Other parts like the recently added --new-type-inference are way more approachable even though they are more complex). I therefore understand that you wanted to special case this.

However, in this case it feels like the code would be more approachable if it was actually implemented in the in operator narrowing part of the code. It just feels super weird that there would be two places that narrow None in different ways.

Also you handled TupleExpr, which would imply a different behavior for TupleType, but that's fixable, I guess.

But then again I'm not a Mypy dev, so they might feel different about this PR.

@tyralla
Copy link
Collaborator Author

tyralla commented Mar 18, 2024

@davidhalter Thanks for your opinion on this, which I understand. I started with extending the mentioned in operator section and targeting TupleType but felt that a separate handling for TupleExpr would be simpler and maybe more correct.

Regarding the different ways to narrow None, I think this problem already exists. This is how the testNarrowTypeAfterInTuple test case looks like without my changes:

from typing import Optional
class A: pass
class B(A): pass
class C(A): pass

y: Optional[B]
if y in (B(), C()):
    reveal_type(y) # N: Revealed type is "__main__.B"
else:
    reveal_type(y) # N: Revealed type is "Union[__main__.B, None]"

But if one uses == instead of in, the result (again, without my changes) is:

if y == B() or y == C():
    reveal_type(y) # N: Revealed type is "Union[__main__.B, None]"
else:
    reveal_type(y) # N: Revealed type is "Union[__main__.B, None]"

So, for this aspect, I would say the proposed change increases consistency (always the second result).

I might have been too cautious about the TupleExpr vs. TupleType issue. I wanted to avoid handling code where users apply tuple subclasses with overwritten __contains__ methods. However, I now checked it and realised that Mypy models such subclasses with Instance and not TupleType. So, generally, targeting TupleType seems the right choice (although I expect 99 % of all potential use cases to work with tuples created on the fly so that targeting TupleExpr only could be satisfying enough).

Two Mypy primer responses are satisfying (discord), and one needs investigation (mkosi). However, for Mypyc, there seems to be a relevant problem. (I have no Mypyc experience, but my first impression is that the proposed transformation confuses Mypy. Maybe the left operand is only available for the first comparison with the items of the right operand?)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@tyralla
Copy link
Collaborator Author

tyralla commented Mar 21, 2024

The mkosi and mypyc problems are fixed. I also included a small documentation update in case this pull request gets accepted. It's clearly not the most general solution, but it should handle the most frequent use case (at least the one often relevant to me...), so I will wait for a review.

This comment has been minimized.

@tyralla tyralla changed the title Support narrowing literals and enums using the in operator in combination with tuple expressions. Support narrowing literals and enums using the in operator in combination with list and tuple expressions. Oct 26, 2024
@tyralla
Copy link
Collaborator Author

tyralla commented Oct 26, 2024

@JukkaL @Jordandev678

I expected this PR to be superseded by the more general PR #17344 and wanted to close it. However, when trying out the new feature in Mypy 3.13, I realised that #17865 reverted #17344. So, I reconsidered my approach and still find it not too bad for a review.

If the "transformation approach" is unacceptable, could we combine the ideas of this PR and #17344?

Also, I widened the scope of this PR by supporting list expressions. Using list expressions for this kind of narrowing is equally fine as using tuple expressions, and by supporting it, I may have an excuse for not supporting tuple types...

I also considered supporting set expressions but was unsure because of a missing literal for empty sets. On the other hand, who would write if x in set()?

@tyralla
Copy link
Collaborator Author

tyralla commented Oct 26, 2024

The new Mypy primer finding indicates a small flaw in the xarray library. Small repro:

from typing import Literal

PadModeOptions = Literal[
    "constant",
    "edge",
    "linear_ramp",
    "maximum",
    "mean",
    "median",
    "minimum",
    "reflect",
    "symmetric",
    "wrap",
]

def pad(mode: PadModeOptions = "constant") -> None:
    # if mode in ("edge", "reflect", "symmetric", "wrap"):  # actual code, Mypy only reports an error with this PR included
    if mode == "edge" or mode == "reflect" or mode == "symmetric" or mode == "wrap"  # for this similar code, Mypy would also report an error without this PR
        coord_pad_mode = mode
    else:
        coord_pad_mode = "constant"

@tyralla tyralla changed the title Support narrowing literals and enums using the in operator in combination with list and tuple expressions. Support narrowing literals and enums using the in operator in combination with list, set, and tuple expressions. Oct 27, 2024
@tyralla
Copy link
Collaborator Author

tyralla commented Oct 27, 2024

I also considered supporting set expressions but was unsure because of a missing literal for empty sets. On the other hand, who would write if x in set()?

After rethinking, there is really no good reason not to support set expressions, so I extended the PR's title again...

Copy link
Contributor

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

pydantic (https://github.com/pydantic/pydantic)
- pydantic/v1/types.py:704: error: Unsupported operand types for >= ("Literal['n']" and "int")  [operator]
- pydantic/v1/types.py:704: error: Unsupported operand types for >= ("Literal['N']" and "int")  [operator]
- pydantic/v1/types.py:704: error: Unsupported operand types for >= ("Literal['F']" and "int")  [operator]
- pydantic/v1/types.py:704: note: Left operand is of type "Literal['n', 'N', 'F'] | int"
- pydantic/v1/types.py:706: error: Unsupported operand types for + ("int" and "Literal['n']")  [operator]
- pydantic/v1/types.py:706: error: Unsupported operand types for + ("int" and "Literal['N']")  [operator]
- pydantic/v1/types.py:706: error: Unsupported operand types for + ("int" and "Literal['F']")  [operator]
- pydantic/v1/types.py:706: note: Right operand is of type "Literal['n', 'N', 'F'] | int"
- pydantic/v1/types.py:714: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]
- pydantic/v1/types.py:715: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]
- pydantic/v1/types.py:718: error: Argument 1 to "abs" has incompatible type "Literal['n', 'N', 'F'] | int"; expected "SupportsAbs[int]"  [arg-type]

altair (https://github.com/vega/altair)
+ altair/utils/mimebundle.py:136: error: Redundant cast to "Literal['png', 'svg', 'pdf', 'vega']"  [redundant-cast]
+ altair/utils/save.py:189: error: Not all union combinations were tried because there are too many unions  [misc]
+ altair/utils/save.py:191: error: Argument "format" to "spec_to_mimebundle" has incompatible type "Literal['svg', 'pdf'] | None"; expected "Literal['pdf', 'svg', 'vega']"  [arg-type]

xarray (https://github.com/pydata/xarray)
+ xarray/core/dataset.py: note: In member "pad" of class "Dataset":
+ xarray/core/dataset.py:9339: error: Incompatible types in assignment (expression has type "Literal['constant']", variable has type "Literal['edge', 'reflect', 'symmetric', 'wrap']")  [assignment]
+ xarray/core/dataset.py: note: At top level:

discord.py (https://github.com/Rapptz/discord.py)
- discord/app_commands/tree.py:447: error: Missing return statement  [return]
- discord/app_commands/tree.py:567: error: Missing return statement  [return]

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.

2 participants