Implement "Statements" package #938
Conversation
| * to a non-const reference variable (thus constituting a `T` -> `&T` conversion.), i.e. | ||
| * initialization and assignment. | ||
| */ | ||
| /* |
There was a problem hiding this comment.
Simple comment formatting, unnecessary split
There was a problem hiding this comment.
Good call. The intention was to split the documentation and the meta-level comment (explaining how this predicate came to be). But like you said it can be disconnected easily, so I'll merge the meta-level comment into the docstring first.
There was a problem hiding this comment.
Addressed in c8c0770.
There was a problem hiding this comment.
Somehow this change didn't make it to c8c0770; it did to a recent commit.
| predicate loopVariableAssignedToNonConstPointerOrReferenceType( | ||
| ForStmt forLoop, VariableAccess loopVariableAccessInCondition | ||
| ) { | ||
| exists(Expr assignmentRhs, DerivedType targetType | |
There was a problem hiding this comment.
Likely want to test that this works for a int * const x:
void f(int * const x) {
(*x)++;
}
int main() {
for (int i = 0; i < 10; ++i) {
f(&i);
std::cout << i << std::endl;
}
}I believe what will happen is that int * const x will be a DerivedType of type SpecifiedType with a const specifier. A SpecifiedType is not instanceof PointerType or instanceof ReferenceType and so this predicate will not hold, even though the value of i is modifiable within f.
You may also have problems with typedefs, such as typedef int *int_ptr_t for the same reason.
The solution here I believe will be to call .getUnderlyingType(). Another option frequently used for this is .stripSpecifiers(). Each of these will remove the const and resolve the typedef. I think .stripSpecifiers() may remove the const in const int*, though, which would make it unsuitable here.
There was a problem hiding this comment.
You're right; the predicate does not catch this example. 🤔 I guess a clever use of one or more of isDeeplyConst, or isDeeplyConstBelow will do the trick.
There was a problem hiding this comment.
Forgetting to handle typedefs or meaningless consts is a very common bug. But you'll (mostly) get in the habit soon enough of always calling one of these four member predicates on the Types you handle in your queries:
getUnderlyingType()resolveTypedefs()stripSpecifiers()stripTopLevelSpecifiers()
Each one does subtly different things.
In this case, I believe the fix is to do:
exists(..., Type targetType, DerivedType strippedType |
isAssignment(assignmentRhs, targetType, _) and
strippedType = targetType.stripTopLevelSpecifiers()
not strippedType.getBaseType().isConst() and
(
strippedType instanceof PointerType or
strippedType instanceof ReferenceType
)The documentation for stripTopLevelSpecifiers says:
Get this type after any top-level specifiers and typedefs have been stripped.
For example, starting with
const i64* const, this predicate will returnconst i64*.
which is actually wrong, as it ignores the fact that i64 is a TypeDefType, so it actually will result in const long long*. Which is what you want!
The TLDR of the other options:
getUnderlyingType()-- resolvesTypdefTypes andDeclTypes, but won't drop the outer specifer inconst i64* const. Stops at the first non-TypedefType/non-DeclType.stripType()-- resolves all typedefs and decltypes and removes allconst/volatilespecifiers recursively all the way down the type chain -- not what you want.resolveTypedefs-- resolves all typedefs and decltypes all the way down the type chain without removingconstorvolatilespecifiers. That would handle typedefs but notint const *.
Note that these predicates can have no result. Only a limited set of types are in the database, and these operations just assume that the type you want is one of those types. resolveTypedefs is also bugged and doesn't recurse into ArrayType.
There was a problem hiding this comment.
Thank you for the detailed breakdown of the related predicates. What I want to express here is definitely "The type we get after we strip all the typedefs and the specifiers is const". I've come to believe stripTopLevelSpecifiers is the one I should use, and swapped the portion with your suggestion.
I also patched an equivalent part in loopVariablePassedAsArgumentToNonConstReferenceParameter, in 7d5f08b.
cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql
Outdated
Show resolved
Hide resolved
cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql
Outdated
Show resolved
Hide resolved
cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql
Outdated
Show resolved
Hide resolved
cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql
Outdated
Show resolved
Hide resolved
cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql
Outdated
Show resolved
Hide resolved
| loopCounterType = forLoopCondition.getLoopCounter().getType() and | ||
| loopBoundType = forLoopCondition.getLoopBound().getType() | ||
| | | ||
| loopCounterType.getSize() < loopBoundType.getSize() |
There was a problem hiding this comment.
Two missed cases here:
- Mixing signed/unsigned types, they may have the same size but they'll hold different ranges.
- The type and runtime value may lead to different conclusions.
I think you may be able to get away with upperBound(loopCounter) < upperBound(loopBound). That would handle signedness, constants (like x < 10ull), and dynamic ranges (like unsigned long long bound = 10; ... x < bound).
There was a problem hiding this comment.
Also almost forgot
Another trap case is that when doing upperBound(e) / lowerBound(e) you usually want upperBound(e.getFullyConverted()). Because conversions on e will change the bound.
There was a problem hiding this comment.
This eliminated a lot of false positives where the counter variable is int and the loop bound is size_t. Thank you!
There was a problem hiding this comment.
Changed the upperBound(loopCounter.getFullyConverted()) to typeUpperBound(loopCounter.getType()). As typeUpperBound resolves references, I didn't have to use getBaseType() on it.
We are interested if the underlying *data* can be mutated, not the pointer itself. Also, the surface type may be a typedef, so resolve that as well.
Both `TLoopBoundIsMutatedVariableAccess` and `TLoopStepIsMutatedVariableAccess` transitively rely on `valueToUpdate`, which overapproximates by looking at the types alone. Therefore we'd like to drop the confidence slightly in reporting the expression where the expression *might* have been changed.
There was a problem hiding this comment.
Pull Request Overview
This PR implements the "Statements" package for the MISRA C++-2023 coding standards, adding three new query rules for analyzing statement structures in C++ code.
- Added rule implementations for RULE-9-4-2, RULE-9-5-1, and RULE-9-5-2
- Added comprehensive test files with both compliant and non-compliant examples
- Created supporting library code for analyzing increment operations and loop conditions
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rule_packages/cpp/Statements.json | Package configuration defining metadata and properties for the three new statement rules |
| cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql | Query implementation to check proper switch statement structure |
| cpp/misra/src/rules/RULE-9-5-1/LegacyForStatementsShouldBeSimple.ql | Query implementation to enforce simple legacy for-loop patterns |
| cpp/misra/src/rules/RULE-9-5-2/ForRangeInitializerAtMostOneFunctionCall.ql | Query implementation to limit function calls in range-based for initializers |
| cpp/misra/test/rules/RULE-9-/ | Test files and expected results for all three rules |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Statements.qll | Auto-generated exclusion metadata for the new package |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Updated metadata registry to include Statements package |
| cpp/common/src/codingstandards/cpp/ast/Increment.qll | New library for analyzing increment/decrement operations |
| cpp/common/src/codingstandards/cpp/Loops.qll | Extended loop analysis with LegacyForLoopCondition class |
Comments suppressed due to low confidence (1)
rule_packages/cpp/Statements.json:1
- Fixed typo 'that that' should be 'that'.
{
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cpp/misra/src/rules/RULE-9-4-2/AppropriateStructureOfSwitchStatement.ql
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…github/codeql-coding-standards into jeongsoolee09/MISRA-C++-2023-Statements
| /* 3-1. The loop counter is mutated somewhere other than its update expression. */ | ||
| TLoopCounterMutatedInLoopBody(ForStmt forLoop, Variable loopCounterVariable) { | ||
| loopCounterVariable = getDeclaredVariableInForLoop(forLoop) and | ||
| variableModifiedInExpression(forLoop.getStmt().getChildStmt().getAChild*(), |
There was a problem hiding this comment.
Do we need to worry about updates in the condition?
for(int i = 0; i++ < 10;) {...}
There was a problem hiding this comment.
I changed the case body to better match the comment /* 3-1. ... */, it now finds all cases where the mutating expression is not in the update expression (25e29e4).
| // anywhere in the loop | ||
| } | ||
|
|
||
| for (int i = 0; i < j; i++) { // COMPLIANT: The loop bound `j` is not mutated |
There was a problem hiding this comment.
Case to test: for (int i = 0; i < j++; i++), and maybe for (int i = 0; i++ < j++; i++) while we're at it.
There was a problem hiding this comment.
Sounds good. Two alerts should appear in the second example:
- The loop bound is mutated (the rule in question here).
- The loop counter is mutated outside of the update expression.
| exists(Expr loopCounterExpr | | ||
| loopCounterExpr = this.getAnOperand() and | ||
| loopBound = this.getAnOperand() and | ||
| loopCounter = loopCounterExpr.getAChild*() and |
There was a problem hiding this comment.
getAChild*() is the right tool here, but it must be coupled with an allow-list or we'll have FNs, because it's still casting a very very wide net.
We should ensure (either here, or reported as an error in the query) that loopCounterExpr is not any arbitrary type of expression.
The following should be non-compliant:
for (int i = 0; f(i) < 10; ++i) {}
for (int i = 0; i * i < 10; ++i) {}
for (int i = 0; i + f() < 10; ++i) {}
for (int i = 0; (i > other_var) < 1; ++i) {}
// etc
Basically, we probably just want an allow-list where every expr from loopCounterExpr to loopCounter is either loopCounter itself or an addition/subtraction with only constant values on one side and an allow-listed expression on the other.
for (int i = 0; i + 10 < 20; ++i) {} // OK, `i` is allowed and `ALLOWED + 10` is allowed
for (int i = 0; 10 - i < 20; ++i) {} // OK, `i` is allowed and `10 - ALLOWED` is allowed
for (int i = 0; -i < 20; ++i) {} // OK, `i` is allowed and `-ALLOWED` is allowed
for (int i = 0; -i + 10 - < 20; ++i) {} // OK, `i` is allowed and -ALLOWED is allowed
for (int i = 0; (i + 5) + 3 < 20; ++i) {} // OK, `i` is allowed, `ALLOWED + 5`, and `ALLOWED + 3` is allowed
for (int i = 0; i + (5 + 3) < 20; ++i) {} // OK, `i` is allowed, `ALLOWED + (5 + 3)` is allowed
for (int i = 0; i + i < 20; ++i) {} // BAD, `i` is allowed but `ALLOWED + ALLOWED` is not allowed
for (int i = 0; i + j < 20; ++j) {} // BAD, 'j' is not allowed
for (int i = 0; (i + 10) + (i + 10) < 20; ++i) {} // BAD, `i` and `ALLOWED + 10` is allowed, but `ALLOWED + ALLOWED` is not allowed
Hopefully that mostly makes sense.
|
|
||
| for (int i = j; i < k; i += l) { // COMPLIANT: The loop step is taken as | ||
| // a const pointer | ||
| const int *m = &l; |
There was a problem hiding this comment.
Add tests that int * const m is non compliant
There was a problem hiding this comment.
Added in 2aed0f1.
This refined definition can handle more cases than the previous one that only looked into the loop body, and better matches the description in the comment above.
2 participants
Description
This PR implements the
Statementspackage.Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-9-4-2RULE-9-5-1RULE-9-5-2Release 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.