From 6c4a79d1eaa49edcd214820efcc402afa16294d6 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 23 Feb 2024 15:39:18 -0800 Subject: [PATCH 1/4] Add boilerplate for new rule --- .vscode/tasks.json | 1 + .../M5-0-2/InsufficientUseOfParentheses.ql | 23 +++++++++++++++++++ .../InsufficientUseOfParentheses.expected | 1 + .../M5-0-2/InsufficientUseOfParentheses.qlref | 1 + .../cpp/exclusions/cpp/OrderOfEvaluation.qll | 17 ++++++++++++++ rule_packages/cpp/OrderOfEvaluation.json | 12 ++++++++++ 6 files changed, 55 insertions(+) create mode 100644 cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql create mode 100644 cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected create mode 100644 cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.qlref diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 9b53539c04..74f065ac3b 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -271,6 +271,7 @@ "Null", "OperatorInvariants", "Operators", + "OrderOfEvaluation", "OutOfBounds", "Pointers", "Pointers1", diff --git a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql new file mode 100644 index 0000000000..0d378e5462 --- /dev/null +++ b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql @@ -0,0 +1,23 @@ +/** + * @id cpp/autosar/insufficient-use-of-parentheses + * @name M5-0-2: Limited dependence should be placed on C++ operator precedence rules in expressions + * @description The use of parentheses can be used to emphasize precedence and increase code + * readability. + * @kind problem + * @precision medium + * @problem.severity recommendation + * @tags external/autosar/id/m5-0-2 + * external/autosar/audit + * readability + * external/autosar/allocated-target/implementation + * external/autosar/enforcement/partially-automated + * external/autosar/obligation/advisory + */ + +import cpp +import codingstandards.cpp.autosar + +from Expr e +where + not isExcluded(e, OrderOfEvaluationPackage::insufficientUseOfParenthesesQuery()) +select e, "Insufficient use of parenthesis in expression." diff --git a/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.qlref b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.qlref new file mode 100644 index 0000000000..733c035604 --- /dev/null +++ b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.qlref @@ -0,0 +1 @@ +rules/M5-0-2/InsufficientUseOfParentheses.ql \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/OrderOfEvaluation.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/OrderOfEvaluation.qll index 2c7da3d64a..9aa62ad377 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/cpp/OrderOfEvaluation.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/OrderOfEvaluation.qll @@ -8,6 +8,7 @@ newtype OrderOfEvaluationQuery = TOperandsOfALogicalAndOrNotParenthesizedQuery() or TExplicitConstructionOfUnnamedTemporaryQuery() or TGratuitousUseOfParenthesesQuery() or + TInsufficientUseOfParenthesesQuery() or TIncrementAndDecrementOperatorsMixedWithOtherOperatorsInExpressionQuery() or TAssignmentInSubExpressionQuery() @@ -50,6 +51,15 @@ predicate isOrderOfEvaluationQueryMetadata( ruleId = "M5-0-2" and category = "advisory" or + query = + // `Query` instance for the `insufficientUseOfParentheses` query + OrderOfEvaluationPackage::insufficientUseOfParenthesesQuery() and + queryId = + // `@id` for the `insufficientUseOfParentheses` query + "cpp/autosar/insufficient-use-of-parentheses" and + ruleId = "M5-0-2" and + category = "advisory" + or query = // `Query` instance for the `incrementAndDecrementOperatorsMixedWithOtherOperatorsInExpression` query OrderOfEvaluationPackage::incrementAndDecrementOperatorsMixedWithOtherOperatorsInExpressionQuery() and @@ -98,6 +108,13 @@ module OrderOfEvaluationPackage { TQueryCPP(TOrderOfEvaluationPackageQuery(TGratuitousUseOfParenthesesQuery())) } + Query insufficientUseOfParenthesesQuery() { + //autogenerate `Query` type + result = + // `Query` type for `insufficientUseOfParentheses` query + TQueryCPP(TOrderOfEvaluationPackageQuery(TInsufficientUseOfParenthesesQuery())) + } + Query incrementAndDecrementOperatorsMixedWithOtherOperatorsInExpressionQuery() { //autogenerate `Query` type result = diff --git a/rule_packages/cpp/OrderOfEvaluation.json b/rule_packages/cpp/OrderOfEvaluation.json index c471ca8f48..00ec0dbc65 100644 --- a/rule_packages/cpp/OrderOfEvaluation.json +++ b/rule_packages/cpp/OrderOfEvaluation.json @@ -90,6 +90,18 @@ "external/autosar/audit", "readability" ] + }, + { + "description": "The use of parentheses can be used to emphasize precedence and increase code readability.", + "kind": "problem", + "name": "Limited dependence should be placed on C++ operator precedence rules in expressions", + "precision": "medium", + "severity": "recommendation", + "short_name": "InsufficientUseOfParentheses", + "tags": [ + "external/autosar/audit", + "readability" + ] } ], "title": "Limited dependence should be placed on C++ operator precedence rules in expressions." From ef44566dd5cd29a729b78af0cf263d2b0dffa472 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 29 Feb 2024 17:05:46 -0800 Subject: [PATCH 2/4] Implement query for dependence on operator precedence --- .../M5-0-2/InsufficientUseOfParentheses.ql | 22 +++++++++++++++---- .../InsufficientUseOfParentheses.expected | 9 +++++++- cpp/autosar/test/rules/M5-0-2/test.cpp | 17 ++++++++++++++ 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql index 0d378e5462..19bd325edd 100644 --- a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql +++ b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql @@ -17,7 +17,21 @@ import cpp import codingstandards.cpp.autosar -from Expr e -where - not isExcluded(e, OrderOfEvaluationPackage::insufficientUseOfParenthesesQuery()) -select e, "Insufficient use of parenthesis in expression." +class InsufficientlyParenthesizedExpr extends Expr { + InsufficientlyParenthesizedExpr() { + exists(BinaryOperation root, BinaryOperation child | child = this | + root.getAnOperand() = child and + root.getOperator() != child.getOperator() and + not any(ParenthesisExpr pe).getExpr() = child + ) + or + exists(ConditionalExpr root, BinaryOperation child | child = this | + root.getAnOperand() = child and + not any(ParenthesisExpr pe).getExpr() = child + ) + } +} + +from InsufficientlyParenthesizedExpr e +where not isExcluded(e, OrderOfEvaluationPackage::insufficientUseOfParenthesesQuery()) +select e, "Dependence on operator precedence rules." diff --git a/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected index 2ec1a0ac6c..ef355c7306 100644 --- a/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected +++ b/cpp/autosar/test/rules/M5-0-2/InsufficientUseOfParentheses.expected @@ -1 +1,8 @@ -No expected results have yet been specified \ No newline at end of file +| test.cpp:40:8:40:13 | ... * ... | Dependence on operator precedence rules. | +| test.cpp:41:19:41:24 | ... * ... | Dependence on operator precedence rules. | +| test.cpp:42:8:42:13 | ... * ... | Dependence on operator precedence rules. | +| test.cpp:42:17:42:22 | ... * ... | Dependence on operator precedence rules. | +| test.cpp:48:8:48:15 | ... == ... | Dependence on operator precedence rules. | +| test.cpp:49:26:49:32 | ... - ... | Dependence on operator precedence rules. | +| test.cpp:50:8:50:15 | ... == ... | Dependence on operator precedence rules. | +| test.cpp:50:24:50:30 | ... - ... | Dependence on operator precedence rules. | diff --git a/cpp/autosar/test/rules/M5-0-2/test.cpp b/cpp/autosar/test/rules/M5-0-2/test.cpp index 06dab1e64c..d028b632f9 100644 --- a/cpp/autosar/test/rules/M5-0-2/test.cpp +++ b/cpp/autosar/test/rules/M5-0-2/test.cpp @@ -31,4 +31,21 @@ void f1() { int **l7; l1 = (*l7)[l2]; // NON_COMPLIANT[FALSE_NEGATIVE] char l8 = (char)(l1 + 1); // NON_COMPLIANT[FALSE_NEGATIVE] +} + +void test_insufficient_parentheses() { + int l1, l2, l3; + + l1 = (2 * l2) + (3 * l3); // COMPLIANT + l1 = 2 * l2 + (3 * l3); // NON_COMPLIANT + l1 = (2 * l2) + 3 * l3; // NON_COMPLIANT + l1 = 2 * l2 + 3 * l3; // NON_COMPLIANT + l1 = (2 * l2) + l3 + 1; // COMPLIANT + l1 = (l2 + 1) - (l2 + l3); // COMPLIANT + l1 = l2 + l3 + 1; // COMPLIANT + + l1 = (l2 == l3) ? l2 : (l2 - l3); // COMPLIANT + l1 = l2 == l3 ? l2 : (l2 - l3); // NON_COMPLIANT + l1 = (l2 == l3) ? l2 : l2 - l3; // NON_COMPLIANT + l1 = l2 == l3 ? l2 : l2 - l3; // NON_COMPLIANT } \ No newline at end of file From e49c8b94520e94966184ef64a25f2d098bbee2bc Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 5 Mar 2024 16:40:55 -0800 Subject: [PATCH 3/4] Exclude asserted expressions The way assert is implemented will result in an alert pointing to the expression being asserted. This will result in a false positive, as the expression does not need to be parenthesized. --- .../M5-0-2/InsufficientUseOfParentheses.ql | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql index 19bd325edd..bb5b1418b3 100644 --- a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql +++ b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql @@ -16,18 +16,24 @@ import cpp import codingstandards.cpp.autosar +import semmle.code.cpp.commons.Assertions class InsufficientlyParenthesizedExpr extends Expr { InsufficientlyParenthesizedExpr() { - exists(BinaryOperation root, BinaryOperation child | child = this | - root.getAnOperand() = child and - root.getOperator() != child.getOperator() and - not any(ParenthesisExpr pe).getExpr() = child - ) - or - exists(ConditionalExpr root, BinaryOperation child | child = this | - root.getAnOperand() = child and - not any(ParenthesisExpr pe).getExpr() = child + // Exclude assertions because if the child is the expression being asserted it + // is not necessary to add parenthesis. + not any(Assertion a).getAsserted() = this and + ( + exists(BinaryOperation root, BinaryOperation child | child = this | + root.getAnOperand() = child and + root.getOperator() != child.getOperator() and + not any(ParenthesisExpr pe).getExpr() = child + ) + or + exists(ConditionalExpr root, BinaryOperation child | child = this | + root.getAnOperand() = child and + not any(ParenthesisExpr pe).getExpr() = child + ) ) } } From a78dd7aca0f1da12654dc3e2ab74aafdcba4f499 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 5 Mar 2024 17:12:03 -0800 Subject: [PATCH 4/4] Generalize exclusion to macro expanded code --- .../src/rules/M5-0-2/InsufficientUseOfParentheses.ql | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql index bb5b1418b3..1dda0df93f 100644 --- a/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql +++ b/cpp/autosar/src/rules/M5-0-2/InsufficientUseOfParentheses.ql @@ -20,9 +20,10 @@ import semmle.code.cpp.commons.Assertions class InsufficientlyParenthesizedExpr extends Expr { InsufficientlyParenthesizedExpr() { - // Exclude assertions because if the child is the expression being asserted it - // is not necessary to add parenthesis. - not any(Assertion a).getAsserted() = this and + // Exclude expressions affected by macros, including assertions, because + // it is unclear that the expression must be parenthesized since it seems + // to be the top-level expression instead of an operand of a binary or ternary operation. + not this.isAffectedByMacro() and ( exists(BinaryOperation root, BinaryOperation child | child = this | root.getAnOperand() = child and