Skip to content

Commit fc282a6

Browse files
knewbury01knewbury01
authored
Merge pull request #593 from knewbury01/knewbury01/revert-fix-118
Revert PR#546, IdentifierHidden performance unblock patch
2 parents c388d2d + 22381ef commit fc282a6

File tree

7 files changed

+70
-104
lines changed

7 files changed

+70
-104
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 | 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 |
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 |

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

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

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

+40-52
Original file line numberDiff line numberDiff line change
@@ -57,18 +57,10 @@ 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() { 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() {
60+
UserVariable() {
6661
exists(getFile().getRelativePath()) and
67-
not this.(Variable).isCompilerGenerated() and
68-
not this.(Function).isCompilerGenerated() and
62+
not isCompilerGenerated() and
6963
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
7264
// compiler inferred parameters have name of p#0
7365
not this.(Parameter).getName() = "p#0"
7466
}
@@ -82,13 +74,11 @@ class Scope extends Element {
8274

8375
int getNumberOfVariables() { result = count(getAVariable()) }
8476

85-
int getNumberOfDeclarations() { result = count(getADeclaration()) }
86-
8777
Scope getAnAncestor() { result = this.getStrictParent+() }
8878

8979
Scope getStrictParent() { result = getParentScope(this) }
9080

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

9383
Expr getAnExpr() { this = getParentScope(result) }
9484

@@ -132,31 +122,31 @@ class GeneratedBlockStmt extends BlockStmt {
132122
GeneratedBlockStmt() { this.getLocation() instanceof UnknownLocation }
133123
}
134124

135-
/** Gets a Declaration that is in the potential scope of Declaration `v`. */
136-
private UserDeclaration getPotentialScopeOfDeclaration_candidate(UserDeclaration v) {
125+
/** Gets a variable that is in the potential scope of variable `v`. */
126+
private UserVariable getPotentialScopeOfVariable_candidate(UserVariable v) {
137127
exists(Scope s |
138-
result = s.getADeclaration() and
128+
result = s.getAVariable() and
139129
(
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
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
143133
or
144-
// In the same scope, but not the same Declaration, and choose just one to report
145-
v = s.getADeclaration() and
134+
// In the same scope, but not the same variable, and choose just one to report
135+
v = s.getAVariable() and
146136
not result = v and
147137
v.getName() <= result.getName()
148138
)
149139
)
150140
}
151141

152-
/** Gets a Declaration that is in the potential scope of Declaration `v`. */
153-
private UserDeclaration getPotentialScopeOfDeclarationStrict_candidate(UserDeclaration v) {
142+
/** Gets a variable that is in the potential scope of variable `v`. */
143+
private UserVariable getOuterScopesOfVariable_candidate(UserVariable v) {
154144
exists(Scope s |
155-
result = s.getADeclaration() and
145+
result = s.getAVariable() and
156146
(
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
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
160150
)
161151
)
162152
}
@@ -171,20 +161,20 @@ predicate inSameTranslationUnit(File f1, File f2) {
171161
}
172162

173163
/**
174-
* Gets a user Declaration which occurs in the "outer scope" of Declaration `v`.
164+
* Gets a user variable which occurs in the "potential scope" of variable `v`.
175165
*/
176166
cached
177-
UserDeclaration getPotentialScopeOfDeclarationStrict(UserDeclaration v) {
178-
result = getPotentialScopeOfDeclarationStrict_candidate(v) and
167+
UserVariable getPotentialScopeOfVariable(UserVariable v) {
168+
result = getPotentialScopeOfVariable_candidate(v) and
179169
inSameTranslationUnit(v.getFile(), result.getFile())
180170
}
181171

182172
/**
183-
* Gets a user variable which occurs in the "potential scope" of variable `v`.
173+
* Gets a user variable which occurs in the "outer scope" of variable `v`.
184174
*/
185175
cached
186-
UserDeclaration getPotentialScopeOfDeclaration(UserDeclaration v) {
187-
result = getPotentialScopeOfDeclaration_candidate(v) and
176+
UserVariable getPotentialScopeOfVariableStrict(UserVariable v) {
177+
result = getOuterScopesOfVariable_candidate(v) and
188178
inSameTranslationUnit(v.getFile(), result.getFile())
189179
}
190180

@@ -214,9 +204,18 @@ class TranslationUnit extends SourceFile {
214204
}
215205

216206
/** Holds if `v2` may hide `v1`. */
217-
private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration v2) {
207+
private predicate hides_candidate(UserVariable v1, UserVariable v2) {
208+
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) {
218217
not v1 = v2 and
219-
v2 = getPotentialScopeOfDeclarationStrict(v1) and
218+
v2 = getPotentialScopeOfVariableStrict(v1) and
220219
v1.getName() = v2.getName() and
221220
// Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
222221
not (v1.isMember() or v2.isMember()) and
@@ -240,15 +239,6 @@ private predicate hides_candidateStrict(UserDeclaration v1, UserDeclaration v2)
240239
)
241240
}
242241

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-
252242
/**
253243
* Gets the enclosing statement of the given variable, if any.
254244
*/
@@ -267,22 +257,20 @@ private Stmt getEnclosingStmt(LocalScopeVariable v) {
267257
}
268258

269259
/** Holds if `v2` hides `v1`. */
270-
predicate hides(UserDeclaration v1, UserDeclaration v2) {
260+
predicate hides(UserVariable v1, UserVariable v2) {
271261
hides_candidate(v1, v2) and
272262
// Confirm that there's no closer candidate variable which `v2` hides
273-
not exists(UserDeclaration mid |
263+
not exists(UserVariable mid |
274264
hides_candidate(v1, mid) and
275265
hides_candidate(mid, v2)
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
266+
)
279267
}
280268

281269
/** Holds if `v2` strictly (`v2` is in an inner scope compared to `v1`) hides `v1`. */
282-
predicate hidesStrict(UserDeclaration v1, UserDeclaration v2) {
270+
predicate hidesStrict(UserVariable v1, UserVariable v2) {
283271
hides_candidateStrict(v1, v2) and
284272
// Confirm that there's no closer candidate variable which `v2` hides
285-
not exists(UserDeclaration mid |
273+
not exists(UserVariable mid |
286274
hides_candidateStrict(v1, mid) and
287275
hides_candidateStrict(mid, v2)
288276
)
@@ -303,7 +291,7 @@ predicate hasBlockScope(Declaration decl) { exists(BlockStmt b | b.getADeclarati
303291
/**
304292
* identifiers in nested (named/nonglobal) namespaces are exceptions to hiding due to being able access via fully qualified ids
305293
*/
306-
predicate excludedViaNestedNamespaces(UserDeclaration outerDecl, UserDeclaration innerDecl) {
294+
predicate excludedViaNestedNamespaces(UserVariable outerDecl, UserVariable innerDecl) {
307295
exists(Namespace inner, Namespace outer |
308296
outer.getAChildNamespace+() = inner and
309297
//outer is not global

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ string step1(string s) {
4747
string step2(string s) { s = "m_" and result = "rn" }
4848

4949
predicate violation(UserVariable v1, UserVariable v2) {
50-
v2 = getPotentialScopeOfDeclaration(v1) and
50+
v2 = getPotentialScopeOfVariable(v1) and
5151
exists(string s1, string s2 |
5252
// over-approximate a match, because it is cheaper to compute
5353
getCanon(v1) = getCanon(v2) and

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

+2-5
Original file line numberDiff line numberDiff line change
@@ -76,18 +76,15 @@ predicate hiddenInLambda(UserVariable outerDecl, UserVariable innerDecl) {
7676
}
7777

7878
query predicate problems(
79-
UserDeclaration innerDecl, string message, UserDeclaration outerDecl, string varName
79+
UserVariable innerDecl, string message, UserVariable outerDecl, string varName
8080
) {
8181
not isExcluded(outerDecl, getQuery()) and
8282
not isExcluded(innerDecl, getQuery()) and
8383
//ignore template variables for this rule
8484
not outerDecl instanceof TemplateVariable and
8585
not innerDecl instanceof TemplateVariable and
86-
//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
87-
not innerDecl instanceof Type and
88-
not outerDecl instanceof Type and
8986
(hidesStrict(outerDecl, innerDecl) or hiddenInLambda(outerDecl, innerDecl)) and
9087
not excludedViaNestedNamespaces(outerDecl, innerDecl) and
9188
varName = outerDecl.getName() and
92-
message = "Declaration is hiding declaration $@."
89+
message = "Variable is hiding variable $@."
9390
}

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

+17-18
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,17 @@
1-
| test.cpp:4:5:4:7 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
2-
| test.cpp:8:5:8:7 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
3-
| test.cpp:20:7:20:9 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
4-
| test.cpp:23:13:23:15 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
5-
| test.cpp:26:12:26:14 | id1 | Declaration is hiding declaration $@. | test.cpp:1:5:1:7 | id1 | id1 |
6-
| test.cpp:27:14:27:16 | id1 | Declaration is hiding declaration $@. | test.cpp:26:12:26:14 | id1 | id1 |
7-
| test.cpp:65:11:65:11 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
8-
| test.cpp:67:9:67:9 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
9-
| test.cpp:70:12:70:12 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
10-
| test.cpp:75:16:75:16 | i | Declaration is hiding declaration $@. | test.cpp:61:7:61:7 | i | i |
11-
| test.cpp:81:5:81:5 | a | Declaration is hiding declaration $@. | test.cpp:79:5:79:5 | a | a |
12-
| test.cpp:102:9:102:9 | b | Declaration is hiding declaration $@. | test.cpp:96:11:96:11 | b | b |
13-
| test.cpp:114:9:114:17 | globalvar | Declaration is hiding declaration $@. | test.cpp:110:5:110:13 | globalvar | globalvar |
14-
| test.cpp:133:11:133:11 | b | Declaration is hiding declaration $@. | test.cpp:127:13:127:13 | b | b |
15-
| test.cpp:142:9:142:10 | a1 | Declaration is hiding declaration $@. | test.cpp:140:14:140:15 | a1 | a1 |
16-
| test.cpp:147:9:147:10 | a2 | Declaration is hiding declaration $@. | test.cpp:145:20:145:21 | a2 | a2 |
17-
| test.cpp:152:9:152:10 | a3 | Declaration is hiding declaration $@. | test.cpp:150:17:150:18 | a3 | a3 |
18-
| test.cpp:164:9:164:10 | a5 | Declaration is hiding declaration $@. | test.cpp:162:13:162:14 | a5 | a5 |
1+
| test.cpp:4:5:4:7 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
2+
| test.cpp:8:5:8:7 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
3+
| test.cpp:20:7:20:9 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
4+
| test.cpp:23:13:23:15 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
5+
| test.cpp:26:12:26:14 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
6+
| test.cpp:27:14:27:16 | id1 | Variable is hiding variable $@. | test.cpp:26:12:26:14 | id1 | id1 |
7+
| test.cpp:65:11:65:11 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
8+
| test.cpp:67:9:67:9 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
9+
| test.cpp:70:12:70:12 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
10+
| test.cpp:75:16:75:16 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
11+
| test.cpp:86:9:86:9 | b | Variable is hiding variable $@. | test.cpp:80:11:80:11 | b | b |
12+
| test.cpp:94:9:94:17 | globalvar | Variable is hiding variable $@. | test.cpp:91:5:91:13 | globalvar | globalvar |
13+
| test.cpp:113:11:113:11 | b | Variable is hiding variable $@. | test.cpp:107:13:107:13 | b | b |
14+
| test.cpp:122:9:122:10 | a1 | Variable is hiding variable $@. | test.cpp:120:14:120:15 | a1 | a1 |
15+
| test.cpp:127:9:127:10 | a2 | Variable is hiding variable $@. | test.cpp:125:20:125:21 | a2 | a2 |
16+
| test.cpp:132:9:132:10 | a3 | Variable is hiding variable $@. | test.cpp:130:17:130:18 | a3 | a3 |
17+
| 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

+3-23
Original file line numberDiff line numberDiff line change
@@ -76,22 +76,6 @@ void test_scope_order() {
7676
}
7777
}
7878

79-
int a;
80-
namespace b {
81-
int a() {} // NON_COMPLIANT
82-
} // namespace b
83-
84-
namespace b1 {
85-
typedef int a; // COMPLIANT - do not consider types
86-
}
87-
88-
namespace ns_exception1_outer {
89-
int a1; // COMPLIANT - exception
90-
namespace ns_exception1_inner {
91-
void a1(); // COMPLIANT - exception
92-
}
93-
} // namespace ns_exception1_outer
94-
9579
void f4() {
9680
int a1, b;
9781
auto lambda1 = [a1]() {
@@ -104,12 +88,8 @@ void f4() {
10488
};
10589
}
10690

107-
void f5(int i) {} // COMPLIANT - exception - assume purposefully overloaded
108-
void f5(double d) {} // COMPLIANT - exception - assume purposefully overloaded
109-
11091
int globalvar = 0;
111-
112-
int f6() {
92+
int f5() {
11393
auto lambda_with_shadowing = []() {
11494
int globalvar = 1; // NON_COMPLIANT - not an exception - not captured but
11595
// still accessible
@@ -121,7 +101,7 @@ int f6() {
121101
return lambda_with_shadowing();
122102
}
123103

124-
void f7(int p) {
104+
void f6(int p) {
125105
// Introduce a nested scope to test scope comparison.
126106
if (p != 0) {
127107
int a1, b;
@@ -136,7 +116,7 @@ void f7(int p) {
136116
}
137117
}
138118

139-
void f8() {
119+
void f7() {
140120
static int a1;
141121
auto lambda1 = []() {
142122
int a1 = 10; // NON_COMPLIANT - Lambda can access static variable.

0 commit comments

Comments
 (0)