Skip to content

Implement package preprocessor2 #898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MichaelRFairhurst
Copy link
Contributor

Description

Implement package preprocessor2. Builds on #893

Rule 19-3-4 has a number of reports in popular repositories that are not false positives by the rule text:

  • most commonly, macros like #define M(X, Y) f(X, Y) which do not need parenthesis but the rule text disallows
  • Cases of macros calling macros, #define M1(..., X, ...) ... (X) ..., #define M2(X) M1(..., X, ...) whether this violates the rule by text is ambiguous. This is a subset of the above case. Very difficult to detect.
  • much more rarely, M(arr[1 * 1]) is non-compliant by rule text but clearly safe. MatchingParenthesis module could be extended to exclude these findings, with additional work.
  • Other rare cases, for example #define M(X) {X;}

The matching parenthesis parser is does not have error recovery. There are potentially false negatives when parenthesis are unbalanced, but

  • such cases should be rare, require trickery to even write
  • such cases should indicate a low confidence in our analysis

Adding error recovery would likely be a modest amount of work. An easier solution, though it still suffers from the same two above problems, would be a recursive getDepth function that matches the behavior described in the rule text (depth +1 on '(', depth - 1 on ')', depth unchanged otherwise).

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-19-2-2
    • RULE-19-3-4
    • RULE-19-6-1
  • 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!)

@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 18:17
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new “preprocessor2” package with MISRA C++ queries for include directives, macro argument parenthesis, and pragma usage, along with supporting changes to the matching‐parenthesis and macro utilities.

  • Introduce three new queries for RULE-19-2-2 (invalid #include), RULE-19-3-4 (unparenthesized macro arguments), and RULE-19-6-1 (disallowed #pragma).
  • Extend MatchingParenthesis to handle end‐of‐input and add parameterPrecedenceUnprotected to Macro.qll.
  • Update exclusion metadata and add .qlref/.expected test definitions for the three new rules.

Reviewed Changes

Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cpp/misra/src/rules/RULE-19-2-2/InvalidIncludeDirective.ql New query to enforce <file> or "file" syntax in #include
cpp/misra/src/rules/RULE-19-3-4/UnparenthesizedMacroArgument.ql New query to flag macro arguments missing parentheses around critical operators
cpp/misra/src/rules/RULE-19-6-1/DisallowedUseOfPragma.ql New query disallowing #pragma and _Pragma usage
cpp/common/src/codingstandards/cpp/MatchingParenthesis.qll Regex update to support end‐of‐input in parenthesis matching
cpp/common/src/codingstandards/cpp/Macro.qll Added parameterPrecedenceUnprotected predicate
cpp/common/src/codingstandards/cpp/exclusions/cpp/Preprocessor2.qll Exclusion metadata for new queries
cpp/misra/test/rules/RULE-19-1-1/*.expected & *.qlref Added expected outputs and refs for defined‐operator tests
Comments suppressed due to low confidence (1)

cpp/misra/src/rules/RULE-19-2-2/InvalidIncludeDirective.ql:1

  • No unit tests provided for this new query (and similarly for RULE-19-3-4 and RULE-19-6-1). Please add compliant and non-compliant test cases under cpp/misra/test/rules for full coverage.
/**

result =
".*(" +
concat(string op |
op in [
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex built in criticalOperatorRegex concatenates a trailing '|' inside the capture group, introducing an empty alternative that matches any input and causing hasCriticalOperator to always return true. Remove the trailing '|' or build the pattern without leading/trailing separators.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this is incorrect, as concat(... | v, "|") should function like join() and not add a trailing |.

@@ -61,7 +61,7 @@ module MatchingParenthesis<InputString Input> {
occurrence = prevOccurrence + 1
) else (
token = TNotParen() and
exists(inputStr.regexpFind("\\(|\\)", prevOccurrence + 1, endPos)) and
exists(inputStr.regexpFind("\\(|\\)|$", prevOccurrence + 1, endPos)) and
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including '$' in the regex "\\(|\\)|$" allows matching an empty string at end‐of‐input, which can lead to zero‐length matches and infinite loops. Consider matching only parentheses here and handling end‐of‐input separately.

Suggested change
exists(inputStr.regexpFind("\\(|\\)|$", prevOccurrence + 1, endPos)) and
exists(inputStr.regexpFind("\\(|\\)", prevOccurrence + 1, endPos)) and
(endPos < inputStr.length() or endPos = inputStr.length()) and

Copilot uses AI. Check for mistakes.

@MichaelRFairhurst MichaelRFairhurst requested a review from lcartey May 15, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant