Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new C++ MISRA-C++-2023 “Declarations5” rule package and introduces two new rule queries (RULE-6-8-4 and RULE-6-9-1), wiring them into rule metadata/exclusions and adding unit tests.
Changes:
- Add new CodeQL queries for RULE-6-8-4 and RULE-6-9-1 under the Declarations5 package.
- Add/route rule package metadata (rules.csv +
rule_packages/cpp/Declarations5.json) and integrate the new package into exclusions metadata. - Add unit tests (
.qlref,.expected,test.cpp) for both new queries.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Re-points RULE-6-8-4 and RULE-6-9-1 to the new Declarations5 package. |
| rule_packages/cpp/Declarations5.json | Defines rule-package metadata and query metadata for the two new rules. |
| cpp/misra/src/rules/RULE-6-8-4/MemberFunctionsRefqualified.ql | Implements the RULE-6-8-4 query logic. |
| cpp/misra/src/rules/RULE-6-9-1/TypeAliasesDeclaration.ql | Implements the RULE-6-9-1 query logic. |
| cpp/misra/test/rules/RULE-6-8-4/test.cpp | Adds test cases for ref-qualification rule behavior. |
| cpp/misra/test/rules/RULE-6-8-4/MemberFunctionsRefqualified.qlref | Connects tests to the production RULE-6-8-4 query. |
| cpp/misra/test/rules/RULE-6-8-4/MemberFunctionsRefqualified.expected | Adds expected results for RULE-6-8-4 tests. |
| cpp/misra/test/rules/RULE-6-9-1/test.cpp | Adds test cases for type-alias consistency across redeclarations. |
| cpp/misra/test/rules/RULE-6-9-1/TypeAliasesDeclaration.qlref | Connects tests to the production RULE-6-9-1 query. |
| cpp/misra/test/rules/RULE-6-9-1/TypeAliasesDeclaration.expected | Adds expected results for RULE-6-9-1 tests. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Registers the new Declarations5 package in the C++ exclusions metadata dispatcher. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Declarations5.qll | Adds autogenerated metadata mappings and query constructors for the new package. |
Copilot's findings
Comments suppressed due to low confidence (1)
cpp/misra/src/rules/RULE-6-8-4/MemberFunctionsRefqualified.ql:56
MembersReturningSubObjectcurrently matchesreturn field;even when the function returns the field by value. This causes false positives (for example, an&&overload that returns a subobject by value to avoid dangling references). The rule description is about returning references/pointers; please add a return-type guard so only reference/pointer returns are considered violations.
class MembersReturningSubObject extends MembersReturningObjectOrSubobject {
MembersReturningSubObject() {
exists(ReturnStmt r, FieldSubObjectDeclaration field |
r.getEnclosingFunction() = this and
(
r.getAChild*() = field.(Field).getAnAccess()
or
exists(PointerDereferenceExpr p |
p.getAChild*() = field.(Field).getAnAccess() and
r.getAChild*() = p
)
) and
field.(Field).getDeclaringType() = this.getDeclaringType()
)
}
- Files reviewed: 12/12 changed files
- Comments generated: 6
| | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. | | ||
| | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. | |
There was a problem hiding this comment.
The .expected output contains three identical rows for the same location (test.cpp:42:16). This indicates the query is emitting duplicates; tests should generally not codify duplicate results. Once the query is de-duplicated, update this expected file accordingly.
| | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. | | |
| | test.cpp:42:16:42:16 | Members returning object or subobject | Member function is not properly ref qualified. | |
| INT i; | ||
| extern int i; // NON_COMPLIANT |
There was a problem hiding this comment.
The expected results for this rule report the primary location on INT i; (line 4), but the test marks extern int i; (line 5) as NON_COMPLIANT. Please align the NON_COMPLIANT annotation(s) with the location(s) the query reports (either move/add the annotation to line 4, or adjust the query to report line 5 as the primary location).
| INT i; | |
| extern int i; // NON_COMPLIANT | |
| INT i; // NON_COMPLIANT | |
| extern int i; |
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
There are definitely some tricky nuanced scenarios here, but the good news is I think we have some prior art that can handle them, and is still fairly simple. Here's my thoughts.
Consider making getASubobjectAccessOf public in CppObjects.qll and using that here, so that we can essentially write:
predicate isSubobjectReferenceToThis(ThisExpr root, Expr access) {
// something like this:
access.(AddressOfExpr).getAnOperand() = getASubobjectAccessOf(root)
or
// ... handle the case where we take a reference to a subobject instead of its address
}
We likely want to extend getASubobjectAccessOf in two ways for C++ (it's still a copy of the c version right now):
- A. to unwrap the
thispointer, and - B. to exclude reference accesses
A. would be something like:
Expr getASubobjectAccessOfPointee(Expr e) {
e.getParent() instanceof AddressOfExpr and
result = getASubobjectAccessOf(e.getParent())
or
e.getParent() instanceof PointerFieldAcess() and
....
}
B. excluding reference accesses, would be something like:
Expr getASubobjectAccessOf(Expr e) {
...
or
result.(DotFieldAccess).getQualifier() = getASubobjectAccessOf(e) and
// new part:
not result.(FieldAccess).getField().getUnderlyingType() instanceof ReferenceType
or
...
}
Note that there's overlap between A. and B., because this->x.y may be a subobject (if x and y are not references) or not (if either x or y is a reference).
Something beyond A. and B. that we probably don't want to handle right now, but rather file an issue for, would be to add support for simple getter functions, especially for pointer-like types such as unique_ptr<T> and iterator types. Definitely reasonable to file in the issue tracker as a TODO.
class C {
Member m;
Member &get_member() { return m; }
Member& operator*() { return m; }
Member *operator->() { return &m; }
}
void f() {
C c;
c.get_member(); // accesses member
*c; // accesses member
c->bar; // accesses member
Hopefully this makes some sense, feel free to ping questions of course!
| exists(ReturnStmt r, ThisExpr t | | ||
| r.getEnclosingFunction() = this and | ||
| ( | ||
| r.getAChild*() = t |
There was a problem hiding this comment.
I'm usually not a fan of getAChild*(), because it is typically a bigger hammer than we want. For instance, this will return FPs for return ref ? 1 : 2, or return ref_to_vector.get_size(), etc.
Can we use localFlow instead?
| */ | ||
| class FieldSubObjectDeclaration extends Declaration { | ||
| FieldSubObjectDeclaration() { | ||
| not this.getADeclarationEntry().getType() instanceof ReferenceType and |
There was a problem hiding this comment.
This isn't going to do exactly what we probably want, because we have to worry again about pointers and pointees.
Returning a pointer by value is allowed, and dereferencing that pointer should also be allowed because the result could be any object -- not guaranteed to be a subobject of this. And returning an address or reference to a field that is a pointer should be flagged as well.
class C {
int i;
int *ip;
int **ip2;
int *f() {
return &i; // non compliant
return ip; // compliant -- copies pointer
return *ip2; // compliant -- reads pointer
}
int **f2() {
return &ip; // non compliant
return ip2; // compliant -- copies pointer
}
int &f3() {
return i; // non compliant
return *ip; // compliant -- reads pointer
return **ip2; // compliant -- reads pointer
}
int *&f4() {
return &p; // won't compile
return ip; // non compliant
return *ip2; // compliant -- reads pointer
}
int **&f5() {
return &ip; // won't compile
return ip; // non compliant
}
}
Description
Declarations 5
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.