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

[jakkdl]

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 python-trio/trio#671 (comment) 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]

Via python-trio/trio#671 (comment):

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]

Ah, so:

• If a nursery is opened and saved in variable X
• and inside that nursery a trio (or all?) async context manager is opened and saved in variable Y
• and inside that context manager there's a call X.start[_soon]([...], Y, [...])

[Zac-HD]

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

[jakkdl]

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 😁

"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]

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

[jakkdl]

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

Code should be all pretty and commented now~ ✨

[Zac-HD]

Check looks great; some comments about the message (tricky!) and the tests (also tricky 😅) but I think we'll probably ship this today!

[Zac-HD]

Suggested change

	"variable {2}, from context manager on line {0}, "
	"passed to {3} from nursery opened on {1}, might get closed while in use"
	"variable {} is usable within the context manager on line {}, but that "
	"will close before nursery opened on line {} - this is usually a bug. "
	"Nurseries should generally be the inner-most context manager."

Not super happy with this, but I think it gets at the core problem pretty directly. (message also requires some changes to the construction of the error, but dropping one and rearranging isn't too hard.)

[jakkdl]

No need to drop/rearrange with indexed args, and redundant args getting ignored :P Long term maybe nice to do, but I think I'll leave the redundant arg for now for ease of putting it back.

There's also a question on how wordy one wants the error message to be, I personally prefer messages to be short enough to get shown as a one-liner in a status bar - esp the gist of them.

I'd maybe do something like

variable {2}, from context manager on line {0}, passed to {3} from nursery opened outside of it on line {1}, might get invalidly accessed due to context manager closing before the nursery.

(though not sure about invalidly vs illegally vs ??)
But I'll leave yours as is and let you decide.

[Zac-HD]

Spent a while trying to come up with a message I liked more... and couldn't. Let's ship it!