From 73b186a6952f0d12e40e5de4a9ea5bf34a2a3bf7 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 7 Nov 2018 13:33:18 +0000
Subject: [PATCH 1/7] CPP: Add test case.

---
 .../ExprHasNoEffect/ExprHasNoEffect.expected  |  2 ++
 .../Likely Typos/ExprHasNoEffect/template.cpp | 22 +++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp

diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected
index 20cc25fb99b1..379943d85823 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
@@ -1,5 +1,7 @@
 | 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:4:3:4:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | template.cpp:9:10:9:19 | operator++ | operator++ |
+| 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 |  |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp
new file mode 100644
index 000000000000..d0a922a3f981
--- /dev/null
+++ b/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) [FALSE POSITIVE]
+}
+
+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);
+}

From 7bf9200a18b1da9045e0d2d96ed3d0a43daf9eb5 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 7 Nov 2018 14:12:52 +0000
Subject: [PATCH 2/7] CPP: Fix (it looks like we already had a similar test,
 both are fixed.

---
 cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql      | 2 +-
 .../Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected   | 6 ------
 .../Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp   | 2 +-
 .../Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp       | 2 +-
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql
index 9e42cb5d2ea0..56715956635f 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql	
@@ -99,7 +99,7 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql
       not peivc.(FunctionCall).getTarget().hasName("operator==") and
       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
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected
index 379943d85823..f50c352fe340 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
@@ -1,6 +1,5 @@
 | 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:4:3:4:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | template.cpp:9:10:9:19 | operator++ | operator++ |
 | 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 |  |
@@ -21,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= |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp
index d0a922a3f981..ecc3d6246034 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp	
@@ -1,7 +1,7 @@
 
 template<class T>
 void Increment(T &t) {
-	t++; // GOOD (sometimes has an effect) [FALSE POSITIVE]
+	t++; // GOOD (sometimes has an effect)
 }
 
 class Nothing {
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp
index 33235b5c4454..1658f210cf3b 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp	
+++ b/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;
 	}

From 5f12c188df15a9a5cf1ac690ff75cf4c39c57730 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 7 Nov 2018 14:19:12 +0000
Subject: [PATCH 3/7] CPP: Change note.

---
 change-notes/1.19/analysis-cpp.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md
index 4835afd0c70d..60e038cf557e 100644
--- a/change-notes/1.19/analysis-cpp.md
+++ b/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. |

From d6f27f0b2d51decaed5219fe5972ebe6daba6a11 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 8 Nov 2018 09:55:39 +0000
Subject: [PATCH 4/7] CPP: Add a test of macros.

---
 .../Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected  | 1 +
 .../Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c      | 7 +++++++
 2 files changed, 8 insertions(+)
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c

diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected
index f50c352fe340..9341c913f822 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
@@ -1,3 +1,4 @@
+| macros.c:6:9:6:13 | param | This expression has no effect. | macros.c:6:9:6:13 | param |  |
 | 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++ |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c
new file mode 100644
index 000000000000..9a8a922e4184
--- /dev/null
+++ b/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 [FALSE POSITIVE]
+}

From 5b09e11a5224a95e9b55085fc8fd02a22352a2c3 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 8 Nov 2018 10:01:07 +0000
Subject: [PATCH 5/7] CPP: Repair macro case.

---
 cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql          | 1 +
 .../Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected       | 1 -
 .../Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c           | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql
index 56715956635f..2db4616a08a1 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql	
@@ -99,6 +99,7 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql
       not peivc.(FunctionCall).getTarget().hasName("operator==") and
       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
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected
index 9341c913f822..f50c352fe340 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected	
@@ -1,4 +1,3 @@
-| macros.c:6:9:6:13 | param | This expression has no effect. | macros.c:6:9:6:13 | param |  |
 | 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++ |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c
index 9a8a922e4184..2f7fcc1e05e2 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c	
@@ -3,5 +3,5 @@
 
 void test2(int param)
 {
-	UNUSED(param); // GOOD [FALSE POSITIVE]
+	UNUSED(param); // GOOD
 }

From 3f0e28aea9139f4f19799f21e83c20e548a39dcf Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 9 Nov 2018 17:12:51 +0000
Subject: [PATCH 6/7] CPP: Fix additional expr_has_no_effect test.

---
 .../queries/expr_has_no_effect/expr_has_no_effect.expected       | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected b/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected
index af2bacade1a2..5982cfa589ef 100644
--- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected
+++ b/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 |  |

From 09782d145eabdf83ed69821b75b4cadc5e3c0da7 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 9 Nov 2018 17:23:32 +0000
Subject: [PATCH 7/7] CPP: Annotate expr_has_no_effect test.

---
 .../queries/expr_has_no_effect/calls.cpp             |  8 ++++----
 .../queries/expr_has_no_effect/templatey.cpp         | 12 ++++++------
 .../queries/expr_has_no_effect/volatile.c            | 10 +++++-----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp b/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp
index d9a71fa7bafa..2acdfcf80f8f 100644
--- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp
+++ b/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp
@@ -5,11 +5,11 @@ int external();
 class Base {
 public:
   virtual int thingy() {
-    1;
+    1; // BAD
   }
 
   int our_thingy() {
-    Base::thingy();
+    Base::thingy(); // BAD
     return 2;
   }
 };
@@ -17,14 +17,14 @@ class Base {
 class Derived : public Base {
 public:
   virtual int thingy() {
-    external();
+    external(); // GOOD
     return 3;
   }
 };
 
 void internal() {
   Base* ptr = new Derived();
-  ptr->thingy();
+  ptr->thingy(); // GOOD
 }
 
 }
\ No newline at end of file
diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp b/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp
index 114e2052a530..7d2b6b19777e 100644
--- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp
+++ b/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
 }
diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c b/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c
index 27ee532c99c2..c34e0818f192 100644
--- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c
+++ b/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);