Implement package SideEffects6 with rule 28.3.1, predicate should not have side effects#1051
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements MISRA C++ 2023 rule RULE-28-3-1 ("Predicates shall not have persistent side effects") as part of the new SideEffects6 package. The implementation introduces two complementary queries that detect different aspects of predicate misuse in C++ standard library algorithms.
Changes:
- Adds new template substitution tracking library (Templates.qll) to model how template parameters are instantiated with concrete types
- Adds new predicate detection library (Predicate.qll) to identify predicate function objects and function pointers passed to standard library algorithms
- Implements two queries: one detecting persistent side effects (global/static modifications) in predicates, and one detecting non-const operator() in predicate function objects
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Updates RULE-28-3-1 entry to reference SideEffects6 package |
| rule_packages/cpp/SideEffects6.json | Defines package metadata and query configurations for two queries targeting RULE-28-3-1 |
| cpp/common/src/codingstandards/cpp/types/Templates.qll | New library modeling template parameter substitutions (ClassTemplateInstantiation, FunctionTemplateInstantiation, VariableTemplateInstantiation) |
| cpp/common/src/codingstandards/cpp/types/Predicate.qll | New library for detecting predicate types, function objects, and function pointer uses in standard library algorithms |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/SideEffects6.qll | Auto-generated exclusion module for the two queries |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Integrates SideEffects6 package into the exclusion metadata system |
| cpp/misra/src/rules/RULE-28-3-1/PredicateWithPersistentSideEffects.ql | Query detecting predicates with persistent side effects (global/static modifications) |
| cpp/misra/src/rules/RULE-28-3-1/NonConstPredicateFunctionObject.ql | Query detecting function objects with non-const operator() used as predicates |
| cpp/misra/test/rules/RULE-28-3-1/test.cpp | Comprehensive test cases covering both compliant and non-compliant scenarios |
| cpp/misra/test/rules/RULE-28-3-1/*.qlref | Test reference files pointing to the query implementations |
| cpp/misra/test/rules/RULE-28-3-1/*.expected | Expected test results for both queries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cpp,MISRA-C++-2023,RULE-25-5-3,Yes,Mandatory,Undecidable,System,"The pointer returned by the C++ Standard Library functions asctime, ctime, gmtime, localtime, localeconv, getenv, setlocale or strerror must not be used following a subsequent call to the same function",RULE-21-20,ImportMisra23,Import, | ||
| cpp,MISRA-C++-2023,RULE-26-3-1,Yes,Advisory,Decidable,Single Translation Unit,std::vector should not be specialized with bool,A18-1-2,ImportMisra23,Import, | ||
| cpp,MISRA-C++-2023,RULE-28-3-1,Yes,Required,Undecidable,System,Predicates shall not have persistent side effects,A25-1-1,SideEffects3,Easy, | ||
| cpp,MISRA-C++-2023,RULE-28-3-1,Yes,Required,Undecidable,System,Predicates shall not have persistent side effects,A25-1-1,SideEffects6,Easy, |
There was a problem hiding this comment.
The PR description lists "RULE-28-3-" which is incomplete. It should be "RULE-28-3-1" to match the actual rule being implemented.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * `TemplateClass`, `TemplateFunction`, or `TemplateVariable`, which complicates their usage. | ||
| * | ||
| * A `Substitution` in particular refers to an instantiation of that template of some kind, and | ||
| * allows analysis of which parameters were substituted with which types in that instatiation. |
There was a problem hiding this comment.
Typo in doc comment: "instatiation" should be "instantiation".
| * allows analysis of which parameters were substituted with which types in that instatiation. | |
| * allows analysis of which parameters were substituted with which types in that instantiation. |
| } | ||
|
|
||
| /** | ||
| * Get a `Locatable` that represents a where this substitution was declared in the source code. |
There was a problem hiding this comment.
Grammar in doc comment is off: "represents a where" should be reworded (e.g., "represents where" or "represents a location where").
| * Get a `Locatable` that represents a where this substitution was declared in the source code. | |
| * Get a `Locatable` that represents where this substitution was declared in the source code. |
| * To be more widely useful, we match more flexibly on the name, as `_Compare` is also common, etc. | ||
| * | ||
| * To get a particular `Type` that is _used_ as a predicate type, see `getASubstitutedType()`, | ||
| * rather than the type parameter itself, see `getASubstitutedType()`. |
There was a problem hiding this comment.
The doc comment repeats the same guidance twice ("see getASubstitutedType()" in consecutive lines). This should be clarified to avoid confusion about whether a different API was intended for the second reference.
| * rather than the type parameter itself, see `getASubstitutedType()`. | |
| * rather than inspecting the type parameter itself. |
| not callOperator instanceof ConstMemberFunction | ||
| select usageSite, "Predicate usage of $@ has $@", callOperator.getDeclaringType(), | ||
| "callable object " + callOperator.getDeclaringType().getName(), callOperator, | ||
| "non const operator()." |
There was a problem hiding this comment.
The alert message text "non const operator()." is grammatically awkward and includes punctuation inside the operator name. Consider changing it to "non-const operator()" (and keep punctuation consistent with the rest of the message).
| "non const operator()." | |
| "non-const operator()" |
1 participant
Description
New library to handle template parameters and find their substitutions.
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-28-3-Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
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.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
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
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.
Reviewer
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.