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 TRIO232: blocking sync call on file objects #104

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Jan 19, 2023

A first draft at tracking types for file/path/httpx/etc and warning on sync calls. Except this one just does it for open() and the varios io.* types it can return as a start.
This and others that build on top of it should finish off #58

Branched from on top of #103 - but you can just view the latest commit to see the contents of this one.

  • The logic for tracking variables and their type will be moved to a superclass that different error_classes can inherit from (or possibly just implemented in Flake8TrioVisitor.
    • Child classes will probably be able to register calls and their return type so the superclass can use that. (so something like super().register('open', 'io.TextIOWrapper') in the __init__
    • code can then maybe be reused for the error classes tracking nurseries
  • I don't think anybody in their right mind is going to send around io.TextIOWrapper objects to functions, but that's going to be much more common with Path and httpx objects, so I implemented it for this as well just to show the logic.
  • Unless reimplemented as a plugin an top of a type checker this will ofc never be perfect, but handling Optional[Path] and Path|None will be done. list[Path] and similar cases can be done if it makes sense but I don't think will be common of any of the types we care about.
  • I thiiink scope rules are handled correctly at the moment, but if that turns out to be tricky it can probably be dropped without worry.
  • Code in general isn't super clean and will be rewritten, so don't bother with nitpicking much/at all

@jakkdl jakkdl force-pushed the path_file_objects branch 3 times, most recently from 0e2bf9a to 21e2cc5 Compare January 26, 2023 09:37
@Zac-HD
Copy link
Member

Zac-HD commented Jan 27, 2023

Approach seems promising, agree that list[whatever] isn't worth handling but whatever|None probably is (if easy!). I look forward to seeing where this goes after rebasing 😁

@jakkdl
Copy link
Member Author

jakkdl commented Jan 27, 2023

Rebased on top of main (which wasn't trivial since it was forked from an outdated #103 that was later amended - fun stuff!).
Will now continue working on it~

@jakkdl jakkdl marked this pull request as ready for review January 27, 2023 13:27
@jakkdl
Copy link
Member Author

jakkdl commented Jan 27, 2023

I'll refactor it in a different PR as I write the next object sync checker, as this one should work well as a standalone. Ready to merge!

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny comment on tests, otherwise looks great.

Comment on lines +45 to +46
if f:
f.read() # TRIO232: 8, 'read', 'f'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this still give a lint error without the if, or in an else? It'd be nice to demonstrate that with tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it currently does not care at all that it could be a None - but I had type-checking turned on in my editor when writing this one hence the if's. You're completely right it should be shown in tests though - and will fix in followup PR 👍

@Zac-HD Zac-HD merged commit 3582444 into python-trio:main Jan 27, 2023
@Zac-HD
Copy link
Member

Zac-HD commented Jan 27, 2023

For the future refactored version, one annoying (to lint, it's fortunately rare) pattern is defining a httpx.Client() as a global variable...

@jakkdl
Copy link
Member Author

jakkdl commented Jan 28, 2023

For the future refactored version, one annoying (to lint, it's fortunately rare) pattern is defining a httpx.Client() as a global variable...

oh and it should work fine with global variables as long as they're defined in the same file. Will add tests to show that.

I'll make a PR just with these two fixes + another pattern I just realized will be useful:

async def type_assign():
    f: TextIOWrapper = ...
    f.read()

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

Successfully merging this pull request may close these issues.

None yet

2 participants