-
-
Notifications
You must be signed in to change notification settings - Fork 5
Open
Labels
Description
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)
, suggestawait 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 eachawait 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).To pick up a draggable item, press the space bar. While dragging, use the arrow keys to move the item. Press space again to drop the item in its new position, or press escape to cancel.
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?
Metadata
Metadata
Assignees
Labels
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
jakkdl commentedon Nov 29, 2022
Yeah 1 sounds great.
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 autofixZac-HD commentedon Nov 29, 2022
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!
[-]Idea: warn on unnecessary checkpoints added to avoid TRIO107/108[/-][+]Warn on unnecessary checkpoints added to avoid TRIO107/108[/+]jakkdl commentedon Jan 31, 2023
Before tackling unnecessary checkpoints I wanted to finish up the reformat of visitors as per #83, to make
visitor107_108.visit_Try
andvisitor107_108.visit_loop
less of a hellscape - splitting them up intoon_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-functionsThis 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 itAlso 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 ofast
. 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 commentedon Feb 3, 2023
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:flake8
for that? I presume similar, so how much work is that to add?jakkdl commentedon Feb 4, 2023
Moved discussion of libcst to #124
jakkdl commentedon Feb 14, 2023
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 commentedon Feb 14, 2023
Definitely postpone, I'd always try to use this via a janky autofix script, so doing it properly from the start sounds way better.
[-]Warn on unnecessary checkpoints added to avoid TRIO107/108[/-][+]TRIO912: Warn on unnecessary checkpoints added to avoid TRIO910/911[/+][-]TRIO912: Warn on unnecessary checkpoints added to avoid TRIO910/911[/-][+]ASYNC912: Warn on unnecessary checkpoints added to avoid ASYNC910/911[/+]