[PATCH] D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 16 07:46:56 PST 2020
whisperity added a comment.
Herald added a subscriber: shchenz.
In D69560#2319340 <https://reviews.llvm.org/D69560#2319340>, @aaron.ballman wrote:
> Congrats on the SCAM acceptance, I hope the presentation goes well!
The entire event was amazing, even if short. There was another paper which seemed to target the issue of swaps at call sites (D20689 <https://reviews.llvm.org/D20689>) in the section where I was presenting and due to the SCAM being small and having no parallel session, I can say with confidence, that it was this particular section where the longest discussion was spawned from the topics we discussed. (In every other section people left the call a bit too early. The worst problem of online conferencing... 😦)
----
I've gone over the reports on some projects one by one. Due to the vast number of reports on existing projects, I've only selected //3// of them: Bitcoin (C++), Postgres (C), and Apache Xerces (C++). I've specifically wanted to add a C project, however, Postgres produced so many reports across the configurations (1460, 3033, 2415 and 4253, respectively) that it would take multiple inhumane months to go over them one by one... So, let's focus on Bitcoin and Xerces. Reasonably sized projects both being roughly a single library, they are both written to the OOP paradigm and are of reasonably modern C++.
I've run 4 analyses of each project:
- normal mode (named simply `len2` in the uploaded database, only strict type equality is reported)
- normal mode, with relatedness heuristics filtering (`len2-rel`, D78652 <https://reviews.llvm.org/D78652>, this should be the strictest mode and produce the least number of results. **This mode is //most// conformant in matching the Guideline rule!**)
- CVR+Imp mode (`len2-cvr-imp`, type equality is relatex to type convertibility, which generally means an adjacency of `int, const int` (CVR) and `int, double` (Imp) is reported too. CVR is implemented in this patch, Imp is added in a subsequent patch, D75041 <https://reviews.llvm.org/D75041>. This should be the broadest configuration.)
- CVR+Imp mode, with relatedness filtering (`len2-cvr-imp-rel`)
The number of reports were, in this order of configurations:
- Bitcoin: 183, 82, 516, 259
- Postgres: 3033, 1460, 4253, 2415
- Xerces: 254, 79, 412, 165
In CodeChecker, I have marked cases where simple refactorings (strong typedef or a range-constrained type) could help remove the issue as **Confirmed bug**. **False positive**s are cases that cannot be removed by heuristics easily, while **Intentional** bugs (this is a classification category in CodeChecker you can select for your bug, the naming is wonky here but let's just consider it a third bucket) were the ones where sensible heuristics can be devised to suppress these reports. We'll get back to this later. I've checked the results both with "report uniqueing" (1) on and off.
----
BitCoin
=======
| //Project "component"// | Confirmed cases (unique) | False positive (unique) | Suppressable (unique) |
| Qt MOC generated code | | | 8 |
| Third-party library hosted in repository | 13 | 28 (27) | 11 |
| Test code | 34 | 20 | 12 |
| Project core | 136 | 137 | 55 (53) |
| **Total** | 183 | 185 (184) | 86 (84) |
|
These were the numbers for the "normal mode". Now, let's see how it changes when relatedness filtering (as in D78652 <https://reviews.llvm.org/D78652> Diff 259231 <http://reviews.llvm.org/D78652?id=259321>) is introduced.
| //Project "component"// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| Qt MOC generated code | | | |
| Third-party library hosted in repository | 5 | 7 | 4 |
| Test code | 15 | 11 | 2 |
| Project core | 62 | 37 | 14 |
| **Total** | 82 | 55 | 20 |
|
(Numbers did not change with enabling "unique reports".)
Modelling implicit conversions blows up the result set. Note that for this analysis, the definition behind `confirmed bug` is changed a bit: in case of reports where implicit conversions are reported, if the swap through the implicit conversion //is dangerous// it is reported as a confirmed bug. (In many cases, the implicit conversions stem from `f(int, double)` functions... where the introduction of a semantic typedef or constrained integer type with no implicit conversions would fix the issue).
The results for `cvr-imp` are:
| //Project "component"// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| Qt MOC generated code | 136 | | 11 |
| Third-party library hosted in repository | 26 | 29 (28) | 11 |
| Test code | 56 | 11 | 14 |
| Project core | 298 | 138 | 63 (61) |
| **Total** | 516 | 178 (177) | 99 (97) |
|
Now, enabling relatedness filtering over these results, gives us:
| //Project "component"// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| Qt MOC generated code | 68 | | |
| Third-party library hosted in repository | 11 | 7 | 4 |
| Test code | 31 | 10 | 3 |
| Project core | 149 | 42 | 19 |
| **Total** | 259 | 59 | 26 |
|
(Numbers did not change with enabling "unique reports".)
----
Xerces
======
I'll use the same order in the reports. In Xerces, building the tests are not part of building the project itself. This is why the test files were not logged and thus were not analysed. In addition, Xerces contains no well-identifiable third-party libraries living in-tree, so the tables below are shorter.
In normal mode:
| //Bug kind// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| **Total** | 254 (246) | 149 (144) | 101 (95) |
|
With relatedness filtering:
| //Bug kind// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| **Total** | 79 | 61 (59) | 53 |
|
In broad matching (CVR-Imp) mode:
| //Bug kind// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| **Total** | 412 (380) | 158 (154) | 135 (125) |
|
WIth relatedness filtering:
| //Bug kind// | Confirmed cases (Unique) | False positive (Unique) | Suppressable (unique) |
| **Total** | 165 (159) | 55 | 87 (82) |
|
----
I've attached the result database: F14738797: AdjacentParams.sqlite <https://reviews.llvm.org/F14738797>
So what heuristics can we still put in? While the check is about parameter **types** and not **names**, I think we can still (perhaps hidden behind an option) rely on //some// properites of the parameter name to make decisions. One such is that reports involving otherwise unnamed parameters should be squelched. The other is the ignoring of parameters which have a "patterned name" (i.e. `text1, text2, text3, text4` (live example from Xerces!)), as mentioned in [Rice2017].
In addition, I think it would be useful to make certain type //names// ignored and this list of names configured by the user. `bool` is the most striking candidate that comes to mind... you can't really do anything with an `f(bool a, bool b)`, at least nothing that would //improve// the safety and readability of the code. Strong typedefs over `bool` could exist (this was proposed before for LLVM itself, see this llvm-dev post <http://lists.llvm.org/pipermail/llvm-dev/2019-August/134302.html> and D66148 <https://reviews.llvm.org/D66148>), but having a -- to paraphrase my own paper -- `f(ShouldFlip flip, ShouldStretch stretch)` is absolutely bonkers. By throwing out some types ahead of time, we can lessen the number of crappy reports and noise.
In addition, I plan to investigate whether reporting one-way implicit conversions is useful in the first place. I believe it would be beneficial to put the reporting of that behind a distinct option (i.e. "ReportOneWayImplicit" and "ReportTwoWayImplicit" toggling separate things!) and leaving it off by default. Far too many silly cases of only one way implicitness (such as `*` or `&` upcasts, where you can't //really// call an `f(B* b, D* d)` with `f(d, b)` anyways...) producing results.
Perhaps it would also be good to ignore all binary operators, and call operators of functors that are equality predicates, comparators, etc. Some of these cases are yanked by the relatedness heuristics already, but not all...
----
In addition to all this, and tracking back to the talk about default configuration... @aaron.ballman, I have some questions. I've seen that Tidy now supports "check aliases". Do you think moving forward in a way that we rename this check and put it into another group (maybe calling it `bugprone-adjacent-parameters-of-same-type`) with //more sensible defaults// and offering a version of the check under `cppcoreguidelines-` with the defaults more matching the guideline rule would be a good way forward, eventually? Implicit conversions seem to be a real edge of the analysis (hence why I pinned my paper about that problem, also, this was the novelty, the rest of the mixability analysis was done decades ago), and seem to hurt more than simple type equality. However, the guideline is cautious about only warning for same type, and considers `const T*` and `T*` already distinct.
----
//(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.
//[Rice2017]//: Andrew Rice, et al.: Detecting argument selection defects. In: Proceedings of the ACM on Programming Languages, **1**, pp. 104:1-104:23, 2017.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69560/new/
https://reviews.llvm.org/D69560
More information about the cfe-commits
mailing list