[Diagnostic] add a warning which warns about misleading indentation

[Tyker]

Summary

Add a warning for misleading indentation similar to GCC's -Wmisleading-indentation

[xbolva00]

Great start!

clang/lib/Parse/ParseStmt.cpp

1194

Misleading

[xbolva00]

cc @thakis as he wanted this in clang too

[xbolva00]

clang/include/clang/Basic/DiagnosticGroups.td

830

Move it to All/Most

GCC enables this with -Wall

[Tyker]

fixed comments.

[xbolva00]

Small nits.

Generally, I am fine with this. Aaron?

clang/include/clang/Basic/DiagnosticGroups.td

866

Still typo here

[xbolva00]

clang/lib/Parse/ParseStmt.cpp

1199

Save P.getPreprocessor().getSourceManager() to variable.

clang/test/SemaCXX/warn-misleanding-indentation.cpp

60

Add newline

[Tyker]

fixed comments.

[xbolva00]

Thanks.

But please wait for Aaron/Nico.

clang/lib/Parse/ParseStmt.cpp

1198

SourceManager &SM

clang-format this diff?

[aaron.ballman]

Thank you for working on this, it's very useful functionality!

clang/include/clang/Basic/DiagnosticGroups.td

866

Why is this in Most? Shouldn't it be in All, like GCC?

clang/include/clang/Basic/DiagnosticParseKinds.td

65

How about: misleading indentation; %select{statement|declaration}0 is not part of the previous '%select{if|else|for|while}1'?

Right now, you only seem to be covering statements, but I presume you want to also handle cases like:

if (foo)
  bar();
  int baz;

This also fixes a typo with "misleading" and handles localization better by using a selection for the "previous" construct.

clang/test/SemaCXX/warn-misleanding-indentation.cpp

1

The test case file name has a typo: misleanding -> misleading

You should also add a run line that tests this in C mode, because the parsing rules can be subtly different in C and C++.

8

I'd like to see a test case where the misleading indentation is not harmful, because we shouldn't diagnose that. e.g.,

if (foo)
  bar();
  ;  // this is not really a misleading indentation (-Wextra-semi-stmt covers it for style)

I'd also like to see a test where the indentation isn't on a statement but is still misleading. e.g.,

if (foo)
  bar();
  int x; // This should diagnose

if (foo)
  bar();
  typedef int frobble; // This should diagnose

if (foo)
  bar();
  [[]]; // No idea how I feel about this diagnosing or not

We should have some test cases for degenerate indentation, e.g.,

if (foo)
bar(); // GCC doesn't think this is misleading, we shouldn't either
baz(); // This is not misleading indentation

And some tests for other forms of the constructs, like if constexpr (whatever) and for (foo : bar)

I won't ask for a test case involving zero-width space characters. ;-)

[Tyker]

fixed comment.

[xbolva00]

int bar(void);

int* foo(int* dst)
{
  if (dst)
    return dst + \
    bar();

  return nullptr;
}

This code is kinda weird, but please add it as a test (be sure we do not warn here).

Another case (but I am fine if you decide to not implement it yet, not blocking request)

void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

User forgot to write ; behind return.

[aaron.ballman]

clang/test/SemaCXX/warn-misleading-indentation.cpp

18 ↗

(On Diff #230807)

Please use the full diagnostic text unless the identical full text has already been verified on the same kind of construct previously (same goes for most of the diagnostics here). We only cut out repetitive bits of frequent diagnostics, generally speaking, but we want the full diagnostics to be checked whenever it's a unique diagnostic.

59–71 ↗

(On Diff #230807)

A few more test cases:

// No diagnostics from GCC on this
void f0(int i) {
  if (1)
    i = 2;
  else
         if (i == 3)
    i = 4;
    i = 5;
}

// Or this
#define TEST i = 5
void f0(int i) {
  if (1)
    i = 2;
  else
    i = 5;
    TEST;
}

void f0(int i) {
  if (1)
    i = 2;
  else
    if (i == 3)
    i = 4;
    i = 5; // This one is diagnosed
}

void f0(int i) {
  if (1)
    i = 2;
  else
    if (i == 3)
    {i = 4;}
    i = 5; // As is this one
}

[Tyker]

added tests.
all the tests were already passing.

[xbolva00]

void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

Well, for this case (void function) we could warn. But okay to ignore it for now.

[Tyker]

In D70638#1758130, @xbolva00 wrote:

void bar(void);
void foo(int* dst)
{   
    if (dst)
       return
    bar();
}

Well, for this case (void function) we could warn. But okay to ignore it for now.

this can be done in an other patch

[xbolva00]

Yes, looks fine.

[aaron.ballman]

LGTM, thank you!

[Tyker]

Thanks for the review

[xbolva00]

@tstellar reverted this change.

[Tyker]

yeah i saw. this version of the patch builds all llvm subproject(not tested on llgo) without warnings. there is only one case in llvm's code in which this warning is more agressive thant GCC's.

if (1)
  i = 0;

// comment
  i = 1; //  clang + this patch warns through the comment wereas gcc doesn't.

i don't believe a warning in this case is too bad.

[xbolva00]

Please land code fixes as NFC patch and then try to reland this patch.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2532 ↗

(On Diff #231534)

Please land as separated NFCI commit.

[Tyker]

I just found out that Parser::isCXXDeclarationStatement does more then just disambiguation it can emit diagnostics. which can cause error on correct code. so we can't use it in this context to disambiguate.

so it is not possible to easily disambiguate between statements and declaration. i changed the warning message to reflect that but the message isn't so good now. gcc just calls everything a statement with is perhaps a good solution.

[xbolva00]

clang/include/clang/Parse/Parser.h

2269 ↗

(On Diff #231545)

Extra change? Your code does not use isCXXDeclarationStatement.

[xbolva00]

there is only one case in llvm's code in which this warning is more agressive thant GCC's.

I think that's okay. If after-commit feedback shows this is bad, we can always tune this diagnostic more and do not warn in that case.

[Tyker]

Improve the warning for else if

[xbolva00]

clang/lib/Parse/ParseStmt.cpp

1239

I dont think we need this.

Tests will fail anyway if someone removes check() calls.

1333

What is wrong with code you used some rev ago?

if (usable) check();

Now you uselessly instantiate MIChecker since if Usable = false, check is not called.

I like the older code more...

If (usable) checkForMisleadingIndention(...)

Was good and accepted.

[Tyker]

clang/lib/Parse/ParseStmt.cpp

1333

previous patch gave up on else if because we can't know wether there are braces.

this revision can produce correct diagnostics on else if

[Tyker]

clang/lib/Parse/ParseStmt.cpp

1333

the MisleadingIndentationChecker gathers information during its construction this allows having more context and removes many false positives.

but I can bring back
If (usable) MIChecker.Check(...)

[xbolva00]

Looks better, thanks.

Aaron ?

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

1328–1329

This looks incorrect to me. Consider a case like:

if (0) {
} else [[gsl::suppress("foo")]] if (1) {
}

I'm a little uneasy calling anything but ParseStatement() here as that is what needs to be parsed at this stage.

[Tyker]

improved based on aaron's comment.

[aaron.ballman]

LGTM with some minor nits.

clang/lib/Parse/ParseStmt.cpp

1219

Spurious whitespace, but it would be useful to add a newline above the Check() declaration for visual separation.

1239

This can be hoisted (negated) into the preceding if statement.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2532 ↗

(On Diff #231534)

Yeah, these changes seem unrelated to the patch.

[nathanchance]

As an FYI, this appears to cause several false positive warnings with the Linux kernel:

../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
        if (fb_logo.depth > 4 && depth > 4) {
        ^
../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
        else
        ^
../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
        return ret;
        ^
../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
        if (!ret)
        ^
2 warnings generated.

Corresponding to:

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665

and

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072

[xbolva00]

fbmem.c case reduced:

https://godbolt.org/z/ma4aFA

Looks like kernel uses tabs here

So we have BinaryOperator <line:5:3, col:9>

If we use spaces: BinaryOperator <line:5:9, col:15> 'int' '='

clang/test/Parser/warn-misleading-indentation.cpp

209 ↗

(On Diff #231962)

Please add newline.

[Tyker]

i did a few test on the linux kernel on prior version of this patchs and the mix of spaces and tabs caused some false positives.
for the mix of space and tabs. gcc has a -ftabstop=X to specify how large tabs should be counted as.
clang has it as well i am going to check that it is taken in account.

i will investigate

[nickdesaulniers]

In D70638#1768200, @nathanchance wrote:

As an FYI, this appears to cause several false positive warnings with the Linux kernel:

../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
        if (fb_logo.depth > 4 && depth > 4) {
        ^
../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
        else
        ^
../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
        return ret;
        ^
../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
        if (!ret)
        ^
2 warnings generated.

Corresponding to:

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665

and

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072

IMO that is misleading indentation. A single space followed by a tab? WTF

[Tyker]

the warning was not affected by -ftabstop. i made a patch that fix this.

https://reviews.llvm.org/D71037

and I agree that mixing tabs and space is a horrible idea for many reasons.

[nathanchance]

IMO that is misleading indentation. A single space followed by a tab? WTF

I guess I will take a look over all of the warnings that cropped up and see if this is the case for all of them. If it is, I will just send patches to fix those (with the justification of the warning and the fact that it would be a checkpatch warning), rather than adding -ftabstop to KBUILD_CFLAGS.

$ ./scripts/checkpatch.pl --terse --types LEADING_SPACE -f drivers/video/fbdev/core/fbmem.c
drivers/video/fbdev/core/fbmem.c:665: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:666: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:667: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:668: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:669: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:670: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:671: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:672: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:673: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:674: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:675: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:676: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:677: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:678: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1063: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1064: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1070: WARNING: please, no spaces at the start of a line
drivers/video/fbdev/core/fbmem.c:1075: WARNING: please, no spaces at the start of a line

[thakis]

Hey, cool! I gave this a shot myself many years ago at http://llvm.org/bugs/show_bug.cgi?id=18938 That bug has a list of interesting test cases I had collected back then. Maybe you want to check and see how this does on those cases?

[thakis]

Also, did you check if this has a measurable impact on compilation speed?

[Tyker]

i have not tested the performance impact. but i tried to do the heavier checks last to minimize the impact.
i added your tests file to the improvement commit.

[nathanchance]

As a follow up to my previous post, I have sent patches to clean up all of the warnings that I see in the Linux kernel. However, I found one that I do believe is a false positive:

../drivers/staging/uwb/allocator.c:353:3: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
  alloc_found:
  ^
../drivers/staging/uwb/allocator.c:350:2: note: previous statement is here
        else
        ^
1 warning generated.

Corresponding to https://github.com/torvalds/linux/blob/2187f215ebaac73ddbd814696d7c7fa34f0c3de0/drivers/staging/uwb/allocator.c#L346-L353.

Simplified:

$ cat test.c
int a(int b, int c) {
	if (b)
		goto label;

	if (c)
		return 0;

  label:
	return 1;
}

$ clang -Wmisleading-indentation -o /dev/null -c test.c
test.c:8:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
  label:
  ^
test.c:5:2: note: previous statement is here
        if (c)
        ^
1 warning generated.

goto labels are unaffected by indentation so there should be no warning here. While I think that the labels should be unindented for style, the driver is marked as obsolete and is scheduled to be deleted so I am not sure such a patch would be welcomed.

[xbolva00]

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

[aaron.ballman]

In D70638#1803364, @xbolva00 wrote:

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

I agree -- @Tyker, are you planning to work on that fix?

[Tyker]

In D70638#1804138, @aaron.ballman wrote:

In D70638#1803364, @xbolva00 wrote:

(re-ping; I think this false positive for goto label case is important to be fixed before 10 release)

I agree -- @Tyker, are you planning to work on that fix?

i made a patch. https://reviews.llvm.org/D72202

[kastiglione]

@Tyker Hi, I noticed a case that isn't caught by -Wmisleading-indentation. In code that uses two space indents, there's a corner case that isn't caught when the preceding if uses curly braces. I've noticed a couple instances of this in lldb.

For example:

if (cond) {
  then1();
  then2();
  // ...
} else
  else1();
  else2();

The else2(); statement doesn't trigger the warning.

It seems that the logic is to use the column of the else keyword, not the column of the right brace. When using a four space indent (any indent != 2), the warning is emitted:

} else
    else1();
    else2();

[kastiglione]

@Tyker in case you didn't see my previous message, I'm curious if you'd be willing to take a look at the bug. Thanks!

clang/include/clang/Basic/DiagnosticGroups.td

 def VLA : DiagGroup<"vla">;
 def VLAExtension : DiagGroup<"vla-extension">;
 def VolatileRegisterVar : DiagGroup<"volatile-register-var">;
 def Visibility : DiagGroup<"visibility">;
 def ZeroLengthArray : DiagGroup<"zero-length-array">;
 def GNUZeroLineDirective : DiagGroup<"gnu-zero-line-directive">;
 def GNUZeroVariadicMacroArguments : DiagGroup<"gnu-zero-variadic-macro-arguments">;
 def Fallback : DiagGroup<"fallback">;
+def MisleadingIndentation : DiagGroup<"misleading-indentation">;
 
 // This covers both the deprecated case (in C++98)
 // and the extension case (in C++11 onwards).
 def WritableStrings : DiagGroup<"writable-strings", [DeprecatedWritableStr]>;
 
 // GCC calls -Wdeprecated-writable-strings -Wwrite-strings.
 //
 // Bizarrely, this warning flag enables -fconst-strings in C. This is
def Extra : DiagGroup<"extra", [
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
     NullPointerArithmetic,
     EmptyInitStatement
   ]>;
 
 def Most : DiagGroup<"most", [
     CharSubscript,
     Comment,
     DeleteNonVirtualDtor,
     ForLoopAnalysis,
     Format,
     Implicit,
def Most : DiagGroup<"most", [
     VolatileRegisterVar,
     ObjCMissingSuperCalls,
     ObjCDesignatedInit,
     ObjCFlexibleArray,
     OverloadedVirtual,
     PrivateExtern,
     SelTypeCast,
     ExternCCompat,
-    UserDefinedWarnings
+    UserDefinedWarnings,
+    MisleadingIndentation
  ]>;
 
 // Thread Safety warnings
 def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
 def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
 def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
 def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
 def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;

clang/include/clang/Basic/DiagnosticParseKinds.td

 def warn_extra_semi_after_mem_fn_def : Warning<
   "extra ';' after member function definition">,
   InGroup<ExtraSemi>, DefaultIgnore;
 def warn_null_statement : Warning<
   "empty expression statement has no effect; "
   "remove unnecessary ';' to silence this warning">,
   InGroup<ExtraSemiStmt>, DefaultIgnore;
 
+def warn_misleading_indentation : Warning<
+  "this indention is missleading; this statement is not part of the previous"
+  " '%0'">, InGroup<MisleadingIndentation>, DefaultIgnore;
+def note_previous_statement : Note<
+  "previous statement is here">;
+
 def ext_thread_before : Extension<"'__thread' before '%0'">;
 def ext_keyword_as_ident : ExtWarn<
   "keyword '%0' will be made available as an identifier "
   "%select{here|for the remainder of the translation unit}1">,
   InGroup<KeywordCompat>;
 
 def ext_nullability : Extension<
   "type nullability specifier %0 is a Clang extension">,

clang/lib/Parse/ParseStmt.cpp

while (Tok.is(tok::r_paren)) {
     Diag(Tok, diag::err_extraneous_rparen_in_condition)
       << FixItHint::CreateRemoval(Tok.getLocation());
     ConsumeParen();
   }
 
   return false;
 }
 
+static void MaybeDiagnoseMisleadingIndentation(Parser &P, Token Tok,
+                                                SourceLocation PrevLoc,
+                                                SourceLocation StmtLoc,
+                                                const char *StmtName) {
+  SourceManager& SM = P.getPreprocessor().getSourceManager();
+  unsigned PrevStmtColNum = SM.getSpellingColumnNumber(PrevLoc);
+  unsigned CurStmtColNum = SM.getSpellingColumnNumber(Tok.getLocation());
+  if (PrevStmtColNum != 0 && CurStmtColNum != 0 &&
+      PrevStmtColNum <= CurStmtColNum) {
+    P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) << StmtName;
+    P.Diag(StmtLoc, diag::note_previous_statement);
+  }
+}
 
 /// ParseIfStatement
 ///       if-statement: [C99 6.8.4.1]
 ///         'if' '(' expression ')' statement
 ///         'if' '(' expression ')' statement 'else' statement
 /// [C++]   'if' '(' condition ')' statement
 /// [C++]   'if' '(' condition ')' statement 'else' statement
 ///
 StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
   assert(Tok.is(tok::kw_if) && "Not an if stmt!");
   SourceLocation IfLoc = ConsumeToken();  // eat the 'if'.
 
   bool IsConstexpr = false;
   if (Tok.is(tok::kw_constexpr)) {
     Diag(Tok, getLangOpts().CPlusPlus17 ? diag::warn_cxx14_compat_constexpr_if
                                         : diag::ext_constexpr_if);
     IsConstexpr = true;
     ConsumeToken();
   }
 
   if (Tok.isNot(tok::l_paren)) {
     Diag(Tok, diag::err_expected_lparen_after) << "if";
     SkipUntil(tok::semi);
     return StmtError();
   }
 
   bool C99orCXX = getLangOpts().C99 || getLangOpts().CPlusPlus;
 
   // C99 6.8.4p3 - In C99, the if statement is a block.  This is not
   // the case for C90.
   //
   // C++ 6.4p3:
   // A name introduced by a declaration in a condition is in scope from its
   // point of declaration until the end of the substatements controlled by the
   // condition.
   // C++ 3.3.2p4:
   // Names declared in the for-init-statement, and in the condition of if,
   // while, for, and switch statements are local to the if, while, for, or
   // switch statement (including the controlled statement).
   //
   ParseScope IfScope(this, Scope::DeclScope | Scope::ControlScope, C99orCXX);
EnterExpressionEvaluationContext PotentiallyDiscarded(
         Sema::ExpressionEvaluationContextRecord::EK_Other,
         /*ShouldEnter=*/ConstexprCondition && !*ConstexprCondition);
     ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc);
   }
 
   // Pop the 'if' scope if needed.
   InnerScope.Exit();
 
+  if (Tok.isNot(tok::kw_else))
+    MaybeDiagnoseMisleadingIndentation(*this, Tok, ThenStmtLoc, IfLoc, "if");
+
   // If it has an else, parse it.
   SourceLocation ElseLoc;
   SourceLocation ElseStmtLoc;
   StmtResult ElseStmt;
 
   if (Tok.is(tok::kw_else)) {
     if (TrailingElseLoc)
       *TrailingElseLoc = Tok.getLocation();
if (Tok.is(tok::kw_else)) {
     //
     ParseScope InnerScope(this, Scope::DeclScope, C99orCXX,
                           Tok.is(tok::l_brace));
 
     EnterExpressionEvaluationContext PotentiallyDiscarded(
         Actions, Sema::ExpressionEvaluationContext::DiscardedStatement, nullptr,
         Sema::ExpressionEvaluationContextRecord::EK_Other,
         /*ShouldEnter=*/ConstexprCondition && *ConstexprCondition);
     ElseStmt = ParseStatement();
 
     // Pop the 'else' scope if needed.
     InnerScope.Exit();
+    if (ElseStmt.isUsable())
+      MaybeDiagnoseMisleadingIndentation(
+          *this, Tok, ElseStmt.get()->getBeginLoc(), ElseLoc, "else");
   } else if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteAfterIf(getCurScope());
     cutOffParsing();
     return StmtError();
   } else if (InnerStatementTrailingElseLoc.isValid()) {
     Diag(InnerStatementTrailingElseLoc, diag::warn_dangling_else);
   }
 
StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
 
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
   // Pop the body scope if needed.
   InnerScope.Exit();
   WhileScope.Exit();
 
+  if (Body.isUsable())
+    MaybeDiagnoseMisleadingIndentation(*this, Tok, Body.get()->getBeginLoc(),
+                                        WhileLoc, "while");
+
   if (Cond.isInvalid() || Body.isInvalid())
     return StmtError();
 
   return Actions.ActOnWhileStmt(WhileLoc, Cond, Body.get());
 }
 
 /// ParseDoStatement
 ///       do-statement: [C99 6.8.5.2]
StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
   // Pop the body scope if needed.
   InnerScope.Exit();
 
   // Leave the for-scope.
   ForScope.Exit();
 
+  if (Body.isUsable())
+    MaybeDiagnoseMisleadingIndentation(*this, Tok, Body.get()->getBeginLoc(),
+                                        ForLoc, "for");
+
   if (Body.isInvalid())
     return StmtError();
 
   if (ForEach)
    return Actions.FinishObjCForCollectionStmt(ForEachStmt.get(),
                                               Body.get());
 
   if (ForRangeInfo.ParsedForRangeDecl())

clang/test/SemaCXX/warn-misleanding-indentation.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -DWITH_WARN %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-misleading-indentation %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i = i + 1;
+    i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{indention is missleading}}
+#endif
+  return;
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i = i + 1;
+     i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{indention is missleading}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{indention is missleading}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+    i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i *= 2;
+    return;
+#ifdef WITH_WARN
+// expected-warning@-2 {{indention is missleading}}
+#endif
+}