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

Add pyright --verifytypes to CI & checks #2625

Closed
3 tasks done
jakkdl opened this issue Mar 31, 2023 · 4 comments
Closed
3 tasks done

Add pyright --verifytypes to CI & checks #2625

jakkdl opened this issue Mar 31, 2023 · 4 comments

Comments

@jakkdl
Copy link
Member

jakkdl commented Mar 31, 2023

In order to be able to eventually retire trio-typing (see https://github.com/python-trio/trio-typing), track progress on #543 (and once type complete, make sure that we stay that way) and as a minor bonus give a check on docstrings, I suggest adding
pyright --verifytypes to the checks in CI, that at a beginning is required only to go up from current state, and when we've achieved 100% type completeness will then require not dropping below that.
See #2623 where I reinvented the wheel in a quite clunky way and some discussion around it, https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#type-completeness for the definition of type completeness, and https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#verifying-type-completeness for documentation on --verifytypes.

There's unfortunately a couple blockers before we're able to use it.

  1. Imports in __init_.py, socket.py, abc.py, from_thread.py, lowlevel.py, socket.py and to_thread.py are not visible to --verifytypes. Also related to Use less magic in constructing our public API exports, to make trio more intelligible to static analysis #542
  2. trio.tests is a public module and is included as a submodule that needs verifying. See Fix our test layout #274, suggestions there include renaming trio/tests to trio/_tests, moving it out to a separate top-level tests/, or removing trio/tests/__init__.py.
  3. How to actually check that the type completeness score doesn't go down.
    • A relatively straightforward solution would be to use --outputjson and dump the results in a _type_completeness.json, which in check.sh is parsed for the % and the individual values, then regenerated on the new code and reparsed, and finally compare the individual values so none of them are dropping (other than the number of symbols exported, or the number of symbols exported with known type.) This'd be a quick&dirty python/shell script, and can make it take a command-line flag whether to overwrite the file or not (that is set in CI, but otherwise not) so it can be used locally repeatedly when developing.

Ticking all these boxes is definitely gonna take a while and bunch of discussion, so I don't personally consider it completely out of the question to use #2623 in the meanwhile - but a better solution would probably be a dirty script that does something like

  1. copy the contents of trio/ to a separate directory
  2. run a replace on __init__.py and friends to change all the imports to from X import A as A
  3. remove/rename the trio/tests/ directory

This can then be retired/simplified as 1&2 are fixed.

@A5rocks @Fuyukai @Zac-HD @njsmith

@jakkdl
Copy link
Member Author

jakkdl commented Mar 31, 2023

For reference: #2623 shows 39 symbols being typed (though some are incomplete) while it lists 314 symbols as not typed, so about 11% complete. Though this can probably be improved quite quickly by copying over from the stubs, and leave typing messy internal functions for later.

@Fuyukai
Copy link
Member

Fuyukai commented Mar 31, 2023

On one hand, the pyright design decision is really stupid. On the other hand, I hate "private" packages/modules. I'd say I support removing private packages (and just making a normal package layout) more.

@njsmith
Copy link
Member

njsmith commented Mar 31, 2023

We're not going to remove the underscores from the internal implementation modules... I've been burned too often by Hyrum's law :-) The details of how pyright does this are annoying, but it's actually good that it won't let code that uses our private names directly type-check. The idea is to make sure that what we make public as supported interfaces is exactly what we intend it to be, no more or less.

IIRC if we do import * then it will break pylint or pycharm... I'd have to go dig through #594 and #542 to find the details, but we used to define __all__ in each submodule and then in the top-level import * + define __all__ = submodule1.__all__ + submodule2.__all__ + ..., and we switched to the current explicit list of imports because at least one static analyzer absolutely refused to work like that.

Using import X as X everywhere in our public APIs might be the least-bad option here? It's silly looking, but at least it's a straightforward mechanical change, and it keeps the nice property that when we add a new public export, there's still only one line that needs to be added. (Versus eg defining __all__, where you need to make sure the imports and the __all__ are kept in sync.)

@A5rocks
Copy link
Contributor

A5rocks commented Jul 2, 2023

I believe everything in this issue is done? (I've ticked the checkboxes just to make sure)

If not, feel free to reopen!

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

4 participants