Narrowly focused implementation of RULE 15-0-1#1121
Narrowly focused implementation of RULE 15-0-1#1121MichaelRFairhurst wants to merge 15 commits intomainfrom
Conversation
…ub.com:github/codeql-coding-standards into michaelrfairhurst/classes-3-take-2-rule-15-0-1
…ub.com:github/codeql-coding-standards into michaelrfairhurst/classes-3-take-2-rule-15-0-1
There was a problem hiding this comment.
Pull request overview
Adds a new MISRA C++:2023 RULE-15-0-1 implementation (plus an audit companion query) to detect incorrectly provided special member functions, including packaging/metadata wiring and a dedicated test suite.
Changes:
- Introduces
ImproperlyProvidedSpecialMemberFunctions.qlandImproperlyProvidedSpecialMemberFunctionsAudit.qlfor RULE-15-0-1. - Adds helper library logic (
AnalyzableClass.qll) to model special-member availability/customization. - Registers the new rule package in exclusions/metadata and adds comprehensive
.qlref/.expected/C++ test fixtures.
Show a summary per file
| File | Description |
|---|---|
| rule_packages/cpp/Classes3.json | Adds RULE-15-0-1 rule package entries (main + audit). |
| cpp/misra/src/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctions.ql | Main RULE-15-0-1 query implementation and message logic. |
| cpp/misra/src/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctionsAudit.ql | Audit query for “not analyzable” classes. |
| cpp/misra/src/rules/RULE-15-0-1/AnalyzableClass.qll | Shared modeling of special member functions for analyzable classes. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Wires the new Classes3 package into query metadata/exclusions. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Classes3.qll | New autogenerated metadata module for the Classes3 package. |
| cpp/misra/test/rules/RULE-15-0-1/test.cpp | New C++ test fixture covering compliant/non-compliant and audit cases. |
| cpp/misra/test/rules/RULE-15-0-1/*.qlref | Test references to the production queries. |
| cpp/misra/test/rules/RULE-15-0-1/*.expected | Expected results for both main and audit queries. |
Copilot's findings
Comments suppressed due to low confidence (4)
cpp/misra/test/rules/RULE-15-0-1/test.cpp:376
- The audit
.expectedfile reports an alert at this class, but the test source does not mark it// NON_COMPLIANT(or similar). Please add the appropriateNON_COMPLIANTannotation here or remove/update the corresponding row inImproperlyProvidedSpecialMemberFunctionsAudit.expected.
class UnmovableDerivedPublicVirtualDtor
: public UnmovableBasePublicVirtualDtor {};
cpp/misra/test/rules/RULE-15-0-1/test.cpp:403
- The audit
.expectedfile reports an alert at this class, but the test source does not mark it// NON_COMPLIANT(or similar). Please add the appropriateNON_COMPLIANTannotation here or remove/update the corresponding row inImproperlyProvidedSpecialMemberFunctionsAudit.expected.
class ProtectedDtorDerived : public BaseProtectedDtor {};
cpp/misra/test/rules/RULE-15-0-1/test.cpp:416
- The audit
.expectedfile reports an alert at this class, but the test source does not mark it// NON_COMPLIANT(or similar). Please add the appropriateNON_COMPLIANTannotation here or remove/update the corresponding row inImproperlyProvidedSpecialMemberFunctionsAudit.expected.
class VirtualProtectedDtorDerived : public BaseVirtualProtectedDtor {};
cpp/misra/test/rules/RULE-15-0-1/test.cpp:390
- The audit
.expectedfile reports an alert at this class, but the test source does not mark it// NON_COMPLIANT(or similar). Please add the appropriateNON_COMPLIANTannotation here or remove/update the corresponding row inImproperlyProvidedSpecialMemberFunctionsAudit.expected.
class UnmovablePrivateVirtualDtorDerived : public UnmovablePrivateVirtualDtor {
};
- Files reviewed: 11/11 changed files
- Comments generated: 9
| Constructor getMoveConstructor(Class c) { | ||
| if | ||
| not exists(MoveConstructor mc | mc = c.getAConstructor() and isUserDeclared(mc)) and | ||
| implicitMoveIsSuppressed(c) | ||
| then result = c.getAConstructor().(CopyConstructor) | ||
| else result = c.getAConstructor().(MoveConstructor) | ||
| } | ||
|
|
||
| Operator getMoveAssign(Class c) { | ||
| if | ||
| not exists(MoveAssignmentOperator mc | mc = c.getAMemberFunction() and isUserDeclared(mc)) and | ||
| implicitMoveIsSuppressed(c) | ||
| then result = c.getAMemberFunction().(CopyAssignmentOperator) | ||
| else result = c.getAMemberFunction().(MoveAssignmentOperator) | ||
| } |
There was a problem hiding this comment.
getMoveConstructor and getMoveAssign are public predicates/functions in a library file but have no QLDoc. Either make them private if they are internal helpers, or add QLDoc documenting their behavior and parameters.
| newtype TSpecialMember = | ||
| TMoveConstructor() or | ||
| TMoveAssignmentOperator() or | ||
| TCopyConstructor() or | ||
| TCopyAssignmentOperator() or | ||
| TDestructor() | ||
|
|
||
| class AnalyzableClass extends Class { | ||
| CopyConstructor copyCtor; | ||
| // The move constructor may be suppressed, and the copy constructor may be used during moves. |
There was a problem hiding this comment.
TSpecialMember and AnalyzableClass are public declarations without QLDoc. Repository guidelines require all public types/classes/newtypes in .qll libraries to be documented, or marked private if they are not part of the public API.
| DELETED, DELETED, DEFAULTED) | ||
| }; | ||
|
|
||
| class UnmovableNonvirtualDerived : public UnmovableBaseNonvirtualDtor {}; |
There was a problem hiding this comment.
The audit .expected file reports an alert at this class, but the test source does not mark it // NON_COMPLIANT (or similar). Please add the appropriate NON_COMPLIANT annotation here or remove/update the corresponding row in ImproperlyProvidedSpecialMemberFunctionsAudit.expected.
This issue also appears in the following locations of the same file:
- line 375
- line 389
- line 403
- line 416
| result = "destructor" | ||
| } | ||
|
|
||
| string missingKinds(Class c) { result = concat(missingKind(c), " and ") } |
There was a problem hiding this comment.
missingKinds uses concat(...), which can yield non-deterministic ordering when multiple kinds are missing, making alert messages (and .expected files) flaky. Consider using strictconcat(...) or otherwise imposing a stable order on the joined kinds.
| string missingKinds(Class c) { result = concat(missingKind(c), " and ") } | |
| string missingKinds(Class c) { result = strictconcat(missingKind(c), " and ") } |
| import cpp | ||
|
|
There was a problem hiding this comment.
This .qll file is missing a top-level QLDoc header comment. Repository guidelines require library files to start with a documentation comment explaining the purpose of the library.
See below for a potential fix:
@ -1,3 +1,7 @@
/**
* Provides classes and predicates for identifying analyzable classes and
* relevant special member functions for MISRA rule `RULE-15-0-1`.
*/
| #define DEFINE_ALL_SPECIAL_MEMBERS(NAME, \ | ||
| COPY_CTOR_SPEC, MOVE_CTOR_SPEC, COPY_ASSIGN_SPEC, MOVE_ASSIGN_SPEC, DTOR_SPEC) \ | ||
| COPY_CTOR(NAME) COPY_CTOR_SPEC /* T(const T&) = default; */ \ | ||
| MOVE_CTOR(NAME) MOVE_CTOR_SPEC /* T(T&&) = default; */ \ | ||
| COPY_ASSIGN(NAME) COPY_ASSIGN_SPEC /* T& operator=(const T&) = default; */ \ | ||
| MOVE_ASSIGN(NAME) MOVE_ASSIGN_SPEC /* T& operator=(T&&) = default; */ \ | ||
| DTOR(NAME) DTOR_SPEC /* ~T() = default; */ |
There was a problem hiding this comment.
These macro definitions exceed the repository's 100-character line limit for CodeQL test files (see custom-instructions/repo/.github/copilot-instructions.md:79). Please wrap/split the macro parameter list and/or align continuations so each line stays within 100 characters.
| #define DEFINE_ALL_SPECIAL_MEMBERS(NAME, \ | |
| COPY_CTOR_SPEC, MOVE_CTOR_SPEC, COPY_ASSIGN_SPEC, MOVE_ASSIGN_SPEC, DTOR_SPEC) \ | |
| COPY_CTOR(NAME) COPY_CTOR_SPEC /* T(const T&) = default; */ \ | |
| MOVE_CTOR(NAME) MOVE_CTOR_SPEC /* T(T&&) = default; */ \ | |
| COPY_ASSIGN(NAME) COPY_ASSIGN_SPEC /* T& operator=(const T&) = default; */ \ | |
| MOVE_ASSIGN(NAME) MOVE_ASSIGN_SPEC /* T& operator=(T&&) = default; */ \ | |
| DTOR(NAME) DTOR_SPEC /* ~T() = default; */ | |
| #define DEFINE_ALL_SPECIAL_MEMBERS( \ | |
| NAME, COPY_CTOR_SPEC, MOVE_CTOR_SPEC, \ | |
| COPY_ASSIGN_SPEC, MOVE_ASSIGN_SPEC, DTOR_SPEC) \ | |
| COPY_CTOR(NAME) COPY_CTOR_SPEC /* T(const T&) = default; */ \ | |
| MOVE_CTOR(NAME) MOVE_CTOR_SPEC /* T(T&&) = default; */ \ | |
| COPY_ASSIGN(NAME) COPY_ASSIGN_SPEC \ | |
| /* T& operator=(const T&) = default; */ \ | |
| MOVE_ASSIGN(NAME) MOVE_ASSIGN_SPEC \ | |
| /* T& operator=(T&&) = default; */ \ | |
| DTOR(NAME) DTOR_SPEC /* ~T() = default; */ |
| "Class '" + c.getName() + "' is not analyzable because the " + kinds + | ||
| " are not present in the CodeQL database." |
There was a problem hiding this comment.
The message always uses the plural verb "are" ("the X are not present"), which is grammatically incorrect when only one kind is missing (for example, "the destructor"). Adjust the message to use "is" for a single missing kind, or restructure the sentence to avoid number agreement issues.
| "Class '" + c.getName() + "' is not analyzable because the " + kinds + | |
| " are not present in the CodeQL database." | |
| "Class '" + c.getName() + "' is not analyzable because the CodeQL database does not " + | |
| "contain the " + kinds + "." |
| { | ||
| "MISRA-C++-2023": { | ||
| "RULE-15-0-1": { | ||
| "properties": { | ||
| "enforcement": "decidable", | ||
| "obligation": "required" | ||
| }, |
There was a problem hiding this comment.
The PR description still contains the unfilled template (placeholders/unchecked checklists), which makes it hard to verify whether a change note is required and what type of change this is. Please update the PR description sections (change type, rule list, and change note confirmation) to reflect this PR.
| private predicate implicitMoveIsSuppressed(Class c) { | ||
| isUserDeclared(c.getAConstructor().(CopyConstructor)) | ||
| or | ||
| isUserDeclared(c.getAConstructor().(CopyAssignmentOperator)) |
There was a problem hiding this comment.
implicitMoveIsSuppressed checks c.getAConstructor().(CopyAssignmentOperator), but copy assignment operators are not constructors. This makes the suppression logic incorrect for classes with a user-declared copy assignment operator, which can affect how move operations are modeled. Use c.getAMemberFunction().(CopyAssignmentOperator) (or equivalent) here.
| isUserDeclared(c.getAConstructor().(CopyAssignmentOperator)) | |
| isUserDeclared(c.getAMemberFunction().(CopyAssignmentOperator)) |
Description
please enter the description of your change here
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
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.