[clang-tidy] Check for suspicious string assignments.

[xazax.hun]

Summary

It is possible to assign arbitrary integer types to strings. Sometimes it is the result of missing to_string call or apostrophes.

This check aims to find such errors.

[xazax.hun]

Minor update to docs.

[alexfh]

That's quite a surprising behavior. Looks like a bug in the library implementation (or in the standard) to me.

But the check is awesome. Thank you!

clang-tidy/misc/StringAssignmentCheck.cpp

21

Please only register the matcher for C++ code (see almost any other check for an example). It also seems that you don't need the Matcher variable.

33

The isIntegerType() check can be moved to the matcher. I'd also add a matcher for isAnyCharacterType and move that check to the matcher as well. The latter is fine for a follow-up.

36

Instead of storing the fixits in an array, I'd organize the code as follows:

auto Diag = diag(...);
if (!Loc.isMacroID()) {
  Diag << FixItHint::CreateInsertion(...) << FixItHint::CreateInsertion(...);
}

38

Please check that we're in C++11 mode before suggesting this fix.

clang-tidy/misc/StringAssignmentCheck.h

1

I suggest making the name more specific, e.g. StringIntegerAssignmentCheck. Same for the check name (misc-string-integer-assignment).

[alexfh]

BTW, Richard, is the possibility of assignment of an integer to a std::string a bug in the standard?

[xazax.hun]

• Modified the patch according to the review.

[xazax.hun]

Thank you for the review. I have seen a few false positives in the llvm codebase. I agree that these kind of conversions are unfortunate but I am sure fixing this would cause a huge backward compatibility problem. Nontheless I also think that it might be something that is worth to consider.

[alexfh]

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

45 ↗

(On Diff #42427)

I'd expand the message with an explanation of what actually happens here. Also it's not clear what the "to_string operation" refers to. And one more thing I missed, is other character types.

So I'd go with something similar to: "an integer is interpreted as a character code when assigning it to a string; if this is intended, cast the integer to '%0'; if you want a string representation, use %select{std::to_string()|std::to_wstring()|appropriate conversion facility}1", where %0 should be substituted with char/wchar_t/whatever the specific character type is.

(and please add tests for other character types)

[aaron.ballman]

In D15411#307030, @alexfh wrote:

BTW, Richard, is the possibility of assignment of an integer to a std::string a bug in the standard?

This is the result of:

basic_string& operator=(charT c);
Returns: *this = basic_string(1,c).

So I believe it is by design.

[alexfh]

In D15411#307274, @aaron.ballman wrote:

In D15411#307030, @alexfh wrote:

BTW, Richard, is the possibility of assignment of an integer to a std::string a bug in the standard?

This is the result of:

basic_string& operator=(charT c);
Returns: *this = basic_string(1,c).

So I believe it is by design.

I see how the current behavior can be explained by the standard, however I'm not sure this was the intention of the standard to specifically allow this behavior.

And if it's unintended (and I'd say undesired), the standard could also be changed to enforce a mechanism to disallow assignment of std::string from other integer types (e.g. a templated deleted operator= enable_if'd for all integral types except for charT, or something like that).

Maybe it wouldn't be reasonable for some reason, but it's worth trying ;)

[aaron.ballman]

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

19 ↗

(On Diff #42427)

I think this would be reasonable to add to ASTMatchers.h, but with the name isAnyCharacterType.

44 ↗

(On Diff #42427)

Would it make sense to allow = 0 or += 0 for null terminators? Requiring '\0' isn't the end of the world, but I think 0 is pretty common for that particular case.

46 ↗

(On Diff #42427)

I don't think to_string() is required for all cases. For instance:

s += 12; // --> s += "12";
s = 32; // --> s = ' '; or s = "32";
s = 4; // --> s = '4'; or s = "4";
s += some_integer; // --> s += std::to_string(some_integer);

This is also currently doing the wrong thing for std::wstring objects, or std::basic_string<char32_t>, etc.

clang-tidy/misc/StringIntegerAssignmentCheck.h

18 ↗

(On Diff #42427)

"where an integer" instead of "where integer".

docs/clang-tidy/checks/misc-string-integer-assignment.rst

9 ↗

(On Diff #42427)

Can remove the comma.

10 ↗

(On Diff #42427)

"It is possible to assign an integer"

test/clang-tidy/misc-string-integer-assignment.cpp

39 ↗

(On Diff #42427)

I would also like to see some tests for std::wstring.

[LegalizeAdulthood]

test/clang-tidy/misc-string-integer-assignment.cpp

25 ↗

(On Diff #42427)

Use {} initialization perhaps?

[xazax.hun]

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

19 ↗

(On Diff #42427)

I choose the name isAnyCharacter to be consistent with isInteger which also lacks the Type suffix. Do you still prefer the isAnyCharacterType name?

44 ↗

(On Diff #42427)

The = 0 or += 0 will not compile due to ambiguous overload (it could both be converted to const CharT * or CharT).

46 ↗

(On Diff #42427)

I think it might be even better to not to suggest fixits in this case, since there are some false positives. But if you think it is ok to have fixits I will come up some heuristics like:
For one digit constants use 'const' or L'const'.
For more than one digit constants use "const" or L"const".
For unknown character types do not recommend fixits.
For other cases and C++11 use to_string or to_wstring.

[xazax.hun]

• Addressed review comments.
• Removed fixit suggestions.

[alexfh]

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

20 ↗

(On Diff #42698)

I prefer isAnyCharacter for consistency with isInteger. I'd also suggest to move the matcher to ASTMatchers.h eventually.

47 ↗

(On Diff #42698)

I'd suggest to still add fixits. The heuristics you suggest seems reasonable. Fine for a follow-up.

docs/clang-tidy/checks/misc-string-integer-assignment.rst

2 ↗

(On Diff #42698)

nit: Please add more =s

9 ↗

(On Diff #42698)

"The source of the problem" seems slightly confusing here (we didn't yet define "the problem"). I'd say slightly differently:

The check finds assignments of an integer to ``std::basic_string<T>`` (``std::string``, ``std::wstring``, etc.). The source of the problem is the following assignment operator of ``std::basic_string``:

.. code:: c++
  basic_string& operator=( CharT ch );

and the fact that numeric types can be implicitly converted to most character types.

test/clang-tidy/misc-string-integer-assignment.cpp

7 ↗

(On Diff #42698)

I don't think the definitions of the methods are helpful here.

26 ↗

(On Diff #42698)

Why do you need to initialize the strings?

[aaron.ballman]

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

20 ↗

(On Diff #42698)

I prefer isAnyCharacter for consistency with isInteger. I'd also suggest to move the matcher to ASTMatchers.h eventually.

(CCing in Manuel for his opinion as well.)

I like the consistency idea, but it was my understanding that we want all AST matcher functions to match the C++ API they are exposing, which is why I still have a slight preference for isAnyCharacterType().

45 ↗

(On Diff #42698)

The = 0 or += 0 will not compile due to ambiguous overload (it could both be converted to const CharT * or CharT).

Excellent point. :-)

47 ↗

(On Diff #42698)

I'd suggest to still add fixits. The heuristics you suggest seems reasonable. Fine for a follow-up.

Agreed.

[xazax.hun]

• Reimplemented fixits.
• Addressed the review comments.

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

20 ↗

(On Diff #42698)

Adding this checker to ASTMatchers.h seems like a trivial modification. I think I will just add it and alter this patch once this check is accepted and you decided how to call it.

test/clang-tidy/misc-string-integer-assignment.cpp

26 ↗

(On Diff #42698)

I do not need to initialize them. I made the tests more minimal.

[LegalizeAdulthood]

test/clang-tidy/misc-string-integer-assignment.cpp

33 ↗

(On Diff #42710)

What happens if the test code had used static_cast<char>?

46 ↗

(On Diff #42710)

Use static_cast<> instead of C-style cast?

[alexfh]

Looks good with a few nits.

Thank you for the new awesome check!

clang-tidy/misc/StringIntegerAssignmentCheck.cpp

74 ↗

(On Diff #42710)

Don't use else after a return.

test/clang-tidy/misc-string-integer-assignment.cpp

27 ↗

(On Diff #42710)

I'd also remove the check name from all CHECK lines after the first one to fit the CHECK lines to 80 columns.

46 ↗

(On Diff #42710)

I think, both casts should be tested at least once. And it doesn't matter what cast style is used in the rest of the test.

[xazax.hun]

In D15411#310539, @alexfh wrote:

Thank you for the new awesome check!

Thank you for the review. I committed the new matcher to clang as well. I went with isAnyCharacter name for now, but we can rename it anytime.

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MiscTidyModule.cpp
   MoveConstantArgumentCheck.cpp 
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
+  StringAssignmentCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
+#include "StringAssignmentCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 
CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
+    CheckFactories.registerCheck<StringAssignmentCheck>(
+        "misc-string-assignment");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");

clang-tidy/misc/StringAssignmentCheck.h

+//===--- StringAssignmentCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_ASSIGNMENT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_ASSIGNMENT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Finds instances where integer is assigned to a string.
+///
+/// For more details see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-assignment.html
+class StringAssignmentCheck : public ClangTidyCheck {
+public:
+  StringAssignmentCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_ASSIGNMENT_H
+

clang-tidy/misc/StringAssignmentCheck.cpp

+//===--- StringAssignmentCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StringAssignmentCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void StringAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher = cxxOperatorCallExpr(
+      anyOf(hasOverloadedOperatorName("="), hasOverloadedOperatorName("+=")),
+      callee(cxxMethodDecl(ofClass(hasName("::std::basic_string")))),
+      hasArgument(1, ignoringImpCasts(expr().bind("expr"))),
+      unless(isInTemplateInstantiation()));
+  Finder->addMatcher(Matcher, this);
+}
+
+void StringAssignmentCheck::check(const MatchFinder::MatchResult &Result) {
+  auto Argument = Result.Nodes.getNodeAs<Expr>("expr");
+  QualType ArgType = Argument->getType();
+
+  if (!ArgType->isIntegerType() || ArgType->isAnyCharacterType())
+    return;
+  SourceLocation Loc = Argument->getLocStart();
+  SmallVector<FixItHint, 2> Hints;
+  if (!Loc.isMacroID()) {
+    Hints.push_back(FixItHint::CreateInsertion(Loc, "std::to_string("));
+    Hints.push_back(FixItHint::CreateInsertion(
+        Lexer::getLocForEndOfToken(Argument->getLocEnd(), 0,
+                                   *Result.SourceManager,
+                                   Result.Context->getLangOpts()),
+        ")"));
+  }
+
+  diag(Loc, "probably missing to_string operation") << Hints;
+}
+
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
+   misc-string-assignment
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    modernize-loop-convert

docs/clang-tidy/checks/misc-string-assignment.rst

+misc-string-assignment
+======================
+
+The ``std::basic_string`` has an assignment operator with the following signature:
+
+.. code:: c++
+  basic_string& operator=( CharT ch );
+
+The source of the problem is that, the numeric types can be implicity casted to most of the
+character types. It possible to assign integers to ``basic_string``.
+
+.. code:: c++
+  std::string s;
+  int x = 5965;
+  s = 6;
+  s = x;
+
+Use the appropriate conversion functions or character literals.
+
+.. code:: c++
+  std::string s;
+  int x = 5965;
+  s = '6';
+  s = std::to_string(x);
+
+In order to suppress false positives, use an explicit cast.
+
+.. code:: c++
+  std::string s;
+  s = static_cast<char>(6);

test/clang-tidy/misc-string-assignment.cpp

+// RUN: %check_clang_tidy %s misc-string-assignment %t
+
+namespace std {
+template<typename T>
+struct basic_string {
+  basic_string(const T*) {}
+  basic_string& operator=(T) {
+    return *this;
+  }
+  basic_string& operator=(basic_string) {
+    return *this;
+  }
+  basic_string& operator+=(T) {
+    return *this;
+  }
+  basic_string& operator+=(basic_string) {
+    return *this;
+  }
+};
+
+typedef basic_string<char> string;
+}
+
+int main() {
+  std::string s("foobar");
+
+  s = 6;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: probably missing to_string operation [misc-string-assignment]
+// CHECK-FIXES: {{^}}  s = std::to_string(6);{{$}}
+  s = 'c';
+  s = (char)6;
+
+// +=
+  s += 6;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: probably missing to_string operation [misc-string-assignment]
+// CHECK-FIXES: {{^}}  s += std::to_string(6);{{$}}
+  s += 'c';
+  s += (char)6;
+}