whisperity updated this revision to Diff 339533. whisperity edited the summary of this revision. whisperity added a comment.
- **NFC** Fix lint of header guard nomenclature - **NFC** Tear out obsolete `LLVM_DEBUG` calls from the implementation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D20689/new/ https://reviews.llvm.org/D20689 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp @@ -0,0 +1,477 @@ +// RUN: %check_clang_tidy %s readability-suspicious-call-argument %t -- -- -std=c++11 + +void foo_1(int aaaaaa, int bbbbbb) {} + +void foo_2(int source, int aaaaaa) {} + +void foo_3(int valToRet, int aaaaaa) {} + +void foo_4(int pointer, int aaaaaa) {} + +void foo_5(int aaaaaa, int bbbbbb, int cccccc, ...) {} + +void foo_6(const int dddddd, bool &eeeeee) {} + +void foo_7(int aaaaaa, int bbbbbb, int cccccc, int ffffff = 7) {} + +void foo_8(int frobble1, int frobble2) {} + +// Test functions for convertible argument--parameter types. +void fun(const int &m); +void fun2() { + int m = 3; + fun(m); +} + +// Test cases for parameters of const reference and value. +void value_const_reference(int llllll, const int &kkkkkk); + +void const_ref_value_swapped() { + const int &kkkkkk = 42; + const int &llllll = 42; + value_const_reference(kkkkkk, llllll); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'kkkkkk' (passed to 'llllll') looks like it might be swapped with the 2nd, 'llllll' (passed to 'kkkkkk') [readability-suspicious-call-argument] + // CHECK-MESSAGES: :[[@LINE-7]]:6: note: in the call to 'value_const_reference', declared here +} + +// Const, non const references. +void const_nonconst_parameters(const int &mmmmmm, int &nnnnnn); + +void const_nonconst_swap1() { + const int &nnnnnn = 42; + int mmmmmm; + // Do not check, because non-const reference parameter cannot bind to const reference argument. + const_nonconst_parameters(nnnnnn, mmmmmm); +} + +void const_nonconst_swap3() { + const int nnnnnn = 42; + int m = 42; + int &mmmmmm = m; + // Do not check, const int does not bind to non const reference. + const_nonconst_parameters(nnnnnn, mmmmmm); +} + +void const_nonconst_swap2() { + int nnnnnn; + int mmmmmm; + // Check for swapped arguments. (Both arguments are non-const.) + const_nonconst_parameters(nnnnnn, mmmmmm); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'nnnnnn' (passed to 'mmmmmm') looks like it might be swapped with the 2nd, 'mmmmmm' (passed to 'nnnnnn') +} + +void const_nonconst_pointers(const int *mmmmmm, int *nnnnnn); +void const_nonconst_pointers2(const int *mmmmmm, const int *nnnnnn); + +void const_nonconst_pointers_swapped() { + int *mmmmmm; + const int *nnnnnn; + const_nonconst_pointers(nnnnnn, mmmmmm); +} + +void const_nonconst_pointers_swapped2() { + const int *mmmmmm; + int *nnnnnn; + const_nonconst_pointers2(nnnnnn, mmmmmm); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'nnnnnn' (passed to 'mmmmmm') looks like it might be swapped with the 2nd, 'mmmmmm' (passed to 'nnnnnn') +} + +// Test cases for pointers and arrays. +void pointer_array_parameters( + int *pppppp, int qqqqqq[4]); + +void pointer_array_swap() { + int qqqqqq[5]; + int *pppppp; + // Check for swapped arguments. An array implicitly converts to a pointer. + pointer_array_parameters(qqqqqq, pppppp); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'qqqqqq' (passed to 'pppppp') looks like it might be swapped with the 2nd, 'pppppp' (passed to 'qqqqqq') +} + +// Test cases for multilevel pointers. +void multilevel_pointer_parameters(int *const **pppppp, + const int *const *volatile const *qqqqqq); +void multilevel_pointer_parameters2( + char *****nnnnnn, char *volatile *const *const *const *const &mmmmmm); + +typedef float T; +typedef T *S; +typedef S *const volatile R; +typedef R *Q; +typedef Q *P; +typedef P *O; +void multilevel_pointer_parameters3(float **const volatile ***rrrrrr, O &ssssss); + +void multilevel_pointer_swap() { + int *const **qqqqqq; + int *const **pppppp; + multilevel_pointer_parameters(qqqqqq, pppppp); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'qqqqqq' (passed to 'pppppp') looks like it might be swapped with the 2nd, 'pppppp' (passed to 'qqqqqq') + + char *****mmmmmm; + char *****nnnnnn; + multilevel_pointer_parameters2(mmmmmm, nnnnnn); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'mmmmmm' (passed to 'nnnnnn') looks like it might be swapped with the 2nd, 'nnnnnn' (passed to 'mmmmmm') + + float **const volatile ***rrrrrr; + float **const volatile ***ssssss; + multilevel_pointer_parameters3(ssssss, rrrrrr); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'ssssss' (passed to 'rrrrrr') looks like it might be swapped with the 2nd, 'rrrrrr' (passed to 'ssssss') +} + +void multilevel_pointer_parameters4(char ****pppppp, + char *const volatile **const *qqqqqq); +void multilevel_pointer_parameters5( + bool *****nnnnnn, bool *volatile *const *const *const *&mmmmmm); +void multilevel_pointer_parameters6(double **llllll, char **&kkkkkk); +void multilevel_pointer_parameters7(const volatile int ***iiiiii, + const int *const *const *jjjjjj); + +void multilevel_pointer_swap3() { + char ****qqqqqq; + char *const volatile **const *pppppp; + // Do not check. + multilevel_pointer_parameters4(qqqqqq, pppppp); + + bool *****mmmmmm; + bool *volatile *const *const *const *nnnnnn; + // Do not check. + multilevel_pointer_parameters5(mmmmmm, nnnnnn); + + double **kkkkkk; + char **llllll; + multilevel_pointer_parameters6(kkkkkk, llllll); + + const volatile int ***jjjjjj; + const int *const *const *iiiiii; + multilevel_pointer_parameters7(jjjjjj, iiiiii); +} + +// Test cases for multidimesional arrays. +void multilevel_array_parameters(int pppppp[2][2][2], const int qqqqqq[][2][2]); + +void multilevel_array_parameters2(int (*mmmmmm)[2][2], int nnnnnn[9][2][23]); + +void multilevel_array_parameters3(int (*eeeeee)[2][2], int (&ffffff)[1][2][2]); + +void multilevel_array_parameters4(int (*llllll)[2][2], int kkkkkk[2][2]); + +void multilevel_array_parameters5(int iiiiii[2][2], char jjjjjj[2][2]); + +void multilevel_array_parameters6(int (*bbbbbb)[2][2], int cccccc[1][2][2]); + +void multilevel_array_swap() { + int qqqqqq[1][2][2]; + int pppppp[][2][2] = {{{1, 2}, {1, 2}}, {{1, 2}, {1, 2}}}; // int [2][2][2] + multilevel_array_parameters(qqqqqq, pppppp); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'qqqqqq' (passed to 'pppppp') looks like it might be swapped with the 2nd, 'pppppp' (passed to 'qqqqqq') + + int(*nnnnnn)[2][2]; + int mmmmmm[9][2][23]; + // Do not check, array sizes has to match in every dimension, except the first. + multilevel_array_parameters2(nnnnnn, mmmmmm); + + int ffffff[][2][2] = {{{1, 2}, {1, 2}}, {{1, 2}, {1, 2}}}; // int [2][2][2] + int eeeeee[1][2][2] = {{{1, 2}, {1, 2}}}; // int [1][2][2] + // Do not check, for array references, size has to match in every dimension. + multilevel_array_parameters3(ffffff, eeeeee); + + int kkkkkk[2][2][2]; + int(*llllll)[2]; + // Do not check, argument dimensions differ. + multilevel_array_parameters4(kkkkkk, llllll); + + int jjjjjj[2][2]; + char iiiiii[2][2]; + // Do not check, array element types differ. + multilevel_array_parameters5(jjjjjj, iiiiii); + + int t[][2][2] = {{{1, 2}, {1, 2}}, {{1, 2}, {1, 2}}}; // int [2][2][2] + int(*cccccc)[2][2] = t; // int (*)[2][2] + int bbbbbb[][2][2] = {{{1, 2}, {1, 2}}}; // int [1][2][2] + multilevel_array_parameters6(cccccc, bbbbbb); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'bbbbbb') looks like it might be swapped with the 2nd, 'bbbbbb' (passed to 'cccccc') +} + +void multilevel_array_swap2() { + int qqqqqq[2][2][2]; + const int pppppp[][2][2] = {{{1, 2}, {1, 2}}, {{1, 2}, {1, 2}}}; + // Do not check, pppppp is const and cannot bind to an array with nonconst elements. + multilevel_array_parameters(qqqqqq, pppppp); +} + +// Complex test case. +void multilevel_pointer_array_parameters(const int(*const (*volatile const (*const (*const (*const &aaaaaa)[1])[32])[4])[3][2][2]), const int(*const (*volatile const (*const (*const (*&bbbbbb)[1])[32])[4])[3][2][2])); + +void multilevel_pointer_array_swap() { + const int( + *const(*volatile const(*const(*const(*aaaaaa)[1])[32])[4])[3][2][2]); + const int( + *const(*volatile const(*const(*const(*bbbbbb)[1])[32])[4])[3][2][2]); + multilevel_pointer_array_parameters(bbbbbb, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'bbbbbb' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaaaa' (passed to 'bbbbbb') +} + +enum class numbers_scoped { one, + two }; + +// Test cases for arithmetic types. +void arithmetic_type_parameters(float vvvvvv, int wwwwww); +void arithmetic_type_parameters2(numbers_scoped vvvvvv, int wwwwww); + +void arithmetic_types_swap1() { + bool wwwwww; + float vvvvvv; + arithmetic_type_parameters(wwwwww, vvvvvv); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'wwwwww' (passed to 'vvvvvv') looks like it might be swapped with the 2nd, 'vvvvvv' (passed to 'wwwwww') +} + +void arithmetic_types_swap3() { + char wwwwww; + unsigned long long int vvvvvv; + arithmetic_type_parameters(wwwwww, vvvvvv); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'wwwwww' (passed to 'vvvvvv') looks like it might be swapped with the 2nd, 'vvvvvv' (passed to 'wwwwww') +} + +void arithmetic_types_swap4() { + enum numbers { one, + two }; + numbers wwwwww = numbers::one; + int vvvvvv; + arithmetic_type_parameters(wwwwww, vvvvvv); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'wwwwww' (passed to 'vvvvvv') looks like it might be swapped with the 2nd, 'vvvvvv' (passed to 'wwwwww') +} + +void arithmetic_types_swap5() { + wchar_t vvvvvv; + float wwwwww; + arithmetic_type_parameters(wwwwww, vvvvvv); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'wwwwww' (passed to 'vvvvvv') looks like it might be swapped with the 2nd, 'vvvvvv' (passed to 'wwwwww') +} + +void arithmetic_types_swap6() { + wchar_t vvvvvv; + numbers_scoped wwwwww = numbers_scoped::one; + // Do not check, numers is a scoped enum type. + arithmetic_type_parameters2(wwwwww, vvvvvv); +} + +// Base, derived +class TestClass { +public: + void thisFunction(int integerParam, int thisIsPARAM) {} +}; + +class DerivedTestClass : public TestClass {}; + +void base_derived_pointer_parameters(TestClass *aaaaaa, + DerivedTestClass *bbbbbb); + +void base_derived_swap1() { + TestClass *bbbbbb; + DerivedTestClass *aaaaaa; + // Do not check, because TestClass does not convert implicitly to DerivedTestClass. + base_derived_pointer_parameters(bbbbbb, aaaaaa); +} + +void base_derived_swap2() { + DerivedTestClass *bbbbbb, *aaaaaa; + // Check for swapped arguments, DerivedTestClass converts to TestClass implicitly. + base_derived_pointer_parameters(bbbbbb, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'bbbbbb' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaaaa' (passed to 'bbbbbb') +} + +// Multilevel inheritance +class DerivedOfDerivedTestClass : public DerivedTestClass {}; + +void multi_level_inheritance_swap() { + DerivedOfDerivedTestClass *aaaaaa, *bbbbbb; + // Check for swapped arguments. Derived classes implicitly convert to their base. + base_derived_pointer_parameters( + bbbbbb, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 1st argument 'bbbbbb' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaaaa' (passed to 'bbbbbb') +} + +// Tests for function pointer swaps +void funct_ptr_params(double (*ffffff)(int, int), double (*gggggg)(int, int)); +void funct_ptr_params(double (*ffffff)(int, int), int (*gggggg)(int, int)); + +double ffffff(int a, int b) { return 0; } +double gggggg(int a, int b) { return 0; } + +void funtionc_ptr_params_swap() { + funct_ptr_params(gggggg, ffffff); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'gggggg' (passed to 'ffffff') looks like it might be swapped with the 2nd, 'ffffff' (passed to 'gggggg') +} + +int fffff(int a, int b) { return 0; } + +void function_ptr_swap2() { + // Do not check, because the function `ffffff` cannot convert to a function + // with prototype: double(int,int). + funct_ptr_params(gggggg, fffff); +} + +// Paraphrased example from Z3 (src/qe/qe_arrays.cpp) which originally produced +// a false positive. Operator() calls should ignore the called object +// "argument". +struct type1; +struct type2; +struct type3; + +struct callable1 { + void operator()(type1 &mdl, type2 &arr_vars, type3 &fml, type2 &aux_vars) const {} +}; + +struct callable2 { + void operator()(type1 &mdl, type2 &arr_vars, type3 &fml, type2 &aux_vars, + bool reduce_all_selects) const { + (void)reduce_all_selects; + callable1 pe; + pe(mdl, arr_vars, fml, aux_vars); + // NO-WARN: Argument and parameter names match perfectly, "pe" should be + // ignored! + } +}; + +struct binop_t {}; + +binop_t operator+(const binop_t &lhs, const binop_t &rhs) { return lhs; } +bool operator<(const binop_t &lhs, const binop_t &rhs) { return true; } +bool operator>(const binop_t &aaaaaa, const binop_t &bbbbbb) { return false; } + +void binop_test() { + // NO-WARN: Binary operators are ignored. + binop_t lhs, rhs; + if (lhs + rhs < rhs) + return; + + if (operator<(rhs, lhs)) + return; + + binop_t aaaaaa, cccccc; + if (operator>(cccccc, aaaaaa)) + return; +} + +int recursion(int aaaa, int bbbb) { + if (aaaa) + return 0; + + int cccc = 0; + return recursion(bbbb, cccc); + // NO-WARN: Recursive calls usually shuffle with arguments and we ignore those. +} + +void pass_by_copy(binop_t xxxx, binop_t yyyy) {} + +// Paraphrased example from LLVM's code (lib/Analysis/InstructionSimplify.cpp) +// that generated a false positive. +struct value; +enum opcode { Foo, + Bar }; +static value *SimplifyRightShift( + opcode Opcode, value *Op0, value *Op1, bool isExact, + const type1 &Q, unsigned MaxRecurse) {} +static value *SimplifyLShrInst(value *Op0, value *Op1, bool isExact, + const type1 &Q, unsigned MaxRecurse) { + if (value *V = SimplifyRightShift(Foo, Op0, Op1, isExact, Q, MaxRecurse)) + return V; + // NO-WARN: Argument names perfectly match parameter names, sans the enum. + + return nullptr; +} + +void has_unnamed(int aaaaaa, int) {} + +int main() { + // Equality test. + int aaaaaa, cccccc = 0; + foo_1(cccccc, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaaaa' (passed to 'bbbbbb') + + binop_t xxxx, yyyy; + pass_by_copy(yyyy, xxxx); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'yyyy' (passed to 'xxxx') looks like it might be swapped with the 2nd, 'xxxx' (passed to 'yyyy') + + // Abbreviation test. + int src = 0; + foo_2(aaaaaa, src); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'aaaaaa' (passed to 'source') looks like it might be swapped with the 2nd, 'src' (passed to 'aaaaaa') + + // Levenshtein test. + int aaaabb = 0; + foo_1(cccccc, aaaabb); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaabb' (passed to 'bbbbbb') + + // Prefix test. + int aaaa = 0; + foo_1(cccccc, aaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaa' (passed to 'bbbbbb') + + // Suffix test. + int urce = 0; + foo_2(cccccc, urce); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'source') looks like it might be swapped with the 2nd, 'urce' (passed to 'aaaaaa') + + // Substring test. + int ourc = 0; + foo_2(cccccc, ourc); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'source') looks like it might be swapped with the 2nd, 'ourc' (passed to 'aaaaaa') + + // Jaro-Winkler test. + int iPonter = 0; + foo_4(cccccc, iPonter); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'pointer') looks like it might be swapped with the 2nd, 'iPonter' (passed to 'aaaaaa') + + // Dice test. + int aaabaa = 0; + foo_1(cccccc, aaabaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaabaa' (passed to 'bbbbbb') + + // Variadic function test. + int bbbbbb = 0; + foo_5(src, bbbbbb, cccccc, aaaaaa); // Should pass. + foo_5(cccccc, bbbbbb, aaaaaa, src); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 3rd, 'aaaaaa' (passed to 'cccccc') + + // Test function with default argument. + foo_7(src, bbbbbb, cccccc, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'src' (passed to 'aaaaaa') looks like it might be swapped with the 4th, 'aaaaaa' (passed to 'ffffff') + + foo_7(cccccc, bbbbbb, aaaaaa, src); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'cccccc' (passed to 'aaaaaa') looks like it might be swapped with the 3rd, 'aaaaaa' (passed to 'cccccc') + + int ffffff = 0; + foo_7(ffffff, bbbbbb, cccccc); // NO-WARN: Even though 'ffffff' is passed to 'aaaaaa' and there is a 4th parameter 'ffffff', there isn't a **swap** here. + + int frobble1 = 1, frobble2 = 2; + foo_8(frobble2, frobble1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'frobble2' (passed to 'frobble1') looks like it might be swapped with the 2nd, 'frobble1' (passed to 'frobble2') + + int bar1 = 1, bar2 = 2; + foo_8(bar2, bar1); // NO-WARN. + + // Type match + bool dddddd = false; + int eeeeee = 0; + auto szam = 0; + foo_6(eeeeee, dddddd); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'eeeeee' (passed to 'dddddd') looks like it might be swapped with the 2nd, 'dddddd' (passed to 'eeeeee') + foo_1(szam, aaaaaa); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'szam' (passed to 'aaaaaa') looks like it might be swapped with the 2nd, 'aaaaaa' (passed to 'bbbbbb') + + // Test lambda. + auto testMethod = [&](int method, int randomParam) { return 0; }; + int method = 0; + testMethod(method, 0); // Should pass. + + // Member function test. + TestClass test; + int integ, thisIsAnArg = 0; + test.thisFunction(integ, thisIsAnArg); // Should pass. + + has_unnamed(1, bbbbbb); + + return 0; +} Index: clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst @@ -0,0 +1,242 @@ +.. title:: clang-tidy - readability-suspicious-call-argument + +readability-suspicious-call-argument +==================================== + +Finds function calls where the arguments passed are provided out of order, +based on the difference between the argument name and the parameter names +of the function. + +Given a function call ``f(foo, bar);`` and a function signature +``void f(T tvar, U uvar)``, the arguments ``foo`` and ``bar`` are swapped if +``foo`` (the argument name) is more similar to ``uvar`` (the other parameter) +than ``tvar`` (the parameter it is currently passed to) **and** ``bar`` is +more similar to ``tvar`` than ``uvar``. + +Warnings might indicate either that the arguments are swapped, or that the +names' cross-similarity might hinder code comprehension. + +.. _heuristics: + +Heuristics +---------- + +The following heuristics are implemented in the check. +If **any** of the enabled heuristics deem the arguments to be provided out of +order, a warning will be issued. + +The heuristics themselves are implemented by considering pairs of strings, and +are symmetric, so in the following there is no distinction on which string is +the argument name and which string is the parameter name. + +Equality +^^^^^^^^ + +The most trivial heuristic, which compares the two strings for case-insensitive +equality. + +.. _abbreviation_heuristic: + +Abbreviation +^^^^^^^^^^^^ + +Common abbreviations can be specified which will deem the strings similar if +the abbreviated and the abbreviation stand together. +For example, if ``src`` is registered as an abbreviation for ``source``, then +the following code example will be warned about. + +.. code-block:: c++ + + void foo(int source, int x); + + foo(b, src); + +The abbreviations to recognise can be configured with the +:ref:`Abbreviations<opt_Abbreviations>` check option. +This heuristic is case-insensitive. + +Prefix +^^^^^^ + +The *prefix* heuristic reports if one of the strings is a sufficiently long +prefix of the other string, e.g. ``target`` to ``targetPtr``. +The similarity percentage is the length ratio of the prefix to the longer +string, in the previous example, it would be `6 / 9 = 66.66...`\%. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `25`\% dissimilar and above `30`\% similar. +This heuristic is case-insensitive. + +Suffix +^^^^^^ + +Analogous to the `Prefix` heuristic. +In the case of ``oldValue`` and ``value`` compared, the similarity percentage +is `8 / 5 = 62.5`\%. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `25`\% dissimilar and above `30`\% similar. +This heuristic is case-insensitive. + +Substring +^^^^^^^^^ + +The substring heuristic combines the prefix and the suffix heuristic, and tries +to find the *longest common substring* in the two strings provided. +The similarity percentage is the ratio of the found longest common substring +against the *longer* of the two input strings. +For example, given ``val`` and ``rvalue``, the similarity is `3 / 6 = 50`\%. +If no characters are common in the two string, `0`\%. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `40`\% dissimilar and above `50`\% similar. +This heuristic is case-sensitive. + +Levenshtein distance (as `Levenshtein`) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The `Levenshtein distance <http://en.wikipedia.org/wiki/Levenshtein_distance>`_ +describes how many single-character changes (additions, changes, or removals) +must be applied to transform one string into another. + +The Levenshtein distance is translated into a similarity percentage by dividing +it with the length of the *longer* string, and taking its complement with +regards to `100`\%. +For example, given ``something`` and ``anything``, the distance is `4` edits, +and the similarity percentage is `100`\% `- 4 / 9 = 55.55...`\%. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `50`\% dissimilar and above `66`\% similar. +This heuristic is case-sensitive. + +JaroâWinkler distance (as `JaroWinkler`) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The `JaroâWinkler distance <http://en.wikipedia.org/wiki/JaroâWinkler_distance>`_ +is an edit distance like the Levenshtein distance. +It is calculated from the amount of common characters that are sufficiently +close to each other in position, and to-be-changed characters. +The original definition of Jaro has been extended by Winkler to weigh prefix +similarities more. +The similarity percentage is expressed as an average of the common and +non-common characters against the length of both strings. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `75`\% dissimilar and above `85`\% similar. +This heuristic is case-insensitive. + +SørensenâDice coefficient (as `Dice`) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The `SørensenâDice coefficient <http://en.wikipedia.org/wiki/SørensenâDice_coefficient>`_ +was originally defined to measure the similarity of two sets. +Formally, the coefficient is calculated by dividing `2 * #(intersection)` with +`#(set1) + #(set2)`, where `#()` is the cardinality function of sets. +This metric is applied to strings by creating bigrams (substring sequences of +length 2) of the two strings and using the set of bigrams for the two strings +as the two sets. + +This heuristic can be configured with :ref:`bounds<opt_Bounds>`. +The default bounds are: below `60`\% dissimilar and above `70`\% similar. +This heuristic is case-insensitive. + + +Options +------- + +.. option:: MinimumIdentifierNameLength + + Sets the minimum required length the argument and parameter names + need to have. Names shorter than this length will be ignored. + Defaults to `3`. + +.. _opt_Abbreviations: + +.. option:: Abbreviations + + For the **Abbreviation** heuristic + (:ref:`see here<abbreviation_heuristic>`), this option configures the + abbreviations in the `"abbreviation=abbreviated_value"` format. + The option is a string, with each value joined by `";"`. + + By default, the following abbreviations are set: + + * `addr=address` + * `arr=array` + * `attr=attribute` + * `buf=buffer` + * `cl=client` + * `cnt=count` + * `col=column` + * `cpy=copy` + * `dest=destination` + * `dist=distance` + * `dst=distance` + * `elem=element` + * `hght=height` + * `i=index` + * `idx=index` + * `len=length` + * `ln=line` + * `lst=list` + * `nr=number` + * `num=number` + * `pos=position` + * `ptr=pointer` + * `ref=reference` + * `src=source` + * `srv=server` + * `stmt=statement` + * `str=string` + * `val=value` + * `var=variable` + * `vec=vector` + * `wdth=width` + +The configuration options for each implemented heuristic (see above) is +constructed dynamically. +In the following, `<HeuristicName>` refers to one of the keys from the +heuristics implemented. + +.. option:: <HeuristicName> + + `True` or `False`, whether a particular heuristic, such as `Equality` or + `Levenshtein` is enabled. + + Defaults to `True` for every heuristic. + +.. _opt_Bounds: + +.. option:: <HeuristicName>DissimilarBelow, <HeuristicName>SimilarAbove + + A value between `0` and `100`, expressing a percentage. + The bounds set what percentage of similarity the heuristic must deduce + for the two identifiers to be considered similar or dissimilar by the + check. + + Given arguments ``arg1`` and ``arg2`` passed to ``param1`` and ``param2``, + respectively, the bounds check is performed in the following way: + If the similarity of the currently passed argument order + (``arg1`` to ``param1``) is **below** the `DissimilarBelow` threshold, and + the similarity of the suggested swapped order (``arg1`` to ``param2``) is + **above** the `SimilarAbove` threshold, the swap is reported. + + For the defaults of each heuristic, :ref:`see above<heuristics>`. + + +Name synthesis +-------------- + +When comparing the argument names and parameter names, the following logic is +used to gather the names for comparison: + +Parameter names are the identifiers as written in the source code. + +Argument names are: + + * If a variable is passed, the variable's name. + * If a subsequent function call's return value is used as argument, the called + function's name. + * Otherwise, empty string. + +Empty argument or parameter names are ignored by the heuristics. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -309,6 +309,7 @@ `readability-static-accessed-through-instance <readability-static-accessed-through-instance.html>`_, "Yes" `readability-static-definition-in-anonymous-namespace <readability-static-definition-in-anonymous-namespace.html>`_, "Yes" `readability-string-compare <readability-string-compare.html>`_, "Yes" + `readability-suspicious-call-argument <readability-suspicious-call-argument.html>`_, `readability-uniqueptr-delete-release <readability-uniqueptr-delete-release.html>`_, "Yes" `readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes" `readability-use-anyofallof <readability-use-anyofallof.html>`_, Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -110,6 +110,13 @@ :doc:`concurrency-thread-canceltype-asynchronous <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` was added. +- New `readability-suspicious-call-argument + <clang-tidy/checks/readability-suspicious-call-argument>`_ check + + Finds function calls where the arguments passed are provided out of order, + based on the difference between the argument name and the parameter names + of the function. + Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h @@ -0,0 +1,100 @@ +//===--- SuspiciousCallArgumentCheck.h - clang-tidy -------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SUSPICIOUSCALLARGUMENT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SUSPICIOUSCALLARGUMENT_H + +#include "../ClangTidyCheck.h" +#include "llvm/ADT/StringSet.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Finds function calls where the arguments passed are provided out of order, +/// based on the difference between the argument name and the parameter names +/// of the function. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-suspicious-call-argument.html +class SuspiciousCallArgumentCheck : public ClangTidyCheck { + enum class Heuristic { + Equality, + Abbreviation, + Prefix, + Suffix, + Substring, + Levenshtein, + JaroWinkler, + Dice + }; + + /// When applying a heuristic, the value of this enum decides which kind of + /// bound will be selected from the bounds configured for the heuristic. + /// This only applies to heuristics that can take bounds. + enum class BoundKind { + /// Check for dissimilarity of the names. Names are deemed dissimilar if + /// the similarity measurement is **below** the configured threshold. + DissimilarBelow, + + /// Check for similarity of the names. Names are deemed similar if the + /// similarity measurement (the result of heuristic) is **above** the + /// configured threshold. + SimilarAbove + }; + +public: + static constexpr std::size_t SmallVectorSize = 8; + static constexpr std::size_t HeuristicCount = + static_cast<std::size_t>(Heuristic::Dice) + 1; + + SuspiciousCallArgumentCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const std::size_t MinimumIdentifierNameLength; + + /// The configuration for which heuristics were enabled. + SmallVector<Heuristic, HeuristicCount> AppliedHeuristics; + + /// The lower and upper bounds for each heuristic, as configured by the user. + SmallVector<std::pair<int8_t, int8_t>, HeuristicCount> ConfiguredBounds; + + /// The abbreviation-to-abbreviated map for the Abbreviation heuristic. + llvm::StringMap<std::string> AbbreviationDictionary; + + bool isHeuristicEnabled(Heuristic H) const; + Optional<int8_t> getBound(Heuristic H, BoundKind BK) const; + + // Runtime information of the currently analyzed function call. + SmallVector<QualType, SmallVectorSize> ArgTypes; + SmallVector<StringRef, SmallVectorSize> ArgNames; + SmallVector<QualType, SmallVectorSize> ParamTypes; + SmallVector<StringRef, SmallVectorSize> ParamNames; + + void setParamNamesAndTypes(const FunctionDecl *CalleeFuncDecl); + + void setArgNamesAndTypes(const CallExpr *MatchedCallExpr, + std::size_t InitialArgIndex); + + bool areParamAndArgComparable(std::size_t Position1, std::size_t Position2, + const ASTContext &Ctx) const; + + bool areArgsSwapped(std::size_t Position1, std::size_t Position2) const; + + bool areNamesSimilar(StringRef Arg, StringRef Param, Heuristic H, + BoundKind BK) const; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SUSPICIOUS_CALL_ARGUMENT_H Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp @@ -0,0 +1,805 @@ +//===--- SuspiciousCallArgumentCheck.cpp - clang-tidy ---------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "SuspiciousCallArgumentCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include <sstream> + +using namespace clang::ast_matchers; +namespace optutils = clang::tidy::utils::options; + +namespace clang { +namespace tidy { +namespace readability { + +namespace { +struct DefaultHeuristicConfiguration { + /// Whether the heuristic is to be enabled by default. + const bool Enabled; + + /// The upper bound of % of similarity the two strings might have to be + /// considered dissimilar. + /// (For purposes of configuration, -1 if the heuristic is not configurable + /// with bounds.) + const int8_t DissimilarBelow; + + /// The lower bound of % of similarity the two string must have to be + /// considered similar. + /// (For purposes of configuration, -1 if the heuristic is not configurable + /// with bounds.) + const int8_t SimilarAbove; + + /// Can the heuristic be configured with bounds? + bool hasBounds() const { return DissimilarBelow > -1 && SimilarAbove > -1; } +}; +} // namespace + +static constexpr std::size_t DefaultMinimumIdentifierNameLength = 3; + +static constexpr StringRef HeuristicToString[] = { + "Equality", "Abbreviation", "Prefix", "Suffix", + "Substring", "Levenshtein", "JaroWinkler", "Dice"}; + +static constexpr DefaultHeuristicConfiguration Defaults[] = { + {true, -1, -1}, // Equality. + {true, -1, -1}, // Abbreviation. + {true, 25, 30}, // Prefix. + {true, 25, 30}, // Suffix. + {true, 40, 50}, // Substring. + {true, 50, 66}, // Levenshtein. + {true, 75, 85}, // Jaro-Winkler. + {true, 60, 70}, // Dice. +}; + +static_assert( + sizeof(HeuristicToString) / sizeof(HeuristicToString[0]) == + SuspiciousCallArgumentCheck::HeuristicCount, + "Ensure that every heuristic has a corresponding stringified name"); +static_assert(sizeof(Defaults) / sizeof(Defaults[0]) == + SuspiciousCallArgumentCheck::HeuristicCount, + "Ensure that every heuristic has a default configuration."); + +namespace { +template <std::size_t I> struct HasWellConfiguredBounds { + static constexpr bool Value = + !((Defaults[I].DissimilarBelow == -1) ^ (Defaults[I].SimilarAbove == -1)); + static_assert(Value, "A heuristic must either have a dissimilarity and " + "similarity bound, or neither!"); +}; + +template <std::size_t I> struct HasWellConfiguredBoundsFold { + static constexpr bool Value = HasWellConfiguredBounds<I>::Value && + HasWellConfiguredBoundsFold<I - 1>::Value; +}; + +template <> struct HasWellConfiguredBoundsFold<0> { + static constexpr bool Value = HasWellConfiguredBounds<0>::Value; +}; + +struct AllHeuristicsBoundsWellConfigured { + static constexpr bool Value = + HasWellConfiguredBoundsFold<SuspiciousCallArgumentCheck::HeuristicCount - + 1>::Value; +}; + +static_assert(AllHeuristicsBoundsWellConfigured::Value, ""); +} // namespace + +static const std::string DefaultAbbreviations = + optutils::serializeStringList({"addr=address", + "arr=array", + "attr=attribute", + "buf=buffer", + "cl=client", + "cnt=count", + "col=column", + "cpy=copy", + "dest=destination", + "dist=distance" + "dst=distance", + "elem=element", + "hght=height", + "i=index", + "idx=index", + "len=length", + "ln=line", + "lst=list", + "nr=number", + "num=number", + "pos=position", + "ptr=pointer", + "ref=reference", + "src=source", + "srv=server", + "stmt=statement", + "str=string", + "val=value", + "var=variable", + "vec=vector", + "wdth=width"}); + +static constexpr std::size_t SmallVectorSize = + SuspiciousCallArgumentCheck::SmallVectorSize; + +/// Returns how many % X is of Y. +static inline double percentage(double X, double Y) { return X / Y * 100.0; } + +static bool applyEqualityHeuristic(StringRef Arg, StringRef Param) { + return Arg.equals_lower(Param); +} + +static bool applyAbbreviationHeuristic( + const llvm::StringMap<std::string> &AbbreviationDictionary, StringRef Arg, + StringRef Param) { + if (AbbreviationDictionary.find(Arg) != AbbreviationDictionary.end() && + Param.equals(AbbreviationDictionary.lookup(Arg))) + return true; + + if (AbbreviationDictionary.find(Param) != AbbreviationDictionary.end() && + Arg.equals(AbbreviationDictionary.lookup(Param))) + return true; + + return false; +} + +/// Check whether the shorter String is a prefix of the longer String. +static bool applyPrefixHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + StringRef Shorter = Arg.size() < Param.size() ? Arg : Param; + StringRef Longer = Arg.size() >= Param.size() ? Arg : Param; + + if (Longer.startswith_lower(Shorter)) + return percentage(Shorter.size(), Longer.size()) > Threshold; + + return false; +} + +/// Check whether the shorter String is a suffix of the longer String. +static bool applySuffixHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + StringRef Shorter = Arg.size() < Param.size() ? Arg : Param; + StringRef Longer = Arg.size() >= Param.size() ? Arg : Param; + + if (Longer.endswith_lower(Shorter)) + return percentage(Shorter.size(), Longer.size()) > Threshold; + + return false; +} + +static bool applySubstringHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + std::size_t MaxLength = 0; + SmallVector<std::size_t, SmallVectorSize> Current(Param.size()); + SmallVector<std::size_t, SmallVectorSize> Previous(Param.size()); + + for (std::size_t I = 0; I < Arg.size(); ++I) { + for (std::size_t J = 0; J < Param.size(); ++J) { + if (Arg[I] == Param[J]) { + if (I == 0 || J == 0) + Current[J] = 1; + else + Current[J] = 1 + Previous[J - 1]; + + MaxLength = std::max(MaxLength, Current[J]); + } else + Current[J] = 0; + } + + Current.swap(Previous); + } + + size_t LongerLength = std::max(Arg.size(), Param.size()); + return percentage(MaxLength, LongerLength) > Threshold; +} + +static bool applyLevenshteinHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + std::size_t LongerLength = std::max(Arg.size(), Param.size()); + double Dist = Arg.edit_distance(Param); + Dist = (1 - Dist / LongerLength) * 100; + return Dist > Threshold; +} + +// Based on http://en.wikipedia.org/wiki/JaroâWinkler_distance. +static bool applyJaroWinklerHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + std::size_t Match = 0, Transpos = 0; + std::ptrdiff_t ArgLen = Arg.size(); + std::ptrdiff_t ParamLen = Param.size(); + SmallVector<int, SmallVectorSize> ArgFlags(ArgLen); + SmallVector<int, SmallVectorSize> ParamFlags(ParamLen); + std::ptrdiff_t Range = + std::max(std::ptrdiff_t{0}, std::max(ArgLen, ParamLen) / 2 - 1); + + // Calculate matching characters. + for (std::ptrdiff_t I = 0; I < ParamLen; ++I) + for (std::ptrdiff_t J = std::max(I - Range, std::ptrdiff_t{0}), + L = std::min(I + Range + 1, ArgLen); + J < L; ++J) + if (tolower(Param[I]) == tolower(Arg[J]) && !ArgFlags[J]) { + ArgFlags[J] = 1; + ParamFlags[I] = 1; + ++Match; + break; + } + + if (!Match) + return false; + + // Calculate character transpositions. + std::ptrdiff_t L = 0; + for (std::ptrdiff_t I = 0; I < ParamLen; ++I) { + if (ParamFlags[I] == 1) { + std::ptrdiff_t J; + for (J = L; J < ArgLen; ++J) + if (ArgFlags[J] == 1) { + L = J + 1; + break; + } + + if (tolower(Param[I]) != tolower(Arg[J])) + ++Transpos; + } + } + Transpos /= 2; + + // Jaro distance. + double MatchD = Match; + double Dist = ((MatchD / ArgLen) + (MatchD / ParamLen) + + ((MatchD - Transpos) / Match)) / + 3.0; + + // Calculate common string prefix up to 4 chars. + L = 0; + for (std::ptrdiff_t I = 0; + I < std::min(std::min(ArgLen, ParamLen), std::ptrdiff_t{4}); ++I) + if (tolower(Arg[I]) == tolower(Param[I])) + ++L; + + // Jaro-Winkler distance. + Dist = (Dist + (L * 0.1 * (1 - Dist))) * 100; + return Dist > Threshold; +} + +// Based on http://en.wikipedia.org/wiki/SørensenâDice_coefficient +static bool applyDiceHeuristic(StringRef Arg, StringRef Param, + int8_t Threshold) { + llvm::StringSet<> ArgBigrams; + llvm::StringSet<> ParamBigrams; + + // Extract character bigrams from Arg. + for (std::ptrdiff_t I = 0; I < static_cast<std::ptrdiff_t>(Arg.size()) - 1; + ++I) + ArgBigrams.insert(Arg.substr(I, 2).lower()); + + // Extract character bigrams from Param. + for (std::ptrdiff_t I = 0; I < static_cast<std::ptrdiff_t>(Param.size()) - 1; + ++I) + ParamBigrams.insert(Param.substr(I, 2).lower()); + + std::size_t Intersection = 0; + + // Find the intersection between the two sets. + for (auto IT = ParamBigrams.begin(); IT != ParamBigrams.end(); ++IT) + Intersection += ArgBigrams.count((IT->getKey())); + + // Calculate Dice coefficient. + return percentage(Intersection * 2.0, + ArgBigrams.size() + ParamBigrams.size()) > Threshold; +} + +/// Checks if ArgType binds to ParamType ragerding reference-ness and +/// cv-qualifiers. +static bool areRefAndQualCompatible(QualType ArgType, QualType ParamType) { + return !ParamType->isReferenceType() || + ParamType.getNonReferenceType().isAtLeastAsQualifiedAs( + ArgType.getNonReferenceType()); +} + +static bool isPointerOrArray(const QualType &TypeToCheck) { + return TypeToCheck->isPointerType() || TypeToCheck->isArrayType(); +} + +/// Checks whether ArgType is an array type identical to ParamType's array type. +/// Enforces array elements' qualifier compatibility as well. +static bool isCompatibleWithArrayReference(const QualType &ArgType, + const QualType &ParamType) { + if (!ArgType->isArrayType()) + return false; + // Here, qualifiers belong to the elements of the arrays. + if (!ParamType.isAtLeastAsQualifiedAs(ArgType)) + return false; + + return ParamType.getUnqualifiedType() == ArgType.getUnqualifiedType(); +} + +static void convertToPointeeOrArrayElementQualType(QualType &TypeToConvert) { + unsigned CVRqualifiers = 0; + // Save array element qualifiers, since getElementType() removes qualifiers + // from array elements. + if (TypeToConvert->isArrayType()) + CVRqualifiers = TypeToConvert.getLocalQualifiers().getCVRQualifiers(); + TypeToConvert = TypeToConvert->isPointerType() + ? TypeToConvert->getPointeeType() + : TypeToConvert->getAsArrayTypeUnsafe()->getElementType(); + TypeToConvert = TypeToConvert.withCVRQualifiers(CVRqualifiers); +} + +/// Checks if multilevel pointers' qualifiers compatibility continues on the +/// current pointer level. For multilevel pointers, C++ permits conversion, if +/// every cv-qualifier in ArgType also appears in the corresponding position in +/// ParamType, and if PramType has a cv-qualifier that's not in ArgType, then +/// every * in ParamType to the right of that cv-qualifier, except the last +/// one, must also be const-qualified. +static bool arePointersStillQualCompatible(QualType ArgType, QualType ParamType, + bool &IsParamContinuouslyConst) { + // The types are compatible, if the parameter is at least as qualified as the + // argument, and if it is more qualified, it has to be const on upper pointer + // levels. + bool AreTypesQualCompatible = + ParamType.isAtLeastAsQualifiedAs(ArgType) && + (!ParamType.hasQualifiers() || IsParamContinuouslyConst); + // Check whether the parameter's constness continues at the current pointer + // level. + IsParamContinuouslyConst &= ParamType.isConstQualified(); + + return AreTypesQualCompatible; +} + +/// Checks whether multilevel pointers are compatible in terms of levels, +/// qualifiers and pointee type. +static bool arePointerTypesCompatible(QualType ArgType, QualType ParamType, + bool IsParamContinuouslyConst) { + + if (!arePointersStillQualCompatible(ArgType, ParamType, + IsParamContinuouslyConst)) + return false; + + do { + // Step down one pointer level. + convertToPointeeOrArrayElementQualType(ArgType); + convertToPointeeOrArrayElementQualType(ParamType); + + // Check whether cv-qualifiers premit compatibility on + // current level. + if (!arePointersStillQualCompatible(ArgType, ParamType, + IsParamContinuouslyConst)) + return false; + + if (ParamType.getUnqualifiedType() == ArgType.getUnqualifiedType()) + return true; + + } while (ParamType->isPointerType() && ArgType->isPointerType()); + // The final type does not match, or pointer levels differ. + return false; +} + +/// Checks whether ArgType converts implicitly to ParamType. +static bool areTypesCompatible(QualType ArgType, QualType ParamType, + const ASTContext &Ctx) { + if (ArgType.isNull() || ParamType.isNull()) + return false; + + ArgType = ArgType.getCanonicalType(); + ParamType = ParamType.getCanonicalType(); + + if (ArgType == ParamType) + return true; + + // Check for constness and reference compatibility. + if (!areRefAndQualCompatible(ArgType, ParamType)) + return false; + + bool IsParamReference = ParamType->isReferenceType(); + + // Reference-ness has already been checked ad should be removed + // before further checking. + ArgType = ArgType.getNonReferenceType(); + ParamType = ParamType.getNonReferenceType(); + + bool IsParamContinuouslyConst = + !IsParamReference || ParamType.getNonReferenceType().isConstQualified(); + + if (ParamType.getUnqualifiedType() == ArgType.getUnqualifiedType()) + return true; + + // Arithmetic types are interconvertible, except scoped enums. + if (ParamType->isArithmeticType() && ArgType->isArithmeticType()) { + if ((ParamType->isEnumeralType() && + ParamType->getAs<EnumType>()->getDecl()->isScoped()) || + (ArgType->isEnumeralType() && + ArgType->getAs<EnumType>()->getDecl()->isScoped())) + return false; + + return true; + } + + // Check if the argument and the param are both function types (the parameter + // decayed to + // a function pointer). + if (ArgType->isFunctionType() && ParamType->isFunctionPointerType()) { + ParamType = ParamType->getPointeeType(); + return ArgType == ParamType; + } + + // Arrays or pointer arguments convert to array or pointer parameters. + if (!(isPointerOrArray(ArgType) && isPointerOrArray(ParamType))) + return false; + + // When ParamType is an array reference, ArgType has to be of the same sized, + // array type with cv-compatible elements. + if (IsParamReference && ParamType->isArrayType()) + return isCompatibleWithArrayReference(ArgType, ParamType); + + // Remove the first level of indirection. + convertToPointeeOrArrayElementQualType(ArgType); + convertToPointeeOrArrayElementQualType(ParamType); + + // Check qualifier compatibility on the next level. + if (!ParamType.isAtLeastAsQualifiedAs(ArgType)) + return false; + + if (ParamType.getUnqualifiedType() == ArgType.getUnqualifiedType()) + return true; + + // At this point, all possible C language implicit conversion were checked + if (!Ctx.getLangOpts().CPlusPlus) + return false; + + // Check whether ParamType and ArgType were both pointers to a class or a + // struct, and check for inheritance. + if (ParamType->isStructureOrClassType() && + ArgType->isStructureOrClassType()) { + auto *ArgDecl = ArgType->getAsCXXRecordDecl(); + auto *ParamDecl = ParamType->getAsCXXRecordDecl(); + if (!ArgDecl || !ArgDecl->hasDefinition() || !ParamDecl || + !ParamDecl->hasDefinition()) + return false; + + return ArgDecl->isDerivedFrom(ParamDecl); + } + + // Unless argument and param are both multilevel pointers, the types are not + // convertible. + if (!(ParamType->isAnyPointerType() && ArgType->isAnyPointerType())) + return false; + + return arePointerTypesCompatible(ArgType, ParamType, + IsParamContinuouslyConst); +} + +static bool isOverloadedUnaryOrBinarySymbolOperator(const FunctionDecl *FD) { + switch (FD->getOverloadedOperator()) { + case OO_None: + case OO_Call: + case OO_Subscript: + case OO_New: + case OO_Delete: + case OO_Array_New: + case OO_Array_Delete: + case OO_Conditional: + case OO_Coawait: + return false; + + default: + return FD->getNumParams() <= 2; + } +} + +SuspiciousCallArgumentCheck::SuspiciousCallArgumentCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + MinimumIdentifierNameLength(Options.get( + "MinimumIdentifierNameLength", DefaultMinimumIdentifierNameLength)) { + auto GetToggleOpt = [this](Heuristic H) -> bool { + auto Idx = static_cast<std::size_t>(H); + assert(Idx < HeuristicCount); + return Options.get(HeuristicToString[Idx], Defaults[Idx].Enabled); + }; + auto GetBoundOpt = [this](Heuristic H, BoundKind BK) -> int8_t { + auto Idx = static_cast<std::size_t>(H); + assert(Idx < HeuristicCount); + + SmallString<32> Key = HeuristicToString[Idx]; + Key.append(BK == BoundKind::DissimilarBelow ? "DissimilarBelow" + : "SimilarAbove"); + int8_t Default = BK == BoundKind::DissimilarBelow + ? Defaults[Idx].DissimilarBelow + : Defaults[Idx].SimilarAbove; + return Options.get(Key, Default); + }; + for (std::size_t Idx = 0; Idx < HeuristicCount; ++Idx) { + auto H = static_cast<Heuristic>(Idx); + if (GetToggleOpt(H)) + AppliedHeuristics.emplace_back(H); + ConfiguredBounds.emplace_back( + std::make_pair(GetBoundOpt(H, BoundKind::DissimilarBelow), + GetBoundOpt(H, BoundKind::SimilarAbove))); + } + + for (const std::string &Abbreviation : optutils::parseStringList( + Options.get("Abbreviations", DefaultAbbreviations))) { + auto KeyAndValue = StringRef{Abbreviation}.split("="); + assert(!KeyAndValue.first.empty() && !KeyAndValue.second.empty()); + AbbreviationDictionary.insert( + std::make_pair(KeyAndValue.first.str(), KeyAndValue.second.str())); + } +} + +void SuspiciousCallArgumentCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "MinimumIdentifierNameLength", + MinimumIdentifierNameLength); + const auto &SetToggleOpt = [this, &Opts](Heuristic H) -> void { + auto Idx = static_cast<std::size_t>(H); + Options.store(Opts, HeuristicToString[Idx], isHeuristicEnabled(H)); + }; + const auto &SetBoundOpt = [this, &Opts](Heuristic H, BoundKind BK) -> void { + auto Idx = static_cast<std::size_t>(H); + assert(Idx < HeuristicCount); + if (!Defaults[Idx].hasBounds()) + return; + + SmallString<32> Key = HeuristicToString[Idx]; + Key.append(BK == BoundKind::DissimilarBelow ? "DissimilarBelow" + : "SimilarAbove"); + Options.store(Opts, Key, getBound(H, BK).getValue()); + }; + + for (std::size_t Idx = 0; Idx < HeuristicCount; ++Idx) { + auto H = static_cast<Heuristic>(Idx); + SetToggleOpt(H); + SetBoundOpt(H, BoundKind::DissimilarBelow); + SetBoundOpt(H, BoundKind::SimilarAbove); + } + + SmallVector<std::string, 32> Abbreviations; + for (const auto &Abbreviation : AbbreviationDictionary) { + SmallString<32> EqualSignJoined; + EqualSignJoined.append(Abbreviation.first()); + EqualSignJoined.append("="); + EqualSignJoined.append(Abbreviation.second); + + if (!Abbreviation.second.empty()) + Abbreviations.emplace_back(EqualSignJoined.str()); + } + Options.store(Opts, "Abbreviations", + optutils::serializeStringList(Abbreviations)); +} + +bool SuspiciousCallArgumentCheck::isHeuristicEnabled(Heuristic H) const { + return llvm::find(AppliedHeuristics, H) != AppliedHeuristics.end(); +} + +Optional<int8_t> SuspiciousCallArgumentCheck::getBound(Heuristic H, + BoundKind BK) const { + auto Idx = static_cast<std::size_t>(H); + assert(Idx < HeuristicCount); + + if (!Defaults[Idx].hasBounds()) + return {}; + + switch (BK) { + case BoundKind::DissimilarBelow: + return ConfiguredBounds[Idx].first; + case BoundKind::SimilarAbove: + return ConfiguredBounds[Idx].second; + } + llvm_unreachable("Unhandled Bound kind."); +} + +void SuspiciousCallArgumentCheck::registerMatchers(MatchFinder *Finder) { + // Only match calls with at least 2 arguments. + Finder->addMatcher( + functionDecl(forEachDescendant(callExpr(unless(anyOf(argumentCountIs(0), + argumentCountIs(1)))) + .bind("functionCall"))) + .bind("callingFunc"), + this); +} + +void SuspiciousCallArgumentCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedCallExpr = + Result.Nodes.getNodeAs<CallExpr>("functionCall"); + const auto *Caller = Result.Nodes.getNodeAs<FunctionDecl>("callingFunc"); + assert(MatchedCallExpr && Caller); + + const Decl *CalleeDecl = MatchedCallExpr->getCalleeDecl(); + if (!CalleeDecl) + return; + + const FunctionDecl *CalleeFuncDecl = CalleeDecl->getAsFunction(); + if (!CalleeFuncDecl) + return; + if (CalleeFuncDecl == Caller) + // Ignore recursive calls. + return; + if (isOverloadedUnaryOrBinarySymbolOperator(CalleeFuncDecl)) + return; + + // Get param attributes. + setParamNamesAndTypes(CalleeFuncDecl); + + if (ParamNames.empty()) + return; + + // Get Arg attributes. + std::size_t InitialArgIndex = 0; + + if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(CalleeFuncDecl)) { + if (MethodDecl->getParent()->isLambda()) + // Lambda functions' first Arg are the lambda object. + InitialArgIndex = 1; + else if (MethodDecl->getOverloadedOperator() == OO_Call) + // For custom operator()s, the first Arg is the called object. + InitialArgIndex = 1; + } + + setArgNamesAndTypes(MatchedCallExpr, InitialArgIndex); + + if (ArgNames.empty()) + return; + + std::size_t ParamCount = ParamNames.size(); + + // Check similarity. + for (std::size_t I = 0; I < ParamCount; ++I) { + for (std::size_t J = I + 1; J < ParamCount; ++J) { + // Do not check if param or arg names are short, or not convertible. + if (!areParamAndArgComparable(I, J, *Result.Context)) + continue; + if (!areArgsSwapped(I, J)) + continue; + + // Warning at the call itself. + diag(MatchedCallExpr->getExprLoc(), + "%ordinal0 argument '%1' (passed to '%2') looks like it might be " + "swapped with the %ordinal3, '%4' (passed to '%5')") + << static_cast<unsigned>(I + 1) << ArgNames[I] << ParamNames[I] + << static_cast<unsigned>(J + 1) << ArgNames[J] << ParamNames[J] + << MatchedCallExpr->getArg(I)->getSourceRange() + << MatchedCallExpr->getArg(J)->getSourceRange(); + + // Note at the functions declaration. + SourceLocation IParNameLoc = + CalleeFuncDecl->getParamDecl(I)->getLocation(); + SourceLocation JParNameLoc = + CalleeFuncDecl->getParamDecl(J)->getLocation(); + + diag(CalleeFuncDecl->getLocation(), "in the call to %0, declared here", + DiagnosticIDs::Note) + << CalleeFuncDecl + << CharSourceRange::getTokenRange(IParNameLoc, IParNameLoc) + << CharSourceRange::getTokenRange(JParNameLoc, JParNameLoc); + } + } +} + +void SuspiciousCallArgumentCheck::setParamNamesAndTypes( + const FunctionDecl *CalleeFuncDecl) { + // Reset vectors, and fill them with the currently checked function's + // parameters' data. + ParamNames.clear(); + ParamTypes.clear(); + + for (std::size_t I = 0, E = CalleeFuncDecl->getNumParams(); I != E; ++I) { + if (const auto *Param = CalleeFuncDecl->getParamDecl(I)) { + ParamTypes.push_back(Param->getType()); + + if (IdentifierInfo *II = Param->getIdentifier()) + ParamNames.push_back(II->getName()); + else + ParamNames.push_back(StringRef()); + } + } +} + +void SuspiciousCallArgumentCheck::setArgNamesAndTypes( + const CallExpr *MatchedCallExpr, std::size_t InitialArgIndex) { + // Reset vectors, and fill them with the currently checked function's + // arguments' data. + ArgNames.clear(); + ArgTypes.clear(); + + for (std::size_t I = InitialArgIndex, J = MatchedCallExpr->getNumArgs(); + I < J; ++I) { + if (const auto *ArgExpr = dyn_cast<DeclRefExpr>( + MatchedCallExpr->getArg(I)->IgnoreUnlessSpelledInSource())) { + if (const auto *Var = dyn_cast<VarDecl>(ArgExpr->getDecl())) { + ArgTypes.push_back(Var->getType()); + ArgNames.push_back(Var->getName()); + } else if (const auto *FCall = + dyn_cast<FunctionDecl>(ArgExpr->getDecl())) { + ArgTypes.push_back(FCall->getType()); + ArgNames.push_back(FCall->getName()); + } else { + ArgTypes.push_back(QualType()); + ArgNames.push_back(StringRef()); + } + } else { + ArgTypes.push_back(QualType()); + ArgNames.push_back(StringRef()); + } + } +} + +bool SuspiciousCallArgumentCheck::areParamAndArgComparable( + std::size_t Position1, std::size_t Position2, const ASTContext &Ctx) const { + if (Position1 >= ArgNames.size() || Position2 >= ArgNames.size()) + return false; + + // Do not report for too short strings. + if (ArgNames[Position1].size() < MinimumIdentifierNameLength || + ArgNames[Position2].size() < MinimumIdentifierNameLength || + ParamNames[Position1].size() < MinimumIdentifierNameLength || + ParamNames[Position2].size() < MinimumIdentifierNameLength) + return false; + + if (!areTypesCompatible(ArgTypes[Position1], ParamTypes[Position2], Ctx) || + !areTypesCompatible(ArgTypes[Position2], ParamTypes[Position1], Ctx)) + return false; + + return true; +} + +bool SuspiciousCallArgumentCheck::areArgsSwapped(std::size_t Position1, + std::size_t Position2) const { + for (Heuristic H : AppliedHeuristics) { + bool A1ToP2Similar = areNamesSimilar( + ArgNames[Position2], ParamNames[Position1], H, BoundKind::SimilarAbove); + bool A2ToP1Similar = areNamesSimilar( + ArgNames[Position1], ParamNames[Position2], H, BoundKind::SimilarAbove); + + bool A1ToP1Dissimilar = + !areNamesSimilar(ArgNames[Position1], ParamNames[Position1], H, + BoundKind::DissimilarBelow); + bool A2ToP2Dissimilar = + !areNamesSimilar(ArgNames[Position2], ParamNames[Position2], H, + BoundKind::DissimilarBelow); + + if ((A1ToP2Similar || A2ToP1Similar) && A1ToP1Dissimilar && + A2ToP2Dissimilar) + return true; + } + return false; +} + +bool SuspiciousCallArgumentCheck::areNamesSimilar(StringRef Arg, + StringRef Param, Heuristic H, + BoundKind BK) const { + int8_t Threshold = -1; + if (Optional<int8_t> GotBound = getBound(H, BK)) + Threshold = GotBound.getValue(); + + switch (H) { + case Heuristic::Equality: + return applyEqualityHeuristic(Arg, Param); + case Heuristic::Abbreviation: + return applyAbbreviationHeuristic(AbbreviationDictionary, Arg, Param); + case Heuristic::Prefix: + return applyPrefixHeuristic(Arg, Param, Threshold); + case Heuristic::Suffix: + return applySuffixHeuristic(Arg, Param, Threshold); + case Heuristic::Substring: + return applySubstringHeuristic(Arg, Param, Threshold); + case Heuristic::Levenshtein: + return applyLevenshteinHeuristic(Arg, Param, Threshold); + case Heuristic::JaroWinkler: + return applyJaroWinklerHeuristic(Arg, Param, Threshold); + case Heuristic::Dice: + return applyDiceHeuristic(Arg, Param, Threshold); + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -43,6 +43,7 @@ #include "StaticAccessedThroughInstanceCheck.h" #include "StaticDefinitionInAnonymousNamespaceCheck.h" #include "StringCompareCheck.h" +#include "SuspiciousCallArgumentCheck.h" #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" #include "UseAnyOfAllOfCheck.h" @@ -122,6 +123,8 @@ "readability-redundant-string-init"); CheckFactories.registerCheck<SimplifyBooleanExprCheck>( "readability-simplify-boolean-expr"); + CheckFactories.registerCheck<SuspiciousCallArgumentCheck>( + "readability-suspicious-call-argument"); CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>( "readability-uniqueptr-delete-release"); CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>( Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -40,6 +40,7 @@ StaticAccessedThroughInstanceCheck.cpp StaticDefinitionInAnonymousNamespaceCheck.cpp StringCompareCheck.cpp + SuspiciousCallArgumentCheck.cpp UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp UseAnyOfAllOfCheck.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits