[clang-tidy] Fix crash on "reference to array" parameters in 'bugprone-easily-swappable-parameters'

[whisperity]

Summary

Fixes #50995.

A(n otherwise unexercised) code path related to trying to model "array-to-pointer decay" resulted in a null pointer dereference crash when parameters of type "reference to array" are encountered.

The aforementioned code path is not needed, as cases when arrays can decay are already handled by the main function of the modelling routine when desugaring the DecayedType.

[whisperity]

Add some more test cases!

[whisperity]

NFC Typo-fix: Remove empty line from end of diff.

[aaron.ballman]

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

46

Can you also add a test case like:

template <int N, int M>
void func(int (&array1)[N], int (&array2)[M]) {}

void instantiate() {
  int foo[12], bar[12];
  func(foo, bar);

  int baz[12], quux[42];
  func(baz, quux);
}

[whisperity]

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

46

(Implicitly) instantiated decls are excluded from the check as a whole, so this test case will not drive the check.

[whisperity]

Add a test case where the referenced arrays are deduced from template parameters.

[aaron.ballman]

LGTM

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

60

Thank you for adding the comment, I'd have otherwise assumed there was a bug with the first example in this function not warning. :-)

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

approximateStandardConversionSequence(const TheCheck &Check, QualType From,
   // Get out the qualifiers of the original type. This will always be
   // re-applied to the WorkType to ensure it is the same qualification as the
   // original From was.
   auto QualifiersToApply = From.split().Quals.getAsOpaqueValue();
 
   // LValue->RValue is irrelevant for the check, because it is a thing to be
   // done at a call site, and will be performed if need be performed.
 
-  // Array->Ptr decay.
-  if (const auto *ArrayT = dyn_cast<ArrayType>(From)) {
-    LLVM_DEBUG(llvm::dbgs() << "--- approximateStdConv. Array->Ptr decayed.\n");
-    WorkType = ArrayT->getPointeeType();
-  }
+  // Array->Pointer decay is handled by the main method in desugaring
+  // the parameter's DecayedType as "useless sugar".
 
   // Function->Pointer conversions are also irrelevant, because a
   // "FunctionType" cannot be the type of a parameter variable, so this
   // conversion is only meaningful at call sites.
 
   // Numeric promotions and conversions.
   const auto *FromBuiltin = WorkType->getAs<BuiltinType>();
   const auto *ToBuiltin = To->getAs<BuiltinType>();

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

 
 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.
 
+typedef int Point2D[2];
+typedef int Point3D[3];
+
+void arrays1(Point2D P2D, Point3D P3D) {} // In reality this is (int*, int*).
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'arrays1' of similar type ('int *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'P2D'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in the range is 'P3D'
+
+void crefToArrayTypedef1(int I, const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedef2(int *IA, const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedef3(int P1[2], const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedefBoth1(const Point2D &VecDescartes, const Point3D &VecThreeD) {}
+// NO-WARN: Distinct types.
+
+template <int N, int M>
+void templatedArrayRef(int (&Array1)[N], int (&Array2)[M]) {}
+// NO-WARN: Distinct template types in the primary template.
+
+void templatedArrayRefTest() {
+  int Foo[12], Bar[12];
+  templatedArrayRef(Foo, Bar);
+
+  int Baz[12], Quux[42];
+  templatedArrayRef(Baz, Quux);
+
+  // NO-WARN: Implicit instantiations are not checked.
+}
+
+template <>
+void templatedArrayRef(int (&Array1)[8], int (&Array2)[8]) { templatedArrayRef(Array2, Array1); }
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'templatedArrayRef<8, 8>' of similar type ('int (&)[8]') are
+// CHECK-MESSAGES: :[[@LINE-2]]:30: note: the first parameter in the range is 'Array1'
+// CHECK-MESSAGES: :[[@LINE-3]]:48: note: the last parameter in the range is 'Array2'
+
+template <>
+void templatedArrayRef(int (&Array1)[16], int (&Array2)[24]) {}
+// NO-WARN: Not the same type.
+
 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

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

 // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: 3 adjacent parameters of 'referenceThroughTypedef' of similar type are
 // CHECK-MESSAGES: :[[@LINE-2]]:34: note: the first parameter in the range is 'I'
 // CHECK-MESSAGES: :[[@LINE-3]]:62: note: the last parameter in the range is 'MyInt'
 // CHECK-MESSAGES: :[[@LINE-4]]:37: note: 'int' and 'ICRTy' parameters accept and bind the same kind of values
 // CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, 'int' and 'MyIntCRTy' are the same
 // CHECK-MESSAGES: :[[@LINE-6]]:52: note: 'int' and 'MyIntCRTy' parameters accept and bind the same kind of values
 // CHECK-MESSAGES: :[[@LINE-7]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'const int &'
 
+typedef int Point2D[2];
+typedef int Point3D[3];
+
+void arrays1(Point2D P2D, Point3D P3D) {} // In reality this is (int*, int*).
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'arrays1' of similar type ('int *') are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'P2D'
+// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in the range is 'P3D'
+
+void crefToArrayTypedef1(int I, const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedef2(int *IA, const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedef3(int P1[2], const Point2D &P) {}
+// NO-WARN.
+
+void crefToArrayTypedefBoth1(const Point2D &VecDescartes, const Point3D &VecThreeD) {}
+// NO-WARN: Distinct types and no conversion because of &.
+
 short const typedef int unsigned Eldritch;
 typedef const unsigned short Holy;
 
 void collapse(Eldritch Cursed, Holy Blessed) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'collapse' of similar type are
 // CHECK-MESSAGES: :[[@LINE-2]]:24: note: the first parameter in the range is 'Cursed'
 // CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in the range is 'Blessed'
 // CHECK-MESSAGES: :[[@LINE-4]]:15: note: after resolving type aliases, the common type of 'Eldritch' and 'Holy' is 'const unsigned short'

 template <typename T>
 using TwoOf = Pair<T, T>;
 
 void templateAndAliasTemplate(Pair<int, int> P, TwoOf<int> I) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 2 adjacent parameters of 'templateAndAliasTemplate' of similar type ('Pair<int, int>')
 // CHECK-MESSAGES: :[[@LINE-2]]:46: note: the first parameter in the range is 'P'
 // CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'I'
 
+template <int N, int M>
+void templatedArrayRef(int (&Array1)[N], int (&Array2)[M]) {}
+// NO-WARN: Distinct template types in the primary template.
+
+void templatedArrayRefTest() {
+  int Foo[12], Bar[12];
+  templatedArrayRef(Foo, Bar);
+
+  int Baz[12], Quux[42];
+  templatedArrayRef(Baz, Quux);
+
+  // NO-WARN: Implicit instantiations are not checked.
+}
+
+template <>
+void templatedArrayRef(int (&Array1)[8], int (&Array2)[8]) { templatedArrayRef(Array2, Array1); }
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'templatedArrayRef<8, 8>' of similar type ('int (&)[8]') are
+// CHECK-MESSAGES: :[[@LINE-2]]:30: note: the first parameter in the range is 'Array1'
+// CHECK-MESSAGES: :[[@LINE-3]]:48: note: the last parameter in the range is 'Array2'
+
+template <>
+void templatedArrayRef(int (&Array1)[16], int (&Array2)[24]) {}
+// NO-WARN: Not the same type.
+
 template <typename T>
 struct Vector {
   typedef T element_type;
   typedef T &reference_type;
   typedef const T const_element_type;
   typedef const T &const_reference_type;
 };