A2-10-1: add functions and types to identifier consideration

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

@@ -1,5 +1,5 @@
-| test.c:4:7:4:9 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
-| test.c:7:13:7:15 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
-| test.c:10:12:10:14 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
-| test.c:11:14:11:16 | id1 | Variable is hiding variable $@. | test.c:10:12:10:14 | id1 | id1 |
-| test.c:24:24:24:26 | id2 | Variable is hiding variable $@. | test.c:22:5:22:7 | id2 | id2 |
+| test.c:4:7:4:9 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
+| test.c:7:13:7:15 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
+| test.c:10:12:10:14 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
+| test.c:11:14:11:16 | id1 | Declaration is hiding declaration $@. | test.c:10:12:10:14 | id1 | id1 |
+| test.c:24:24:24:26 | id2 | Declaration is hiding declaration $@. | test.c:22:5:22:7 | id2 | id2 |

change_notes/2024-02-27-identifier-hidden.md

@@ -0,0 +1,3 @@
+- `A2-10-1`, `RULE-5-3` - `IdentifierHiding.ql`, `IdentifierHidingC.ql`:
+  - Address FN reported in #118. Rule was missing detection of functions. Additionally omitted class template instantiations.
+  - Fix FP for identifiers in nested namespaces.

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

@@ -57,10 +57,18 @@ private Element getParentScope(Element e) {
 
 /** A variable which is defined by the user, rather than being from a third party or compiler generated. */
 class UserVariable extends Variable {
-  UserVariable() {
+  UserVariable() { this instanceof UserDeclaration }
+}
+
+/** A construct which is defined by the user, rather than being from a third party or compiler generated. */
+class UserDeclaration extends Declaration {
+  UserDeclaration() {
     exists(getFile().getRelativePath()) and
-    not isCompilerGenerated() and
+    not this.(Variable).isCompilerGenerated() and
+    not this.(Function).isCompilerGenerated() and
     not this.(Parameter).getFunction().isCompilerGenerated() and
+    // Class template instantiations are compiler generated instances that share the same parent scope. This will result in a cross-product on class template instantiations because they have the same name and same parent scope. We therefore exclude these from consideration like we do with other compiler generated identifiers of interest.
+    not this instanceof ClassTemplateInstantiation and
     // compiler inferred parameters have name of p#0
     not this.(Parameter).getName() = "p#0"
   }
@@ -74,11 +82,13 @@ class Scope extends Element {
 
   int getNumberOfVariables() { result = count(getAVariable()) }
 
+  int getNumberOfDeclarations() { result = count(getADeclaration()) }
+
   Scope getAnAncestor() { result = this.getStrictParent+() }
 
   Scope getStrictParent() { result = getParentScope(this) }
 
-  Declaration getADeclaration() { getParentScope(result) = this }
+  UserDeclaration getADeclaration() { getParentScope(result) = this }
 
   Expr getAnExpr() { this = getParentScope(result) }
 
@@ -122,31 +132,31 @@ class GeneratedBlockStmt extends BlockStmt {
   GeneratedBlockStmt() { this.getLocation() instanceof UnknownLocation }
 }
 
-/** Gets a variable that is in the potential scope of variable `v`. */
-private UserVariable getPotentialScopeOfVariable_candidate(UserVariable v) {
+/** Gets a Declaration that is in the potential scope of Declaration `v`. */
+private UserDeclaration getPotentialScopeOfDeclaration_candidate(UserDeclaration v) {
   exists(Scope s |
-    result = s.getAVariable() and
+    result = s.getADeclaration() and
     (
-      // Variable in an ancestor scope, but only if there are less than 100 variables in this scope
-      v = s.getAnAncestor().getAVariable() and
-      s.getNumberOfVariables() < 100
+      // Declaration in an ancestor scope, but only if there are less than 100 declarations in this scope
+      v = s.getAnAncestor().getADeclaration() and
+      s.getNumberOfDeclarations() < 100
       or
-      // In the same scope, but not the same variable, and choose just one to report
-      v = s.getAVariable() and
+      // In the same scope, but not the same Declaration, and choose just one to report
+      v = s.getADeclaration() and
       not result = v and
       v.getName() <= result.getName()
     )
   )
 }
 
-/** Gets a variable that is in the potential scope of variable `v`. */
-private UserVariable getOuterScopesOfVariable_candidate(UserVariable v) {
+/** Gets a Declarationthat is in the potential scope of Declaration `v`. */
+private UserDeclaration getOuterScopesOfDeclaration_candidate(UserDeclaration v) {
   exists(Scope s |
-    result = s.getAVariable() and
+    result = s.getADeclaration() and
     (
-      // Variable in an ancestor scope, but only if there are less than 100 variables in this scope
-      v = s.getAnAncestor().getAVariable() and
-      s.getNumberOfVariables() < 100
+      // Declaration in an ancestor scope, but only if there are less than 100 variables in this scope
+      v = s.getAnAncestor().getADeclaration() and
+      s.getNumberOfDeclarations() < 100
     )
   )
 }
@@ -161,20 +171,20 @@ predicate inSameTranslationUnit(File f1, File f2) {
 }
 
 /**
- * Gets a user variable which occurs in the "potential scope" of variable `v`.
+ * Gets a user Declaration which occurs in the "outer scope" of Declaration `v`.
  */
 cached
-UserVariable getPotentialScopeOfVariable(UserVariable v) {
-  result = getPotentialScopeOfVariable_candidate(v) and
+UserDeclaration getPotentialScopeOfDeclarationStrict(UserDeclaration v) {
+  result = getOuterScopesOfDeclaration_candidate(v) and
   inSameTranslationUnit(v.getFile(), result.getFile())
 }
 
 /**
- * Gets a user variable which occurs in the "outer scope" of variable `v`.
+ * Gets a user variable which occurs in the "potential scope" of variable `v`.
  */
 cached
-UserVariable getPotentialScopeOfVariableStrict(UserVariable v) {
-  result = getOuterScopesOfVariable_candidate(v) and
+UserDeclaration getPotentialScopeOfDeclaration(UserDeclaration v) {
+  result = getPotentialScopeOfDeclaration_candidate(v) and
   inSameTranslationUnit(v.getFile(), result.getFile())
 }
 
@@ -204,18 +214,9 @@ class TranslationUnit extends SourceFile {
 }
 
 /** Holds if `v2` may hide `v1`. */
-private predicate hides_candidate(UserVariable v1, UserVariable v2) {
+private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration v2) {
   not v1 = v2 and
-  v2 = getPotentialScopeOfVariable(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())
-}
-
-/** Holds if `v2` may hide `v1`. */
-private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
-  not v1 = v2 and
-  v2 = getPotentialScopeOfVariableStrict(v1) and
+  v2 = getPotentialScopeOfDeclarationStrict(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()) and
@@ -239,6 +240,15 @@ private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
   )
 }
 
+/** Holds if `v2` may hide `v1`. */
+private predicate hides_candidate(UserDeclaration v1, UserDeclaration v2) {
+  not v1 = v2 and
+  v2 = getPotentialScopeOfDeclaration(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())
+}
+
 /**
  * Gets the enclosing statement of the given variable, if any.
  */
@@ -257,20 +267,22 @@ private Stmt getEnclosingStmt(LocalScopeVariable v) {
 }
 
 /** Holds if `v2` hides `v1`. */
-predicate hides(UserVariable v1, UserVariable v2) {
+predicate hides(UserDeclaration v1, UserDeclaration v2) {
   hides_candidate(v1, v2) and
   // Confirm that there's no closer candidate variable which `v2` hides
-  not exists(UserVariable mid |
+  not exists(UserDeclaration mid |
     hides_candidate(v1, mid) and
     hides_candidate(mid, v2)
-  )
+  ) and
+  // Unlike `hidesStrict`, that requires a different scope, `hides` considers declarations in the same scope. This will include function overloads based on their name. To remove overloads from consideration, we exclude them.
+  not v1.(Function).getAnOverload() = v2
 }
 
 /** Holds if `v2` strictly (`v2` is in an inner scope compared to `v1`) hides `v1`. */
-predicate hidesStrict(UserVariable v1, UserVariable v2) {
+predicate hidesStrict(UserDeclaration v1, UserDeclaration v2) {
   hides_candidateStrict(v1, v2) and
   // Confirm that there's no closer candidate variable which `v2` hides
-  not exists(UserVariable mid |
+  not exists(UserDeclaration mid |
     hides_candidateStrict(v1, mid) and
     hides_candidateStrict(mid, v2)
   )
@@ -287,3 +299,18 @@ predicate hasClassScope(Declaration decl) { exists(decl.getDeclaringType()) }
 
 /** Holds if `decl` has block scope. */
 predicate hasBlockScope(Declaration decl) { exists(BlockStmt b | b.getADeclaration() = decl) }
+
+/**
+ * identifiers in nested (named/nonglobal) namespaces are exceptions to hiding due to being able access via fully qualified ids
+ */
+predicate excludedViaNestedNamespaces(UserDeclaration v2, UserDeclaration v1) {
+  exists(Namespace inner, Namespace outer |
+    outer.getAChildNamespace+() = inner and
+    //outer is not global
+    not outer instanceof GlobalNamespace and
+    not outer.isAnonymous() and
+    not inner.isAnonymous() and
+    v2.getNamespace() = inner and
+    v1.getNamespace() = outer
+  )
+}

cpp/common/src/codingstandards/cpp/rules/differentidentifiersnottypographicallyunambiguous/DifferentIdentifiersNotTypographicallyUnambiguous.qll

@@ -47,7 +47,7 @@ string step1(string s) {
 string step2(string s) { s = "m_" and result = "rn" }
 
 predicate violation(UserVariable v1, UserVariable v2) {
-  v2 = getPotentialScopeOfVariable(v1) and
+  v2 = getPotentialScopeOfDeclaration(v1) and
   exists(string s1, string s2 |
     // over-approximate a match, because it is cheaper to compute
     getCanon(v1) = getCanon(v2) and

cpp/common/src/codingstandards/cpp/rules/identifierhidden/IdentifierHidden.qll

@@ -11,13 +11,36 @@ abstract class IdentifierHiddenSharedQuery extends Query { }
 
 Query getQuery() { result instanceof IdentifierHiddenSharedQuery }
 
-query predicate problems(UserVariable v2, string message, UserVariable v1, string varName) {
+/**
+ * There is a lambda that contains a declaration
+ * that hides something that is captured
+ * and the lambda exists in the function where this lamda is enclosed
+ */
+predicate hiddenInLambda(UserDeclaration v2, UserDeclaration v1) {
+  exists(Scope s, Closure le |
+    s.getADeclaration() = v2 and
+    s.getAnAncestor() = le and
+    le.getEnclosingFunction().getBasicBlock().(Scope) = v1.getParentScope() and
+    exists(LambdaCapture cap, Variable v |
+      v.getAnAccess() = cap.getInitializer().(VariableAccess) and
+      v = v1 and
+      le.getLambdaExpression().getACapture() = cap
+    ) and
+    v2.getName() = v1.getName()
+  )
+}
+
+query predicate problems(UserDeclaration v2, string message, UserDeclaration v1, string varName) {
   not isExcluded(v1, getQuery()) and
   not isExcluded(v2, getQuery()) and
   //ignore template variables for this rule
   not v1 instanceof TemplateVariable and
   not v2 instanceof TemplateVariable and
-  hidesStrict(v1, v2) and
+  //ignore types for this rule
+  not v2 instanceof Type and
+  not v1 instanceof Type and
+  (hidesStrict(v1, v2) or hiddenInLambda(v2, v1)) and
+  not excludedViaNestedNamespaces(v2, v1) and
   varName = v1.getName() and
-  message = "Variable is hiding variable $@."
+  message = "Declaration is hiding declaration $@."
 }

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

@@ -1,11 +1,12 @@
-| test.cpp:4:5:4:7 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
-| test.cpp:8:5:8:7 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
-| test.cpp:11:5:11:7 | id1 | Variable is hiding variable $@. | test.cpp:8:5:8:7 | id1 | id1 |
-| test.cpp:20:7:20:9 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
-| 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 |
+| test.cpp:4:5:4:7 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
+| test.cpp:8:5:8:7 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
+| test.cpp:20:7:20:9 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
+| test.cpp:23:13:23:15 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
+| test.cpp:26:12:26:14 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
+| test.cpp:27:14:27:16 | id1 | Declaration is hiding declaration $@. | test.cpp:26:12:26:14 | id1 | id1 |
+| test.cpp:65:11:65:11 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:67:9:67:9 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:70:12:70:12 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:75:16:75:16 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:81:5:81:5 | a | Declaration is hiding declaration $@. | test.cpp:79:5:79:5 | a | a |
+| test.cpp:102:9:102:9 | b | Declaration is hiding declaration $@. | test.cpp:96:11:96:11 | b | b |

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

@@ -74,4 +74,35 @@ void test_scope_order() {
 
   } catch (int i) { // NON_COMPLIANT
   }
-}
+}
+
+int a;
+namespace b {
+int a() {} // NON_COMPLIANT
+} // namespace b
+
+namespace b1 {
+typedef int a; // COMPLIANT - do not consider types
+}
+
+namespace ns_exception1_outer {
+int a1; // COMPLIANT - exception
+namespace ns_exception1_inner {
+void a1(); // COMPLIANT - exception
+}
+} // namespace ns_exception1_outer
+
+void f4() {
+  int a1, b;
+  auto lambda1 = [a1]() {
+    int b = 10; // COMPLIANT - exception - non captured variable b
+  };
+
+  auto lambda2 = [b]() {
+    int b = 10; // NON_COMPLIANT - not an exception - captured
+                // variable b
+  };
+}
+
+void f5(int i) {}    // COMPLIANT - exception - assume purposefully overloaded
+void f5(double d) {} // COMPLIANT - exception - assume purposefully overloaded