Add pyrefly type checking.#3412
Conversation
|
Typing is not my forte. In my projects I just fire up a So I'll let other give feedback on that topic and I'll just follow the movement! 😁 |
|
I'd prefer to stick to mypy for now. If we're still testing with pyright, we can remove that; it was added when I thought we needed to test with all of them. But really, we just need to test with I know pyrefly is working on an export verifcation as well, but I tried last month and both its type checking and verifcation weren't ready yet. Same with ty. I'm really interested in those two, but prefer we wait for now. mypy, for all its quirks, is very mature and actively maintained. |
|
Regarding static typing in general, remember: annotations are there to serve us and our users, not the other way around. Click (and all of Pallets) predates static typing, and we would design it differently today to be better typed. So if we're doing something weird and typing can't express it (or it's too complicated to figure out), you're fine adding |
|
Wasn't expecting the comments just yet. 😅 I was primarily using this to experiment a bit on where we could improve typing. It's very neat to see where we could make the typing experience Click offers better, and also make it less of a pain to use inside of the codebase. So far it's teaching and frustrating me, as I'm expecting certain typing to be possible (as someone who uses Rust quite a bit) and then it seems to not be possible. My current struggle is trying to plug through the binary/text file modes without having to repeat I'll see where my experimentation gets me. But I'm not expecting all that much just yet. So far Pyrefly is easier to use than
And Regardless of where the typing experimentation goes, I do think it'd be useful to have multiple type checkers available. It sometimes helps to see how each one of them views a problem, even when we only care about a single one (mypy). Would we be able to have pyrefly and ty added and available via |
|
Yeah it's fine to add config for it and use it to figure things out, just don't want to require it in CI yet. |
|
I am interested to see where this goes. I switched to pyrefly mostly because the Language server works better in vscode. |
jorenham
left a comment
There was a problem hiding this comment.
Yay Pyrefly 🎉.
I took a quick look and left some comments; hope you don't mind.
| - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 | ||
| with: | ||
| python-version-file: pyproject.toml |
There was a problem hiding this comment.
There's no need for setup-python if you're using setup-uv
| ) -> tuple[t.IO[bytes], bool]: ... | ||
|
|
||
|
|
||
| @t.overload | ||
| def open_stream( | ||
| filename: str | os.PathLike[str], | ||
| mode: OpenTextMode = "r", | ||
| encoding: str | None = None, | ||
| errors: str | None = "strict", | ||
| atomic: bool = False, | ||
| ) -> tuple[t.IO[str], bool]: ... | ||
|
|
||
|
|
||
| def open_stream( |
There was a problem hiding this comment.
nit: those newlines here aren't necessary, and makes things easier to read IMO.
| ) -> tuple[t.IO[bytes], bool]: ... | |
| @t.overload | |
| def open_stream( | |
| filename: str | os.PathLike[str], | |
| mode: OpenTextMode = "r", | |
| encoding: str | None = None, | |
| errors: str | None = "strict", | |
| atomic: bool = False, | |
| ) -> tuple[t.IO[str], bool]: ... | |
| def open_stream( | |
| ) -> tuple[t.IO[bytes], bool]: ... | |
| @t.overload | |
| def open_stream( | |
| filename: str | os.PathLike[str], | |
| mode: OpenTextMode = "r", | |
| encoding: str | None = None, | |
| errors: str | None = "strict", | |
| atomic: bool = False, | |
| ) -> tuple[t.IO[str], bool]: ... | |
| def open_stream( |
(btw, I like these overloads)
| return getattr(self._f, name) | ||
|
|
||
| def __enter__(self) -> _AtomicFile: | ||
| def __enter__(self) -> _AtomicFile[AnyStr]: |
There was a problem hiding this comment.
| def __enter__(self) -> _AtomicFile[AnyStr]: | |
| def __enter__(self) -> t.Self: |
| class _AtomicFile(t.IO[AnyStr], t.Generic[AnyStr]): | ||
| def __init__(self, f: t.IO[AnyStr], tmp_filename: str, real_filename: str) -> None: | ||
| self._f: t.IO[AnyStr] = f |
There was a problem hiding this comment.
It's almost always better to avoid using AnyStr if you can (there are differences between how type-checkers treat it because it's very underspecified in the typing spec). And here, you can parametrize the entire f type, i.e. with
_FileT_co = t.TypeVar("_FileT_co", bound=t.IO[Any], covariant=True)This way you capture more typing information, avoid issues with invariance, and avoid having to deal with the awkward AnyStr.
| from typing_extensions import Buffer | ||
|
|
||
| class _MSVCRTModule(t.Protocol): | ||
| def get_osfhandle(self, fd: int) -> int: ... |
There was a problem hiding this comment.
nit:
| def get_osfhandle(self, fd: int) -> int: ... | |
| def get_osfhandle(self, fd: int, /) -> int: ... |
|
|
||
|
|
||
| ShellCompleteType = t.TypeVar("ShellCompleteType", bound="type[ShellComplete]") | ||
| ShellCompleteType = t.TypeVar("ShellCompleteType", bound="ShellComplete") |
There was a problem hiding this comment.
nit: Now that it's no longer bound to a type, the name isn't accurate anymore. The type variable naming convention is to use private (underscore-prefixed) naming and a T as suffix.
| def secho( | ||
| message: t.Any | None = None, | ||
| file: t.IO[t.AnyStr] | None = None, | ||
| file: t.TextIO | t.BinaryIO | None = None, |
There was a problem hiding this comment.
t.TextIO | t.BinaryIOis a bit stricter (narrower) thant.IO`, which might lead to false positives.
| file: t.TextIO | t.BinaryIO | None = None, | |
| file: t.IO[Any] | None = None, |
|
|
||
|
|
||
| class File(ParamType[t.IO[t.Any]]): | ||
| class File(ParamType[t.IO[AnyStr]], t.Generic[AnyStr, OpenFileMode]): |
There was a problem hiding this comment.
Same as above; it's better to parametrize this using a covariant type variable with bound=t.IO[Any]
|
|
||
|
|
||
| class File(ParamType[t.IO[t.Any]]): | ||
| class File(ParamType[t.IO[AnyStr]], t.Generic[AnyStr, OpenFileMode]): |
There was a problem hiding this comment.
I would strongly recommend against using OpenFileMode here: Pyright, for example, tends to upcasts Literal string values to str when used in generics, so this can lead to unexpected and different outcomes between type-checkers.
If I were in you should, I'd instead use a TypeVar(..., bound=bytes | str) here, and update the __init__ overloads accordingly.
This adds pyrefly to the type checking. I'm working on a lot of typing improvements and it's helpful to have a third one because it's sometimes not really clear why pyright is saying typing is wrong. It already is saying some things are incorrect which I'm planning to look into.
I hope it's not going to lead to a conflict of typing issues, and if so, we can review this more closely in the future.
As of writing I've not fixed the typing issues, but I'll publish the PR once done.