From c8f559c1af50db1419c068dfbcac3cf587639449 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 29 Jul 2025 19:33:14 -0400 Subject: [PATCH 01/47] Generate query files for "Statements" package --- .../cpp/exclusions/cpp/RuleMetadata.qll | 3 + .../cpp/exclusions/cpp/Statements.qll | 61 +++++++++++++++ .../AppropriateStructureOfSwitchStatement.ql | 25 ++++++ .../LegacyForStatementsShouldBeSimple.ql | 24 ++++++ ...orRangeInitializerAtMostOneFunctionCall.ql | 25 ++++++ ...opriateStructureOfSwitchStatement.expected | 1 + ...ppropriateStructureOfSwitchStatement.qlref | 1 + ...LegacyForStatementsShouldBeSimple.expected | 1 + .../LegacyForStatementsShouldBeSimple.qlref | 1 + ...eInitializerAtMostOneFunctionCall.expected | 1 + ...angeInitializerAtMostOneFunctionCall.qlref | 1 + rule_packages/cpp/Statements.json | 78 +++++++++++++++++++ 12 files changed, 222 insertions(+) create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/cpp/Statements.qll create mode 100644 cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql create mode 100644 cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql create mode 100644 cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql create mode 100644 cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected create mode 100644 cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.qlref create mode 100644 cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected create mode 100644 cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.qlref create mode 100644 cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected create mode 100644 cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.qlref create mode 100644 rule_packages/cpp/Statements.json diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll index abd6aeff96..0db51d4224 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll @@ -47,6 +47,7 @@ import SideEffects1 import SideEffects2 import SmartPointers1 import SmartPointers2 +import Statements import Strings import Templates import Toolchain @@ -102,6 +103,7 @@ newtype TCPPQuery = TSideEffects2PackageQuery(SideEffects2Query q) or TSmartPointers1PackageQuery(SmartPointers1Query q) or TSmartPointers2PackageQuery(SmartPointers2Query q) or + TStatementsPackageQuery(StatementsQuery q) or TStringsPackageQuery(StringsQuery q) or TTemplatesPackageQuery(TemplatesQuery q) or TToolchainPackageQuery(ToolchainQuery q) or @@ -157,6 +159,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isSideEffects2QueryMetadata(query, queryId, ruleId, category) or isSmartPointers1QueryMetadata(query, queryId, ruleId, category) or isSmartPointers2QueryMetadata(query, queryId, ruleId, category) or + isStatementsQueryMetadata(query, queryId, ruleId, category) or isStringsQueryMetadata(query, queryId, ruleId, category) or isTemplatesQueryMetadata(query, queryId, ruleId, category) or isToolchainQueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/Statements.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Statements.qll new file mode 100644 index 0000000000..fe202ce31f --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Statements.qll @@ -0,0 +1,61 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype StatementsQuery = + TAppropriateStructureOfSwitchStatementQuery() or + TLegacyForStatementsShouldBeSimpleQuery() or + TForRangeInitializerAtMostOneFunctionCallQuery() + +predicate isStatementsQueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `appropriateStructureOfSwitchStatement` query + StatementsPackage::appropriateStructureOfSwitchStatementQuery() and + queryId = + // `@id` for the `appropriateStructureOfSwitchStatement` query + "cpp/misra/appropriate-structure-of-switch-statement" and + ruleId = "RULE-9-4-2" and + category = "required" + or + query = + // `Query` instance for the `legacyForStatementsShouldBeSimple` query + StatementsPackage::legacyForStatementsShouldBeSimpleQuery() and + queryId = + // `@id` for the `legacyForStatementsShouldBeSimple` query + "cpp/misra/legacy-for-statements-should-be-simple" and + ruleId = "RULE-9-5-1" and + category = "advisory" + or + query = + // `Query` instance for the `forRangeInitializerAtMostOneFunctionCall` query + StatementsPackage::forRangeInitializerAtMostOneFunctionCallQuery() and + queryId = + // `@id` for the `forRangeInitializerAtMostOneFunctionCall` query + "cpp/misra/for-range-initializer-at-most-one-function-call" and + ruleId = "RULE-9-5-2" and + category = "required" +} + +module StatementsPackage { + Query appropriateStructureOfSwitchStatementQuery() { + //autogenerate `Query` type + result = + // `Query` type for `appropriateStructureOfSwitchStatement` query + TQueryCPP(TStatementsPackageQuery(TAppropriateStructureOfSwitchStatementQuery())) + } + + Query legacyForStatementsShouldBeSimpleQuery() { + //autogenerate `Query` type + result = + // `Query` type for `legacyForStatementsShouldBeSimple` query + TQueryCPP(TStatementsPackageQuery(TLegacyForStatementsShouldBeSimpleQuery())) + } + + Query forRangeInitializerAtMostOneFunctionCallQuery() { + //autogenerate `Query` type + result = + // `Query` type for `forRangeInitializerAtMostOneFunctionCall` query + TQueryCPP(TStatementsPackageQuery(TForRangeInitializerAtMostOneFunctionCallQuery())) + } +} diff --git a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql new file mode 100644 index 0000000000..8bfa68f5f2 --- /dev/null +++ b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql @@ -0,0 +1,25 @@ +/** + * @id cpp/misra/appropriate-structure-of-switch-statement + * @name RULE-9-4-2: The structure of a switch statement shall be appropriate + * @description A switch statement should have an appropriate structure with proper cases, default + * labels, and break statements to ensure clear control flow and prevent unintended + * fall-through behavior. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-9-4-2 + * correctness + * maintainability + * readability + * external/misra/allocated-target/single-translation-unit + * external/misra/enforcement/decidable + * external/misra/obligation/required + */ + +import cpp +import codingstandards.cpp.misra + +from +where + not isExcluded(x, StatementsPackage::appropriateStructureOfSwitchStatementQuery()) and +select diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql new file mode 100644 index 0000000000..1a29e90a40 --- /dev/null +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -0,0 +1,24 @@ +/** + * @id cpp/misra/legacy-for-statements-should-be-simple + * @name RULE-9-5-1: Legacy for statements should be simple + * @description Legacy for statements with complex initialization, condition, and increment + * expressions can be difficult to understand and maintain. Simple for loops are more + * readable. + * @kind problem + * @precision high + * @problem.severity recommendation + * @tags external/misra/id/rule-9-5-1 + * maintainability + * readability + * external/misra/allocated-target/single-translation-unit + * external/misra/enforcement/decidable + * external/misra/obligation/advisory + */ + +import cpp +import codingstandards.cpp.misra + +from +where + not isExcluded(x, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and +select diff --git a/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql new file mode 100644 index 0000000000..47d27287bc --- /dev/null +++ b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql @@ -0,0 +1,25 @@ +/** + * @id cpp/misra/for-range-initializer-at-most-one-function-call + * @name RULE-9-5-2: A for-range-initializer shall contain at most one function call + * @description Multiple function calls in a for-range-initializer can lead to unclear iteration + * behavior and potential side effects that make the code harder to understand and + * debug. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-9-5-2 + * correctness + * maintainability + * readability + * external/misra/allocated-target/single-translation-unit + * external/misra/enforcement/decidable + * external/misra/obligation/required + */ + +import cpp +import codingstandards.cpp.misra + +from +where + not isExcluded(x, StatementsPackage::forRangeInitializerAtMostOneFunctionCallQuery()) and +select diff --git a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.qlref b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.qlref new file mode 100644 index 0000000000..9f475afbe1 --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.qlref @@ -0,0 +1 @@ +rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.qlref b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.qlref new file mode 100644 index 0000000000..bf443d6d68 --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.qlref @@ -0,0 +1 @@ +rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.qlref b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.qlref new file mode 100644 index 0000000000..be7fc0ef10 --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.qlref @@ -0,0 +1 @@ +rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql \ No newline at end of file diff --git a/rule_packages/cpp/Statements.json b/rule_packages/cpp/Statements.json new file mode 100644 index 0000000000..91a643253d --- /dev/null +++ b/rule_packages/cpp/Statements.json @@ -0,0 +1,78 @@ +{ + "MISRA-C++-2023": { + "RULE-9-4-2": { + "properties": { + "allocated-target": [ + "Single Translation Unit" + ], + "enforcement": "decidable", + "obligation": "required" + }, + "queries": [ + { + "description": "A switch statement should have an appropriate structure with proper cases, default labels, and break statements to ensure clear control flow and prevent unintended fall-through behavior.", + "kind": "problem", + "name": "The structure of a switch statement shall be appropriate", + "precision": "very-high", + "severity": "error", + "short_name": "AppropriateStructureOfSwitchStatement", + "tags": [ + "correctness", + "maintainability", + "readability" + ] + } + ], + "title": "The structure of a switch statement shall be appropriate" + }, + "RULE-9-5-1": { + "properties": { + "allocated-target": [ + "Single Translation Unit" + ], + "enforcement": "decidable", + "obligation": "advisory" + }, + "queries": [ + { + "description": "Legacy for statements with complex initialization, condition, and increment expressions can be difficult to understand and maintain. Simple for loops are more readable.", + "kind": "problem", + "name": "Legacy for statements should be simple", + "precision": "high", + "severity": "recommendation", + "short_name": "LegacyForStatementsShouldBeSimple", + "tags": [ + "maintainability", + "readability" + ] + } + ], + "title": "Legacy for statements should be simple" + }, + "RULE-9-5-2": { + "properties": { + "allocated-target": [ + "Single Translation Unit" + ], + "enforcement": "decidable", + "obligation": "required" + }, + "queries": [ + { + "description": "Multiple function calls in a for-range-initializer can lead to unclear iteration behavior and potential side effects that make the code harder to understand and debug.", + "kind": "problem", + "name": "A for-range-initializer shall contain at most one function call", + "precision": "very-high", + "severity": "error", + "short_name": "ForRangeInitializerAtMostOneFunctionCall", + "tags": [ + "correctness", + "maintainability", + "readability" + ] + } + ], + "title": "A for-range-initializer shall contain at most one function call" + } + } +} From ffcb4323eb4ad9d95f1ed6332a06457afb92ed54 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 30 Jul 2025 17:13:22 -0400 Subject: [PATCH 02/47] Add agent-generated first draft --- .../AppropriateStructureOfSwitchStatement.ql | 52 ++- ...opriateStructureOfSwitchStatement.expected | 16 +- cpp/misra/test/rules/RULE-9-4-2/test.cpp | 304 ++++++++++++++++++ 3 files changed, 368 insertions(+), 4 deletions(-) create mode 100644 cpp/misra/test/rules/RULE-9-4-2/test.cpp diff --git a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql index 8bfa68f5f2..3d68e31abb 100644 --- a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql +++ b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql @@ -18,8 +18,54 @@ import cpp import codingstandards.cpp.misra +import codingstandards.cpp.SwitchStatement -from +from SwitchStmt switch, string message where - not isExcluded(x, StatementsPackage::appropriateStructureOfSwitchStatementQuery()) and -select + not isExcluded(switch, StatementsPackage::appropriateStructureOfSwitchStatementQuery()) and + ( + // RULE-16-1: Switch not well-formed (has inappropriate statements) + exists(SwitchCase case | + case = switch.getASwitchCase() and + switchCaseNotWellFormed(case) and + message = "has a case that contains inappropriate statements (only expression, compound, selection, iteration or try statements are allowed)" + ) + or + // RULE-16-2: Nested switch labels + exists(SwitchCase case | + case = switch.getASwitchCase() and + case instanceof NestedSwitchCase and + message = "contains a switch label that is not directly within the switch body" + ) + or + // RULE-16-3: Non-empty case doesn't terminate with break + exists(SwitchCase case | + case = switch.getASwitchCase() and + case instanceof CaseDoesNotTerminate and + message = "has a non-empty case that does not terminate with an unconditional break or throw statement" + ) + or + // RULE-16-4: Missing default clause + not switch.hasDefaultCase() and + message = "is missing a default clause" + or + // RULE-16-5: Default clause not first or last + exists(SwitchCase defaultCase | + switch.getDefaultCase() = defaultCase and + exists(defaultCase.getPreviousSwitchCase()) and + finalClauseInSwitchNotDefault(switch) and + message = "has a default clause that is not the first or last switch label" + ) + or + // RULE-16-6: Less than two case clauses + count(SwitchCase case | + switch.getASwitchCase() = case and + case.getNextSwitchCase() != case.getFollowingStmt() + ) + 1 < 2 and + message = "has fewer than two switch-clauses" + or + // RULE-16-7: Boolean switch expression + switch instanceof BooleanSwitchStmt and + message = "has a controlling expression of essentially Boolean type" + ) +select switch, "Switch statement " + message + "." diff --git a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected index 2ec1a0ac6c..8900facc63 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected +++ b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected @@ -1 +1,15 @@ -No expected results have yet been specified \ No newline at end of file +| test.cpp:40:3:40:8 | Switch statement has a case that contains inappropriate statements (only expression, compound, selection, iteration or try statements are allowed). | +| test.cpp:86:3:86:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | +| test.cpp:86:3:86:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | +| test.cpp:98:3:98:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | +| test.cpp:123:3:123:8 | Switch statement is missing a default clause. | +| test.cpp:166:3:166:8 | Switch statement has a default clause that is not the first or last switch label. | +| test.cpp:203:3:203:8 | Switch statement has fewer than two switch-clauses. | +| test.cpp:210:3:210:8 | Switch statement has fewer than two switch-clauses. | +| test.cpp:235:3:235:8 | Switch statement has a controlling expression of essentially Boolean type. | +| test.cpp:245:3:245:8 | Switch statement has a controlling expression of essentially Boolean type. | +| test.cpp:266:3:266:8 | Switch statement has a controlling expression of essentially Boolean type. | +| test.cpp:266:3:266:8 | Switch statement is missing a default clause. | +| test.cpp:266:3:266:8 | Switch statement has fewer than two switch-clauses. | +| test.cpp:275:3:275:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | +| test.cpp:275:3:275:8 | Switch statement has a default clause that is not the first or last switch label. | \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-9-4-2/test.cpp b/cpp/misra/test/rules/RULE-9-4-2/test.cpp new file mode 100644 index 0000000000..fc948480b2 --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-4-2/test.cpp @@ -0,0 +1,304 @@ +// Test cases for RULE-9-4-2: The structure of a switch statement shall be +// appropriate This rule combines RULE-16-1 through RULE-16-7 + +void test_rule_16_1_well_formed(int expr) { + int i = 0; + + // COMPLIANT - well-formed switch with proper statements + switch (expr) { + case 1: + i++; // expression statement + break; + case 2: { // compound statement + i++; + } break; + case 3: + if (i > 0) { // selection statement + i++; + } + break; + case 4: + while (i < 10) { // iteration statement + i++; + } + break; + default: + break; + } + + // NON_COMPLIANT - switch with inappropriate statement (declaration) + switch (expr) { + case 1: + int j = 5; // declaration statement - not allowed + break; + default: + break; + } +} + +void test_rule_16_2_nested_labels(int expr) { + // COMPLIANT - labels directly within switch body + switch (expr) { + case 1: + break; + case 2: + break; + default: + break; + } + + // NON_COMPLIANT - nested switch labels (this would be a compiler error in + // practice) switch (expr) { case 1: + // { + // case 2: // nested label - not allowed + // break; + // } + // break; + // default: + // break; + // } +} + +void test_rule_16_3_termination(int expr) { + int i = 0; + + // COMPLIANT - all cases properly terminated + switch (expr) { + case 1: + i++; + break; + case 2: + case 3: // empty cases are fine + i++; + break; + case 4: + throw "error"; // throw is also valid termination + default: + break; + } + + // NON_COMPLIANT - case 1 falls through without break + switch (expr) { + case 1: // NON_COMPLIANT - missing break + i++; + case 2: // COMPLIANT - properly terminated + i++; + break; + default: + break; + } + + // NON_COMPLIANT - default case falls through + switch (expr) { + case 1: + i++; + break; + default: // NON_COMPLIANT - missing break + i++; + } +} + +void test_rule_16_4_default_label(int expr) { + int i = 0; + + // COMPLIANT - has default label + switch (expr) { + case 1: + i++; + break; + case 2: + i++; + break; + default: + break; + } + + // NON_COMPLIANT - missing default label + switch (expr) { + case 1: + i++; + break; + case 2: + i++; + break; + } +} + +void test_rule_16_5_default_position(int expr) { + int i = 0; + + // COMPLIANT - default is first + switch (expr) { + default: + i++; + break; + case 1: + i++; + break; + case 2: + i++; + break; + } + + // COMPLIANT - default is last + switch (expr) { + case 1: + i++; + break; + case 2: + i++; + break; + default: + i++; + break; + } + + // NON_COMPLIANT - default is in the middle + switch (expr) { + case 1: + i++; + break; + default: // NON_COMPLIANT - not first or last + i++; + break; + case 2: + i++; + break; + } +} + +void test_rule_16_6_two_clauses(int expr) { + int i = 0; + + // COMPLIANT - has multiple clauses + switch (expr) { + case 1: + i++; + break; + case 2: + i++; + break; + default: + break; + } + + // NON_COMPLIANT - only has default (single clause) + switch (expr) { + default: + i++; + break; + } + + // NON_COMPLIANT - only has one case plus default (still only one effective + // clause) + switch (expr) { + case 1: + default: + i++; + break; + } +} + +void test_rule_16_7_boolean_expression() { + int i = 0; + bool flag = true; + + // COMPLIANT - non-boolean expression + switch (i) { + case 0: + break; + case 1: + break; + default: + break; + } + + // NON_COMPLIANT - boolean expression + switch (flag) { + case true: + break; + case false: + break; + default: + break; + } + + // NON_COMPLIANT - boolean comparison expression + switch (i == 0) { + case true: + break; + case false: + break; + default: + break; + } +} + +int f() { return 1; } + +void test_complex_violations(int expr) { + int i = 0; + bool flag = true; + + // NON_COMPLIANT - multiple violations: + // - Boolean expression (16-7) + // - Missing default (16-4) + // - Single clause (16-6) + switch (flag) { + case true: + i++; + } + + // NON_COMPLIANT - multiple violations: + // - Fall-through case (16-3) + // - Default not first/last (16-5) + switch (expr) { + case 1: // NON_COMPLIANT - falls through + i++; + default: // NON_COMPLIANT - not first/last + i++; + break; + case 2: + i++; + break; + } + + switch (expr) { + int i = 0; + case 1: + i++; + } + + switch (int x = f(); x) { + case 1: + i++; + } + + switch (expr = f(); expr) { + case 1: + i++; + } + + switch (expr) { + case 1: + { + case 2: + i++; + } + } + + switch (expr) { + case 1: { + i++; + goto someLabel; + someLabel: + i++; + } + } + + switch (expr) { + int x = 1; + case 1: + i++; + } +} \ No newline at end of file From 221b9b2c4b1be5074aacb763549e8fa1c78bfa52 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 30 Jul 2025 18:41:25 -0400 Subject: [PATCH 03/47] Add some test cases --- cpp/misra/test/rules/RULE-9-4-2/test.cpp | 310 ++++++++++------------- 1 file changed, 128 insertions(+), 182 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-4-2/test.cpp b/cpp/misra/test/rules/RULE-9-4-2/test.cpp index fc948480b2..e99a2fb1f4 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-4-2/test.cpp @@ -1,304 +1,250 @@ -// Test cases for RULE-9-4-2: The structure of a switch statement shall be -// appropriate This rule combines RULE-16-1 through RULE-16-7 +int i = 0; -void test_rule_16_1_well_formed(int expr) { - int i = 0; - - // COMPLIANT - well-formed switch with proper statements - switch (expr) { +/** + * Test the initializer of a switch statement. + */ +void testInitializer(int expr) { + switch (expr) { // COMPLIANT: No initializer case 1: - i++; // expression statement - break; - case 2: { // compound statement i++; - } break; - case 3: - if (i > 0) { // selection statement - i++; - } - break; - case 4: - while (i < 10) { // iteration statement - i++; - } break; default: + i--; break; } - // NON_COMPLIANT - switch with inappropriate statement (declaration) - switch (expr) { + switch (int j = 0; + expr) { // COMPLIANT: Only declaration statement can be an initializer case 1: - int j = 5; // declaration statement - not allowed + j++; break; default: + j--; break; } -} -void test_rule_16_2_nested_labels(int expr) { - // COMPLIANT - labels directly within switch body - switch (expr) { + switch ( + i = 1; + expr) { // NON_COMPLIANT: Only declaration statement can be an initializer case 1: - break; - case 2: + i++; break; default: + i--; break; } - - // NON_COMPLIANT - nested switch labels (this would be a compiler error in - // practice) switch (expr) { case 1: - // { - // case 2: // nested label - not allowed - // break; - // } - // break; - // default: - // break; - // } } -void test_rule_16_3_termination(int expr) { - int i = 0; - - // COMPLIANT - all cases properly terminated - switch (expr) { +void testNestedCaseLabels(int expr) { + switch (expr) { // COMPLIANT: Consecutive case labels are allowed case 1: - i++; - break; case 2: - case 3: // empty cases are fine i++; break; - case 4: - throw "error"; // throw is also valid termination default: + i--; break; } - // NON_COMPLIANT - case 1 falls through without break - switch (expr) { - case 1: // NON_COMPLIANT - missing break - i++; - case 2: // COMPLIANT - properly terminated + switch (expr) { // NON_COMPLIANT: Statements with case labels should all be at + // the same level + case 1: { + case 2: i++; break; + } default: break; } - // NON_COMPLIANT - default case falls through - switch (expr) { + switch (expr) { // NON_COMPLIANT: Statements with case labels should all be at + // the same level case 1: i++; break; - default: // NON_COMPLIANT - missing break - i++; + case 2: { + default: + break; + } } } -void test_rule_16_4_default_label(int expr) { - int i = 0; - - // COMPLIANT - has default label - switch (expr) { - case 1: +void testOtherLabelsInBranch(int expr) { + switch (expr) { // NON_COMPLIANT: Non-case labels appearing in a switch branch + case 1: { i++; - break; - case 2: + goto someLabel; + someLabel: i++; break; default: break; } + } +} + +void testLeadingNonCaseStatement(int expr) { + switch (expr) { // NON_COMPLIANT: Non-case statement is the first statement in + // the switch body - // NON_COMPLIANT - missing default label - switch (expr) { case 1: i++; break; - case 2: - i++; + default: break; } } -void test_rule_16_5_default_position(int expr) { - int i = 0; +[[noreturn]] void f() {} +void g() {} - // COMPLIANT - default is first - switch (expr) { - default: - i++; - break; +void testSwitchBranchTerminator(int expr) { + switch (expr) { // COMPLIANT: Break is allowed as a branch terminator case 1: i++; break; - case 2: - i++; + default: break; } - // COMPLIANT - default is last - switch (expr) { + for (int j = 0; j++; j < 10) { + switch (expr) { // COMPLIANT: Continue is allowed as a branch terminator + case 1: + i++; + continue; + default: + continue; + } + } + + switch (expr) { // COMPLIANT: Goto is allowed as a branch terminator case 1: i++; - break; - case 2: - i++; - break; + goto error; default: - i++; - break; + goto error; } - // NON_COMPLIANT - default is in the middle - switch (expr) { + switch (expr) { // COMPLIANT: Throw is allowed as a branch terminator case 1: i++; - break; - default: // NON_COMPLIANT - not first or last - i++; - break; - case 2: - i++; - break; + throw; + default: + throw; } -} - -void test_rule_16_6_two_clauses(int expr) { - int i = 0; - // COMPLIANT - has multiple clauses - switch (expr) { + switch (expr) { // COMPLIANT: Call to a `[[noreturn]]` function is allowed as + // a branch terminator case 1: i++; - break; - case 2: - i++; - break; + f(); default: - break; + f(); } - // NON_COMPLIANT - only has default (single clause) - switch (expr) { + switch (expr) { // NON_COMPLIANT: Branch ends with a call to a function that + // is not `[[noreturn]]` + case 1: + i++; + g(); default: + g(); + } + + switch (expr) { // COMPLIANT: Return is allowed as a branch terminator + case 1: i++; - break; + return; + default: + return; } - // NON_COMPLIANT - only has one case plus default (still only one effective - // clause) - switch (expr) { + switch (expr) { // COMPLIANT: Empty statement with `[[fallthrough]]` is + // allowed as a branch terminator case 1: + i++; + [[fallthrough]]; default: i++; - break; } -} -void test_rule_16_7_boolean_expression() { - int i = 0; - bool flag = true; +error: + return; +} - // COMPLIANT - non-boolean expression - switch (i) { - case 0: - break; +void testSwitchBranchCount(int expr) { + switch (expr) { // COMPLIANT: Branch count is 2 case 1: + i++; break; default: + i++; break; } - // NON_COMPLIANT - boolean expression - switch (flag) { - case true: - break; - case false: - break; + switch (expr) { // NON_COMPLIANT: Branch count is 1 default: + i++; break; } - // NON_COMPLIANT - boolean comparison expression - switch (i == 0) { - case true: - break; - case false: - break; + switch (expr) { // NON_COMPLIANT: Branch count is 1 + case 1: + case 2: default: + i++; break; } } -int f() { return 1; } - -void test_complex_violations(int expr) { - int i = 0; - bool flag = true; - - // NON_COMPLIANT - multiple violations: - // - Boolean expression (16-7) - // - Missing default (16-4) - // - Single clause (16-6) - switch (flag) { - case true: - i++; - } +enum E { V1, V2, V3 }; - // NON_COMPLIANT - multiple violations: - // - Fall-through case (16-3) - // - Default not first/last (16-5) - switch (expr) { - case 1: // NON_COMPLIANT - falls through - i++; - default: // NON_COMPLIANT - not first/last +void testDefaultLabelPresence(int expr) { + switch (expr) { // COMPLIANT: There is a default branch + case 1: i++; break; - case 2: + default: i++; break; } - switch (expr) { - int i = 0; - case 1: - i++; - } - - switch (int x = f(); x) { + switch (expr) { // NON_COMPLIANT: Default branch is missing case 1: i++; + break; } - switch (expr = f(); expr) { - case 1: - i++; - } + E e; - switch (expr) { - case 1: - { - case 2: - i++; - } + switch (e) { // COMPLIANT: There is a default branch + case V1: + i++; + break; + default: + break; } - switch (expr) { - case 1: { + switch (e) { // NON_COMPLIANT: Default branch is missing on a non-exhaustive + // enum switch + case V1: i++; - goto someLabel; - someLabel: + break; + case V2: i++; - } + break; } - switch (expr) { - int x = 1; - case 1: + switch (e) { // COMPLIANT: Default branch can be omitted on an exhaustive enum + // switch + case V1: + i++; + break; + case V2: i++; + break; + case V3: + i++; + break; } } \ No newline at end of file From fabb7e5db8af43e4b245dee1fb61dc971674b1b2 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 31 Jul 2025 18:39:06 -0400 Subject: [PATCH 04/47] Finish first draft --- .../AppropriateStructureOfSwitchStatement.ql | 109 +++++++++++------- ...opriateStructureOfSwitchStatement.expected | 27 ++--- cpp/misra/test/rules/RULE-9-4-2/test.cpp | 9 +- 3 files changed, 83 insertions(+), 62 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql index 3d68e31abb..f419834325 100644 --- a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql +++ b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql @@ -19,53 +19,74 @@ import cpp import codingstandards.cpp.misra import codingstandards.cpp.SwitchStatement +import codingstandards.cpp.Noreturn from SwitchStmt switch, string message where not isExcluded(switch, StatementsPackage::appropriateStructureOfSwitchStatementQuery()) and - ( - // RULE-16-1: Switch not well-formed (has inappropriate statements) - exists(SwitchCase case | - case = switch.getASwitchCase() and - switchCaseNotWellFormed(case) and - message = "has a case that contains inappropriate statements (only expression, compound, selection, iteration or try statements are allowed)" + /* 1. There is a statement that appears as an initializer and is not a declaration statement. */ + exists(Stmt initializer | initializer = switch.getInitialization() | + not initializer instanceof DeclStmt + ) and + message = "contains a statement that that is not a simple declaration" + or + /* 2. There is a switch case label that does not lead a branch (i.e. a switch case label is nested). */ + exists(SwitchCase case | case = switch.getASwitchCase() | case instanceof NestedSwitchCase) and + message = "contains a switch label that is not directly within the switch body" + or + /* 3. There is a non-case label in a label group. */ + exists(SwitchCase case | case = switch.getASwitchCase() | + case.getAStmt().getChildStmt*() instanceof LabelStmt + ) and + message = "contains a statement label that is not a case label" + or + /* 4. There is a statement before the first case label. */ + exists(Stmt switchBody | switchBody = switch.getStmt() | + not switchBody.getChild(0) instanceof SwitchCase + ) and + message = "has a statement that is not a case label as its first element" + or + /* 5. There is a switch case whose terminator is not one of the allowed kinds. */ + exists(SwitchCase case, Stmt lastStmt | + case = switch.getASwitchCase() and lastStmt = case.getLastStmt() + | + not ( + lastStmt instanceof BreakStmt or + lastStmt instanceof ReturnStmt or + lastStmt instanceof GotoStmt or + lastStmt instanceof ContinueStmt or + lastStmt.(ExprStmt).getExpr() instanceof ThrowExpr or + lastStmt.(ExprStmt).getExpr().(Call).getTarget() instanceof NoreturnFunction or + lastStmt.getAnAttribute().getName().matches("%fallthrough") // We'd like to consider compiler variants such as `clang::fallthrough`. ) - or - // RULE-16-2: Nested switch labels - exists(SwitchCase case | - case = switch.getASwitchCase() and - case instanceof NestedSwitchCase and - message = "contains a switch label that is not directly within the switch body" - ) - or - // RULE-16-3: Non-empty case doesn't terminate with break - exists(SwitchCase case | - case = switch.getASwitchCase() and - case instanceof CaseDoesNotTerminate and - message = "has a non-empty case that does not terminate with an unconditional break or throw statement" - ) - or - // RULE-16-4: Missing default clause - not switch.hasDefaultCase() and - message = "is missing a default clause" - or - // RULE-16-5: Default clause not first or last - exists(SwitchCase defaultCase | - switch.getDefaultCase() = defaultCase and - exists(defaultCase.getPreviousSwitchCase()) and - finalClauseInSwitchNotDefault(switch) and - message = "has a default clause that is not the first or last switch label" - ) - or - // RULE-16-6: Less than two case clauses - count(SwitchCase case | - switch.getASwitchCase() = case and - case.getNextSwitchCase() != case.getFollowingStmt() - ) + 1 < 2 and - message = "has fewer than two switch-clauses" - or - // RULE-16-7: Boolean switch expression - switch instanceof BooleanSwitchStmt and - message = "has a controlling expression of essentially Boolean type" - ) + ) and + message = "is missing a terminator that moves the control out of its body" + or + /* 6. The switch statement does not have more than two unique branches. */ + count(SwitchCase case | + case = switch.getASwitchCase() and + /* + * If the next switch case is the following statement of this switch case, then the two + * switch cases are consecutive and should be considered as constituting one branch + * together. + */ + + not case.getNextSwitchCase() = case.getFollowingStmt() + | + case + ) < 2 and + message = "contains less than two branches" + or + /* 7-1. The switch statement is not an enum switch statement and is missing a default case. */ + not switch instanceof EnumSwitch and + not switch.hasDefaultCase() and + message = "lacks a default case" + or + /* + * 7-2. The switch statement is an enum switch statement and is missing a branch for a + * variant. + */ + + exists(switch.(EnumSwitch).getAMissingCase()) and + message = "lacks a case for one of its variants" select switch, "Switch statement " + message + "." diff --git a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected index 8900facc63..ab8320ef8f 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected +++ b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected @@ -1,15 +1,12 @@ -| test.cpp:40:3:40:8 | Switch statement has a case that contains inappropriate statements (only expression, compound, selection, iteration or try statements are allowed). | -| test.cpp:86:3:86:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | -| test.cpp:86:3:86:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | -| test.cpp:98:3:98:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | -| test.cpp:123:3:123:8 | Switch statement is missing a default clause. | -| test.cpp:166:3:166:8 | Switch statement has a default clause that is not the first or last switch label. | -| test.cpp:203:3:203:8 | Switch statement has fewer than two switch-clauses. | -| test.cpp:210:3:210:8 | Switch statement has fewer than two switch-clauses. | -| test.cpp:235:3:235:8 | Switch statement has a controlling expression of essentially Boolean type. | -| test.cpp:245:3:245:8 | Switch statement has a controlling expression of essentially Boolean type. | -| test.cpp:266:3:266:8 | Switch statement has a controlling expression of essentially Boolean type. | -| test.cpp:266:3:266:8 | Switch statement is missing a default clause. | -| test.cpp:266:3:266:8 | Switch statement has fewer than two switch-clauses. | -| test.cpp:275:3:275:8 | Switch statement has a non-empty case that does not terminate with an unconditional break or throw statement. | -| test.cpp:275:3:275:8 | Switch statement has a default clause that is not the first or last switch label. | \ No newline at end of file +| test.cpp:28:3:37:3 | switch (...) ... | Switch statement contains a statement that that is not a simple declaration. | +| test.cpp:51:3:60:3 | switch (...) ... | Switch statement contains a switch label that is not directly within the switch body. | +| test.cpp:62:3:71:3 | switch (...) ... | Switch statement contains a switch label that is not directly within the switch body. | +| test.cpp:75:3:85:3 | switch (...) ... | Switch statement contains a statement label that is not a case label. | +| test.cpp:89:3:97:3 | switch (...) ... | Switch statement has a statement that is not a case label as its first element. | +| test.cpp:147:3:154:3 | switch (...) ... | Switch statement is missing a terminator that moves the control out of its body. | +| test.cpp:188:3:192:3 | switch (...) ... | Switch statement contains less than two branches. | +| test.cpp:194:3:200:3 | switch (...) ... | Switch statement contains less than two branches. | +| test.cpp:215:3:219:3 | switch (...) ... | Switch statement contains less than two branches. | +| test.cpp:215:3:219:3 | switch (...) ... | Switch statement lacks a default case. | +| test.cpp:223:3:229:3 | switch (...) ... | Switch statement lacks a case for one of its variants. | +| test.cpp:231:3:239:3 | switch (...) ... | Switch statement lacks a case for one of its variants. | diff --git a/cpp/misra/test/rules/RULE-9-4-2/test.cpp b/cpp/misra/test/rules/RULE-9-4-2/test.cpp index e99a2fb1f4..6bbb97e81b 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-4-2/test.cpp @@ -1,3 +1,5 @@ +#include + int i = 0; /** @@ -77,16 +79,16 @@ void testOtherLabelsInBranch(int expr) { someLabel: i++; break; + } default: break; } - } } void testLeadingNonCaseStatement(int expr) { switch (expr) { // NON_COMPLIANT: Non-case statement is the first statement in // the switch body - + int x = 1; case 1: i++; break; @@ -95,7 +97,7 @@ void testLeadingNonCaseStatement(int expr) { } } -[[noreturn]] void f() {} +[[noreturn]] void f() { exit(0); } void g() {} void testSwitchBranchTerminator(int expr) { @@ -166,6 +168,7 @@ void testSwitchBranchTerminator(int expr) { [[fallthrough]]; default: i++; + break; } error: From 6909528361de035834ce6cdd60e7fa30311560cc Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 1 Aug 2025 14:56:20 -0400 Subject: [PATCH 05/47] Add test case for Rule 9.5.2 --- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 129 +++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 cpp/misra/test/rules/RULE-9-5-2/test.cpp diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp new file mode 100644 index 0000000000..9d5255f0c0 --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -0,0 +1,129 @@ +#include + +/* Helper functions */ +std::vector getData() { return {1, 2, 3}; } +std::vector processData(const std::vector &input) { return input; } +std::vector getContainer() { return {4, 5, 6}; } + +class MyContainer { +public: + MyContainer() = default; + MyContainer(std::vector data) : data_(data) {} + std::vector::iterator begin() { return data_.begin(); } + std::vector::iterator end() { return data_.end(); } + +private: + std::vector data_{7, 8, 9}; +}; + +class ConvertibleToVector { +public: + operator std::vector() const { return {}; } + std::array::iterator begin() { return data_.begin(); } + std::array::iterator end() { return data_.end(); } + +private: + std::array data_{7, 8, 9}; +}; + +std::vector operator+(const std::vector &a, + const std::vector &b) { + std::vector result = a; + result.insert(result.end(), b.begin(), b.end()); + return result; +} + +std::vector convertToIntVector(std::vector vector) { return vector; } + +int main() { + std::vector localVec = {1, 2, 3}; + std::vector *vecPtr = &localVec; + ConvertibleToVector convertible; + + /* ========== 1. EXPLICIT FUNCTION CALLS ========== */ + + for (auto x : getContainer()) { // COMPLIANT: 1 function call only + } + + for (auto x : processData(getData())) { // NON_COMPLIANT: 2 function calls + } + + /* ========== 2. OBJECT CREATION (CONSTRUCTOR CALLS) ========== */ + + for (auto x : + std::vector{1, 2, 3}) { // COMPLIANT: 1 constructor call only + } + + for (auto x : MyContainer()) { // COMPLIANT: 1 constructor call only + } + + for (auto x : std::string("hello")) { // COMPLIANT: 1 constructor call only + } + + for (auto x : std::vector( + getData())) { // NON-COMPLIANT: 1 constructor + 1 function call + } + + for (auto x : MyContainer(processData( + localVec))) { // NON-COMPLIANT: 1 constructor + 1 function call + } + + auto data = std::vector(getData()); + for (auto x : data) { // NON-COMPLIANT: 1 constructor + 1 function call + } + + MyContainer myContainer = MyContainer(processData(localVec)); + for (auto x : myContainer) { // NON-COMPLIANT: 1 constructor + 1 function call + } + + /* ========== 3. OPERATOR OVERLOADING ========== */ + + std::vector anotherVec = {4, 5, 6}; + for (auto x : localVec + anotherVec) { // COMPLIANT: 1 operator+ call only + } + + std::vector vec1 = {1}, vec2 = {2}, vec3 = {3}; + for (auto x : (vec1 + vec2) + vec3) { // NON-COMPLIANT: 2 operator+ calls + } + + for (auto x : + getData() + + processData( + localVec)) { // NON-COMPLIANT: 2 function calls + 1 operator call + } + + std::vector vec1 = {1}, vec2 = {2}, vec3 = {3}; + std::vector appendedVector = (vec1 + vec2) + vec3; + for (auto x : appendedVector) { // COMPLIANT: 0 calls + } + + std::vector appendedVector2 = getData() + processData(localVec); + for (auto x : appendedVector2) { // COMPLIANT: 0 calls + } + + /* ========== 4. IMPLICIT CONVERSIONS ========== */ + + ConvertibleToVector convertible; + for (auto x : convertible) { // COMPLIANT: 1 conversion operator call only + } + + for (auto x : + convertToIntVector(convertible)) { // NON_COMPLIANT: 1 function call + 1 + // conversion operator call + } + + for (auto x : + convertToIntVector(convertible)) { // NON_COMPLIANT: 1 function call + 1 + // conversion operator call + } + + std::vector intVector1 = convertToIntVector(convertible); + for (auto x : intVector1) { // NON_COMPLIANT: 1 function call + 1 + // conversion operator call + } + + std::vector intVector2 = convertToIntVector(convertible); + for (auto x : intVector2) { // NON_COMPLIANT: 1 function call + 1 + // conversion operator call + } +} \ No newline at end of file From 0b5250f892849cee3c9959005534c1196513a30f Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 1 Aug 2025 15:07:53 -0400 Subject: [PATCH 06/47] Minor --- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp index 9d5255f0c0..ba3bfa191b 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -18,7 +18,7 @@ class MyContainer { class ConvertibleToVector { public: - operator std::vector() const { return {}; } + operator std::vector() const { return {7, 8, 9}; } std::array::iterator begin() { return data_.begin(); } std::array::iterator end() { return data_.end(); } From b23986210658da7dab5751520793f26aad706c2a Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 1 Aug 2025 15:52:58 -0400 Subject: [PATCH 07/47] Update test case for Rule 9.5.2 --- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 26 +++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp index ba3bfa191b..025a247ad7 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -1,4 +1,5 @@ #include +#include /* Helper functions */ std::vector getData() { return {1, 2, 3}; } @@ -9,8 +10,8 @@ class MyContainer { public: MyContainer() = default; MyContainer(std::vector data) : data_(data) {} - std::vector::iterator begin() { return data_.begin(); } - std::vector::iterator end() { return data_.end(); } + std::vector::const_iterator begin() const { return data_.begin(); } + std::vector::const_iterator end() const { return data_.end(); } private: std::vector data_{7, 8, 9}; @@ -19,15 +20,15 @@ class MyContainer { class ConvertibleToVector { public: operator std::vector() const { return {7, 8, 9}; } - std::array::iterator begin() { return data_.begin(); } - std::array::iterator end() { return data_.end(); } + std::array::const_iterator begin() const { return data_.begin(); } + std::array::const_iterator end() const { return data_.end(); } private: std::array data_{7, 8, 9}; }; -std::vector operator+(const std::vector &a, - const std::vector &b) { +std::vector operator+(std::vector a, + std::vector b) { std::vector result = a; result.insert(result.end(), b.begin(), b.end()); return result; @@ -37,8 +38,6 @@ std::vector convertToIntVector(std::vector vector) { return vector; } int main() { std::vector localVec = {1, 2, 3}; - std::vector *vecPtr = &localVec; - ConvertibleToVector convertible; /* ========== 1. EXPLICIT FUNCTION CALLS ========== */ @@ -92,7 +91,6 @@ int main() { localVec)) { // NON-COMPLIANT: 2 function calls + 1 operator call } - std::vector vec1 = {1}, vec2 = {2}, vec3 = {3}; std::vector appendedVector = (vec1 + vec2) + vec3; for (auto x : appendedVector) { // COMPLIANT: 0 calls } @@ -104,26 +102,26 @@ int main() { /* ========== 4. IMPLICIT CONVERSIONS ========== */ ConvertibleToVector convertible; - for (auto x : convertible) { // COMPLIANT: 1 conversion operator call only + for (int x : convertible) { // COMPLIANT: 1 conversion operator call only } - for (auto x : + for (int x : convertToIntVector(convertible)) { // NON_COMPLIANT: 1 function call + 1 // conversion operator call } - for (auto x : + for (int x : convertToIntVector(convertible)) { // NON_COMPLIANT: 1 function call + 1 // conversion operator call } std::vector intVector1 = convertToIntVector(convertible); - for (auto x : intVector1) { // NON_COMPLIANT: 1 function call + 1 + for (int x : intVector1) { // NON_COMPLIANT: 1 function call + 1 // conversion operator call } std::vector intVector2 = convertToIntVector(convertible); - for (auto x : intVector2) { // NON_COMPLIANT: 1 function call + 1 + for (int x : intVector2) { // NON_COMPLIANT: 1 function call + 1 // conversion operator call } } \ No newline at end of file From a0133887da526bb2ab7ed971556ecfe142d07b1a Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 1 Aug 2025 18:01:15 -0400 Subject: [PATCH 08/47] Add first draft of `ForRangeInitializerAtMostOneFunctionCall` --- ...orRangeInitializerAtMostOneFunctionCall.ql | 8 +-- ...eInitializerAtMostOneFunctionCall.expected | 9 +++- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 54 ++++++++++--------- 3 files changed, 43 insertions(+), 28 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql index 47d27287bc..b78da2ad99 100644 --- a/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql +++ b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql @@ -19,7 +19,9 @@ import cpp import codingstandards.cpp.misra -from +from RangeBasedForStmt foreach, string message where - not isExcluded(x, StatementsPackage::forRangeInitializerAtMostOneFunctionCallQuery()) and -select + not isExcluded(foreach, StatementsPackage::forRangeInitializerAtMostOneFunctionCallQuery()) and + count(Call call | call = foreach.getRange().getAChild*() | call) >= 2 and + message = "has nested call expression in its initializer" +select foreach, "Range-based for loop " + message + "." diff --git a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected index 2ec1a0ac6c..a8567ff48d 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected +++ b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected @@ -1 +1,8 @@ -No expected results have yet been specified \ No newline at end of file +| test.cpp:48:3:49:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:56:3:59:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:71:3:73:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:95:3:97:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:99:3:101:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:103:3:107:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:116:3:119:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:121:3:124:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp index 025a247ad7..b9ec5a0574 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -1,5 +1,5 @@ -#include #include +#include /* Helper functions */ std::vector getData() { return {1, 2, 3}; } @@ -10,8 +10,8 @@ class MyContainer { public: MyContainer() = default; MyContainer(std::vector data) : data_(data) {} - std::vector::const_iterator begin() const { return data_.begin(); } - std::vector::const_iterator end() const { return data_.end(); } + std::vector::iterator begin() { return data_.begin(); } + std::vector::iterator end() { return data_.end(); } private: std::vector data_{7, 8, 9}; @@ -20,15 +20,16 @@ class MyContainer { class ConvertibleToVector { public: operator std::vector() const { return {7, 8, 9}; } - std::array::const_iterator begin() const { return data_.begin(); } - std::array::const_iterator end() const { return data_.end(); } + std::array::iterator begin() { return data_.begin(); } + std::array::iterator end() { return data_.end(); } + std::array::const_iterator begin() const { return data_.cbegin(); } + std::array::const_iterator end() const { return data_.cend(); } private: std::array data_{7, 8, 9}; }; -std::vector operator+(std::vector a, - std::vector b) { +std::vector operator+(std::vector a, std::vector b) { std::vector result = a; result.insert(result.end(), b.begin(), b.end()); return result; @@ -49,8 +50,12 @@ int main() { /* ========== 2. OBJECT CREATION (CONSTRUCTOR CALLS) ========== */ + for (auto x : std::vector(3)) { // COMPLIANT: 1 constructor call only + } + for (auto x : - std::vector{1, 2, 3}) { // COMPLIANT: 1 constructor call only + std::vector{1, 2, 3}) { // NON_COMPLIANT: 2 constructor call to + // `vector` and `initializer_list`, respectively } for (auto x : MyContainer()) { // COMPLIANT: 1 constructor call only @@ -77,12 +82,22 @@ int main() { /* ========== 3. OPERATOR OVERLOADING ========== */ + std::vector vec1 = {1}, vec2 = {2}, vec3 = {3}; + std::vector appendedVector = (vec1 + vec2) + vec3; + for (auto x : appendedVector) { // COMPLIANT: 0 calls + } + + std::vector appendedVector2 = getData() + processData(localVec); + for (auto x : appendedVector2) { // COMPLIANT: 0 calls + } + std::vector anotherVec = {4, 5, 6}; - for (auto x : localVec + anotherVec) { // COMPLIANT: 1 operator+ call only + for (auto x : localVec + anotherVec) { // NON_COMPLIANT: 2 calls to vector's + // constructor, 1 operator+ call } - std::vector vec1 = {1}, vec2 = {2}, vec3 = {3}; - for (auto x : (vec1 + vec2) + vec3) { // NON-COMPLIANT: 2 operator+ calls + for (auto x : (vec1 + vec2) + vec3) { // NON-COMPLIANT: 3 calls to vector's + // constructor, 2 operator+ calls } for (auto x : @@ -91,18 +106,11 @@ int main() { localVec)) { // NON-COMPLIANT: 2 function calls + 1 operator call } - std::vector appendedVector = (vec1 + vec2) + vec3; - for (auto x : appendedVector) { // COMPLIANT: 0 calls - } - - std::vector appendedVector2 = getData() + processData(localVec); - for (auto x : appendedVector2) { // COMPLIANT: 0 calls - } - /* ========== 4. IMPLICIT CONVERSIONS ========== */ ConvertibleToVector convertible; - for (int x : convertible) { // COMPLIANT: 1 conversion operator call only + for (int x : + ConvertibleToVector()) { // COMPLIANT: 1 conversion operator call only } for (int x : @@ -116,12 +124,10 @@ int main() { } std::vector intVector1 = convertToIntVector(convertible); - for (int x : intVector1) { // NON_COMPLIANT: 1 function call + 1 - // conversion operator call + for (int x : intVector1) { // COMPLIANT: 0 function calls } std::vector intVector2 = convertToIntVector(convertible); - for (int x : intVector2) { // NON_COMPLIANT: 1 function call + 1 - // conversion operator call + for (int x : intVector2) { // COMPLIANT: 0 function calls } } \ No newline at end of file From 1c623f4dc2c3664152114ba5622b9ae515415b2c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 8 Aug 2025 12:57:43 -0400 Subject: [PATCH 09/47] Add test cases --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 147 +++++++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 cpp/misra/test/rules/RULE-9-5-1/test.cpp diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp new file mode 100644 index 0000000000..0dafea56ef --- /dev/null +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -0,0 +1,147 @@ +void f(int &x) {} // Function that takes a non-const integer reference +void g(int *x) {} // Function that takes a non-const integer pointer + +int main() { + int j = 5; + + /* ========== 1. Type of the initialized counter variable ========== */ + + for (int i = 0; i < 10; i++) { // COMPLIANT: `i` has an integer type + } + + for (float i = 0.0; i < 10; + i++) { // NON_COMPLIANT: `i` has a non-integer type + } + + /* ========== 2. Termination condition ========== */ + + for (int i = 0; i < 10; i++) { // COMPLIANT: `<` is a relational operator + } + + for (int i = 0; i == 10; + i++) { // NON_COMPLIANT: `==` is not a relational operator + } + + for (int i = 0; j < 10; i++) { // NON_COMPLIANT: `j` is not the loop counter + j++; + } + + /* ========== 3. Updating expression ========== */ + + for (int i = 0; i < 10; + ++i) { // COMPLIANT: Pre-increment operator used as the update expression + } + + for (int i = 0; i < 10; i++) { // COMPLIANT: Post-increment operator used as + // the update expression + } + + for (int i = 0; i < 10; i += 3) { // COMPLIANT: Add-and-assign operator used + // as the update expression with loop step 3 + } + + for (int i = 0; i < 10; + i *= 2) { // NON_COMPLIANT: Mutiplication is not incrementing + } + + /* ========== 4. Type of the loop counter and the loop bound ========== */ + + for (int i = 0; i < 10; i++) { // COMPLIANT: 0 and 10 are of same type + } + + for (unsigned long long int i = 0; i < 10; + i++) { // COMPLIANT: The loop counter has type bigger than that of the + // loop bound + } + + for (int i = 0; i < 10ull; + i++) { // NON_COMPLIANT: The type of the loop counter is not bigger + // than that of the loop bound + } + + for (int i = 0; i < j; + i++) { // NON_COMPLIANT: The loop bound is not a constant + } + + /* ========== 5. Immutability of the loop bound and the loop step ========== + */ + + for (int i = 0; i < 10; + i++) { // COMPLIANT: The update expression is an post-increment operation + // and its loop step is always 1 + } + + for (int i = 0; i < 10; i += 2) { // COMPLIANT: The loop step is always 2 + } + + for (int i = 0; i < 10; + i += + j) { // COMPLIANT: The loop step `j` is not mutated anywhere in the loop + } + + for (int i = 0; i < 10; + i += j) { // NON_COMPLIANT: The loop step `j` is mutated in the loop + j++; + } + + for (int i = 0; i < 10; + i += j, j++) { // NON_COMPLIANT: The loop step `j` is mutated in the loop + } + + for (int i = 0; i < j; i++) { // COMPLIANT: The loop bound `j` is not mutated + // anywhere in the loop + } + + for (int i = 0; i < j; i++) { // COMPLIANT: The loop bound `j` is not mutated + // anywhere in the loop + } + + for (int i = 0; i < j; + i++) { // NON_COMPLIANT: The loop bound `j` is mutated in the loop + j++; + } + + /* ========== 6. Existence of pointers to the loop counter, loop bound, and + * loop step ========== */ + + int k = 10; + int l = 2; + + for (int i = 0; i < k; i += l) { // COMPLIANT: The loop counter, bound, and + // step are not taken addresses of + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + // as a non-const reference + f(j); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + // as a non-const pointer + g(&j); + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop bound is passed as a non-const pointer + f(k); + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop bound is passed as a non-const pointer + g(&k); + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer + f(l); + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer + g(&l); + } +} \ No newline at end of file From c2cbed3bd746656e34259a00f01014f07a539e0e Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 18 Aug 2025 18:51:46 -0400 Subject: [PATCH 10/47] Update unit test --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 40 ++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 0dafea56ef..f4c486ba04 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -113,17 +113,51 @@ int main() { for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed // as a non-const reference - f(j); + int &m = i; } for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed // as a non-const pointer - g(&j); + int *m = &i; + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop bound is passed as a non-const reference + int &m = k; } for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is passed as a non-const pointer + int *m = &k; + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop step is passed as a non-const reference + int &m = l; + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer + int *m = &l; + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + // as a non-const reference + f(i); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + // as a non-const pointer + g(&i); + } + + for (int i = j; i < k; + i += + l) { // NON_COMPLIANT: The loop bound is passed as a non-const reference f(k); } @@ -135,7 +169,7 @@ int main() { for (int i = j; i < k; i += - l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer + l) { // NON_COMPLIANT: The loop step is passed as a non-const reference f(l); } From b7ae38e82281e0411068e9e13bc51c8457e5f687 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 27 Aug 2025 18:54:50 -0400 Subject: [PATCH 11/47] Add more cases and more vocabularies to reason about them --- .../LegacyForStatementsShouldBeSimple.ql | 234 +++++++++++++++++- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 140 ++++++++--- 2 files changed, 337 insertions(+), 37 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 1a29e90a40..257e151580 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -18,7 +18,235 @@ import cpp import codingstandards.cpp.misra -from +/** + * A comparison expression that has the minimum qualification as being a valid termination + * condition of a legacy for-loop. It is characterized by a value read from a variable being + * compared to a value, which is supposed to be the loop bound. + */ +class LegacyForLoopCondition extends RelationalOperation { + ForStmt forLoop; + VariableAccess loopCounter; + Expr loopBound; + + LegacyForLoopCondition() { + loopCounter = this.getAnOperand() and + loopBound = this.getAnOperand() and + loopCounter.getTarget() = forLoop.getInitialization().(DeclStmt).getADeclaration() and + loopBound != loopCounter + } + + VariableAccess getLoopCounter() { result = loopCounter } + + Expr getLoopBound() { result = loopBound } +} + +/** + * Holds if the given expression is impure and contains an access to the variable, and + * thus may mutate the variable. + * + * Note that this relation over-approximates and might include impure expressions that + * in fact do not mutate the variable. + */ +predicate exprWithVarAccessMaybeImpure(Expr expr, Variable variable) { + exists(VariableAccess varAccess | + expr.mayBeImpure() and + expr.getAChild*() = varAccess and + variable = varAccess.getTarget() + ) +} + +/** + * Gets the loop step of a legacy for loop. + * + * This predicate assumes the update expression of the given for loop is an add-and-assign + * (`+=`) or sub-and-assign (`-=`) expression, so the update expression that is an increment + * (`++`) or a decrement (`--`) operation should be handled using different means than this + * predicate. + */ +Expr getLoopStepOfForStmt(ForStmt forLoop) { + result = forLoop.getUpdate().(AssignAddExpr).getRValue() or + result = forLoop.getUpdate().(AssignSubExpr).getRValue() +} + +/** + * Holds if the given function has as parameter at a given index a pointer to a + * constant value or a reference of a constant value. + */ +predicate functionHasConstPointerOrReferenceParameter(Function function, int index) { + function.getParameter(index).getType().(PointerType).getBaseType().isConst() or + function.getParameter(index).getType().(ReferenceType).getBaseType().isConst() +} + +/** + * Holds if the the variable behind a given variable access is taken its address + * in a declaration in either the body of the for-loop or in its update expression. + * + * e.g.1. The loop counter variable `i` in the body is taken its address in the + * declaration of a pointer variable `m`. + * ``` C++ + * for (int i = 0; i < k; i += l) { + * int *m = &i; + * } + * ``` + * e.g.2. The loop bound variable `k` in the body is taken its address in the + * declaration of a pointer variable `m`. + * ``` C++ + * for (int i = j; i < k; i += l) { + * int *m = &k; + * } + * ``` + */ +predicate variableAddressTakenInDeclaration(ForStmt forLoop, VariableAccess baseVariableAccess) { + exists(AddressOfExpr addressOfExpr, DeclStmt decl | + decl.getParentStmt+() = forLoop and + decl.getADeclarationEntry().(VariableDeclarationEntry).getVariable().getInitializer().getExpr() = + addressOfExpr and + baseVariableAccess.getTarget() = + forLoop.getCondition().(LegacyForLoopCondition).getLoopBound().(VariableAccess).getTarget() + ) +} + +/** + * Holds if the the variable behind a given variable access is taken its address + * as an argument of a call in either the body of the for-loop or in its update + * expression. + * + * e.g.1. The loop counter variable `i` in the body is taken its address in the + * declaration of a pointer variable `m`. + * ``` C++ + * for (int i = 0; i < k; i += l) { + * g(&i); + * } + * ``` + * e.g.2. The loop bound variable `k` in the body is taken its address in the + * declaration of a pointer variable `m`. + * ``` C++ + * for (int i = j; i < k; i += l) { + * g(&k); + * } + * ``` + */ +predicate variableAddressTakenAsConstArgument( + ForStmt forLoop, VariableAccess baseVariableAccess, Call call +) { + exists(AddressOfExpr addressOfExpr, int index | + call.getParent+() = forLoop.getAChild+() and + call.getArgument(index).getAChild*() = addressOfExpr and + functionHasConstPointerOrReferenceParameter(call.getTarget(), index) and + addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() + ) +} + +/** + * Holds if the the variable behind a given variable access is taken its address + * as an argument of a complex expression in either the body of the for-loop or + * in its update expression. + * + * e.g. The loop counter variable `i` in the body and the loop bound variable `k` + * is taken its address in a compound expression. + * ``` C++ + * for (int i = 0; i < k; i += l) { + * *(cond ? &i : &k) += 1; + * } + * ``` + */ +predicate variableAddressTakenInExpression(ForStmt forLoop, VariableAccess baseVariableAccess) { + exists(AddressOfExpr addressOfExpr | + baseVariableAccess.getParent+() = forLoop.getAChild+() and + addressOfExpr.getParent+() = forLoop.getAChild+() and + addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() + ) and + not exists(Call call | variableAddressTakenAsConstArgument(forLoop, baseVariableAccess, call)) +} + +from ForStmt forLoop where - not isExcluded(x, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and -select + not isExcluded(forLoop, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and + /* 1. There is a counter variable that is not of an integer type. */ + exists(Type type | type = forLoop.getAnIterationVariable().getType() | + not ( + type instanceof IntegralType or + type instanceof FixedWidthIntegralType + ) + ) + or + /* + * 2. The loop condition checks termination without comparing the counter variable and the + * loop bound using a relational operator. + */ + + not forLoop.getCondition() instanceof LegacyForLoopCondition + or + /* 3. The loop counter is mutated somewhere other than its update expression. */ + exists(Expr mutatingExpr, Variable loopCounter | + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild() and + loopCounter = forLoop.getAnIterationVariable() + | + exprWithVarAccessMaybeImpure(mutatingExpr, loopCounter) + ) + or + /* 4. The type size of the loop counter is not greater or equal to that of the loop counter. */ + exists(LegacyForLoopCondition forLoopCondition | forLoopCondition = forLoop.getCondition() | + exists(Type loopCounterType, Type loopBoundType | + loopCounterType = forLoopCondition.getLoopCounter().getType() and + loopBoundType = forLoopCondition.getLoopBound().getType() + | + loopCounterType.getSize() < loopBoundType.getSize() + ) + ) + or + /* 5. The loop bound and the loop step is a variable that is mutated in the for loop. */ + exists(Expr mutatingExpr | + ( + /* 1. The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* 2. The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + ) + | + /* 5-1. The mutating expression mutates the loop bound. */ + exists(LegacyForLoopCondition forLoopCondition, Variable loopBoundVariable | + forLoopCondition = forLoop.getCondition() and + loopBoundVariable = forLoopCondition.getLoopBound().(VariableAccess).getTarget() + | + exprWithVarAccessMaybeImpure(mutatingExpr, loopBoundVariable) + ) + or + /* 5-2. The mutating expression mutates the loop step. */ + exists(VariableAccess loopStep | loopStep = getLoopStepOfForStmt(forLoop) | + exprWithVarAccessMaybeImpure(mutatingExpr, loopStep.getTarget()) + ) + ) + or + /* + * 6. Any of the loop counter, loop bound, or a loop step is taken as a mutable reference + * or its address to a mutable pointer. + */ + + /* 6-1. The loop counter is taken a mutable reference or its address to a mutable pointer. */ + variableAddressTakenInDeclaration(forLoop, + forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter()) + or + /* 6-2. The loop bound is taken a mutable reference or its address to a mutable pointer. */ + none() + or + /* 6-3. The loop step is taken a mutable reference or its address to a mutable pointer. */ + none() +select forLoop, "TODO" + +private module Notebook { + private predicate test(Function function) { + function.getParameter(_).getType().(PointerType).getBaseType().isConst() or + function.getParameter(_).getType().(ReferenceType).getBaseType().isConst() + } + + private predicate test2(Expr expr, string qlClasses) { + expr.getType().isConst() and + qlClasses = expr.getPrimaryQlClasses() + } + + private predicate test3(Function function, string qlClasses) { + qlClasses = function.getParameter(_).getType().getAQlClass() + } +} diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index f4c486ba04..08bdea3b61 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -1,5 +1,7 @@ -void f(int &x) {} // Function that takes a non-const integer reference -void g(int *x) {} // Function that takes a non-const integer pointer +void f1(int &x) {} // Function that takes a non-const integer reference +void g1(int *x) {} // Function that takes a non-const integer pointer +void f2(const int &x) {} // Function that takes a non-const integer reference +void g2(const int *x) {} // Function that takes a non-const integer pointer int main() { int j = 5; @@ -111,71 +113,141 @@ int main() { // step are not taken addresses of } - for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken // as a non-const reference int &m = i; } - for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken // as a non-const pointer int *m = &i; } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop bound is passed as a non-const reference + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + // as a const reference + const int &m = i; + } + + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + // as a const pointer + const int *m = &i; + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is taken as + // a non-const reference int &m = k; } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop bound is passed as a non-const pointer + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is taken as + // a non-const pointer int *m = &k; } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop step is passed as a non-const reference + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + // as a const reference + const int &m = k; + } + + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + // as a const pointer + const int *m = &k; + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is taken as + // a non-const reference int &m = l; } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is taken as + // a non-const pointer int *m = &l; } + for (int i = j; i < k; i += l) { // COMPLIANT: The loop step is taken as + // a const reference + const int &m = l; + } + + for (int i = j; i < k; i += l) { // COMPLIANT: The loop step is taken as + // a const pointer + const int *m = &l; + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed - // as a non-const reference - f(i); + // to a non-const reference parameter + f1(i); } for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed - // as a non-const pointer - g(&i); + // to a non-const pointer parameter + g1(&i); } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop bound is passed as a non-const reference - f(k); + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is passed + // to a const reference parameter + f2(i); } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop bound is passed as a non-const pointer - g(&k); + for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is passed + // to a const pointer parameter + g2(&i); } - for (int i = j; i < k; - i += - l) { // NON_COMPLIANT: The loop step is passed as a non-const reference - f(l); + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is passed to + // a non-const reference parameter + f1(k); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is passed to + // a non-const pointer parameter + g1(&k); + } + + for (int i = j; i < k; i += l) { // COMPLIANT: The loop bound is passed to a + // const reference parameter + f2(k); } for (int i = j; i < k; i += - l) { // NON_COMPLIANT: The loop step is passed as a non-const pointer - g(&l); + l) { // COMPLIANT: The loop bound is passed to a const pointer parameter + g2(&k); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to + // a non-const reference parameter + f1(l); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to + // a non-const pointer parameter + g1(&l); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to + // a non-const reference parameter + f2(l); + } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to + // a non-const pointer parameter + g2(&l); + } + + int n = 0; + + for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken + // as a non-const pointer + *(true ? &i : &n) += 1; + } + + for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken + // as a non-const pointer + *(true ? &k : &n) += 1; + } + + for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken + // as a non-const pointer + *(true ? &l : &n) += 1; } } \ No newline at end of file From 86aaa0ee4a52dff29c11875144269d6b8b699ad2 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 8 Sep 2025 15:59:53 -0400 Subject: [PATCH 12/47] Fix labeling of two cases --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 08bdea3b61..78f7f17d2f 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -224,13 +224,13 @@ int main() { g1(&l); } - for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to - // a non-const reference parameter + for (int i = j; i < k; i += l) { // COMPLIANT: The loop step is passed to + // a const reference parameter f2(l); } - for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to - // a non-const pointer parameter + for (int i = j; i < k; i += l) { // COMPLIANT: The loop step is passed to + // a const pointer parameter g2(&l); } From 49bdc0768521e643d6b740b81b862cefa1a078a5 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 8 Sep 2025 16:00:13 -0400 Subject: [PATCH 13/47] Add cases of addresses taken as part of non-const declaration or expressions --- .../LegacyForStatementsShouldBeSimple.ql | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 257e151580..fa42c56606 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -72,14 +72,14 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { * Holds if the given function has as parameter at a given index a pointer to a * constant value or a reference of a constant value. */ -predicate functionHasConstPointerOrReferenceParameter(Function function, int index) { +private predicate functionHasConstPointerOrReferenceParameter(Function function, int index) { function.getParameter(index).getType().(PointerType).getBaseType().isConst() or function.getParameter(index).getType().(ReferenceType).getBaseType().isConst() } /** * Holds if the the variable behind a given variable access is taken its address - * in a declaration in either the body of the for-loop or in its update expression. + * in a non-const variable declaration, in the body of the for-loop. * * e.g.1. The loop counter variable `i` in the body is taken its address in the * declaration of a pointer variable `m`. @@ -96,13 +96,21 @@ predicate functionHasConstPointerOrReferenceParameter(Function function, int ind * } * ``` */ -predicate variableAddressTakenInDeclaration(ForStmt forLoop, VariableAccess baseVariableAccess) { +predicate variableAddressTakenInNonConstDeclaration( + ForStmt forLoop, VariableAccess baseVariableAccess +) { exists(AddressOfExpr addressOfExpr, DeclStmt decl | decl.getParentStmt+() = forLoop and decl.getADeclarationEntry().(VariableDeclarationEntry).getVariable().getInitializer().getExpr() = addressOfExpr and - baseVariableAccess.getTarget() = - forLoop.getCondition().(LegacyForLoopCondition).getLoopBound().(VariableAccess).getTarget() + addressOfExpr.getOperand() = baseVariableAccess and + not decl.getADeclarationEntry() + .(VariableDeclarationEntry) + .getVariable() + .getType() + .(PointerType) + .getBaseType() + .isConst() ) } @@ -126,14 +134,15 @@ predicate variableAddressTakenInDeclaration(ForStmt forLoop, VariableAccess base * } * ``` */ -predicate variableAddressTakenAsConstArgument( +private predicate variableAddressTakenAsConstArgument( ForStmt forLoop, VariableAccess baseVariableAccess, Call call ) { exists(AddressOfExpr addressOfExpr, int index | - call.getParent+() = forLoop.getAChild+() and + call.getParent+() = forLoop.getAChild+() and // TODO: Bad call.getArgument(index).getAChild*() = addressOfExpr and functionHasConstPointerOrReferenceParameter(call.getTarget(), index) and - addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() + addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() and + baseVariableAccess.getParent+() = forLoop ) } @@ -152,11 +161,11 @@ predicate variableAddressTakenAsConstArgument( */ predicate variableAddressTakenInExpression(ForStmt forLoop, VariableAccess baseVariableAccess) { exists(AddressOfExpr addressOfExpr | - baseVariableAccess.getParent+() = forLoop.getAChild+() and + baseVariableAccess.getParent+() = forLoop.getAChild+() and // TODO: Bad addressOfExpr.getParent+() = forLoop.getAChild+() and addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() ) and - not exists(Call call | variableAddressTakenAsConstArgument(forLoop, baseVariableAccess, call)) + not variableAddressTakenAsConstArgument(forLoop, baseVariableAccess.getTarget().getAnAccess(), _) } from ForStmt forLoop @@ -225,8 +234,17 @@ where */ /* 6-1. The loop counter is taken a mutable reference or its address to a mutable pointer. */ - variableAddressTakenInDeclaration(forLoop, - forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter()) + exists(VariableAccess loopCounterAccessInCondition | + loopCounterAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() + | + exists(VariableAccess loopCounterAccessTakenAddress | + loopCounterAccessInCondition.getTarget() = loopCounterAccessTakenAddress.getTarget() + | + variableAddressTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddress) + or + variableAddressTakenInExpression(forLoop, loopCounterAccessTakenAddress) + ) + ) or /* 6-2. The loop bound is taken a mutable reference or its address to a mutable pointer. */ none() From 01e32760d6955ab5c9166903e753890ca02d370a Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 9 Sep 2025 14:55:31 -0400 Subject: [PATCH 14/47] Finish first draft --- .../LegacyForStatementsShouldBeSimple.ql | 206 ++++++++++++++---- 1 file changed, 165 insertions(+), 41 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index fa42c56606..ce14aac596 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -24,6 +24,9 @@ import codingstandards.cpp.misra * compared to a value, which is supposed to be the loop bound. */ class LegacyForLoopCondition extends RelationalOperation { + /** + * The legacy for-loop this relational operation is a condition of. + */ ForStmt forLoop; VariableAccess loopCounter; Expr loopBound; @@ -35,8 +38,14 @@ class LegacyForLoopCondition extends RelationalOperation { loopBound != loopCounter } + /** + * Gets the variable access to the loop counter variable, embedded in this loop condition. + */ VariableAccess getLoopCounter() { result = loopCounter } + /** + * Gets the variable access to the loop bound variable, embedded in this loop condition. + */ Expr getLoopBound() { result = loopBound } } @@ -69,17 +78,16 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { } /** - * Holds if the given function has as parameter at a given index a pointer to a - * constant value or a reference of a constant value. + * Holds if the given function has as parameter a pointer to a constant + * value, at a given index. */ -private predicate functionHasConstPointerOrReferenceParameter(Function function, int index) { - function.getParameter(index).getType().(PointerType).getBaseType().isConst() or - function.getParameter(index).getType().(ReferenceType).getBaseType().isConst() +private predicate functionHasConstPointerParameter(Function function, int index) { + function.getParameter(index).getType().(PointerType).getBaseType().isConst() } /** - * Holds if the the variable behind a given variable access is taken its address - * in a non-const variable declaration, in the body of the for-loop. + * Holds if the variable behind a given variable access is taken its address in + * a non-const variable declaration, in the body of the for-loop. * * e.g.1. The loop counter variable `i` in the body is taken its address in the * declaration of a pointer variable `m`. @@ -115,22 +123,26 @@ predicate variableAddressTakenInNonConstDeclaration( } /** - * Holds if the the variable behind a given variable access is taken its address + * Holds if the variable behind a given variable access is taken its address * as an argument of a call in either the body of the for-loop or in its update * expression. * - * e.g.1. The loop counter variable `i` in the body is taken its address in the - * declaration of a pointer variable `m`. + * e.g.1. The address of the loop counter variable `i` is passed as argument + * to the call to `g`. * ``` C++ + * void g1(int *x); + * * for (int i = 0; i < k; i += l) { - * g(&i); + * g1(&i); * } * ``` - * e.g.2. The loop bound variable `k` in the body is taken its address in the - * declaration of a pointer variable `m`. + * e.g.2. The address of the loop counter variable `k` is passed as argument + * to the call to `g`. * ``` C++ + * void g1(int *x); + * * for (int i = j; i < k; i += l) { - * g(&k); + * g1(&k); * } * ``` */ @@ -140,18 +152,30 @@ private predicate variableAddressTakenAsConstArgument( exists(AddressOfExpr addressOfExpr, int index | call.getParent+() = forLoop.getAChild+() and // TODO: Bad call.getArgument(index).getAChild*() = addressOfExpr and - functionHasConstPointerOrReferenceParameter(call.getTarget(), index) and + exists(PointerType parameterType | + parameterType = call.getTarget().getParameter(index).getType() and + not parameterType.getBaseType().isConst() + ) and addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() and baseVariableAccess.getParent+() = forLoop ) } /** - * Holds if the the variable behind a given variable access is taken its address + * Holds if the variable behind a given variable access is taken its address * as an argument of a complex expression in either the body of the for-loop or * in its update expression. * - * e.g. The loop counter variable `i` in the body and the loop bound variable `k` + * e.g.1. The loop counter variable `i` in the body and the loop bound variable `k` + * is taken its address in a call. + * ``` C++ + * void g1(int *x); + * + * for (int i = j; i < k; i += l) { + * g1(&i); + * } + * ``` + * e.g.2. The loop counter variable `i` in the body and the loop bound variable `k` * is taken its address in a compound expression. * ``` C++ * for (int i = 0; i < k; i += l) { @@ -159,13 +183,88 @@ private predicate variableAddressTakenAsConstArgument( * } * ``` */ +/* TODO: Do we need to use Expr.getUnderlyingType() to ensure that the expression is non-const? */ predicate variableAddressTakenInExpression(ForStmt forLoop, VariableAccess baseVariableAccess) { exists(AddressOfExpr addressOfExpr | baseVariableAccess.getParent+() = forLoop.getAChild+() and // TODO: Bad addressOfExpr.getParent+() = forLoop.getAChild+() and addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() - ) and - not variableAddressTakenAsConstArgument(forLoop, baseVariableAccess.getTarget().getAnAccess(), _) + ) +} + +/** + * Holds if the variable behind a given variable access is taken its reference + * in a non-const variable declaration, in the body of the for-loop. + * + * e.g.1. The loop counter variable `i` in the body is taken its reference in + * the declaration of a variable `m`. + * ``` C++ + * for (int i = j; i < k; i += l) { + * int &m = i; + * } + * ``` + * e.g.2. The loop bound variable `k` in the body is taken its reference in the + * declaration of a variable `m`. + * ``` C++ + * for (int i = j; i < k; i += l) { + * int &m = k; + * } + * ``` + */ +predicate variableReferenceTakenInNonConstDeclaration( + ForStmt forLoop, VariableAccess baseVariableAccess +) { + exists(DeclStmt decl, Variable definedVariable, ReferenceType definedVariableType | + decl.getParentStmt+() = forLoop and + not decl = forLoop.getInitialization() and // Exclude the for-loop counter initialization. + definedVariable = decl.getADeclarationEntry().(VariableDeclarationEntry).getVariable() and + definedVariable.getInitializer().getExpr() = baseVariableAccess and + definedVariableType = definedVariable.getType() and + not definedVariableType.getBaseType().isConst() + ) +} + +/** + * Holds if the variable behind a given variable access is taken its reference + * as an argument of a call in either the body of the for-loop or in its update + * expression. + * + * e.g.1. The loop counter variable `i` in the body is passed by reference to the + * call to `f1`. + * ``` C++ + * void f1(int &x); + * + * for (int i = j; i < k; i += l) { + * f1(i); + * } + * ``` + * e.g.2. The loop bound variable `k` in the body is passed by reference to the + * call to `f1`. + * ``` C++ + * void f1(int &x); + * + * for (int i = j; i < k; i += l) { + * f1(k); + * } + * ``` + */ +private predicate variableReferenceTakenAsNonConstArgument( + ForStmt forLoop, VariableAccess baseVariableAccess, Call call +) { + exists(int index | + call.getParent+() = forLoop.getAChild+() and + call.getArgument(index).getAChild*() = baseVariableAccess.getTarget().getAnAccess() and + /* + * The given function has as parameter a reference of a constant + * value, at a given index. + */ + + exists(ReferenceType parameterType | + parameterType = call.getTarget().getParameter(index).getType() and + not parameterType.getBaseType().isConst() + ) and + baseVariableAccess.getParent+() = forLoop + ) } from ForStmt forLoop @@ -237,34 +336,59 @@ where exists(VariableAccess loopCounterAccessInCondition | loopCounterAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() | - exists(VariableAccess loopCounterAccessTakenAddress | - loopCounterAccessInCondition.getTarget() = loopCounterAccessTakenAddress.getTarget() + exists(VariableAccess loopCounterAccessTakenAddressOrReference | + loopCounterAccessInCondition.getTarget() = + loopCounterAccessTakenAddressOrReference.getTarget() | - variableAddressTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddress) + variableAddressTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddressOrReference) + or + variableAddressTakenInExpression(forLoop, loopCounterAccessTakenAddressOrReference) and + not variableAddressTakenAsConstArgument(forLoop, + loopCounterAccessTakenAddressOrReference.getTarget().getAnAccess(), _) + or + variableReferenceTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddressOrReference) or - variableAddressTakenInExpression(forLoop, loopCounterAccessTakenAddress) + variableReferenceTakenAsNonConstArgument(forLoop, + loopCounterAccessTakenAddressOrReference.getTarget().getAnAccess(), _) ) ) or /* 6-2. The loop bound is taken a mutable reference or its address to a mutable pointer. */ - none() + exists(VariableAccess loopBoundAccessInCondition | + loopBoundAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() + | + exists(VariableAccess loopBoundAccessTakenAddressOrReference | + loopBoundAccessInCondition.getTarget() = loopBoundAccessTakenAddressOrReference.getTarget() + | + variableAddressTakenInNonConstDeclaration(forLoop, loopBoundAccessTakenAddressOrReference) + or + variableAddressTakenInExpression(forLoop, loopBoundAccessTakenAddressOrReference) and + not variableAddressTakenAsConstArgument(forLoop, + loopBoundAccessTakenAddressOrReference.getTarget().getAnAccess(), _) + or + variableReferenceTakenInNonConstDeclaration(forLoop, loopBoundAccessTakenAddressOrReference) + or + variableReferenceTakenAsNonConstArgument(forLoop, loopBoundAccessTakenAddressOrReference, _) + ) + ) or /* 6-3. The loop step is taken a mutable reference or its address to a mutable pointer. */ - none() + exists(VariableAccess loopStepAccessInCondition | + loopStepAccessInCondition = getLoopStepOfForStmt(forLoop) + | + exists(VariableAccess loopStepAccessTakenAddressOrReference | + loopStepAccessInCondition.getTarget() = loopStepAccessTakenAddressOrReference.getTarget() + | + variableAddressTakenInNonConstDeclaration(forLoop, loopStepAccessTakenAddressOrReference) + or + variableAddressTakenInExpression(forLoop, loopStepAccessTakenAddressOrReference) and + not variableAddressTakenAsConstArgument(forLoop, + loopStepAccessTakenAddressOrReference.getTarget().getAnAccess(), _) + or + variableReferenceTakenInNonConstDeclaration(forLoop, loopStepAccessTakenAddressOrReference) + or + variableReferenceTakenAsNonConstArgument(forLoop, + loopStepAccessTakenAddressOrReference.getTarget().getAnAccess(), _) + ) + ) select forLoop, "TODO" - -private module Notebook { - private predicate test(Function function) { - function.getParameter(_).getType().(PointerType).getBaseType().isConst() or - function.getParameter(_).getType().(ReferenceType).getBaseType().isConst() - } - - private predicate test2(Expr expr, string qlClasses) { - expr.getType().isConst() and - qlClasses = expr.getPrimaryQlClasses() - } - - private predicate test3(Function function, string qlClasses) { - qlClasses = function.getParameter(_).getType().getAQlClass() - } -} From 2f6fc3dd94e72794d3c6d55390ac43f315a75dbf Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 10 Sep 2025 17:49:11 -0400 Subject: [PATCH 15/47] Finish first draft --- .../LegacyForStatementsShouldBeSimple.ql | 123 +++++++++--------- 1 file changed, 65 insertions(+), 58 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index ce14aac596..dfc54bc7f6 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -17,6 +17,8 @@ import cpp import codingstandards.cpp.misra +import codingstandards.cpp.Call +import codingstandards.cpp.misra.BuiltInTypeRules::MisraCpp23BuiltInTypes /** * A comparison expression that has the minimum qualification as being a valid termination @@ -59,7 +61,7 @@ class LegacyForLoopCondition extends RelationalOperation { predicate exprWithVarAccessMaybeImpure(Expr expr, Variable variable) { exists(VariableAccess varAccess | expr.mayBeImpure() and - expr.getAChild*() = varAccess and + expr.getAChild*() = varAccess and // TODO: the `l` in the `i += l` is not mutated! variable = varAccess.getTarget() ) } @@ -267,6 +269,61 @@ private predicate variableReferenceTakenAsNonConstArgument( ) } +predicate loopVariableAssignedToPointerOrReferenceType( + ForStmt forLoop, VariableAccess loopVariableAccessInCondition +) { + exists(Expr assignmentRhs, DerivedType targetType | + assignmentRhs.getEnclosingStmt().getParent*() = forLoop.getStmt() and + ( + assignmentRhs.(AddressOfExpr).getOperand() = + loopVariableAccessInCondition.getTarget().getAnAccess() or + assignmentRhs = loopVariableAccessInCondition.getTarget().getAnAccess() + ) and + isAssignment(assignmentRhs, targetType, _) and + ( + targetType instanceof PointerType or + targetType instanceof ReferenceType + ) and + not targetType.getBaseType().isConst() + ) +} + +/* + * An adapted part of `BuiltinTypeRules::MisraCpp23BuiltInTypes::isPreConversionAssignment` + * that is only relevant to an argument passed to a parameter, seen as an assignment. + * + * This predicate adds two constraints to the target type, as compared to the original + * portion of the predicate: + * + * 1. This predicate adds type constraint that the target type is a `ReferenceType`. + * 2. This predicate adds the constraint that the target type is not `const`. + * + * Also, this predicate requires that the call is the body of the given for-loop. + */ + +predicate loopVariablePassedAsArgumentToReferenceParameter( + ForStmt forLoop, Expr loopVariableAccessInCondition +) { + exists(ReferenceType targetType | + exists(Call call, int i | + call.getArgument(i) = loopVariableAccessInCondition and + call.getEnclosingStmt().getParent*() = forLoop.getStmt() and + not targetType.getBaseType().isConst() + | + /* A regular function call */ + targetType = call.getTarget().getParameter(i).getType() + or + /* A function call where the argument is passed as varargs */ + call.getTarget().getNumberOfParameters() <= i and + /* The rule states that the type should match the "adjusted" type of the argument */ + targetType = loopVariableAccessInCondition.getFullyConverted().getType() + or + /* An expression call - get the function type, then the parameter type */ + targetType = getExprCallFunctionType(call).getParameterType(i) + ) + ) +} + from ForStmt forLoop where not isExcluded(forLoop, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and @@ -332,63 +389,13 @@ where * or its address to a mutable pointer. */ - /* 6-1. The loop counter is taken a mutable reference or its address to a mutable pointer. */ - exists(VariableAccess loopCounterAccessInCondition | - loopCounterAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() + exists(VariableAccess loopVariableAccessInCondition | + loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or + loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or + loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) | - exists(VariableAccess loopCounterAccessTakenAddressOrReference | - loopCounterAccessInCondition.getTarget() = - loopCounterAccessTakenAddressOrReference.getTarget() - | - variableAddressTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddressOrReference) - or - variableAddressTakenInExpression(forLoop, loopCounterAccessTakenAddressOrReference) and - not variableAddressTakenAsConstArgument(forLoop, - loopCounterAccessTakenAddressOrReference.getTarget().getAnAccess(), _) - or - variableReferenceTakenInNonConstDeclaration(forLoop, loopCounterAccessTakenAddressOrReference) - or - variableReferenceTakenAsNonConstArgument(forLoop, - loopCounterAccessTakenAddressOrReference.getTarget().getAnAccess(), _) - ) - ) - or - /* 6-2. The loop bound is taken a mutable reference or its address to a mutable pointer. */ - exists(VariableAccess loopBoundAccessInCondition | - loopBoundAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() - | - exists(VariableAccess loopBoundAccessTakenAddressOrReference | - loopBoundAccessInCondition.getTarget() = loopBoundAccessTakenAddressOrReference.getTarget() - | - variableAddressTakenInNonConstDeclaration(forLoop, loopBoundAccessTakenAddressOrReference) - or - variableAddressTakenInExpression(forLoop, loopBoundAccessTakenAddressOrReference) and - not variableAddressTakenAsConstArgument(forLoop, - loopBoundAccessTakenAddressOrReference.getTarget().getAnAccess(), _) - or - variableReferenceTakenInNonConstDeclaration(forLoop, loopBoundAccessTakenAddressOrReference) - or - variableReferenceTakenAsNonConstArgument(forLoop, loopBoundAccessTakenAddressOrReference, _) - ) - ) - or - /* 6-3. The loop step is taken a mutable reference or its address to a mutable pointer. */ - exists(VariableAccess loopStepAccessInCondition | - loopStepAccessInCondition = getLoopStepOfForStmt(forLoop) - | - exists(VariableAccess loopStepAccessTakenAddressOrReference | - loopStepAccessInCondition.getTarget() = loopStepAccessTakenAddressOrReference.getTarget() - | - variableAddressTakenInNonConstDeclaration(forLoop, loopStepAccessTakenAddressOrReference) - or - variableAddressTakenInExpression(forLoop, loopStepAccessTakenAddressOrReference) and - not variableAddressTakenAsConstArgument(forLoop, - loopStepAccessTakenAddressOrReference.getTarget().getAnAccess(), _) - or - variableReferenceTakenInNonConstDeclaration(forLoop, loopStepAccessTakenAddressOrReference) - or - variableReferenceTakenAsNonConstArgument(forLoop, - loopStepAccessTakenAddressOrReference.getTarget().getAnAccess(), _) - ) + loopVariableAssignedToPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToReferenceParameter(forLoop, loopVariableAccessInCondition) ) select forLoop, "TODO" From 999e87027bcb81ca2be126c03751336a91cafd41 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 17 Sep 2025 17:16:20 -0400 Subject: [PATCH 16/47] Tidy up, refine a bit more, add a series of test cases --- .../LegacyForStatementsShouldBeSimple.ql | 324 +++++------------- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 59 ++-- 2 files changed, 120 insertions(+), 263 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index dfc54bc7f6..3fd271c444 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -16,8 +16,10 @@ */ import cpp +import semmle.code.cpp.dataflow.internal.AddressFlow import codingstandards.cpp.misra import codingstandards.cpp.Call +import codingstandards.cpp.Loops import codingstandards.cpp.misra.BuiltInTypeRules::MisraCpp23BuiltInTypes /** @@ -36,7 +38,7 @@ class LegacyForLoopCondition extends RelationalOperation { LegacyForLoopCondition() { loopCounter = this.getAnOperand() and loopBound = this.getAnOperand() and - loopCounter.getTarget() = forLoop.getInitialization().(DeclStmt).getADeclaration() and + loopCounter.getTarget() = getAnIterationVariable(forLoop) and loopBound != loopCounter } @@ -52,18 +54,22 @@ class LegacyForLoopCondition extends RelationalOperation { } /** - * Holds if the given expression is impure and contains an access to the variable, and - * thus may mutate the variable. - * - * Note that this relation over-approximates and might include impure expressions that - * in fact do not mutate the variable. + * Holds if the given expression may mutate the variable. */ -predicate exprWithVarAccessMaybeImpure(Expr expr, Variable variable) { - exists(VariableAccess varAccess | - expr.mayBeImpure() and - expr.getAChild*() = varAccess and // TODO: the `l` in the `i += l` is not mutated! - variable = varAccess.getTarget() - ) +predicate variableModifiedInExpression(Expr expr, VariableAccess va) { + /* + * 1. Direct modification (assignment, increment, etc.) or a function call. + */ + + expr.getAChild+() = va and + va.isModified() + or + /* + * 2. Address taken for non-const access that can potentially lead to modification. + * This overlaps with the former example on cases where `expr` is a function call. + */ + + valueToUpdate(va, _, expr) } /** @@ -79,212 +85,26 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { result = forLoop.getUpdate().(AssignSubExpr).getRValue() } -/** - * Holds if the given function has as parameter a pointer to a constant - * value, at a given index. - */ -private predicate functionHasConstPointerParameter(Function function, int index) { - function.getParameter(index).getType().(PointerType).getBaseType().isConst() -} - -/** - * Holds if the variable behind a given variable access is taken its address in - * a non-const variable declaration, in the body of the for-loop. - * - * e.g.1. The loop counter variable `i` in the body is taken its address in the - * declaration of a pointer variable `m`. - * ``` C++ - * for (int i = 0; i < k; i += l) { - * int *m = &i; - * } - * ``` - * e.g.2. The loop bound variable `k` in the body is taken its address in the - * declaration of a pointer variable `m`. - * ``` C++ - * for (int i = j; i < k; i += l) { - * int *m = &k; - * } - * ``` - */ -predicate variableAddressTakenInNonConstDeclaration( - ForStmt forLoop, VariableAccess baseVariableAccess -) { - exists(AddressOfExpr addressOfExpr, DeclStmt decl | - decl.getParentStmt+() = forLoop and - decl.getADeclarationEntry().(VariableDeclarationEntry).getVariable().getInitializer().getExpr() = - addressOfExpr and - addressOfExpr.getOperand() = baseVariableAccess and - not decl.getADeclarationEntry() - .(VariableDeclarationEntry) - .getVariable() - .getType() - .(PointerType) - .getBaseType() - .isConst() - ) -} - -/** - * Holds if the variable behind a given variable access is taken its address - * as an argument of a call in either the body of the for-loop or in its update - * expression. - * - * e.g.1. The address of the loop counter variable `i` is passed as argument - * to the call to `g`. - * ``` C++ - * void g1(int *x); - * - * for (int i = 0; i < k; i += l) { - * g1(&i); - * } - * ``` - * e.g.2. The address of the loop counter variable `k` is passed as argument - * to the call to `g`. - * ``` C++ - * void g1(int *x); - * - * for (int i = j; i < k; i += l) { - * g1(&k); - * } - * ``` - */ -private predicate variableAddressTakenAsConstArgument( - ForStmt forLoop, VariableAccess baseVariableAccess, Call call -) { - exists(AddressOfExpr addressOfExpr, int index | - call.getParent+() = forLoop.getAChild+() and // TODO: Bad - call.getArgument(index).getAChild*() = addressOfExpr and - exists(PointerType parameterType | - parameterType = call.getTarget().getParameter(index).getType() and - not parameterType.getBaseType().isConst() - ) and - addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() and - baseVariableAccess.getParent+() = forLoop - ) -} - -/** - * Holds if the variable behind a given variable access is taken its address - * as an argument of a complex expression in either the body of the for-loop or - * in its update expression. - * - * e.g.1. The loop counter variable `i` in the body and the loop bound variable `k` - * is taken its address in a call. - * ``` C++ - * void g1(int *x); - * - * for (int i = j; i < k; i += l) { - * g1(&i); - * } - * ``` - * e.g.2. The loop counter variable `i` in the body and the loop bound variable `k` - * is taken its address in a compound expression. - * ``` C++ - * for (int i = 0; i < k; i += l) { - * *(cond ? &i : &k) += 1; - * } - * ``` - */ -/* TODO: Do we need to use Expr.getUnderlyingType() to ensure that the expression is non-const? */ -predicate variableAddressTakenInExpression(ForStmt forLoop, VariableAccess baseVariableAccess) { - exists(AddressOfExpr addressOfExpr | - baseVariableAccess.getParent+() = forLoop.getAChild+() and // TODO: Bad - addressOfExpr.getParent+() = forLoop.getAChild+() and - addressOfExpr.getOperand() = baseVariableAccess.getTarget().getAnAccess() - ) -} - -/** - * Holds if the variable behind a given variable access is taken its reference - * in a non-const variable declaration, in the body of the for-loop. - * - * e.g.1. The loop counter variable `i` in the body is taken its reference in - * the declaration of a variable `m`. - * ``` C++ - * for (int i = j; i < k; i += l) { - * int &m = i; - * } - * ``` - * e.g.2. The loop bound variable `k` in the body is taken its reference in the - * declaration of a variable `m`. - * ``` C++ - * for (int i = j; i < k; i += l) { - * int &m = k; - * } - * ``` - */ -predicate variableReferenceTakenInNonConstDeclaration( - ForStmt forLoop, VariableAccess baseVariableAccess -) { - exists(DeclStmt decl, Variable definedVariable, ReferenceType definedVariableType | - decl.getParentStmt+() = forLoop and - not decl = forLoop.getInitialization() and // Exclude the for-loop counter initialization. - definedVariable = decl.getADeclarationEntry().(VariableDeclarationEntry).getVariable() and - definedVariable.getInitializer().getExpr() = baseVariableAccess and - definedVariableType = definedVariable.getType() and - not definedVariableType.getBaseType().isConst() - ) -} - -/** - * Holds if the variable behind a given variable access is taken its reference - * as an argument of a call in either the body of the for-loop or in its update - * expression. - * - * e.g.1. The loop counter variable `i` in the body is passed by reference to the - * call to `f1`. - * ``` C++ - * void f1(int &x); - * - * for (int i = j; i < k; i += l) { - * f1(i); - * } - * ``` - * e.g.2. The loop bound variable `k` in the body is passed by reference to the - * call to `f1`. - * ``` C++ - * void f1(int &x); - * - * for (int i = j; i < k; i += l) { - * f1(k); - * } - * ``` - */ -private predicate variableReferenceTakenAsNonConstArgument( - ForStmt forLoop, VariableAccess baseVariableAccess, Call call -) { - exists(int index | - call.getParent+() = forLoop.getAChild+() and - call.getArgument(index).getAChild*() = baseVariableAccess.getTarget().getAnAccess() and - /* - * The given function has as parameter a reference of a constant - * value, at a given index. - */ - - exists(ReferenceType parameterType | - parameterType = call.getTarget().getParameter(index).getType() and - not parameterType.getBaseType().isConst() - ) and - baseVariableAccess.getParent+() = forLoop - ) -} - -predicate loopVariableAssignedToPointerOrReferenceType( +predicate loopVariableAssignedToNonConstPointerOrReferenceType( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { exists(Expr assignmentRhs, DerivedType targetType | - assignmentRhs.getEnclosingStmt().getParent*() = forLoop.getStmt() and - ( - assignmentRhs.(AddressOfExpr).getOperand() = - loopVariableAccessInCondition.getTarget().getAnAccess() or - assignmentRhs = loopVariableAccessInCondition.getTarget().getAnAccess() - ) and isAssignment(assignmentRhs, targetType, _) and + not targetType.getBaseType().isConst() and ( targetType instanceof PointerType or targetType instanceof ReferenceType - ) and - not targetType.getBaseType().isConst() + ) + | + assignmentRhs.getEnclosingStmt().getParent*() = forLoop.getStmt() and + ( + /* 1. The address is taken: A loop variable access */ + assignmentRhs.(AddressOfExpr).getOperand() = + loopVariableAccessInCondition.getTarget().getAnAccess() + or + /* 2. The address is taken: A loop variable access */ + assignmentRhs = loopVariableAccessInCondition.getTarget().getAnAccess() + ) ) } @@ -301,12 +121,12 @@ predicate loopVariableAssignedToPointerOrReferenceType( * Also, this predicate requires that the call is the body of the given for-loop. */ -predicate loopVariablePassedAsArgumentToReferenceParameter( - ForStmt forLoop, Expr loopVariableAccessInCondition +predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( + ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { exists(ReferenceType targetType | exists(Call call, int i | - call.getArgument(i) = loopVariableAccessInCondition and + call.getArgument(i) = loopVariableAccessInCondition.getTarget().getAnAccess() and call.getEnclosingStmt().getParent*() = forLoop.getStmt() and not targetType.getBaseType().isConst() | @@ -343,11 +163,8 @@ where not forLoop.getCondition() instanceof LegacyForLoopCondition or /* 3. The loop counter is mutated somewhere other than its update expression. */ - exists(Expr mutatingExpr, Variable loopCounter | - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild() and - loopCounter = forLoop.getAnIterationVariable() - | - exprWithVarAccessMaybeImpure(mutatingExpr, loopCounter) + exists(Variable loopCounter | + isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) ) or /* 4. The type size of the loop counter is not greater or equal to that of the loop counter. */ @@ -360,27 +177,45 @@ where ) ) or - /* 5. The loop bound and the loop step is a variable that is mutated in the for loop. */ - exists(Expr mutatingExpr | - ( - /* 1. The mutating expression may be in the loop body. */ + /* + * 5. The loop bound and the loop step are non-const expressions, or are variables that are + * mutated in the for loop. + */ + + /* 5-1. The mutating expression mutates the loop bound. */ + exists(Expr loopBound | + loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() + | + exists(Expr mutatingExpr | + /* The mutating expression may be in the loop body. */ mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() or - /* 2. The mutating expression may be in the loop updating expression. */ + /* The mutating expression may be in the loop updating expression. */ mutatingExpr = forLoop.getUpdate().getAChild*() - ) - | - /* 5-1. The mutating expression mutates the loop bound. */ - exists(LegacyForLoopCondition forLoopCondition, Variable loopBoundVariable | - forLoopCondition = forLoop.getCondition() and - loopBoundVariable = forLoopCondition.getLoopBound().(VariableAccess).getTarget() | - exprWithVarAccessMaybeImpure(mutatingExpr, loopBoundVariable) + /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, + loopBound.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ + not loopBound instanceof VariableAccess and not loopBound.isConstant() ) - or - /* 5-2. The mutating expression mutates the loop step. */ - exists(VariableAccess loopStep | loopStep = getLoopStepOfForStmt(forLoop) | - exprWithVarAccessMaybeImpure(mutatingExpr, loopStep.getTarget()) + ) + or + /* 5-2. The mutating expression mutates the loop step. */ + exists(Expr loopStep | loopStep = getLoopStepOfForStmt(forLoop) | + exists(Expr mutatingExpr | + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + | + /* 5-1-2. The loop step is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ + not loopStep instanceof VariableAccess and not loopStep.isConstant() ) ) or @@ -390,12 +225,17 @@ where */ exists(VariableAccess loopVariableAccessInCondition | - loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or - loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or - loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) - | - loopVariableAssignedToPointerOrReferenceType(forLoop, loopVariableAccessInCondition) - or - loopVariablePassedAsArgumentToReferenceParameter(forLoop, loopVariableAccessInCondition) + ( + loopVariableAccessInCondition = + forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or + loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or + loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) + ) and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, + loopVariableAccessInCondition) + ) ) select forLoop, "TODO" diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 78f7f17d2f..0d8cfa35fb 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -1,3 +1,6 @@ +#include +#include + void f1(int &x) {} // Function that takes a non-const integer reference void g1(int *x) {} // Function that takes a non-const integer pointer void f2(const int &x) {} // Function that takes a non-const integer reference @@ -5,6 +8,8 @@ void g2(const int *x) {} // Function that takes a non-const integer pointer int main() { int j = 5; + int k = 10; + int l = 2; /* ========== 1. Type of the initialized counter variable ========== */ @@ -62,7 +67,7 @@ int main() { } for (int i = 0; i < j; - i++) { // NON_COMPLIANT: The loop bound is not a constant + i++) { // COMPLIANT: The loop step and the loop bound has the same type } /* ========== 5. Immutability of the loop bound and the loop step ========== @@ -103,12 +108,41 @@ int main() { j++; } + int n = 0; + + for (int i = 0; i < k; + i += l) { // NON_COMPLIANT: The loop bound is mutated through an address + *(true ? &k : &n) += 1; + } + + for (int i = 0; i < k; + i += l) { // NON_COMPLIANT: The loop step is mutated through an address + *(true ? &l : &n) += 1; + } + + std::string hello1 = "hello"; + std::string_view hello2{"hello"}; + + for (int i = 0; i < hello1.size(); + i++) { // NON_COMPLIANT: The loop bound is not a constant expression + } + + for (int i = 0; i < hello2.size(); + i++) { // COMPLIANT: The loop bound is a constant expression + } + + for (int i = 0; i < j; i += hello1.size()) { // NON_COMPLIANT: The loop step + // is not a constant expression + } + + for (int i = 0; i < j; + i += + hello2.size()) { // COMPLIANT: The loop step is a constant expression + } + /* ========== 6. Existence of pointers to the loop counter, loop bound, and * loop step ========== */ - int k = 10; - int l = 2; - for (int i = 0; i < k; i += l) { // COMPLIANT: The loop counter, bound, and // step are not taken addresses of } @@ -233,21 +267,4 @@ int main() { // a const pointer parameter g2(&l); } - - int n = 0; - - for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken - // as a non-const pointer - *(true ? &i : &n) += 1; - } - - for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken - // as a non-const pointer - *(true ? &k : &n) += 1; - } - - for (int i = 0; i < k; i += l) { // NON_COMPLIANT: The loop counter is taken - // as a non-const pointer - *(true ? &l : &n) += 1; - } } \ No newline at end of file From 562c7bea40d922e03a1326b5b938ed321ee0b4f9 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 17 Sep 2025 18:14:39 -0400 Subject: [PATCH 17/47] Add two more cases --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 0d8cfa35fb..718bc4d068 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -6,6 +6,9 @@ void g1(int *x) {} // Function that takes a non-const integer pointer void f2(const int &x) {} // Function that takes a non-const integer reference void g2(const int *x) {} // Function that takes a non-const integer pointer +int h1() { return 1; } +constexpr int h2() { return 1; } + int main() { int j = 5; int k = 10; @@ -120,24 +123,20 @@ int main() { *(true ? &l : &n) += 1; } - std::string hello1 = "hello"; - std::string_view hello2{"hello"}; - - for (int i = 0; i < hello1.size(); + for (int i = 0; i < h1(); i++) { // NON_COMPLIANT: The loop bound is not a constant expression } - for (int i = 0; i < hello2.size(); + for (int i = 0; i < h2(); i++) { // COMPLIANT: The loop bound is a constant expression } - for (int i = 0; i < j; i += hello1.size()) { // NON_COMPLIANT: The loop step - // is not a constant expression + for (int i = 0; i < j; + i += h1()) { // NON_COMPLIANT: The loop step is not a constant expression } for (int i = 0; i < j; - i += - hello2.size()) { // COMPLIANT: The loop step is a constant expression + i += h2()) { // COMPLIANT: The loop step is a constant expression } /* ========== 6. Existence of pointers to the loop counter, loop bound, and From 6855e6a2df445c7e14fc3e2cd24984c3aa16bdf6 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 17 Sep 2025 18:14:50 -0400 Subject: [PATCH 18/47] Add QLDocs to two helper predicates --- .../LegacyForStatementsShouldBeSimple.ql | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 3fd271c444..00f6f12f64 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -85,6 +85,21 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { result = forLoop.getUpdate().(AssignSubExpr).getRValue() } +/** + * Holds if either of the following holds for the given variable access: + * 1. Another variable access of the same variable as the given variable access is taken an + * address and is assigned to a non-const pointer variable, i.e. initialization, assignment, + * and pass-by-value. + * 2. Another variable access of the same variable as the given variable access is assigned + * to a non-const reference variable (thus constituting a `T` -> `&T` conversion.), i.e. + * initialization and assignment. + */ +/* + * Note that pass-by-reference is dealt with in a different predicate named + * `loopVariablePassedAsArgumentToNonConstReferenceParameter`, due to implementation + * limitations. + */ + predicate loopVariableAssignedToNonConstPointerOrReferenceType( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { @@ -121,6 +136,11 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( * Also, this predicate requires that the call is the body of the given for-loop. */ +/** + * Holds if the given variable access has another variable access with the same target + * variable that is passed as reference to a non-const reference parameter of a function, + * constituting a `T` -> `&T` conversion. + */ predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { From 5e24d1bd07fd0e535879ecc336af667993345488 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 22 Sep 2025 11:30:53 -0400 Subject: [PATCH 19/47] Introduce `from` variables and fix logical operator association --- .../LegacyForStatementsShouldBeSimple.ql | 191 ++++++++++-------- 1 file changed, 104 insertions(+), 87 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 00f6f12f64..5ed79e2747 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -164,98 +164,115 @@ predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( ) } -from ForStmt forLoop +from ForStmt forLoop, Locatable forLoopExpr, string message where not isExcluded(forLoop, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and - /* 1. There is a counter variable that is not of an integer type. */ - exists(Type type | type = forLoop.getAnIterationVariable().getType() | - not ( - type instanceof IntegralType or - type instanceof FixedWidthIntegralType - ) - ) - or - /* - * 2. The loop condition checks termination without comparing the counter variable and the - * loop bound using a relational operator. - */ + ( + /* 1. There is a counter variable that is not of an integer type. */ + exists(Type type | type = forLoop.getAnIterationVariable().getType() | + not ( + type instanceof IntegralType or + type instanceof FixedWidthIntegralType + ) + ) and + forLoopExpr = forLoop.getAnIterationVariable() and + message = "The counter variable is not of an integer type." + or + /* + * 2. The loop condition checks termination without comparing the counter variable to the + * loop bound using a relational operator. + */ - not forLoop.getCondition() instanceof LegacyForLoopCondition - or - /* 3. The loop counter is mutated somewhere other than its update expression. */ - exists(Variable loopCounter | - isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) - ) - or - /* 4. The type size of the loop counter is not greater or equal to that of the loop counter. */ - exists(LegacyForLoopCondition forLoopCondition | forLoopCondition = forLoop.getCondition() | - exists(Type loopCounterType, Type loopBoundType | - loopCounterType = forLoopCondition.getLoopCounter().getType() and - loopBoundType = forLoopCondition.getLoopBound().getType() - | - loopCounterType.getSize() < loopBoundType.getSize() - ) - ) - or - /* - * 5. The loop bound and the loop step are non-const expressions, or are variables that are - * mutated in the for loop. - */ + not forLoop.getCondition() instanceof LegacyForLoopCondition and + forLoopExpr = forLoop.getCondition() and + message = "TODO" + or + /* 3. The loop counter is mutated somewhere other than its update expression. */ + exists(Variable loopCounter | + isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) + ) and + forLoopExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() and + message = "TODO" + or + /* 4. The type size of the loop counter is not greater or equal to that of the loop counter. */ + exists(LegacyForLoopCondition forLoopCondition | forLoopCondition = forLoop.getCondition() | + exists(Type loopCounterType, Type loopBoundType | + loopCounterType = forLoopCondition.getLoopCounter().getType() and + loopBoundType = forLoopCondition.getLoopBound().getType() + | + loopCounterType.getSize() < loopBoundType.getSize() + ) + ) and + forLoopExpr = forLoop.getCondition() and + message = "TODO" + or + /* + * 5. The loop bound and the loop step are non-const expressions, or are variables that are + * mutated in the for loop. + */ - /* 5-1. The mutating expression mutates the loop bound. */ - exists(Expr loopBound | - loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() - | - exists(Expr mutatingExpr | - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() + /* 5-1. The mutating expression mutates the loop bound. */ + exists(Expr loopBound | + loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() | - /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ - variableModifiedInExpression(mutatingExpr, - loopBound.(VariableAccess).getTarget().getAnAccess()) - or - /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ - not loopBound instanceof VariableAccess and not loopBound.isConstant() - ) - ) - or - /* 5-2. The mutating expression mutates the loop step. */ - exists(Expr loopStep | loopStep = getLoopStepOfForStmt(forLoop) | - exists(Expr mutatingExpr | - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - | - /* 5-1-2. The loop step is a variable that is mutated in the for loop. */ - variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) - or - /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ - not loopStep instanceof VariableAccess and not loopStep.isConstant() - ) - ) - or - /* - * 6. Any of the loop counter, loop bound, or a loop step is taken as a mutable reference - * or its address to a mutable pointer. - */ + exists(Expr mutatingExpr | + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + | + /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, + loopBound.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ + not loopBound instanceof VariableAccess and not loopBound.isConstant() + ) + ) and + forLoopExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and + message = "TODO" + or + /* 5-2. The mutating expression mutates the loop step. */ + exists(Expr loopStep | loopStep = getLoopStepOfForStmt(forLoop) | + exists(Expr mutatingExpr | + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + | + /* 5-1-2. The loop step is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, + loopStep.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ + not loopStep instanceof VariableAccess and not loopStep.isConstant() + ) + ) and + forLoopExpr = getLoopStepOfForStmt(forLoop) and + message = "TODO" + or + /* + * 6. Any of the loop counter, loop bound, or a loop step is taken as a mutable reference + * or its address to a mutable pointer. + */ - exists(VariableAccess loopVariableAccessInCondition | - ( - loopVariableAccessInCondition = - forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or - loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or - loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) + exists(VariableAccess loopVariableAccessInCondition | + ( + loopVariableAccessInCondition = + forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or + loopVariableAccessInCondition = + forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or + loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) + ) and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, + loopVariableAccessInCondition) + ) ) and - ( - loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) - or - loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, - loopVariableAccessInCondition) - ) + message = "TODO" ) -select forLoop, "TODO" +select forLoop, message, forLoopExpr, "???" From 18daff7c1e2d5c58414b75f33cade74b84d2139e Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 22 Sep 2025 16:17:36 -0400 Subject: [PATCH 20/47] Introduce newtype --- .../LegacyForStatementsShouldBeSimple.ql | 374 +++++++++++++----- 1 file changed, 274 insertions(+), 100 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 5ed79e2747..a78e6e7d43 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -164,115 +164,289 @@ predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( ) } -from ForStmt forLoop, Locatable forLoopExpr, string message -where - not isExcluded(forLoop, StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) and - ( - /* 1. There is a counter variable that is not of an integer type. */ - exists(Type type | type = forLoop.getAnIterationVariable().getType() | +private newtype TAlertType = + /* 1. There is a counter variable that is not of an integer type. */ + TNonIntegerTypeCounterVariable(ForStmt forLoop, Variable iterationVariable) { + iterationVariable = forLoop.getAnIterationVariable() and + exists(Type type | type = iterationVariable.getType() | not ( type instanceof IntegralType or type instanceof FixedWidthIntegralType ) + ) + } or + /* + * 2. The loop condition checks termination without comparing the counter variable to the + * loop bound using a relational operator. + */ + + TNoRelationalOperatorInLoopCondition(ForStmt forLoop, Expr condition) { + condition = forLoop.getCondition() and + not condition instanceof LegacyForLoopCondition + } or + /* 3. The loop counter is mutated somewhere other than its update expression. */ + TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounter) { + isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) + } or + /* 4. The type size of the loop counter is smaller than that of the loop bound. */ + TLoopCounterSmallerThanLoopBound(ForStmt forLoop, LegacyForLoopCondition forLoopCondition) { + forLoopCondition = forLoop.getCondition() and + exists(Type loopCounterType, Type loopBoundType | + loopCounterType = forLoopCondition.getLoopCounter().getType() and + loopBoundType = forLoopCondition.getLoopBound().getType() + | + loopCounterType.getSize() < loopBoundType.getSize() + ) + } or + /* + * 5-1. The loop bound is a non-const expression, or a variable that is mutated in the for loop. + */ + + TLoopBoundIsNonConstExprOrMutatedVariableAccess(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { + loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and + ( + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() ) and - forLoopExpr = forLoop.getAnIterationVariable() and - message = "The counter variable is not of an integer type." + /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ + ( + variableModifiedInExpression(mutatingExpr, + loopBound.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-1-2. The loop bound is not a variable access nor a constant expression. */ + not loopBound instanceof VariableAccess and not loopBound.isConstant() + ) + } or + /* + * 5-2. The loop step is a non-const expression, or are variable that is mutated in the for loop. + */ + + TLoopStepIsNonConstExprOrMutatedVariableAccess(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { + loopStep = getLoopStepOfForStmt(forLoop) and + ( + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + ) and + ( + /* 5-2-2. The loop step is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) + or + /* 5-2-2. The loop step is not a variable access nor a constant expression. */ + not loopStep instanceof VariableAccess and not loopStep.isConstant() + ) + } or + /* + * 6-1. The loop counter is taken as a mutable reference or its address to a mutable pointer. + */ + + TLoopCounterIsTakenNonConstAddress(ForStmt forLoop, VariableAccess loopVariableAccessInCondition) { + loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, + loopVariableAccessInCondition) + ) + } or + /* + * 6-2. The loop bound is taken as a mutable reference or its address to a mutable pointer. + */ + + TLoopBoundIsTakenNonConstAddress(ForStmt forLoop, Expr loopVariableAccessInCondition) { + loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, + loopVariableAccessInCondition) + ) + } or + /* + * 6-3. The loop step is taken as a mutable reference or its address to a mutable pointer. + */ + + TLoopStepIsTakenNonConstAddress(ForStmt forLoop, Expr loopVariableAccessInCondition) { + loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, + loopVariableAccessInCondition) + ) + } + +class AlertType extends TAlertType { + /** + * Extract the primary location depending on the case of this instance. + */ + Location getLocation() { result = this.asElement().getLocation() } + + Element asElement() { + this = TNonIntegerTypeCounterVariable(result, _) or + this = TNoRelationalOperatorInLoopCondition(result, _) or + this = TLoopCounterMutatedInLoopBody(result, _) or + this = TLoopCounterSmallerThanLoopBound(result, _) or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(result, _, _) or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(result, _, _) or + this = TLoopCounterIsTakenNonConstAddress(result, _) or + this = TLoopBoundIsTakenNonConstAddress(result, _) or + this = TLoopStepIsTakenNonConstAddress(result, _) + } + + /** + * Gets the target the link leads to depending on the case of this instance. + */ + Locatable getLinkTarget1() { + this = TNonIntegerTypeCounterVariable(_, result) + or + this = TNoRelationalOperatorInLoopCondition(_, result) + or + this = TLoopCounterMutatedInLoopBody(_, result) + or + exists(LegacyForLoopCondition forLoopCondition | + this = TLoopCounterSmallerThanLoopBound(_, forLoopCondition) and + result = forLoopCondition.getLoopCounter() + ) + or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, result, _) + or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, result, _) + or + this = TLoopCounterIsTakenNonConstAddress(_, result) or - /* - * 2. The loop condition checks termination without comparing the counter variable to the - * loop bound using a relational operator. - */ + this = TLoopBoundIsTakenNonConstAddress(_, result) + or + this = TLoopStepIsTakenNonConstAddress(_, result) + } - not forLoop.getCondition() instanceof LegacyForLoopCondition and - forLoopExpr = forLoop.getCondition() and - message = "TODO" + /** + * Gets the text of the link depending on the case of this instance. + */ + string getLinkText1() { + this = TNonIntegerTypeCounterVariable(_, _) and + result = "counter variable" or - /* 3. The loop counter is mutated somewhere other than its update expression. */ - exists(Variable loopCounter | - isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) - ) and - forLoopExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() and - message = "TODO" - or - /* 4. The type size of the loop counter is not greater or equal to that of the loop counter. */ - exists(LegacyForLoopCondition forLoopCondition | forLoopCondition = forLoop.getCondition() | - exists(Type loopCounterType, Type loopBoundType | - loopCounterType = forLoopCondition.getLoopCounter().getType() and - loopBoundType = forLoopCondition.getLoopBound().getType() - | - loopCounterType.getSize() < loopBoundType.getSize() - ) - ) and - forLoopExpr = forLoop.getCondition() and - message = "TODO" + this = TNoRelationalOperatorInLoopCondition(_, _) and + result = "loop condition" or - /* - * 5. The loop bound and the loop step are non-const expressions, or are variables that are - * mutated in the for loop. - */ + this = TLoopCounterMutatedInLoopBody(_, _) and + result = "counter variable" + or + this = TLoopCounterSmallerThanLoopBound(_, _) and + result = "counter variable" + or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "loop bound" + or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "loop step" + or + this = TLoopCounterIsTakenNonConstAddress(_, _) and + result = "loop counter" + or + this = TLoopBoundIsTakenNonConstAddress(_, _) and + result = "loop bound" + or + this = TLoopStepIsTakenNonConstAddress(_, _) and + result = "loop step" + } - /* 5-1. The mutating expression mutates the loop bound. */ - exists(Expr loopBound | - loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() - | - exists(Expr mutatingExpr | - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - | - /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ - variableModifiedInExpression(mutatingExpr, - loopBound.(VariableAccess).getTarget().getAnAccess()) - or - /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ - not loopBound instanceof VariableAccess and not loopBound.isConstant() - ) - ) and - forLoopExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and - message = "TODO" - or - /* 5-2. The mutating expression mutates the loop step. */ - exists(Expr loopStep | loopStep = getLoopStepOfForStmt(forLoop) | - exists(Expr mutatingExpr | - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - | - /* 5-1-2. The loop step is a variable that is mutated in the for loop. */ - variableModifiedInExpression(mutatingExpr, - loopStep.(VariableAccess).getTarget().getAnAccess()) - or - /* 5-1-2. The loop bound is not a variable access and is not a constant expression. */ - not loopStep instanceof VariableAccess and not loopStep.isConstant() - ) - ) and - forLoopExpr = getLoopStepOfForStmt(forLoop) and - message = "TODO" + /** + * Gets the message with a placeholder, depending on the case of this instance. + */ + string getMessage() { + this = TNonIntegerTypeCounterVariable(_, _) and + result = "The $@ is not of an integer type." // Throwaway placeholder + or + this = TNoRelationalOperatorInLoopCondition(_, _) and + result = + "The $@ does not compare the counter variable to an expression using a relational operator." // Throwaway placeholder + or + this = TLoopCounterMutatedInLoopBody(_, _) and + result = "The $@ may be mutated in a location other than its update expression." + or + this = TLoopCounterSmallerThanLoopBound(_, _) and + result = "The $@ has a smaller type than that of the $@." + or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + or + this = TLoopCounterIsTakenNonConstAddress(_, _) and + result = "The $@ is taken as a mutable reference or its address to a mutable pointer." + or + this = TLoopBoundIsTakenNonConstAddress(_, _) and + result = "The $@ is taken as a mutable reference or its address to a mutable pointer." or - /* - * 6. Any of the loop counter, loop bound, or a loop step is taken as a mutable reference - * or its address to a mutable pointer. - */ + this = TLoopStepIsTakenNonConstAddress(_, _) and + result = "The $@ is taken as a mutable reference or its address to a mutable pointer." + } + + Locatable getLinkTarget2() { + this = TNonIntegerTypeCounterVariable(_, result) // Throwaway + or + this = TNoRelationalOperatorInLoopCondition(_, result) // Throwaway + or + this = TLoopCounterMutatedInLoopBody(_, _) // Throwaway + or + exists(LegacyForLoopCondition forLoopCondition | + this = TLoopCounterSmallerThanLoopBound(_, forLoopCondition) and + result = forLoopCondition.getLoopBound() + ) + or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, result) + or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, result) + or + this = TLoopCounterIsTakenNonConstAddress(_, result) // Throwaway + or + this = TLoopBoundIsTakenNonConstAddress(_, result) // Throwaway + or + this = TLoopStepIsTakenNonConstAddress(_, result) // Throwaway + } + + string getLinkText2() { + this = TNonIntegerTypeCounterVariable(_, _) and + result = "N/A" // Throwaway + or + this = TNoRelationalOperatorInLoopCondition(_, _) and + result = "N/A" // Throwaway + or + this = TLoopCounterMutatedInLoopBody(_, _) and + result = "N/A" // Throwaway + or + this = TLoopCounterSmallerThanLoopBound(_, _) and + result = "loop bound" + or + this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "mutated" + or + this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + result = "mutated" + or + this = TLoopCounterIsTakenNonConstAddress(_, _) and + result = "N/A" // Throwaway + or + this = TLoopBoundIsTakenNonConstAddress(_, _) and + result = "N/A" // Throwaway + or + this = TLoopStepIsTakenNonConstAddress(_, _) and + result = "N/A" // Throwaway + } + + string toString() { result = this.asElement().toString() } +} + +from AlertType alert +where not isExcluded(alert.asElement(), StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) +select alert, alert.getMessage(), alert.getLinkTarget1(), alert.getLinkText1(), + alert.getLinkTarget2(), alert.getLinkText2() - exists(VariableAccess loopVariableAccessInCondition | - ( - loopVariableAccessInCondition = - forLoop.getCondition().(LegacyForLoopCondition).getLoopCounter() or - loopVariableAccessInCondition = - forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() or - loopVariableAccessInCondition = getLoopStepOfForStmt(forLoop) - ) and - ( - loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) - or - loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, - loopVariableAccessInCondition) - ) - ) and - message = "TODO" - ) -select forLoop, message, forLoopExpr, "???" From 55b847637bf0411c295aa3200e5467de09e178fc Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 22 Sep 2025 17:26:43 -0400 Subject: [PATCH 21/47] Split cases `5-1` and `5-2` --- .../LegacyForStatementsShouldBeSimple.ql | 100 ++++++++++++------ 1 file changed, 66 insertions(+), 34 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index a78e6e7d43..a22c15ed45 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -198,11 +198,8 @@ private newtype TAlertType = loopCounterType.getSize() < loopBoundType.getSize() ) } or - /* - * 5-1. The loop bound is a non-const expression, or a variable that is mutated in the for loop. - */ - - TLoopBoundIsNonConstExprOrMutatedVariableAccess(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { + /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ + TLoopBoundIsMutatedVariableAccess(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and ( /* The mutating expression may be in the loop body. */ @@ -211,20 +208,22 @@ private newtype TAlertType = /* The mutating expression may be in the loop updating expression. */ mutatingExpr = forLoop.getUpdate().getAChild*() ) and - /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ + variableModifiedInExpression(mutatingExpr, loopBound.(VariableAccess).getTarget().getAnAccess()) + } or + /* 5-1-2. The loop bound is not a variable access nor a constant expression. */ + TLoopBoundIsNonConstExpr(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { + loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and ( - variableModifiedInExpression(mutatingExpr, - loopBound.(VariableAccess).getTarget().getAnAccess()) + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() or - /* 5-1-2. The loop bound is not a variable access nor a constant expression. */ - not loopBound instanceof VariableAccess and not loopBound.isConstant() - ) + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + ) and + (not loopBound instanceof VariableAccess and not loopBound.isConstant()) } or - /* - * 5-2. The loop step is a non-const expression, or are variable that is mutated in the for loop. - */ - - TLoopStepIsNonConstExprOrMutatedVariableAccess(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { + /* 5-2-1. The loop step is a variable that is mutated in the for loop. */ + TLoopStepIsMutatedVariableAccess(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { loopStep = getLoopStepOfForStmt(forLoop) and ( /* The mutating expression may be in the loop body. */ @@ -233,13 +232,19 @@ private newtype TAlertType = /* The mutating expression may be in the loop updating expression. */ mutatingExpr = forLoop.getUpdate().getAChild*() ) and + variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) + } or + /* 5-2-2. The loop step is not a variable access nor a constant expression. */ + TLoopStepIsNonConstExpr(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { + loopStep = getLoopStepOfForStmt(forLoop) and ( - /* 5-2-2. The loop step is a variable that is mutated in the for loop. */ - variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) + /* The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() or - /* 5-2-2. The loop step is not a variable access nor a constant expression. */ - not loopStep instanceof VariableAccess and not loopStep.isConstant() - ) + /* The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + ) and + (not loopStep instanceof VariableAccess and not loopStep.isConstant()) } or /* * 6-1. The loop counter is taken as a mutable reference or its address to a mutable pointer. @@ -292,8 +297,10 @@ class AlertType extends TAlertType { this = TNoRelationalOperatorInLoopCondition(result, _) or this = TLoopCounterMutatedInLoopBody(result, _) or this = TLoopCounterSmallerThanLoopBound(result, _) or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(result, _, _) or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(result, _, _) or + this = TLoopBoundIsMutatedVariableAccess(result, _, _) or + this = TLoopStepIsNonConstExpr(result, _, _) or + this = TLoopBoundIsMutatedVariableAccess(result, _, _) or + this = TLoopStepIsNonConstExpr(result, _, _) or this = TLoopCounterIsTakenNonConstAddress(result, _) or this = TLoopBoundIsTakenNonConstAddress(result, _) or this = TLoopStepIsTakenNonConstAddress(result, _) @@ -314,9 +321,13 @@ class AlertType extends TAlertType { result = forLoopCondition.getLoopCounter() ) or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, result, _) + this = TLoopBoundIsNonConstExpr(_, result, _) + or + this = TLoopBoundIsMutatedVariableAccess(_, result, _) or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, result, _) + this = TLoopStepIsNonConstExpr(_, result, _) + or + this = TLoopStepIsMutatedVariableAccess(_, result, _) or this = TLoopCounterIsTakenNonConstAddress(_, result) or @@ -341,10 +352,16 @@ class AlertType extends TAlertType { this = TLoopCounterSmallerThanLoopBound(_, _) and result = "counter variable" or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopBoundIsMutatedVariableAccess(_, _, _) and + result = "loop bound" + or + this = TLoopBoundIsNonConstExpr(_, _, _) and result = "loop bound" or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopStepIsMutatedVariableAccess(_, _, _) and + result = "loop step" + or + this = TLoopStepIsNonConstExpr(_, _, _) and result = "loop step" or this = TLoopCounterIsTakenNonConstAddress(_, _) and @@ -374,10 +391,16 @@ class AlertType extends TAlertType { this = TLoopCounterSmallerThanLoopBound(_, _) and result = "The $@ has a smaller type than that of the $@." or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopBoundIsNonConstExpr(_, _, _) and + result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + or + this = TLoopBoundIsMutatedVariableAccess(_, _, _) and result = "The $@ is a non-const expression, or a variable that is $@ in the loop." or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopStepIsNonConstExpr(_, _, _) and + result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + or + this = TLoopStepIsMutatedVariableAccess(_, _, _) and result = "The $@ is a non-const expression, or a variable that is $@ in the loop." or this = TLoopCounterIsTakenNonConstAddress(_, _) and @@ -402,9 +425,13 @@ class AlertType extends TAlertType { result = forLoopCondition.getLoopBound() ) or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, result) + this = TLoopBoundIsNonConstExpr(_, _, result) + or + this = TLoopBoundIsMutatedVariableAccess(_, _, result) or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, result) + this = TLoopStepIsNonConstExpr(_, _, result) + or + this = TLoopStepIsMutatedVariableAccess(_, _, result) or this = TLoopCounterIsTakenNonConstAddress(_, result) // Throwaway or @@ -426,10 +453,16 @@ class AlertType extends TAlertType { this = TLoopCounterSmallerThanLoopBound(_, _) and result = "loop bound" or - this = TLoopBoundIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopBoundIsNonConstExpr(_, _, _) and result = "mutated" or - this = TLoopStepIsNonConstExprOrMutatedVariableAccess(_, _, _) and + this = TLoopBoundIsMutatedVariableAccess(_, _, _) and + result = "mutated" + or + this = TLoopStepIsNonConstExpr(_, _, _) and + result = "mutated" + or + this = TLoopStepIsMutatedVariableAccess(_, _, _) and result = "mutated" or this = TLoopCounterIsTakenNonConstAddress(_, _) and @@ -449,4 +482,3 @@ from AlertType alert where not isExcluded(alert.asElement(), StatementsPackage::legacyForStatementsShouldBeSimpleQuery()) select alert, alert.getMessage(), alert.getLinkTarget1(), alert.getLinkText1(), alert.getLinkTarget2(), alert.getLinkText2() - From 662f51f1fe8f51d2c1918a85b7ba641310ee5386 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 22 Sep 2025 17:52:07 -0400 Subject: [PATCH 22/47] Debug 5-1-2 and 5-2-2 not being reported --- .../LegacyForStatementsShouldBeSimple.ql | 52 +++++++------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index a22c15ed45..a7054a3407 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -211,15 +211,8 @@ private newtype TAlertType = variableModifiedInExpression(mutatingExpr, loopBound.(VariableAccess).getTarget().getAnAccess()) } or /* 5-1-2. The loop bound is not a variable access nor a constant expression. */ - TLoopBoundIsNonConstExpr(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { + TLoopBoundIsNonConstExpr(ForStmt forLoop, Expr loopBound) { loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and - ( - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - ) and (not loopBound instanceof VariableAccess and not loopBound.isConstant()) } or /* 5-2-1. The loop step is a variable that is mutated in the for loop. */ @@ -235,15 +228,8 @@ private newtype TAlertType = variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) } or /* 5-2-2. The loop step is not a variable access nor a constant expression. */ - TLoopStepIsNonConstExpr(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { + TLoopStepIsNonConstExpr(ForStmt forLoop, Expr loopStep) { loopStep = getLoopStepOfForStmt(forLoop) and - ( - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - ) and (not loopStep instanceof VariableAccess and not loopStep.isConstant()) } or /* @@ -298,9 +284,9 @@ class AlertType extends TAlertType { this = TLoopCounterMutatedInLoopBody(result, _) or this = TLoopCounterSmallerThanLoopBound(result, _) or this = TLoopBoundIsMutatedVariableAccess(result, _, _) or - this = TLoopStepIsNonConstExpr(result, _, _) or - this = TLoopBoundIsMutatedVariableAccess(result, _, _) or - this = TLoopStepIsNonConstExpr(result, _, _) or + this = TLoopBoundIsNonConstExpr(result, _) or + this = TLoopStepIsMutatedVariableAccess(result, _, _) or + this = TLoopStepIsNonConstExpr(result, _) or this = TLoopCounterIsTakenNonConstAddress(result, _) or this = TLoopBoundIsTakenNonConstAddress(result, _) or this = TLoopStepIsTakenNonConstAddress(result, _) @@ -321,11 +307,11 @@ class AlertType extends TAlertType { result = forLoopCondition.getLoopCounter() ) or - this = TLoopBoundIsNonConstExpr(_, result, _) + this = TLoopBoundIsNonConstExpr(_, result) or this = TLoopBoundIsMutatedVariableAccess(_, result, _) or - this = TLoopStepIsNonConstExpr(_, result, _) + this = TLoopStepIsNonConstExpr(_, result) or this = TLoopStepIsMutatedVariableAccess(_, result, _) or @@ -355,13 +341,13 @@ class AlertType extends TAlertType { this = TLoopBoundIsMutatedVariableAccess(_, _, _) and result = "loop bound" or - this = TLoopBoundIsNonConstExpr(_, _, _) and + this = TLoopBoundIsNonConstExpr(_, _) and result = "loop bound" or this = TLoopStepIsMutatedVariableAccess(_, _, _) and result = "loop step" or - this = TLoopStepIsNonConstExpr(_, _, _) and + this = TLoopStepIsNonConstExpr(_, _) and result = "loop step" or this = TLoopCounterIsTakenNonConstAddress(_, _) and @@ -391,14 +377,14 @@ class AlertType extends TAlertType { this = TLoopCounterSmallerThanLoopBound(_, _) and result = "The $@ has a smaller type than that of the $@." or - this = TLoopBoundIsNonConstExpr(_, _, _) and - result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + this = TLoopBoundIsNonConstExpr(_, _) and + result = "The $@ is a $@." or this = TLoopBoundIsMutatedVariableAccess(_, _, _) and result = "The $@ is a non-const expression, or a variable that is $@ in the loop." or - this = TLoopStepIsNonConstExpr(_, _, _) and - result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + this = TLoopStepIsNonConstExpr(_, _) and + result = "The $@ is a $@." or this = TLoopStepIsMutatedVariableAccess(_, _, _) and result = "The $@ is a non-const expression, or a variable that is $@ in the loop." @@ -425,11 +411,11 @@ class AlertType extends TAlertType { result = forLoopCondition.getLoopBound() ) or - this = TLoopBoundIsNonConstExpr(_, _, result) + this = TLoopBoundIsNonConstExpr(_, result) or this = TLoopBoundIsMutatedVariableAccess(_, _, result) or - this = TLoopStepIsNonConstExpr(_, _, result) + this = TLoopStepIsNonConstExpr(_, result) or this = TLoopStepIsMutatedVariableAccess(_, _, result) or @@ -453,14 +439,14 @@ class AlertType extends TAlertType { this = TLoopCounterSmallerThanLoopBound(_, _) and result = "loop bound" or - this = TLoopBoundIsNonConstExpr(_, _, _) and - result = "mutated" + this = TLoopBoundIsNonConstExpr(_, _) and + result = "non-const expression" or this = TLoopBoundIsMutatedVariableAccess(_, _, _) and result = "mutated" or - this = TLoopStepIsNonConstExpr(_, _, _) and - result = "mutated" + this = TLoopStepIsNonConstExpr(_, _) and + result = "non-const expression" or this = TLoopStepIsMutatedVariableAccess(_, _, _) and result = "mutated" From a653b66511c1eef3f7a94932a333546b4f0c9b83 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 23 Sep 2025 17:05:36 -0400 Subject: [PATCH 23/47] Add LegacyForLoopUpdateExpression and test cases --- .../LegacyForStatementsShouldBeSimple.ql | 96 ++++++++++++++++++- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 23 +++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index a7054a3407..708b4eaf73 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -72,6 +72,60 @@ predicate variableModifiedInExpression(Expr expr, VariableAccess va) { valueToUpdate(va, _, expr) } +abstract class LegacyForLoopUpdateExpression extends Expr { + ForStmt forLoop; + + LegacyForLoopUpdateExpression() { this = forLoop.getUpdate().getAChild*() } + + abstract Expr getLoopStep(); +} + +class CrementLegacyForLoopUpdateExpression extends LegacyForLoopUpdateExpression { + CrementLegacyForLoopUpdateExpression() { this instanceof CrementOperation } + + override Expr getLoopStep() { none() } +} + +class AssignAddOrSubExpr extends LegacyForLoopUpdateExpression { + AssignAddOrSubExpr() { + this instanceof AssignAddExpr or + this instanceof AssignSubExpr + } + + override Expr getLoopStep() { + result = this.(AssignAddExpr).getRValue() or + result = this.(AssignSubExpr).getRValue() + } +} + +class AddOrSubThenAssignExpr extends LegacyForLoopUpdateExpression { + Expr assignRhs; + + AddOrSubThenAssignExpr() { + this.(AssignExpr).getRValue() = assignRhs and + ( + assignRhs instanceof AddExpr or + assignRhs instanceof SubExpr + ) + } + + override Expr getLoopStep() { + ( + result = assignRhs.(AddExpr).getAnOperand() or + result = assignRhs.(SubExpr).getAnOperand() + ) and + exists(VariableAccess iterationVariableAccess | + ( + iterationVariableAccess = assignRhs.(AddExpr).getAnOperand() + or + iterationVariableAccess = assignRhs.(SubExpr).getAnOperand() + ) and + iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and + result != iterationVariableAccess + ) + } +} + /** * Gets the loop step of a legacy for loop. * @@ -81,8 +135,36 @@ predicate variableModifiedInExpression(Expr expr, VariableAccess va) { * predicate. */ Expr getLoopStepOfForStmt(ForStmt forLoop) { - result = forLoop.getUpdate().(AssignAddExpr).getRValue() or - result = forLoop.getUpdate().(AssignSubExpr).getRValue() + /* + * NOTE: We compute the transitive closure of `getAChild` on the update expression, + * since the update expression may be a compound one that embeds the four aforementioned + * expression types, such as a comma expression (e.g. `i += 1, E` where `E` is an + * arbitrary expression). + * + * This may be detrimental to performance, but we keep it for soundness. A possible + * alternative is an IR-based solution. + */ + + /* 1. Get the expression `E` when the update expression is `i += E` or `i -= E`. */ + result = forLoop.getUpdate().getAChild*().(AssignAddExpr).getRValue() + or + result = forLoop.getUpdate().getAChild*().(AssignSubExpr).getRValue() + or + /* 2. Get the expression `E` when the update expression is `i = i + E` or `i = i - E`. */ + ( + result = forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(AddExpr).getAnOperand() or + result = forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(SubExpr).getAnOperand() + ) and + exists(VariableAccess iterationVariableAccess | + ( + iterationVariableAccess = + forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(AddExpr).getAnOperand() or + iterationVariableAccess = + forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(SubExpr).getAnOperand() + ) and + iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and + result != iterationVariableAccess + ) } /** @@ -184,9 +266,15 @@ private newtype TAlertType = condition = forLoop.getCondition() and not condition instanceof LegacyForLoopCondition } or - /* 3. The loop counter is mutated somewhere other than its update expression. */ + /* 3-1. The loop counter is mutated somewhere other than its update expression. */ TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounter) { - isIrregularLoopCounterModification(forLoop, loopCounter, loopCounter.getAnAccess()) + loopCounter = forLoop.getAnIterationVariable() and + variableModifiedInExpression(forLoop.getStmt().getChildStmt().getAChild*(), + loopCounter.getAnAccess()) + } or + /* 3-2. The loop counter is not updated using either of `++`, `--`, `+=`, or `-=`. */ + TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(ForStmt forLoop) { + none() // TODO } or /* 4. The type size of the loop counter is smaller than that of the loop bound. */ TLoopCounterSmallerThanLoopBound(ForStmt forLoop, LegacyForLoopCondition forLoopCondition) { diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 718bc4d068..cb6905525e 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -46,10 +46,33 @@ int main() { // the update expression } + for (int i = 20; i > 10; + --i) { // COMPLIANT: Pre-increment operator used as the update expression + } + + for (int i = 20; i > 10; i--) { // COMPLIANT: Post-increment operator used as + // the update expression + } + for (int i = 0; i < 10; i += 3) { // COMPLIANT: Add-and-assign operator used // as the update expression with loop step 3 } + for (int i = 20; i > 10; + i -= 3) { // COMPLIANT: Add-and-assign operator used + // as the update expression with loop step 3 + } + + for (int i = 0; i < 10; + i = i + + 3) { // COMPLIANT: Direct assignment with addition with loop step 3 + } + + for (int i = 20; i < 10; + i = i - + 3) { // COMPLIANT: Direct assignment with addition with loop step 3 + } + for (int i = 0; i < 10; i *= 2) { // NON_COMPLIANT: Mutiplication is not incrementing } From c8c0770d19aafe77ec70ff1c246cecd7d9766510 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 24 Sep 2025 14:10:29 -0400 Subject: [PATCH 24/47] Separate out helper classes into libraries --- cpp/common/src/codingstandards/cpp/Loops.qll | 31 ++++++ .../src/codingstandards/cpp/ast/Increment.qll | 57 +++++++++++ .../LegacyForStatementsShouldBeSimple.ql | 95 +------------------ 3 files changed, 91 insertions(+), 92 deletions(-) create mode 100644 cpp/common/src/codingstandards/cpp/ast/Increment.qll diff --git a/cpp/common/src/codingstandards/cpp/Loops.qll b/cpp/common/src/codingstandards/cpp/Loops.qll index aa3dc64ea5..ae5f6cf7f9 100644 --- a/cpp/common/src/codingstandards/cpp/Loops.qll +++ b/cpp/common/src/codingstandards/cpp/Loops.qll @@ -374,3 +374,34 @@ predicate isInvalidLoop(ForStmt forLoop, string reason, Locatable reasonLocation reason = "its $@ is not a boolean" and reasonLabel = "loop control variable" } + +/** + * A comparison expression that has the minimum qualification as being a valid termination + * condition of a legacy for-loop. It is characterized by a value read from a variable being + * compared to a value, which is supposed to be the loop bound. + */ +class LegacyForLoopCondition extends RelationalOperation { + /** + * The legacy for-loop this relational operation is a condition of. + */ + ForStmt forLoop; + VariableAccess loopCounter; + Expr loopBound; + + LegacyForLoopCondition() { + loopCounter = this.getAnOperand() and + loopBound = this.getAnOperand() and + loopCounter.getTarget() = getAnIterationVariable(forLoop) and + loopBound != loopCounter + } + + /** + * Gets the variable access to the loop counter variable, appearing in this loop condition. + */ + VariableAccess getLoopCounter() { result = loopCounter } + + /** + * Gets the variable access to the loop bound variable, appearing in this loop condition. + */ + Expr getLoopBound() { result = loopBound } +} diff --git a/cpp/common/src/codingstandards/cpp/ast/Increment.qll b/cpp/common/src/codingstandards/cpp/ast/Increment.qll new file mode 100644 index 0000000000..315748e067 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/ast/Increment.qll @@ -0,0 +1,57 @@ +import cpp + +abstract class LegacyForLoopUpdateExpression extends Expr { + ForStmt forLoop; + + LegacyForLoopUpdateExpression() { this = forLoop.getUpdate().getAChild*() } + + abstract Expr getLoopStep(); + /* TODO: Complete below and use it for 3-2 */ + // abstract VariableAccess getUpdatedVariable(); +} + +class CrementLegacyForLoopUpdateExpression extends LegacyForLoopUpdateExpression { + CrementLegacyForLoopUpdateExpression() { this instanceof CrementOperation } + + override Expr getLoopStep() { none() } +} + +class AssignAddOrSubExpr extends LegacyForLoopUpdateExpression { + AssignAddOrSubExpr() { + this instanceof AssignAddExpr or + this instanceof AssignSubExpr + } + + override Expr getLoopStep() { + result = this.(AssignAddExpr).getRValue() or + result = this.(AssignSubExpr).getRValue() + } +} + +class AddOrSubThenAssignExpr extends LegacyForLoopUpdateExpression { + Expr assignRhs; + + AddOrSubThenAssignExpr() { + this.(AssignExpr).getRValue() = assignRhs and + ( + assignRhs instanceof AddExpr or + assignRhs instanceof SubExpr + ) + } + + override Expr getLoopStep() { + ( + result = assignRhs.(AddExpr).getAnOperand() or + result = assignRhs.(SubExpr).getAnOperand() + ) and + exists(VariableAccess iterationVariableAccess | + ( + iterationVariableAccess = assignRhs.(AddExpr).getAnOperand() + or + iterationVariableAccess = assignRhs.(SubExpr).getAnOperand() + ) and + iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and + result != iterationVariableAccess + ) + } +} \ No newline at end of file diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 708b4eaf73..91212d2830 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -22,37 +22,6 @@ import codingstandards.cpp.Call import codingstandards.cpp.Loops import codingstandards.cpp.misra.BuiltInTypeRules::MisraCpp23BuiltInTypes -/** - * A comparison expression that has the minimum qualification as being a valid termination - * condition of a legacy for-loop. It is characterized by a value read from a variable being - * compared to a value, which is supposed to be the loop bound. - */ -class LegacyForLoopCondition extends RelationalOperation { - /** - * The legacy for-loop this relational operation is a condition of. - */ - ForStmt forLoop; - VariableAccess loopCounter; - Expr loopBound; - - LegacyForLoopCondition() { - loopCounter = this.getAnOperand() and - loopBound = this.getAnOperand() and - loopCounter.getTarget() = getAnIterationVariable(forLoop) and - loopBound != loopCounter - } - - /** - * Gets the variable access to the loop counter variable, embedded in this loop condition. - */ - VariableAccess getLoopCounter() { result = loopCounter } - - /** - * Gets the variable access to the loop bound variable, embedded in this loop condition. - */ - Expr getLoopBound() { result = loopBound } -} - /** * Holds if the given expression may mutate the variable. */ @@ -72,60 +41,6 @@ predicate variableModifiedInExpression(Expr expr, VariableAccess va) { valueToUpdate(va, _, expr) } -abstract class LegacyForLoopUpdateExpression extends Expr { - ForStmt forLoop; - - LegacyForLoopUpdateExpression() { this = forLoop.getUpdate().getAChild*() } - - abstract Expr getLoopStep(); -} - -class CrementLegacyForLoopUpdateExpression extends LegacyForLoopUpdateExpression { - CrementLegacyForLoopUpdateExpression() { this instanceof CrementOperation } - - override Expr getLoopStep() { none() } -} - -class AssignAddOrSubExpr extends LegacyForLoopUpdateExpression { - AssignAddOrSubExpr() { - this instanceof AssignAddExpr or - this instanceof AssignSubExpr - } - - override Expr getLoopStep() { - result = this.(AssignAddExpr).getRValue() or - result = this.(AssignSubExpr).getRValue() - } -} - -class AddOrSubThenAssignExpr extends LegacyForLoopUpdateExpression { - Expr assignRhs; - - AddOrSubThenAssignExpr() { - this.(AssignExpr).getRValue() = assignRhs and - ( - assignRhs instanceof AddExpr or - assignRhs instanceof SubExpr - ) - } - - override Expr getLoopStep() { - ( - result = assignRhs.(AddExpr).getAnOperand() or - result = assignRhs.(SubExpr).getAnOperand() - ) and - exists(VariableAccess iterationVariableAccess | - ( - iterationVariableAccess = assignRhs.(AddExpr).getAnOperand() - or - iterationVariableAccess = assignRhs.(SubExpr).getAnOperand() - ) and - iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and - result != iterationVariableAccess - ) - } -} - /** * Gets the loop step of a legacy for loop. * @@ -146,9 +61,7 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { */ /* 1. Get the expression `E` when the update expression is `i += E` or `i -= E`. */ - result = forLoop.getUpdate().getAChild*().(AssignAddExpr).getRValue() - or - result = forLoop.getUpdate().getAChild*().(AssignSubExpr).getRValue() + result = forLoop.getUpdate().getAChild*().(AssignAddOrSubExpr).getLoopStep() or /* 2. Get the expression `E` when the update expression is `i = i + E` or `i = i - E`. */ ( @@ -175,13 +88,11 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { * 2. Another variable access of the same variable as the given variable access is assigned * to a non-const reference variable (thus constituting a `T` -> `&T` conversion.), i.e. * initialization and assignment. - */ -/* + * * Note that pass-by-reference is dealt with in a different predicate named * `loopVariablePassedAsArgumentToNonConstReferenceParameter`, due to implementation * limitations. */ - predicate loopVariableAssignedToNonConstPointerOrReferenceType( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { @@ -199,7 +110,7 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( assignmentRhs.(AddressOfExpr).getOperand() = loopVariableAccessInCondition.getTarget().getAnAccess() or - /* 2. The address is taken: A loop variable access */ + /* 2. A reference is taken: A loop variable access */ assignmentRhs = loopVariableAccessInCondition.getTarget().getAnAccess() ) ) From 2227255825358b3cec67478284b682037361ad54 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 24 Sep 2025 15:28:13 -0400 Subject: [PATCH 25/47] Finish draft of `LegacyForStatementsShouldBeSimple` --- .../LegacyForStatementsShouldBeSimple.ql | 47 ++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 91212d2830..633aac3811 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -20,8 +20,13 @@ import semmle.code.cpp.dataflow.internal.AddressFlow import codingstandards.cpp.misra import codingstandards.cpp.Call import codingstandards.cpp.Loops +import codingstandards.cpp.ast.Increment import codingstandards.cpp.misra.BuiltInTypeRules::MisraCpp23BuiltInTypes +Variable getDeclaredVariableInForLoop(ForStmt forLoop) { + result = forLoop.getADeclaration().getADeclarationEntry().(VariableDeclarationEntry).getVariable() +} + /** * Holds if the given expression may mutate the variable. */ @@ -75,7 +80,7 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { iterationVariableAccess = forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(SubExpr).getAnOperand() ) and - iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and + iterationVariableAccess.getTarget() = getDeclaredVariableInForLoop(forLoop) and result != iterationVariableAccess ) } @@ -159,9 +164,9 @@ predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( private newtype TAlertType = /* 1. There is a counter variable that is not of an integer type. */ - TNonIntegerTypeCounterVariable(ForStmt forLoop, Variable iterationVariable) { - iterationVariable = forLoop.getAnIterationVariable() and - exists(Type type | type = iterationVariable.getType() | + TNonIntegerTypeCounterVariable(ForStmt forLoop, Variable loopCounter) { + loopCounter = getDeclaredVariableInForLoop(forLoop) and + exists(Type type | type = loopCounter.getType() | not ( type instanceof IntegralType or type instanceof FixedWidthIntegralType @@ -178,14 +183,18 @@ private newtype TAlertType = not condition instanceof LegacyForLoopCondition } or /* 3-1. The loop counter is mutated somewhere other than its update expression. */ - TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounter) { - loopCounter = forLoop.getAnIterationVariable() and + TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounterVariable) { + loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and variableModifiedInExpression(forLoop.getStmt().getChildStmt().getAChild*(), - loopCounter.getAnAccess()) + loopCounterVariable.getAnAccess()) } or /* 3-2. The loop counter is not updated using either of `++`, `--`, `+=`, or `-=`. */ - TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(ForStmt forLoop) { - none() // TODO + TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr( + ForStmt forLoop, Variable loopCounterVariable, Expr updateExpr + ) { + loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and + variableModifiedInExpression(updateExpr, loopCounterVariable.getAnAccess()) and + not updateExpr instanceof LegacyForLoopUpdateExpression } or /* 4. The type size of the loop counter is smaller than that of the loop bound. */ TLoopCounterSmallerThanLoopBound(ForStmt forLoop, LegacyForLoopCondition forLoopCondition) { @@ -281,6 +290,7 @@ class AlertType extends TAlertType { this = TNonIntegerTypeCounterVariable(result, _) or this = TNoRelationalOperatorInLoopCondition(result, _) or this = TLoopCounterMutatedInLoopBody(result, _) or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(result, _, _) or this = TLoopCounterSmallerThanLoopBound(result, _) or this = TLoopBoundIsMutatedVariableAccess(result, _, _) or this = TLoopBoundIsNonConstExpr(result, _) or @@ -301,6 +311,8 @@ class AlertType extends TAlertType { or this = TLoopCounterMutatedInLoopBody(_, result) or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(_, result, _) + or exists(LegacyForLoopCondition forLoopCondition | this = TLoopCounterSmallerThanLoopBound(_, forLoopCondition) and result = forLoopCondition.getLoopCounter() @@ -334,6 +346,9 @@ class AlertType extends TAlertType { this = TLoopCounterMutatedInLoopBody(_, _) and result = "counter variable" or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(_, _, _) and + result = "counter variable" + or this = TLoopCounterSmallerThanLoopBound(_, _) and result = "counter variable" or @@ -364,15 +379,18 @@ class AlertType extends TAlertType { */ string getMessage() { this = TNonIntegerTypeCounterVariable(_, _) and - result = "The $@ is not of an integer type." // Throwaway placeholder + result = "The $@ is not of an integer type." or this = TNoRelationalOperatorInLoopCondition(_, _) and result = - "The $@ does not compare the counter variable to an expression using a relational operator." // Throwaway placeholder + "The $@ does not compare the counter variable to an expression using a relational operator." or this = TLoopCounterMutatedInLoopBody(_, _) and result = "The $@ may be mutated in a location other than its update expression." or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(_, _, _) and + result = "The $@ is not updated with an $@ other than addition or subtraction." + or this = TLoopCounterSmallerThanLoopBound(_, _) and result = "The $@ has a smaller type than that of the $@." or @@ -403,7 +421,9 @@ class AlertType extends TAlertType { or this = TNoRelationalOperatorInLoopCondition(_, result) // Throwaway or - this = TLoopCounterMutatedInLoopBody(_, _) // Throwaway + this = TLoopCounterMutatedInLoopBody(_, result) // Throwaway + or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(_, _, result) or exists(LegacyForLoopCondition forLoopCondition | this = TLoopCounterSmallerThanLoopBound(_, forLoopCondition) and @@ -435,6 +455,9 @@ class AlertType extends TAlertType { this = TLoopCounterMutatedInLoopBody(_, _) and result = "N/A" // Throwaway or + this = TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr(_, _, _) and + result = "expression" + or this = TLoopCounterSmallerThanLoopBound(_, _) and result = "loop bound" or From 38b5fbcaaa61b8860de5cc3619c89ff43294bf0e Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 24 Sep 2025 16:19:14 -0400 Subject: [PATCH 26/47] Decouple ForStmt from `Increment.qll` and rewrite `getLoopStepOfForStmt` --- .../src/codingstandards/cpp/ast/Increment.qll | 103 +++++++++++------- .../LegacyForStatementsShouldBeSimple.ql | 20 +--- 2 files changed, 63 insertions(+), 60 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/ast/Increment.qll b/cpp/common/src/codingstandards/cpp/ast/Increment.qll index 315748e067..00c893baae 100644 --- a/cpp/common/src/codingstandards/cpp/ast/Increment.qll +++ b/cpp/common/src/codingstandards/cpp/ast/Increment.qll @@ -1,57 +1,76 @@ -import cpp - -abstract class LegacyForLoopUpdateExpression extends Expr { - ForStmt forLoop; - - LegacyForLoopUpdateExpression() { this = forLoop.getUpdate().getAChild*() } - - abstract Expr getLoopStep(); - /* TODO: Complete below and use it for 3-2 */ - // abstract VariableAccess getUpdatedVariable(); -} +/** + * Provides a library for working with expressions that update the value + * of a numeric variable by incrementing or decrementing it by a certain + * amount. + */ -class CrementLegacyForLoopUpdateExpression extends LegacyForLoopUpdateExpression { - CrementLegacyForLoopUpdateExpression() { this instanceof CrementOperation } - - override Expr getLoopStep() { none() } -} +import cpp -class AssignAddOrSubExpr extends LegacyForLoopUpdateExpression { +private class AssignAddOrSubExpr extends AssignArithmeticOperation { AssignAddOrSubExpr() { this instanceof AssignAddExpr or this instanceof AssignSubExpr } +} - override Expr getLoopStep() { - result = this.(AssignAddExpr).getRValue() or - result = this.(AssignSubExpr).getRValue() +private class AddOrSubExpr extends BinaryArithmeticOperation { + AddOrSubExpr() { + this instanceof AddExpr or + this instanceof SubExpr } } -class AddOrSubThenAssignExpr extends LegacyForLoopUpdateExpression { - Expr assignRhs; +/** + * An expression that updates a numeric variable by adding to or subtracting + * from it a certain amount. + */ +abstract class StepCrementUpdateExpr extends Expr { + /** + * The expression in the abstract syntax tree that represents the amount of + * value by which the variable is updated. + */ + abstract Expr getAmountExpr(); +} + +/** + * An increment or decrement operator application, either postfix or prefix. + */ +class PostfixOrPrefixCrementExpr extends CrementOperation, StepCrementUpdateExpr { + override Expr getAmountExpr() { none() } +} + +/** + * An add-then-assign or subtract-then-assign expression in a shortened form, + * i.e. `+=` or `-=`. + */ +class AssignAddOrSubUpdateExpr extends AssignAddOrSubExpr, StepCrementUpdateExpr { + override Expr getAmountExpr() { result = this.getRValue() } +} + +/** + * An add-then-assign expression or a subtract-then-assign expression, i.e. + * `x = x + E` or `x = x - E`, where `x` is some variable and `E` an + * arbitrary expression. + */ +class AddOrSubThenAssignExpr extends AssignExpr, StepCrementUpdateExpr { + /** The `x` as in the left-hand side of `x = x + E`. */ + VariableAccess lvalueVariable; + /** The `x + E` as in `x = x + E`. */ + AddOrSubExpr addOrSubExpr; + /** The `E` as in `x = x + E`. */ + Expr amountExpr; AddOrSubThenAssignExpr() { - this.(AssignExpr).getRValue() = assignRhs and - ( - assignRhs instanceof AddExpr or - assignRhs instanceof SubExpr + this.getLValue() = lvalueVariable and + this.getRValue() = addOrSubExpr and + exists(VariableAccess lvalueVariableAsRvalue | + lvalueVariableAsRvalue = addOrSubExpr.getAnOperand() and + amountExpr = addOrSubExpr.getAnOperand() and + lvalueVariableAsRvalue != amountExpr + | + lvalueVariable.getTarget() = lvalueVariableAsRvalue.(VariableAccess).getTarget() ) } - override Expr getLoopStep() { - ( - result = assignRhs.(AddExpr).getAnOperand() or - result = assignRhs.(SubExpr).getAnOperand() - ) and - exists(VariableAccess iterationVariableAccess | - ( - iterationVariableAccess = assignRhs.(AddExpr).getAnOperand() - or - iterationVariableAccess = assignRhs.(SubExpr).getAnOperand() - ) and - iterationVariableAccess.getTarget() = forLoop.getAnIterationVariable() and - result != iterationVariableAccess - ) - } -} \ No newline at end of file + override Expr getAmountExpr() { result = amountExpr } +} diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 633aac3811..458fbc4f91 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -66,23 +66,7 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { */ /* 1. Get the expression `E` when the update expression is `i += E` or `i -= E`. */ - result = forLoop.getUpdate().getAChild*().(AssignAddOrSubExpr).getLoopStep() - or - /* 2. Get the expression `E` when the update expression is `i = i + E` or `i = i - E`. */ - ( - result = forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(AddExpr).getAnOperand() or - result = forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(SubExpr).getAnOperand() - ) and - exists(VariableAccess iterationVariableAccess | - ( - iterationVariableAccess = - forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(AddExpr).getAnOperand() or - iterationVariableAccess = - forLoop.getUpdate().getAChild*().(AssignExpr).getRValue().(SubExpr).getAnOperand() - ) and - iterationVariableAccess.getTarget() = getDeclaredVariableInForLoop(forLoop) and - result != iterationVariableAccess - ) + result = forLoop.getUpdate().getAChild*().(StepCrementUpdateExpr).getAmountExpr() } /** @@ -194,7 +178,7 @@ private newtype TAlertType = ) { loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and variableModifiedInExpression(updateExpr, loopCounterVariable.getAnAccess()) and - not updateExpr instanceof LegacyForLoopUpdateExpression + not updateExpr instanceof StepCrementUpdateExpr } or /* 4. The type size of the loop counter is smaller than that of the loop bound. */ TLoopCounterSmallerThanLoopBound(ForStmt forLoop, LegacyForLoopCondition forLoopCondition) { From 23aa711400340873fd8d580c08255b476535b934 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 29 Sep 2025 18:46:04 -0400 Subject: [PATCH 27/47] Update expected result of `RULE-9-5-1` --- ...LegacyForStatementsShouldBeSimple.expected | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected index 2ec1a0ac6c..1c85002d8c 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected +++ b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected @@ -1 +1,36 @@ -No expected results have yet been specified \ No newline at end of file +| test2.cpp:8:5:11:5 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test2.cpp:8:14:8:14 | i | counter variable | test2.cpp:9:9:9:9 | call to f | expression | +| test2.cpp:8:5:11:5 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test2.cpp:8:14:8:14 | i | counter variable | test2.cpp:8:14:8:14 | i | N/A | +| test.cpp:22:3:24:3 | for(...;...;...) ... | The $@ is not of an integer type. | test.cpp:22:14:22:14 | i | counter variable | test.cpp:22:14:22:14 | i | N/A | +| test.cpp:31:3:33:3 | for(...;...;...) ... | The $@ does not compare the counter variable to an expression using a relational operator. | test.cpp:31:19:31:25 | ... == ... | loop condition | test.cpp:31:19:31:25 | ... == ... | N/A | +| test.cpp:35:3:37:3 | for(...;...;...) ... | The $@ does not compare the counter variable to an expression using a relational operator. | test.cpp:35:19:35:24 | ... < ... | loop condition | test.cpp:35:19:35:24 | ... < ... | N/A | +| test.cpp:76:3:78:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:76:12:76:12 | i | counter variable | test.cpp:77:8:77:13 | ... *= ... | expression | +| test.cpp:90:3:93:3 | for(...;...;...) ... | The $@ has a smaller type than that of the $@. | test.cpp:90:19:90:19 | i | counter variable | test.cpp:90:23:90:27 | 10 | loop bound | +| test.cpp:115:3:118:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:116:13:116:13 | j | loop step | test.cpp:117:5:117:7 | ... ++ | mutated | +| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:121:13:121:13 | j | loop step | test.cpp:121:8:121:18 | ... , ... | mutated | +| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:121:13:121:13 | j | loop step | test.cpp:121:16:121:18 | ... ++ | mutated | +| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:120:12:120:12 | i | counter variable | test.cpp:121:8:121:18 | ... , ... | expression | +| test.cpp:132:3:135:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:132:23:132:23 | j | loop bound | test.cpp:134:5:134:7 | ... ++ | mutated | +| test.cpp:139:3:142:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:139:23:139:23 | k | loop bound | test.cpp:141:5:141:26 | ... += ... | mutated | +| test.cpp:144:3:147:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:145:13:145:13 | l | loop step | test.cpp:146:5:146:26 | ... += ... | mutated | +| test.cpp:149:3:151:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:149:23:149:24 | call to h1 | loop bound | test.cpp:149:23:149:24 | call to h1 | non-const expression | +| test.cpp:157:3:159:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:158:13:158:14 | call to h1 | loop step | test.cpp:158:13:158:14 | call to h1 | non-const expression | +| test.cpp:172:3:175:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:172:19:172:19 | i | loop counter | test.cpp:172:19:172:19 | i | N/A | +| test.cpp:177:3:180:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:177:19:177:19 | i | loop counter | test.cpp:177:19:177:19 | i | N/A | +| test.cpp:192:3:195:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:192:23:192:23 | k | loop bound | test.cpp:192:23:192:23 | k | N/A | +| test.cpp:197:3:200:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:197:23:197:23 | k | loop bound | test.cpp:197:23:197:23 | k | N/A | +| test.cpp:212:3:215:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:212:31:212:31 | l | loop step | test.cpp:212:31:212:31 | l | N/A | +| test.cpp:217:3:220:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:217:31:217:31 | l | loop step | test.cpp:217:31:217:31 | l | N/A | +| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:232:12:232:12 | i | counter variable | test.cpp:234:5:234:6 | call to f1 | expression | +| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:232:19:232:19 | i | loop counter | test.cpp:232:19:232:19 | i | N/A | +| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:232:12:232:12 | i | counter variable | test.cpp:232:12:232:12 | i | N/A | +| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:237:12:237:12 | i | counter variable | test.cpp:239:5:239:6 | call to g1 | expression | +| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:237:19:237:19 | i | loop counter | test.cpp:237:19:237:19 | i | N/A | +| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:237:12:237:12 | i | counter variable | test.cpp:237:12:237:12 | i | N/A | +| test.cpp:252:3:255:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:252:23:252:23 | k | loop bound | test.cpp:254:5:254:6 | call to f1 | mutated | +| test.cpp:252:3:255:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:252:23:252:23 | k | loop bound | test.cpp:252:23:252:23 | k | N/A | +| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:257:23:257:23 | k | loop bound | test.cpp:259:5:259:6 | call to g1 | mutated | +| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:257:23:257:23 | k | loop bound | test.cpp:257:23:257:23 | k | N/A | +| test.cpp:273:3:276:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:273:31:273:31 | l | loop step | test.cpp:275:5:275:6 | call to f1 | mutated | +| test.cpp:273:3:276:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:273:31:273:31 | l | loop step | test.cpp:273:31:273:31 | l | N/A | +| test.cpp:278:3:281:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:278:31:278:31 | l | loop step | test.cpp:280:5:280:6 | call to g1 | mutated | +| test.cpp:278:3:281:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:278:31:278:31 | l | loop step | test.cpp:278:31:278:31 | l | N/A | From f0b53e22affe1a8554fc58e05fa08a2d2642cbbf Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 29 Sep 2025 19:19:26 -0400 Subject: [PATCH 28/47] Update expected results of `RULE-9-5-2` --- .../ForRangeInitializerAtMostOneFunctionCall.ql | 9 +++++---- ...angeInitializerAtMostOneFunctionCall.expected | 16 ++++++++-------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql index b78da2ad99..e6b8df9f87 100644 --- a/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql +++ b/cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql @@ -19,9 +19,10 @@ import cpp import codingstandards.cpp.misra -from RangeBasedForStmt foreach, string message +from RangeBasedForStmt foreach, Expr initializer where not isExcluded(foreach, StatementsPackage::forRangeInitializerAtMostOneFunctionCallQuery()) and - count(Call call | call = foreach.getRange().getAChild*() | call) >= 2 and - message = "has nested call expression in its initializer" -select foreach, "Range-based for loop " + message + "." + initializer = foreach.getRange() and + count(Call call | call = initializer.getAChild*() | call) >= 2 +select foreach, "Range-based for loop has nested call expression in its $@.", initializer, + "initializer" diff --git a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected index a8567ff48d..8b0fcb1d4e 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected +++ b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected @@ -1,8 +1,8 @@ -| test.cpp:48:3:49:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:56:3:59:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:71:3:73:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:95:3:97:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:99:3:101:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:103:3:107:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:116:3:119:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | -| test.cpp:121:3:124:3 | for(...:...) ... | Range-based for loop has nested call expression in its initializer. | +| test.cpp:48:3:49:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:48:17:48:27 | call to processData | initializer | +| test.cpp:56:3:59:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:57:8:57:32 | call to vector | initializer | +| test.cpp:71:3:73:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:71:17:72:21 | call to MyContainer | initializer | +| test.cpp:95:3:97:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:95:26:95:26 | call to operator+ | initializer | +| test.cpp:99:3:101:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:99:31:99:31 | call to operator+ | initializer | +| test.cpp:103:3:107:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:104:18:104:18 | call to operator+ | initializer | +| test.cpp:116:3:119:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:117:8:117:25 | call to convertToIntVector | initializer | +| test.cpp:121:3:124:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:122:8:122:25 | call to convertToIntVector | initializer | From 7d5f08b3e788afbe46fe865f4243205263eb3cd0 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 7 Oct 2025 15:23:40 -0400 Subject: [PATCH 29/47] Count in typedefs and cv-qualifiers We are interested if the underlying *data* can be mutated, not the pointer itself. Also, the surface type may be a typedef, so resolve that as well. --- .../LegacyForStatementsShouldBeSimple.ql | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 458fbc4f91..ee8bf06361 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -85,9 +85,10 @@ Expr getLoopStepOfForStmt(ForStmt forLoop) { predicate loopVariableAssignedToNonConstPointerOrReferenceType( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { - exists(Expr assignmentRhs, DerivedType targetType | + exists(Expr assignmentRhs, Type targetType, DerivedType strippedType | isAssignment(assignmentRhs, targetType, _) and - not targetType.getBaseType().isConst() and + strippedType = targetType.stripTopLevelSpecifiers() and + not strippedType.getBaseType().isConst() and ( targetType instanceof PointerType or targetType instanceof ReferenceType @@ -105,9 +106,14 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( ) } -/* - * An adapted part of `BuiltinTypeRules::MisraCpp23BuiltInTypes::isPreConversionAssignment` - * that is only relevant to an argument passed to a parameter, seen as an assignment. +/** + * Holds if the given variable access has another variable access with the same target + * variable that is passed as reference to a non-const reference parameter of a function, + * constituting a `T` -> `&T` conversion. + * + * This is an adapted part of + * `BuiltinTypeRules::MisraCpp23BuiltInTypes::isPreConversionAssignment` that is only + * relevant to an argument passed to a parameter, seen as an assignment. * * This predicate adds two constraints to the target type, as compared to the original * portion of the predicate: @@ -117,20 +123,15 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( * * Also, this predicate requires that the call is the body of the given for-loop. */ - -/** - * Holds if the given variable access has another variable access with the same target - * variable that is passed as reference to a non-const reference parameter of a function, - * constituting a `T` -> `&T` conversion. - */ predicate loopVariablePassedAsArgumentToNonConstReferenceParameter( ForStmt forLoop, VariableAccess loopVariableAccessInCondition ) { - exists(ReferenceType targetType | + exists(Type targetType, ReferenceType strippedReferenceType | exists(Call call, int i | call.getArgument(i) = loopVariableAccessInCondition.getTarget().getAnAccess() and call.getEnclosingStmt().getParent*() = forLoop.getStmt() and - not targetType.getBaseType().isConst() + strippedReferenceType = targetType.stripTopLevelSpecifiers() and + not strippedReferenceType.getBaseType().isConst() | /* A regular function call */ targetType = call.getTarget().getParameter(i).getType() From e135fe6c93ec6b0b0d6e0e9cbde2d58906a3376b Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 7 Oct 2025 15:25:26 -0400 Subject: [PATCH 30/47] Fix cross-join issue in `TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr` --- .../src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index ee8bf06361..4ad3be0ef6 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -178,6 +178,7 @@ private newtype TAlertType = ForStmt forLoop, Variable loopCounterVariable, Expr updateExpr ) { loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and + updateExpr = forLoop.getUpdate() and variableModifiedInExpression(updateExpr, loopCounterVariable.getAnAccess()) and not updateExpr instanceof StepCrementUpdateExpr } or From b13ffd2b48b7fff06dc74074021c56fec305d176 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 7 Oct 2025 15:26:00 -0400 Subject: [PATCH 31/47] Slightly change wording in the message Both `TLoopBoundIsMutatedVariableAccess` and `TLoopStepIsMutatedVariableAccess` transitively rely on `valueToUpdate`, which overapproximates by looking at the types alone. Therefore we'd like to drop the confidence slightly in reporting the expression where the expression *might* have been changed. --- .../src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 4ad3be0ef6..4186f4149c 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -384,13 +384,13 @@ class AlertType extends TAlertType { result = "The $@ is a $@." or this = TLoopBoundIsMutatedVariableAccess(_, _, _) and - result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + result = "The $@ is a non-const expression, or a variable that may be $@ in the loop." or this = TLoopStepIsNonConstExpr(_, _) and result = "The $@ is a $@." or this = TLoopStepIsMutatedVariableAccess(_, _, _) and - result = "The $@ is a non-const expression, or a variable that is $@ in the loop." + result = "The $@ is a non-const expression, or a variable that may be $@ in the loop." or this = TLoopCounterIsTakenNonConstAddress(_, _) and result = "The $@ is taken as a mutable reference or its address to a mutable pointer." From 82f9adc44785ad6f2c30747eb00fb2faac4eb12d Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 7 Oct 2025 16:55:10 -0400 Subject: [PATCH 32/47] Make the loop counter detection more relaxed --- cpp/common/src/codingstandards/cpp/Loops.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/Loops.qll b/cpp/common/src/codingstandards/cpp/Loops.qll index ae5f6cf7f9..f817ed6b5b 100644 --- a/cpp/common/src/codingstandards/cpp/Loops.qll +++ b/cpp/common/src/codingstandards/cpp/Loops.qll @@ -389,7 +389,7 @@ class LegacyForLoopCondition extends RelationalOperation { Expr loopBound; LegacyForLoopCondition() { - loopCounter = this.getAnOperand() and + loopCounter = this.getAnOperand().getAChild*() and loopBound = this.getAnOperand() and loopCounter.getTarget() = getAnIterationVariable(forLoop) and loopBound != loopCounter From 211640077b41a0091d5565b10a08515651c23a2a Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 8 Oct 2025 17:29:53 -0400 Subject: [PATCH 33/47] Refine `LegacyForLoopCondition` --- cpp/common/src/codingstandards/cpp/Loops.qll | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/Loops.qll b/cpp/common/src/codingstandards/cpp/Loops.qll index f817ed6b5b..a18c8e34af 100644 --- a/cpp/common/src/codingstandards/cpp/Loops.qll +++ b/cpp/common/src/codingstandards/cpp/Loops.qll @@ -389,12 +389,21 @@ class LegacyForLoopCondition extends RelationalOperation { Expr loopBound; LegacyForLoopCondition() { - loopCounter = this.getAnOperand().getAChild*() and - loopBound = this.getAnOperand() and - loopCounter.getTarget() = getAnIterationVariable(forLoop) and - loopBound != loopCounter + this = forLoop.getCondition() and + exists(Expr loopCounterExpr | + loopCounterExpr = this.getAnOperand() and + loopBound = this.getAnOperand() and + loopCounter = loopCounterExpr.getAChild*() and + loopCounter.getTarget() = getAnIterationVariable(forLoop) and + loopBound != loopCounterExpr + ) } + /** + * Gets the for-loop this expression is a termination condition of. + */ + ForStmt getForLoop() { result = forLoop } + /** * Gets the variable access to the loop counter variable, appearing in this loop condition. */ From 1f8083a99752dd270e94d94e2655134a70cc0ffb Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 8 Oct 2025 17:30:18 -0400 Subject: [PATCH 34/47] Change phrasing of message from `TNoRelationalOperatorInLoopCondition` --- .../src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 4186f4149c..8adfaf07a2 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -369,7 +369,7 @@ class AlertType extends TAlertType { or this = TNoRelationalOperatorInLoopCondition(_, _) and result = - "The $@ does not compare the counter variable to an expression using a relational operator." + "The $@ does not determine termination based only on a comparison against the value of the counter variable." or this = TLoopCounterMutatedInLoopBody(_, _) and result = "The $@ may be mutated in a location other than its update expression." From 1355effa734c50c900474c6d368bf98d89493e93 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Wed, 8 Oct 2025 18:52:24 -0400 Subject: [PATCH 35/47] Use `upperbound/0` and `getFullyConverted/0` to more precisely infer sizes --- .../RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 8adfaf07a2..f34b8aec53 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -185,11 +185,10 @@ private newtype TAlertType = /* 4. The type size of the loop counter is smaller than that of the loop bound. */ TLoopCounterSmallerThanLoopBound(ForStmt forLoop, LegacyForLoopCondition forLoopCondition) { forLoopCondition = forLoop.getCondition() and - exists(Type loopCounterType, Type loopBoundType | - loopCounterType = forLoopCondition.getLoopCounter().getType() and - loopBoundType = forLoopCondition.getLoopBound().getType() - | - loopCounterType.getSize() < loopBoundType.getSize() + exists(Expr loopCounter, Expr loopBound | + loopCounter = forLoopCondition.getLoopCounter() and + loopBound = forLoopCondition.getLoopBound() and + upperBound(loopCounter.getFullyConverted()) < upperBound(loopBound.getFullyConverted()) ) } or /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ From 3ad5ef2ae7028d24b57a4899d30f6881c410416b Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 9 Oct 2025 14:41:10 -0400 Subject: [PATCH 36/47] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp index b9ec5a0574..c832f9735b 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -54,7 +54,7 @@ int main() { } for (auto x : - std::vector{1, 2, 3}) { // NON_COMPLIANT: 2 constructor call to + std::vector{1, 2, 3}) { // NON_COMPLIANT: 2 constructor calls to // `vector` and `initializer_list`, respectively } From fb2006513b29d87df34e13e1b9f44618f462215f Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 9 Oct 2025 14:41:27 -0400 Subject: [PATCH 37/47] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql index f419834325..893ebfe735 100644 --- a/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql +++ b/cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql @@ -28,7 +28,7 @@ where exists(Stmt initializer | initializer = switch.getInitialization() | not initializer instanceof DeclStmt ) and - message = "contains a statement that that is not a simple declaration" + message = "contains a statement that is not a simple declaration" or /* 2. There is a switch case label that does not lead a branch (i.e. a switch case label is nested). */ exists(SwitchCase case | case = switch.getASwitchCase() | case instanceof NestedSwitchCase) and From bce38a04eb31b9288b1db6b4ec57bf905ca9e08b Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 9 Oct 2025 14:50:55 -0400 Subject: [PATCH 38/47] Reformat test cases of 9-4-2 and 9-5-2 --- cpp/misra/test/rules/RULE-9-4-2/test.cpp | 2 +- cpp/misra/test/rules/RULE-9-5-2/test.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-4-2/test.cpp b/cpp/misra/test/rules/RULE-9-4-2/test.cpp index 6bbb97e81b..0e8c2e2ec2 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-4-2/test.cpp @@ -88,7 +88,7 @@ void testOtherLabelsInBranch(int expr) { void testLeadingNonCaseStatement(int expr) { switch (expr) { // NON_COMPLIANT: Non-case statement is the first statement in // the switch body - int x = 1; + int x = 1; case 1: i++; break; diff --git a/cpp/misra/test/rules/RULE-9-5-2/test.cpp b/cpp/misra/test/rules/RULE-9-5-2/test.cpp index b9ec5a0574..bc8c6c6acf 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-2/test.cpp @@ -53,9 +53,9 @@ int main() { for (auto x : std::vector(3)) { // COMPLIANT: 1 constructor call only } - for (auto x : - std::vector{1, 2, 3}) { // NON_COMPLIANT: 2 constructor call to - // `vector` and `initializer_list`, respectively + for (auto x : std::vector{ + 1, 2, 3}) { // NON_COMPLIANT: 2 constructor call to + // `vector` and `initializer_list`, respectively } for (auto x : MyContainer()) { // COMPLIANT: 1 constructor call only From 59a40964aaab48d546a96756e13ca8137e5908e0 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 9 Oct 2025 15:57:50 -0400 Subject: [PATCH 39/47] Update expected results coming from change in message --- .../RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected | 2 +- .../ForRangeInitializerAtMostOneFunctionCall.expected | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected index ab8320ef8f..673e570631 100644 --- a/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected +++ b/cpp/misra/test/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.expected @@ -1,4 +1,4 @@ -| test.cpp:28:3:37:3 | switch (...) ... | Switch statement contains a statement that that is not a simple declaration. | +| test.cpp:28:3:37:3 | switch (...) ... | Switch statement contains a statement that is not a simple declaration. | | test.cpp:51:3:60:3 | switch (...) ... | Switch statement contains a switch label that is not directly within the switch body. | | test.cpp:62:3:71:3 | switch (...) ... | Switch statement contains a switch label that is not directly within the switch body. | | test.cpp:75:3:85:3 | switch (...) ... | Switch statement contains a statement label that is not a case label. | diff --git a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected index 8b0fcb1d4e..fdd088bf35 100644 --- a/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected +++ b/cpp/misra/test/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.expected @@ -1,5 +1,5 @@ | test.cpp:48:3:49:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:48:17:48:27 | call to processData | initializer | -| test.cpp:56:3:59:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:57:8:57:32 | call to vector | initializer | +| test.cpp:56:3:59:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:56:17:57:19 | call to vector | initializer | | test.cpp:71:3:73:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:71:17:72:21 | call to MyContainer | initializer | | test.cpp:95:3:97:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:95:26:95:26 | call to operator+ | initializer | | test.cpp:99:3:101:3 | for(...:...) ... | Range-based for loop has nested call expression in its $@. | test.cpp:99:31:99:31 | call to operator+ | initializer | From d37bc70d1e653fe76b509595399a0bedf7453bea Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Thu, 9 Oct 2025 15:58:43 -0400 Subject: [PATCH 40/47] Use different range predicate and update tests --- .../LegacyForStatementsShouldBeSimple.ql | 5 +- ...LegacyForStatementsShouldBeSimple.expected | 68 +++++++++---------- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 3 +- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index f34b8aec53..d63e8b4890 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -187,8 +187,9 @@ private newtype TAlertType = forLoopCondition = forLoop.getCondition() and exists(Expr loopCounter, Expr loopBound | loopCounter = forLoopCondition.getLoopCounter() and - loopBound = forLoopCondition.getLoopBound() and - upperBound(loopCounter.getFullyConverted()) < upperBound(loopBound.getFullyConverted()) + loopBound = forLoopCondition.getLoopBound() + | + typeUpperBound(loopCounter.getType()) < upperBound(loopBound.getFullyConverted()) ) } or /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ diff --git a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected index 1c85002d8c..d40663abf5 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected +++ b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected @@ -1,36 +1,32 @@ -| test2.cpp:8:5:11:5 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test2.cpp:8:14:8:14 | i | counter variable | test2.cpp:9:9:9:9 | call to f | expression | -| test2.cpp:8:5:11:5 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test2.cpp:8:14:8:14 | i | counter variable | test2.cpp:8:14:8:14 | i | N/A | -| test.cpp:22:3:24:3 | for(...;...;...) ... | The $@ is not of an integer type. | test.cpp:22:14:22:14 | i | counter variable | test.cpp:22:14:22:14 | i | N/A | -| test.cpp:31:3:33:3 | for(...;...;...) ... | The $@ does not compare the counter variable to an expression using a relational operator. | test.cpp:31:19:31:25 | ... == ... | loop condition | test.cpp:31:19:31:25 | ... == ... | N/A | -| test.cpp:35:3:37:3 | for(...;...;...) ... | The $@ does not compare the counter variable to an expression using a relational operator. | test.cpp:35:19:35:24 | ... < ... | loop condition | test.cpp:35:19:35:24 | ... < ... | N/A | -| test.cpp:76:3:78:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:76:12:76:12 | i | counter variable | test.cpp:77:8:77:13 | ... *= ... | expression | -| test.cpp:90:3:93:3 | for(...;...;...) ... | The $@ has a smaller type than that of the $@. | test.cpp:90:19:90:19 | i | counter variable | test.cpp:90:23:90:27 | 10 | loop bound | -| test.cpp:115:3:118:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:116:13:116:13 | j | loop step | test.cpp:117:5:117:7 | ... ++ | mutated | -| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:121:13:121:13 | j | loop step | test.cpp:121:8:121:18 | ... , ... | mutated | -| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:121:13:121:13 | j | loop step | test.cpp:121:16:121:18 | ... ++ | mutated | -| test.cpp:120:3:122:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:120:12:120:12 | i | counter variable | test.cpp:121:8:121:18 | ... , ... | expression | -| test.cpp:132:3:135:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:132:23:132:23 | j | loop bound | test.cpp:134:5:134:7 | ... ++ | mutated | -| test.cpp:139:3:142:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:139:23:139:23 | k | loop bound | test.cpp:141:5:141:26 | ... += ... | mutated | -| test.cpp:144:3:147:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:145:13:145:13 | l | loop step | test.cpp:146:5:146:26 | ... += ... | mutated | -| test.cpp:149:3:151:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:149:23:149:24 | call to h1 | loop bound | test.cpp:149:23:149:24 | call to h1 | non-const expression | -| test.cpp:157:3:159:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:158:13:158:14 | call to h1 | loop step | test.cpp:158:13:158:14 | call to h1 | non-const expression | -| test.cpp:172:3:175:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:172:19:172:19 | i | loop counter | test.cpp:172:19:172:19 | i | N/A | -| test.cpp:177:3:180:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:177:19:177:19 | i | loop counter | test.cpp:177:19:177:19 | i | N/A | -| test.cpp:192:3:195:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:192:23:192:23 | k | loop bound | test.cpp:192:23:192:23 | k | N/A | -| test.cpp:197:3:200:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:197:23:197:23 | k | loop bound | test.cpp:197:23:197:23 | k | N/A | -| test.cpp:212:3:215:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:212:31:212:31 | l | loop step | test.cpp:212:31:212:31 | l | N/A | -| test.cpp:217:3:220:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:217:31:217:31 | l | loop step | test.cpp:217:31:217:31 | l | N/A | -| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:232:12:232:12 | i | counter variable | test.cpp:234:5:234:6 | call to f1 | expression | -| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:232:19:232:19 | i | loop counter | test.cpp:232:19:232:19 | i | N/A | -| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:232:12:232:12 | i | counter variable | test.cpp:232:12:232:12 | i | N/A | -| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:237:12:237:12 | i | counter variable | test.cpp:239:5:239:6 | call to g1 | expression | -| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:237:19:237:19 | i | loop counter | test.cpp:237:19:237:19 | i | N/A | -| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:237:12:237:12 | i | counter variable | test.cpp:237:12:237:12 | i | N/A | -| test.cpp:252:3:255:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:252:23:252:23 | k | loop bound | test.cpp:254:5:254:6 | call to f1 | mutated | -| test.cpp:252:3:255:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:252:23:252:23 | k | loop bound | test.cpp:252:23:252:23 | k | N/A | -| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:257:23:257:23 | k | loop bound | test.cpp:259:5:259:6 | call to g1 | mutated | -| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:257:23:257:23 | k | loop bound | test.cpp:257:23:257:23 | k | N/A | -| test.cpp:273:3:276:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:273:31:273:31 | l | loop step | test.cpp:275:5:275:6 | call to f1 | mutated | -| test.cpp:273:3:276:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:273:31:273:31 | l | loop step | test.cpp:273:31:273:31 | l | N/A | -| test.cpp:278:3:281:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that is $@ in the loop. | test.cpp:278:31:278:31 | l | loop step | test.cpp:280:5:280:6 | call to g1 | mutated | -| test.cpp:278:3:281:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:278:31:278:31 | l | loop step | test.cpp:278:31:278:31 | l | N/A | +| test.cpp:23:3:25:3 | for(...;...;...) ... | The $@ is not of an integer type. | test.cpp:23:14:23:14 | i | counter variable | test.cpp:23:14:23:14 | i | N/A | +| test.cpp:32:3:34:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:32:19:32:25 | ... == ... | loop condition | test.cpp:32:19:32:25 | ... == ... | N/A | +| test.cpp:36:3:38:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:36:19:36:24 | ... < ... | loop condition | test.cpp:36:19:36:24 | ... < ... | N/A | +| test.cpp:77:3:79:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:77:12:77:12 | i | counter variable | test.cpp:78:8:78:13 | ... *= ... | expression | +| test.cpp:91:3:94:3 | for(...;...;...) ... | The $@ has a smaller type than that of the $@. | test.cpp:91:21:91:21 | i | counter variable | test.cpp:91:25:91:53 | call to max | loop bound | +| test.cpp:116:3:119:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:117:13:117:13 | j | loop step | test.cpp:118:5:118:7 | ... ++ | mutated | +| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:122:13:122:13 | j | loop step | test.cpp:122:8:122:18 | ... , ... | mutated | +| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:122:13:122:13 | j | loop step | test.cpp:122:16:122:18 | ... ++ | mutated | +| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:121:12:121:12 | i | counter variable | test.cpp:122:8:122:18 | ... , ... | expression | +| test.cpp:133:3:136:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:133:23:133:23 | j | loop bound | test.cpp:135:5:135:7 | ... ++ | mutated | +| test.cpp:140:3:143:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:140:23:140:23 | k | loop bound | test.cpp:142:5:142:26 | ... += ... | mutated | +| test.cpp:145:3:148:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:146:13:146:13 | l | loop step | test.cpp:147:5:147:26 | ... += ... | mutated | +| test.cpp:150:3:152:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:150:23:150:24 | call to h1 | loop bound | test.cpp:150:23:150:24 | call to h1 | non-const expression | +| test.cpp:158:3:160:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:159:13:159:14 | call to h1 | loop step | test.cpp:159:13:159:14 | call to h1 | non-const expression | +| test.cpp:173:3:176:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:173:19:173:19 | i | loop counter | test.cpp:173:19:173:19 | i | N/A | +| test.cpp:178:3:181:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:178:19:178:19 | i | loop counter | test.cpp:178:19:178:19 | i | N/A | +| test.cpp:193:3:196:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:193:23:193:23 | k | loop bound | test.cpp:193:23:193:23 | k | N/A | +| test.cpp:198:3:201:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:198:23:198:23 | k | loop bound | test.cpp:198:23:198:23 | k | N/A | +| test.cpp:213:3:216:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:213:31:213:31 | l | loop step | test.cpp:213:31:213:31 | l | N/A | +| test.cpp:218:3:221:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:218:31:218:31 | l | loop step | test.cpp:218:31:218:31 | l | N/A | +| test.cpp:233:3:236:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:233:19:233:19 | i | loop counter | test.cpp:233:19:233:19 | i | N/A | +| test.cpp:233:3:236:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:233:12:233:12 | i | counter variable | test.cpp:233:12:233:12 | i | N/A | +| test.cpp:238:3:241:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:238:19:238:19 | i | loop counter | test.cpp:238:19:238:19 | i | N/A | +| test.cpp:238:3:241:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:238:12:238:12 | i | counter variable | test.cpp:238:12:238:12 | i | N/A | +| test.cpp:253:3:256:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:253:23:253:23 | k | loop bound | test.cpp:255:5:255:6 | call to f1 | mutated | +| test.cpp:253:3:256:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:253:23:253:23 | k | loop bound | test.cpp:253:23:253:23 | k | N/A | +| test.cpp:258:3:261:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:258:23:258:23 | k | loop bound | test.cpp:260:5:260:6 | call to g1 | mutated | +| test.cpp:258:3:261:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:258:23:258:23 | k | loop bound | test.cpp:258:23:258:23 | k | N/A | +| test.cpp:274:3:277:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:274:31:274:31 | l | loop step | test.cpp:276:5:276:6 | call to f1 | mutated | +| test.cpp:274:3:277:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:274:31:274:31 | l | loop step | test.cpp:274:31:274:31 | l | N/A | +| test.cpp:279:3:282:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:279:31:279:31 | l | loop step | test.cpp:281:5:281:6 | call to g1 | mutated | +| test.cpp:279:3:282:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:279:31:279:31 | l | loop step | test.cpp:279:31:279:31 | l | N/A | diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index cb6905525e..6e7e09870b 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -1,3 +1,4 @@ +#include #include #include @@ -87,7 +88,7 @@ int main() { // loop bound } - for (int i = 0; i < 10ull; + for (short i = 0; i < std::numeric_limits::max(); i++) { // NON_COMPLIANT: The type of the loop counter is not bigger // than that of the loop bound } From 25e29e48e4daf0bd97c3059dff85861784b6ef3c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Fri, 10 Oct 2025 17:59:52 -0400 Subject: [PATCH 41/47] Refine `TLoopCounterMutatedInLoopBody` This refined definition can handle more cases than the previous one that only looked into the loop body, and better matches the description in the comment above. --- .../rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index d63e8b4890..e2d4e1d16e 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -170,8 +170,9 @@ private newtype TAlertType = /* 3-1. The loop counter is mutated somewhere other than its update expression. */ TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounterVariable) { loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and - variableModifiedInExpression(forLoop.getStmt().getChildStmt().getAChild*(), - loopCounterVariable.getAnAccess()) + exists(Expr mutatingExpr | not mutatingExpr = forLoop.getUpdate().getAChild*() | + variableModifiedInExpression(mutatingExpr, loopCounterVariable.getAnAccess()) + ) } or /* 3-2. The loop counter is not updated using either of `++`, `--`, `+=`, or `-=`. */ TLoopCounterUpdatedNotByCrementOrAddSubAssignmentExpr( From 01216fa1cdd0b4e8a01bedd09b8ab52e53453c61 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 13 Oct 2025 16:17:54 -0400 Subject: [PATCH 42/47] Add more candidate exprs and remove duplicate reportings on compound mutating exprs --- .../LegacyForStatementsShouldBeSimple.ql | 80 ++++++++++++------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index e2d4e1d16e..6de2d18de3 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -194,16 +194,28 @@ private newtype TAlertType = ) } or /* 5-1-1. The loop bound is a variable that is mutated in the for loop. */ - TLoopBoundIsMutatedVariableAccess(ForStmt forLoop, Expr loopBound, Expr mutatingExpr) { - loopBound = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and - ( - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - ) and - variableModifiedInExpression(mutatingExpr, loopBound.(VariableAccess).getTarget().getAnAccess()) + TLoopBoundIsMutatedVariableAccess( + ForStmt forLoop, VariableAccess variableAccess, VariableAccess mutatedVariableAccess + ) { + exists(Expr loopBoundExpr, Expr mutatingExpr | + loopBoundExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and + ( + /* 1. The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* 2. The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + or + /* 3. The mutating expression may be in the loop condition */ + mutatingExpr = forLoop.getCondition().getAChild*() + or + /* 4. The mutating expression may be in the loop initializer */ + mutatingExpr = forLoop.getInitialization().getAChild*() + ) and + variableAccess = loopBoundExpr.getAChild*() and + mutatedVariableAccess = variableAccess.getTarget().getAnAccess() and + variableModifiedInExpression(mutatingExpr, mutatedVariableAccess) + ) } or /* 5-1-2. The loop bound is not a variable access nor a constant expression. */ TLoopBoundIsNonConstExpr(ForStmt forLoop, Expr loopBound) { @@ -211,16 +223,28 @@ private newtype TAlertType = (not loopBound instanceof VariableAccess and not loopBound.isConstant()) } or /* 5-2-1. The loop step is a variable that is mutated in the for loop. */ - TLoopStepIsMutatedVariableAccess(ForStmt forLoop, Expr loopStep, Expr mutatingExpr) { - loopStep = getLoopStepOfForStmt(forLoop) and - ( - /* The mutating expression may be in the loop body. */ - mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() - or - /* The mutating expression may be in the loop updating expression. */ - mutatingExpr = forLoop.getUpdate().getAChild*() - ) and - variableModifiedInExpression(mutatingExpr, loopStep.(VariableAccess).getTarget().getAnAccess()) + TLoopStepIsMutatedVariableAccess( + ForStmt forLoop, VariableAccess variableAccess, VariableAccess mutatedVariableAccess + ) { + exists(Expr loopStepExpr, Expr mutatingExpr | + loopStepExpr = getLoopStepOfForStmt(forLoop) and + ( + /* 1. The mutating expression may be in the loop body. */ + mutatingExpr = forLoop.getStmt().getChildStmt().getAChild*() + or + /* 2. The mutating expression may be in the loop updating expression. */ + mutatingExpr = forLoop.getUpdate().getAChild*() + or + /* 3. The mutating expression may be in the loop condition */ + mutatingExpr = forLoop.getCondition().getAChild*() + or + /* 4. The mutating expression may be in the loop initializer */ + mutatingExpr = forLoop.getInitialization().getAChild*() + ) and + variableAccess = loopStepExpr.getAChild*() and + mutatedVariableAccess = variableAccess.getTarget().getAnAccess() and + variableModifiedInExpression(mutatingExpr, mutatedVariableAccess) + ) } or /* 5-2-2. The loop step is not a variable access nor a constant expression. */ TLoopStepIsNonConstExpr(ForStmt forLoop, Expr loopStep) { @@ -244,13 +268,15 @@ private newtype TAlertType = * 6-2. The loop bound is taken as a mutable reference or its address to a mutable pointer. */ - TLoopBoundIsTakenNonConstAddress(ForStmt forLoop, Expr loopVariableAccessInCondition) { - loopVariableAccessInCondition = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and - ( - loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, loopVariableAccessInCondition) - or - loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, - loopVariableAccessInCondition) + TLoopBoundIsTakenNonConstAddress(ForStmt forLoop, Expr loopBoundExpr) { + loopBoundExpr = forLoop.getCondition().(LegacyForLoopCondition).getLoopBound() and + exists(VariableAccess variableAccess | + variableAccess = loopBoundExpr.getAChild*() and + ( + loopVariableAssignedToNonConstPointerOrReferenceType(forLoop, variableAccess) + or + loopVariablePassedAsArgumentToNonConstReferenceParameter(forLoop, variableAccess) + ) ) } or /* From 0f998ea7b1ec8087c12d001693c0d4cd34288f8c Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 13 Oct 2025 16:18:46 -0400 Subject: [PATCH 43/47] Add more test cases --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 6e7e09870b..11dfb51b38 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -135,6 +135,14 @@ int main() { j++; } + for (int i = 0; i < j++; + i++) { // NON_COMPLIANT: The loop bound `j` is mutated in the loop + } + + for (int i = 0; i++ < j++; + i++) { // NON_COMPLIANT: The loop bound `j` is mutated in the loop + } + int n = 0; for (int i = 0; i < k; From 87a515886bfb3b414eb6020e402c3139d8d094ee Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 13 Oct 2025 17:17:15 -0400 Subject: [PATCH 44/47] Fix loop counter -> loop bound --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 11dfb51b38..81894406be 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -208,12 +208,12 @@ int main() { int *m = &k; } - for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + for (int i = j; i < k; i += l) { // COMPLIANT: The loop bound is taken // as a const reference const int &m = k; } - for (int i = j; i < k; i += l) { // COMPLIANT: The loop counter is taken + for (int i = j; i < k; i += l) { // COMPLIANT: The loop bound is taken // as a const pointer const int *m = &k; } From 2aed0f125fcc52d81a55285a8271c69fd82eaf74 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Mon, 13 Oct 2025 17:31:17 -0400 Subject: [PATCH 45/47] Add const-but-mutable pointer examples --- cpp/misra/test/rules/RULE-9-5-1/test.cpp | 31 ++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/cpp/misra/test/rules/RULE-9-5-1/test.cpp b/cpp/misra/test/rules/RULE-9-5-1/test.cpp index 81894406be..59f077c5bc 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/test.cpp +++ b/cpp/misra/test/rules/RULE-9-5-1/test.cpp @@ -6,6 +6,7 @@ void f1(int &x) {} // Function that takes a non-const integer reference void g1(int *x) {} // Function that takes a non-const integer pointer void f2(const int &x) {} // Function that takes a non-const integer reference void g2(const int *x) {} // Function that takes a non-const integer pointer +void f3(int *const x) {} int h1() { return 1; } constexpr int h2() { return 1; } @@ -198,6 +199,11 @@ int main() { const int *m = &i; } + for (int i = j; i < k; i += l) { // NON-COMPLIANT: The loop counter is taken + // as a const but mutable pointer + int *const m = &i; + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is taken as // a non-const reference int &m = k; @@ -218,6 +224,11 @@ int main() { const int *m = &k; } + for (int i = j; i < k; i += l) { // NON-COMPLIANT: The loop bound is taken as + // a const but mutable pointer + int *const m = &k; + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is taken as // a non-const reference int &m = l; @@ -238,6 +249,11 @@ int main() { const int *m = &l; } + for (int i = j; i < k; i += l) { // NON-COMPLIANT: The loop step is taken as + // a const but mutable pointer + int *const m = &l; + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed // to a non-const reference parameter f1(i); @@ -258,6 +274,11 @@ int main() { g2(&i); } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop counter is passed + // to a const but mutable pointer parameter + f3(&i); + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is passed to // a non-const reference parameter f1(k); @@ -279,6 +300,11 @@ int main() { g2(&k); } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop bound is passed to + // a const but mutable pointer parameter + f3(&k); + } + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to // a non-const reference parameter f1(l); @@ -298,4 +324,9 @@ int main() { // a const pointer parameter g2(&l); } + + for (int i = j; i < k; i += l) { // NON_COMPLIANT: The loop step is passed to + // a const but mutable pointer parameter + f3(&l); + } } \ No newline at end of file From 33222aef74c5fe67d70c7b685f3033cff04f8a21 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 14 Oct 2025 11:23:34 -0400 Subject: [PATCH 46/47] Debug loopVariableAssignedToNonConstPointOrReferenceType This is to cover the cases where the pointers are constant but the data behind it can be mutated through it. --- .../rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql index 6de2d18de3..8d44c50bdf 100644 --- a/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql +++ b/cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql @@ -90,8 +90,8 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( strippedType = targetType.stripTopLevelSpecifiers() and not strippedType.getBaseType().isConst() and ( - targetType instanceof PointerType or - targetType instanceof ReferenceType + strippedType instanceof PointerType or + strippedType instanceof ReferenceType ) | assignmentRhs.getEnclosingStmt().getParent*() = forLoop.getStmt() and @@ -118,7 +118,7 @@ predicate loopVariableAssignedToNonConstPointerOrReferenceType( * This predicate adds two constraints to the target type, as compared to the original * portion of the predicate: * - * 1. This predicate adds type constraint that the target type is a `ReferenceType`. + * 1. This predicate adds a type constraint that the target type is a `ReferenceType`. * 2. This predicate adds the constraint that the target type is not `const`. * * Also, this predicate requires that the call is the body of the given for-loop. From 3dca053db68f77a0df9aee06bb1ddff441d251e1 Mon Sep 17 00:00:00 2001 From: Jeongsoo Lee Date: Tue, 14 Oct 2025 13:53:34 -0400 Subject: [PATCH 47/47] Update expected results of 9-5-1 --- ...LegacyForStatementsShouldBeSimple.expected | 77 +++++++++++-------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected index d40663abf5..a2b79b724d 100644 --- a/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected +++ b/cpp/misra/test/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.expected @@ -1,32 +1,45 @@ -| test.cpp:23:3:25:3 | for(...;...;...) ... | The $@ is not of an integer type. | test.cpp:23:14:23:14 | i | counter variable | test.cpp:23:14:23:14 | i | N/A | -| test.cpp:32:3:34:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:32:19:32:25 | ... == ... | loop condition | test.cpp:32:19:32:25 | ... == ... | N/A | -| test.cpp:36:3:38:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:36:19:36:24 | ... < ... | loop condition | test.cpp:36:19:36:24 | ... < ... | N/A | -| test.cpp:77:3:79:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:77:12:77:12 | i | counter variable | test.cpp:78:8:78:13 | ... *= ... | expression | -| test.cpp:91:3:94:3 | for(...;...;...) ... | The $@ has a smaller type than that of the $@. | test.cpp:91:21:91:21 | i | counter variable | test.cpp:91:25:91:53 | call to max | loop bound | -| test.cpp:116:3:119:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:117:13:117:13 | j | loop step | test.cpp:118:5:118:7 | ... ++ | mutated | -| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:122:13:122:13 | j | loop step | test.cpp:122:8:122:18 | ... , ... | mutated | -| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:122:13:122:13 | j | loop step | test.cpp:122:16:122:18 | ... ++ | mutated | -| test.cpp:121:3:123:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:121:12:121:12 | i | counter variable | test.cpp:122:8:122:18 | ... , ... | expression | -| test.cpp:133:3:136:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:133:23:133:23 | j | loop bound | test.cpp:135:5:135:7 | ... ++ | mutated | -| test.cpp:140:3:143:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:140:23:140:23 | k | loop bound | test.cpp:142:5:142:26 | ... += ... | mutated | -| test.cpp:145:3:148:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:146:13:146:13 | l | loop step | test.cpp:147:5:147:26 | ... += ... | mutated | -| test.cpp:150:3:152:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:150:23:150:24 | call to h1 | loop bound | test.cpp:150:23:150:24 | call to h1 | non-const expression | -| test.cpp:158:3:160:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:159:13:159:14 | call to h1 | loop step | test.cpp:159:13:159:14 | call to h1 | non-const expression | -| test.cpp:173:3:176:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:173:19:173:19 | i | loop counter | test.cpp:173:19:173:19 | i | N/A | -| test.cpp:178:3:181:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:178:19:178:19 | i | loop counter | test.cpp:178:19:178:19 | i | N/A | -| test.cpp:193:3:196:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:193:23:193:23 | k | loop bound | test.cpp:193:23:193:23 | k | N/A | -| test.cpp:198:3:201:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:198:23:198:23 | k | loop bound | test.cpp:198:23:198:23 | k | N/A | -| test.cpp:213:3:216:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:213:31:213:31 | l | loop step | test.cpp:213:31:213:31 | l | N/A | -| test.cpp:218:3:221:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:218:31:218:31 | l | loop step | test.cpp:218:31:218:31 | l | N/A | -| test.cpp:233:3:236:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:233:19:233:19 | i | loop counter | test.cpp:233:19:233:19 | i | N/A | -| test.cpp:233:3:236:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:233:12:233:12 | i | counter variable | test.cpp:233:12:233:12 | i | N/A | -| test.cpp:238:3:241:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:238:19:238:19 | i | loop counter | test.cpp:238:19:238:19 | i | N/A | -| test.cpp:238:3:241:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:238:12:238:12 | i | counter variable | test.cpp:238:12:238:12 | i | N/A | -| test.cpp:253:3:256:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:253:23:253:23 | k | loop bound | test.cpp:255:5:255:6 | call to f1 | mutated | -| test.cpp:253:3:256:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:253:23:253:23 | k | loop bound | test.cpp:253:23:253:23 | k | N/A | -| test.cpp:258:3:261:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:258:23:258:23 | k | loop bound | test.cpp:260:5:260:6 | call to g1 | mutated | -| test.cpp:258:3:261:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:258:23:258:23 | k | loop bound | test.cpp:258:23:258:23 | k | N/A | -| test.cpp:274:3:277:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:274:31:274:31 | l | loop step | test.cpp:276:5:276:6 | call to f1 | mutated | -| test.cpp:274:3:277:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:274:31:274:31 | l | loop step | test.cpp:274:31:274:31 | l | N/A | -| test.cpp:279:3:282:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:279:31:279:31 | l | loop step | test.cpp:281:5:281:6 | call to g1 | mutated | -| test.cpp:279:3:282:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:279:31:279:31 | l | loop step | test.cpp:279:31:279:31 | l | N/A | +| test.cpp:24:3:26:3 | for(...;...;...) ... | The $@ is not of an integer type. | test.cpp:24:14:24:14 | i | counter variable | test.cpp:24:14:24:14 | i | N/A | +| test.cpp:33:3:35:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:33:19:33:25 | ... == ... | loop condition | test.cpp:33:19:33:25 | ... == ... | N/A | +| test.cpp:37:3:39:3 | for(...;...;...) ... | The $@ does not determine termination based only on a comparison against the value of the counter variable. | test.cpp:37:19:37:24 | ... < ... | loop condition | test.cpp:37:19:37:24 | ... < ... | N/A | +| test.cpp:78:3:80:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:78:12:78:12 | i | counter variable | test.cpp:79:8:79:13 | ... *= ... | expression | +| test.cpp:92:3:95:3 | for(...;...;...) ... | The $@ has a smaller type than that of the $@. | test.cpp:92:21:92:21 | i | counter variable | test.cpp:92:25:92:53 | call to max | loop bound | +| test.cpp:117:3:120:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:118:13:118:13 | j | loop step | test.cpp:119:5:119:5 | j | mutated | +| test.cpp:122:3:124:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:123:13:123:13 | j | loop step | test.cpp:123:16:123:16 | j | mutated | +| test.cpp:122:3:124:3 | for(...;...;...) ... | The $@ is not updated with an $@ other than addition or subtraction. | test.cpp:122:12:122:12 | i | counter variable | test.cpp:123:8:123:18 | ... , ... | expression | +| test.cpp:134:3:137:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:134:23:134:23 | j | loop bound | test.cpp:136:5:136:5 | j | mutated | +| test.cpp:139:3:141:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:139:23:139:25 | ... ++ | loop bound | test.cpp:139:23:139:25 | ... ++ | non-const expression | +| test.cpp:139:3:141:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:139:23:139:23 | j | loop bound | test.cpp:139:23:139:23 | j | mutated | +| test.cpp:143:3:145:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:143:25:143:27 | ... ++ | loop bound | test.cpp:143:25:143:27 | ... ++ | non-const expression | +| test.cpp:143:3:145:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:143:25:143:25 | j | loop bound | test.cpp:143:25:143:25 | j | mutated | +| test.cpp:143:3:145:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:143:12:143:12 | i | counter variable | test.cpp:143:12:143:12 | i | N/A | +| test.cpp:149:3:152:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:149:23:149:23 | k | loop bound | test.cpp:151:15:151:15 | k | mutated | +| test.cpp:154:3:157:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:155:13:155:13 | l | loop step | test.cpp:156:15:156:15 | l | mutated | +| test.cpp:159:3:161:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:159:23:159:24 | call to h1 | loop bound | test.cpp:159:23:159:24 | call to h1 | non-const expression | +| test.cpp:167:3:169:3 | for(...;...;...) ... | The $@ is a $@. | test.cpp:168:13:168:14 | call to h1 | loop step | test.cpp:168:13:168:14 | call to h1 | non-const expression | +| test.cpp:182:3:185:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:182:19:182:19 | i | loop counter | test.cpp:182:19:182:19 | i | N/A | +| test.cpp:187:3:190:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:187:19:187:19 | i | loop counter | test.cpp:187:19:187:19 | i | N/A | +| test.cpp:202:3:205:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:202:19:202:19 | i | loop counter | test.cpp:202:19:202:19 | i | N/A | +| test.cpp:207:3:210:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:207:23:207:23 | k | loop bound | test.cpp:207:23:207:23 | k | N/A | +| test.cpp:212:3:215:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:212:23:212:23 | k | loop bound | test.cpp:212:23:212:23 | k | N/A | +| test.cpp:227:3:230:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:227:23:227:23 | k | loop bound | test.cpp:227:23:227:23 | k | N/A | +| test.cpp:232:3:235:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:232:31:232:31 | l | loop step | test.cpp:232:31:232:31 | l | N/A | +| test.cpp:237:3:240:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:237:31:237:31 | l | loop step | test.cpp:237:31:237:31 | l | N/A | +| test.cpp:252:3:255:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:252:31:252:31 | l | loop step | test.cpp:252:31:252:31 | l | N/A | +| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:257:19:257:19 | i | loop counter | test.cpp:257:19:257:19 | i | N/A | +| test.cpp:257:3:260:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:257:12:257:12 | i | counter variable | test.cpp:257:12:257:12 | i | N/A | +| test.cpp:262:3:265:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:262:19:262:19 | i | loop counter | test.cpp:262:19:262:19 | i | N/A | +| test.cpp:262:3:265:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:262:12:262:12 | i | counter variable | test.cpp:262:12:262:12 | i | N/A | +| test.cpp:277:3:280:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:277:19:277:19 | i | loop counter | test.cpp:277:19:277:19 | i | N/A | +| test.cpp:277:3:280:3 | for(...;...;...) ... | The $@ may be mutated in a location other than its update expression. | test.cpp:277:12:277:12 | i | counter variable | test.cpp:277:12:277:12 | i | N/A | +| test.cpp:282:3:285:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:282:23:282:23 | k | loop bound | test.cpp:284:8:284:8 | k | mutated | +| test.cpp:282:3:285:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:282:23:282:23 | k | loop bound | test.cpp:282:23:282:23 | k | N/A | +| test.cpp:287:3:290:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:287:23:287:23 | k | loop bound | test.cpp:289:9:289:9 | k | mutated | +| test.cpp:287:3:290:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:287:23:287:23 | k | loop bound | test.cpp:287:23:287:23 | k | N/A | +| test.cpp:303:3:306:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:303:23:303:23 | k | loop bound | test.cpp:305:9:305:9 | k | mutated | +| test.cpp:303:3:306:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:303:23:303:23 | k | loop bound | test.cpp:303:23:303:23 | k | N/A | +| test.cpp:308:3:311:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:308:31:308:31 | l | loop step | test.cpp:310:8:310:8 | l | mutated | +| test.cpp:308:3:311:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:308:31:308:31 | l | loop step | test.cpp:308:31:308:31 | l | N/A | +| test.cpp:313:3:316:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:313:31:313:31 | l | loop step | test.cpp:315:9:315:9 | l | mutated | +| test.cpp:313:3:316:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:313:31:313:31 | l | loop step | test.cpp:313:31:313:31 | l | N/A | +| test.cpp:328:3:331:3 | for(...;...;...) ... | The $@ is a non-const expression, or a variable that may be $@ in the loop. | test.cpp:328:31:328:31 | l | loop step | test.cpp:330:9:330:9 | l | mutated | +| test.cpp:328:3:331:3 | for(...;...;...) ... | The $@ is taken as a mutable reference or its address to a mutable pointer. | test.cpp:328:31:328:31 | l | loop step | test.cpp:328:31:328:31 | l | N/A |