implement autofix for TRIO100 by jakkdl · Pull Request #149 · python-trio/flake8-async

jakkdl · 2023-03-06T17:51:51Z

Before jumping on autofixing TRIO91X I decided to start with the simpler TRIO100.
Turns out I'd forgotten how messy it is to manipulate code and preserve whitespace, comments, lines, etc etc etc 🙃

But I remember solving it in shed, so I can probably crib that from there and write some helper function.
Also remaining is actually changing the file on disk, and adding a flag.

~~shed/Black/isort is also back to being incredibly annoying, I'll have to figure out a better way of writing autofix files so they don't get fucked with as much as is done currently.~~

jakkdl · 2023-03-08T15:23:30Z

It be done!

Very happy with this new autofix testing infrastructure, and I even just now realized that I can exclude autofix files from being shed'd by excluding them in pre-commit ^^

Uses a similar approach to Zac-HD/shed#52 ; and shed would normally codemod away the useless pass statements that are inserted in the autofix files sometimes ... which would cause the autofix tests to fail.
It would be great if I could highly selectively disable pass removal, so shed can check that the autofixed files are otherwise generated with good formatting. fmt: off does nothing.

--generate-autofix maybe deserves a mention in CONTRIBUTING.md - it helps a lot when developing. And ofcourse --autofix should be documented, but it should probably be a comma-separated list anyway.

jakkdl

Some comments to ease reviewing

jakkdl · 2023-03-08T15:25:10Z

flake8_trio/runner.py

@@ -100,20 +100,20 @@ def visit(self, node: ast.AST):


 class Flake8TrioRunner_cst:


the runner is kind of .. not necessary and clunky, esp as currently __init__ does a bunch of logic itself. Might warrant some restructuring.

jakkdl · 2023-03-08T15:25:57Z

flake8_trio/runner.py


-    def run(self, module: Module) -> Iterable[Error]:
+    def run(self) -> Iterable[Error]:
        if not self.visitors:
            return
-        wrapper = cst.MetadataWrapper(module)
        for v in self.visitors:
-            _ = wrapper.visit(v)
+            self.module = cst.MetadataWrapper(self.module).visit(v)
        yield from self.state.problems


now saves and updates self.module after visiting.

jakkdl · 2023-03-08T15:28:19Z

tests/eval_files/trio100.py


 with trio.move_on_after(10):  # error: 5, "trio", "move_on_after"
-    pass
+    ...


replaced pass with ellipsis partly in attempts to avoid shed headaches, but means you can see a difference with ellipsis's transferred over, vs injected passs, in the autofix file.

jakkdl · 2023-03-08T15:29:29Z

tests/autofix_files/trio100.py

@@ -0,0 +1,84 @@
+# type: ignore


there's not a ton to see here ("trio100_simple_autofix.py" is easier to read), but if you want to review it you should view this and tests/eval_files/trio100.py with a difftool of your choice.

jakkdl · 2023-03-08T15:31:38Z

tests/eval_files/trio100_simple_autofix.py

+# extreme case I'm not gonna care about, i.e. one item in the with, but it's multiline.
+# Only these leading comments, and the last one, are kept, the rest are lost.
+with (  # a
+    # b
+    # c
+    trio.move_on_after(  # error: 4, "trio", "move_on_after"
+        # e
+        9999999999999999999999999999999999999999999999999999999  # f
+        # g
+    )  # h
+    # i
+):  # this comment is kept
+    ...


I can maybe detect this and abort the autofix, rather than eating the comments. But I'm not sure if this happens ever? Maybe if you're passing a lot of parameters to one of fail_after/fail_at/move_on_after/CancelScope so it's gotten split across lines by Black.

Fixed, comments are now preserved.

jakkdl · 2023-03-08T15:32:12Z

tests/test_config_and_args.py

+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr(
+        sys, "argv", [tmp_path / "flake8_trio", "--autofix", "./example.py"]
+    )


might deserve a helper function, and/or helper fixture

jakkdl · 2023-03-08T15:34:41Z

tests/test_flake8_trio.py

+def test_assure_all_autofix_files_used():
+    assert set(autofix_files.keys()) - checked_autofix == set()


this one is a minor pain since I usually develop locally with --failed-first, but didn't see any simple way of forcing this to be run late without installing pytest plugins (and that might still get overridden by --failed-first). But should help the occasional messed up file management by making sure all files in autofixed_files are used.
(although ig that could be done by static analysis of directory contents....)

fixed, now statically checking the contents of the file lists.

tox.ini

flake8_trio/visitors/helpers.py

jakkdl · 2023-03-11T03:00:16Z

I realize now I added this comment in #124

just remove the context, unless there's other warnings about the content of the context.

that's ... really really hard to implement properly - at least until everything is converted to libcst. So that will have to wait for a later PR, if it turns out to actually be of use. I was probably mainly thinking of missing awaits - but if I can fix trio typing so type checkers can warn on async function calls not being awaited that should be a non-issue.

Zac-HD

This all looks good to me! Rebase on master and then merge?

Zac-HD · 2023-03-14T05:08:20Z

flake8_trio/visitors/visitor100.py

            for res in self.node_dict[original_node]:
                self.error(res.node, res.base, res.function)
-        # if: autofixing is enabled for this code
-        # then: remove the with and pop out it's body
+
+            if self.options.autofix and len(updated_node.items) == 1:
+                return flatten_preserving_comments(updated_node)


I assume that we don't actually emit the error for autofixed code?

This currently always does, I'll introduce ways of configuring that soon :)

jakkdl · 2023-03-14T10:41:25Z

merged! 🚀

jakkdl force-pushed the 91x_autofix branch from e0fed3e to e7e4a95 Compare March 8, 2023 15:12

jakkdl marked this pull request as ready for review March 8, 2023 15:13

jakkdl force-pushed the 91x_autofix branch from e7e4a95 to e2d230a Compare March 8, 2023 15:20

jakkdl commented Mar 8, 2023

View reviewed changes

jakkdl force-pushed the 91x_autofix branch 2 times, most recently from 7b54bf6 to 522c1db Compare March 9, 2023 12:05

jakkdl commented Mar 9, 2023

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

flake8_trio/visitors/helpers.py Show resolved Hide resolved

jakkdl mentioned this pull request Mar 11, 2023

Switch backend to libCST #124

Open

63 tasks

Zac-HD approved these changes Mar 14, 2023

View reviewed changes

Implement basic autofix infrastructure and autofixer for TRIO100

77c61dd

jakkdl force-pushed the 91x_autofix branch from 522c1db to 77c61dd Compare March 14, 2023 10:40

jakkdl merged commit 76f5363 into python-trio:main Mar 14, 2023

jakkdl deleted the 91x_autofix branch March 14, 2023 10:40

		@@ -100,20 +100,20 @@ def visit(self, node: ast.AST):


		class Flake8TrioRunner_cst:

		def test_assure_all_autofix_files_used():
		assert set(autofix_files.keys()) - checked_autofix == set()

Uh oh!

Conversation

jakkdl commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakkdl commented Mar 8, 2023

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jakkdl commented Mar 11, 2023

Uh oh!

Zac-HD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakkdl commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jakkdl commented Mar 6, 2023 •

edited

Loading

jakkdl commented Mar 14, 2023 •

edited

Loading