Merge pull request #423 from github/lcartey/a2-10-1-point-of-decl

Identifier hiding: consider point of declaration

change_notes/2023-11-03-identifier-hiding-improvements.md

@@ -0,0 +1,3 @@
+ - `A2-10-1`, `RULE-5-3`:
+   - Reduce false positives by considering point of declaration for local variables.
+   - Reduce false negatives by considering catch block parameters to be in scope in the catch block.

cpp/common/src/codingstandards/cpp/Scope.qll

@@ -42,7 +42,16 @@ private Element getParentScope(Element e) {
   then result = e.getParentScope()
   else (
     // Statements do no have a parent scope, so return the enclosing block.
-    result = e.(Stmt).getEnclosingBlock() or result = e.(Expr).getEnclosingBlock()
+    result = e.(Stmt).getEnclosingBlock()
+    or
+    result = e.(Expr).getEnclosingBlock()
+    or
+    // Catch block parameters don't have an enclosing scope, so attach them to the
+    // the block itself
+    exists(CatchBlock cb |
+      e = cb.getParameter() and
+      result = cb
+    )
   )
 }
 
@@ -209,7 +218,42 @@ private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
   v2 = getPotentialScopeOfVariableStrict(v1) and
   v1.getName() = v2.getName() and
   // Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
-  not (v1.isMember() or v2.isMember())
+  not (v1.isMember() or v2.isMember()) and
+  (
+    // If v1 is a local variable, ensure that v1 is declared before v2
+    (
+      v1 instanceof LocalVariable and
+      // Ignore variables declared in conditional expressions, as they apply to
+      // the nested scope
+      not v1 = any(ConditionDeclExpr cde).getVariable() and
+      // Ignore variables declared in loops
+      not exists(Loop l | l.getADeclaration() = v1)
+    )
+    implies
+    exists(BlockStmt bs, DeclStmt v1Stmt, Stmt v2Stmt |
+      v1 = v1Stmt.getADeclaration() and
+      getEnclosingStmt(v2).getParentStmt*() = v2Stmt
+    |
+      bs.getIndexOfStmt(v1Stmt) <= bs.getIndexOfStmt(v2Stmt)
+    )
+  )
+}
+
+/**
+ * Gets the enclosing statement of the given variable, if any.
+ */
+private Stmt getEnclosingStmt(LocalScopeVariable v) {
+  result.(DeclStmt).getADeclaration() = v
+  or
+  exists(ConditionDeclExpr cde |
+    cde.getVariable() = v and
+    result = cde.getEnclosingStmt()
+  )
+  or
+  exists(CatchBlock cb |
+    cb.getParameter() = v and
+    result = cb.getEnclosingStmt()
+  )
 }
 
 /** Holds if `v2` hides `v1`. */

cpp/common/test/rules/identifierhidden/IdentifierHidden.expected

@@ -5,3 +5,7 @@
 | test.cpp:23:13:23:15 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
 | test.cpp:26:12:26:14 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
 | test.cpp:27:14:27:16 | id1 | Variable is hiding variable $@. | test.cpp:26:12:26:14 | id1 | id1 |
+| test.cpp:65:11:65:11 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:67:9:67:9 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:70:12:70:12 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:75:16:75:16 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |

cpp/common/test/rules/identifierhidden/test.cpp

@@ -40,4 +40,38 @@ template <class T> constexpr T foo1 = T(1.1L);
 template <class T, class R> T f(T r) {
   T v = foo1<T> * r * r;  // COMPLIANT
   T v1 = foo1<R> * r * r; // COMPLIANT
+}
+
+void test_scope_order() {
+  {
+    {
+      int i; // COMPLIANT
+    }
+    int i; // COMPLIANT
+  }
+
+  for (int i = 0; i < 10; i++) { // COMPLIANT
+  }
+
+  try {
+
+  } catch (int i) { // COMPLIANT
+  }
+
+  int i; // COMPLIANT
+
+  {
+    {
+      int i; // NON_COMPLIANT
+    }
+    int i; // NON_COMPLIANT
+  }
+
+  for (int i = 0; i < 10; i++) { // NON_COMPLIANT
+  }
+
+  try {
+
+  } catch (int i) { // NON_COMPLIANT
+  }
 }