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

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

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

48

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

Typo of "potential" as "potentional".

56

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

23

nit: const

41

nit: const

68

of if statement -> previous if statement

73

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.

78

nit: remove empty line

clang-tidy/readability/MisleadingIndentationCheck.h

31

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

63

I would like to see more tests with { }

if (xxx) {
} else {
}

if (xxx) {
}
else {
}

if (xxx)
{
}
else
{
}

if (xxx)
  {
  }
else
  {
  }

63

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

17

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).

18

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, ...

64

What about this comment?

64

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

16

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

17

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

17

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

48

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

64

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

[xazax.hun]

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

17

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

21

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?

35

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.

45

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

21

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

21

Documented this limitation.

[alexfh]

clang-tidy/readability/MisleadingIndentationCheck.cpp

73

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/..."?

80

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

[danielmarjamaki]

clang-tidy/readability/MisleadingIndentationCheck.cpp

43

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

49

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

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

22

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

80

I agree but unfortunately that does not compile.

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

22

Thank you, good idea!

[alexfh]

LG

clang-tidy/readability/MisleadingIndentationCheck.cpp

80

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

80

That did the trick, thanks :)

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  MisleadingIndentationCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   SimplifyBooleanExprCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp

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.
+///
+/// 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;
+  void danglingElseCheck(const ast_matchers::MatchFinder::MatchResult &Result);
+  void missingBracesCheck(const ast_matchers::MatchFinder::MatchResult &Result);
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H

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 MatchFinder::MatchResult &Result) {
+  auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+  if (!If->getElse())
+    return;
+
+  SourceLocation IfLoc = If->getIfLoc();
+  SourceLocation ElseLoc = If->getElseLoc();
+
+  if (Result.SourceManager->getExpansionColumnNumber(IfLoc) !=
+      Result.SourceManager->getExpansionColumnNumber(ElseLoc))
+    diag(ElseLoc, "potentional dangling else");
+}
+
+void MisleadingIndentationCheck::missingBracesCheck(
+    const MatchFinder::MatchResult &Result) {
+  auto *CStmt = Result.Nodes.getNodeAs<CompoundStmt>("stmt");
+  for (unsigned int i = 0; i < CStmt->size() - 1; i++) {
+    Stmt *CurrentStmt = CStmt->body_begin()[i];
+
+    if (!isa<IfStmt>(CurrentStmt))
+      continue;
+
+    IfStmt *CurrentIf = dyn_cast<IfStmt>(CurrentStmt);
+    Stmt *InIf =
+        CurrentIf->getElse() ? CurrentIf->getElse() : CurrentIf->getThen();
+
+    if (isa<CompoundStmt>(InIf))
+      continue;
+
+    SourceLocation StmtLoc = InIf->getLocStart();
+    Stmt *NextStmt = CStmt->body_begin()[i + 1];
+    SourceLocation NextLoc = NextStmt->getLocStart();
+
+    if (Result.SourceManager->getExpansionColumnNumber(StmtLoc) ==
+        Result.SourceManager->getExpansionColumnNumber(NextLoc))
+      diag(NextLoc, "Wrong Indentation - statement is indentated as a member "
+                    "of if statement");
+  }
+}
+
+void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(compoundStmt(has(ifStmt())).bind("stmt"), this);
+}
+
+void MisleadingIndentationCheck::check(const MatchFinder::MatchResult &Result) {
+
+  if (Result.Nodes.getNodeAs<IfStmt>("if"))
+    danglingElseCheck(Result);
+
+  if (Result.Nodes.getNodeAs<CompoundStmt>("stmt"))
+    missingBracesCheck(Result);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
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<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");

docs/clang-tidy/checks/list.rst

.. toctree::
    performance-implicit-cast-in-loop
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-simplify-boolean-expr
    readability-uniqueptr-delete-release

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

+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==================================
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical structure and the indentation of the code may reveal serious 
+
+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 t
+
+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 part of if(cond1)

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

+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main()
+{
+   bool cond1 = true;
+   bool cond2 = true;
+
+   if (cond1)
+      if (cond2)
+         foo1();
+   else
+     foo2();  // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs to if(cond2) statement
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1) {
+      if (cond2)
+         foo1();
+   }
+   else
+      foo2();  // ok, else belongs to if(cond1) statement
+
+   if (cond1)
+     if (cond2)
+        foo1();
+     else
+        foo2();  // ok, indentation matches to syntactical structure
+
+   if (cond1)
+      foo1();
+      foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond2)
+      foo1();
+      foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond2)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1)
+   {
+      foo1();
+   }
+      foo2();  // ok
+
+   if (cond1)
+      foo1();
+   foo2();  // ok
+
+  if (cond1)
+      foo1();
+      foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+      foo2(); // no need for redundant warning
+
+   return 0;
+}