[clang-tidy] suggest replacing chained assignments with a sequence of individual assignments

[LegalizeAdulthood]

Suppose we have code like:

// ...
baz = foo = goink = 0;

Because the assignments are all daisy-chained together, it makes it difficult for automated refactoring tools to do things like join a variable's declaration with it's first assignment, localize a variable to it's first use, detect redundant assignments and so-on. Additional analysis could be built into these existing tools, or a simpler clang-tidy replacement can be applied first to enable other tools.

Write a clang-tidy check that splits this chained assignment into a sequence of individual assignments:

// ...
goink = 0;
foo = 0;
baz = 0;

Disable any such fixit if the variable is of a type that has a custom operator= as that might introduce additional side-effects that are not easily detected resulting in broken code should the chained assignment be split into a sequence of individual assignments.

Note that the assignments are emitted starting from the right-most variable in the chain, as the compound assignment baz = foo = goink = 0; is equivalent to baz = (foo = (goink = 0)); due to the associativity for = being right-to-left, so the sequence of individual assignments should proceed from right-to-left.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[LegalizeAdulthood]

Not sure which clang-tidy module would be best for this check. Readability?

[PiotrZSL]

yes readability, i could implement this, but only if you would review it. otherwise is waste of time.

[firewave]

I think you also need to consider cases like a = b = std::move(c).

And what about cases where the types are not all the same?

Not sure which clang-tidy module would be best for this check. Readability?

Seems natural as there's already a similar check - see https://clang.llvm.org/extra/clang-tidy/checks/readability/isolate-declaration.html.

[PiotrZSL]

so safest way is to unwind:

a = b = std::move(c).

into 
b = std::move(c);
a = b;

and in case of:

baz = foo = goink = 0;
into
goink = 0;
foo = goink;
baz = foo;

and let compiler optimize this.

[LegalizeAdulthood]

@firewave I would skip anything that has move assignments. My motivation for this check comes from modernizing some 30 yr old code recently.

If you unwind baz = foo = goink = 0; into coupled assignments goink = 0; foo = goink; baz = foo;, then you haven't improved the situation w.r.t. localizing variables and decoupling the code, which is the whole point of this check. The assignments have to be determined to be side-effect free (hence my comment about skipping when a user-defined operator is involved). I hadn't considered side-effects of std::move, so that is another case that would cause this check to not suggest any fixits, but maybe issue a warning to let you apply manual changes. (Maybe emit the warning only through an option that is off by default.)

[LegalizeAdulthood]

yes readability, i could implement this, but only if you would review it. otherwise is waste of time.

I generally file issues for checks I will implement myself, but if other people want to implement them I generally don't complain. I can't promise bandwidth to review, however. And honestly, phabricator is my nemesis. I can barely find things in there and the default queries either return no results or too many irrelevant results.

[PiotrZSL]

still you can have:

bool test()
{
    float a;
    int b;
    float c;

    a = b = c = 1.5f;
    return a == 1.0f;
}

so this only make sense if there are same types involved. still this could be done.

[firewave]

baz = foo = goink = 0;
into
goink = 0;
foo = goink;
baz = foo;

This results in less readable IMO - maybe it simply shouldn't provide a fix-it as suggested for custom operator= cases.

My motivation for this check comes from modernizing some 30 yr old code recently.

Regarding such code another check comes to mind: suggest to move the variable declaration to the first actual usage. It seems to apply to some parts of the code. Not to the amount of old C code but still.

[PiotrZSL]

generally file issues for checks I will implement myself

ok, sure, not problem, i got a backlog...

[LegalizeAdulthood]

Regarding such code another check comes to mind: suggest to move the variable declaration to the first actual usage. It seems to apply to some parts of the code. Not to the amount of old C code but still.

ReSharper for C++ does this already, even if clang-tidy doesn't. For clang-tidy additions, I look to things that my IDE doesn't already know, because once I teach clang-tidy to do it, it will show up in my IDE via clang-tidy fixit support.

[PiotrZSL]

Regarding such code another check comes to mind: suggest to move the variable declaration to the first actual usage. It seems to apply to some parts of the code. Not to the amount of old C code but still.

Writing this in form a warning, is easy, providing fixes may not always be easy... I have this in my backlog, already half implemented but as performance checks, to find heavy variables that won't by used due to early exist in some if..

[firewave]

And honestly, phabricator is my nemesis. I can barely find things in there and the default queries either return no results or too many irrelevant results.

I just search for "tidy" when I start peeping around to get a taste of what might be upcoming. There's the "clang-tools-extra" tag as well.

ReSharper for C++ does this already, even if clang-tidy doesn't. For clang-tidy additions, I look to things that my IDE doesn't already know, because once I teach clang-tidy to do it, it will show up in my IDE via clang-tidy fixit support.

Booooo. 😉 I don't use that although I use CLion. variableScope in Cppcheck only works with scopes so I would like to see that in a command-line tool for scope-less cases as well. So I will file another ticket about it.

[PiotrZSL]

Or create custom query with "[clang-tidy]" as search and order by latest changed. It's very easy..

[LegalizeAdulthood]

Booooo. 😉 I don't use that although I use CLion.

You may already have access to this functionality since CLion and R++ are both JetBrains products. It's been my experience that they maintain pretty good feature parity between the two products. The complete list of R++ code inspections may give you a hint of how to access the corresponding feature in CLion.

[firewave]

The complete list of R++ code inspections may give you a hint of how to access the corresponding feature in CLion.

CLion has the issue that it runs into timeouts with the data-flow analysis (DFA) on a lot of (even very small and/or simple) files so I don't see a lot of the warnings I should see. It's an extremely long-standing problem. But we should stop getting off-topic though.