Merge pull request github#2035 from geoffw0/comparison

CPP: Unclear comparison precedence template fix

Author: jbj

change-notes/1.23/analysis-cpp.md

@@ -20,6 +20,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | More correct results | This query now checks for the beginning date of the Reiwa era (1st May 2019). |
 | Too few arguments to formatting function (`cpp/wrong-number-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
 | Too many arguments to formatting function (`cpp/too-many-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
+| Unclear comparison precedence (`cpp/comparison-precedence`) | Fewer false positive results | False positives involving template classes and functions have been fixed. |
 
 ## Changes to QL libraries
 

cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql

@@ -14,5 +14,8 @@
 import cpp
 
 from ComparisonOperation co, ComparisonOperation chco
-where co.getAChild() = chco and not chco.isParenthesised()
+where
+  co.getAChild() = chco and
+  not chco.isParenthesised() and
+  not co.isFromUninstantiatedTemplate(_)
 select co, "Check the comparison operator precedence."

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/ComparisonPrecedence/ComparisonPrecedence.expected

@@ -0,0 +1,9 @@
+| template.cpp:4:7:4:15 | ... < ... | Check the comparison operator precedence. |
+| test.cpp:42:6:42:14 | ... < ... | Check the comparison operator precedence. |
+| test.cpp:43:6:43:14 | ... > ... | Check the comparison operator precedence. |
+| test.cpp:44:6:44:16 | ... <= ... | Check the comparison operator precedence. |
+| test.cpp:45:6:45:16 | ... <= ... | Check the comparison operator precedence. |
+| test.cpp:46:6:46:14 | ... > ... | Check the comparison operator precedence. |
+| test.cpp:50:6:50:32 | ... < ... | Check the comparison operator precedence. |
+| test.cpp:51:6:51:18 | ... < ... | Check the comparison operator precedence. |
+| test.cpp:54:8:54:16 | ... < ... | Check the comparison operator precedence. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/ComparisonPrecedence/ComparisonPrecedence.qlref

@@ -0,0 +1 @@
+Likely Bugs/Arithmetic/ComparisonPrecedence.ql

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/ComparisonPrecedence/template.cpp

@@ -0,0 +1,32 @@
+
+template <typename T>
+void templateFunc1(T x, T y, T z) {
+  if (x < y < z) {} // BAD (though dubious as we can imagine other instantiations using an overloaded `operator<`)
+  if (x < y && y < z) {} // GOOD
+};
+
+template <typename T>
+void templateFunc2(T x, T y, T z) {
+  if (x < y < z) {} // GOOD (used with an overloaded `operator<`)
+  if (x < y && y < z) {} // GOOD
+};
+
+struct myStruct {
+  operator bool() {
+    return true;
+  }
+
+  myStruct operator<(myStruct &other) {
+    return other; // non-standard `operator<` behaviour
+  }
+};
+
+int main() {
+  int x = 3;
+  myStruct y;
+ 
+  templateFunc1(x, x, x);
+  templateFunc2(y, y, y);
+
+  return 0;
+}

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/ComparisonPrecedence/test.cpp

@@ -0,0 +1,69 @@
+
+/**
+ * MyClass1 contains an `int` and has well behaved `operator<`
+ */
+class MyClass1 {
+public:
+	MyClass1() : v(0) {};
+	MyClass1(int _v) : v(_v) {};
+
+	bool operator<(const MyClass1 &other) {
+		return v < other.v;
+	}
+
+	operator bool() {
+		return true;
+	}
+
+	int v;
+};
+
+/**
+ * MyClass2 contains an `int` but has an unusual `operator<`
+ */
+class MyClass2 {
+public:
+	MyClass2() : v(0) {};
+	MyClass2(int _v) : v(_v) {};
+
+	MyClass2 operator<(const MyClass2 &other) {
+		return MyClass2(other.v);
+	}
+
+	operator bool() {
+		return true;
+	}
+
+	int v;
+};
+
+void test1(int x, int y, int z) {
+	// built-in comparison
+	if (x < y < z) {} // BAD
+	if (x > y > z) {} // BAD
+	if (x <= y <= z) {} // BAD
+	if (x <= y <= z) {} // BAD
+	if (x < y > z) {} // BAD
+	if ((x < y) && (y < z)) {} // GOOD
+	if (x < y && y < z) {} // GOOD
+
+	if ((x + 1) < (y + 1) < (z + 1)) {} // BAD
+	if (x < x + y < z) {} // BAD
+
+	if ((x < y) < z) {} // GOOD (this is deliberately allowed)
+	if (!(x < y < z)) {} // BAD
+
+	// overloaded comparison
+	{
+		MyClass1 a, b, c;
+
+		if (a < b < c) {} // BAD (the overloaded `operator<` behaves like `<`) [NOT DETECTED]
+	}
+
+	// overloaded non-comparison
+	{
+		MyClass2 a, b, c;
+
+		if (a < b < c) {} // GOOD (the overloaded `operator<` does not behave like `<`)
+	}
+}