Fix False Positives of M5-0-12
#925
Conversation
- 1. Assigning a char to another char - 1-1. Assigning a char to a char variable - 1-2. Assigning a char to a char member - 1-3. Assigning a char to a char through a pointer - 2. Passing a char argument to a char parameter - 2-1. Passing char argument to a char parameter of a regular function - 2-2. Passing char argument to a char parameter through a template - 2-3. Passing a char argument to a char parameter through a template
…oding-standards into jeongsoolee09/FP-M5-0-12
|
This query runs on openpilot-72d1744 in 17 seconds (on an M1 Max MacBook), and produces one true positive result. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes false positives in rule M5-0-12 by improving the detection of implicit conversions from plain char to explicitly signed/unsigned char types. The fix addresses scenarios involving templates and provides more accurate reporting of where violations occur.
Key Changes:
- Complete rewrite of the query logic to handle template instantiations properly
- Enhanced test coverage with comprehensive template scenarios
- Updated expected results to reflect accurate violation detection
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
SignedCharAndUnsignedCharTypeShallOnlyBeUsedForTheStorageAndUseOfNumericValues.ql |
Complete rewrite with new classes to handle template instantiations and implicit conversions |
test.cpp |
Extensive expansion of test cases covering templates, function calls, and various char type scenarios |
SignedCharAndUnsignedCharTypeShallOnlyBeUsedForTheStorageAndUseOfNumericValues.expected |
Updated expected results reflecting the improved detection logic |
Comments suppressed due to low confidence (2)
cpp/autosar/test/rules/M5-0-12/test.cpp:92
- [nitpick] The variable names 'v1' and 'v2' are not descriptive. Consider using more meaningful names like 'templateVarUnsigned' and 'templateVarSigned'.
v1<unsigned char> =
| TemplateInstantiation templateInstantiation, | ||
| ImplicitConversionFromPlainCharType implicitConversion | ||
| ) { | ||
| implicitConversion.getEnclosingElement+() = templateInstantiation.asElement() |
There was a problem hiding this comment.
The use of getEnclosingElement+() (transitive closure) could be expensive and may match unintended nested elements. Consider using a more specific predicate or adding bounds to limit the scope.
Copilot uses AI. Check for mistakes.
| result = this.asTemplateVariable().toString() | ||
| } | ||
|
|
||
| Location getLocation() { |
There was a problem hiding this comment.
If you added Element getElement() then this could be getElement().getLocation(), and toString() could be getElement().toString()
| @@ -16,23 +16,237 @@ | |||
| import cpp | |||
| import codingstandards.cpp.autosar | |||
|
|
|||
| from Variable v, Expr aexp | |||
| newtype TTemplateElement = | |||
There was a problem hiding this comment.
I love that you're solving this directly. I think this type (and TemplateElement) could be moved into cpp/common/src/codinstandards/cpp under a qll (like Templates.qll or something).
I think it also might be less verbose if you made a common Element subclass, like TemplateElement extends Element with characteristic predicate this instanceof TemplateClass or this instanceof TemplateFunction or ....
| newtype TTemplateInstantiation = | ||
| TClassTemplateInstantiation(ClassTemplateInstantiation c) or | ||
| TFunctionTemplateInstantiation(FunctionTemplateInstantiation f) or | ||
| TVariableTemplateInstantiation(VariableTemplateInstantiation v) |
There was a problem hiding this comment.
again, awesome and could be in a shared qll! And I think these could also be a class that extends Element?
| } | ||
|
|
||
| string toString() { | ||
| result = this.asClassTemplateInstantiation().toString() or |
There was a problem hiding this comment.
This member predicate (and getLocation()) could be result = this.asElement().toString()/.getLocation().
| this.getExpr().getUnspecifiedType() instanceof PlainCharType and | ||
| ( | ||
| this.getUnspecifiedType() instanceof SignedCharType or | ||
| this.getUnspecifiedType() instanceof UnsignedCharType |
There was a problem hiding this comment.
Nice on using .getUnspecifiedType()! I forgot to mention this quirk of writing cpp queries.
| } | ||
| } | ||
|
|
||
| newtype TImplicitConversionElement = |
There was a problem hiding this comment.
This is cool, nicely done.
In theory I think what you've written here could be used even more widely too. In the future, this could be a module under cpp/common/src/codingstandards/cpp/alertreporting with a parameterized module e.g.
signature class ElementSig = Element;
module TemplatableElement<ElementSig Elem> {
newtype TTemplatableElement = TElementOutsideTemplate(Elem elem) { ... }
or
TElementInsideTemplate(TemplateInstantiation templateInstantiation, Elem elem) { ... };
...|
|
||
| string toString() { | ||
| result = this.asImplicitConversionOutsideTemplate().toString() or | ||
| exists(ImplicitConversionFromPlainCharType implicitConversion | |
There was a problem hiding this comment.
can just be result = this.asInstantiationOfImplicitConversionTemplate(_).toString(), both here and in a few other spots
| ) | ||
| } | ||
|
|
||
| Element asElement() { |
There was a problem hiding this comment.
I think this name is a bit confusing, since I wouldn't expect it to call getAUse in the templated case. I might just name this getAUse() or maybe getAnOccurrence() or something like that?
A comment would suffice too!
| "Assignment of an non-integer type to variable $@ which is a variable with an explicitly signed char type", | ||
| v, v.getName() | ||
| implicitConversion = implicitConversionLocation.getImplicitConversion() | ||
| select implicitConversionLocation.asElement(), getMessageTemplate(implicitConversionLocation), |
There was a problem hiding this comment.
select implicitConversionLocation, ... might also work, since you define getLocation()
Templates are tricky issue; we'd like to address this in a different PR.
|
I have reduced the scope of this query to only deal with cases that does not involve templates, as templates are difficult topic and deserves a discussion of its own. Subsequently, both the test cases and the results are trimmed down. |
Description
Fix false positives of
M5-0-12(Closes #541).Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
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.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
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
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.
Reviewer
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.