[clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t

[BRevzin]

Summary

Adds a check to clang-tidy to warn when streaming objects of type int8_t or uint8_t, when the likely intended behavior is to promote to int first.

[BRevzin]

So admittedly, I don't have any experience in clang-tidy. This check is intended to catch what is to us a really annoying source of error (and I'm probably not the only one?). I suspect there are far better ways of writing this check than the one that I pretty much guess-and-checked my way into.

[• Quuxplusone]

clang-tidy/bugprone/StreamInt8Check.cpp

45

I don't know clang-tidy style either, but might it be more appropriate to say something like "value of type int8_t will be printed as character, not number"? I had to go all the way down to the test cases in this patch before it occurred to me what the actual problem being diagnosed here was.

And speaking of "printed": do you care about streaming *in* values of these types with ">>"?

[Eugene.Zelenko]

clang-tidy/bugprone/StreamInt8Check.cpp

2

Please add real description, use space after it and shorten to 80 symbols.

26

May be match should be configurable, so other streams could be analyzed too? How about LLVM's raw_ostream?

37

Please don't use auto in cases where return type could not be easily deduced. See modernize-use-auto. Same for auto Underlying.

clang-tidy/bugprone/StreamInt8Check.h

2

Please add real description and use space after it.

20

Please write real description here.

docs/ReleaseNotes.rst

62

Please move to new checks section in alphabetical order and add one statement description (usually first statement from documentation).

test/clang-tidy/bugprone-stream-int8.cpp

33

Please use meaningful name or main.

[JonasToth]

Could you please add a test case with a template that reduces the type to int8 or uint8?

[JonasToth]

clang-tidy/bugprone/StreamInt8Check.cpp

27

You can use bind for the expr and get Offender from it.

42

The duplicated diag can be removed. You can use placeholders in diag and make the if include both types.
See https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp#L246

I think having a list of possible typedefnames would be nice. Some projects might not use the standard ones (e.g. Qt?).

docs/ReleaseNotes.rst

62

Since the branch occured it might be better to rebase first. Then the Release Notes are empty.

[BRevzin]

Updates based on review comments - and rebased off of latest so as to get the ReleaseNotes right. Added options so that the user can provide the list of stream types and int typedef types, as desired, defaulting to just basic_ostream and int8_t/uint8_t.

[BRevzin]

In D41740#968134, @JonasToth wrote:

Could you please add a test case with a template that reduces the type to int8 or uint8?

I don't actually know how to do that. I tried a few things, but getting the type of the expression through a template gets me directly to unsigned char, not to uint8_t.

[JonasToth]

In D41740#968567, @BRevzin wrote:

In D41740#968134, @JonasToth wrote:

Could you please add a test case with a template that reduces the type to int8 or uint8?

I don't actually know how to do that. I tried a few things, but getting the type of the expression through a template gets me directly to unsigned char, not to uint8_t.

Yes that is possible. Templates ignore the typedefnames. I think that needs to be considered. It might be relevant for a templated operator<<.

It would be nice to have these cases in tests to document the issue. A note in the docs wouldn't hurt too.

[JonasToth]

clang-tidy/bugprone/StreamInt8Check.h

20

Missing full stop in comment

[aaron.ballman]

clang-tidy/bugprone/BugproneTidyModule.cpp

64

I don't think this is a good name for the check, especially because the check can be configured for types other than integers. How about "bugprone-suspicious-stream-value-type"? If you rename the check name, you should also update the name of the files and check accordingly.

clang-tidy/bugprone/StreamInt8Check.cpp

25

This should be ::std::basic_ostream just in case someone does something awful like put namespace std inside another namespace.

48

It'd be nice to have a qualifier-agnostic way of expressing this (there are other qualifiers, like volatile, and we don't want to have to list them all out unless it's important for the check functionality).

56–57

You can use llvm::find(AliasTypes, Name) here instead.

It might also make sense to use an llvm::StringSet instead of a vector of strings that you have to do a linear search over.

63

const auto *?

64

Should not use auto here.

67

This diagnostic doesn't really explain what the issue is. It should probably be reworded to something more along the lines of "%0 will not undergo integral promotion prior to streaming; cast to 'int' to prevent streaming a character".

68

If you pass in Name, then it won't be properly quoted. You should pass Typedef->getDecl() (but not repeat the expression from above).

72–73

You can hoist this into the for loop.

clang-tidy/bugprone/StreamInt8Check.h

19

types -> type

docs/ReleaseNotes.rst

63

Extra space between the comma and "if".

I'd reword this a bit to:

Checks that an object of type int8_t or uint8_t is not streamed. Those types are usually a typedef to a character type that are printed as a character rather than an integer. The user likely intends to stream such a value as an int instead.

(Or something like this.)

docs/clang-tidy/checks/bugprone-stream-int8.rst

6–8

Same wording suggestions as above.

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "DanglingHandleCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
+#include "StreamInt8Check.h"
 #include "StringConstructorCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
         "bugprone-misplaced-operator-in-strlen-in-alloc");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<StreamInt8Check>(
+        "bugprone-stream-int8");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");

clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IntegerDivisionCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
+  StreamInt8Check.cpp
   StringConstructorCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/bugprone/StreamInt8Check.h

+//===--- StreamInt8Check.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_BUGPRONE_STREAMUINT8_TCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STREAMUINT8_TCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Checks that objects of types int8_t or uint8_t aren't streamed to ostreams,
+/// where the intended behavior is likely to stream them as ints
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-stream-int8.html
+class StreamInt8Check : public ClangTidyCheck {
+public:
+  StreamInt8Check(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  bool isBugproneAlias(StringRef Name) const;
+
+  const std::vector<std::string> StreamTypes;
+  const std::vector<std::string> AliasTypes;
+  const ast_matchers::internal::Matcher<Decl> IsAStream;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STREAMUINT8_TCHECK_H

clang-tidy/bugprone/StreamInt8Check.cpp

+//===--- StreamInt8Check.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 "StreamInt8Check.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+StreamInt8Check::StreamInt8Check(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context),
+    StreamTypes(utils::options::parseStringList(Options.get(
+      "StreamTypes",
+      "std::basic_ostream"))),
+    AliasTypes(utils::options::parseStringList(Options.get(
+      "AliasTypes",
+      "int8_t;uint8_t"))),
+    IsAStream(cxxRecordDecl(hasAnyName(std::vector<StringRef>(
+      StreamTypes.begin(), StreamTypes.end()))))
+{ }
+
+void StreamInt8Check::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StreamTypes",
+                utils::options::serializeStringList(StreamTypes));
+  Options.store(Opts, "AliasTypes",
+                utils::options::serializeStringList(AliasTypes));
+}
+
+void StreamInt8Check::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+    cxxOperatorCallExpr(
+      hasOverloadedOperatorName("<<"),
+      hasArgument(0, expr(hasType(qualType(hasCanonicalType(
+        hasDeclaration(IsAStream)))))),
+      hasArgument(1, expr(hasType(hasCanonicalType(
+        anyOf(asString("signed char"),
+              asString("const signed char"),
+              asString("unsigned char"),
+              asString("const unsigned char")))))
+        .bind("offender"))
+    ), this);
+}
+
+bool StreamInt8Check::isBugproneAlias(StringRef Name) const {
+  return std::find(AliasTypes.begin(), AliasTypes.end(), Name)
+         != AliasTypes.end();
+}
+
+void StreamInt8Check::check(const MatchFinder::MatchResult &Result) {
+  const auto *Offender = Result.Nodes.getNodeAs<Expr>("offender");
+
+  for (auto Typedef = Offender->getType().getTypePtr()->getAs<TypedefType>(); Typedef; ) {
+    auto Name = Typedef->getDecl()->getNameAsString();
+    if (isBugproneAlias(Name)) {
+      diag(Offender->getLocStart(),
+           "streaming %0; did you mean to stream an int?")
+        << Name << Offender->getSourceRange();
+      break;
+    }
+
+    QualType Underlying = Typedef->getDecl()->getUnderlyingType();
+    Typedef = Underlying.getTypePtr()->getAs<TypedefType>();
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
-- ...
+- New `bugprone-stream-int8_t
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-stream-int8_t.html>`_ check
+
+  Checks that objects of type ``int8_t`` or ``uint8_t`` aren't streamed,  if those
+  are typedefs to ``signed char`` or ``unsigned char``, as this likely intends to
+  be streaming these types as ``int`` s instead.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/bugprone-stream-int8.rst

+.. title:: clang-tidy - bugprone-stream-int8
+
+bugprone-stream-int8
+====================
+
+Checks that objects of type ``int8_t`` or ``uint8_t`` aren't streamed,  if those
+are typedefs to ``signed char`` or ``unsigned char``, as this likely intends to
+be streaming these types as ``int`` s instead.
+
+Examples:
+
+.. code-block:: c++
+
+  uint8_t value = 0;
+  std::cout << "Value is " << value; // prints ^@ instead of likely intended 0
+
+Options
+-------
+
+.. option:: StreamTypes
+
+   A semicolon-separated list of class names that should be treated as streams.
+   By default only ``std::basic_ostream`` is considered.
+
+.. option:: AliasTypes
+
+   A semicolon-separated list of alias names that should be treated as integer
+   types to check against. By default only ``int8_t`` and ``uint8_t`` are
+   considered.

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-dangling-handle
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
    bugprone-inaccurate-erase
    bugprone-integer-division
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
+   bugprone-stream-int8
    bugprone-string-constructor
    bugprone-suspicious-memset-usage
    bugprone-undefined-memory-manipulation
    bugprone-use-after-move
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp

test/clang-tidy/bugprone-stream-int8.cpp

+// RUN: %check_clang_tidy %s bugprone-stream-int8 %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: bugprone-stream-int8.AliasTypes, \
+// RUN:               value: 'int8_t; uint8_t; widget'}]}" \
+// RUN:   -- -std=c++11
+
+using int8_t = signed char;
+using uint8_t = unsigned char;
+using widget = unsigned char;
+
+namespace std {
+    template <typename CharT>
+    struct basic_ostream;
+
+    using ostream = basic_ostream<char>;
+
+    basic_ostream<char>& operator<<(basic_ostream<char>&, char );
+    basic_ostream<char>& operator<<(basic_ostream<char>&, unsigned char );
+    basic_ostream<char>& operator<<(basic_ostream<char>&, signed char );
+    basic_ostream<char>& operator<<(basic_ostream<char>&, int );
+}
+
+void BadStreaming(std::ostream& os, uint8_t i) {
+    // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: streaming uint8_t; did you mean to stream an int? [bugprone-stream-int8]
+    os << i;
+
+    // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: streaming uint8_t; did you mean to stream an int? [bugprone-stream-int8]
+    os << 4 << i;
+
+    // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: streaming int8_t; did you mean to stream an int? [bugprone-stream-int8]
+    os << int8_t{};
+
+    using Num = int8_t;
+    // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: streaming int8_t; did you mean to stream an int? [bugprone-stream-int8]
+    os << Num{};
+
+    // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: streaming widget; did you mean to stream an int? [bugprone-stream-int8]
+    os << widget{};
+}
+
+void GoodStreaming(std::ostream& os, uint8_t i) {
+    using UC = unsigned char;
+
+    unsigned char uc;
+    os << +i << 4 << uc << UC{};
+}