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

reportUnreachable and if sys.version_info #938

Closed
jorenham opened this issue Dec 8, 2024 · 9 comments
Closed

reportUnreachable and if sys.version_info #938

jorenham opened this issue Dec 8, 2024 · 9 comments

Comments

@jorenham
Copy link
Contributor

jorenham commented Dec 8, 2024

Environment-dependent conditionals like these shouldn't reportUnreachable. When testing this on multiple python versions, then it's impossible to pyright: ignore[reportUnreachable]. So the only options left are to either disable it per-module, or disable it globally. Both are sub-optimal.

import sys

if sys.version_info >= (3, 13):
    def spam(*, py313_only_kwarg: str = "poopoo") -> str:
        return py313_only_kwarg
else:
    def spam() -> None: ...  # Code is unreachable  (reportUnreachable)

https://basedpyright.com/?typeCheckingMode=all&code=JYWwDg9gTgLgBAZwJ4ILACgPAGaJQOgDcBTKBYCAOwH1hLsI4A%2BAXjgAoBmAGjgEZOASgBcGOOLgATYrgRgAhiHYAqXmCScB1KgBsk1ANYB3eVADmwxDChw2AIkgRHdwXAC0TK1FHoJfuFDEMACuUJRw6pqc2pR6hibmGMQ6CMQ%2BftKyCkquHnAAclRpcPilGEA

So I think that instead, reportUnreachable should be limited to code that's "unreachable on all valid platforms and python versions".

@DetachHead
Copy link
Owner

Duplicate of #8

@DetachHead DetachHead marked this as a duplicate of #8 Dec 8, 2024
@DetachHead DetachHead closed this as not planned Won't fix, can't repro, duplicate, stale Dec 8, 2024
@jorenham
Copy link
Contributor Author

jorenham commented Dec 9, 2024

So I think that instead, reportUnreachable should be limited to code that's "unreachable on all valid platforms and python versions".

well, kinda, but I also propose a semi-solution here:

So I think that instead, reportUnreachable should be limited to code that's "unreachable on all valid platforms and python versions".

@DetachHead
Copy link
Owner

i haven't looked into it but i think that would require the full solution, because pyright currently doesn't have a way to specify multiple valid python versions

@jorenham
Copy link
Contributor Author

jorenham commented Dec 9, 2024

pyright currently doesn't have a way to specify multiple valid python versions

That's not needed in this case. The current pythonVersion version is a the lower bound, so everything >=pythonVersion can be considered as "valid" python versions.

@DetachHead
Copy link
Owner

from my understanding pythonVersion doesn't currently mean the lower bound, but the exact minor version of python that pyright expects will be used to run your code, which is what it says in the docs:

  • pythonVersion [string, optional]: Specifies the version of Python that will be used to execute the source code. The version should be specified as a string in the format "M.m" where M is the major version and m is the minor (e.g. "3.0" or "3.6"). If a version is provided, pyright will generate errors if the source code makes use of language features that are not supported in that version. It will also tailor its use of type stub files, which conditionalizes type definitions based on the version. If no version is specified, pyright will use the version of the current python interpreter, if one is present.

i agree that this is pretty annoying in many cases because i'm pretty sure the most common use case for it is to define a lower bound.

@jorenham
Copy link
Contributor Author

jorenham commented Dec 9, 2024

from my understanding pythonVersion doesn't currently mean the lower bound, but the exact minor version of python that pyright expects will be used to run your code, which is what it says in the docs:

Python typing is for the most part backwards-compatible. And if it's not, e.g. when typing.Hashable will be removed, then (based)pyright would indeed fail to see that using typing.Hashable is invalid. So there can be cases like that (when you have a large support window) where using sys.version_info is the only possible way, e.g.

if sys.version_info >= (3, 12):
    from collections.abc import Hashable
else:
    # deprecated in 3.12
    from typing import Hashable

So in this case 3.12 should be considered a valid version (for the purpose of refining reportUnreachable in the presence of a sys.version_info), because pythonVersion is set to something below that, e.g. pythonVersion=3.7.

@DetachHead
Copy link
Owner

i agree that it should behave like that, but since it currently doesn't, and because i want to maintain backwards compatibility with pyright, i will probably keep the current behavior when specifying pythonVersion = "3.7" but add support for a range syntax to support this use case (eg. pythonVersion = ">=3.7"). this will also allow me to address #921 as well

@jorenham
Copy link
Contributor Author

i want to maintain backwards compatibility with pyright

I like that!

But I thought you didn't care about that before? ... or was that only @KotlinIsland 🤔

@DetachHead
Copy link
Owner

@KotlinIsland and i have different goals with our projects. i think backwards compatibility is important (unfortunately) and i can't expect basedpyright to gain any serious adoption without it

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