Merge pull request #4202 from geoffw0/localhidesparam

C++: Improve handling of template functions in cpp/declaration-hides-parameter

Author: jbj

change-notes/1.26/analysis-cpp.md

@@ -13,6 +13,7 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Declaration hides parameter (`cpp/declaration-hides-parameter`) | Fewer false positive results | False positives involving template functions have been fixed. |
 | Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | | The precision of this query has been decreased from "high" to "medium". As a result, the query is still run but results are no longer displayed on LGTM by default. |
 | Comparison result is always the same (`cpp/constant-comparison`) | More correct results | Bounds on expressions involving multiplication can now be determined in more cases. |

cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql

@@ -11,20 +11,35 @@
 
 import cpp
 
+/**
+ * Gets the template that a function `f` is constructed from, or just `f` if it
+ * is not from a template instantiation.
+ */
+Function getConstructedFrom(Function f) {
+  f.isConstructedFrom(result)
+  or
+  not f.isConstructedFrom(_) and
+  result = f
+}
+
 /**
  * Gets the parameter of `f` with name `name`, which has to come from the
  * _definition_ of `f` and not a prototype declaration.
  * We also exclude names from functions that have multiple definitions.
  * This should not happen in a single application but since we
  * have a system wide view it is likely to happen for instance for
  * the main function.
+ *
+ * Note: we use `getConstructedFrom` to ensure that we look at template
+ * functions rather than their instantiations. We get better results this way
+ * as the instantiation is artificial and may have inherited parameter names
+ * from the declaration rather than the definition.
  */
 ParameterDeclarationEntry functionParameterNames(Function f, string name) {
   exists(FunctionDeclarationEntry fe |
     result.getFunctionDeclarationEntry() = fe and
-    fe.getFunction() = f and
+    getConstructedFrom(f).getDefinition() = fe and
     fe.getLocation() = f.getDefinitionLocation() and
-    result.getFile() = fe.getFile() and // Work around CPP-331
     strictcount(f.getDefinitionLocation()) = 1 and
     result.getName() = name
   )

cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/DeclarationHidesParameter.expected

@@ -1,2 +1,7 @@
 | hiding.cpp:4:17:4:18 | ii | Local variable 'ii' hides a $@. | hiding.cpp:2:12:2:13 | definition of ii | parameter of the same name |
 | hiding.cpp:15:15:15:16 | kk | Local variable 'kk' hides a $@. | hiding.cpp:12:25:12:26 | definition of kk | parameter of the same name |
+| hiding.cpp:28:7:28:7 | a | Local variable 'a' hides a $@. | hiding.cpp:26:21:26:21 | definition of a | parameter of the same name |
+| hiding.cpp:45:7:45:7 | a | Local variable 'a' hides a $@. | hiding.cpp:43:41:43:41 | definition of a | parameter of the same name |
+| hiding.cpp:64:11:64:11 | i | Local variable 'i' hides a $@. | hiding.cpp:61:20:61:20 | definition of i | parameter of the same name |
+| hiding.cpp:78:7:78:10 | arg1 | Local variable 'arg1' hides a $@. | hiding.cpp:74:28:74:31 | definition of arg1 | parameter of the same name |
+| hiding.cpp:79:5:79:8 | arg2 | Local variable 'arg2' hides a $@. | hiding.cpp:74:36:74:39 | definition of arg2 | parameter of the same name |

cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.cpp

@@ -1,7 +1,7 @@
 
 void f(int ii) {
     if (1) {
-        for(int ii = 1; ii < 10; ii++) {
+        for(int ii = 1; ii < 10; ii++) { // local variable hides parameter of the same name
             ;
         }
     }
@@ -12,7 +12,7 @@ namespace foo {
     void f2(int ii, int kk) {
       try {
         for (ii = 0; ii < 3; ii++) {
-          int kk;
+          int kk; // local variable hides parameter of the same name
         }
       }
       catch (int ee) {
@@ -21,4 +21,61 @@ namespace foo {
   }
 }
 
+void myFunction(int a, int b, int c);
 
+void myFunction(int a, int b, int _c) {
+	{
+		int a = a; // local variable hides parameter of the same name
+		int _b = b;
+		int c = _c;
+
+		// ...
+	}
+}
+
+template<class T>
+class MyTemplateClass {
+public:
+	void myMethod(int a, int b, int c);
+};
+
+template<class T>
+void MyTemplateClass<T> :: myMethod(int a, int b, int _c) {
+	{
+		int a = a; // local variable hides parameter of the same name
+		int _b = b;
+		int c = _c;
+
+		// ...
+	}
+}
+
+MyTemplateClass<int> mtc_i;
+
+void test() {
+	mtc_i.myMethod(0, 0, 0);
+}
+
+#define MYMACRO for (int i = 0; i < 10; i++) {}
+
+void testMacro(int i) {
+	MYMACRO;
+
+	for (int i = 0; i < 10; i++) {}; // local variable hides parameter of the same name
+}
+
+#include "hiding.h"
+
+void myClass::myCaller(void) {
+  this->myMethod(5, 6);
+}
+
+template <typename T>
+void myClass::myMethod(int arg1, T arg2) {
+	{
+		int protoArg1;
+		T protoArg2;
+		int arg1; // local variable hides parameter of the same name
+		T arg2; // local variable hides parameter of the same name
+	}
+}

cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.h

@@ -0,0 +1,7 @@
+
+class myClass {
+public:
+  template <typename T>
+  void myMethod(int protoArg1, T protoArg2);
+  void myCaller(void);
+};