Merge pull request #1050 from github/michaelrfairhurst/side-effects-5-rule-8-14-1-rhs-of-and-and-should-have-no-side-effects

mbaluda · web-flow · commit b5a248795883 · 2026-03-02T22:38:28.000Z
Move M5-14-1 implementation into shared query with Rule 8.14.1, increase test coverage.
diff --git a/change_notes/2026-02-21-share-implementation-of-m5-14-1.md b/change_notes/2026-02-21-share-implementation-of-m5-14-1.md
@@ -0,0 +1,2 @@
+ - `M5-14-1` - `RightHandOperandOfALogicalAndOperatorsContainSideEffects.ql`:
+   - Implementation has been refactored to share logic with Rule 8.14.1. No observable changes to results expected.
diff --git a/cpp/autosar/src/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.ql b/cpp/autosar/src/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.ql
@@ -16,15 +16,14 @@
 
 import cpp
 import codingstandards.cpp.autosar
-import codingstandards.cpp.SideEffect
-import codingstandards.cpp.sideeffect.DefaultEffects
-import codingstandards.cpp.Expr
+import codingstandards.cpp.rules.shortcircuitedpersistentsideeffectshared.ShortCircuitedPersistentSideEffectShared
 
-from BinaryLogicalOperation op, Expr rhs
-where
-  not isExcluded(op,
-    SideEffects1Package::rightHandOperandOfALogicalAndOperatorsContainSideEffectsQuery()) and
-  rhs = op.getRightOperand() and
-  hasSideEffect(rhs) and
-  not rhs instanceof UnevaluatedExprExtension
-select op, "The $@ may have a side effect that is not always evaluated.", rhs, "right-hand operand"
+module RightHandOperandOfALogicalAndOperatorsContainSideEffectsConfig implements
+  ShortCircuitedPersistentSideEffectSharedConfigSig
+{
+  Query getQuery() {
+    result = SideEffects1Package::rightHandOperandOfALogicalAndOperatorsContainSideEffectsQuery()
+  }
+}
+
+import ShortCircuitedPersistentSideEffectShared<RightHandOperandOfALogicalAndOperatorsContainSideEffectsConfig>
diff --git a/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.expected b/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.expected
diff --git a/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.qlref b/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.qlref
diff --git a/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.testref b/cpp/autosar/test/rules/M5-14-1/RightHandOperandOfALogicalAndOperatorsContainSideEffects.testref
@@ -0,0 +1 @@
+cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.ql
diff --git a/cpp/autosar/test/rules/M5-14-1/test.cpp b/cpp/autosar/test/rules/M5-14-1/test.cpp
diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll
@@ -72,6 +72,7 @@ import Scope
 import SideEffects1
 import SideEffects2
 import SideEffects4
+import SideEffects5
 import SmartPointers1
 import SmartPointers2
 import Statements
@@ -156,6 +157,7 @@ newtype TCPPQuery =
   TSideEffects1PackageQuery(SideEffects1Query q) or
   TSideEffects2PackageQuery(SideEffects2Query q) or
   TSideEffects4PackageQuery(SideEffects4Query q) or
+  TSideEffects5PackageQuery(SideEffects5Query q) or
   TSmartPointers1PackageQuery(SmartPointers1Query q) or
   TSmartPointers2PackageQuery(SmartPointers2Query q) or
   TStatementsPackageQuery(StatementsQuery q) or
@@ -240,6 +242,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat
   isSideEffects1QueryMetadata(query, queryId, ruleId, category) or
   isSideEffects2QueryMetadata(query, queryId, ruleId, category) or
   isSideEffects4QueryMetadata(query, queryId, ruleId, category) or
+  isSideEffects5QueryMetadata(query, queryId, ruleId, category) or
   isSmartPointers1QueryMetadata(query, queryId, ruleId, category) or
   isSmartPointers2QueryMetadata(query, queryId, ruleId, category) or
   isStatementsQueryMetadata(query, queryId, ruleId, category) or
diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/SideEffects5.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/SideEffects5.qll
@@ -0,0 +1,26 @@
+//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY.  **/
+import cpp
+import RuleMetadata
+import codingstandards.cpp.exclusions.RuleMetadata
+
+newtype SideEffects5Query = TShortCircuitedPersistentSideEffectQuery()
+
+predicate isSideEffects5QueryMetadata(Query query, string queryId, string ruleId, string category) {
+  query =
+    // `Query` instance for the `shortCircuitedPersistentSideEffect` query
+    SideEffects5Package::shortCircuitedPersistentSideEffectQuery() and
+  queryId =
+    // `@id` for the `shortCircuitedPersistentSideEffect` query
+    "cpp/misra/short-circuited-persistent-side-effect" and
+  ruleId = "RULE-8-14-1" and
+  category = "advisory"
+}
+
+module SideEffects5Package {
+  Query shortCircuitedPersistentSideEffectQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `shortCircuitedPersistentSideEffect` query
+      TQueryCPP(TSideEffects5PackageQuery(TShortCircuitedPersistentSideEffectQuery()))
+  }
+}
diff --git a/cpp/common/src/codingstandards/cpp/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.qll b/cpp/common/src/codingstandards/cpp/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.qll
@@ -0,0 +1,32 @@
+/**
+ * Provides a configurable module ShortCircuitedPersistentSideEffectShared with a `problems` predicate
+ * for the following issue:
+ * The right-hand operand of a logical && or || operator should not contain persistent
+ * side effects, as this may violate developer intent and expectations.
+ */
+
+import cpp
+import codingstandards.cpp.Customizations
+import codingstandards.cpp.Exclusions
+import codingstandards.cpp.SideEffect
+import codingstandards.cpp.sideeffect.DefaultEffects
+import codingstandards.cpp.Expr
+
+signature module ShortCircuitedPersistentSideEffectSharedConfigSig {
+  Query getQuery();
+}
+
+module ShortCircuitedPersistentSideEffectShared<
+  ShortCircuitedPersistentSideEffectSharedConfigSig Config>
+{
+  query predicate problems(
+    BinaryLogicalOperation op, string message, Expr rhs, string rhsDescription
+  ) {
+    not isExcluded(op, Config::getQuery()) and
+    rhs = op.getRightOperand() and
+    hasSideEffect(rhs) and
+    not rhs instanceof UnevaluatedExprExtension and
+    message = "The $@ may have a side effect that is not always evaluated." and
+    rhsDescription = "right-hand operand"
+  }
+}
diff --git a/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.expected b/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.expected
@@ -0,0 +1,7 @@
+| test.cpp:20:7:20:14 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:20:12:20:14 | ... ++ | right-hand operand |
+| test.cpp:23:7:23:21 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:23:13:23:20 | ... == ... | right-hand operand |
+| test.cpp:26:7:26:15 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:26:12:26:13 | call to f1 | right-hand operand |
+| test.cpp:29:7:29:16 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:29:12:29:13 | call to f3 | right-hand operand |
+| test.cpp:45:7:45:41 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:45:26:45:26 | call to operator== | right-hand operand |
+| test.cpp:56:7:56:15 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:56:12:56:13 | call to f8 | right-hand operand |
+| test.cpp:66:7:66:16 | ... \|\| ... | The $@ may have a side effect that is not always evaluated. | test.cpp:66:12:66:14 | call to f10 | right-hand operand |
diff --git a/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.ql b/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.ql
@@ -0,0 +1,8 @@
+// GENERATED FILE - DO NOT MODIFY
+import codingstandards.cpp.rules.shortcircuitedpersistentsideeffectshared.ShortCircuitedPersistentSideEffectShared
+
+module TestFileConfig implements ShortCircuitedPersistentSideEffectSharedConfigSig {
+  Query getQuery() { result instanceof TestQuery }
+}
+
+import ShortCircuitedPersistentSideEffectShared<TestFileConfig>
diff --git a/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/test.cpp b/cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/test.cpp
@@ -0,0 +1,68 @@
+int i = 0;
+
+bool f1() {
+  i++;
+  return i == 1;
+}
+
+bool f2() {
+  int i = 0;
+  return i++ == 1;
+}
+
+bool f3(int &i) {
+  i++;
+  return i == 1;
+}
+
+void f4(bool b) {
+  int j = 0;
+  if (b || i++) { // NON_COMPLIANT
+  }
+
+  if (b || (j == i++)) { // NON_COMPLIANT
+  }
+
+  if (b || f1()) { // NON_COMPLIANT, f1 has global side-effects
+  }
+
+  if (b || f3(j)) { // NON_COMPLIANT, f3 has local side-effects
+  }
+}
+
+int g1 = 0;
+int f5() { return g1++; }
+int f6() { return 1; }
+
+#include <typeinfo>
+
+void f7() {
+  if (1 && sizeof(f5())) {
+  } // COMPLIANT  - sizeof operands not evaluated
+  if (1 &&noexcept(f5()) &&noexcept(f5())) {
+  } // COMPLIANT  - noexcept operands not evaluated
+
+  if (1 || (typeid(f6()) == typeid(f5()))) {
+  } // NON_COMPLIANT  - typeid operands not evaluated, but the ==operator is
+}
+
+bool f8() {
+  static int k = 0;
+  k++;
+  return k == 1;
+}
+
+void f9(bool b) {
+  if (b || f8()) { // NON_COMPLIANT, f8 has static side-effects
+  }
+}
+
+bool f10() {
+  volatile bool m = 0;
+  return m;
+}
+
+void f11(bool b) {
+  if (b || f10()) { // NON_COMPLIANT, f10 has volatile side-effects
+  }
+}
diff --git a/cpp/misra/src/rules/RULE-8-14-1/ShortCircuitedPersistentSideEffect.ql b/cpp/misra/src/rules/RULE-8-14-1/ShortCircuitedPersistentSideEffect.ql
@@ -0,0 +1,28 @@
+/**
+ * @id cpp/misra/short-circuited-persistent-side-effect
+ * @name RULE-8-14-1: The right-hand operand of a logical && or || operator should not contain persistent side effects
+ * @description The right-hand operand of a logical && or || operator should not contain persistent
+ *              side effects, as such side effects will only conditionally occur, which may violate
+ *              developer intent and/or expectations.
+ * @kind problem
+ * @precision high
+ * @problem.severity error
+ * @tags external/misra/id/rule-8-14-1
+ *       scope/system
+ *       correctness
+ *       readability
+ *       external/misra/enforcement/undecidable
+ *       external/misra/obligation/advisory
+ */
+
+import cpp
+import codingstandards.cpp.misra
+import codingstandards.cpp.rules.shortcircuitedpersistentsideeffectshared.ShortCircuitedPersistentSideEffectShared
+
+module ShortCircuitedPersistentSideEffectConfig implements
+  ShortCircuitedPersistentSideEffectSharedConfigSig
+{
+  Query getQuery() { result = SideEffects5Package::shortCircuitedPersistentSideEffectQuery() }
+}
+
+import ShortCircuitedPersistentSideEffectShared<ShortCircuitedPersistentSideEffectConfig>
diff --git a/cpp/misra/test/rules/RULE-8-14-1/ShortCircuitedPersistentSideEffect.testref b/cpp/misra/test/rules/RULE-8-14-1/ShortCircuitedPersistentSideEffect.testref
@@ -0,0 +1 @@
+cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.ql
diff --git a/rule_packages/cpp/SideEffects1.json b/rule_packages/cpp/SideEffects1.json
@@ -39,6 +39,7 @@
           "precision": "high",
           "severity": "warning",
           "short_name": "RightHandOperandOfALogicalAndOperatorsContainSideEffects",
+          "shared_implementation_short_name": "ShortCircuitedPersistentSideEffectShared",
           "tags": [
             "correctness"
           ]
diff --git a/rule_packages/cpp/SideEffects5.json b/rule_packages/cpp/SideEffects5.json
@@ -0,0 +1,27 @@
+{
+  "MISRA-C++-2023": {
+    "RULE-8-14-1": {
+      "properties": {
+        "enforcement": "undecidable",
+        "obligation": "advisory"
+      },
+      "queries": [
+        {
+          "description": "The right-hand operand of a logical && or || operator should not contain persistent side effects, as such side effects will only conditionally occur, which may violate developer intent and/or expectations.",
+          "kind": "problem",
+          "name": "The right-hand operand of a logical && or || operator should not contain persistent side effects",
+          "precision": "high",
+          "severity": "error",
+          "short_name": "ShortCircuitedPersistentSideEffect",
+          "shared_implementation_short_name": "ShortCircuitedPersistentSideEffectShared",
+          "tags": [
+            "scope/system",
+            "correctness",
+            "readability"
+          ]
+        }
+      ],
+      "title": "The right-hand operand of a logical && or || operator should not contain persistent side effects"
+    }
+  }
+}
diff --git a/rules.csv b/rules.csv
@@ -898,7 +898,7 @@ cpp,MISRA-C++-2023,RULE-8-3-2,Yes,Advisory,Decidable,Single Translation Unit,The
 cpp,MISRA-C++-2023,RULE-8-7-1,Yes,Required,Undecidable,System,Pointer arithmetic shall not form an invalid pointer,ARR30-C,Memory1,Easy,
 cpp,MISRA-C++-2023,RULE-8-7-2,Yes,Required,Undecidable,System,Subtraction between pointers shall only be applied to pointers that address elements of the same array,ARR36-C,Memory2,Easy,
 cpp,MISRA-C++-2023,RULE-8-9-1,Yes,Required,Undecidable,System,"The built-in relational operators >, >=, < and <= shall not be applied to objects of pointer type, except where they point to elements of the same array",ARR36-C,Memory3,Easy,
-cpp,MISRA-C++-2023,RULE-8-14-1,Yes,Advisory,Undecidable,System,The right-hand operand of a logical && or operator should not contain persistent side effects,"M5-14-1, RULE-13-5",SideEffects3,Medium,
+cpp,MISRA-C++-2023,RULE-8-14-1,Yes,Advisory,Undecidable,System,The right-hand operand of a logical && or operator should not contain persistent side effects,"M5-14-1, RULE-13-5",SideEffects5,Medium,
 cpp,MISRA-C++-2023,RULE-8-18-1,Yes,Mandatory,Undecidable,System,An object or subobject must not be copied to an overlapping object,"M0-2-1, RULE-19-1",Memory4,Hard,
 cpp,MISRA-C++-2023,RULE-8-18-2,Yes,Advisory,Decidable,Single Translation Unit,The result of an assignment operator should not be used,RULE-13-4,ImportMisra23,Import,
 cpp,MISRA-C++-2023,RULE-8-19-1,Yes,Advisory,Decidable,Single Translation Unit,The comma operator should not be used,M5-18-1,ImportMisra23,Import,

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+ - `M5-14-1` - `RightHandOperandOfALogicalAndOperatorsContainSideEffects.ql`:
	`2`	`+ - Implementation has been refactored to share logic with Rule 8.14.1. No observable changes to results expected.`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+cpp/common/test/rules/shortcircuitedpersistentsideeffectshared/ShortCircuitedPersistentSideEffectShared.ql`