[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixab…

…ility because of implicit conversions

Adds a relaxation option ModelImplicitConversions which will make the check
report for cases where parameters refer to types that are implicitly
convertible to one another.

Example:

    struct IntBox { IntBox(int); operator int(); };
    void foo(int i, double d, IntBox ib) {}

Implicit conversions are the last to model in the set of things that are
reasons for the possibility of a function being called the wrong way which is
not always immediately apparent when looking at the function (signature or
call).

Reviewed By: aaron.ballman, martong

Differential Revision: http://reviews.llvm.org/D75041

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

@@ -39,8 +39,14 @@ class EasilySwappableParametersCheck : public ClangTidyCheck {
   /// ignored.
   const std::vector<std::string> IgnoredParameterTypeSuffixes;
 
-  /// Whether to consider an unqualified and a qualified type mixable.
+  /// Whether to consider differently qualified versions of the same type
+  /// mixable.
   const bool QualifiersMix;
+
+  /// Whether to model implicit conversions "in full" (conditions apply)
+  /// during analysis and consider types that are implicitly convertible to
+  /// one another mixable.
+  const bool ModelImplicitConversions;
 };
 
 } // namespace bugprone

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst

@@ -57,8 +57,40 @@ report longer mixable ranges.
 
     .. code-block:: c++
 
-        void *memcpy(const void *Destination, void *Source, std::size_t N) {}
+        void *memcpy(const void *Destination, void *Source, std::size_t N) { /* ... */ }
 
+.. option:: ModelImplicitConversions
+
+    Whether to consider parameters of type ``T`` and ``U`` mixable if there
+    exists an implicit conversion from ``T`` to ``U`` and ``U`` to ``T``.
+    If `false`, the check will not consider implicitly convertible types for
+    mixability.
+    `True` turns warnings for implicit conversions on.
+    Defaults to `true`.
+
+    The following examples produce a diagnostic only if
+    `ModelImplicitConversions` is enabled:
+
+    .. code-block:: c++
+
+        void fun(int Int, double Double) { /* ... */ }
+        void compare(const char *CharBuf, std::string String) { /* ... */ }
+
+    .. note::
+
+        Changing the qualifiers of an expression's type (e.g. from ``int`` to
+        ``const int``) is defined as an *implicit conversion* in the C++
+        Standard.
+        However, the check separates this decision-making on the mixability of
+        differently qualified types based on whether `QualifiersMix` was
+        enabled.
+
+        For example, the following code snippet will only produce a diagnostic
+        if **both** `QualifiersMix` and `ModelImplicitConversions` are enabled:
+
+        .. code-block:: c++
+
+            void fun2(int Int, const double Double) { /* ... */ }
 
 Filtering options
 ^^^^^^^^^^^^^^^^^
@@ -133,7 +165,7 @@ None of the following cases produce a diagnostic:
     template <typename T, typename U>
     int add(T X, U Y) { return X + Y };
 
-    void TheseAreNotWarnedAbout() {
+    void theseAreNotWarnedAbout() {
         printf("%d %d\n", 1, 2);   // Two ints passed, they could be swapped.
         someOldCFunction(1, 2, 3); // Similarly, multiple ints passed.
 
@@ -153,14 +185,43 @@ not diagnosed.
 
     // Diagnosed: Explicit instantiation was done by the user, we can prove it
     // is the same type.
-    void Explicit(int A, Vector<int>::element_type B) { /* ... */ }
+    void instantiated(int A, Vector<int>::element_type B) { /* ... */ }
 
     // Diagnosed: The two parameter types are exactly the same.
     template <typename T>
-    void Exact(typename Vector<T>::element_type A,
+    void exact(typename Vector<T>::element_type A,
                typename Vector<T>::element_type B) { /* ... */ }
 
     // Skipped: The two parameters are both 'T' but we can not prove this
     // without actually instantiating.
     template <typename T>
-    void FalseNegative(T A, typename Vector<T>::element_type B) { /* ... */ }
+    void falseNegative(T A, typename Vector<T>::element_type B) { /* ... */ }
+
+In the context of *implicit conversions* (when `ModelImplicitConversions` is
+enabled), the modelling performed by the check
+warns if the parameters are swappable and the swapped order matches implicit
+conversions.
+It does not model whether there exists an unrelated third type from which
+*both* parameters can be given in a function call.
+This means that in the following example, even while ``strs()`` clearly carries
+the possibility to be called with swapped arguments (as long as the arguments
+are string literals), will not be warned about.
+
+.. code-block:: c++
+
+    struct String {
+        String(const char *Buf);
+    };
+
+    struct StringView {
+        StringView(const char *Buf);
+        operator const char *() const;
+    };
+
+    // Skipped: Directly swapping expressions of the two type cannot mix.
+    // (Note: StringView -> const char * -> String would be **two**
+    // user-defined conversions, which is disallowed by the language.)
+    void strs(String Str, StringView SV) { /* ... */ }
+
+    // Diagnosed: StringView implicitly converts to and from a buffer.
+    void cStr(StringView SV, const char *Buf() { /* ... */ }

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp

@@ -3,7 +3,8 @@
 // RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
 // RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: "\"\";Foo;Bar"}, \
 // RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"}, \
-// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp

@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \
+// RUN:  ]}' --
+
+void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); }
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'numericAndQualifierConversion' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:56: note: the last parameter in the range is 'CD'
+// CHECK-MESSAGES: :[[@LINE-4]]:43: note: 'int' and 'const double' parameters accept and bind the same kind of values
+// CHECK-MESSAGES: :[[@LINE-5]]:43: note: 'int' and 'const double' may be implicitly converted: 'int' -> 'const double' (as 'double'), 'const double' (as 'double') -> 'int'

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c

@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1} \
+// RUN:  ]}' --
+
+void implicitDoesntBreakOtherStuff(int A, int B) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: 2 adjacent parameters of 'implicitDoesntBreakOtherStuff' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:40: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-3]]:47: note: the last parameter in the range is 'B'
+
+void arrayAndPtr1(int *IP, int IA[]) { arrayAndPtr1(IA, IP); }
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP'
+// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA'
+
+void arrayAndPtr2(int *IP, int IA[8]) { arrayAndPtr2(IA, IP); }
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'arrayAndPtr2' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'IP'
+// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in the range is 'IA'
+
+void arrayAndElement(int I, int IA[]) {} // NO-WARN.
+
+void numericConversion1(int I, double D) { numericConversion1(D, I); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion1' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:39: note: the last parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'double' may be implicitly converted{{$}}
+
+void numericConversion2(int I, short S) { numericConversion2(S, I); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion2' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: the last parameter in the range is 'S'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'short' may be implicitly converted{{$}}
+
+void numericConversion3(float F, unsigned long UL) { numericConversion3(UL, F); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion3' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:31: note: the first parameter in the range is 'F'
+// CHECK-MESSAGES: :[[@LINE-3]]:48: note: the last parameter in the range is 'UL'
+// CHECK-MESSAGES: :[[@LINE-4]]:34: note: 'float' and 'unsigned long' may be implicitly converted{{$}}
+
+enum Unscoped { U_A,
+                U_B };
+enum UnscopedFixed : char { UF_A,
+                            UF_B };
+
+void numericConversion4(int I, enum Unscoped U) { numericConversion4(U, I); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion4' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:46: note: the last parameter in the range is 'U'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'enum Unscoped' may be implicitly converted{{$}}
+
+void numericConversion5(int I, enum UnscopedFixed UF) { numericConversion5(UF, I); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion5' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:51: note: the last parameter in the range is 'UF'
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: 'int' and 'enum UnscopedFixed' may be implicitly converted{{$}}
+
+void numericConversion7(double D, enum Unscoped U) { numericConversion7(U, D); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion7' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-3]]:49: note: the last parameter in the range is 'U'
+// CHECK-MESSAGES: :[[@LINE-4]]:35: note: 'double' and 'enum Unscoped' may be implicitly converted{{$}}
+
+void numericConversion8(double D, enum UnscopedFixed UF) { numericConversion8(UF, D); }
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 2 adjacent parameters of 'numericConversion8' of convertible types
+// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-3]]:54: note: the last parameter in the range is 'UF'
+// CHECK-MESSAGES: :[[@LINE-4]]:35: note: 'double' and 'enum UnscopedFixed' may be implicitly converted{{$}}
+
+void pointeeConverison(int *IP, double *DP) { pointeeConversion(DP, IP); }
+// NO-WARN: Even though this is possible in C, a swap is diagnosed by the compiler.