compiler plugin check for if/while/true bodies with possibly {} missing

Change-Id: Ia84c70006b0b8a039b6fea27f3c5cde796f25d03

5 files changed, 206 insertions, 1 deletions

diff --git a/compilerplugins/Makefile-clang.mk b/compilerplugins/Makefile-clang.mk
index 9b37fb9939c7..ca95f11aaff3 100644
--- a/compilerplugins/Makefile-clang.mk
+++ b/compilerplugins/Makefile-clang.mk
@@ -9,7 +9,10 @@
 # Make sure variables in this Makefile do not conflict with other variables (e.g. from gbuild).
 
 # The list of source files.
-CLANGSRC=compileplugin.cxx unusedvariablecheck.cxx
+CLANGSRC=compileplugin.cxx \
+    bodynotinblock.cxx \
+    unusedvariablecheck.cxx \
+
 
 # You may occassionally want to override some of these
 
diff --git a/compilerplugins/README b/compilerplugins/README
index 2430d40fb72e..50c7505dd72e 100644
--- a/compilerplugins/README
+++ b/compilerplugins/README
@@ -28,6 +28,18 @@ All warnings and errors are marked '[loplugin]' in the message.
 
 Additional check for unused variables.
 
+==== Body of if/while/for not in {} ====
+
+- statement aligned as second statement in if/while/for body but not in a statement block [loplugin]
+
+Warn about the following construct:
+
+    if( a != 0 )
+        b = 2;
+        c = 3;
+        
+Here either both statements should be inside {} or the second statement in indented wrong.
+
 
 == Code documentation / howtos ==
 
diff --git a/compilerplugins/clang/bodynotinblock.cxx b/compilerplugins/clang/bodynotinblock.cxx
new file mode 100644
index 000000000000..9f5bf0e75c4e
--- /dev/null
+++ b/compilerplugins/clang/bodynotinblock.cxx
@@ -0,0 +1,147 @@
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * Based on LLVM/Clang.
+ *
+ * This file is distributed under the University of Illinois Open Source
+ * License. See LICENSE.TXT for details.
+ *
+ */
+
+#include "bodynotinblock.hxx"
+
+#include <clang/Basic/SourceManager.h>
+
+using namespace clang;
+
+namespace loplugin
+{
+
+/*
+Check for two statements that are both indented to look like a body of if/while/for
+but are not inside a compound statement and thus the second one is unrelated.
+*/
+
+BodyNotInBlock::BodyNotInBlock( ASTContext& context )
+    : context( context )
+    {
+    }
+
+DiagnosticBuilder BodyNotInBlock::report( DiagnosticsEngine::Level level, StringRef message, SourceLocation loc )
+    {
+    // Do some mappings (e.g. for -Werror) that clang does not do for custom messages for some reason.
+    DiagnosticsEngine& diag = context.getDiagnostics();
+    if( level == DiagnosticsEngine::Warning && diag.getWarningsAsErrors())
+        level = DiagnosticsEngine::Error;
+    if( level == DiagnosticsEngine::Error && diag.getErrorsAsFatal())
+        level = DiagnosticsEngine::Fatal;
+    return diag.Report( loc, diag.getCustomDiagID( level, message ));
+    }
+
+void BodyNotInBlock::run()
+    {
+    TraverseDecl( context.getTranslationUnitDecl());
+    }
+
+bool BodyNotInBlock::VisitFunctionDecl( FunctionDecl* declaration )
+    {
+    // TODO also LO header files? or a subdir?
+    if( !context.getSourceManager().isFromMainFile( declaration->getLocStart()))
+        return true;
+    if( !declaration->doesThisDeclarationHaveABody())
+        return true;
+    StmtParents parents;
+    traverseStatement( declaration->getBody(), parents );
+    return true;
+    }
+
+void BodyNotInBlock::traverseStatement( const Stmt* stmt, StmtParents& parents )
+    {
+    parents.push_back( stmt );
+    for( ConstStmtIterator it = stmt->child_begin();
+         it != stmt->child_end();
+         ++it )
+        {
+        if( *it != NULL ) // some children can be apparently NULL
+            {
+            traverseStatement( *it, parents ); // substatements first
+            parents.push_back( *it );
+            if( const IfStmt* ifstmt = dyn_cast< IfStmt >( *it ))
+                {
+                checkBody( ifstmt->getThen(), parents, 0 );
+                checkBody( ifstmt->getElse(), parents, 0 );
+                }
+            else if( const WhileStmt* whilestmt = dyn_cast< WhileStmt >( *it ))
+                checkBody( whilestmt->getBody(), parents, 1 );
+            else if( const ForStmt* forstmt = dyn_cast< ForStmt >( *it ))
+                checkBody( forstmt->getBody(), parents, 2 );
+            else if( const CXXForRangeStmt* forstmt = dyn_cast< CXXForRangeStmt >( *it ))
+                checkBody( forstmt->getBody(), parents, 2 );
+            parents.pop_back();
+            }
+        }
+    assert( parents.back() == stmt );
+    parents.pop_back();
+    }
+
+void BodyNotInBlock::checkBody( const Stmt* body, const StmtParents& parents, int stmtType )
+    {
+    if( body == NULL || parents.size() < 2 )
+        return;
+    if( dyn_cast< CompoundStmt >( body ))
+        return; // if body is a compound statement, then it is in {}
+    // Find the next statement (in source position) after 'body'.
+    for( int parent_pos = parents.size() - 2; // start from grandparent
+         parent_pos >= 0;
+         --parent_pos )
+        {
+        for( ConstStmtIterator it = parents[ parent_pos ]->child_begin();
+             it != parents[ parent_pos ]->child_end();
+             )
+            {
+            if( *it == parents[ parent_pos + 1 ] ) // found grand(grand...)parent
+                {
+                // get next statement after our (grand...)parent
+                ++it;
+                while( it != parents[ parent_pos ]->child_end() && *it == NULL )
+                    ++it; // skip empty ones (missing 'else' bodies for example)
+                if( it != parents[ parent_pos ]->child_end())
+                    {
+                    // TODO: If both statements come from macro expansions, they may be
+                    // below evaluated to be in the same place (because they may be
+                    // the result of expansion of the same macro). Analysing this including
+                    // macro expansions would be probably more complicated, so just
+                    // ignore that in order to avoid false positives.
+                    if( body->getLocStart().isMacroID() && (*it)->getLocStart().isMacroID())
+                        return;
+                    bool invalid1, invalid2;
+                    unsigned bodyColumn = context.getSourceManager()
+                        .getPresumedColumnNumber( body->getLocStart(), &invalid1 );
+                    unsigned nextStatementColumn = context.getSourceManager()
+                        .getPresumedColumnNumber( (*it)->getLocStart(), &invalid2 );
+                    if( invalid1 || invalid2 )
+                        return;
+                    if( bodyColumn == nextStatementColumn )
+                        {
+                        report( DiagnosticsEngine::Warning,
+                            "statement aligned as second statement in %select{if|while|for}0 body but not in a statement block [loplugin]",
+                            (*it)->getLocStart()) << stmtType;
+                        report( DiagnosticsEngine::Note,
+                            "%select{if|while|for}0 body statement is here [loplugin]",
+                            body->getLocStart()) << stmtType;
+                        }
+                    return;
+                    }
+                // else we need to go higher to find the next statement
+                }
+            else
+                ++it;
+            }
+        // If going up would mean leaving a {} block, stop, because the } should
+        // make it visible the two statements are not in the same body.
+        if( dyn_cast< CompoundStmt >( parents[ parent_pos ] ))
+            return;
+        }
+    }
+
+} // namespace
diff --git a/compilerplugins/clang/bodynotinblock.hxx b/compilerplugins/clang/bodynotinblock.hxx
new file mode 100644
index 000000000000..dba82a67917e
--- /dev/null
+++ b/compilerplugins/clang/bodynotinblock.hxx
@@ -0,0 +1,39 @@
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * Based on LLVM/Clang.
+ *
+ * This file is distributed under the University of Illinois Open Source
+ * License. See LICENSE.TXT for details.
+ *
+ */
+
+#ifndef BODYNOTINBLOCK_H
+#define BODYNOTINBLOCK_H
+
+#include <clang/AST/RecursiveASTVisitor.h>
+
+using namespace clang;
+
+namespace loplugin
+{
+
+typedef std::vector< const Stmt* > StmtParents;
+
+class BodyNotInBlock
+    : public RecursiveASTVisitor< BodyNotInBlock >
+    {
+    public:
+        explicit BodyNotInBlock( ASTContext& context );
+        void run();
+        bool VisitFunctionDecl( FunctionDecl* declaration );
+    private:
+        void traverseStatement( const Stmt* stmt, StmtParents& parents );
+        void checkBody( const Stmt* body, const StmtParents& parents, int stmtType );
+        DiagnosticBuilder report( DiagnosticsEngine::Level level, StringRef message, SourceLocation loc );
+        ASTContext& context;
+    };
+
+} // namespace
+
+#endif // BODYNOTINBLOCK_H
diff --git a/compilerplugins/clang/compileplugin.cxx b/compilerplugins/clang/compileplugin.cxx
index 2b1647560117..d2e32c04a5d8 100644
--- a/compilerplugins/clang/compileplugin.cxx
+++ b/compilerplugins/clang/compileplugin.cxx
@@ -17,6 +17,7 @@
 #include <clang/Frontend/FrontendPluginRegistry.h>
 #include <clang/Rewrite/Rewriter.h>
 
+#include "bodynotinblock.hxx"
 #include "unusedvariablecheck.hxx"
 
 using namespace clang;
@@ -33,6 +34,7 @@ class PluginHandler
     public:
         explicit PluginHandler( ASTContext& context )
             : rewriter( context.getSourceManager(), context.getLangOpts())
+            , bodyNotInBlock( context )
             , unusedVariableCheck( context )
             {
             }
@@ -40,6 +42,7 @@ class PluginHandler
             {
             if( context.getDiagnostics().hasErrorOccurred())
                 return;
+            bodyNotInBlock.run();
             unusedVariableCheck.run();
             // TODO also LO header files? or a subdir?
            if( const RewriteBuffer* buf = rewriter.getRewriteBufferFor( context.getSourceManager().getMainFileID()))
@@ -48,6 +51,7 @@ class PluginHandler
             }
     private:
         Rewriter rewriter;
+        BodyNotInBlock bodyNotInBlock;
         UnusedVariableCheck unusedVariableCheck;
     };