change imports to `from Y import X as X` for compatibility with `pyright --verifytypes` #2629

jakkdl · 2023-04-05T15:21:41Z

Completes the first task in #2625

Should probably not be merged before a test for test_exports.py is written that checks that the exports seen by verifytypes is the same as the other static analyzers, and/or the CI check for type completeness.

remaining TODO:

make sure py.typed exists in tests.test_exports.test_static_tool...
save the new results from verifytypes in git in CI
get pyright to play along by setting PYRIGHT_FORCE_PYTHON_VERSION in all the places.

codecov · 2023-04-05T15:26:40Z

Codecov Report

Merging #2629 (d007808) into master (bec7cc6) will increase coverage by 0.00%.
The diff coverage is 90.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2629   +/-   ##
=======================================
  Coverage   98.10%   98.11%           
=======================================
  Files         118      118           
  Lines       16471    16480    +9     
  Branches     2977     2983    +6     
=======================================
+ Hits        16159    16169   +10     
  Misses        255      255           
+ Partials       57       56    -1

Impacted Files	Coverage Δ
trio/abc.py	`100.00% <ø> (ø)`
trio/testing/__init__.py	`0.00% <0.00%> (ø)`
trio/__init__.py	`100.00% <100.00%> (ø)`
trio/_tests/test_exports.py	`95.73% <100.00%> (+0.96%)`	⬆️
trio/from_thread.py	`100.00% <100.00%> (ø)`
trio/lowlevel.py	`100.00% <100.00%> (ø)`
trio/socket.py	`100.00% <100.00%> (ø)`
trio/to_thread.py	`100.00% <100.00%> (ø)`

…ht --verifytypes

A5rocks · 2023-04-05T21:50:03Z

Funnily enough I also did this yesterday, though I see I did trio/socket and you didn't. (I'm not too sure if the as is necessary there TBF).

Also we re-export some symbols via from A import X as Y and I'm not sure how to make that work here.

URL: A5rocks@37bb8ed

jakkdl · 2023-04-06T17:54:29Z

Funnily enough I also did this yesterday, though I see I did trio/socket and you didn't. (I'm not too sure if the as is necessary there TBF).

Also we re-export some symbols via from A import X as Y and I'm not sure how to make that work here.

URL: A5rocks@37bb8ed

haha, good that it's mostly just some regex search&replace.

The as is only needed when you're re-exporting from a private module, so I intentionally skipped all the ones in socket - and one or two in the other files iirc. But a test_export test should settle it anyhow

…bols, and add missing exports

trio/tests/verify_types.json

trio/tests/test_exports.py

trio/tests/verify_types.json

jakkdl · 2023-04-10T10:26:22Z

whee, finally managed to wrangle CI to pass
.
Only thing left now is getting CI to save the changes in verify_types.json after running check_type_completeness in in this repo, or another repo, or somewhere else.

It seems there's a bunch of ways of doing that but idk what the setup with secrets and stuff is, and seems related to the discussion that went on in #2592 that stalled out when it got into complications around that.

A5rocks · 2023-04-11T11:06:40Z

Only thing left now is getting CI to save the changes in verify_types.json after running check_type_completeness in in this repo, or another repo, or somewhere else.

Why not make the test error when type completeness increases, telling the user to update the JSON file?

jakkdl · 2023-04-12T09:00:03Z

Only thing left now is getting CI to save the changes in verify_types.json after running check_type_completeness in in this repo, or another repo, or somewhere else.

Why not make the test error when type completeness increases, telling the user to update the JSON file?

ooh that's a simple solution. Me like!

jakkdl · 2023-04-12T13:31:17Z

That works, but only after #2628 has been merged - cause at least on my different environments I get different results for a bunch of symbols in trio.tests, possibly due to different python versions, which might make regenerating the json file a possible nightmare for some users if they've not been very careful with their virtual environments.

A5rocks

Minor comments, will be happy to merge once you fix. I'll try to be much more responsive with types-specific PRs!

check.sh

trio/__init__.py

trio/_tests/check_type_completeness.py

trio/_tests/test_exports.py

A5rocks · 2023-05-29T02:27:00Z

trio/_tests/test_exports.py

+            if modname + "." in x.decode()
+        }
+
+        # don't require verifytypes to see reexported constants from socket


I disagree with this comment but if it's annoying we can bump this off

Oh, trio.socket needs a similar treatment as trio.Path had to get the constants properly visible.

wait no. hmm

seems like pytest, similar to with __version__, ignores constants with the same name as those in socket (probably because we import from it?) - even when running --verbose. It seems to even ignore them when there's typing errors in them.

Type annotations can be omitted in a few specific cases where the type is obvious from the context:

...

Constants that are assigned simple literal values (e.g. RED = '#F00' or MAX_TIMEOUT = 50 or room_temperature: Final = 20). A constant is a symbol that is assigned only once and is either annotated with Final or is named in all-caps. A constant that is not assigned a simple literal value requires explicit annotations, preferably with a Final annotation (e.g. WOODWINDS: Final[list[str]] = ['Oboe', 'Bassoon']).
https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#type-completeness

So it's plausibly a bug that they don't show up in --verbose or --outputjson, but it's intended that they're not covered by --verifytypes.

Actually no, it's the usual

Imported symbols are considered private by default. If they use the “import A as A” (a redundant module alias), “from X import A as A” (a redundant symbol alias), or “from . import A” forms, symbol “A” is not private unless the name begins with an underscore.

that hits here. So what'd need to be changed here is changing the huge list of imports from socket to be from socket import (X as X, Y as Y, Z as Z).

I don't think we care for this in terms of --verifytypes, but if somebody uses pyright as their type checker it will give an error of reportPrivateImportUsage when they try accessing it. That seems important enough to warrant making that huge bulk of an import statement even messier

Hm yeah, well it's a gigantic change so let's maybe sit on it a bit! ~~Maybe an ad-hoc codemod would help out a ton, in combination with a run from black?~~ Actually it's simple enough to find-and-replace, honestly. NVM

vim macros FTW :D

trio/_tests/test_exports.py

trio/_timeouts.py

trio/socket.py

…xports

jakkdl · 2023-05-30T13:09:25Z

I have no clue why the omit isn't working in .coveragerc
EDIT: should be fixed by #2654

jakkdl · 2023-05-31T11:36:41Z

Okay, I think that's all comments addressed. The drop in codecov I think is solely due to #2645

A5rocks · 2023-06-01T00:19:58Z

trio/_tests/check_type_completeness.py

+        new_symbols = []
+        # remove path & line/col references in errors
+        for symbol in current_result["typeCompleteness"]["symbols"]:
+            # filter out symbols with no errors
+            if not symbol["diagnostics"]:
+                continue
+            for diag in symbol["diagnostics"]:
+                del diag["file"]
+                diag.pop("range", None)
+            new_symbols.append(symbol)
+
+        current_result["typeCompleteness"]["symbols"] = new_symbols
+
+        with open(RESULT_FILE, "w") as file:
+            json.dump(current_result, file, sort_keys=True, indent=2)
+            # add newline at end of file so it's easier to manually modify
+            file.write("\n")


OK I reread things and I kinda don't get why we keep symbols at all. I don't see them being used elsewhere, am I missing something? They're a gigantic list and probably most of the reason the file is as big as it is.

We initially removed it, but I realized when I was trying out stuff that without it you won't get any feedback if/when the check fails. The numbers will change, but unless you checkout previous revisions, manually run pyright --verifytypes, pipe into a file, and manually diff against a manual --verifytypes run on your PR - you're left guessing what symbol you broke where. And if your local environment differs from CI, then you're absolutely toast. (I have recurring problems with CI not giving same results as running stuff in my virtualenv and/or tox envs)

I could shorten the file further, removing more info that's probably redundant, not pretty-print it - or like dump symbols into a compressed pickle that the script reads to say what symbols have changed, but:

the json file is not included when packaging

the json file will likely drop down in size a lot early on as I do a first pass and introduce a lot of trivial typing on functions.

If you still think it's a problem, I'll make a pass at sizing it down. Or I can toss symbols out for now, run the script modified locally, and after (2) when the file is smaller add it to the repo.

Other esoteric options include commiting the file to a different branch, git-annex, have it in a submodule...

Hmm, my concern is that we're making the repository larger for anyone who wants a (full) clone. Do you have measurements of the .git folder before and after this? (You may want to rebase first to get rid of the old much bigger json file)

Looks like going from something like 7.9M -> 8.1M
But it's a good point, I hadn't realized that big files will stick around forever in the history. I'll test pruning it down to being just a list of symbols with no extra info

jakkdl · 2023-06-01T20:14:11Z

decided to go back to running with --ignoreexternal - while some symbols were stuff that should be fixed, a lot of it was it complaining about the hacky imports in trio.socket and similar low prio stuff. #FutureProblem

A5rocks

I think this looks good? I'll review in slightly more detail once this is rebased!

jakkdl · 2023-06-06T10:00:37Z

@A5rocks merged main

A5rocks

Looks good overall, small little nitpicks.

trio/__init__.py

ci.sh

trio/_tests/check_type_completeness.py

A5rocks · 2023-06-09T08:54:08Z

trio/_tests/check_type_completeness.py

+
+    print("*" * 20)
+
+    return int(failed)


bool is a subclass of int so this isn't necessary. But maybe nice for clarity? Dunno.

haha, I guess it's my c++ programming leaking. But yeah I don't mind the int conversion

A5rocks · 2023-06-09T08:58:57Z

trio/socket.py

+# all these constants, but it lets static analysis tools understand what's
+# going on. There's a test in test_exports.py to make sure that the list is
+# kept up to date.
+try:


Maybe we should put this in an if TYPE_CHECKING block? But eh, I guess it does literally say "It always fails at runtime" and I don't know how this might change tools other than type checkers. Ig we can push this change off if you want!

looks like jedi doesn't care, so switching it to be a TYPE_CHECKING block instead.

A5rocks

Looks good, quickly glanced through and didn't see anything! If tests pass, feel free to merge this after making commit history more succinct.

jakkdl · 2023-06-09T11:50:00Z

🎉 🎉 🚀 🚀 🚀

jakkdl force-pushed the import_x_as_x branch 2 times, most recently from 1348e3e to b4a46d5 Compare April 5, 2023 15:24

Change imports to 'from y import x as x' for compatibility with pyrig…

98ba041

…ht --verifytypes

jakkdl force-pushed the import_x_as_x branch from b4a46d5 to 98ba041 Compare April 5, 2023 15:28

jakkdl added 2 commits April 7, 2023 13:39

Add pyright_verifytypes to test_exports.test_static_tool_sees_all_sym…

d66b030

…bols, and add missing exports

Add check for type completeness not going down

98d9db6

jakkdl force-pushed the import_x_as_x branch from d85891c to 98d9db6 Compare April 7, 2023 12:55

A5rocks reviewed Apr 7, 2023

View reviewed changes

trio/tests/verify_types.json Outdated Show resolved Hide resolved

trio/tests/test_exports.py Outdated Show resolved Hide resolved

jakkdl added 3 commits April 7, 2023 16:20

trim json file, get closer to it working in CI

881f45b

fix failing CI

133b222

.

bbb4c9b

A5rocks reviewed Apr 9, 2023

View reviewed changes

trio/tests/verify_types.json Outdated Show resolved Hide resolved

trio/tests/verify_types.json Outdated Show resolved Hide resolved

jakkdl added 3 commits April 10, 2023 12:03

remove paths from json, fix ci?

e0f68fc

fix symbols on windows&mac, fix formatting errors

2f6cfc3

undo modifications to req files, import WaitForSingleObject as...

ffd1984

jakkdl mentioned this pull request Apr 15, 2023

move **/tests/ to **/_tests/ #2628

Merged

jakkdl added 3 commits May 25, 2023 15:59

move files before merge

57f564d

Merge branch 'main' into merge_importx

2446507

update verify_types.json

14cf061

jakkdl force-pushed the import_x_as_x branch from 033c62f to 14cf061 Compare May 25, 2023 14:38

jakkdl added 3 commits May 25, 2023 16:48

print json diff if modified

ee9bcb8

update pyright version

8b01b68

fix windows 3.7 CI, remove version from json

fe68911

A5rocks requested changes May 29, 2023

View reviewed changes

jakkdl added 3 commits May 30, 2023 13:30

Merge branch 'main' into import_x_as_x

2e34089

fix review comments, add full verify_types.json

2c161b9

remove path/line/col references in errors

a78123a

jakkdl mentioned this pull request May 30, 2023

specify --cov-config=.coveragerc in ci.sh #2654

Merged

filter out symbols with no errors in json, use --outputjson in test_e…

7e12b88

…xports

jakkdl and others added 4 commits May 31, 2023 12:23

Merge branch 'master' into import_x_as_x

4560301

reexport constants from socket with 'import x as x' for pyright

0a65d5d

update comment on __version__

ddece80

type: ignore[attr-defined] on the big import in socket

21b3f75

jakkdl requested a review from A5rocks May 31, 2023 11:37

jakkdl added 2 commits May 31, 2023 13:59

update verify_types since trio.socket re-exports are visible

968ec64

remove --ignoreexternal

b018e76

A5rocks reviewed Jun 1, 2023

View reviewed changes

reintroduce --ignoreexternal, prune symbols list heavily

811a4b8

jakkdl requested a review from A5rocks June 5, 2023 15:49

A5rocks reviewed Jun 6, 2023

View reviewed changes

jakkdl added 2 commits June 6, 2023 11:32

merge main

54f79ab

update verify_types.json

bf1d995

jakkdl mentioned this pull request Jun 6, 2023

Test sdist contents instead of Git checkout in CI #2541

Merged

A5rocks approved these changes Jun 9, 2023

View reviewed changes

review fixes

d007808

A5rocks approved these changes Jun 9, 2023

View reviewed changes

jakkdl merged commit 493c915 into python-trio:master Jun 9, 2023

jakkdl deleted the import_x_as_x branch June 9, 2023 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change imports to `from Y import X as X` for compatibility with `pyright --verifytypes` #2629

change imports to `from Y import X as X` for compatibility with `pyright --verifytypes` #2629

jakkdl commented Apr 5, 2023 •

edited by A5rocks

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading

A5rocks commented Apr 5, 2023

jakkdl commented Apr 6, 2023 •

edited

Loading

jakkdl commented Apr 10, 2023

A5rocks commented Apr 11, 2023

jakkdl commented Apr 12, 2023

jakkdl commented Apr 12, 2023

A5rocks left a comment

A5rocks May 29, 2023

jakkdl May 30, 2023

jakkdl May 30, 2023

jakkdl May 30, 2023

jakkdl May 31, 2023

jakkdl May 31, 2023

A5rocks Jun 1, 2023 •

edited

Loading

jakkdl Jun 1, 2023

jakkdl commented May 30, 2023 •

edited

Loading

jakkdl commented May 31, 2023

A5rocks Jun 1, 2023

jakkdl Jun 1, 2023

A5rocks Jun 1, 2023

jakkdl Jun 1, 2023

jakkdl commented Jun 1, 2023 •

edited

Loading

A5rocks left a comment

jakkdl commented Jun 6, 2023

A5rocks left a comment

A5rocks Jun 9, 2023

jakkdl Jun 9, 2023

A5rocks Jun 9, 2023

jakkdl Jun 9, 2023

A5rocks left a comment

jakkdl commented Jun 9, 2023

change imports to from Y import X as X for compatibility with pyright --verifytypes #2629

change imports to from Y import X as X for compatibility with pyright --verifytypes #2629

Conversation

jakkdl commented Apr 5, 2023 • edited by A5rocks Loading

codecov bot commented Apr 5, 2023 • edited Loading

Codecov Report

A5rocks commented Apr 5, 2023

jakkdl commented Apr 6, 2023 • edited Loading

jakkdl commented Apr 10, 2023

A5rocks commented Apr 11, 2023

jakkdl commented Apr 12, 2023

jakkdl commented Apr 12, 2023

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A5rocks Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented May 30, 2023 • edited Loading

jakkdl commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Jun 1, 2023 • edited Loading

A5rocks left a comment

Choose a reason for hiding this comment

jakkdl commented Jun 6, 2023

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A5rocks left a comment

Choose a reason for hiding this comment

jakkdl commented Jun 9, 2023

change imports to `from Y import X as X` for compatibility with `pyright --verifytypes` #2629

change imports to `from Y import X as X` for compatibility with `pyright --verifytypes` #2629

jakkdl commented Apr 5, 2023 •

edited by A5rocks

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading

jakkdl commented Apr 6, 2023 •

edited

Loading

A5rocks Jun 1, 2023 •

edited

Loading

jakkdl commented May 30, 2023 •

edited

Loading

jakkdl commented Jun 1, 2023 •

edited

Loading