Add RUF005 "unpack instead of concatenating" check

[akx]

This PR adds a new check that turns expressions such as [1, 2, 3] + foo into [1, 2, 3, *foo], since the latter is easier to read and faster:

~ $ python3.11 -m timeit -s 'b = [6, 5, 4]' '[1, 2, 3] + b'
5000000 loops, best of 5: 81.4 nsec per loop
~ $ python3.11 -m timeit -s 'b = [6, 5, 4]' '[1, 2, 3, *b]'
5000000 loops, best of 5: 66.2 nsec per loop

However there's a couple of gotchas:

• This felt like a simplify rule, so I borrowed an unused SIM code even if the upstream flake8-simplify doesn't do this transform. If it should be assigned some other code, let me know 😄
• More importantly this transform could be unsafe if the other operand of the + operation has overridden __add__ to do something else. What's the ruff policy around potentially unsafe operations? (I think some of the suggestions other ported rules give could be semantically different from the original code, but I'm not sure.)
• I'm not a very established Rustacean, so there's no doubt my code isn't quite idiomatic. (For instance, is there a neater way to write that four-way match statement?)

Thanks for ruff, by the way! :)

[charliermarsh]

Thank you for this contribution! It looks great. I'll look to add comments soon.

The main thing I'm trying to decide is whether this should be in SIM, or RUF. I'm not sure yet. Historically, we've avoided adding checks to the "ported" plugins even if they make sense to include conceptually, instead opting to put novel rules in RUF. The thinking is that the use of SIM is mostly about compatibility -- it lets users that are already using flake8-simplify adopt Ruff and enable SIM without any additional work. (The other argument would be that enabling SIM is a sign that you want simplification checks, which I may not be explaining well, but is subtly different.) So in that light, it'd be surprising if you enabled SIM and saw new checks that didn't exist "upstream".

In the future (not certain when), we'll likely recategorize the rules under more meaningful categories (like complexity -- see the initial proposal in #1774), while retaining this compatibility layer (so the flake8-simplify checks would continue to exist under flake8-simplify in some form, but also as part of a broader complexity category). At that point, the above issue wouldn't matter much, since anyone that enables the complexity category would get this rule regardless.

My gut is to keep this in RUF, or perhaps even introduce a new category along the lines of "experimental" or "nursery" (#1774) for now.

What do you think? Would love to hear your reactions.

[akx]

Hi @charliermarsh, thanks for the kind words!

I'm absolutely more than fine moving this to RUF, or even some sort of RUX (for eXperimental or, um, possiblyXunsafe) category; I felt a bit bad shoving this under flake8-simplify's namespace to begin with anyway.

Beyond the categorization effort in #1774, maybe it might be worth it to be able to mark checks (like this) separately as unsafe or aggressive because you'll have to think a bit before blindly applying the suggestion – e.g. you'd have to opt-in to --fix-aggressive?

As an aside, I fixed the cargo fmt issue that appeared in the checks (and added it as a pre-commit check in #1968...)

[andersk]

Beyond the categorization effort in #1774, maybe it might be worth it to be able to mark checks (like this) separately as unsafe or aggressive because you'll have to think a bit before blindly applying the suggestion – e.g. you'd have to opt-in to --fix-aggressive?

Some prior art, if only for the amusement value: __CARGO_FIX_YOLO=1

[charliermarsh]

Beyond the categorization effort in #1774, maybe it might be worth it to be able to mark checks (like this) separately as unsafe or aggressive because you'll have to think a bit before blindly applying the suggestion – e.g. you'd have to opt-in to --fix-aggressive?

One way we could model this is that we could omit this from the fixable list by default. (We already support turning autofix on and off on a per-rule basis: https://github.com/charliermarsh/ruff#unfixable.) Right now, fixable, by default, includes every rule. But we could instead start to omit some rules from fixable, and thus require users to opt-in explicitly. I'm not sure how we'd message this to users though.

[bluetech]

From @andersk's link I saw that rustc/clippy assigns "applicability" to suggestions: https://doc.rust-lang.org/stable/nightly-rustc/rustc_errors/enum.Applicability.html

[charliermarsh]

@akx - Let's go ahead and move this under RUF for now.

@bluetech - Applicability looks nice and is something we should support (defined at the Rule level, configurable at the command-line and in pyproject.toml).

\cc @not-my-profile who's probably interested in that too.

[akx]

@charliermarsh Moved to RUF005. I also added a commented-out "failing test" in the fixture – chained concatenations aren't handled gracefully at all at present, but I suppose that could be improved later :)

[not-my-profile]

I agree about rule applicability being something we should have ... I had already noticed it when looking at the clippy lint documentation ... I just opened #1997 for tracking that :)

[charliermarsh]

Great, I'll look to get this merged today!

[akx]

@charliermarsh Thanks for the review!

I think this looks pretty good now:

 class Fun:
     words = ("how", "fun!")

     def yay(self):
         return self.words

 yay = Fun().yay
 
 foo = [4, 5, 6]
-bar = [1, 2, 3] + foo
+bar = [1, 2, 3, *foo]
 zoob = tuple(bar)
-quux = (7, 8, 9) + zoob
-spam = quux + (10, 11, 12)
+quux = (7, 8, 9, *zoob)
+spam = (*quux, 10, 11, 12)
 spom = list(spam)
-eggs = spom + [13, 14, 15]
-elatement = ("we all say", ) + yay()
-excitement = ("we all think", ) + Fun().yay()
-astonishment = ("we all feel", ) + Fun.words
+eggs = [*spom, 13, 14, 15]
+elatement = ("we all say", *yay())
+excitement = ("we all think", *Fun().yay())
+astonishment = ("we all feel", *Fun.words)
 
-chain = ['a', 'b', 'c'] + eggs + list(('yes', 'no', 'pants') + zoob)
+chain = ["a", "b", "c", *eggs, *list(("yes", "no", "pants", *zoob))]

[rhkleijn]

An interesting edge case to test would be empty tuples. Does it handle the following correctly?

baz = () + zoob

Unpacking as (*zoob) would be invalid here, I guess. It would need a trailing comma (*zoob,).

[akx]

@rhkleijn I'm a bit surprised myself, but it does the right thing!

-baz = () + zoob
+baz = (*zoob,)

[charliermarsh]

@akx - How would you feel if I subsequently turned off autofix (by default) on this rule for now, instead requiring it to be opt-in (via the fixable setting in pyproject.toml)? I'm not completely decided either way, but I wanted to run it by you before I made that change.

[charliermarsh]

(Merging regardless but may follow-up to tweak the default fix list -- LMK what you think!)

[akx]

@charliermarsh Sounds good to me! Thanks for the merge :)