[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a reviewer: aaron.ballman.
whisperity added a comment.

Right, let's bump. I've checked the output of the checker on a set of test projects (our usual testbed, I should say) which contains around 15 projects, half of which was C and the other half C++. All projects were analysed independently in a virtual machine. All projects were analysed after checking out the //latest release tag// that was closest to Jan 2019. (I know this sounds dated, but the whole reproduction image was originally constructed for another checker I'm working on, and I did not wish to re-do the whole //"What is needed to install this particular version?"// pipeline.) The more important thing is that //releases// were analysed, which should, in theory, under-approximate the findings because releases are generally more polished than in-the-works code. The check was run as-is (sans a minor rebase issue that does not affect functionality) currently in the patch, namely, Diff 259508 <http://reviews.llvm.org/D20689?id=259508>.

In total, I had received **194 reports** (129 unique (1)). From this, I had found **8** (7 unique) **true positives** (marked //Confirmed bug// in CodeChecker terms), where something is visibly wrong with the call.
>From this, there were 3 in a project where the called function's //comment// said that //"The order of arguments <> and <> does not matter."//, however, because this can never be figured out totally from the checker, I regarded the swapped call as a true positive. The fact that they had to seemingly create, inside the called function, some logic to detect and handle the swap shows that something was going wrong with the code's design for a long time.

In addition to these findings, I have identified **122** (75 unique) function call sites where the report about the potential swap (and the similarity to a //different// parameter name) is justifiable because the **understandability** of the code (especially to someone who is an outsider from virtually all of the projects analysed (2)) **is hindered** by the poor choice of argument or parameter names. The conclusions from these cases (marked //Intentional// in the CodeChecker database) are consistent with those drawn in [Pradel2013] and [Liu2016].

Now, onto the **false positives**. There were **64** (47 unique) cases. However, these cases can be further broken down into different categories, which I wasn't able to tally easily as CodeChecker only supports 3 unique categories, not infinite ones.

- Some of the false positives are what one would say "borderline": if the person reading the code reads it accurately, the reported "swap" does not fall into the //understandability issue// category. However, a famous case of this is from Postgres: `reloid` (IIRC, meaning **rel**ation **o**wner **id**) and `roleid` (**role** (user) **id**) are the names of args/params of some functions. They are not swapped in the calls that exist in the code, but the similarity (swapping only 2 letters) makes it very easy to typo or misread the thing. In Postgres, there were 7 such cases.
- Approximately 5+5 cases are false positives but can be dealt with in heuristics. However, across the 17 projects, they do not account for a sizeable amount of cases.
  - Recursive function calls should be ignored. (This was mentioned in [Rice2017].)
  - Swapped and not-swapped calls appearing close to one another (i.e. constructs like `b ? f(x,y) : f(y,x)`) should be ignored too. This seems a bit harder to implement.
- There is a **bug** in the check's current implementation when calls from/to `operator()` is handled. (3) I'll look into fixing this.
- Binary operators should be ignored from reporting in general. Their parameters tend to have generic names, and the reports created from them is confusing.

Another generic observation is that the check's output is pretty wonky and hard to read at a glance, but this should be easy to fix. In addition, the observation of [Rice2017] about ignoring "patterned names" (i.e. `arg1, arg2, ...`) seems like a useful thing to add to the implementation, even though I had no findings //at all// where "ignoring patterned names" would've squelched a false positive report.

Agreeably, this check is limited compared to the previously linked [Rice2017], as it only checks the names in the call, not all variables in the surrounding context.

----

//(1)//: I'm not exactly sure as to what "report uniqueing" in CodeChecker precisely does these days, but basically it uses the context of the bug and the checker message and whatnot to create a hash for the bug - "unique reports" mode shows each report belonging to the same hash only once.
//(2)//: From the set of test projects, I only have some hands-on experience with parts of LLVM and Apache Xerces.
//(3)//: Codes similar in nature to the following example of exact value forwarding to the call operator seems to still trigger the check. I have yet to actually pin down what causes this.

  struct S
  {
      int operator()(int a, int b, const char* c, double d);
  };
  
  struct T
  {
      S* s;
      int operator(int a, int b, const char* c, double d)
      {
          return (*s)(a, b, c, d);
      }
  };

//[Pradel2013]//: Michael Pradel, and Thomas R. Gross: Name-based analysis of equally typed method arguments. In: IEEE Transactions on Software Engineering, **39**(8), pp. 1127-1143, 2013.
//[Liu2016]//: Hiu Liu, et al.: Nomen est Omen: Exploring and exploiting similarities between argument and parameter names. In: 38th IEEE International Conference on Software Engineering, pp. 1063-1073, 2016.
//[Rice2017]//: Andrew Rice, et al.: Detecting argument selection defects. In: Proceedings of the ACM on Programming Languages, **1**, pp. 104:1-104:23, 2017.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D20689/new/

https://reviews.llvm.org/D20689