Michaelrfairhurst/sideffects 4 package 4 6 1

[MichaelRFairhurst]

Description

Uses parametric modules more heavily in ordering configs.

Change request type

• Release or process automation (GitHub workflows, internal scripts)
• Internal documentation
• External documentation
• Query files (.ql, .qll, .qls or unit tests)
• External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

• No rules added
• Queries have been added for the following rules:
  • RULE-4-6-1
• Queries have been modified for the following rules:
  • A5-0-1
  • RULE-13-2
  • EXP50-CPP

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

• The structure or layout of the release artifacts.
• The evaluation performance (memory, execution time) of an existing query.
• The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

• Yes
• No

🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

• Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

• Confirmed

Query development review checklist

For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:

Author

• Have all the relevant rule package description files been checked in?
• Have you verified that the metadata properties of each new query is set appropriately?
• Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
• Are the alert messages properly formatted and consistent with the style guide?
• Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
  As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
• Does the query have an appropriate level of in-query comments/documentation?
• Have you considered/identified possible edge cases?
• Does the query not reinvent features in the standard library?
• Can the query be simplified further (not golfed!)

Reviewer

• Have all the relevant rule package description files been checked in?
• Have you verified that the metadata properties of each new query is set appropriately?
• Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
• Are the alert messages properly formatted and consistent with the style guide?
• Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
  As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
• Does the query have an appropriate level of in-query comments/documentation?
• Have you considered/identified possible edge cases?
• Does the query not reinvent features in the standard library?
• Can the query be simplified further (not golfed!)

[Copilot]

Pull request overview

This PR refactors the ordering/sequencing analysis infrastructure to use parametric modules instead of class-based configurations, and adds support for MISRA C++ 2023 RULE-4-6-1 which checks for appropriately sequenced operations on memory locations. The refactoring enables cleaner separation between C++14 and C++17 sequencing semantics.

Changes:

• Refactors Ordering.qll to use signature modules (ConfigSig) and parametric modules (Make<Config>) for sequencing edge definitions
• Adds MISRA C++ 2023 RULE-4-6-1 query for detecting unsequenced operations on memory locations (C++17 semantics)
• Updates AUTOSAR A5-0-1 and C MISRA RULE-13-2 queries to use the new parametric module pattern
• Introduces shared ExpressionWithUnsequencedSideEffects query module with configurable sequencing logic

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
rules.csv	Updates RULE-4-6-1 to use SideEffects4 package
rule_packages/cpp/SideEffects4.json	Adds new package definition for RULE-4-6-1
rule_packages/cpp/OrderOfEvaluation.json	Adds shared implementation reference for A5-0-1
cpp/misra/src/rules/RULE-4-6-1/MemoryUsageNotSequenced.ql	Implements new query using C++17 sequencing rules
cpp/misra/test/rules/RULE-4-6-1/MemoryUsageNotSequenced.testref	Links to shared test file
cpp/common/test/rules/expressionwithunsequencedsideeffects/test.cpp	Adds test cases with C++14/C++17 annotations
cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.ql	C++14 test query
cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffectsCpp17.ql	C++17 test query
cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.expected	Expected results for C++14
cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffectsCpp17.expected	Expected results for C++17
cpp/common/src/codingstandards/cpp/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.qll	Shared query implementation with parametric module
cpp/common/src/codingstandards/cpp/orderofevaluation/VariableAccessOrdering.qll	Splits into Cpp14 and Cpp17 configuration modules
cpp/common/src/codingstandards/cpp/exclusions/cpp/SideEffects4.qll	Adds exclusion metadata for new package
cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll	Registers SideEffects4 package
cpp/common/src/codingstandards/cpp/Ordering.qll	Major refactoring to use signature/parametric modules, adds cpp14Edge and cpp17Edge predicates
cpp/autosar/src/rules/A5-0-1/ExpressionShouldNotRelyOnOrderOfEvaluation.ql	Refactored to use new parametric module pattern
c/misra/src/rules/RULE-13-2/UnsequencedSideEffects.ql	Updated to use parametric module pattern
c/misra/src/rules/RULE-13-2/UnsequencedAtomicReads.ql	Updated to use parametric module pattern
c/common/src/codingstandards/c/orderofevaluation/VariableAccessOrdering.qll	Converted to use signature module
c/common/src/codingstandards/c/Ordering.qll	Refactored to use signature/parametric modules pattern

Comments suppressed due to low confidence (1)

cpp/common/src/codingstandards/cpp/Ordering.qll:185

• The comment states "The right operand is sequenced before the left operand" but the code sequences lhs (n1) before rhs (n2). According to C++17 [expr.ass], the right-hand side should be evaluated before the left-hand side. The assignments should be swapped: rhs should map to n1 and lhs should map to n2.

    // [expr.ass] The right operand is sequenced before the left operand for all assignment operators.
    exists(Assignment assign, ConstituentExpr lhs, ConstituentExpr rhs |
      lhs = assign.getLValue() and
      rhs = assign.getRValue()
    |
      lhs = n1.toExpr().getParent*() and rhs = n2.toExpr().getParent*()
    )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The check for e1.getASuccessor() = e2 was removed from the FullExpr sequencing rule. The current implementation will consider ANY two distinct FullExprs as having a sequencing relationship, not just consecutive ones. This could lead to incorrect results if the isCandidate predicate allows non-consecutive FullExprs to be compared. Verify that this removal is intentional and that the isCandidate predicate properly restricts the pairs that are compared.

This issue also appears on line 179 of the same file.

Suggested change

	exists(FullExpr e1, FullExpr e2 | e1 = n1.toExpr() and e2 = n2.toExpr())
	exists(FullExpr e1, FullExpr e2 |
	e1 = n1.toExpr() and
	e2 = n2.toExpr() and
	e1.getASuccessor() = e2
	)