IdentifierHidden: revert PR#546

change_notes/2024-05-21-identifier-hidden.md

@@ -0,0 +1,2 @@
+- `A2-10-1`, `RULE-5-3` - `IdentifierHiding.ql`, `IdentifierHidingC.ql`:
+  - Revert some changes previously made in PR #546 (addressing issue #118). Revert expansion to function identifiers.

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

@@ -57,18 +57,10 @@ 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() { 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() {
+  UserVariable() {
     exists(getFile().getRelativePath()) and
-    not this.(Variable).isCompilerGenerated() and
-    not this.(Function).isCompilerGenerated() and
+    not 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"
   }
@@ -82,13 +74,11 @@ class Scope extends Element {
 
   int getNumberOfVariables() { result = count(getAVariable()) }
 
-  int getNumberOfDeclarations() { result = count(getADeclaration()) }
-
   Scope getAnAncestor() { result = this.getStrictParent+() }
 
   Scope getStrictParent() { result = getParentScope(this) }
 
-  UserDeclaration getADeclaration() { getParentScope(result) = this }
+  Declaration getADeclaration() { getParentScope(result) = this }
 
   Expr getAnExpr() { this = getParentScope(result) }
 
@@ -132,31 +122,31 @@ class GeneratedBlockStmt extends BlockStmt {
   GeneratedBlockStmt() { this.getLocation() instanceof UnknownLocation }
 }
 
-/** Gets a Declaration that is in the potential scope of Declaration `v`. */
-private UserDeclaration getPotentialScopeOfDeclaration_candidate(UserDeclaration v) {
+/** Gets a variable that is in the potential scope of variable `v`. */
+private UserVariable getPotentialScopeOfVariable_candidate(UserVariable v) {
   exists(Scope s |
-    result = s.getADeclaration() and
+    result = s.getAVariable() and
     (
-      // 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
+      // 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
       or
-      // In the same scope, but not the same Declaration, and choose just one to report
-      v = s.getADeclaration() and
+      // In the same scope, but not the same variable, and choose just one to report
+      v = s.getAVariable() and
       not result = v and
       v.getName() <= result.getName()
     )
   )
 }
 
-/** Gets a Declaration that is in the potential scope of Declaration `v`. */
-private UserDeclaration getPotentialScopeOfDeclarationStrict_candidate(UserDeclaration v) {
+/** Gets a variable that is in the potential scope of variable `v`. */
+private UserVariable getOuterScopesOfVariable_candidate(UserVariable v) {
   exists(Scope s |
-    result = s.getADeclaration() and
+    result = s.getAVariable() and
     (
-      // 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
+      // 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
     )
   )
 }
@@ -171,20 +161,20 @@ predicate inSameTranslationUnit(File f1, File f2) {
 }
 
 /**
- * Gets a user Declaration which occurs in the "outer scope" of Declaration `v`.
+ * Gets a user variable which occurs in the "potential scope" of variable `v`.
  */
 cached
-UserDeclaration getPotentialScopeOfDeclarationStrict(UserDeclaration v) {
-  result = getPotentialScopeOfDeclarationStrict_candidate(v) and
+UserVariable getPotentialScopeOfVariable(UserVariable v) {
+  result = getPotentialScopeOfVariable_candidate(v) and
   inSameTranslationUnit(v.getFile(), result.getFile())
 }
 
 /**
- * Gets a user variable which occurs in the "potential scope" of variable `v`.
+ * Gets a user variable which occurs in the "outer scope" of variable `v`.
  */
 cached
-UserDeclaration getPotentialScopeOfDeclaration(UserDeclaration v) {
-  result = getPotentialScopeOfDeclaration_candidate(v) and
+UserVariable getPotentialScopeOfVariableStrict(UserVariable v) {
+  result = getOuterScopesOfVariable_candidate(v) and
   inSameTranslationUnit(v.getFile(), result.getFile())
 }
 
@@ -214,9 +204,18 @@ class TranslationUnit extends SourceFile {
 }
 
 /** Holds if `v2` may hide `v1`. */
-private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration v2) {
+private predicate hides_candidate(UserVariable v1, UserVariable 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 = getPotentialScopeOfDeclarationStrict(v1) and
+  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()) and
@@ -240,15 +239,6 @@ private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration 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.
  */
@@ -267,22 +257,20 @@ private Stmt getEnclosingStmt(LocalScopeVariable v) {
 }
 
 /** Holds if `v2` hides `v1`. */
-predicate hides(UserDeclaration v1, UserDeclaration v2) {
+predicate hides(UserVariable v1, UserVariable v2) {
   hides_candidate(v1, v2) and
   // Confirm that there's no closer candidate variable which `v2` hides
-  not exists(UserDeclaration mid |
+  not exists(UserVariable 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(UserDeclaration v1, UserDeclaration v2) {
+predicate hidesStrict(UserVariable v1, UserVariable v2) {
   hides_candidateStrict(v1, v2) and
   // Confirm that there's no closer candidate variable which `v2` hides
-  not exists(UserDeclaration mid |
+  not exists(UserVariable mid |
     hides_candidateStrict(v1, mid) and
     hides_candidateStrict(mid, v2)
   )
@@ -303,7 +291,7 @@ predicate hasBlockScope(Declaration decl) { exists(BlockStmt b | b.getADeclarati
 /**
  * identifiers in nested (named/nonglobal) namespaces are exceptions to hiding due to being able access via fully qualified ids
  */
-predicate excludedViaNestedNamespaces(UserDeclaration outerDecl, UserDeclaration innerDecl) {
+predicate excludedViaNestedNamespaces(UserVariable outerDecl, UserVariable innerDecl) {
   exists(Namespace inner, Namespace outer |
     outer.getAChildNamespace+() = inner and
     //outer is not global

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

@@ -76,18 +76,15 @@ predicate hiddenInLambda(UserVariable outerDecl, UserVariable innerDecl) {
 }
 
 query predicate problems(
-  UserDeclaration innerDecl, string message, UserDeclaration outerDecl, string varName
+  UserVariable innerDecl, string message, UserVariable outerDecl, string varName
 ) {
   not isExcluded(outerDecl, getQuery()) and
   not isExcluded(innerDecl, getQuery()) and
   //ignore template variables for this rule
   not outerDecl instanceof TemplateVariable and
   not innerDecl instanceof TemplateVariable and
-  //ignore types for this rule as the Misra C/C++ 23 version of this rule (rule 6.4.1 and 6.4.2) focuses solely on variables and functions
-  not innerDecl instanceof Type and
-  not outerDecl instanceof Type and
   (hidesStrict(outerDecl, innerDecl) or hiddenInLambda(outerDecl, innerDecl)) and
   not excludedViaNestedNamespaces(outerDecl, innerDecl) and
   varName = outerDecl.getName() and
-  message = "Declaration is hiding declaration $@."
+  message = "Variable is hiding variable $@."
 }

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

@@ -1,18 +1,17 @@
-| 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 |
-| test.cpp:114:9:114:17 | globalvar | Declaration is hiding declaration $@. | test.cpp:110:5:110:13 | globalvar | globalvar |
-| test.cpp:133:11:133:11 | b | Declaration is hiding declaration $@. | test.cpp:127:13:127:13 | b | b |
-| test.cpp:142:9:142:10 | a1 | Declaration is hiding declaration $@. | test.cpp:140:14:140:15 | a1 | a1 |
-| test.cpp:147:9:147:10 | a2 | Declaration is hiding declaration $@. | test.cpp:145:20:145:21 | a2 | a2 |
-| test.cpp:152:9:152:10 | a3 | Declaration is hiding declaration $@. | test.cpp:150:17:150:18 | a3 | a3 |
-| test.cpp:164:9:164:10 | a5 | Declaration is hiding declaration $@. | test.cpp:162:13:162:14 | a5 | a5 |
+| 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: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:86:9:86:9 | b | Variable is hiding variable $@. | test.cpp:80:11:80:11 | b | b |
+| test.cpp:94:9:94:17 | globalvar | Variable is hiding variable $@. | test.cpp:91:5:91:13 | globalvar | globalvar |
+| test.cpp:113:11:113:11 | b | Variable is hiding variable $@. | test.cpp:107:13:107:13 | b | b |
+| test.cpp:122:9:122:10 | a1 | Variable is hiding variable $@. | test.cpp:120:14:120:15 | a1 | a1 |
+| test.cpp:127:9:127:10 | a2 | Variable is hiding variable $@. | test.cpp:125:20:125:21 | a2 | a2 |
+| test.cpp:132:9:132:10 | a3 | Variable is hiding variable $@. | test.cpp:130:17:130:18 | a3 | a3 |
+| test.cpp:144:9:144:10 | a5 | Variable is hiding variable $@. | test.cpp:142:13:142:14 | a5 | a5 |

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

@@ -76,22 +76,6 @@ void test_scope_order() {
   }
 }
 
-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]() {
@@ -104,12 +88,8 @@ void f4() {
   };
 }
 
-void f5(int i) {}    // COMPLIANT - exception - assume purposefully overloaded
-void f5(double d) {} // COMPLIANT - exception - assume purposefully overloaded
-
 int globalvar = 0;
-
-int f6() {
+int f5() {
   auto lambda_with_shadowing = []() {
     int globalvar = 1; // NON_COMPLIANT - not an exception - not captured but
                        // still accessible
@@ -121,7 +101,7 @@ int f6() {
   return lambda_with_shadowing();
 }
 
-void f7(int p) {
+void f6(int p) {
   // Introduce a nested scope to test scope comparison.
   if (p != 0) {
     int a1, b;
@@ -136,7 +116,7 @@ void f7(int p) {
   }
 }
 
-void f8() {
+void f7() {
   static int a1;
   auto lambda1 = []() {
     int a1 = 10; // NON_COMPLIANT - Lambda can access static variable.