Skip to content

Commit 380417f

Browse files
knewbury01knewbury01
authored
Merge pull request #546 from knewbury01/knewbury01/fix-118
A2-10-1: add functions and types to identifier consideration
2 parents c6fb3af + 80432d8 commit 380417f

File tree

9 files changed

+352
-137
lines changed

9 files changed

+352
-137
lines changed

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

+5-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
| test.c:4:7:4:9 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
2-
| test.c:7:13:7:15 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
3-
| test.c:10:12:10:14 | id1 | Variable is hiding variable $@. | test.c:1:5:1:7 | id1 | id1 |
4-
| test.c:11:14:11:16 | id1 | Variable is hiding variable $@. | test.c:10:12:10:14 | id1 | id1 |
5-
| test.c:24:24:24:26 | id2 | Variable is hiding variable $@. | test.c:22:5:22:7 | id2 | id2 |
1+
| test.c:4:7:4:9 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
2+
| test.c:7:13:7:15 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
3+
| test.c:10:12:10:14 | id1 | Declaration is hiding declaration $@. | test.c:1:5:1:7 | id1 | id1 |
4+
| test.c:11:14:11:16 | id1 | Declaration is hiding declaration $@. | test.c:10:12:10:14 | id1 | id1 |
5+
| 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

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

‎cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql

+2-73
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import codingstandards.cpp.autosar
1818
import codingstandards.cpp.TrivialType
1919
import codingstandards.cpp.SideEffect
2020
import semmle.code.cpp.controlflow.SSA
21+
import codingstandards.cpp.Expr
2122

2223
predicate isZeroInitializable(Variable v) {
2324
not exists(v.getInitializer().getExpr()) and
@@ -34,78 +35,6 @@ predicate isTypeZeroInitializable(Type t) {
3435
t.getUnderlyingType() instanceof ArrayType
3536
}
3637

37-
/**
38-
* An optimized set of expressions used to determine the flow through constexpr variables.
39-
*/
40-
class VariableAccessOrCallOrLiteral extends Expr {
41-
VariableAccessOrCallOrLiteral() {
42-
this instanceof VariableAccess or
43-
this instanceof Call or
44-
this instanceof Literal
45-
}
46-
}
47-
48-
/**
49-
* Holds if the value of source flows through compile time evaluated variables to target.
50-
*/
51-
predicate flowsThroughConstExprVariables(
52-
VariableAccessOrCallOrLiteral source, VariableAccessOrCallOrLiteral target
53-
) {
54-
(
55-
source = target
56-
or
57-
source != target and
58-
exists(SsaDefinition intermediateDef, StackVariable intermediate |
59-
intermediateDef.getAVariable().getFunction() = source.getEnclosingFunction() and
60-
intermediateDef.getAVariable().getFunction() = target.getEnclosingFunction() and
61-
intermediateDef.getAVariable() = intermediate and
62-
intermediate.isConstexpr()
63-
|
64-
DataFlow::localExprFlow(source, intermediateDef.getDefiningValue(intermediate)) and
65-
flowsThroughConstExprVariables(intermediateDef.getAUse(intermediate), target)
66-
)
67-
)
68-
}
69-
70-
/*
71-
* Returns true if the given call may be evaluated at compile time and is compile time evaluated because
72-
* all its arguments are compile time evaluated and its default values are compile time evaluated.
73-
*/
74-
75-
predicate isCompileTimeEvaluated(Call call) {
76-
// 1. The call may be evaluated at compile time, because it is constexpr, and
77-
call.getTarget().isConstexpr() and
78-
// 2. all its arguments are compile time evaluated, and
79-
forall(DataFlow::Node ultimateArgSource, DataFlow::Node argSource |
80-
argSource = DataFlow::exprNode(call.getAnArgument()) and
81-
DataFlow::localFlow(ultimateArgSource, argSource) and
82-
not DataFlow::localFlowStep(_, ultimateArgSource)
83-
|
84-
(
85-
ultimateArgSource.asExpr() instanceof Literal
86-
or
87-
any(Call c | isCompileTimeEvaluated(c)) = ultimateArgSource.asExpr()
88-
) and
89-
// If the ultimate argument source is not the same as the argument source, then it must flow through
90-
// constexpr variables.
91-
(
92-
ultimateArgSource != argSource
93-
implies
94-
flowsThroughConstExprVariables(ultimateArgSource.asExpr(), argSource.asExpr())
95-
)
96-
) and
97-
// 3. all the default values used are compile time evaluated.
98-
forall(Expr defaultValue, Parameter parameterUsingDefaultValue, int idx |
99-
parameterUsingDefaultValue = call.getTarget().getParameter(idx) and
100-
not exists(call.getArgument(idx)) and
101-
parameterUsingDefaultValue.getAnAssignedValue() = defaultValue
102-
|
103-
defaultValue instanceof Literal
104-
or
105-
any(Call c | isCompileTimeEvaluated(c)) = defaultValue
106-
)
107-
}
108-
10938
from Variable v
11039
where
11140
not isExcluded(v, ConstPackage::variableMissingConstexprQuery()) and
@@ -119,7 +48,7 @@ where
11948
(
12049
v.getInitializer().getExpr().isConstant()
12150
or
122-
any(Call call | isCompileTimeEvaluated(call)) = v.getInitializer().getExpr()
51+
any(Call call | isCompileTimeEvaluatedCall(call)) = v.getInitializer().getExpr()
12352
or
12453
isZeroInitializable(v)
12554
or

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

+81
Original file line numberDiff line numberDiff line change
@@ -191,8 +191,89 @@ module MisraExpr {
191191
}
192192

193193
/**
194+
* An optimized set of expressions used to determine the flow through constexpr variables.
195+
*/
196+
class VariableAccessOrCallOrLiteral extends Expr {
197+
VariableAccessOrCallOrLiteral() {
198+
this instanceof VariableAccess and this.(VariableAccess).getTarget().isConstexpr()
199+
or
200+
this instanceof Call
201+
or
202+
this instanceof Literal
203+
}
204+
}
205+
206+
/**
207+
* Holds if the value of source flows through compile time evaluated variables to target.
208+
*/
209+
predicate flowsThroughConstExprVariables(
210+
VariableAccessOrCallOrLiteral source, VariableAccessOrCallOrLiteral target
211+
) {
212+
(
213+
source = target
214+
or
215+
source != target and
216+
exists(SsaDefinition intermediateDef, StackVariable intermediate |
217+
intermediateDef.getAVariable().getFunction() = source.getEnclosingFunction() and
218+
intermediateDef.getAVariable().getFunction() = target.getEnclosingFunction() and
219+
intermediateDef.getAVariable() = intermediate and
220+
intermediate.isConstexpr()
221+
|
222+
DataFlow::localExprFlow(source, intermediateDef.getDefiningValue(intermediate)) and
223+
flowsThroughConstExprVariables(intermediateDef.getAUse(intermediate), target)
224+
)
225+
)
226+
}
227+
228+
predicate isCompileTimeEvaluatedExpression(Expr expression) {
229+
forall(DataFlow::Node ultimateSource, DataFlow::Node source |
230+
source = DataFlow::exprNode(expression) and
231+
DataFlow::localFlow(ultimateSource, source) and
232+
not DataFlow::localFlowStep(_, ultimateSource)
233+
|
234+
isDirectCompileTimeEvaluatedExpression(ultimateSource.asExpr()) and
235+
// If the ultimate source is not the same as the source, then it must flow through
236+
// constexpr variables.
237+
(
238+
ultimateSource != source
239+
implies
240+
flowsThroughConstExprVariables(ultimateSource.asExpr(), source.asExpr())
241+
)
242+
)
243+
}
244+
245+
predicate isDirectCompileTimeEvaluatedExpression(Expr expression) {
246+
expression instanceof Literal
247+
or
248+
any(Call c | isCompileTimeEvaluatedCall(c)) = expression
249+
}
250+
251+
/*
252+
* Returns true if the given call may be evaluated at compile time and is compile time evaluated because
253+
* all its arguments are compile time evaluated and its default values are compile time evaluated.
254+
*/
255+
256+
predicate isCompileTimeEvaluatedCall(Call call) {
257+
// 1. The call may be evaluated at compile time, because it is constexpr, and
258+
call.getTarget().isConstexpr() and
259+
// 2. all its arguments are compile time evaluated, and
260+
forall(Expr argSource | argSource = call.getAnArgument() |
261+
isCompileTimeEvaluatedExpression(argSource)
262+
) and
263+
// 3. all the default values used are compile time evaluated.
264+
forall(Expr defaultValue, Parameter parameterUsingDefaultValue, int idx |
265+
parameterUsingDefaultValue = call.getTarget().getParameter(idx) and
266+
not exists(call.getArgument(idx)) and
267+
parameterUsingDefaultValue.getAnAssignedValue() = defaultValue
268+
|
269+
isDirectCompileTimeEvaluatedExpression(defaultValue)
270+
)
271+
}
272+
273+
/*
194274
* an operator that does not evaluate its operand
195275
*/
276+
196277
class UnevaluatedExprExtension extends Expr {
197278
UnevaluatedExprExtension() {
198279
this.getAChild().isUnevaluated()

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

+66-39
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,18 @@ private Element getParentScope(Element e) {
5757

5858
/** A variable which is defined by the user, rather than being from a third party or compiler generated. */
5959
class UserVariable extends Variable {
60-
UserVariable() {
60+
UserVariable() { this instanceof UserDeclaration }
61+
}
62+
63+
/** A construct which is defined by the user, rather than being from a third party or compiler generated. */
64+
class UserDeclaration extends Declaration {
65+
UserDeclaration() {
6166
exists(getFile().getRelativePath()) and
62-
not isCompilerGenerated() and
67+
not this.(Variable).isCompilerGenerated() and
68+
not this.(Function).isCompilerGenerated() and
6369
not this.(Parameter).getFunction().isCompilerGenerated() and
70+
// 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.
71+
not this instanceof ClassTemplateInstantiation and
6472
// compiler inferred parameters have name of p#0
6573
not this.(Parameter).getName() = "p#0"
6674
}
@@ -74,11 +82,13 @@ class Scope extends Element {
7482

7583
int getNumberOfVariables() { result = count(getAVariable()) }
7684

85+
int getNumberOfDeclarations() { result = count(getADeclaration()) }
86+
7787
Scope getAnAncestor() { result = this.getStrictParent+() }
7888

7989
Scope getStrictParent() { result = getParentScope(this) }
8090

81-
Declaration getADeclaration() { getParentScope(result) = this }
91+
UserDeclaration getADeclaration() { getParentScope(result) = this }
8292

8393
Expr getAnExpr() { this = getParentScope(result) }
8494

@@ -122,31 +132,31 @@ class GeneratedBlockStmt extends BlockStmt {
122132
GeneratedBlockStmt() { this.getLocation() instanceof UnknownLocation }
123133
}
124134

125-
/** Gets a variable that is in the potential scope of variable `v`. */
126-
private UserVariable getPotentialScopeOfVariable_candidate(UserVariable v) {
135+
/** Gets a Declaration that is in the potential scope of Declaration `v`. */
136+
private UserDeclaration getPotentialScopeOfDeclaration_candidate(UserDeclaration v) {
127137
exists(Scope s |
128-
result = s.getAVariable() and
138+
result = s.getADeclaration() and
129139
(
130-
// Variable in an ancestor scope, but only if there are less than 100 variables in this scope
131-
v = s.getAnAncestor().getAVariable() and
132-
s.getNumberOfVariables() < 100
140+
// Declaration in an ancestor scope, but only if there are less than 100 declarations in this scope
141+
v = s.getAnAncestor().getADeclaration() and
142+
s.getNumberOfDeclarations() < 100
133143
or
134-
// In the same scope, but not the same variable, and choose just one to report
135-
v = s.getAVariable() and
144+
// In the same scope, but not the same Declaration, and choose just one to report
145+
v = s.getADeclaration() and
136146
not result = v and
137147
v.getName() <= result.getName()
138148
)
139149
)
140150
}
141151

142-
/** Gets a variable that is in the potential scope of variable `v`. */
143-
private UserVariable getOuterScopesOfVariable_candidate(UserVariable v) {
152+
/** Gets a Declaration that is in the potential scope of Declaration `v`. */
153+
private UserDeclaration getPotentialScopeOfDeclarationStrict_candidate(UserDeclaration v) {
144154
exists(Scope s |
145-
result = s.getAVariable() and
155+
result = s.getADeclaration() and
146156
(
147-
// Variable in an ancestor scope, but only if there are less than 100 variables in this scope
148-
v = s.getAnAncestor().getAVariable() and
149-
s.getNumberOfVariables() < 100
157+
// Declaration in an ancestor scope, but only if there are less than 100 variables in this scope
158+
v = s.getAnAncestor().getADeclaration() and
159+
s.getNumberOfDeclarations() < 100
150160
)
151161
)
152162
}
@@ -161,20 +171,20 @@ predicate inSameTranslationUnit(File f1, File f2) {
161171
}
162172

163173
/**
164-
* Gets a user variable which occurs in the "potential scope" of variable `v`.
174+
* Gets a user Declaration which occurs in the "outer scope" of Declaration `v`.
165175
*/
166176
cached
167-
UserVariable getPotentialScopeOfVariable(UserVariable v) {
168-
result = getPotentialScopeOfVariable_candidate(v) and
177+
UserDeclaration getPotentialScopeOfDeclarationStrict(UserDeclaration v) {
178+
result = getPotentialScopeOfDeclarationStrict_candidate(v) and
169179
inSameTranslationUnit(v.getFile(), result.getFile())
170180
}
171181

172182
/**
173-
* Gets a user variable which occurs in the "outer scope" of variable `v`.
183+
* Gets a user variable which occurs in the "potential scope" of variable `v`.
174184
*/
175185
cached
176-
UserVariable getPotentialScopeOfVariableStrict(UserVariable v) {
177-
result = getOuterScopesOfVariable_candidate(v) and
186+
UserDeclaration getPotentialScopeOfDeclaration(UserDeclaration v) {
187+
result = getPotentialScopeOfDeclaration_candidate(v) and
178188
inSameTranslationUnit(v.getFile(), result.getFile())
179189
}
180190

@@ -204,18 +214,9 @@ class TranslationUnit extends SourceFile {
204214
}
205215

206216
/** Holds if `v2` may hide `v1`. */
207-
private predicate hides_candidate(UserVariable v1, UserVariable v2) {
217+
private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration v2) {
208218
not v1 = v2 and
209-
v2 = getPotentialScopeOfVariable(v1) and
210-
v1.getName() = v2.getName() and
211-
// Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
212-
not (v1.isMember() or v2.isMember())
213-
}
214-
215-
/** Holds if `v2` may hide `v1`. */
216-
private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
217-
not v1 = v2 and
218-
v2 = getPotentialScopeOfVariableStrict(v1) and
219+
v2 = getPotentialScopeOfDeclarationStrict(v1) and
219220
v1.getName() = v2.getName() and
220221
// Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
221222
not (v1.isMember() or v2.isMember()) and
@@ -239,6 +240,15 @@ private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
239240
)
240241
}
241242

243+
/** Holds if `v2` may hide `v1`. */
244+
private predicate hides_candidate(UserDeclaration v1, UserDeclaration v2) {
245+
not v1 = v2 and
246+
v2 = getPotentialScopeOfDeclaration(v1) and
247+
v1.getName() = v2.getName() and
248+
// Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
249+
not (v1.isMember() or v2.isMember())
250+
}
251+
242252
/**
243253
* Gets the enclosing statement of the given variable, if any.
244254
*/
@@ -257,20 +267,22 @@ private Stmt getEnclosingStmt(LocalScopeVariable v) {
257267
}
258268

259269
/** Holds if `v2` hides `v1`. */
260-
predicate hides(UserVariable v1, UserVariable v2) {
270+
predicate hides(UserDeclaration v1, UserDeclaration v2) {
261271
hides_candidate(v1, v2) and
262272
// Confirm that there's no closer candidate variable which `v2` hides
263-
not exists(UserVariable mid |
273+
not exists(UserDeclaration mid |
264274
hides_candidate(v1, mid) and
265275
hides_candidate(mid, v2)
266-
)
276+
) and
277+
// 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.
278+
not v1.(Function).getAnOverload() = v2
267279
}
268280

269281
/** Holds if `v2` strictly (`v2` is in an inner scope compared to `v1`) hides `v1`. */
270-
predicate hidesStrict(UserVariable v1, UserVariable v2) {
282+
predicate hidesStrict(UserDeclaration v1, UserDeclaration v2) {
271283
hides_candidateStrict(v1, v2) and
272284
// Confirm that there's no closer candidate variable which `v2` hides
273-
not exists(UserVariable mid |
285+
not exists(UserDeclaration mid |
274286
hides_candidateStrict(v1, mid) and
275287
hides_candidateStrict(mid, v2)
276288
)
@@ -287,3 +299,18 @@ predicate hasClassScope(Declaration decl) { exists(decl.getDeclaringType()) }
287299

288300
/** Holds if `decl` has block scope. */
289301
predicate hasBlockScope(Declaration decl) { exists(BlockStmt b | b.getADeclaration() = decl) }
302+
303+
/**
304+
* identifiers in nested (named/nonglobal) namespaces are exceptions to hiding due to being able access via fully qualified ids
305+
*/
306+
predicate excludedViaNestedNamespaces(UserDeclaration outerDecl, UserDeclaration innerDecl) {
307+
exists(Namespace inner, Namespace outer |
308+
outer.getAChildNamespace+() = inner and
309+
//outer is not global
310+
not outer instanceof GlobalNamespace and
311+
not outer.isAnonymous() and
312+
not inner.isAnonymous() and
313+
innerDecl.getNamespace() = inner and
314+
outerDecl.getNamespace() = outer
315+
)
316+
}

0 commit comments

Comments
 (0)