[clang-tidy] extend bugprone-signed-char-misuse check with array subscript case.

[ztamas]

Summary

To cover STR34-C rule's second use case, where `signed char` is
used for array subscript after an integer conversion. In the case
of non-ASCII character this conversion will result in a value
in excess of UCHAR_MAX.

There is another clang-tidy check which catches these cases.
cppcoreguidelines-pro-bounds-constant-array-index catches any
indexing which is not integer constant. I think this check is
very strict about the index (e.g. constant), so it's still useful
to cover the `signed char` use case in this check, so we
can provide a way to catch the SEI cert rule's use cases on a
codebase, where this CPP guideline is not used.

[ztamas]

I run the check both on LibreOffice and LLVM codebase but did not find any catch. I expect that it's not a frequent use case.
I added this extension only to fully cover the CERT rule. If this patch is accepted I can add a cert alias for this check.

[ztamas]

Fix typo

[aaron.ballman]

LGTM aside from some minor nits.

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

106

singed -> signed

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

36

unequality -> inequality

37

to integer before array subscript -> to an integer in the array subscript

[ztamas]

Spelling / grammar fixes.

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

const auto CompareOperator =
       expr(binaryOperator(hasAnyOperatorName("==", "!="),
                           anyOf(allOf(hasLHS(SignedCharCastExpr),
                                       hasRHS(UnSignedCharCastExpr)),
                                 allOf(hasLHS(UnSignedCharCastExpr),
                                       hasRHS(SignedCharCastExpr)))))
           .bind("comparison");
 
   Finder->addMatcher(CompareOperator, this);
+
+  // Catch array subscripts with singed char -> integer conversion.
+  // Matcher for C arrays.
+  const auto CArraySubscript =
+      arraySubscriptExpr(hasIndex(SignedCharCastExpr)).bind("arraySubscript");
+
+  Finder->addMatcher(CArraySubscript, this);
+
+  // Matcher for std arrays.
+  const auto STDArraySubscript =
+      cxxOperatorCallExpr(
+          hasOverloadedOperatorName("[]"),
+          hasArgument(0, hasType(cxxRecordDecl(hasName("::std::array")))),
+          hasArgument(1, SignedCharCastExpr))
+          .bind("arraySubscript");
+
+  Finder->addMatcher(STDArraySubscript, this);
 }
 
 void SignedCharMisuseCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *SignedCastExpression =
       Result.Nodes.getNodeAs<ImplicitCastExpr>("signedCastExpression");
+  const auto *IntegerType = Result.Nodes.getNodeAs<QualType>("integerType");
+  assert(SignedCastExpression);
+  assert(IntegerType);
 
   // Ignore the match if we know that the signed char's value is not negative.
   // The potential misinterpretation happens for negative values only.
   Expr::EvalResult EVResult;
   if (!SignedCastExpression->isValueDependent() &&
       SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
                                                         *Result.Context)) {
     llvm::APSInt Value = EVResult.Val.getInt();
if (!UnSignedCastExpression->isValueDependent() &&
                                                             *Result.Context)) {
       llvm::APSInt Value = EVResult.Val.getInt();
       if (Value <= UnsignedASCIIUpperBound)
         return;
     }
 
     diag(Comparison->getBeginLoc(),
          "comparison between 'signed char' and 'unsigned char'");
-  } else if (const auto *IntegerType =
-                 Result.Nodes.getNodeAs<QualType>("integerType")) {
+  } else if (Result.Nodes.getNodeAs<Expr>("arraySubscript")) {
+    diag(SignedCastExpression->getBeginLoc(),
+         "'signed char' to %0 conversion in array subscript; "
+         "consider casting to 'unsigned char' first.")
+        << *IntegerType;
+  } else {
     diag(SignedCastExpression->getBeginLoc(),
          "'signed char' to %0 conversion; "
          "consider casting to 'unsigned char' first.")
         << *IntegerType;
-  } else
-    llvm_unreachable("Unexpected match");
+  }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst

 For ``signed char``, the non-ASCII characters are stored as a value in [-128..-1]
 interval, while the same characters are stored in the [128..255] interval for
 an ``unsigned char``.
 
 It depends on the actual platform whether plain ``char`` is handled as ``signed char``
 by default and so it is caught by this check or not. To change the default behavior
 you can use ``-funsigned-char`` and ``-fsigned-char`` compilation options.
 
-Currently, this check is limited to assignments and variable declarations,
-where a ``signed char`` is assigned to an integer variable and to
-equality/inequality comparisons between ``signed char`` and ``unsigned char``.
-There are other use cases where the unexpected value ranges might lead to
-similar bogus behavior.
+Currently, this check warns in the following cases:
+- ``signed char`` is assgined to an integer variable
+- ``signed char`` and ``unsigned char`` are compared with equality/unequality operator
+- ``signed char`` is converted to integer before array subscript
 
 See also:
 `STR34-C. Cast characters to unsigned char before converting to larger integer sizes
 <https://wiki.sei.cmu.edu/confluence/display/c/STR34-C.+Cast+characters+to+unsigned+char+before+converting+to+larger+integer+sizes>`_
 
 A good example from the CERT description when a ``char`` variable is used to
 read from a file that might contain non-ASCII characters. The problem comes
 up when the code uses the ``-1`` integer value as EOF, while the 255 character

clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp

 // RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t
 
 ///////////////////////////////////////////////////////////////////
 /// Test cases correctly caught by the check.
 
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename T, size_t N>
+struct array {
+  T &operator[](size_t n);
+  T &at(size_t n);
+};
+} // namespace std
+
 int SimpleVarDeclaration() {
   signed char CCharacter = -5;
   int NCharacter = CCharacter;
   // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
 
   return NCharacter;
 }
 

 
 int CompareWithUnsignedNonAsciiConstant(signed char SCharacter) {
   const unsigned char USCharacter = 128;
   if (USCharacter == SCharacter) // CHECK-MESSAGES: [[@LINE]]:7: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
     return 1;
   return 0;
 }
 
+int SignedCharCArraySubscript(signed char SCharacter) {
+  int Array[3] = {1, 2, 3};
+
+  return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+}
+
+int SignedCharSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) {
+  return Array[static_cast<unsigned int>(SCharacter)]; // CHECK-MESSAGES: [[@LINE]]:42: warning: 'signed char' to 'unsigned int' conversion in array subscript; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+}
+
 ///////////////////////////////////////////////////////////////////
 /// Test cases correctly ignored by the check.
 
 int UnsignedCharCast() {
   unsigned char CCharacter = 'a';
   int NCharacter = CCharacter;
 
   return NCharacter;

 }
 
 int CompareWithUnsignedAsciiConstant(signed char SCharacter) {
   const unsigned char USCharacter = 'a';
   if (USCharacter == SCharacter)
     return 1;
   return 0;
 }
+
+int UnsignedCharCArraySubscript(unsigned char USCharacter) {
+  int Array[3] = {1, 2, 3};
+
+  return Array[static_cast<unsigned int>(USCharacter)];
+}
+
+int CastedCArraySubscript(signed char SCharacter) {
+  int Array[3] = {1, 2, 3};
+
+  return Array[static_cast<unsigned char>(SCharacter)];
+}
+
+int UnsignedCharSTDArraySubscript(std::array<int, 3> Array, unsigned char USCharacter) {
+  return Array[static_cast<unsigned int>(USCharacter)];
+}
+
+int CastedSTDArraySubscript(std::array<int, 3> Array, signed char SCharacter) {
+  return Array[static_cast<unsigned char>(SCharacter)];
+}