Merge pull request #546 from knewbury01/knewbury01/fix-118

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

Author: knewbury01

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/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql

@@ -18,6 +18,7 @@ import codingstandards.cpp.autosar
 import codingstandards.cpp.TrivialType
 import codingstandards.cpp.SideEffect
 import semmle.code.cpp.controlflow.SSA
+import codingstandards.cpp.Expr
 
 predicate isZeroInitializable(Variable v) {
   not exists(v.getInitializer().getExpr()) and
@@ -34,78 +35,6 @@ predicate isTypeZeroInitializable(Type t) {
   t.getUnderlyingType() instanceof ArrayType
 }
 
-/**
- * An optimized set of expressions used to determine the flow through constexpr variables.
- */
-class VariableAccessOrCallOrLiteral extends Expr {
-  VariableAccessOrCallOrLiteral() {
-    this instanceof VariableAccess or
-    this instanceof Call or
-    this instanceof Literal
-  }
-}
-
-/**
- * Holds if the value of source flows through compile time evaluated variables to target.
- */
-predicate flowsThroughConstExprVariables(
-  VariableAccessOrCallOrLiteral source, VariableAccessOrCallOrLiteral target
-) {
-  (
-    source = target
-    or
-    source != target and
-    exists(SsaDefinition intermediateDef, StackVariable intermediate |
-      intermediateDef.getAVariable().getFunction() = source.getEnclosingFunction() and
-      intermediateDef.getAVariable().getFunction() = target.getEnclosingFunction() and
-      intermediateDef.getAVariable() = intermediate and
-      intermediate.isConstexpr()
-    |
-      DataFlow::localExprFlow(source, intermediateDef.getDefiningValue(intermediate)) and
-      flowsThroughConstExprVariables(intermediateDef.getAUse(intermediate), target)
-    )
-  )
-}
-
-/*
- * Returns true if the given call may be evaluated at compile time and is compile time evaluated because
- * all its arguments are compile time evaluated and its default values are compile time evaluated.
- */
-
-predicate isCompileTimeEvaluated(Call call) {
-  // 1. The call may be evaluated at compile time, because it is constexpr, and
-  call.getTarget().isConstexpr() and
-  // 2. all its arguments are compile time evaluated, and
-  forall(DataFlow::Node ultimateArgSource, DataFlow::Node argSource |
-    argSource = DataFlow::exprNode(call.getAnArgument()) and
-    DataFlow::localFlow(ultimateArgSource, argSource) and
-    not DataFlow::localFlowStep(_, ultimateArgSource)
-  |
-    (
-      ultimateArgSource.asExpr() instanceof Literal
-      or
-      any(Call c | isCompileTimeEvaluated(c)) = ultimateArgSource.asExpr()
-    ) and
-    // If the ultimate argument source is not the same as the argument source, then it must flow through
-    // constexpr variables.
-    (
-      ultimateArgSource != argSource
-      implies
-      flowsThroughConstExprVariables(ultimateArgSource.asExpr(), argSource.asExpr())
-    )
-  ) and
-  // 3. all the default values used are compile time evaluated.
-  forall(Expr defaultValue, Parameter parameterUsingDefaultValue, int idx |
-    parameterUsingDefaultValue = call.getTarget().getParameter(idx) and
-    not exists(call.getArgument(idx)) and
-    parameterUsingDefaultValue.getAnAssignedValue() = defaultValue
-  |
-    defaultValue instanceof Literal
-    or
-    any(Call c | isCompileTimeEvaluated(c)) = defaultValue
-  )
-}
-
 from Variable v
 where
   not isExcluded(v, ConstPackage::variableMissingConstexprQuery()) and
@@ -119,7 +48,7 @@ where
   (
     v.getInitializer().getExpr().isConstant()
     or
-    any(Call call | isCompileTimeEvaluated(call)) = v.getInitializer().getExpr()
+    any(Call call | isCompileTimeEvaluatedCall(call)) = v.getInitializer().getExpr()
     or
     isZeroInitializable(v)
     or

cpp/common/src/codingstandards/cpp/Expr.qll

@@ -191,8 +191,89 @@ module MisraExpr {
 }
 
 /**
+ * An optimized set of expressions used to determine the flow through constexpr variables.
+ */
+class VariableAccessOrCallOrLiteral extends Expr {
+  VariableAccessOrCallOrLiteral() {
+    this instanceof VariableAccess and this.(VariableAccess).getTarget().isConstexpr()
+    or
+    this instanceof Call
+    or
+    this instanceof Literal
+  }
+}
+
+/**
+ * Holds if the value of source flows through compile time evaluated variables to target.
+ */
+predicate flowsThroughConstExprVariables(
+  VariableAccessOrCallOrLiteral source, VariableAccessOrCallOrLiteral target
+) {
+  (
+    source = target
+    or
+    source != target and
+    exists(SsaDefinition intermediateDef, StackVariable intermediate |
+      intermediateDef.getAVariable().getFunction() = source.getEnclosingFunction() and
+      intermediateDef.getAVariable().getFunction() = target.getEnclosingFunction() and
+      intermediateDef.getAVariable() = intermediate and
+      intermediate.isConstexpr()
+    |
+      DataFlow::localExprFlow(source, intermediateDef.getDefiningValue(intermediate)) and
+      flowsThroughConstExprVariables(intermediateDef.getAUse(intermediate), target)
+    )
+  )
+}
+
+predicate isCompileTimeEvaluatedExpression(Expr expression) {
+  forall(DataFlow::Node ultimateSource, DataFlow::Node source |
+    source = DataFlow::exprNode(expression) and
+    DataFlow::localFlow(ultimateSource, source) and
+    not DataFlow::localFlowStep(_, ultimateSource)
+  |
+    isDirectCompileTimeEvaluatedExpression(ultimateSource.asExpr()) and
+    // If the ultimate source is not the same as the source, then it must flow through
+    // constexpr variables.
+    (
+      ultimateSource != source
+      implies
+      flowsThroughConstExprVariables(ultimateSource.asExpr(), source.asExpr())
+    )
+  )
+}
+
+predicate isDirectCompileTimeEvaluatedExpression(Expr expression) {
+  expression instanceof Literal
+  or
+  any(Call c | isCompileTimeEvaluatedCall(c)) = expression
+}
+
+/*
+ * Returns true if the given call may be evaluated at compile time and is compile time evaluated because
+ * all its arguments are compile time evaluated and its default values are compile time evaluated.
+ */
+
+predicate isCompileTimeEvaluatedCall(Call call) {
+  // 1. The call may be evaluated at compile time, because it is constexpr, and
+  call.getTarget().isConstexpr() and
+  // 2. all its arguments are compile time evaluated, and
+  forall(Expr argSource | argSource = call.getAnArgument() |
+    isCompileTimeEvaluatedExpression(argSource)
+  ) and
+  // 3. all the default values used are compile time evaluated.
+  forall(Expr defaultValue, Parameter parameterUsingDefaultValue, int idx |
+    parameterUsingDefaultValue = call.getTarget().getParameter(idx) and
+    not exists(call.getArgument(idx)) and
+    parameterUsingDefaultValue.getAnAssignedValue() = defaultValue
+  |
+    isDirectCompileTimeEvaluatedExpression(defaultValue)
+  )
+}
+
+/*
  * an operator that does not evaluate its operand
  */
+
 class UnevaluatedExprExtension extends Expr {
   UnevaluatedExprExtension() {
     this.getAChild().isUnevaluated()

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 Declaration that is in the potential scope of Declaration `v`. */
+private UserDeclaration getPotentialScopeOfDeclarationStrict_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 = getPotentialScopeOfDeclarationStrict_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 outerDecl, UserDeclaration innerDecl) {
+  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
+    innerDecl.getNamespace() = inner and
+    outerDecl.getNamespace() = outer
+  )
+}