[I.24] Adjacent parameters of the same type - issues in implementing proper automated analysis

[whisperity]

Follow-up for #1579, both @vingeldal and I started to implement a check in Clang-Tidy that is aimed to target the I.24 rule. I'm still trying to perfect the implementation and continue drumming...
However, with the maintainer of Clang-Tidy, @AaronBallman, we are at an impasse, not about the implementation, but about the rule itself. I've run extensive testing and hand-evaluation of findings on multiple open-source projects, and one or two internal ones. Together with my colleagues, we devised some rules (now still dubbed relatedness-heuristics, although they'll be renamed soon due to #1579 being fixed) and filterings (follow-up #1288, I've offered in the check a list of banned parameter names and types, including bool). Now the problem is, all these measurements were done on project that do not follow CPPCG (at least not this rule in particular, but most likely not the vast majority of the rules!), so naturally there are a lot of warnings.

I believe that this rule is worth some automated checking for. Aaron suggested me coming to you about improving the rule itself, he believes it's too vague. But the guideline itself

(Simple) Warn if two consecutive parameters have the same type

is waay too noisy. Did you intend this rule to be automatically checked? After #1579, "changes meaning" is impossible to prove in static analysis, but the given exceptions are easy to approximate.

Do you believe coming up with more exception cases in an analysis tool (without explicitly specifying them in the guideline) is worth it?
I'm coming close to a 25% false positive ratio with a few simple heuristics that looks at the code, and also things like "ignore binary operators", or "ignore unnamed parameters", etc.

Some of the results of hand-evaluation are compiled here, in the discussion for the implementation of the checker in particular: https://reviews.llvm.org/D69560#2457961.

[hsutter]

Editors call: Thanks, these numbers are very informative.

Could you suggest a way extend/qualify the Enforcement that would reduce false positives?

We could also add more examples (e.g., qsort perhaps), do you have any to suggest?

[whisperity]

We've come up with the following heuristics, originally dubbed "relatedness heuristics", back when the rule was titled as such. All of these naturally have a chance to introduce false negatives! So all of these break the idea as a "compliance legislation" but try to allow for easing the load on developers when actual warnings and put on their desks.

• Ignore parameters that appear together in a common enough expression. E.g. if you take a pair of iterators and you have something like while (first < last) then do not warn about them. Common enough expression means subexpressions until a statement. I.e. while (first) { foo(last); } is deemed not common enough.
• Do not warn about parameter (pairs) which are returned on different paths of the function.
• Do not warn about parameters that are passed to the same function's same overload on the same index in different calls. (E.g. f(a, b, x); and then later f(a, b, y); makes x and y related.)
• Do not warn about parameters that have the same member accessed within the function (either getting the same data member, or calling the same member function).

We added another heuristic for "patterned names"... i.e. if you have a parameter sequence like arg1, arg2, ... or qmat, rmat, tmat, we decided to offer silencing the warning. There's a lot of existing literature on name-based tinkering for bogus function call detection (one of the newest and most comprehensive paper being Andrew Rice, et al.: Detecting Argument Selection Defects), with rather interesting findings. I view I.24 as the preventive sibling rule to that field.

---

In addition, maybe the rules should be categorised in multiple ways, not just the "simpleness" of the enforcement (which is good in the case of I.24 but it gets woefully verbose), but also the "simpleness" of fixing the bug (which is pretty hard in the case of I.24) or the "applicability" to already existing projects versus green-field ones (which is pretty bad in the case of I.24). Unlike other guidelines like SEI CERT, CPPCG seems not to have a "severity" suggestion on the individual rules.

[hsutter]

Editors call: Thank you, this has been very helpful.

We need to do more work before we can make a change. We are not sure whether this rule can be enforced in legacy code or only through new design or redesign of interfaces. We are most concerned about people swapping arguments at call sites (which don't necessarily see the definition), rather than providing advice for function implementers. However, violations can only be addressed by the interface's designer so there is a tension here we need to think about more.

For now, the rule is admittedly noisy and you would be right to let users selectively opt into enabling it. We've added a Note that "Only the interface's designer can adequately address the source of violations of this guideline." and added a comment to the Enforcement that "We are still looking for a less-simple enforcement." -- further suggestions welcome.