Merge pull request #430 from geoffw0/exprtemplate

CPP: Exclude template code from ExprHasNoEffect.ql

Author: jbj

change-notes/1.19/analysis-cpp.md

@@ -16,6 +16,7 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. |
+| Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. |
 | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
 | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type. |

cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql

@@ -100,6 +100,7 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql
       not accessInInitOfForStmt(peivc) and
       not peivc.isCompilerGenerated() and
       not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and
+      not peivc.isFromTemplateInstantiation(_) and
       parent = peivc.getParent() and
       not parent.isInMacroExpansion() and
       not parent instanceof PureExprInVoidContext and

cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp

@@ -5,26 +5,26 @@ int external();
 class Base {
 public:
   virtual int thingy() {
-    1;
+    1; // BAD
   }
 
   int our_thingy() {
-    Base::thingy();
+    Base::thingy(); // BAD
     return 2;
   }
 };
 
 class Derived : public Base {
 public:
   virtual int thingy() {
-    external();
+    external(); // GOOD
     return 3;
   }
 };
 
 void internal() {
   Base* ptr = new Derived();
-  ptr->thingy();
+  ptr->thingy(); // GOOD
 }
 
 }

cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected

@@ -1,6 +1,5 @@
 | calls.cpp:8:5:8:5 | 1 | This expression has no effect. | calls.cpp:8:5:8:5 | 1 |  |
 | calls.cpp:12:5:12:16 | call to thingy | This expression has no effect (because $@ has no external side effects). | calls.cpp:7:15:7:20 | thingy | thingy |
-| templatey.cpp:4:3:4:8 | ... << ... | This expression has no effect. | templatey.cpp:4:3:4:8 | ... << ... |  |
 | templatey.cpp:39:3:39:23 | call to pointless_add_numbers | This expression has no effect (because $@ has no external side effects). | templatey.cpp:29:5:29:25 | pointless_add_numbers | pointless_add_numbers |
 | volatile.c:9:5:9:5 | c | This expression has no effect. | volatile.c:9:5:9:5 | c |  |
 | volatile.c:12:5:12:9 | access to array | This expression has no effect. | volatile.c:12:5:12:9 | access to array |  |

cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp

@@ -1,7 +1,7 @@
 template <typename T>
 void foo(T x, T y)
 {
-  x << y;
+  x << y; // GOOD (effect depends on T)
 };
 
 struct streamable
@@ -15,9 +15,9 @@ void operator<<(streamable& lhs, streamable& rhs)
 int main()
 {
   int x = 3;
-  foo(x, x);
+  foo(x, x); // BAD [NOT DETECTED]
   streamable y;
-  foo(y, y);
+  foo(y, y); // BAD [NOT DETECTED]
   return 0;
 }
 
@@ -34,7 +34,7 @@ int pointless_add_numbers(int lhs, int rhs)
 void call_add_numbers()
 {
   int accum = 0;
-  add_numbers(accum, 4);
-  add_numbers(accum, 10);
-  pointless_add_numbers(accum, 20);
+  add_numbers(accum, 4); // GOOD
+  add_numbers(accum, 10); // GOOD
+  pointless_add_numbers(accum, 20); // BAD
 }

cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c

@@ -6,18 +6,18 @@ char *pc;
 volatile char *pv;
 
 void f(void) {
-    c;
-    v;
+    c; // BAD
+    v; // (accesses to volatile variables are considered impure)
 
-    pc[5];
+    pc[5]; // BAD
     pv[5];
     ((volatile char *)pc)[5];
 
-    *pc;
+    *pc; // BAD
     *pv;
     *((volatile char *)pc);
 
-    *(pc + 5);
+    *(pc + 5); // BAD
     *(pv + 5);
     *((volatile char *)(pc + 5));
     *(((volatile char *)pc) + 5);

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected

@@ -1,5 +1,6 @@
 | preproc.c:89:2:89:4 | call to fn4 | This expression has no effect (because $@ has no external side effects). | preproc.c:33:5:33:7 | fn4 | fn4 |
 | preproc.c:94:2:94:4 | call to fn9 | This expression has no effect (because $@ has no external side effects). | preproc.c:78:5:78:7 | fn9 | fn9 |
+| template.cpp:19:3:19:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | template.cpp:9:10:9:19 | operator++ | operator++ |
 | test.c:7:5:7:5 | 0 | This expression has no effect. | test.c:7:5:7:5 | 0 |  |
 | test.c:9:8:9:8 | 1 | This expression has no effect. | test.c:9:8:9:8 | 1 |  |
 | test.c:9:11:9:11 | 2 | This expression has no effect. | test.c:9:11:9:11 | 2 |  |
@@ -19,11 +20,6 @@
 | test.c:26:15:26:16 | 32 | This expression has no effect. | test.c:26:15:26:16 | 32 |  |
 | test.c:27:9:27:10 | 33 | This expression has no effect. | test.c:27:9:27:10 | 33 |  |
 | test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
-| test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
-| test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
-| test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
-| test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
 | test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
-| test.cpp:26:3:26:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ |
 | test.cpp:62:5:62:5 | call to operator= | This expression has no effect (because $@ has no external side effects). | test.cpp:47:14:47:22 | operator= | operator= |
 | test.cpp:65:5:65:5 | call to operator= | This expression has no effect (because $@ has no external side effects). | test.cpp:55:7:55:7 | operator= | operator= |

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c

@@ -0,0 +1,7 @@
+
+#define UNUSED(x) (x)
+
+void test2(int param)
+{
+	UNUSED(param); // GOOD
+}

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp

@@ -0,0 +1,22 @@
+
+template<class T>
+void Increment(T &t) {
+	t++; // GOOD (sometimes has an effect)
+}
+
+class Nothing {
+public:
+	Nothing operator++(int) {
+		return *this; // do nothing
+	}
+};
+
+void myTemplateTest() {
+	int i = 0;
+	Nothing n;
+
+	i++; // GOOD (always has an effect)
+	n++; // BAD (never has an effect)
+	Increment(i);
+	Increment(n);
+}

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp

@@ -23,7 +23,7 @@ class MyTemplateClass
 
 		++arg1; // pure, does nothing
 		++arg2; // pure, does nothing
-		++arg3; // not pure in all cases (when _It is int this has a side-effect) [FALSE POSITIVE]
+		++arg3; // not pure in all cases (when _It is int this has a side-effect)
 
 		return arg2;
 	}