Merge pull request #1237 from geoffw0/commentedoutcode2

CPP: Fix FPs from detecting commented out preprocessor logic

cpp/ql/src/Documentation/CommentedOutCode.qll

@@ -47,12 +47,38 @@ private predicate looksLikeCode(string line) {
   )
 }
 
+/**
+ * Holds if there is a preprocessor directive on the line indicated by
+ * `f` and `line` that we permit code comments besides.  For example this
+ * is considered acceptable:
+ * ```
+ * #ifdef MYMACRO
+ * ...
+ * #endif // #ifdef MYMACRO
+ * ```
+ */
+private predicate preprocLine(File f, int line) {
+  exists(PreprocessorDirective pd, Location l |
+    (
+      pd instanceof PreprocessorElse or
+      pd instanceof PreprocessorElif or
+      pd instanceof PreprocessorEndif
+    ) and
+    pd.getLocation() = l and
+    l.getFile() = f and
+    l.getStartLine() = line
+  )
+}
+
 /**
  * The line of a C++-style comment within its file `f`.
  */
 private int lineInFile(CppStyleComment c, File f) {
   f = c.getFile() and
-  result = c.getLocation().getStartLine()
+  result = c.getLocation().getStartLine() and
+
+  // Ignore comments on the same line as a preprocessor directive.
+  not preprocLine(f, result)
 }
 
 /**
@@ -89,9 +115,17 @@ private int commentId(CppStyleComment c, File f, int line) {
  */
 class CommentBlock extends Comment {
   CommentBlock() {
-    this instanceof CppStyleComment
-    implies
-    not exists(CppStyleComment pred, File f | lineInFile(pred, f) + 1 = lineInFile(this, f))
+    (
+      this instanceof CppStyleComment
+      implies
+      not exists(CppStyleComment pred, File f | lineInFile(pred, f) + 1 = lineInFile(this, f))
+    ) and (
+      // Ignore comments on the same line as a preprocessor directive.
+      not exists(Location l |
+        l = this.getLocation() and
+        preprocLine(l.getFile(), l.getStartLine())
+      )
+    )
   }
 
   /**
@@ -133,6 +167,7 @@ class CommentBlock extends Comment {
 
   predicate isCommentedOutCode() {
     not this.isDocumentation() and
+    not this.getFile().(HeaderFile).noTopLevelCode() and
     this.numCodeLines().(float) / this.numLines().(float) > 0.5
   }
 

cpp/ql/src/jsf/4.07 Header Files/AV Rule 35.ql

@@ -133,11 +133,7 @@ where not hf instanceof IncludeGuardedHeader
   // Exclude files which contain no declaration entries or top level
   // declarations (e.g. just preprocessor directives; or non-top level
   // code).
-  and (
-    exists(DeclarationEntry de | de.getFile() = hf) or
-    exists(Declaration d | d.getFile() = hf and d.isTopLevel()) or
-    exists(UsingEntry ue | ue.getFile() = hf)
-  )
+  and not hf.noTopLevelCode()
   // Exclude files which look like they contain 'x-macros'
   and not hasXMacro(hf)
   // Exclude files which are always #imported.

cpp/ql/src/semmle/code/cpp/File.qll

@@ -453,6 +453,20 @@ class HeaderFile extends File {
     exists(Include i | i.getIncludedFile() = this)
   }
 
+  /**
+   * Holds if this header file does not contain any declaration entries or top level
+   * declarations.  For example it might be:
+   *  - a file containing only preprocessor directives and/or comments
+   *  - an empty file
+   *  - a file that contains non-top level code or data that's included in an
+   *    unusual way
+   */
+  predicate noTopLevelCode()
+  {
+    not exists(DeclarationEntry de | de.getFile() = this) and
+    not exists(Declaration d | d.getFile() = this and d.isTopLevel()) and
+    not exists(UsingEntry ue | ue.getFile() = this)
+  }
 }
 
 /**

cpp/ql/test/query-tests/Documentation/CommentedOutCode/CommentedOutCode.expected

@@ -8,6 +8,11 @@
 | test2.cpp:63:1:63:15 | // #pragma once | This comment appears to contain commented-out code |
 | test2.cpp:65:1:65:17 | //  # pragma once | This comment appears to contain commented-out code |
 | test2.cpp:67:1:67:19 | /*#error"myerror"*/ | This comment appears to contain commented-out code |
+| test2.cpp:91:1:95:2 | /*\n#ifdef MYMACRO\n\t// ...\n#endif // #ifdef MYMACRO\n*/ | This comment appears to contain commented-out code |
+| test2.cpp:107:21:107:43 | // #include "config2.h" | This comment appears to contain commented-out code |
+| test2.cpp:115:16:115:35 | /* #ifdef MYMACRO */ | This comment appears to contain commented-out code |
+| test2.cpp:117:1:117:24 | // commented_out_code(); | This comment appears to contain commented-out code |
+| test2.cpp:120:2:120:25 | // commented_out_code(); | This comment appears to contain commented-out code |
 | test.c:2:1:2:22 | // commented out code; | This comment appears to contain commented-out code |
 | test.c:4:1:7:8 | // some; | This comment appears to contain commented-out code |
 | test.c:9:1:13:8 | // also; | This comment appears to contain commented-out code |

cpp/ql/test/query-tests/Documentation/CommentedOutCode/config.h

@@ -0,0 +1,19 @@
+
+/* define if you want setting 1 */
+#define SETTING1
+
+// define if you want setting 2
+//#define SETTING2
+
+/* define if you want setting 3 */
+//#define SETTING3
+
+/* define if you want setting 4 */
+/* #define SETTING4 */
+
+#if defined(SETTING3) && defined(SETTING4)
+	#error "can't have both SETTING3 and SETTING4"
+#endif
+
+/* uncomment if you don't want setting 5 */
+/* #undef SETTING5 */

cpp/ql/test/query-tests/Documentation/CommentedOutCode/test2.cpp

@@ -65,3 +65,57 @@ void myFunction();
 //  # pragma once
 
 /*#error"myerror"*/
+
+#ifdef MYMACRO
+
+	// ...
+
+#endif // #ifdef MYMACRO
+
+#if !defined(MYMACRO)
+
+	// ...
+
+#else // #if !defined(MYMACRO)
+
+	// ...
+
+#endif // #else #if !defined(MYMACRO)
+
+#ifdef MYMACRO
+
+	// ...
+
+#endif   //    #ifdef MYMACRO       (comment)
+
+/*
+#ifdef MYMACRO
+	// ...
+#endif // #ifdef MYMACRO
+*/
+
+
+#ifdef MYMACRO1
+	#ifdef MYMACRO2
+
+		// ...
+
+		// comment at end of block
+	#endif // #ifdef MYMACRO2
+#endif // #ifdef MYMACRO1
+
+#include "config.h" // #include "config2.h"
+
+#ifdef MYMACRO
+
+	// ...
+
+#endif /* #ifdef MYMACRO */
+
+#error "error" /* #ifdef MYMACRO */
+
+// commented_out_code();
+
+#if 0
+	// commented_out_code();
+#endif