[clang-tidy] Fix more false positives for bugprone-string-integer-assignment

[courbet]

Summary

And add various tests gleaned for our codebase.

See PR27723.

[alexfh]

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

50 ↗

(On Diff #190608)

No need for the top-level const in the first parameter.

70 ↗

(On Diff #190608)

nit: Please capitalize the first word.

115 ↗

(On Diff #190608)

I believe you should also check (or assert) that E is not instantiation-dependent before running the evaluator.

[courbet]

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

115 ↗

(On Diff #190608)

Interesting. AFAICT if I don't check that , I end up warning in the cases when the instantiation does result in a too large constant, which is what we want (the user should add a cast if they want to silence this).

[courbet]

Address review comments.

[alexfh]

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

115 ↗

(On Diff #190608)

IIUC, expression evaluation is just not supposed to be used on instantiation-dependent expressions. I've recently fixed a related crash (https://reviews.llvm.org/rL355401). I guess, there's a similar possibility here.

[courbet]

Ignore template contexts and add a test.

[courbet]

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

115 ↗

(On Diff #190608)

Thanks for the pointer.

[alexfh]

LG. Thanks for improving the check!

clang-tools-extra/trunk/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp

Finder->addMatcher(
                        // `basic_string<CodePoint>`
                        unless(hasType(qualType(
                            hasCanonicalType(equalsBoundNode("type"))))))
                       .bind("expr"))),
           unless(isInTemplateInstantiation())),
       this);
 }
 
-static bool isLikelyCharExpression(const Expr *Argument,
-                                   const ASTContext &Ctx) {
-  const auto *BinOp = dyn_cast<BinaryOperator>(Argument);
-  if (!BinOp)
-    return false;
+class CharExpressionDetector {
+public:
+  CharExpressionDetector(QualType CharType, const ASTContext &Ctx)
+      : CharType(CharType), Ctx(Ctx) {}
+
+  bool isLikelyCharExpression(const Expr *E) const {
+    if (isCharTyped(E))
+      return true;
+
+    if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
       const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
       const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
-  // <expr> & <mask>, mask is a compile time constant.
-  Expr::EvalResult RHSVal;
-  if (BinOp->getOpcode() == BO_And &&
-      (RHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects) ||
-       LHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects)))
+      // Handle both directions, e.g. `'a' + (i % 26)` and `(i % 26) + 'a'`.
+      if (BinOp->isAdditiveOp() || BinOp->isBitwiseOp())
+        return handleBinaryOp(BinOp->getOpcode(), LHS, RHS) ||
+               handleBinaryOp(BinOp->getOpcode(), RHS, LHS);
+      // Except in the case of '%'.
+      if (BinOp->getOpcode() == BO_Rem)
+        return handleBinaryOp(BinOp->getOpcode(), LHS, RHS);
+      return false;
+    }
+
+    // Ternary where at least one branch is a likely char expression, e.g.
+    //    i < 265 ? i : ' '
+    if (const auto *CondOp = dyn_cast<AbstractConditionalOperator>(E))
+      return isLikelyCharExpression(
+                 CondOp->getFalseExpr()->IgnoreParenImpCasts()) ||
+             isLikelyCharExpression(
+                 CondOp->getTrueExpr()->IgnoreParenImpCasts());
+    return false;
+  }
+
+private:
+  bool handleBinaryOp(clang::BinaryOperatorKind Opcode, const Expr *const LHS,
+                      const Expr *const RHS) const {
+    // <char_expr> <op> <char_expr> (c++ integer promotion rules make this an
+    // int), e.g.
+    //    'a' + c
+    if (isCharTyped(LHS) && isCharTyped(RHS))
       return true;
-  // <char literal> + (<expr> % <mod>), where <base> is a char literal.
-  const auto IsCharPlusModExpr = [](const Expr *L, const Expr *R) {
-    const auto *ROp = dyn_cast<BinaryOperator>(R);
-    return ROp && ROp->getOpcode() == BO_Rem && isa<CharacterLiteral>(L);
-  };
-  if (BinOp->getOpcode() == BO_Add) {
-    if (IsCharPlusModExpr(LHS, RHS) || IsCharPlusModExpr(RHS, LHS))
+
+    // <expr> & <char_valued_constant> or <expr> % <char_valued_constant>, e.g.
+    //    i & 0xff
+    if ((Opcode == BO_And || Opcode == BO_Rem) && isCharValuedConstant(RHS))
       return true;
-  }
+
+    // <char_expr> | <char_valued_constant>, e.g.
+    //    c | 0x80
+    if (Opcode == BO_Or && isCharTyped(LHS) && isCharValuedConstant(RHS))
+      return true;
+
+    // <char_constant> + <likely_char_expr>, e.g.
+    //    'a' + (i % 26)
+    if (Opcode == BO_Add)
+      return isCharConstant(LHS) && isLikelyCharExpression(RHS);
+
     return false;
   }
 
+  // Returns true if `E` is an character constant.
+  bool isCharConstant(const Expr *E) const {
+    return isCharTyped(E) && isCharValuedConstant(E);
+  };
+
+  // Returns true if `E` is an integer constant which fits in `CharType`.
+  bool isCharValuedConstant(const Expr *E) const {
+    if (E->isInstantiationDependent())
+      return false;
+    Expr::EvalResult EvalResult;
+    if (!E->EvaluateAsInt(EvalResult, Ctx, Expr::SE_AllowSideEffects))
+      return false;
+    return EvalResult.Val.getInt().getActiveBits() <= Ctx.getTypeSize(CharType);
+  };
+
+  // Returns true if `E` has the right character type.
+  bool isCharTyped(const Expr *E) const {
+    return E->getType().getCanonicalType().getTypePtr() ==
+           CharType.getTypePtr();
+  };
+
+  const QualType CharType;
+  const ASTContext &Ctx;
+};
+
 void StringIntegerAssignmentCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Argument = Result.Nodes.getNodeAs<Expr>("expr");
+  const auto CharType =
+      Result.Nodes.getNodeAs<QualType>("type")->getCanonicalType();
   SourceLocation Loc = Argument->getBeginLoc();
 
   // Try to detect a few common expressions to reduce false positives.
-  if (isLikelyCharExpression(Argument, *Result.Context))
+  if (CharExpressionDetector(CharType, *Result.Context)
+          .isLikelyCharExpression(Argument))
     return;
 
   auto Diag =
       diag(Loc, "an integer is interpreted as a character code when assigning "
                 "it to a string; if this is intended, cast the integer to the "
                 "appropriate character type; if you want a string "
                 "representation, use the appropriate conversion facility");
 
   if (Loc.isMacroID())
     return;
 
-  auto CharType = *Result.Nodes.getNodeAs<QualType>("type");
   bool IsWideCharType = CharType->isWideCharType();
   if (!CharType->isCharType() && !IsWideCharType)
     return;
   bool IsOneDigit = false;
   bool IsLiteral = false;
   if (const auto *Literal = dyn_cast<IntegerLiteral>(Argument)) {
     IsOneDigit = Literal->getValue().getLimitedValue() < 10;
     IsLiteral = true;

clang-tools-extra/trunk/test/clang-tidy/bugprone-string-integer-assignment.cpp

 // RUN: %check_clang_tidy %s bugprone-string-integer-assignment %t
 
 namespace std {
 template<typename T>
 struct basic_string {
   basic_string& operator=(T);
   basic_string& operator=(basic_string);
   basic_string& operator+=(T);
   basic_string& operator+=(basic_string);
+  const T &operator[](int i) const;
+  T &operator[](int i);
 };
 
 typedef basic_string<char> string;
 typedef basic_string<wchar_t> wstring;
 
 int tolower(int i);
 int toupper(int i);
 }
 
 int tolower(int i);
 int toupper(int i);
 
 typedef int MyArcaneChar;
 
+constexpr char kCharConstant = 'a';
+
 int main() {
   std::string s;
   std::wstring ws;
   int x = 5;
+  const char c = 'c';
 
   s = 6;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an integer is interpreted as a character code when assigning {{.*}} [bugprone-string-integer-assignment]
 // CHECK-FIXES: {{^}}  s = '6';{{$}}
   s = 66;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an integer is interpreted as a chara
 // CHECK-FIXES: {{^}}  s = "66";{{$}}
   s = x;
// CHECK-FIXES: {{^}}  ws += std::to_wstring(x);{{$}}
 
   std::basic_string<MyArcaneChar> as;
   as = 6;
   as = static_cast<MyArcaneChar>(6);
   as = 'a';
 
   s += toupper(x);
   s += tolower(x);
-  s += std::tolower(x);
+  s += (std::tolower(x));
+
+  s += c & s[1];
+  s += c ^ s[1];
+  s += c | s[1];
+
+  s[x] += 1;
+  s += s[x];
+  as += as[x];
 
   // Likely character expressions.
   s += x & 0xff;
   s += 0xff & x;
+  s += x % 26;
+  s += 26 % x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(26 % x);{{$}}
+  s += c | 0x80;
+  s += c | 0x8000;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(c | 0x8000);{{$}}
+  as += c | 0x8000;
 
   s += 'a' + (x % 26);
+  s += kCharConstant + (x % 26);
+  s += 'a' + (s[x] & 0xf);
   s += (x % 10) + 'b';
+
+  s += x > 255 ? c : x;
+  s += x > 255 ? 12 : x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(x > 255 ? 12 : x);{{$}}
+}
+
+namespace instantiation_dependent_exprs {
+template<typename T>
+struct S {
+  static constexpr T t = 0x8000;
+  std::string s;
+  void f(char c) { s += c | static_cast<int>(t); }
+};
+
+template S<int>;
 }