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

PEP 604 typing duplication removal #982

Open
walter9388 opened this issue Oct 23, 2024 · 3 comments
Open

PEP 604 typing duplication removal #982

walter9388 opened this issue Oct 23, 2024 · 3 comments

Comments

@walter9388
Copy link

This is a bit niche, but when I was reviewing some naive code submitted to me I saw some bizarre typing generated by pyupgrade which looked something like

def f() -> int | None | None:
    ...

where None is duplicated.

When looking into how this had happened, I noticed it came from the following:

-def f() -> Optional[Union[int, None]]):
+def f() -> int | None | None:
     ...

Clearly wrapping Union[..., None] with Optional is bad, but it made me wonder which dev tool can detect this sort of issue. It doesn't seem to be something that is picked up by mypy, flake8, pyright, etc.
(Just to clarify this bad typing had been generated by an internal tool which wraps given types with Optional for PATCH methods in a RESTful API, so it is very much an edge case)

Do you think deduplication of types is something that can live in the PEP604 part of pyupgrade? I think it is unlikely that anyone would write int | None | None manually, but I can see when over complicating unions this sort of thing might happen.

I added the following test to test_fix_pep604_types just to check this was expected behaviour:

        pytest.param(
            'from typing import Optional, Union\n'
            'def f(x: Optional[Union[int, None]]): pass\n'
            'def g(x: Union[Union[int, None], None]): pass\n'
            'def h(x: Union[int, int, None]): pass\n'
            'def k(x: Union[Union[int, None], int]): pass\n',

            'from typing import Optional, Union\n'
            'def f(x: int | None): pass\n'
            'def g(x: int | None): pass\n'
            'def h(x: int | None): pass\n'
            'def k(x: int | None): pass\n',

            id='nested unions or optionals with duplicated typing',
        ),

and sure enough pytest failed with the following duplicated types:

    from typing import Optional, Union
  - def f(x: int | None): pass
  + def f(x: int | None | None): pass
  ?               +++++++
  - def g(x: int | None): pass
  + def g(x: int | None | None): pass
  ?               +++++++
  - def ag(x: int | None): pass
  + def ag(x: int | int | None): pass
  ?                ++++++
  - def h(x: int | None): pass
  + def h(x: int | None | int): pass
  ?                    ++++++

I'd be interested to hear your opinions on this. If you do think it would be a complimentary feature for pyupgrade then I'm happy to submit a PR for consideration to get the test above passing.

@asottile
Copy link
Owner

this is pretty difficult to do in a single pass but should be somewhat doable

@asottile
Copy link
Owner

something similar is done for except blocks already if you want to try and tackle this for type unions as well

@walter9388
Copy link
Author

Ok nice, I just had a very quick look and can see where this happens for except blocks.
I will review when I have a bit more time in the next few days and submit a PR when I can :)
Cheers

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

No branches or pull requests

2 participants