Skip to content

Implement Conversions rule package #919

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 57 commits into
base: main
Choose a base branch
from
Open

Conversation

lcartey
Copy link
Collaborator

@lcartey lcartey commented Jul 7, 2025

Description

This PR adds support for an updated Conversions package.

As a number of the early rules in the original conversions package - particularly Rule 7.0.6 - turned out to be more complex than expected, I've created a Conversions2 package and shuffled around the queries in the following way:

  • Rules 7.0.1, 7.0.2, 7.0.3, 7.0.5, 7.0.6, 7.11.3 are retained in the Conversions package.
  • Rule 7.0.4 is moved into Conversions from Preconditions - as the rule concepts related more closely to the other Conversions queries than to preconditions.
  • The remaining rules are moved into Conversions2.

Implementation notes:

  • This PR introduces the use of the BigInt type, to help support analysis of larger integer constants. In CodeQL CLI 2.19.4, this feature is considered "experimental". Therefore, I would recommend we merge the PR to upgrade the CodeQL dependency to 2.20.7 (Upgrade github/codeql dependency to 2.20.7 #913) before we merge this PR.
  • Many of the queries in this package are backed by the newly created BuiltInTypeRules CodeQL library, which implements the behaviour described in the section 4.7.0 The built-in type rules of MISRA C++ 2023. These are similar in purpose to the essential type rules from MISRA C, but different enough that we needed to provide a clean implementation.
  • This PR introduces improvements to many of the standard library stubs for C++, which were necessary to support the various test cases for the conversion rules.
  • Rule 7.0.5 has a known limitation related to preprocessor directives.

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

lcartey added 30 commits June 10, 2025 12:34
Detects implicit and explicit conversions from type bool to other types,
preventing potential confusion between bitwise and logical operators and
ensuring clear type usage. [a]
 - Simplify query implementation
 - Format test code
Detects implicit conversions to bool type from fundamental types,
unscoped enumerations, and pointers that may lead to unintended behavior
in conditional expressions.
Detects inappropriate assignments between numeric types that violate type
compatibility requirements, including implicit conversions that may cause
information loss or unexpected behavior changes. [a]
 - Add additional test cases
 - Allow name qualifiers
 - Prohibit explicit casts (inc. parameters)
Reference types can be numeric types according to the rule.

In addition to making NumericTypes reference types, we also add
a helper predicate which gets the size of the real type (rather
than the size of the reference).
Migrate conversion generic code to a shared library.
 - Add extra testing
 - Support signed bitfields
 - Exclude booleans and chars from our determination of numeric
   types.
 - Deduplicate integer types deduced for bitfields - identifying
   a canonical set of integer types.
The `getValue()` provided in the database applies the conversions,
which can be unhelpful when trying to write rules the refer to
conversions.
 - Use IntegerConstantExpr to determine both the expressions which
   are constant, and the BigInt value of those constants (before
   final conversion).
 - Implement a BigInt type upper/lower bound to determine whether
   a constant assignment is valid.
 - Support assignment to pointer to members
 - Support pointer-to-member function calls
Overload independence should consider what parameters are default.
Consider the conversion as the source, not the pre-conversion
value.
lcartey added 27 commits June 20, 2025 09:10
These relate to the built-in type rules.
 - Extract the determination of ExprCall FunctionTypes
 - Ensure type matching is inlined
Detects inappropriate implicit conversions from function type to
pointer-to-function type outside of static_cast or assignment contexts.
Prevents ambiguous code where function pointer usage in boolean or
arithmetic expressions may contradict developer intent. [a]
Adds a query that identifies when a numerical value of a
character has been used.

Also fixes a character type category bug, exposed getBuiltinType
and provide a CharacterType class.
Detects integral promotions and arithmetic conversions that change the
signedness or type category of operands, preventing unexpected behavior
from implicit type conversions. [a]
For determining the size of int on the given platform.
Not all integer promotions or usual arithmetic conversions are
actually Conversions in our model.
 - More shift testing
 - Mixed signed/unsigned
AssignOperations do not include a Conversion in the database for
lvalues, so create new classes to capture those cases.
Rule 7.0.4 is now in the Conversions package.
Rules 8.x in the Conversions package are now moved to Conversions2.

This is to avoid having too many complex queries in one package,
and because 7.0.4 is more closely related to the other 7.x rules.
Detects inappropriate operands for bitwise and shift operators that
violate type requirements. Identifies signed operands in bitwise
operations and improper shift operand types that may cause undefined
behavior. [a]
Preprocessor directives are not currently supported.
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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