Implement "Statements" package

[jeongsoolee09]

Description

This PR implements the Statements package.

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-9-4-2
  • RULE-9-5-1
  • RULE-9-5-2
• Queries have been modified for the following rules:
  • rule number here

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!)

[MichaelRFairhurst]

This is really coming along and looking really good!!

[MichaelRFairhurst]

Simple comment formatting, unnecessary split

[jeongsoolee09]

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.

[jeongsoolee09]

Addressed in c8c0770.

[jeongsoolee09]

Somehow this change didn't make it to c8c0770; it did to a recent commit.

[MichaelRFairhurst]

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.

[jeongsoolee09]

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.

[MichaelRFairhurst]

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 return const 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() -- resolves TypdefTypes and DeclTypes, but won't drop the outer specifer in const i64* const. Stops at the first non-TypedefType/non-DeclType.
• stripType() -- resolves all typedefs and decltypes and removes all const/volatile specifiers 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 removing const or volatile specifiers. That would handle typedefs but not int 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.

[jeongsoolee09]

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.

[MichaelRFairhurst]

You may want to add not assignmentRhs.isInUnevaluatedContext() for safety.

That would prevent reporting cases like sizeof(g(&i)) or decltype(g(&i)).

[MichaelRFairhurst]

Feel free to resolve this.

I mostly wanted to point this out because this is a common trap case in coding standards query writing. In C/C++ you can write any expression inside of a sizeof() check, as well as other exprs like decltype()/alignof()/constexpr()/requires()`, and these aren't evaluated.

So if you're looking at a rule that says something like, "Never pass nullptr into std::some_function(x)", then that's the kind of rule where we may want to be careful that sizeof(std::some_function(nullptr)) isn't flagged, because it won't actually execute std::some_function().

In this case, I've convinced myself this isn't something we have to worry about in this case. Hopefully I'm not wrong about that! :)

A really complicated example with TLDR, no need to worry about this.

Here's the most reasonable example that Copilot and I came up with:

template<typename IndexType, typename Observer>
void process_with_observer(std::vector<int>& data) {
    Observer observer;
    
    for (IndexType i = 0; i < data.size(); ++i) {
        // Determine observer's interface at compile time
        using observer_result = decltype(observer(&i));
        
        if constexpr (std::is_void_v<observer_result>) {
            observer(&i);
            data[i] = default_transform(data[i]);
        } else {
            auto metadata = observer(&i);
            data[i] = complex_transform(data[i], metadata);
        }
    }
}

This code is almost reasonable. It would allow you to customize the loop behavior by writing a class with an overloaded () operator......but it's really really strange, and the only "problem" we'd have analyzing this code is that it would flag all three sites to observer(&i) when we would "ideally?" only flag two.

So yeah. No need to worry about it :)

[jeongsoolee09]

Since it's nonetheless a valid use case, I might record this in an issue. Thank you for drawing my attention to these special operators!

[MichaelRFairhurst]

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).

[MichaelRFairhurst]

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.

[jeongsoolee09]

This eliminated a lot of false positives where the counter variable is int and the loop bound is size_t. Thank you!

[MichaelRFairhurst]

A thought on simplifying these names.

Maybe instead of naming them loopVariablePassedAs... you can rename them to passedAsNonConstReference/passedAsNonConstPointer and remove the ForStmt argument.

Then at the call sites you can change

loopVariablePassedAsArgumentToNonConstReferenceParameter(loop, va)
// becomes
exists(VariableAccess other |
  passedAsNonConstReference(other) and
  other.getVariable() = loop.getBound().(VariableAccess).getVariable() and
  other.getEnclosingStmt().getParent*() = loop.getStmt()
)

[jeongsoolee09]

I'm not sure this is a good idea because

• TLoopCounterIsTakenNonConstAddress and friends have to have an identical body, and repeating these lines three times will increase the verbosity.
• The predicates loopVariablePassedAs... and the other one are highly specialized to this query alone, so it doesn't hurt much to keep them amalgamated (if that's a word!) and not break them down.

[MichaelRFairhurst]

I wouldn't worry about this as extra verbosity because the extra code is easy to read. In general, I'd worry more about clear/concise naming.

If verbosity is an issue you can add helper methods to LegacyForLoopCondition

class LegacyForLoopCondition {
  ...
  predicate isBoundAccess(VariableAccess va) {
    va.getVariable() = getBound().(VariableAccess).getVariable() and
    other.getEnclosingStmt().getParent*() = loop.getStmt()
  }
  ...
}

...
  exists(VariableAccess other
    | passedAsNonConstReference(other) and loopCondition.isBoundAccess(other))

You can definitely keep it as is if you prefer, and you may think of a better way to abstract the duplicated code than the isBoundAccess() example here.

Definitely your call!

[Copilot]

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.}

[Copilot]

Fixed grammar: 'constructor call' should be 'constructor calls'.

Suggested change

	std::vector<int>{1, 2, 3}) { // NON_COMPLIANT: 2 constructor call to
	std::vector<int>{1, 2, 3}) { // NON_COMPLIANT: 2 constructor calls to

[Copilot]

Fixed typo 'that that' should be 'that'.

Suggested change

	message = "contains a statement that that is not a simple declaration"
	message = "contains a statement that is not a simple declaration"