From cbaf2fd8e2032411266e17651d623659b367bcfe Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 21 May 2024 16:02:45 -0400 Subject: [PATCH 1/3] IdentifierHidden: revert PR#546 --- change_notes/2024-05-21-identifier-hidden.md | 2 + cpp/common/src/codingstandards/cpp/Scope.qll | 92 ++++++++----------- .../identifierhidden/IdentifierHidden.qll | 7 +- .../IdentifierHidden.expected | 35 ++++--- .../test/rules/identifierhidden/test.cpp | 26 +----- 5 files changed, 64 insertions(+), 98 deletions(-) create mode 100644 change_notes/2024-05-21-identifier-hidden.md diff --git a/change_notes/2024-05-21-identifier-hidden.md b/change_notes/2024-05-21-identifier-hidden.md new file mode 100644 index 0000000000..c09d05bd27 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Scope.qll b/cpp/common/src/codingstandards/cpp/Scope.qll index cfa2d062f2..d9a81b98e3 100644 --- a/cpp/common/src/codingstandards/cpp/Scope.qll +++ b/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 diff --git a/cpp/common/src/codingstandards/cpp/rules/identifierhidden/IdentifierHidden.qll b/cpp/common/src/codingstandards/cpp/rules/identifierhidden/IdentifierHidden.qll index d5d8a0d93e..dc71ba843e 100644 --- a/cpp/common/src/codingstandards/cpp/rules/identifierhidden/IdentifierHidden.qll +++ b/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 $@." } diff --git a/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected b/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected index 1b0d94d838..fd657590ef 100644 --- a/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected +++ b/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 | diff --git a/cpp/common/test/rules/identifierhidden/test.cpp b/cpp/common/test/rules/identifierhidden/test.cpp index ede4bb24d6..427afe15d9 100644 --- a/cpp/common/test/rules/identifierhidden/test.cpp +++ b/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. From 499059b3d961b153505fa77b565c736ce93932f2 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 21 May 2024 16:21:53 -0400 Subject: [PATCH 2/3] IdentifierHidden: fix missing predicate name change --- .../DifferentIdentifiersNotTypographicallyUnambiguous.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/differentidentifiersnottypographicallyunambiguous/DifferentIdentifiersNotTypographicallyUnambiguous.qll b/cpp/common/src/codingstandards/cpp/rules/differentidentifiersnottypographicallyunambiguous/DifferentIdentifiersNotTypographicallyUnambiguous.qll index 4876ca9a5c..87a4580ab3 100644 --- a/cpp/common/src/codingstandards/cpp/rules/differentidentifiersnottypographicallyunambiguous/DifferentIdentifiersNotTypographicallyUnambiguous.qll +++ b/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 = getPotentialScopeOfDeclaration(v1) and + v2 = getPotentialScopeOfVariable(v1) and exists(string s1, string s2 | // over-approximate a match, because it is cheaper to compute getCanon(v1) = getCanon(v2) and From 22381ef437fda74c1ba0d83acfcbd8207e9ba7fa Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 22 May 2024 10:26:45 -0400 Subject: [PATCH 3/3] IdentifierHidden: add missing expected file change --- .../rules/identifierhidden/IdentifierHidden.expected | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/c/common/test/rules/identifierhidden/IdentifierHidden.expected b/c/common/test/rules/identifierhidden/IdentifierHidden.expected index d6f574e318..67809ee003 100644 --- a/c/common/test/rules/identifierhidden/IdentifierHidden.expected +++ b/c/common/test/rules/identifierhidden/IdentifierHidden.expected @@ -1,5 +1,5 @@ -| 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 | +| 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 |