[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 ↗

(On Diff #230785)

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 ↗

(On Diff #230788)

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 ↗

(On Diff #230788)

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

2271

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.

1376

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

1376

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

1376

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

1371–1377

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

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", [
     ObjCMissingSuperCalls,
     ObjCDesignatedInit,
     ObjCFlexibleArray,
     OverloadedVirtual,
     PrivateExtern,
     SelTypeCast,
     ExternCCompat,
     UserDefinedWarnings
  ]>;
 
 // 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">;
 def ThreadSafety : DiagGroup<"thread-safety",
                              [ThreadSafetyAttributes,
                               ThreadSafetyAnalysis,
                               ThreadSafetyPrecise,
                               ThreadSafetyReference]>;
 def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
 def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 
 // Uniqueness Analysis warnings
 def Consumed       : DiagGroup<"consumed">;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
-def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool]>;
+def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, MisleadingIndentation]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;
 
 // Warnings enabled by -pedantic.  This is magically filled in by TableGen.
 def Pedantic : DiagGroup<"pedantic">;
 
 // Aliases.

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<
+  "misleading indentation; statement is not part of "
+  "the previous '%select{if|else|for|while|else if}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/include/clang/Lex/Preprocessor.h

public:
 
   /// Retrieve the module loader associated with this preprocessor.
   ModuleLoader &getModuleLoader() const { return TheModuleLoader; }
 
   bool hadModuleLoaderFatalFailure() const {
     return TheModuleLoader.HadFatalFailure;
   }
 
+  /// Retrieve the number of Directives that have been processed by the
+  /// Preprocessor.
+  unsigned getNumDirectives() const {
+    return NumDirectives;
+  }
+
   /// True if we are currently preprocessing a #if or #elif directive
   bool isParsingIfOrElifDirective() const {
     return ParsingIfOrElifDirective;
   }
 
   /// Control whether the preprocessor retains comments in output.
   void SetCommentRetentionState(bool KeepComments, bool KeepMacroComments) {
     this->KeepComments = KeepComments | KeepMacroComments;

clang/include/clang/Parse/Parser.h

StmtResult ParseCompoundStatement(bool isStmtExpr,
                                     unsigned ScopeFlags);
   void ParseCompoundStatementLeadingPragmas();
   bool ConsumeNullStmt(StmtVector &Stmts);
   StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
   bool ParseParenExprOrCondition(StmtResult *InitStmt,
                                  Sema::ConditionResult &CondResult,
                                  SourceLocation Loc,
                                  Sema::ConditionKind CK);
-  StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc);
+  StmtResult
+  ParseIfStatement(SourceLocation *TrailingElseLoc,
+                   SourceLocation ElseLocOnElseIf = SourceLocation());
   StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseDoStatement();
   StmtResult ParseForStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseGotoStatement();
   StmtResult ParseContinueStatement();
   StmtResult ParseBreakStatement();
   StmtResult ParseReturnStatement();
private:
   /// is in parenthesis.
   bool isTypeIdUnambiguously() {
     bool IsAmbiguous;
     if (getLangOpts().CPlusPlus)
       return isCXXTypeId(TypeIdUnambiguous, IsAmbiguous);
     return isTypeSpecifierQualifier();
   }
 
   /// isCXXDeclarationStatement - C++-specialized function that disambiguates
   /// between a declaration or an expression statement, when parsing function
   /// bodies. Returns true for declaration, false for expression.
   bool isCXXDeclarationStatement();
 
   /// isCXXSimpleDeclaration - C++-specialized function that disambiguates
   /// between a simple-declaration or an expression-statement.
   /// If during the disambiguation process a parsing error is encountered,
   /// the function returns true to let the declaration parsing code handle it.

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;
 }
 
+namespace {
+
+enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while, MSK_else_if };
+
+struct MisleadingIndentationChecker {
+  Parser &P;
+  SourceLocation StmtLoc;
+  SourceLocation PrevLoc;
+  unsigned NumDirectives;
+  MisleadingStatementKind Kind;
+  bool NeedsChecking;
+  bool ShouldSkip;
+  MisleadingIndentationChecker(Parser &P, MisleadingStatementKind K,
+                               SourceLocation SL)
+      : P(P), StmtLoc(SL), PrevLoc(P.getCurToken().getLocation()),
+        NumDirectives(P.getPreprocessor().getNumDirectives()), Kind(K),
+        NeedsChecking(true),
+        ShouldSkip(P.getCurToken().is(tok::l_brace) ||
+                   (K == MSK_else && P.getCurToken().is(tok::kw_if))) {}
+  void Check(bool ShouldCheck) {
+    NeedsChecking = false;
+    Token Tok = P.getCurToken();
+    if (!ShouldCheck || ShouldSkip ||
+        NumDirectives != P.getPreprocessor().getNumDirectives() ||
+        Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() ||
+        Tok.getLocation().isMacroID() || PrevLoc.isMacroID() ||
+        StmtLoc.isMacroID())
+      return;
+    SourceManager &SM = P.getPreprocessor().getSourceManager();
+    unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
+    unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
+    unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+
+    if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
+        ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
+         !Tok.isAtStartOfLine())) {
+      if (SM.getPresumedLineNumber(StmtLoc) ==
+          SM.getPresumedLineNumber(Tok.getLocation()))
+        return;
+      P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
+          << Kind;
+      P.Diag(StmtLoc, diag::note_previous_statement);
+    }
+  }
+  ~MisleadingIndentationChecker() {
+    assert(!NeedsChecking && "Check Has not been called");
+  }
+};
+
+}
 
 /// 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) {
+StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc,
+                                    SourceLocation ElseLocOnElseIf) {
   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;
StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc,
   // -Sema will detect name clashes by considering declarations of a
   //    'ControlScope' as part of its direct subscope.
   // -If we wanted the condition and substatement to be in the same scope, we
   //    would have to notify ParseStatement not to create a new scope. It's
   //    simpler to let it create a new scope.
   //
   ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace));
 
+  MisleadingIndentationChecker MIChecker(
+      *this, ElseLocOnElseIf.isInvalid() ? MSK_if : MSK_else_if,
+      ElseLocOnElseIf.isInvalid() ? IfLoc : ElseLocOnElseIf);
+
   // Read the 'then' stmt.
   SourceLocation ThenStmtLoc = Tok.getLocation();
 
   SourceLocation InnerStatementTrailingElseLoc;
   StmtResult ThenStmt;
   {
     EnterExpressionEvaluationContext PotentiallyDiscarded(
         Actions, Sema::ExpressionEvaluationContext::DiscardedStatement, nullptr,
         Sema::ExpressionEvaluationContextRecord::EK_Other,
         /*ShouldEnter=*/ConstexprCondition && !*ConstexprCondition);
     ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc);
   }
 
+  MIChecker.Check(Tok.isNot(tok::kw_else));
+
   // Pop the 'if' scope if needed.
   InnerScope.Exit();
 
   // If it has an else, parse it.
   SourceLocation ElseLoc;
   SourceLocation ElseStmtLoc;
   StmtResult ElseStmt;
 
if (Tok.is(tok::kw_else)) {
     //
     // C++ 6.4p1:
     // The substatement in a selection-statement (each substatement, in the else
     // form of the if statement) implicitly defines a local scope.
     //
     ParseScope InnerScope(this, Scope::DeclScope, C99orCXX,
                           Tok.is(tok::l_brace));
 
+    MisleadingIndentationChecker MIChecker(*this, MSK_else, ElseLoc);
+
     EnterExpressionEvaluationContext PotentiallyDiscarded(
         Actions, Sema::ExpressionEvaluationContext::DiscardedStatement, nullptr,
         Sema::ExpressionEvaluationContextRecord::EK_Other,
         /*ShouldEnter=*/ConstexprCondition && *ConstexprCondition);
+    if (Tok.is(tok::kw_if))
+      ElseStmt = ParseIfStatement(nullptr, ElseLoc);
+    else
       ElseStmt = ParseStatement();
 
+    MIChecker.Check(ElseStmt.isUsable());
+
     // Pop the 'else' scope if needed.
     InnerScope.Exit();
   } 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) {
   // The substatement in an iteration-statement implicitly defines a local scope
   // which is entered and exited each time through the loop.
   //
   // See comments in ParseIfStatement for why we create a scope for the
   // condition and a new scope for substatement in C++.
   //
   ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace));
 
+  MisleadingIndentationChecker MIChecker(*this, MSK_while, WhileLoc);
+
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
+  MIChecker.Check(Body.isUsable());
   // Pop the body scope if needed.
   InnerScope.Exit();
   WhileScope.Exit();
 
   if (Cond.isInvalid() || Body.isInvalid())
     return StmtError();
 
   return Actions.ActOnWhileStmt(WhileLoc, Cond, Body.get());
StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
 
   // The body of the for loop has the same local mangling number as the
   // for-init-statement.
   // It will only be incremented if the body contains other things that would
   // normally increment the mangling number (like a compound statement).
   if (C99orCXXorObjC)
     getCurScope()->decrementMSManglingNumber();
 
+  MisleadingIndentationChecker MIChecker(*this, MSK_for, ForLoc);
+
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
+  MIChecker.Check(Body.isUsable());
+
   // Pop the body scope if needed.
   InnerScope.Exit();
 
   // Leave the for-scope.
   ForScope.Exit();
 
   if (Body.isInvalid())
     return StmtError();

clang/test/Index/pragma-diag-reparse.c

 #pragma clang diagnostic ignored "-Wtautological-compare"
 #include "pragma_disable_warning.h"
 
 int main (int argc, const char * argv[])
 {
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
   int x=0;
 #pragma clang diagnostic pop
 
   return x;
 }
 
+#pragma clang diagnostic ignored "-Wmisleading-indentation"
 void foo() { int b=0; while (b==b); }
 
 // RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_FAILONERROR=1 c-index-test -test-load-source-reparse 5 local \
 // RUN: -I%S/Inputs \
 // RUN:   %s -Wall -Werror | FileCheck %s
 
 // CHECK: pragma-diag-reparse.c:8:7: VarDecl=x:8:7 (Definition) Extent=[8:3 - 8:10]

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wbitwise-conditional-parentheses
 CHECK-NEXT:    -Wbitwise-op-parentheses
 CHECK-NEXT:    -Wshift-op-parentheses
 CHECK-NEXT:    -Woverloaded-shift-op-parentheses
 CHECK-NEXT:    -Wparentheses-equality
 CHECK-NEXT:    -Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
+CHECK-NEXT:  -Wmisleading-indentation
 
 
 CHECK-NOT:-W

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

+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %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;
+    int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i = 0;
+    i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+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 {{misleading indentation; statement is not part of the previous 'for'}}
+#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 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+    i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i *= 2;
+    const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+    i *= 2;
+    ;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i *= 2;
+    typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i *= 2;
+    using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+    if (dst)
+       return
+    bar();
+  if (dst)
+    dst = dst + \
+    bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+    i = 2;
+  else
+         if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+    i = 4;
+    i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+    i = 2;
+  else
+    i = 5;
+    TEST;
+}
+
+void g1(int i) {
+  if (1)
+    i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+      i = 4;
+      i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+    i = 2;
+  else
+    if (i == 3)
+    {i = 4;}
+    i = 5;
+}
+
+void g6(int i) {
+        if (1)
+                if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+                        i = 4;
+                        i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)
+    i = 4;
+#ifdef TEST1
+#endif
+    i = 5;
+}
+
+void a1(int i) { if (1) i = 4; return; }
+
+void a2(int i) {
+  {
+    if (1)
+      i = 4;
+      }
+  return;
+}
+
+void a3(int i) {
+  if (1)
+    {
+    i = 4;
+    }
+    return;
+}

clang/test/Preprocessor/pragma_diagnostic_sections.cpp

-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
     void m1() { int b; while (b==b); } // expected-warning {{always evaluates to true}}
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wtautological-compare"
     void m2() { int b; while (b==b); }