Clang Static Analyzer should issue a warning on mutual assignment

[ryao]

Here is a minimal test case showing mutual assignment where we do b = a; followed by a = b;:

int main(void) {
        int a = 0;
        int b = a;
        int c = 1;
        a = b;
        return a*b*c;
}

I tested this with clang --analyze -Wall -Werror /tmp/ab.c and clang-tidy /tmp/ab.c --, but saw no warnings.

The statement a = b; is redundant. Unless the variables are marked volatile, there should be no legitimate reason for code to do this. We just found an instance of this in OpenZFS:

https://github.com/openzfs/zfs/pull/14565/files#r1124115812

A quick search revealed that PVS Studio has a check for this:

https://pvs-studio.com/en/docs/warnings/v587/

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[steakhal]

I would say that this rule would be useful, but it shouldn't be implemented in a path-sensitive way (aka. inside the Static Analyzer). So, I think @ymand might have some insights about how to implement such a rule using traditional dataflow techniques.

The example I had in mind looks slightly more complicated than yours, but this demonstrates the obstacles quite well:

void foof(int a) {
  int b = a;
  int c;
  if (coin()) {
    c = a;
  } else {
    c = b;
  }
  a = c; // (here)
  return a;
}

In this case, we would need to prove that all paths leading to here, the value of c is equal to the value of a, hence the assignment (a = c) is redundant.

[ryao]

For an initial rule, just catching trivial instances of this would provide value in itself. We could avoid worrying about transitivity, which your example uses, and if not all paths do this, a complaint probably should not be made.

All of the examples of mutual assignment that PVS Studio has detected have been simple cases, such that I am not sure if they are able to detect more complicated cases. However, they have detected enough simple cases in open source projects to make the case that a check that catches those is a useful check by itself:

https://pvs-studio.com/en/blog/examples/v587/

Maybe Clang Tidy or Clang itself could issue a warning about this. It was not clear to me which tool is more appropriate for any given check, so I just filed the idea against the static analyzer.

[steakhal]

Makes sense to explore what could be done using AST matches. However, IDK where to report that, unless of course if you want to make a feature request at SonarSource :D