Switch backend to libCST

[jakkdl]

Moving from #70

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?

Quickly looked at whether the flake8 interface could still work - and it's possible to still use flake8 without much problem. You can indicate that you want the lines of a file and then that can be dumped into libcst.parse_module(). Flake8 does require plugins to accept one of tree, logical_line or physical_line as parameter in __init__ for it to classify the plugin and run it at the correct time, so we'd have to still accept the tree parameter - but we can just throw it out and construct our own tree from the lines sent at the same time.

Somewhat hackish, but means that the consideration of ditching flake8 is pretty much fully independent.

This even means that the transition can even be gradual - where in the intermediate phase you have both an AST and a libCST tree, and run separate runners for them.

TODO

Standalone functionality

1. Color support - I was most of the way to replicating flake8's color printing, but once it got into testing/coverage and windows support I abandoned it and threw it out. For a later PR, if ever.

• Disable visitors with other ways than --enable-visitor-codes-regex add standalone command line args #163
• pre-commit support add libcst as install_requires, print errors to stderr & return 1/0, add pre-commit hook & update config,  #161
• Standalone flake8-async could read config files like flake8 #189
• Add parallell processing of files Switch backend to libCST #124 (comment)
• test that autofixed files don't generate errors test for autofix_files, fix some 91x autofixes, rejig eval_files #159
• first transform, then visit, to not get incorrect line markers #193
  • test
• CLI toggle whether autofix generates an error #192
• test partial enabling of autofixers #191
• Write documentation

Converted to libCST:

• TRIO100 reimplement trio100 & trio101 with libcst #142
• TRIO101 reimplement trio100 & trio101 with libcst #142
• TRIO102
• TRIO103
• TRIO104
• TRIO105
• TRIO106
• TRIO109
• TRIO110
• TRIO111
• TRIO112
• TRIO113
• TRIO114
• TRIO115
• TRIO116
• TRIO117
• TRIO118
• TRIO200
• TRIO210
• TRIO211
• TRIO212
• TRIO220
• TRIO221
• TRIO222
• TRIO230
• TRIO231
• TRIO232
• TRIO240
• TRIO900
• TRIO910 reimplement 91X in libcst #143
• TRIO911 reimplement 91X in libcst #143

Autofixing

• TRIO100: implement autofix for TRIO100 #149
  • don't autofix if there's other warnings about the content of the context. (?)
• TRIO102: it's unsafe to await inside finally: or except BaseException/trio.Cancelled unless you use a shielded
  cancel scope with a timeout.
  • I think a context manager can be added around it?
• TRIO105: Calling a trio async function without immediately awaiting it.
• TRIO112: nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular async function call.
• TRIO113: using nursery.start_soon in __aenter__ doesn't wait for the task to begin. Consider replacing with nursery.start.
• TRIO115: Replace trio.sleep(0) with the more suggestive trio.lowlevel.checkpoint().
• TRIO117: Don't raise or catch trio.[NonBase]MultiError, prefer [exceptiongroup.][Base]ExceptionGroup. Even if Trio still raises MultiError for legacy code, it can be caught with BaseExceptionGroup so it's fully redundant.

user-configured

• TRIO200: User-configured error for blocking sync calls in async functions. Does nothing by default, see trio200-blocking-calls for how to configure it.
  • Could probably add some fancy config flag that will replace stuff. Although the possibly ways of morphing the call might be too complicated to support more than the most basic stuff, will find that out as other 2xx autofixers are written / researched.

I think many of these can be fixed, but needs a long list of what to replace with

• TRIO210: Sync HTTP call in async function, use httpx.AsyncClient.
• TRIO211: Likely sync HTTP call in async function, use httpx.AsyncClient. Looks for urllib3 method calls on pool objects, but only matching on the method signature and not the object.
• TRIO212: Blocking sync HTTP call on httpx object, use httpx.AsyncClient.
• TRIO220: Sync process call in async function, use await nursery.start(trio.run_process, ...).
• TRIO221: Sync process call in async function, use await trio.run_process(...).
• TRIO222: Sync os.* call in async function, wrap in await trio.to_thread.run_sync().
• TRIO230: Sync IO call in async function, use trio.open_file(...).
• TRIO231: Sync IO call in async function, use trio.wrap_file(...).
• TRIO232: Blocking sync call on file object, wrap the file object in trio.wrap_file() to get an async file object.
• TRIO240: Avoid using os.path in async functions, prefer using trio.Path objects.

Requires a bunch of logic

I think I can chuck in a bunch of trio.lowlevel.checkpoint() just before the line that would raise these - but there's going to be a bunch of edge cases that are tricky.
higher priority

• TRIO910: implement autofix for 91x #158
• TRIO911: implement autofix for 91x #158

[jakkdl]

Leaving flake8 as a backend does not really seem worth it at the moment, it's added some headaches and complications at times - but we've managed to work around pretty much all of them so I don't currently see any particular upside in ditching it - unless new versions come out that break things (which isn't that unlikely tbh).
Only thing would be like usability / speed concerns for people that don't want to run it through flake8, or ability to specify version properly in pre-commit.

[Zac-HD]

OK, I like the idea of an incremental migration pathway so let's stick with flake8 for now - we can always revisit after all the substantive logic is converted over.

For autofixes, it'd be nice to hook that from shed in the same way that it already hooks Hypothesis' codemods. I'm OK with saying "lint xor codemod" for any particular check, which is all I'd use, though ideally we could "lint all and codemod some" - not sure how that would be implemented though.

[jakkdl]

I'll have to have a look at hooking in codemods, feels like lint all and codemod some shouldn't be too tricky - but that's a future problem.

I'm realizing now that I don't know how flake8 will react to having the file changed under it - probably badly. A temporary solution could be to write the modified file to {old_filename}_modified.py or placed in a separate directory or something, but that's probably a good enough reason to go ahead with ditching flake8 quite early. One could possibly still have a flake8-hook so it can be run without codemods through flake8 for those that want it. Will have to research the alternatives.

[jakkdl]

I went through the list of checks and categorized them as follows.

Can be autofixed

• TRIO100: a with trio.fail_after(...): or with trio.move_on_after(...):
  context does not contain any await statements. This makes it pointless, as
  the timeout can only be triggered by a checkpoint.
  • just remove the context, unless there's other warnings about the content of the context.
• TRIO101: yield inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling.
  • I think a context manager can be added around the yield?
  • nope, no way to make this safe. (on further thought seems to duplicate TRIO900 🤔)
• TRIO102: it's unsafe to await inside finally: or except BaseException/trio.Cancelled unless you use a shielded
  cancel scope with a timeout.
  • I think a context manager can be added around it?
• TRIO103: except BaseException, except trio.Cancelled or a bare except: with a code path that doesn't re-raise. If you don't want to re-raise BaseException, add a separate handler for trio.Cancelled before.
  • A separate handler for trio.Cancelled can be added before it.
• TRIO105: Calling a trio async function without immediately awaiting it.
• TRIO106: trio must be imported with import trio for the linter to work.
  • I suppose one could do a search&replace on all instances of calls to the imported functions to add trio. before them - but I think support for this will be added soon.
• TRIO111: Variable from context manager opened inside nursery, passed to start[_soon] might be invalidly accessed while in use, due to context manager closing before the nursery. This is usually a bug, and nurseries should generally be the inner-most context manager.
  • Can switcheroo the context managers
  • automatically swapping order of context managers is too dangerous for my taste; better to alert - and perhaps educate - the user, who can then decide what to do about it.
• TRIO112: nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular function call.
• TRIO113: using nursery.start_soon in __aenter__ doesn't wait for the task to begin. Consider replacing with nursery.start.
• TRIO115: Replace trio.sleep(0) with the more suggestive trio.lowlevel.checkpoint().
• TRIO116: trio.sleep() with >24 hour interval should usually betrio.sleep_forever().
• TRIO117: Don't raise or catch trio.[NonBase]MultiError, prefer [exceptiongroup.]BaseExceptionGroup. Even if Trio still raises MultiError for legacy code, it can be caught with BaseExceptionGroup so it's fully redundant.
• TRIO118: Don't assign the value of anyio.get_cancelled_exc_class() to a variable, since that breaks linter checks and multi-backend programs.
  • feels like it'll break code a non-trivial fraction of cases if we remove the variable and then replace instances of the variable with anyio.get_..., but doable (making sure one keeps track of variable scopes).

Cannot be autofixed

• TRIO104: Cancelled and BaseException must be re-raised - when a user tries to return or raise a different exception.
  • logic error that requires manual intervention.
• TRIO109: Async function definition with a timeout parameter - use trio.[fail/move_on]_[after/at] instead
  • think it needs to be fixed at the caller location?
• TRIO110: while <condition>: await trio.sleep() should be replaced by a trio.Event.
  • Maybe some particular conditions can be autofixed, but requires changes in several locations.
• TRIO114: Startable function (i.e. has a task_status keyword parameter) not in --startable-in-context-manager parameter list, please add it so TRIO113 can catch errors when using it.
  • don't think we wanna autofix the config file, although could end the run with suggested new line.
• TRIO900: Async generator without @asynccontextmanager not allowed.
  • can the decorator just be added??
  • nothing we can do here, needs a pretty deep redesign

user-configured

• TRIO200: User-configured error for blocking sync calls in async functions. Does nothing by default, see trio200-blocking-calls for how to configure it.
  • Could probably add some fancy config flag that will replace stuff. Although the possibly ways of morphing the call might be too complicated to support more than the most basic stuff, will find that out as other 2xx autofixers are written / researched.

I think many of these can be fixed, but needs a long list of what to replace with

• TRIO210: Sync HTTP call in async function, use httpx.AsyncClient.
• TRIO211: Likely sync HTTP call in async function, use httpx.AsyncClient. Looks for urllib3 method calls on pool objects, but only matching on the method signature and not the object.
• TRIO212: Blocking sync HTTP call on httpx object, use httpx.AsyncClient.
• TRIO220: Sync process call in async function, use await nursery.start(trio.run_process, ...).
• TRIO221: Sync process call in async function, use await trio.run_process(...).
• TRIO222: Sync os.* call in async function, wrap in await trio.to_thread.run_sync().
• TRIO230: Sync IO call in async function, use trio.open_file(...).
• TRIO231: Sync IO call in async function, use trio.wrap_file(...).
• TRIO232: Blocking sync call on file object, wrap the file object in trio.wrap_file() to get an async file object.
• TRIO240: Avoid using os.path in async functions, prefer using trio.Path objects.

Requires a bunch of logic

I think I can chuck in a bunch of trio.lowlevel.checkpoint() just before the line that would raise these - but there's going to be a bunch of edge cases that are tricky.

• TRIO910: exit or return from async function with no guaranteed checkpoint or exception since function definition.
• TRIO911: exit, yield or return from async iterable with no guaranteed checkpoint since possible function entry (yield or function definition)

[Zac-HD]

I'm realizing now that I don't know how flake8 will react to having the file changed under it - probably badly. A temporary solution could be to write the modified file to {old_filename}_modified.py or placed in a separate directory or something, but that's probably a good enough reason to go ahead with ditching flake8 quite early. One could possibly still have a flake8-hook so it can be run without codemods through flake8 for those that want it. Will have to research the alternatives.

I was thinking that we'd have the Codemod class return both the modified cst, and a list of lint errors which includes whether they're fixed in the new cst. Then we can have flake8 report all lint errors, a pure-autoformat tool (eg shed integration) just fix the autofixable things, and a future standalone CLI write autofixes and report the non-fixed lint errors.

[Zac-HD]

Re: your notes on specific rules, mostly looks great. Few edits:

• TRIO101: nope, no way to make this safe. (on further thought seems to duplicate TRIO900 🤔)
• TRIO111: automatically swapping order of context managers is too dangerous for my taste; better to alert - and perhaps educate - the user, who can then decide what to do about it.
• I wouldn't try to autofix TRIO116 or TRIO118 either
• TRIO900: nothing we can do here, needs a pretty deep redesign

[Zac-HD]

• TRIO112: nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular function call.

Ah, but not if the nursery is passed strict_exception_groups=, because that would change exception types.

[jakkdl]

Working on libcst now and will probably push something up today/tomorrow.

I was thinking that we'd have the Codemod class return both the modified cst, and a list of lint errors which includes whether they're fixed in the new cst. Then we can have flake8 report all lint errors, a pure-autoformat tool (eg shed integration) just fix the autofixable things, and a future standalone CLI write autofixes and report the non-fixed lint errors.

I thought this sounded good, but you're going to end up with two trees, the unmodified and the transformed one, which at the very least are going to desync in line numbers - but possibly in even bigger ways. With autofixing turned on, you want the line numbers from the transformed file in the output, with it off you'll want line numbers from the original tree.
So will at least have to pass a flag to the class/program telling it whether/which codes should be autofixed.

[jakkdl]

To get started on autofixing, I think I might as well get it running as an independent plugin at the same time so it's usable. Looked through how various linters are run, and the one in shed looked simple enough to get started with. So any comments on that before I get started copying much of the implementation of it? https://github.com/Zac-HD/shed/blob/master/src/shed/_cli.py

I'll be adding a couple flags only valid when run independently (which will error out and tell the user to run it without flake8 if passed with that), but try to otherwise keep a very similar interface regardless of if it's run with or without flake8.

[Zac-HD]

That sounds good to me! Obviously the already-converted checks are the best place to start autofixing, but then missing/redundant checkpoints would be highest priority as they're a pita to do manually.

[jakkdl]

That sounds good to me! Obviously the already-converted checks are the best place to start autofixing, but then missing/redundant checkpoints would be highest priority as they're a pita to do manually.

Got started on converting 91X today~