facebook / pyre-check

Performant type-checking for python.
https://pyre-check.org/
MIT License
6.85k stars 437 forks source link

Pyre failing in python 3.8 and 3.9 due to new syntax #827

Closed henrylhtsang closed 5 months ago

henrylhtsang commented 6 months ago

Pyre Bug

Bug description

When using python 3.8 and 3.9 with pyre, getting the following error due to code: Optional[int | str] = None https://github.com/facebook/pyre-check/blob/main/client/language_server/protocol.py#L365

Reproduction steps Enter steps to reproduce the behavior. Follow https://github.com/pytorch/torchrec/blob/main/.github/workflows/pyre.yml to run pyre check with "version": "0.0.101711451648"

Expected behavior It should run.

The syntax is only created in python 3.10 and after. https://stackoverflow.com/questions/76712720/typeerror-unsupported-operand-types-for-type-and-nonetype

Logs

Traceback (most recent call last):
  File "/usr/share/miniconda/envs/test/bin/pyre", line 5, in <module>
    from pyre_check.client.pyre import main
  File "/usr/share/miniconda/envs/test/lib/python3.[8](https://github.com/pytorch/torchrec/actions/runs/8793083113/job/24130384280#step:5:9)/site-packages/pyre_check/client/pyre.py", line 31, in <module>
    from . import (
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/commands/__init__.py", line 16, in <module>
    from . import (  # noqa F401
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/commands/analyze.py", line 30, in <module>
    from . import commands, start, validate_models
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/commands/start.py", line 51, in <module>
    from . import commands, server_event, stop
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/commands/server_event.py", line 22, in <module>
    from ..language_server import connections
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/language_server/__init__.py", line [13](https://github.com/pytorch/torchrec/actions/runs/8793083113/job/24130384280#step:5:14), in <module>
    from . import (  # noqa F401
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/language_server/code_navigation_request.py", line 22, in <module>
    from . import daemon_connection, protocol as lsp
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/language_server/protocol.py", line 361, in <module>
    class Diagnostic(json_mixins.CamlCaseAndExcludeJsonMixin):
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/pyre_check/client/language_server/protocol.py", line 365, in Diagnostic
    code: Optional[int | str] = None
TypeError: unsupported operand type(s) for |: 'type' and 'type'
henrylhtsang commented 6 months ago

cc @MaggieMoss

migeed-z commented 6 months ago

Does the following work for you?

from typing import *

x: Optional[int | str] = None

It does on the latest version of Pyre. If this doesn't work, we may need to do an upgrade.

henrylhtsang commented 6 months ago

@migeed-z not suer if I understand correctly, I thought it got an error since int | str is a new syntax for python 3.10 only? And that part code: Optional[int | str] = None is in pyre code

migeed-z commented 6 months ago

To verify if the error is due to unsupported syntax, could you doublecheck if the code above type check for you?

henrylhtsang commented 6 months ago

@migeed-z interesting, I tested in https://github.com/pytorch/torchrec/actions/runs/8807458504/job/24174554261

pyre is python 3.8 with "version": "0.0.101703592829", but does not see an error.

migeed-z commented 6 months ago

Did you test that one small repro or the entire code?

henrylhtsang commented 6 months ago

@migeed-z I added that code snippet to a single unit test, and then run the pyre github action with it.

This one:

from typing import *

x: Optional[int | str] = None
migeed-z commented 6 months ago

I see. Thanks for the information. Are we able to run on the same version of the original code though? because the error message you have originally suggests the syntax is not supported, so I wonder if there is a variation between both versions or if your original code has a different issue.

cclauss commented 6 months ago

On Python < 3.10 the source file must start with from __future__ import annotations to be able to use PEP 604 syntax.

On Python < 3.10, there should be two tests:

  1. A source file that starts with from __future__ import annotations should pass if it uses PEP 604 syntax.
  2. A source file that starts without from __future__ import annotations should fail if it uses PEP 604 syntax.
stroxler commented 5 months ago

Closing the loop on this: I backed all these out to only use Union.

We cannot make use of from __future__ import annotations because this code is using the types at runtime (via dataclasses_json.

I've increased the number of tests we run in github CI to make this kind of error less likely (we have to rely on github CI because internally we only use 3.10, so we tend to not notice 3.8/3.9 issues right away)