diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -693,6 +693,7 @@ 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). @@ -884,7 +885,7 @@ // 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]>; diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td --- a/clang/include/clang/Basic/DiagnosticParseKinds.td +++ b/clang/include/clang/Basic/DiagnosticParseKinds.td @@ -61,6 +61,13 @@ "remove unnecessary ';' to silence this warning">, InGroup, DefaultIgnore; +def warn_misleading_indentation : Warning< + "misleading indentation; statement is not part of " + "the previous '%select{if|else|for|while|else if}0'">, + InGroup, 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 " diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h --- a/clang/include/clang/Lex/Preprocessor.h +++ b/clang/include/clang/Lex/Preprocessor.h @@ -932,6 +932,12 @@ 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; diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -1122,6 +1122,11 @@ /// point for skipping past a simple-declaration. void SkipMalformedDecl(); + /// The location of the first statement inside an else that might + /// have a missleading indentation. If there is no + /// MisleadingIndentationChecker on an else active, this location is invalid. + SourceLocation MisleadingIndentationElseLoc; + private: //===--------------------------------------------------------------------===// // Lexing and parsing of C++ inline methods. diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp --- a/clang/lib/Parse/ParseStmt.cpp +++ b/clang/lib/Parse/ParseStmt.cpp @@ -1191,6 +1191,59 @@ return false; } +namespace { + +enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while }; + +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)) { + if (!P.MisleadingIndentationElseLoc.isInvalid()) { + StmtLoc = P.MisleadingIndentationElseLoc; + P.MisleadingIndentationElseLoc = SourceLocation(); + } + if (Kind == MSK_else && !ShouldSkip) + P.MisleadingIndentationElseLoc = SL; + } + void Check() { + NeedsChecking = false; + Token Tok = P.getCurToken(); + if (ShouldSkip || NumDirectives != P.getPreprocessor().getNumDirectives() || + Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() || + Tok.getLocation().isMacroID() || PrevLoc.isMacroID() || + StmtLoc.isMacroID() || + (Kind == MSK_else && P.MisleadingIndentationElseLoc.isInvalid())) { + P.MisleadingIndentationElseLoc = SourceLocation(); + 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()) && SM.getPresumedLineNumber(StmtLoc) != + SM.getPresumedLineNumber(Tok.getLocation())) { + P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) + << Kind; + P.Diag(StmtLoc, diag::note_previous_statement); + } + } +}; + +} /// ParseIfStatement /// if-statement: [C99 6.8.4.1] @@ -1265,6 +1318,8 @@ // ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace)); + MisleadingIndentationChecker MIChecker(*this, MSK_if, IfLoc); + // Read the 'then' stmt. SourceLocation ThenStmtLoc = Tok.getLocation(); @@ -1278,6 +1333,9 @@ ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc); } + if (Tok.isNot(tok::kw_else)) + MIChecker.Check(); + // Pop the 'if' scope if needed. InnerScope.Exit(); @@ -1305,12 +1363,17 @@ 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); ElseStmt = ParseStatement(); + if (ElseStmt.isUsable()) + MIChecker.Check(); + // Pop the 'else' scope if needed. InnerScope.Exit(); } else if (Tok.is(tok::code_completion)) { @@ -1484,9 +1547,13 @@ // 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)); + if (Body.isUsable()) + MIChecker.Check(); // Pop the body scope if needed. InnerScope.Exit(); WhileScope.Exit(); @@ -1918,9 +1985,14 @@ if (C99orCXXorObjC) getCurScope()->decrementMSManglingNumber(); + MisleadingIndentationChecker MIChecker(*this, MSK_for, ForLoc); + // Read the body statement. StmtResult Body(ParseStatement(TrailingElseLoc)); + if (Body.isUsable()) + MIChecker.Check(); + // Pop the body scope if needed. InnerScope.Exit(); diff --git a/clang/test/Index/pragma-diag-reparse.c b/clang/test/Index/pragma-diag-reparse.c --- a/clang/test/Index/pragma-diag-reparse.c +++ b/clang/test/Index/pragma-diag-reparse.c @@ -11,6 +11,7 @@ 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 \ diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c --- a/clang/test/Misc/warning-wall.c +++ b/clang/test/Misc/warning-wall.c @@ -90,6 +90,7 @@ CHECK-NEXT: -Wdangling-else CHECK-NEXT: -Wswitch CHECK-NEXT: -Wswitch-bool +CHECK-NEXT: -Wmisleading-indentation CHECK-NOT:-W diff --git a/clang/test/Parser/warn-misleading-indentation.cpp b/clang/test/Parser/warn-misleading-indentation.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Parser/warn-misleading-indentation.cpp @@ -0,0 +1,208 @@ +// 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@-3 {{here}} +#endif + i = 4; + i = 5; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous '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 '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; +} \ No newline at end of file diff --git a/clang/test/Preprocessor/pragma_diagnostic_sections.cpp b/clang/test/Preprocessor/pragma_diagnostic_sections.cpp --- a/clang/test/Preprocessor/pragma_diagnostic_sections.cpp +++ b/clang/test/Preprocessor/pragma_diagnostic_sections.cpp @@ -1,4 +1,4 @@ -// 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 {