Skip to content

Implement Conversions2 rule package #946

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

Conversation

lcartey
Copy link
Collaborator

@lcartey lcartey commented Aug 18, 2025

Description

Follows on from #919.

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-8-2-1
    • RULE-8-2-2
    • RULE-8-2-6
    • RULE-8-2-7
    • RULE-8-2-8
    • `RULE-9-2-1
  • Queries have been modified for the following rules:
    • RULE-1-3
    • RULE-23-3
    • RULE-23-5
    • RULE-23-6

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 26 commits June 27, 2025 13:17
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.
This shared implementation will be used for implementing MISRA C++
2023 8.2.1.
MISRA C++ Rule 8.2.2 is similar, but not identical.
Nested macro invocations do not have getExpandedElements(), but
do have getAffectedElements().
Detects C-style casts and functional notation casts that should be
replaced with explicit cast operators. Prevents unsafe type conversions
that lack clear intent and proper type checking constraints. [a]
Detects casts from integral, enumerated, or void pointer types to object
pointer types that may lead to unspecified behavior. [a]
Detects casts that convert pointer types to integral types, which can
make code harder to understand and may break pointer tracking in analysis
tools. [a]
Detects pointer-to-integral casts that use types other than
std::uintptr_t or std::intptr_t, which may not guarantee representation
of all pointer values. [a]
Detects explicit type conversions using functional notation as standalone
expression statements that create immediately-destroyed temporary
objects. [a]
 - Simplify the query implementation
 - Improve query message
 - Remove redundant code
 - Use stripSpecifiers(..) to check for use of (u)intptr_t.
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.

Query getQuery() { result instanceof PointerToAVirtualBaseClassCastToAPointerSharedQuery }

query predicate problems(Cast cast, string message) {
exists(VirtualBaseClass castFrom, Class castTo |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be non compliant when castFrom is a base class of a virtual base class

not stripSpecifiers(targetType).(UserType).hasGlobalOrStdName(["uintptr_t", "intptr_t"])
select cast,
"Cast of object pointer type to integral type '" + targetType +
"' instead of 'std::uintptr_t' or 'std::intptr_t'."
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately the rule has additional requirements not covered here related to templates. The rule states that the casted type must be "explicitly" uintptr_t or intptr_t, meaning that

template<typename T>
void foo(S* s) {
  reinterpret_cast<T>(s);
}

is non compliant, which isn't covered here

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.

2 participants