ASYNC912: Warn on unnecessary checkpoints added to avoid ASYNC910/911

[Zac-HD]

I've noticed a few places where await trio.sleep(0) is added in places that don't actually need a checkpoint, and so I've thought up a two-part plan to mitigate cargo-cult copy-paste coding.

• Instead of await trio.sleep(0), suggest await trio.lowlevel.checkpoint(). It's an identical runtime effect, but the name is much more suggestive of what's actually going on: "we need a checkpoint here for some low-level reason" ✅
• Refactoring the increasingly-large plugin file as suggested below.
• In each async function, for each await trio.lowlevel.checkpoint(), check if there would be no 107/108 warnings emitted if that checkpoint was removed (in addition to any already-suggested-removals, of course).

I initially marked this as an "idea" issue rather than for implementation because while I'm fairly confident (1) is a good plan, (2) seems a little harder to implement. It's also triggering my "this feels pretty tedious" detector, but building a libCST-based autofixer also seems like a little too much duplication of effort, right? Even if we could autofix 105, 107, 108, 112, and 113; and hook it in to shed --refactor... tempting but probably not worth it 😕

Thoughts?

[jakkdl]

Yeah 1 sounds great.

2. I don't think would be that hard to implement, given how .. extreme .. the 107/108 implementation is already. self.uncheckpointed_statements would instead of a set be a list sets, and when the code previously emptied it, it can instead push a new empty set on the stack and do some magic logic.
   It's possible the trickier cases with loops is harder and not worth it, if that's the case it seems totally fine not to warn about unnecessary checkpoints in that case.
   I guess it's harder when you combine it with other warnings that tell you to refactor, if not impossible without libCST, but it doesn't seem unreasonable to expect users to ignore errors from that when combined with other ones, or could be more explicit and mention that in the error text or even suppress the error if there are other ones in the vicinity.

But biggest downside is probably not implementation time, but that the class is already 300 lines long and I do not envy anybody trying to understand it (imagine a bug turns up in visit_loop 😱) and adding more functionality to it would certainly not make it easier to maintain.
The file is overall becoming quite nasty, and I've had a little bit of trouble getting back into it after not working on it for a while. It might be time to refactor and split out all checks into separate classes, putting them into a separate file, and rejig so Plugin.run only calls run on the superclass, which then handles visiting subclasses and makes it so we only traverse the tree once & groups error messages by location instead of by type as is done currently.

A libCST fixer for #1 is pretty trivial, #2 sounds kinda nasty. Feels like for that case you write a script that parses the output of flake8 and sed away the unnecessary ones, which you run only once upon fixing the codebase. If you wanted autofix for everything then it feels like this shouldn't be a flake8-plugin, and instead refactor the whole project to use libCST instead. 107/108 feels like it might venture into somewhat dangerous territory to autofix

[Zac-HD]

OK then - let's ship (1), then have a big refactoring pr for clarity and perf with no semantic changes, and after that's in a final pr with the new unnecessary-manual-checkpoint check.

LibCST doesn't seem worth the implementation and maintenance cost sadly, though it's possible that might change eventually. I agree that sed is a compelling alternative for checkpoint removal at least!

[jakkdl]

Before tackling unnecessary checkpoints I wanted to finish up the reformat of visitors as per #83, to make visitor107_108.visit_Try and visitor107_108.visit_loop less of a hellscape - splitting them up into on_visit/on_leave and separate visitor functions for attributes. Spent a while thinking of decent ways of going about it, and happened to check out what libcst does ... and turns out it does exactly what I want! https://libcst.readthedocs.io/en/latest/visitors.html?highlight=leave#visit-and-leave-helper-functions

This is now the second time in a short time I start thinking about implementing functionality (previous one being matchers to replace ugly/unreadable isinstance chains) only to realize that libcst already does it

Also looking around, libcst has https://libcst.readthedocs.io/en/latest/visitors.html?highlight=leave#batched-visitors which is what I spent a bunch of time getting runner.py to do...

So I'm definitely starting to consider the idea of having libcst as a backend instead of ast. I at least will postpone big upgrades to the runner, or when I do it just copy however libcst does it.

I'm not 100% sure on all the pros/cons of switching or whether I ultimately think it's a good idea, but it would at the very least enable easily adding autofixing.

[Zac-HD]

Yeah, based on the possibility of autofixing, this actually seems pretty promising. After we get anyio support shipped and make 107/108 optional, can you write up a proposal for me to look at? Particular questions:

• what does the UX look like if we're not leaning on flake8 for that? I presume similar, so how much work is that to add?
• concrete breakdown of what can be autofixed vs continue to emit lint warnings
• suggested breakdown of tasks into reviewable-sized PRs (e.g. "convert UX, switch over logic, add autofixes")

[jakkdl]

Moved discussion of libcst to #124

[jakkdl]

Is this high prio enough you want it before libcst? I would prefer not to touch the mess that is 91x until afterwards, but I can prioritize converting 91x early for libcst (which is also a good stress test) and implement this soon after that's working.

Tentatively marking it postponed

[Zac-HD]

Definitely postpone, I'd always try to use this via a janky autofix script, so doing it properly from the start sounds way better.