Page MenuHomePhabricator

[clang-tidy] Add 'readability-suspicious-call-argument' check
Changes PlannedPublic

Authored by whisperity on May 26 2016, 10:38 AM.

Details

Summary

This check finds function calls where the call arguments might be provided in an incorrect order, known as argument selection defects. It works by comparing the name of the argument with the name of the parameter in the function declaration.
A diagnostic is emitted if an argument name is similar to another argument than the one it is passed to currently. It uses case-insensitive comparison and implements multiple heuristics to lessen the number of noisy warnings.

False-positive warnings from this check are still useful as they indicate bad naming conventions for variables and parameters.

Originally implemented by @varjujan.
@barancsuk contributed by keeping the check further developed.

Diff Detail

Event Timeline

varjujan updated this revision to Diff 58644.May 26 2016, 10:38 AM
varjujan retitled this revision from to [clang-tidy] Suspicious Call Argument checker.
varjujan updated this object.
varjujan added a reviewer: alexfh.
varjujan added subscribers: xazax.hun, cfe-commits.
varjujan updated this object.May 27 2016, 5:10 AM
alexfh edited edge metadata.May 27 2016, 6:54 AM

Thank you for the new check!

Before starting with the review, I'd like to clarify one important thing. It's not immediately obvious that the pattern the check detects is actually a good indicator of a programming mistake. Did you try to run the check on a large enough codebase (at least, on the whole LLVM project) and analyze the results? http://clang.llvm.org/extra/clang-tidy/#running-clang-tidy-on-llvm describes the recommended way to run clang-tidy on LLVM.

Yes, I did. The results from running the checker on LLVM are in the attached file. Sadly, I could'nt find any real mistakes but as I wrote in the summary, false positives can still indicate bad naming convention for some variables.

alexfh added a comment.Jun 3 2016, 3:09 PM

Yes, I did. The results from running the checker on LLVM are in the attached file. Sadly, I could'nt find any real mistakes but as I wrote in the summary, false positives can still indicate bad naming convention for some variables.

It looks like the check doesn't meet the quality bar, at least, in its current form. "Bad naming convention" is a very arguable thing and I'm not sure we can claim that the pattern detected by the check is somehow an indication of a bad naming convention.

alexfh requested changes to this revision.Jun 3 2016, 3:10 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jun 3 2016, 3:10 PM
varjujan updated this revision to Diff 82859.Jan 3 2017, 3:49 AM
varjujan edited edge metadata.

I have implemented some more heuristics to achieve better results.

I ran the check on multiple projects and tried to categorize the warnings: real errors, false positives, naming errors and coincidences. The results are attached. I got no warnings on LLVM.

alexfh requested changes to this revision.Jan 3 2017, 7:46 AM
alexfh edited edge metadata.

I ran the check on multiple projects and tried to categorize the warnings: real errors, false positives, naming errors and coincidences. The results are attached. I got no warnings on LLVM.

I didn't find any "real errors" in the files you posted, which means that either all of these projects are of extreme code quality or that the error is rather unlikely to happen.

Another concern is that the check seems to treat argument types in a rather primitive way, e.g. it doesn't consider any type conversions or promotions.

This revision now requires changes to proceed.Jan 3 2017, 7:46 AM

I think this might be better as a readability checker to find misleading variable or parameter names.

It would also be great to consider types. Unfortunately it probably means reimplementing some of the logic from Sema, since that information is not available at this point.

@varjujan
Do you actually use all of the heuristics that are implemented?

@xazax.hun
Yes I do. Obviously some of them seem to be better than the others so I can remove a couple if needed.

whisperity added a project: Restricted Project.May 12 2017, 2:40 AM
whisperity added subscribers: gsd, dkrupp, whisperity, o.gyorgy.
barancsuk commandeered this revision.Aug 31 2017, 3:12 AM
barancsuk added a reviewer: varjujan.
barancsuk added a subscriber: barancsuk.

Since the previous author, @varjujan is not available anymore, I take over the role of the author of this revision.

barancsuk updated this revision to Diff 113376.EditedAug 31 2017, 4:58 AM
barancsuk edited edge metadata.

Major changes that have been made since the last update are as follows:

  • The checker is moved from the module misc to readability
  • It is checked, whether implicit type conversion is possible from the argument to the other parameter. The following conversion rules are considered:
    • Arithmetic type conversions
    • Pointer to pointer conversion (for multilevel pointers and also base/derived class pointers)
    • Array to pointer conversion
    • Function to function-pointer conversion
    • CV-qualifier compatibility is checked as well.
  • The calculation of a heuristic`s result and the comparison of this result with the corresponding threshold value are performed by the same method, the heuristic`s function.
  • The heuristic`s function is called only if the heuristic itself is turned on by the configuration settings.

Remark:
Implicit conversion rules of C are not checked separately, because, as I experienced when testing the checker on a larger code base, deeming all pointers convertible results in several false positives.

barancsuk updated this revision to Diff 113830.Sep 5 2017, 4:07 AM

Check if argument and parameter numbers differ, add test cases for functions with default parameters

barancsuk added a comment.EditedSep 15 2017, 6:32 AM

@alexfh, would you mind taking a look at the changes that have been introduced in the new patch?

The main improvements are:

  • The checker has been shifted to the module readability.
  • It is checked, whether implicit type conversion is possible from the argument to the parameter.

I have run the modified checker on some large code bases with the following results:
(The errors were categorized subjectively.)

  • PostgreSQL: 32 renaming opportunities of 39 warnings
  • Cpython: 10 renaming opportunities of 15 warnings
  • Xerces: 6 renaming opportunities of 8 warnings
  • FFmpeg: 5 renaming opportunities of 9 warnings
  • OpenSSL: 3 renaming opportunities of 4 warnings
  • LLVM: 20 renaming opportunities of 44 warnings

This article provides some evidence to support the feasibility of the checker as well.


The authors have proven that argument names are generally very similar to the corresponding parameters' names.
The presented empirical evidence also shows, that argument and parameter name dissimilarities are strong indicators of incorrect argument usages, or they identify renaming opportunities to improve code readability.
Moreover, the authors have even found 3 existing bugs in open source projects.

@alexfh, would you mind taking a look at the changes that have been introduced in the new patch?

The main improvements are:

  • The checker has been shifted to the module readability.
  • It is checked, whether implicit type conversion is possible from the argument to the parameter.

I have run the modified checker on some large code bases with the following results:
(The errors were categorized subjectively.)

  • PostgreSQL: 32 renaming opportunities of 39 warnings
  • Cpython: 10 renaming opportunities of 15 warnings
  • Xerces: 6 renaming opportunities of 8 warnings
  • FFmpeg: 5 renaming opportunities of 9 warnings
  • OpenSSL: 3 renaming opportunities of 4 warnings
  • LLVM: 20 renaming opportunities of 44 warnings

Is there a list of all the warnings? I'd like to take a closer look.

This article provides some evidence to support the feasibility of the checker as well.


The authors have proven that argument names are generally very similar to the corresponding parameters' names.
The presented empirical evidence also shows, that argument and parameter name dissimilarities are strong indicators of incorrect argument usages, or they identify renaming opportunities to improve code readability.
Moreover, the authors have even found 3 existing bugs in open source projects.

I attached the results of the tests.
The warnings are categorized into false positives and renaming opportunities.

@alexfh, have you had a chance to look at the results yet?

szepet added a subscriber: szepet.Oct 8 2017, 10:06 AM

Sorry, I lost this patch. I've looked at the results and it still seems that the signal-to-noise ratio is quite low. There's definitely potential in using parameter name and argument spelling to detect possibly swapped arguments, and there's a recent research on this topic, where authors claim to have reached the true positive rate of 85% with decent recall. See https://research.google.com/pubs/pub46317.html. If you're interested in working on this check, I would suggest at least looking at the techniques described in that paper.

alexfh requested changes to this revision.Jan 5 2018, 6:25 AM
This revision now requires changes to proceed.Jan 5 2018, 6:25 AM

I have developed a related check in D69560. That one considers types, but is an interface rule checker, and does not consider (any) potential call sites. Moreover, it does not consider "swaps" that happen across a function call, only, as the name implies, adjacent similar-type ranges.

Maybe one could lift the "is-similar-type", or rather, "is-accidentally-mixable-type" related ruling to some common location, and use type similarity as a precondition gate in the reports of this check?

docs/clang-tidy/checks/misc-redundant-expression.rst
18 ↗(On Diff #113830)

This seems to be an unrelated diff.

test/clang-tidy/misc-redundant-expression.cpp
20 ↗(On Diff #113830)

This entire file seems to be unrelated to the discussion at hand, perhaps a rebase went sideways?

whisperity commandeered this revision.Apr 23 2020, 3:33 AM
whisperity added a reviewer: barancsuk.

Assuming direct control. Previous colleagues and university mates departed for snowier pastures, time to try to do something with this check.

whisperity retitled this revision from [clang-tidy] Suspicious Call Argument checker to [clang-tidy] Add 'readability-suspicious-call-argument' check.
whisperity edited the summary of this revision. (Show Details)
whisperity removed reviewers: varjujan, barancsuk.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity edited subscribers, added: varjujan, zporky; removed: gsd.

First things first, we were 50 thousand (!) patches behind reality. Rebased to master. Fixed it to compile, too. Otherwise, NFC so far.

Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2020, 3:40 AM
whisperity planned changes to this revision.Apr 23 2020, 3:41 AM
whisperity edited the summary of this revision. (Show Details)

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.

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 relation owner 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.

But of course, after having written all of that, I forgot to upload the results themselves... 😲

Right, let's bump.

Thank you for all of the detailed information on the performance of the check! I worked on a similar check in a recent past life and my intuition is that over a large corpus of code, this will still have quite a bit of false positives and false negatives. However, I think those can likely be handled by post-commit improvements like noticing abbreviations (def vs define) or synonyms (number vs numeral), being smarter about patterned names, etc. I think this check is worth moving forward with, but I'm not certain how @alexfh feels about it.

(Typing this in here so it is not forgotten.) @aaron.ballman suggested in http://reviews.llvm.org/D69560#inline-893813 that a case of argument and parameter swaps could be discovered between forward declarations and the definitions, i.e.

void fn(int x, int y);

void fn(int y, int x) { ... } // Proparly mistakenly swapped names!

The conclusion was that this check (given this one does string distance, metrics, etc.) should be extended with such functionality. However, we need a better name for the check in that case!