Misleading Indentation check

[xazax.hun]

Summary

This is a clang-tidy check for llvm. It checks for dangling else and for possible misleading indentation due to omitted braces.

[alexfh]

The patch is broken:

Index: clang-tidy/readability/Mislead
===================================================================
--- /dev/null
+++ clang-tidy/readability/Mislead
@@ -0,0 +1,77 @@
+//===--- MisleadingIndentationCheck.cpp - clang-tidy-----------------------===//

Please fix and re-upload.

[Pajesz]

Probably fixed the broken patch.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

What about for ans while statements?

docs/clang-tidy/checks

10 ↗

(On Diff #55190)

Please highlight if/else with ``.

12 ↗

(On Diff #55190)

Please use ``if'' instead of IF.

[nlewycky]

clang-tidy/readability/MisleadingIndentationCheck.cpp

54 ↗

(On Diff #55247)

What about a one-line "if (x) foo(); else bar();"? Warn on it anyways?

test/clang-tidy/readability-misleading-indentation.cpp

48 ↗

(On Diff #55247)

This is arguably misleading indentation, but I'm ok with not warning on it if you think it will cause too many false positives.

[nlewycky]

clang-tidy/readability/MisleadingIndentationCheck.cpp

31 ↗

(On Diff #55247)

Typo of "potential" as "potentional".

56 ↗

(On Diff #55247)

Typo of "indented" as "indentated".

[Pajesz]

Both dangling else and the other check now ignore one-line if-else statements. Corrected other reviews as well.

[xazax.hun]

In D19586#415123, @Pajesz wrote:

Both dangling else and the other check now ignore one-line if-else statements. Corrected other reviews as well.

I can not see a test case for one line if-then-else. Could you add it?

[etienneb]

Could you lift some logics to the matcher instead of the check.

clang-tidy/readability/MisleadingIndentationCheck.cpp

22 ↗

(On Diff #55389)

nit: const

40 ↗

(On Diff #55389)

nit: const

67 ↗

(On Diff #55389)

of if statement -> previous if statement

72 ↗

(On Diff #55389)

instead of doing check at line 23:

if (!If->getElse())

you should use 'hasElse' matcher here.

As I get it, you want to match something like:

anyOf(
  ifStatement(hasThen(stmt().bind("last")),
                     hasElse(unless(compoundStmt())),
  ifStatement(hasThen(unless(compoundStmt())),
                     unless(hasElse(stmt().bind("last")))
)

Then, by getting node named "last" you can retrieve the indentation of the last statement.

77 ↗

(On Diff #55389)

nit: remove empty line

clang-tidy/readability/MisleadingIndentationCheck.h

30 ↗

(On Diff #55389)

nit: private

docs/clang-tidy/checks/readability-misleading-indentation.rst

8 ↗

(On Diff #55389)

could you fix these long lines.

test/clang-tidy/readability-misleading-indentation.cpp

62 ↗

(On Diff #55389)

I would like to see more tests with { }

if (xxx) {
} else {
}

if (xxx) {
}
else {
}

if (xxx)
{
}
else
{
}

if (xxx)
  {
  }
else
  {
  }

62 ↗

(On Diff #55389)

could you add test with macro:

#define BLOCK
  if (xxx)    \
    stm1();  \
    stm2();

[etienneb]

You should not limit the checker to "if".
The for-statement and while-statement are also wrong for the same reasons:

while (x)
  statement1();
  statement2();

for (...)
  statement1();
  statement2();

You should test your code over large code base.
You should catch these cases:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/frame/FrameView.cpp&q=setMediaType&sq=package:chromium&type=cs&l=1253

You will also find strange cases like:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/i18n/usearch.cpp&q=initializepattern&sq=package:chromium&type=cs&l=438

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/common/ushape.cpp&q=third_party/icu/source/common/ushape.cpp&sq=package:chromium&type=cs&l=784

But this can be a further improvement.

[xazax.hun]

In D19586#417053, @etienneb wrote:

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

But this can be a further improvement.

I believe this might be an intentional omission, since this can not imply semantic problems. But I agree that, this addition makes sense.

[etienneb]

In D19586#417063, @xazax.hun wrote:

In D19586#417053, @etienneb wrote:

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

But this can be a further improvement.

I believe this might be an intentional omission, since this can not imply semantic problems. But I agree that, this addition makes sense.

Fair enough. This checker is in 'readbility', so I don't see why not.
But, feel free to postpone. Or let someone else take it.

[Pajesz]

Checker now works with for and while statements as well, new tests were added, other syntactical and logical updates have been made.

[alexfh]

Gergely, it seems that the last diff is missing clang-tidy/readability/MisleadingIndentationCheck.cpp. A few more comments below.

docs/clang-tidy/checks/readability-misleading-indentation.rst

13 ↗

(On Diff #61651)

nit: Please use double backquotes to mark inline code fragments.

test/clang-tidy/readability-misleading-indentation.cpp

16 ↗

(On Diff #61651)

1. This is not a part of the previous CHECK-MESSAGES. Is it intended? It's fine to have long lines in tests.
2. Code snippets in the message should be enclosed in single quotes ('else', 'if(cond2)' ...).
3. Please specify each unique message once completely (including the [check-name] part).

17 ↗

(On Diff #61651)

These CHECK-FIXES: are totally unreliable, since they are all matching the same pattern. FileCheck (which is used to verify the output against these CHECK lines) maintains no implicit relation between the check line location and the line number matched in the output. The two things that matter to FileCheck are the order of the matched lines and their content. So the patterns need to be unique to avoid matches to incorrect lines. Change them to, e.g. // comment1, // comment2, ...

63 ↗

(On Diff #61651)

What about this comment?

63 ↗

(On Diff #61651)

What about this comment?

[Pajesz]

Minor changes in tests and doc and diff should be full now.

[alexfh]

Please mark all addressed comments "Done".

test/clang-tidy/readability-misleading-indentation.cpp

15 ↗

(On Diff #63925)

Please place // CHECK comments at column 1 or 3. Indenting them like this makes the test less readable.

16 ↗

(On Diff #63925)

Did you run the tests after changes? I don't think the // comment1 line can actually appear in the fixed code, when it's not there before the fix.

[Pajesz]

so i have to submit K good to know

test/clang-tidy/readability-misleading-indentation.cpp

63 ↗

(On Diff #61651)

The checker doesn't work for macro's I don't know why and how to fix it so I just ignored it.

48 ↗

(On Diff #55247)

Since brackets are used I don't think we should warn about it. It would indeed cause man false positives.

16 ↗

(On Diff #63925)

Could you please explain what check-fixes does and how? Im not sure about it.

[xazax.hun]

test/clang-tidy/readability-misleading-indentation.cpp

16 ↗

(On Diff #63925)

You can read the documentation here: http://clang.llvm.org/extra/clang-tidy/#testing-checks

Didn't you execute the tests? (Using make check-all or some similar command)

The CHECK-FIXES line will check the content of the file after the fixits are applied. The check will fail when the specified pattern is not found in the result.

[Pajesz]

Hello!

Gonna fix the tests ASAP! Any other suggestions, fixes, improvements considering the checker?

[danielmarjamaki]

clang-tidy/readability/MisleadingIndentationCheck.cpp

20 ↗

(On Diff #63925)

There is no handling of tabs and spaces by danglingElseCheck as far as I see.

The "if" might for example be indented with spaces. And then the corresponding "else" is indented with a tab. Then I guess there is false positive.

If the "if" is indented with 2 spaces and the "else" is indented with 2 tabs. then I assume there is false negative.

It's unfortunate. Is it ok?

34 ↗

(On Diff #63925)

I see no test case that says "potential dangling else". If I'm not mistaken that should be added.

Is it really sufficient to write that? It's not obvious to me why clang-tidy would think it's dangling.

44 ↗

(On Diff #63925)

You don't need to use both isa and dyn_cast:

if (const auto *CurrentIf = dyn_cast<IfStmt>(CurrentStmt)) {
    Inside = CurrentIf->getElse() ? CurrentIf->getElse() : CurrentIf->getThen();
} ....

clang-tidy/readability/MisleadingIndentationCheck.h

20 ↗

(On Diff #63925)

there are too many spaces in this comment

[xazax.hun]

The original author is no longer available.

[xazax.hun]

• Updated to latest trunk.
• Mentioned check in the release notes.
• Documented the limitation that tabs and spaces need to be consistent for this check to work.
• Fixed (hopefully all) review comments.
• Fixed the test cases.
• Minor code cleanups.

[xazax.hun]

clang-tidy/readability/MisleadingIndentationCheck.cpp

20 ↗

(On Diff #63925)

Documented this limitation.

[alexfh]

clang-tidy/readability/MisleadingIndentationCheck.cpp

72 ↗

(On Diff #88182)

Will it be useful to add a note pointing to the control statement and saying "did you mean this line to be inside this if/while/for/..."?

79 ↗

(On Diff #88182)

has(anyOf(ifStmt(), forStmt(), whileStmt())) would read better.

[danielmarjamaki]

clang-tidy/readability/MisleadingIndentationCheck.cpp

42 ↗

(On Diff #88182)

I would rename Inside to Inner. That would make InnerLoc more "consistent".

48 ↗

(On Diff #88182)

nit: write "const auto *CurrentWhile...". missing "*"?

test/clang-tidy/readability-misleading-indentation.cpp

21 ↗

(On Diff #88182)

I am skeptic about this warning message.

Why does it say "potential". I would say that in this test case the indentation _is_ "dangling".

The message is not very clear to me. I personally don't intuitively understand what is wrong without looking at the code.

I don't know what it should say. Maybe:

different indentation for 'if' and 'else'

[xazax.hun]

• Added a note to make it easier to understand the diagnostics.
• Reworded the error message about dangling else.
• Fixed other review comments.

[xazax.hun]

Thank you for the review!

clang-tidy/readability/MisleadingIndentationCheck.cpp

79 ↗

(On Diff #88182)

I agree but unfortunately that does not compile.

test/clang-tidy/readability-misleading-indentation.cpp

21 ↗

(On Diff #88182)

Thank you, good idea!

[alexfh]

LG

clang-tidy/readability/MisleadingIndentationCheck.cpp

79 ↗

(On Diff #88182)

Yep, with matchers you never know what will compile ;)

Maybe has(stmt(anyOf(ifStmt(), forStmt(), whileStmt())))? If this also doesn't work, then it's fine as is.

[alexfh]

Gábor, thank you for picking up this patch and finishing it!

[xazax.hun]

clang-tidy/readability/MisleadingIndentationCheck.cpp

79 ↗

(On Diff #88182)

That did the trick, thanks :)

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp

clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.h

+//===--- MisleadingIndentationCheck.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_READABILITY_MISLEADING_INDENTATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Checks the code for dangling else, and possible misleading indentations due
+/// to missing braces. Note that this check only works as expected when the tabs
+/// or spaces are used consistently and not mixed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html
+class MisleadingIndentationCheck : public ClangTidyCheck {
+public:
+  MisleadingIndentationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void danglingElseCheck(const SourceManager &SM, const IfStmt *If);
+  void missingBracesCheck(const SourceManager &SM, const CompoundStmt *CStmt);
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H

clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp

+//===--- MisleadingIndentationCheck.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 "MisleadingIndentationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void MisleadingIndentationCheck::danglingElseCheck(const SourceManager &SM,
+                                                   const IfStmt *If) {
+  SourceLocation IfLoc = If->getIfLoc();
+  SourceLocation ElseLoc = If->getElseLoc();
+
+  if (IfLoc.isMacroID() || ElseLoc.isMacroID())
+    return;
+
+  if (SM.getExpansionLineNumber(If->getThen()->getLocEnd()) ==
+      SM.getExpansionLineNumber(ElseLoc))
+    return;
+
+  if (SM.getExpansionColumnNumber(IfLoc) !=
+      SM.getExpansionColumnNumber(ElseLoc))
+    diag(ElseLoc, "different indentation for 'if' and corresponding 'else'");
+}
+
+void MisleadingIndentationCheck::missingBracesCheck(const SourceManager &SM,
+                                                    const CompoundStmt *CStmt) {
+  const static StringRef StmtNames[] = {"if", "for", "while"};
+  for (unsigned int i = 0; i < CStmt->size() - 1; i++) {
+    const Stmt *CurrentStmt = CStmt->body_begin()[i];
+    const Stmt *Inner = nullptr;
+    int StmtKind = 0;
+
+    if (const auto *CurrentIf = dyn_cast<IfStmt>(CurrentStmt)) {
+      StmtKind = 0;
+      Inner =
+          CurrentIf->getElse() ? CurrentIf->getElse() : CurrentIf->getThen();
+    } else if (const auto *CurrentFor = dyn_cast<ForStmt>(CurrentStmt)) {
+      StmtKind = 1;
+      Inner = CurrentFor->getBody();
+    } else if (const auto *CurrentWhile = dyn_cast<WhileStmt>(CurrentStmt)) {
+      StmtKind = 2;
+      Inner = CurrentWhile->getBody();
+    } else {
+      continue;
+    }
+
+    if (isa<CompoundStmt>(Inner))
+      continue;
+
+    SourceLocation InnerLoc = Inner->getLocStart();
+    SourceLocation OuterLoc = CurrentStmt->getLocStart();
+
+    if (SM.getExpansionLineNumber(InnerLoc) ==
+        SM.getExpansionLineNumber(OuterLoc))
+      continue;
+
+    const Stmt *NextStmt = CStmt->body_begin()[i + 1];
+    SourceLocation NextLoc = NextStmt->getLocStart();
+
+    if (InnerLoc.isMacroID() || OuterLoc.isMacroID() || NextLoc.isMacroID())
+      continue;
+
+    if (SM.getExpansionColumnNumber(InnerLoc) ==
+        SM.getExpansionColumnNumber(NextLoc)) {
+      diag(NextLoc, "misleading indentation: statement is indented too deeply");
+      diag(OuterLoc, "did you mean this line to be inside this '%0'",
+           DiagnosticIDs::Note)
+          << StmtNames[StmtKind];
+    }
+  }
+}
+
+void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+      compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()))))
+          .bind("compound"),
+      this);
+}
+
+void MisleadingIndentationCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *If = Result.Nodes.getNodeAs<IfStmt>("if"))
+    danglingElseCheck(*Result.SourceManager, If);
+
+  if (const auto *CStmt = Result.Nodes.getNodeAs<CompoundStmt>("compound"))
+    missingBracesCheck(*Result.SourceManager, CStmt);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<MisleadingIndentationCheck>(
+        "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `readability-misleading-indentation
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
+
+  Finds misleading indentation where braces should be introduced or the code should be reformatted.
+
 - New `safety-no-assembler
   <http://clang.llvm.org/extra/clang-tidy/checks/safety-no-assembler.html>`_ check
 
   Finds uses of inline assembler.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

.. toctree::
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release
    safety-no-assembler

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-misleading-indentation.rst

+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==================================
+
+Correct indentation helps to understand code. Mismatch of the syntactical
+structure and the indentation of the code may hide serious problems.
+Missing braces can also make it significantly harder to read the code,
+therefore it is important to use braces. 
+
+The way to avoid dangling else is to always check that an ``else`` belongs
+to the ``if`` that begins in the same column.
+
+You can omit braces when your inner part of e.g. an ``if`` statement has only
+one statement in it. Although in that case you should begin the next statement
+in the same column with the ``if``.
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+  if (cond1)
+    if (cond2)
+      foo1();
+  else
+    foo2();  // Wrong indentation: else belongs to if(cond2) statement.
+
+  // Missing braces:
+  if (cond1)
+    foo1();
+    foo2();  // Not guarded by if(cond1).
+
+Limitations
+===========
+
+Note that this check only works as expected when the tabs or spaces are used
+consistently and not mixed.

clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp

+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1();
+void foo2();
+
+#define BLOCK \
+  if (cond1)  \
+    foo1();   \
+    foo2();
+
+int main()
+{
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+    if (cond2)
+      foo1();
+  else
+    foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+
+  if (cond1) {
+    if (cond2)
+      foo1();
+  }
+  else
+    foo2();
+
+  if (cond1)
+    if (cond2)
+      foo1();
+    else
+      foo2();
+
+  if (cond2)
+    foo1();
+    foo2();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: misleading indentation: statement is indented too deeply [readability-misleading-indentation]
+    // CHECK-MESSAGES: :[[@LINE-4]]:3: note: did you mean this line to be inside this 'if'
+    foo2(); // No redundant warning.
+
+  if (cond1)
+  {
+    foo1();
+  }
+    foo2();
+
+  if (cond1)
+    foo1();
+  foo2();
+
+  if (cond2)
+    if (cond1) foo1(); else foo2();
+
+  if (cond1) {
+  } else {
+  }
+
+  if (cond1) {
+  }
+  else {
+  }
+
+  if (cond1)
+  {
+  }
+  else
+  {
+  }
+
+  if (cond1)
+    {
+    }
+  else
+    {
+    }
+
+  BLOCK
+}