python-trio / flake8-async

Highly opinionated linter for Trio code
https://flake8-async.readthedocs.io
MIT License
19 stars 2 forks source link

Add trio111, passing variables from context managers to nurseries opened outside them #22

Closed jakkdl closed 2 years ago

jakkdl commented 2 years ago

A couple test cases I'm unsure how they should be handled, so make sure to check the test file thoroughly.

I'm also slightly confused about your definition of inner/outer - you say "should be outermost" but https://github.com/python-trio/trio/issues/671#issuecomment-533978268 shows

async with trio.open_nursery():
  async with trio.open_process():
    ...

being troublesome, which I'd call the nursery being the outer scope - and as such nurseries should be "innermost".

But easy to reverse logic / error message as needed.

Zac-HD commented 2 years ago

Via https://github.com/python-trio/trio/issues/671#issuecomment-533989210:

I guess the general case of detecting when something is closed while in use would require some super sophisticated data-flow analysis, but this specific pattern of async with ... as x around a call to start_soon(..., x) seems like it could be simple enough for a linter to detect + a common enough error to be a useful lint.

Though we might want to special-case async with open_nursery() as nursery: nursery.start_soon(..., nursery), since that's not an error. More generally, I guess the point is that we need to also have some heuristic to rule out cases where the async with surrounds the nursery.

Yeah, you're entirely right; the nursery needs to be innermost not outermost.

Important to note though that (to reduce false-alarms) we only want to warn if we have something from a context manager passed to the task(s) being started, and the context will exit before the nursery exits.

We'll also want to check for nursery.start() as well as .start_soon().

jakkdl commented 2 years ago

Ah, so:

Zac-HD commented 2 years ago

That's right! I'd check for any sync-or-async context manager to start.

jakkdl commented 2 years ago

Haven't made a second pass through the code to clean up and add comments and stuff, but pushing a dirty version so you can check if the tests fit the specification.

The error message is also ... a WIP :grin:

"TRIO302": "call to nursery.start/start_soon with resource from context manager opened on line {} something something nursery on line {}",

should likely add the variable names of both the nursery and the context manager too.

Note to self: I don't currently point to the correct parameter when there's a multi-line call (nor have a check for that). Also realizing now I don't cover nursery.start(*bar), nursery.start(**bar), passing bar several times, or passing stuff like list(tuple(bar)). But think it'll be pretty doable.

I also found small bug in tester printer, will branch that off as separate PR.

jakkdl commented 2 years ago

Fixed stuff mentioned above, though code is still dirty and uncommented. Wanna take a pass through it tomorrow morning with a fresh head.

jakkdl commented 2 years ago

Merged in #34, so you should merge that before looking at the diff.

Code should be all pretty and commented now~ :sparkles: