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

49

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

31

nit: private

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

9

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

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

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

16

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

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

What about this comment?

63

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.

49

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

63

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

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

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

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

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

79 ↗

(On Diff #88182)

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

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-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;
+
+private:
+  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/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.rst

+.. 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 problems.
+Missing braces can also cause some problems in analyzing 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 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
+
+  if (cond1) { // ok
+    foo1();
+  } else {
+    foo2();
+  }
+
+  if (cond1)
+    foo1();
+  else
+    foo2(); // ok
+
+  if (cond1) { // ok
+    foo1();
+  } else {
+    foo2();
+  }
+
+  if (cond1) // ok
+  {
+    foo1();
+  } else {
+    foo2();
+  }
+
+  if (cond1) // ok
+  {
+    foo1();
+  } else {
+    foo2();
+  }
+
+  while (cond1) {
+    foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+    foo1();
+  foo2(); // comment
+          // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of while(cond1)
+          // CHECK-FIXES: {{^}}  // comment
+
+  while (cond1) {
+    foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+    foo1();
+  foo2(); // ok
+
+  for (;;) {
+    foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+    foo1();
+  foo2(); // comment
+          // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of for(;;)
+          // CHECK-FIXES: {{^}}  // comment
+
+  for (;;) {
+    foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+    foo1();
+  foo2(); // ok
+
+  return 0;
+}