[clang-tidy] Check to find unintended semicolons that changes the semantics.

[xazax.hun]

Summary

This patch adds a checker to find semicolons that are probably unintended and modify the semantics of the program. See the examples and the documentation for more details.

[kimgr]

Cool check, I like how it pays attention to indentation!

I found some minor doc nits, see inline.

docs/clang-tidy/checks/misc-suspicious-semicolon.rst

8–9 ↗

(On Diff #45869)

"context" looks like it would fit on the wrapped line.

20 ↗

(On Diff #45869)

incremented?

29 ↗

(On Diff #45869)

Typo: identation

71 ↗

(On Diff #45869)

There's been some preference for "check" over "checker" lately.

test/clang-tidy/misc-suspicious-semicolon.cpp

88 ↗

(On Diff #45869)

Weird pointer alignment is a little distracting here, better stick with LLVM convention and attach it to s?

[xazax.hun]

• Fixed the nits pointed out by the review.

[alexfh]

Sorry for the long delay. Currently rather swamped.

Haojian, could you take a look at this patch?

[hokein]

clang-tidy/misc/SuspiciousSemicolonCheck.cpp

23 ↗

(On Diff #45949)

Looks like this check doesn't handle the case that unintended semicolon is in else statement.

if (condition1) {
} else if (condition2);
  a = 2

test/clang-tidy/misc-suspicious-semicolon.cpp

28 ↗

(On Diff #45949)

Can you add the following if statement cases in the test?

if (condition)
  ;

if (condition)
  ;
else {
}

The behavior of the check is that both two cases are not warned. But I think we should warn the first one since there is no reason to write if code for that.

[xazax.hun]

• Fixed the cases pointed out by the review.

[xazax.hun]

Thank you, good catches!

[hokein]

docs/clang-tidy/checks/misc-suspicious-semicolon.rst

35 ↗

(On Diff #47316)

The doc needs to be updated.

With your latest patch, this is also a warning case.

[xazax.hun]

docs/clang-tidy/checks/misc-suspicious-semicolon.rst

35 ↗

(On Diff #47316)

The documentation implies that this is a warning case but I will make it clearer to be less confusing.

[xazax.hun]

• Documentation clarification.

[hokein]

LGTM, thanks for working on this! Ping @alexfh

[alexfh]

LG. Thanks!

Could you run the check on LLVM and post here a summary of results: how many warnings are generated, whether there are any false positives (based on a reasonably-sized random sample, if there are too many warnings to inspect all relevant code), etc.

[xazax.hun]

In D16535#348469, @alexfh wrote:

Could you run the check on LLVM and post here a summary of results: how many warnings are generated, whether there are any false positives (based on a reasonably-sized random sample, if there are too many warnings to inspect all relevant code), etc.

This check did not give any results for LLVM codebase. I did not see any false positive on other projects yet, so I assume the false positive rate should be low (but might depend on coding style).

[omtcyf0]

@xazax.hun did you actually run the tool on the LLVM codebase?

Because this check generates tons of false-positive reports during codebase analysis.

See the minimal example below.

omtcyf0-laptop:playground omtcyf0$ cat main.cpp 
#include <vector>

int main() {
  std::vector<int> numbers = {1, 2, 3, 4, 5, 6};
  for (std::vector<int>::iterator it = std::begin(numbers),
                                  end = std::end(numbers);
       it != end; ++it) {
    (*it)++;
  }
  return 0;
}
omtcyf0-laptop:playground omtcyf0$ /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy -checks=misc-suspicious-semicolon main.cpp 
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "main.cpp"
No compilation database found in /Users/omtcyf0/Documents/dev/playground or any parent directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1 warning and 1 error generated.
Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp.
/Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: potentially unintended semicolon [misc-suspicious-semicolon]
       it != end; ++it) {
                ^
/usr/include/wchar.h:89:10: error: 'stdarg.h' file not found [clang-diagnostic-error]
#include <stdarg.h>
         ^

And this is happening all over the LLVM codebase, because there is nothing bad there.

Can you please fix that?

[alexfh]

In D16535#362685, @omtcyf0 wrote:

@xazax.hun did you actually run the tool on the LLVM codebase?

Because this check generates tons of false-positive reports during codebase analysis.

See the minimal example below.

omtcyf0-laptop:playground omtcyf0$ cat main.cpp 
#include <vector>

int main() {
  std::vector<int> numbers = {1, 2, 3, 4, 5, 6};
  for (std::vector<int>::iterator it = std::begin(numbers),
                                  end = std::end(numbers);
       it != end; ++it) {
    (*it)++;
  }
  return 0;
}
omtcyf0-laptop:playground omtcyf0$ /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy -checks=misc-suspicious-semicolon main.cpp 
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "main.cpp"
No compilation database found in /Users/omtcyf0/Documents/dev/playground or any parent directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1 warning and 1 error generated.
Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp.
/Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: potentially unintended semicolon [misc-suspicious-semicolon]
       it != end; ++it) {
                ^
/usr/include/wchar.h:89:10: error: 'stdarg.h' file not found [clang-diagnostic-error]
#include <stdarg.h>
         ^

And this is happening all over the LLVM codebase, because there is nothing bad there.

Can you please fix that?

Kirill, the problem in your case may be related to the check seeing incomplete AST due to compilation errors. Can you append -- -std=c++11 to your clang-tidy invocation and try again whether it will be able to parse the file completely (i.e. without any "file not found" and other compilation errors)?

[alexfh]

Having said that, it makes sense to handle invalid AST in the check somehow (e.g. completely disable the check), so it doesn't generate spurious errors.

[omtcyf0]

In D16535#362689, @alexfh wrote:

In D16535#362685, @omtcyf0 wrote:

@xazax.hun did you actually run the tool on the LLVM codebase?

Because this check generates tons of false-positive reports during codebase analysis.

See the minimal example below.

omtcyf0-laptop:playground omtcyf0$ cat main.cpp 
#include <vector>

int main() {
  std::vector<int> numbers = {1, 2, 3, 4, 5, 6};
  for (std::vector<int>::iterator it = std::begin(numbers),
                                  end = std::end(numbers);
       it != end; ++it) {
    (*it)++;
  }
  return 0;
}
omtcyf0-laptop:playground omtcyf0$ /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy -checks=misc-suspicious-semicolon main.cpp 
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "main.cpp"
No compilation database found in /Users/omtcyf0/Documents/dev/playground or any parent directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1 warning and 1 error generated.
Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp.
/Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: potentially unintended semicolon [misc-suspicious-semicolon]
       it != end; ++it) {
                ^
/usr/include/wchar.h:89:10: error: 'stdarg.h' file not found [clang-diagnostic-error]
#include <stdarg.h>
         ^

And this is happening all over the LLVM codebase, because there is nothing bad there.

Can you please fix that?

Kirill, the problem in your case may be related to the check seeing incomplete AST due to compilation errors. Can you append -- -std=c++11 to your clang-tidy invocation and try again whether it will be able to parse the file completely (i.e. without any "file not found" and other compilation errors)?

Yes, you're right, it does suppress the warning.

However, running vanilla run-clang-tidy.py on LLVM codebase still outputs such issues. Is it run-clang-tidy.py's fault? My compilation database contains -std=c++11 specifiers, I assumed this gets added to clang-tidy options while performing analysis of these sources, doesn't it?

[omtcyf0]

Hm. Seems like this is either me using run-clang-tidy.py wrong or it working not properly, because I still get compilation errors. I believe I've done same thing yesterday using older tree and I didn't get any.

Thus said, everything's alright if AST is acquired correctly. Otherwise the check works not as expected, so the fix Alexander wrote about is needed.

[alexfh]

I think, the check can be submitted as is and guards against spurious errors on invalid AST can be added as a follow up.

[xazax.hun]

In D16535#362726, @alexfh wrote:

I think, the check can be submitted as is and guards against spurious errors on invalid AST can be added as a follow up.

This check is already submitted. However I did not found any API in tidy to check whether the compilation failed. It would be great to be able to query that information at the time of registering matchers.

I have one more question though: does it make sense to run clang tidy at all, when the compilation failed? Isn't it a better default behaviour to not to run any of the checks in that case?

[alexfh]

In D16535#362761, @xazax.hun wrote:

In D16535#362726, @alexfh wrote:

I think, the check can be submitted as is and guards against spurious errors on invalid AST can be added as a follow up.

This check is already submitted.

Yup, /me needs to sleep more ;)

However I did not found any API in tidy to check whether the compilation failed. It would be great to be able to query that information at the time of registering matchers.

This information is not available when registering matchers, since this happens before parsing. You can use isInvalidDecl() to check whether particular declaration had any errors, or Result.Context->getTranslationUnitDecl()->isInvalidDecl() to check the whole translation unit.

I have one more question though: does it make sense to run clang tidy at all, when the compilation failed? Isn't it a better default behaviour to not to run any of the checks in that case?

There are some valid use cases for running checks on a non-compilable code, e.g. editor integration.

[LegalizeAdulthood]

docs/clang-tidy/checks/misc-suspicious-semicolon.rst

48–49 ↗

(On Diff #47316)

In this documentation, can we please format code so that control structures don't look like function calls? By this I mean write:

if (x >=y);
  x -= y;

while (readwhitespace());
  Token t = readNextToken();

instead of

if(x >=y);
  x -= y;

while(readwhitespace());
  Token t = readNextToken();

[xazax.hun]

The comments should be addressed in: http://reviews.llvm.org/rL262615

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
+  SuspiciousSemicolonCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
   VirtualNearMissCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
+#include "SuspiciousSemicolonCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "VirtualNearMissCheck.h"
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<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
+    CheckFactories.registerCheck<SuspiciousSemicolonCheck>(
+        "misc-suspicious-semicolon");
     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-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.h

+//===--- SuspiciousSemicolonCheck.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_SUSPICIOUS_SEMICOLON_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_SEMICOLON_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check finds semicolon that modifies the meaning of the program
+/// unintendedly.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-semicolon.html
+class SuspiciousSemicolonCheck : public ClangTidyCheck {
+public:
+  SuspiciousSemicolonCheck(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_SUSPICIOUS_SEMICOLON_H

clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.cpp

+//===--- SuspiciousSemicolonCheck.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 "../utils/LexerUtils.h"
+#include "SuspiciousSemicolonCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void SuspiciousSemicolonCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      stmt(anyOf(ifStmt(hasThen(nullStmt().bind("semi")),
+                        unless(hasElse(stmt()))),
+                 forStmt(hasBody(nullStmt().bind("semi"))),
+                 cxxForRangeStmt(hasBody(nullStmt().bind("semi"))),
+                 whileStmt(hasBody(nullStmt().bind("semi")))))
+          .bind("stmt"),
+      this);
+}
+
+void SuspiciousSemicolonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Semicolon = Result.Nodes.getNodeAs<NullStmt>("semi");
+  SourceLocation LocStart = Semicolon->getLocStart();
+
+  if (LocStart.isMacroID())
+    return;
+
+  ASTContext &Ctxt = *Result.Context;
+  auto Token = lexer_utils::getPreviousNonCommentToken(Ctxt, LocStart);
+  auto &SM = *Result.SourceManager;
+  unsigned SemicolonLine = SM.getSpellingLineNumber(LocStart);
+
+  const auto *Statement = Result.Nodes.getNodeAs<Stmt>("stmt");
+  const bool IsIfStmt = isa<IfStmt>(Statement);
+
+  if (!IsIfStmt &&
+      SM.getSpellingLineNumber(Token.getLocation()) != SemicolonLine)
+    return;
+
+  SourceLocation LocEnd = Semicolon->getLocEnd();
+  FileID FID = SM.getFileID(LocEnd);
+  llvm::MemoryBuffer *Buffer = SM.getBuffer(FID, LocEnd);
+  Lexer Lexer(SM.getLocForStartOfFile(FID), Ctxt.getLangOpts(),
+              Buffer->getBufferStart(), SM.getCharacterData(LocEnd) + 1,
+              Buffer->getBufferEnd());
+  if (Lexer.LexFromRawLexer(Token))
+    return;
+
+  unsigned BaseIndent = SM.getSpellingColumnNumber(Statement->getLocStart());
+  unsigned NewTokenIndent = SM.getSpellingColumnNumber(Token.getLocation());
+  unsigned NewTokenLine = SM.getSpellingLineNumber(Token.getLocation());
+
+  if (!IsIfStmt && NewTokenIndent <= BaseIndent &&
+      Token.getKind() != tok::l_brace && NewTokenLine != SemicolonLine)
+    return;
+
+  diag(LocStart, "potentially unintended semicolon")
+      << FixItHint::CreateRemoval(SourceRange(LocStart, LocEnd));
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
+   misc-suspicious-semicolon
    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
    misc-virtual-near-miss

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-suspicious-semicolon.rst

+.. title:: clang-tidy - misc-suspicious-semicolon
+
+misc-suspicious-semicolon
+=========================
+
+Finds most instances of stray semicolons that unexpectedly alter the meaning of
+the code. More specifically, it looks for `if`, `while`, `for` and `for-range`
+statements whose body is a single semicolon, and then analyzes the context of
+the code (e.g. indentation) in an attempt to determine whether that is
+intentional.
+
+  .. code-block:: c++
+
+    if(x < y);
+    {
+      x++;
+    }
+
+Here the body of the `if` statement consists of only the semicolon at the end of
+the first line, and `x` will be incremented regardless of the condition.
+
+
+  .. code-block:: c++
+
+    while((line = readLine(file)) != NULL);
+      processLine(line);
+
+As a result of this code, `processLine()` will only be called once, when the
+`while` loop with the empty body exits with `line == NULL`. The indentation of
+the code indicates the intention of the programmer.
+
+
+  .. code-block:: c++
+
+    if(x >= y);
+    x -= y;
+
+While the indentation does not imply any nesting, there is simply no valid
+reason to have an `if` statement with an empty body (but it can make sense for
+a loop). So this check issues a warning for the code above.
+
+To solve the issue remove the stray semicolon or in case the empty body is
+intentional, reflect this using code indentation or put the semicolon in a new
+line. For example:
+
+  .. code-block:: c++
+
+    while(readWhitespace());
+      Token t = readNextToken();
+
+Here the second line is indented in a way that suggests that it is meant to be
+the body of the `while` loop - whose body is in fact empty, becasue of the
+semicolon at the end of the first line.
+
+Either remove the indentation from the second line:
+
+  .. code-block:: c++
+
+    while(readWhitespace());
+    Token t = readNextToken();
+
+... or move the semicolon from the end of the first line to a new line:
+
+  .. code-block:: c++
+
+    while(readWhitespace())
+      ;
+
+      Token t = readNextToken();
+
+In this case the check will assume that you know what you are doing, and will
+not raise a warning.

clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-semicolon.cpp

+// RUN: %check_clang_tidy %s misc-suspicious-semicolon %t
+
+int x = 5;
+
+void nop();
+
+void correct1()
+{
+  if(x < 5) nop();
+}
+
+void correct2()
+{
+  if(x == 5)
+    nop();
+}
+
+void correct3()
+{
+  if(x > 5)
+  {
+    nop();
+  }
+}
+
+void fail1()
+{
+  if(x > 5); nop();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon]
+  // CHECK-FIXES: if(x > 5) nop();
+}
+
+void fail2()
+{
+  if(x == 5);
+    nop();
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon]
+  // CHECK-FIXES: if(x == 5){{$}}
+}
+
+void fail3()
+{
+  if(x < 5);
+  {
+    nop();
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: potentially unintended semicolon
+  // CHECK-FIXES: if(x < 5){{$}}
+}
+
+void correct4()
+{
+  while(x % 5 == 1);
+  nop();
+}
+
+void correct5()
+{
+  for(int i = 0; i < x; ++i)
+    ;
+}
+
+void fail4()
+{
+  for(int i = 0; i < x; ++i);
+    nop();
+  // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: potentially unintended semicolon
+  // CHECK-FIXES: for(int i = 0; i < x; ++i){{$}}
+}
+
+void fail5()
+{
+  if(x % 5 == 1);
+    nop();
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: potentially unintended semicolon
+  // CHECK-FIXES: if(x % 5 == 1){{$}}
+}
+
+void fail6() {
+  int a = 0;
+  if (a != 0) {
+  } else if (a != 1);
+    a = 2;
+  // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: potentially unintended semicolon
+  // CHECK-FIXES: } else if (a != 1){{$}}
+}
+
+void fail7() {
+  if (true)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: potentially unintended semicolon
+}
+
+void correct6()
+{
+  do; while(false);
+}
+
+int correct7()
+{
+  int t_num = 0;
+  char c = 'b';
+  char *s = "a";
+  if (s == "(" || s != "'" || c == '"') {
+    t_num += 3;
+    return (c == ')' && c == '\'');
+  }
+
+  return 0;
+}
+
+void correct8() {
+  if (true)
+    ;
+  else {
+  }
+}