diff --git a/change_notes/2023-11-03-identifier-hiding-improvements.md b/change_notes/2023-11-03-identifier-hiding-improvements.md new file mode 100644 index 0000000000..e1e9f8b85f --- /dev/null +++ b/change_notes/2023-11-03-identifier-hiding-improvements.md @@ -0,0 +1,3 @@ + - `A2-10-1`, `RULE-5-3`: + - Reduce false positives by considering point of declaration for local variables. + - Reduce false negatives by considering catch block parameters to be in scope in the catch block. \ 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 1734a1e9e4..4dd727b8d8 100644 --- a/cpp/common/src/codingstandards/cpp/Scope.qll +++ b/cpp/common/src/codingstandards/cpp/Scope.qll @@ -42,7 +42,16 @@ private Element getParentScope(Element e) { then result = e.getParentScope() else ( // Statements do no have a parent scope, so return the enclosing block. - result = e.(Stmt).getEnclosingBlock() or result = e.(Expr).getEnclosingBlock() + result = e.(Stmt).getEnclosingBlock() + or + result = e.(Expr).getEnclosingBlock() + or + // Catch block parameters don't have an enclosing scope, so attach them to the + // the block itself + exists(CatchBlock cb | + e = cb.getParameter() and + result = cb + ) ) } @@ -209,7 +218,42 @@ private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) { 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()) + not (v1.isMember() or v2.isMember()) and + ( + // If v1 is a local variable, ensure that v1 is declared before v2 + ( + v1 instanceof LocalVariable and + // Ignore variables declared in conditional expressions, as they apply to + // the nested scope + not v1 = any(ConditionDeclExpr cde).getVariable() and + // Ignore variables declared in loops + not exists(Loop l | l.getADeclaration() = v1) + ) + implies + exists(BlockStmt bs, DeclStmt v1Stmt, Stmt v2Stmt | + v1 = v1Stmt.getADeclaration() and + getEnclosingStmt(v2).getParentStmt*() = v2Stmt + | + bs.getIndexOfStmt(v1Stmt) <= bs.getIndexOfStmt(v2Stmt) + ) + ) +} + +/** + * Gets the enclosing statement of the given variable, if any. + */ +private Stmt getEnclosingStmt(LocalScopeVariable v) { + result.(DeclStmt).getADeclaration() = v + or + exists(ConditionDeclExpr cde | + cde.getVariable() = v and + result = cde.getEnclosingStmt() + ) + or + exists(CatchBlock cb | + cb.getParameter() = v and + result = cb.getEnclosingStmt() + ) } /** Holds if `v2` hides `v1`. */ diff --git a/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected b/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected index 613dd93f7b..2ea18aa9cd 100644 --- a/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected +++ b/cpp/common/test/rules/identifierhidden/IdentifierHidden.expected @@ -5,3 +5,7 @@ | 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 | diff --git a/cpp/common/test/rules/identifierhidden/test.cpp b/cpp/common/test/rules/identifierhidden/test.cpp index 90f56e7ccf..cdd7137c57 100644 --- a/cpp/common/test/rules/identifierhidden/test.cpp +++ b/cpp/common/test/rules/identifierhidden/test.cpp @@ -40,4 +40,38 @@ template constexpr T foo1 = T(1.1L); template T f(T r) { T v = foo1 * r * r; // COMPLIANT T v1 = foo1 * r * r; // COMPLIANT +} + +void test_scope_order() { + { + { + int i; // COMPLIANT + } + int i; // COMPLIANT + } + + for (int i = 0; i < 10; i++) { // COMPLIANT + } + + try { + + } catch (int i) { // COMPLIANT + } + + int i; // COMPLIANT + + { + { + int i; // NON_COMPLIANT + } + int i; // NON_COMPLIANT + } + + for (int i = 0; i < 10; i++) { // NON_COMPLIANT + } + + try { + + } catch (int i) { // NON_COMPLIANT + } } \ No newline at end of file