RULE-8-2-10: Avoid callers of recursive functions being flagged

[castler]

Description

The rule forbids only recursive functions. It can be fair to have a recursive function and justify this within one. With the behavior before this change, also every caller would need to be argued.

This is additonal burden that is not required and intended by the MISRA standard.

Fixes #935

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 number here
• Queries have been modified for the following rules:
  • RULE-8-2-10

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

[castler]

@MichaelRFairhurst would it be possible to get an approval for the start of the GitHub Actions?

[MichaelRFairhurst]

Hi @castler, huge thanks for the PRs! We're wrapping up some things this week and then we will get to these reviews soon. From a glance, they all look quite good :) really really appreciate you taking the time to help improve the project!! 🥇

This one mostly looks good! I have thoughts related to this comment, which I did confirm is correct: AUTOSAR (and the MISRA 2008 rule it's based on) do contain examples of calls to recursive functions, labeled as non-compliant.

Since AUTOSAR shares this query implementation, and both AUTOSAR and the MISRA 2008 spec the rule flag calls to recursive functions in their examples, we should ideally either:

• Add language to the change note explaining that we've deviated from the examples of A7-5-2 intentionally, and "implementation_scope", or
• Have different implementations for AUTOSAR / MISRA C++23

An extra wart is the exception given for 8-2-10, that functions may be constexpr if called from a recursive context. That exception implies that a call from an outside function may be non-compliant.

One potential easy fix would be to change the isExcluded clause to isExcluded([fc, f], getQuery() so that adding a deviation to the recursive function itself will suppress alerts on all of its call sites.

Would you be interested in following any of those potential next paths? I'd be more than happy to help clarify any of the complex QL involved in any of the possible ways forward.

[castler]

So a colleague of mine already discussed this in one of the last MISRA C++ meetings, and they were pretty clear that the "caller" of a function, just because the function is recursive, shall not be flagged.

Since the normative rule is the same in AUTOSAR and also MISRA 2008, we believe that the examples are just wrong (and also not normative binding).

Thus, we would try to clarify this again within the MISRA forum, to get confirmation for that.

However, I personally have no problem with either of your two options - preferring the first one (based on the MISRA forum outcome).