Trio101: Never yield inside a nursery or cancel scope

[jakkdl]

Not entirely sure dumping a bunch of yields into a set is the best way to avoid false positives, but other than doing the reverse (saving contextmanagers/functiondefs, or their linenos) I couldn't figure out a neater way of doing it.

Didn't bother trying to juggle out trio100 changes, so you might want to merge the first pull request before looking at this diff.

[Zac-HD]

Will review properly after the other PR is merged, but it looks like a good approach 👍

[Zac-HD]

• Requesting a larger design change sorry, but I think it's worth it.
• It'd also be good to avoid false alarms on @(async)contextmanager-decorated functions, in which cases yielding inside these contexts is OK after all. Plus tests for that case.
• New error codes should be added to the README, with as long a message as is needed to explain what's going on and how to fix it. (for that matter the existing TRIO100 message could use a link or two)

[Zac-HD]

I should also check in explicitly - I rebased the branch for you so I could leave a meaningful review more promptly, motivated mostly by timezones. Happy to go with whatever branch strategy or convention is easier for you though, just let me know.

[jakkdl]

I think it'll mostly be a one-time thing anyway, since once the setup is in place the pull requests will be much more independent.
I usually do the majority of my work in my morning, and for that it doesn't really matter when during your workday you give feedback.

I think I prefer merging over rebasing to more easily see what changed when syncing to my local branches.

[jakkdl]

New error codes should be added to the README, with as long a message as is needed to explain what's going on and how to fix it. (for that matter the existing TRIO100 message could use a link or two)

I can give it a try, but not actually knowing Trio (and it usually not being too relevant for implementing the AST crawling) it'll require me to do a bunch of extra reading to write something educational (and correct). But I can give that a go later on.

[Zac-HD]

I can give it a try, but not actually knowing Trio (and it usually not being too relevant for implementing the AST crawling) it'll require me to do a bunch of extra reading to write something educational (and correct). But I can give that a go later on.

Let's just copy the error message from the code to the README then, and I'll improve the docs when we publish.

[jakkdl]

Fixed. Small Q's:
Want to bother with mentioning inside what scope it is in the error as before? Could replace self._yield_is_error with a str with the scope name.

Unrelated to trio101 in particular, but:
Is TRIO100 really only for fail_after and move_on_after? I extended it to all cancel scopes except nursery for now.

The fuzzer is way slower after your rewrite, idk if it's checking more files or something but I killed it after it ran for 3 minutes w/o completion on my system.

[Zac-HD]

• Ooh, yeah, mentioning which scope specifically would be lovely - ideally also with the line number (e.g. "trio.move_on_after on line 5")
• Extending to all cancel scopes is great 👍
• I made the fuzzer run 100x more examples by default, feel free to turn that back down if it's excessive 😅

[jakkdl]

Ooh, yeah, mentioning which scope specifically would be lovely - ideally also with the line number (e.g. "trio.move_on_after on line 5")

Is there any official way to get markers on different lines in flake8? One of my regular linters (idk if it's pylint or mypy though lol) can put an error marker on one line that refers to another line that gets an info marker. Or we could add two "problems" one pointing to the yield and one pointing to the scope.

Extending to all cancel scopes is great +1

And open_nursery with no await is fine?

I made the fuzzer run 100x more examples by default, feel free to turn that back down if it's excessive sweat_smile

Hmm, I suppose that could be added as a parameter. But yeah lowering it to 1k (10x) tests still took my machine a minute 😇

[jakkdl]

Any opinion on self._yield_is_error, self._context_manager and similar being a stack (list) and push/pop'ing values instead of storing them as "outers" in local variable in functions?

[jakkdl]

Do you want

from trio import open_nursery
def foo9():
    with open_nursery:
        yield 1


import trio as mytrio
def foo10():
    with mytrio.open_nursery:
        yield 1

to be detected?

[Zac-HD]

Ooh, yeah, mentioning which scope specifically would be lovely - ideally also with the line number (e.g. "trio.move_on_after on line 5")

Is there any official way to get markers on different lines in flake8? One of my regular linters (idk if it's pylint or mypy though lol) can put an error marker on one line that refers to another line that gets an info marker. Or we could add two "problems" one pointing to the yield and one pointing to the scope.

I meant to have the marker point at the yield, and just mention the other location in the message like "TRIO101: yield inside {call} (line {item.lineno}) is only safe ....

But let's revisit this later, it can be retrofitted in a later PR.

Extending to all cancel scopes is great +1

And open_nursery with no await is fine?

Yep, you might just want to e.g. call nursery.start_soon(...) a few times and that's a sync method.

Any opinion on self._yield_is_error, self._context_manager and similar being a stack (list) and push/pop'ing values instead of storing them as "outers" in local variable in functions?

Some preference for local variables, because they're a simpler representation and much harder to mess with the representation. If we actually need access to other parts of the stack I'd change my mind.

Do you want [code samples] to be detected?

I'd leave a comment to the effect of "We know that this doesn't handle from trio import *, or import trio as x, but those rare cases are omitted for ease of implementation".

We might later add a lint that complains if you do something fancier than just import trio for the top-level namespace, but that's very low priority - I've never seen it in practice, including at work.

[Zac-HD]

Looks great! Merging now 😁

[jakkdl]

I meant to have the marker point at the yield, and just mention the other location in the message like "TRIO101: yield inside {call} (line {item.lineno}) is only safe ....

Ye I followed, but thought one could go a small step further and save people from having to look up line numbers - but unless there's a neat built-in way of doing it it might be ugly to raise two errors for one problem.