[clang-tidy] Add obvious module for obvious bugs

[Prazek]

Summary

Module for checks for obvious bugs that probably won't be found
in working code, but can be found while looking for an
obvious bug in broken code.

[Eugene.Zelenko]

I'm not sure that this is good name for module.

Singe reason for this is check for STL algorithms, may be stl module is more correct destination?

[Prazek]

In D27815#624294, @Eugene.Zelenko wrote:

I'm not sure that this is good name for module.

Singe reason for this is check for STL algorithms, may be stl module is more correct destination?

The reason for this module is that I want to make a bunch of clang-tidy checks that are designed to help programmer find a bug in the recently written code. These check probably won't find any bugs in projects, because with example of the first check, either:
a) the code is dead
b) code is correct because we use 2 references to the same vector

in any other case the app would crash, so it only make sense to run this check while programming and looking for the bugs.

Other example of the check I want to write:

for (int i = 0; i < n; i++) {
   for (int j = 0; i < n; j++) {

These are kinds of bugs that with 99% chances will be found during development, so the goal is to reduce programmer time ti find these kind of bugs.

[Eugene.Zelenko]

There are many other checks or compiler warnings which could be classified as obvious bugs.

[Prazek]

In D27815#624322, @Eugene.Zelenko wrote:

There are many other checks or compiler warnings which could be classified as obvious bugs.

Sure, but unfortunatelly they won't be able to become warning because of false positive rate.
In the example I mentioned this code could be totally valid.

[aaron.ballman]

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

[Prazek]

In D27815#625102, @aaron.ballman wrote:

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

My intention is to have kind of bugs that you facepalm when you find it. Mostly simple typos.
As I said this can't make it to clang because it would have some false positives.

I am ok with making group like bugs that would have some checks that are curently in misc (misc-use-after-move), but still, this group would be special in the sense that it looks for very simple bugs that you can make during coding, but you won't be able to find it in code that seem to work.

[aaron.ballman]

In D27815#625271, @Prazek wrote:

In D27815#625102, @aaron.ballman wrote:

I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that is run less frequently.

Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names:

bugs-
correctness-

My intention is to have kind of bugs that you facepalm when you find it. Mostly simple typos.
As I said this can't make it to clang because it would have some false positives.

Many warnings in clang have some false positives. Do you envision these having a fairly high false positive rate, or expensive to check for?

I am ok with making group like bugs that would have some checks that are curently in misc (misc-use-after-move), but still, this group would be special in the sense that it looks for very simple bugs that you can make during coding, but you won't be able to find it in code that seem to work.

I'm having a hard time imagining how many checks would be in this module from that description. Are you expecting to move items from misc to populate this module (if so, which ones, aside from misc-use-after-move?), or are you intending to add some checks we don't currently have (if so, can you list a few of them)? From your description, it sounds like these are not obvious bugs (since the code seems to work and the bugs are hard to spot), but it may be that "bugs" or "correctness" could also be a poor name. With some examples of checks that could go in the module, we may be able to pick a more appropriate name.

[• Quuxplusone]

The provided example (typoing "i" for "j") sounds like the sort of thing that PVS-Studio catches; maybe see what wording they use for that kind of mistake? Without investigating, I would suggest "cut-and-paste-error" or "likely-typo".

However, the attached patch *doesn't implement* the provided example diagnostic, so right now it's really hard to say what the intent of this patch *is*. It's hard to argue about whether the name "obvious-" is correct for this group of diagnostics when the group is, er, empty. I suggest that the way forward is to implement some of the intended diagnostics under an existing group and then later refactor them into a new group as needed; or else to implement so many of the intended diagnostics in *this* patch that the necessity for the new group is "obvious" :) to everyone involved.

Coincidentally, I was just reading https://en.wikipedia.org/wiki/WP:LEADFOLLOWSBODY last night. I think it's applicable here. :)

[Prazek]

The example of this kind of check is here:
https://reviews.llvm.org/D27806

I am not sure if it make sense to put it as clang warning.

After a little bit of thinking I guess name "typos" would be better, because I want to look for checks that are mostly typos (which are obvious btw) but it would make it more concrete.

[aaron.ballman]

In D27815#626440, @Prazek wrote:

The example of this kind of check is here:
https://reviews.llvm.org/D27806

I am not sure if it make sense to put it as clang warning.

After a little bit of thinking I guess name "typos" would be better, because I want to look for checks that are mostly typos (which are obvious btw) but it would make it more concrete.

When I hear "typo", I think "misspelling" and wonder how it differs from the typo correction that clang frontend has. I've usually heard of what your linked review covers as a "thinko" (https://en.wiktionary.org/wiki/thinko) because you made a logical mistake rather than a misspelling, but that's a pretty awful name for the module. ;-)

Perhaps "mistake-", "oops-", "derp-" or something along those lines (well, maybe not "derp")?

[• Quuxplusone]

PVS-Studio implements tons of checks of this variety. See e.g.
http://www.viva64.com/en/b/0299/#ID0E4KBM
They don't have a catchy name for the category either, but perhaps "suspicious-" or "copypaste-" would do.

I agree with Aaron that "thinko-" would be a little inappropriate for a user-facing name, and besides, I personally would not call this kind of error a "thinko". A thinko stems from a "brain fart", i.e. when you momentarily believe something that in hindsight is obviously false; e.g. writing code for a case that can't ever be hit in practice, or choosing the wrong algorithm because you misunderstood the problem to be solved. The kind of error I'm thinking of in this case stems from not thinking at all, e.g. incomplete copy-pasting, or writing (ch != 'A' || ch != 'a'), or whatever.

clang-tidy/CMakeLists.txt

 add_subdirectory(boost)
 add_subdirectory(cert)
 add_subdirectory(llvm)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)
 add_subdirectory(mpi)
+add_subdirectory(obvious)
 add_subdirectory(performance)
 add_subdirectory(readability)
 add_subdirectory(utils)

clang-tidy/obvious/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyObviousModule
+  ObviousTidyModule.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tidy/obvious/ObviousTidyModule.cpp

+//===------- ObviousTidyModule.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"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace obvious {
+
+class ObviousModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    // Add obvious checks here.
+  }
+};
+
+// Register the ObviousModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<ObviousModule> X("obvious-module",
+                                                     "Add obvious checks.");
+
+} // namespace obvious
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ObviousModule.
+volatile int ObviousModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/plugin/CMakeLists.txt

add_clang_library(clangTidyPlugin
   clangTidy
   clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
+  clangTidyObviousModule
   clangTidyMPIModule
   clangTidyPerformanceModule
   clangTidyReadabilityModule
   clangTooling
   )

clang-tidy/plugin/ClangTidyPlugin.cpp

 static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination =
     MiscModuleAnchorSource;
 
 // This anchor is used to force the linker to link the ModernizeModule.
 extern volatile int ModernizeModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ModernizeModuleAnchorDestination =
     ModernizeModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ObviousModule.
+extern volatile int ObviousModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ObviousModuleAnchorDestination =
+    ObviousModuleAnchorSource;
+
 // This anchor is used to force the linker to link the PerformanceModule.
 extern volatile int PerformanceModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =
     PerformanceModuleAnchorSource;
 
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
     ReadabilityModuleAnchorSource;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/tool/CMakeLists.txt

target_link_libraries(clang-tidy
   clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyMPIModule
+  clangTidyObviousModule
   clangTidyPerformanceModule
   clangTidyReadabilityModule
   clangTooling
   )
 
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
 install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
 install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)

clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED ModernizeModuleAnchorDestination =
     ModernizeModuleAnchorSource;
 
 // This anchor is used to force the linker to link the MPIModule.
 extern volatile int MPIModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED MPIModuleAnchorDestination =
     MPIModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ObviousModule.
+extern volatile int ObviousModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ObviousModuleAnchorDestination =
+  ObviousModuleAnchorSource;
+
 // This anchor is used to force the linker to link the PerformanceModule.
 extern volatile int PerformanceModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =
     PerformanceModuleAnchorSource;
 
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
     ReadabilityModuleAnchorSource;
 
 } // namespace tidy
 } // namespace clang
 
 int main(int argc, const char **argv) {
   return clang::tidy::clangTidyMain(argc, argv);
 }

docs/clang-tidy/index.rst

 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``    Clang Static Analyzer checks.
 ``google-``            Checks related to the Google coding conventions.
 ``llvm-``              Checks related to the LLVM coding conventions.
 ``misc-``              Checks that we didn't have a better category for.
 ``modernize-``         Checks that advocate usage of modern (currently "modern"
                        means "C++11") language constructs.
 ``mpi-``               Checks related to MPI (Message Passing Interface).
+``obvious-``           Checks for obvious bugs that probably won't be found
+                       in working code, but can be found while looking for an
+                       obvious bug in broken code.
 ``performance-``       Checks that target performance-related issues.
 ``readability-``       Checks that target readability-related issues that don't
                        relate to any particular coding style.
 ====================== =========================================================
 
 Clang diagnostics are treated in a similar way as check diagnostics. Clang
 diagnostics are displayed by :program:`clang-tidy` and can be filtered out using
 ``-checks=`` option. However, the ``-checks=`` option does not affect