asottile / pyupgrade

A tool (and pre-commit hook) to automatically upgrade syntax for newer versions of the language.
MIT License
3.62k stars 183 forks source link

PEP 604 typing duplication removal #982

Open walter9388 opened 1 month ago

walter9388 commented 1 month ago

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 commented 1 month ago

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

asottile commented 1 month ago

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

walter9388 commented 1 month ago

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