Add custom rewrites, using e.g. `libcst`

[Zac-HD]

• [x] Split up assert a and b into two separate assertions, for easier debugging. Preserve comments.
• [x] Break heavily-annotated function arguments over multiple lines, like https://github.com/Zac-HD/shed/issues/22#issuecomment-1073072395 - this should be in a separate PR so I can evaluate the heuristic on some real large code corpora
• [x] NoRedundantLambdaRule (impl): "de-proxy" patterns like lambda x: whatever(x) to just whatever.
  • [x] special case for literals: pie807: prefer list builtin on [], plus {}, ().
• [ ] pie801: prefer simple return
• [x] In sorted(list(...)), remove the inner call to list() (etc; see flake8-comprehensions for more)
  • [ ] likewise in "...".join(list(...)), remove the call to list() (or tuple())
  • [ ] ... and similarly in all(), dict(), sum(), min(), and max()
• [x] pie787: no len condition
• [x] pie788: no bool condition
• [x] raise NotImplemented should be raise NotImplementedError
• [x] Remove redundant parens directly around a function call, or around the argument to a function
• [x] Replace assert False with raise AssertionError, handling the optional arg
• [x] Reorder Union and Literal contents so that None is last
• [x] Flatten syntatically nested Union or Literal instances
• [x] Reorder new-style unions using | so that None is last
• [x] Expand convert_none_cmp() to handle True and False too, ala CompareSingletonPrimitivesByIsRule
• [x] Replace builtin calls with literals: we do this for comprehensions, but should also do e.g. tuple()->(), list("")->[], etc.
• [ ] Remove list() (or tuple) call in comprehensions such as [x**2 for x in list(...)] - we're about to iterate anyway. Only applies to list, set, or dict comprehensions; generator expressions might not consume the whole iterable.
• [ ] Replace map(lambda ..., x) with a comprehension, like https://github.com/adamchainz/flake8-comprehensions/pull/409
  • [ ] And likewise rewrite filter(lambda ..., x)
• [ ] Nontrivial cases of CollapseIsinstanceChecksRule (impl): isinstance(x, y) or isinstance(x, z) -> isinstance(x, (y, z))
• [ ] If the last except ...: clause just does raise, remove it and repeat for the preceeding clause. If it's the only except clause and there's no finally:, remove the try: block entirely.
• [ ] pie797: no unnecessary ternary
• [ ] pie800: no unnecessary spread
• [ ] pie804: no unnecessary dict kwargs
• [ ] Replace sorted(...)[0] with min(...) (or [-1] with max(...)). Invert min/max if passed reverse=True (or truthy constant); ignore reverse=False (or falsey); skip if the reverse= argument is present with any non-constant value.

Variously inspired by linters (flake8, flake8-comprehensions, fixit, etc.) and my own observations of some typed code.

[mcarans]

@Zac-HD Are these rewrites always run or only with --refactor?

[Zac-HD]

So far they're always enabled; --refactor only exists for slow+rare changes and fortunately writing my own rewrite rules means I can pack them into a single pass over the syntax tree. Hopefully it'll stay fast as I add more.

Update: it wasn't as fast as I'd hoped, so all these rewrites are only run in --refactor mode

[Zac-HD]

It would also be nice to autofix some warnings from flake8-pie, in particular PIE787, PIE788, PIE801, and PIE802.

[Zac-HD]

Another pattern I've noticed occasionally is that functions with parametrized-generic-type-annoted args are much easier to read with one-param-per-line. I'm still unsure of the exact cutoff for forcing this, but something like:

• six or more annotated arguments, always one-per-line
• four or more arguments annotated with parametrized generics like list[int]
  • three args like list[list[int]], or two like list[list[list[int]]]
  • a single argument doesn't need to be split, no matter how complex

There's probably a nice way of aggregating these factors as a score so that it works nicely in mixed-annotation-depth situations, but I'm happy to look at that when developing a prototype. dict[int, str] might also count for slightly more complexity, as might tuple[...]?

[Zac-HD]

Other rewrites (e.g. removing unused imports) can leave empty blocks like if True: pass lying around. It would be nice to detect and remove them.

[Cheukting]

@Zac-HD I want to try Reorder new-style unions using | so that None is last. By the way, is Flatten syntatically nested Union or Literal instances covered by #37 already?

[Zac-HD]

Go for it, and no, it's not 🙂

[Cheukting]

I think the Reorder new-style unions using | so that None is last case is done with #39 I will then try Flatten syntactically nested Union or Literal instances then

[Cheukting]

@Zac-HD I guess this it the nested Union example: Union[int, Optional[str], bool] # Union[int, str, bool, None]

and this is the nested Literal example: Literal[1, 2] | None # Literal[1, 2, None]

As these are the 2 in the example that is not covered yet?

Correct me if I am wrong, then I will start playing with it.

[Zac-HD]

Yep, that's it 😊

[Cheukting]

I think the last task is not ready till 2023?

[Zac-HD]

Correct, though if you really wanted to you could implement it early conditional on int(black.__version__.split(".")[0]) >= 23 in a separate PR. Waiting is probably easier though 😜

I've also pulled all the other ideas in comments up into the checklist.

[jakkdl]

request: remove empty type-checking block

if running flake8 with flake8-type-checking, you'll be prompted to move imports only used for type-checking into a type-checking block. If you then refactor to not use that type, autoflake will remove it, and you're left with an empty type-checking block a la

if TYPE_CHECKING:
    pass

this can clearly be removed without any side effects.

additional luxury request: automatically add imports for type-checking types (possibly in a type-checking block).

Doing it for all imports seems dangerous, but a whitelist for the types in collections etc, when one of those types is found in a type comment but isn't imported, should be fairly safe. This would require two passes though, once to get the types used, and once to modify imports.

[Zac-HD]

I'd be happy to remove any if-statement where the test is a name and the body is pass - this can't do anything except (rarely) raise a NameError.

Luxury request: https://libcst.readthedocs.io/en/latest/codemods.html#library-of-transforms provides some useful tools for this. Modifying imports shouldn't take a full second pass though, that can be a one-shot modification to the final tree. I'd be happy to accept a PR for this, though it's pretty low priority.

[jakkdl]

Is there any reason that types in Union/Literal/type annotations only put None last and don't sort completely? I just wrote:

    foo: set[cst.Return | cst.Yield] = ...
    bar: list[cst.Yield | cst.Return] = ...

which is quite irritating. I'm pretty sure there's lots of instances of it in flake8-trio. The logic would probably have to be more complex than just sorting lexically, at least putting None last - but one can also put other builtins earlier/later, as well as differentiate between attributes and names, or bracketed/non-bracketed types.

A related rewrite is literal sets:

a = set(2, 1)

and duplicates in either:

bar: set[int | float | int] = set(1, 1)

[Zac-HD]

Why not sort? => I didn't want to break deliberate orderings, and can imagine this being pretty annoying. (otherwise, yeah, there's a lot we could do there)

Set-literal rewrites would be great, but duplicates are probably better as a lint rule since there's a good chance one element is a typo!

[jakkdl]

might I interest you in supporting # noqa and/or configuration to toggle specific codemods? 😉 lints on duplicates does sound superior, so that's one for flake8-bugbear

[jakkdl]

re pie802: prefer simple any/all - this isn't fully safe since any/all will shortcut but any([... for i in ...]) will exhaust the generator. That is kind of the point of the lint, but I recently noqad PIE802 in flake8-trio (although I've since rewritten that code so can't link it) and if an autofixer silently stopped a generator from exhausting, which has side effects in the generator or in the expression, that could definitely lead to bugs that would be tricky to track down.

[Zac-HD]

@charliermarsh I'm not sure whether you've been following this issue or https://github.com/Zac-HD/shed/blob/master/CODEMODS.md, but I'd be very very happy if I could close it as "ruff does that" 😉

[charliermarsh]

@Zac-HD -- Oh cool! I think I'd need to go through and annotate which of these are supported by Ruff. Would that be helpful? Is it mostly the unchecked items that matter?

[jakkdl]

Oh right, I forgot this issue existed.

@Zac-HD -- Oh cool! I think I'd need to go through and annotate which of these are supported by Ruff. Would that be helpful? Is it mostly the unchecked items that matter?

The checked ones will either be listed in CODEMODS.md, or was removed in #105 because they were covered by ruff, so it's only the unchecked ones that have unknown ruff support.

[Zac-HD]

Unchecked items here are unsupported by shed and may or may not already be in ruff. CODEMODS.md lists several additional fixes which we know are not yet supported by ruff 🙂

[charliermarsh]

PIE801: prefer-simple-return

We support this as SIM103 with a fix, though we only catch one of the two cases in the PIE docs. I made an issue here: https://github.com/astral-sh/ruff/issues/10402.

Nontrivial cases of CollapseIsinstanceChecksRule (impl): isinstance(x, y) or isinstance(x, z) -> isinstance(x, (y, z))

We support this as SIM101 with a fix.

Replace map(lambda ..., x) with a comprehension, like https://github.com/adamchainz/flake8-comprehensions/pull/409

We support this as C417, with a fix.

pie800: no unnecessary spread

We support this exact rule, with a fix.

pie804: no unnecessary dict kwargs

We support this exact rule, with a fix.

The rest we either don't do, or we do some but not all... We don't catch the list comprehension in min, max, or join, but we do catch them in any and all, and we do convert dict([(i, i) for i in range(3)]) to a comprehension.