Simple comment formatting, unnecessary split

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.

Addressed in c8c0770.

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

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.

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.

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.

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.

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

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

Changed the upperBound(loopCounter.getFullyConverted()) to typeUpperBound(loopCounter.getType()). As typeUpperBound resolves references, I didn't have to use getBaseType() on it.

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.

OK, I thought that this code was intended to handle the following case, based on real code examples in open pilot:

for (int i = 0; i + 10 < j; ++i) {}

I believe we have leeway to be able to interpret 9-5-1 to accept or reject this. It's trivially clear that i + 10 < j is equivalent to i < j + 10. It's fair to say that the MISRA team doesn't think of every edge case and that we have some discretion here. It's also fair to say that this violates the plain text of the rule.

I'm happy with us rejecting the above case if you think so. However, I don't think the query currently rejects it because of the .getAChild*(). Quickly looking for calls to getLoopCounter, I don't see anything that later checks that getLoopCounter() is a direct operand of the LegacyForLoopCondition.

I believe that 9-5-1 will also currently not flag

for (int i = 0; f(i) < j; ++i) {}

which is clearly intended to be non-compliant.

So we should add tests for these cases. We should test f(i) < j case is flagged as non compliant, and we should add a test for i + 10 < j with a comment about how we came to decide that it should or shouldn't be considered compliant.

My preference would be to remove getAChild*(), and either replace it with getAnOperand() or a predicate that finds a restricted set of operations. Currently LegacyForLoopCondition is only used here, and we can document its restrictions. If you would rather leave it as is and add checks in 9-5-1 that getLoopCounter() = loopCondition.getAnOperand(), that is reasonable too. But getAChild*() casts a very wide net that might not be expected in future queries, so I wouldn't say we need to have that behavior now.

Okay, I believe it is reasonable to do so. Thank you for your second opinion!

I believe this is incorrect. Take the following example:

01  void f(int &args...);
02  void g(int args...);
03  void main() {
04    for(int i = 0; i < 10; ++i) {
05     f(i);
06     g(i);
07   }
08  }

In this example, we want to report f(i) as non-compliant while g(i) is OK. This code is looking to see if i is converted to a const or a non-const reference type. However, it's looking at the wrong i.

loopVariableAccessInCondition refers to the i on line 3. But what we want to see is if the is on lines 4 and 5 are converted to const int& or int&.

We should add tests for this case as well.