[clang-tidy] Add misc-default-numerics

[Prazek]

Summary

This check flags usages of `std::numeric_limits<T>::{min,max}()` for
unspecialized types. It is dangerous because returns T(), which might is
rarely
minimum or maximum for this type.

Consider scenario:

1. Have typedef long long BigInt in source code
2. Use std::numeric_limits<BigInt>::min()
3. Replace the BigInt typedef with class implementing BigIntegers
4. Compile silently your code and find it few years later that you have

of by
9223372036854775808 error.

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-default-numerics.rst

12

May be code-block will be better?

[Prazek]

docs/clang-tidy/checks/misc-default-numerics.rst

12

Right, I was looking for something like an inline code block, but I guess I will just change it to code block with comments

[Prazek]

• fixed docs

[aaron.ballman]

clang-tidy/misc/DefaultNumericsCheck.cpp

38

Can remove the spurious newline.

42

This diagnostic doesn't tell the user what's wrong with their code. I think something along these lines might be better:

'std::numeric_limits::%select{max|min}0' called with type %1; no such specialization exists, so the default value for that type is returned

clang-tidy/misc/DefaultNumericsCheck.h

21

because returns -> because it returns

docs/clang-tidy/checks/misc-default-numerics.rst

8

because returns... -> because the calls return ''T()'' in this case, which is unlikely to represent the minimum or maximum value for the type.

20

Remove spurious newline

25

Is this meant to say "off by X error"? I think a better wording would be that the code continues to compile, but the call to min() returns BigInt{}, or something more explicit.

[Prazek]

• Thanks for the review Aaron, that is much better.

[aaron.ballman]

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

clang-tidy/misc/DefaultNumericsCheck.cpp

31

This should be checking for ::std::numeric_limits (plural).

docs/ReleaseNotes.rst

83

numeric_limits

test/clang-tidy/misc-default-numerics.cpp

7

numeric_limits (same elsewhere in this file).

[alexander-shaposhnikov]

clang-tidy/misc/DefaultNumericsCheck.h

21

nit: (feel free to correct me)
replace
"which might is ..."
with
"which rarely might be minimum or maximum for this type"

[Prazek]

• fixed docs
• fixes

[Prazek]

In D33470#764846, @aaron.ballman wrote:

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

[aaron.ballman]

In D33470#789791, @Prazek wrote:

In D33470#764846, @aaron.ballman wrote:

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

My concern is: does this find any actual issues in real world code? This seems like such a highly specific check -- not many people use numeric_limits in the first place, let alone on non-builtin types, so does it justify running this check when someone batch-includes all of the misc checks?

I don't think this check is going to trigger a ton of false positives. I am wondering more the opposite: will this check ever trigger on anything other than compiler test cases?

clang-tidy/misc/DefaultNumericsCheck.h

22

the minimum or maximum (add the "the").

test/clang-tidy/misc-default-numerics.cpp

33

Can you add a test case where numeric_limits has been properly specialized for the type and the type is not a builtin?

[Prazek]

In D33470#790484, @aaron.ballman wrote:

In D33470#789791, @Prazek wrote:

In D33470#764846, @aaron.ballman wrote:

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

My concern is: does this find any actual issues in real world code? This seems like such a highly specific check -- not many people use numeric_limits in the first place, let alone on non-builtin types, so does it justify running this check when someone batch-includes all of the misc checks?

I don't think this check is going to trigger a ton of false positives. I am wondering more the opposite: will this check ever trigger on anything other than compiler test cases?

The check is based on the real world scenario that my friend had at work, so at least I know about one such case :)
It probably won't be a very popular check in terms of fiding anything, but it should find real bugs. The matcher is very easy so the check should be fast.

[Prazek]

• fixed docs
• fixes
• Last fixes?

[Prazek]

Small fix

[Prazek]

fixed broken test

[aaron.ballman]

test/clang-tidy/misc-default-numerics.cpp

28

This is not a proper specialization according to the standard.

A program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited.

The functions are not marked constexpr or noexcept, and min() must return SpecializedType. Also the is_specialized member needs to be set to true. There are other requirements missing as well, but I think fixing the function signatures is the only important one.

[alexfh]

Should the check be in "bugprone-" instead?

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  DefaultNumericsCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   LambdaFunctionNameCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp

clang-tidy/misc/DefaultNumericsCheck.h

+//===--- DefaultNumericsCheck.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_DEFAULT_NUMERICS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFAULT_NUMERICS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check flags usages of ``std::numeric_limits<T>::{min,max}()`` for
+/// unspecialized types. It is dangerous because it returns T(), which rarely
+/// might be the minimum or the maximum for this type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-default-numerics.html
+class DefaultNumericsCheck : public ClangTidyCheck {
+public:
+  DefaultNumericsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFAULT_NUMERICS_H

clang-tidy/misc/DefaultNumericsCheck.cpp

+//===--- DefaultNumericsCheck.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 "DefaultNumericsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void DefaultNumericsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  // FIXME: We should also warn in template intantiations, but it would require
+  // printing backtrace.
+  Finder->addMatcher(
+      callExpr(
+          callee(cxxMethodDecl(
+              hasAnyName("min", "max"),
+              ofClass(classTemplateSpecializationDecl(
+                  hasName("::std::numeric_limits"),
+                  unless(isExplicitTemplateSpecialization()),
+                  hasTemplateArgument(0, templateArgument().bind("type")))))),
+          unless(isInTemplateInstantiation()))
+          .bind("call"),
+      this);
+}
+
+void DefaultNumericsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto &MatchedCall = *Result.Nodes.getNodeAs<CallExpr>("call");
+  const auto &NumericType = *Result.Nodes.getNodeAs<TemplateArgument>("type");
+  bool IsMax = MatchedCall.getCalleeDecl()->getAsFunction()->getName() == "max";
+
+  diag(MatchedCall.getLocStart(),
+       "'std::numeric_limits::%select{min|max}0' called with type %1; no such "
+       "specialization exists, so the default value for that type is returned")
+      << IsMax << NumericType;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DefaultNumericsCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "LambdaFunctionNameCheck.h"

 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
+    CheckFactories.registerCheck<DefaultNumericsCheck>("misc-default-numerics");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "misc-forwarding-reference-overload");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "misc-lambda-function-name");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(

docs/ReleaseNotes.rst

- New `cert-dcl58-cpp
 
   Finds modification of the ``std`` or ``posix`` namespace.
 
 - Improved `cppcoreguidelines-no-malloc
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
 
   Allow custom memory management functions to be considered as well.
 
+- New `misc-default-numerics
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-default-numerics.html>`_ check
+  Finds uses of ``std::numeric_limits<T>`` for unspecialized types
+
 - New `misc-forwarding-reference-overload
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-forwarding-reference-overload.html>`_ check
 
   Finds perfect forwarding constructors that can unintentionally hide copy or move constructors.
 
 - New `misc-lambda-function-name <http://clang.llvm.org/extra/clang-tidy/checks/misc-lambda-function-name.html>`_ check
 
   Finds uses of ``__func__`` or ``__FUNCTION__`` inside lambdas.

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
+   misc-default-numerics
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
    misc-forwarding-reference-overload
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-lambda-function-name

docs/clang-tidy/checks/misc-default-numerics.rst

+.. title:: clang-tidy - misc-default-numerics
+
+misc-default-numerics
+=====================
+
+This check flags usages of ``std::numeric_limits<T>::{min,max}()`` for
+unspecialized types. It is dangerous because the calls return ``T()``
+in this case, which is unlikely to represent the minimum or maximum value for
+the type.
+
+Consider scenario:
+.. code-block:: c++
+
+  // 1. Have a:
+  typedef long long BigInt
+
+  // 2. Use
+  std::numeric_limits<BigInt>::min()
+
+  // 3. Replace the BigInt typedef with class implementing BigIntegers
+  class BigInt { ;;; };
+
+  // 4. Your code continues to compile, but the call to min() returns BigInt{}

test/clang-tidy/misc-default-numerics.cpp

+// RUN: %check_clang_tidy %s misc-default-numerics %t
+
+namespace std {
+
+template <typename T>
+struct numeric_limits {
+  static T min() { return T(); }
+  static T max() { return T(); }
+};
+
+template <>
+struct numeric_limits<int> {
+  static int min() { return -1; }
+  static int max() { return 1; }
+};
+
+} // namespace std
+
+class MyType {};
+template <typename T>
+class MyTemplate {};
+
+class SpecializedType {};
+
+namespace std {
+template<>
+struct numeric_limits<SpecializedType> {
+  static int min() { return -1; }
+  static SpecializedType max() { return SpecializedType(); }
+
+};
+}
+
+void test() {
+  auto x = std::numeric_limits<int>::min();
+
+  auto y = std::numeric_limits<MyType>::min();
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: 'std::numeric_limits::min' called with type 'MyType'; no such specialization exists, so the default value for that type is returned
+
+  auto z = std::numeric_limits<MyTemplate<int>>::max();
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: 'std::numeric_limits::max' called with type 'MyTemplate<int>';
+
+  auto a = std::numeric_limits<SpecializedType>::min();
+  auto b = std::numeric_limits<SpecializedType>::max();
+}
+
+template <typename T>
+void fun() {
+  auto x = std::numeric_limits<T>::min();
+}
+
+void testTemplate() {
+  fun<int>();
+  // FIXME: This should generate warning with backtrace.
+  fun<MyType>;
+}