-
Notifications
You must be signed in to change notification settings - Fork 76
Add Banned2, Banned3, and Banned4
#1080
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
jeongsoolee09
wants to merge
27
commits into
main
Choose a base branch
from
jeongsoolee09/MISRA-C++-2023-Banned234
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
6dc64be
Add rule package of Banned2
jeongsoolee09 21af40d
Add implementation of Banned2
jeongsoolee09 20f7399
Renumber rules in rules.csv
jeongsoolee09 447d961
Draft out first three cases
jeongsoolee09 1a08d11
Add query files
jeongsoolee09 ba3afac
Add `enumFitsInType` and related test cases
jeongsoolee09 c4179a0
Debug enumFitsInType
jeongsoolee09 c91e5c5
Finish the detection logic
jeongsoolee09 cfc6de7
Minor edits of comments
jeongsoolee09 a1f17d3
Finalize 10.2.3 and update expected results
jeongsoolee09 96fdc0c
Update expected results of 10.2.2
jeongsoolee09 4ae8c1d
Use `NestedEnum` instead of hand-rolled definition
jeongsoolee09 7f4ebd4
Add query files for Banned4
jeongsoolee09 075c51f
Merge branch 'main' into jeongsoolee09/MISRA-C++-2023-Banned234
jeongsoolee09 2e62897
Fix formatting of test case for 10.2.2
jeongsoolee09 b4683d2
Merge branch 'jeongsoolee09/MISRA-C++-2023-Banned234' of github.com:g…
jeongsoolee09 538fafa
Disable clang-format for these two
jeongsoolee09 db276dc
Avoid the default case by asserting the existence of case expr
jeongsoolee09 f7d6a49
Include unsigned long long in `result = 64`
jeongsoolee09 4ab4507
Update expected files of 10.2.2 and 10.2.3
jeongsoolee09 113c464
Fix formatting
jeongsoolee09 2cf4b66
Merge branch 'main' into jeongsoolee09/MISRA-C++-2023-Banned234
jeongsoolee09 d9a3371
Update `@problem.severity` metadata
jeongsoolee09 ce1c02a
Update @tags metadata property
jeongsoolee09 69b3c25
Prune out unneeded comments and files
jeongsoolee09 f04a06d
Use clickable placeholder for named enums
jeongsoolee09 98fb963
Merge branch 'main' into jeongsoolee09/MISRA-C++-2023-Banned234
jeongsoolee09 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
26 changes: 26 additions & 0 deletions
26
cpp/common/src/codingstandards/cpp/exclusions/cpp/Banned2.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
| import cpp | ||
| import RuleMetadata | ||
| import codingstandards.cpp.exclusions.RuleMetadata | ||
|
|
||
| newtype Banned2Query = TUnscopedEnumerationsShouldNotBeDeclaredQuery() | ||
|
|
||
| predicate isBanned2QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
| query = | ||
| // `Query` instance for the `unscopedEnumerationsShouldNotBeDeclared` query | ||
| Banned2Package::unscopedEnumerationsShouldNotBeDeclaredQuery() and | ||
| queryId = | ||
| // `@id` for the `unscopedEnumerationsShouldNotBeDeclared` query | ||
| "cpp/misra/unscoped-enumerations-should-not-be-declared" and | ||
| ruleId = "RULE-10-2-2" and | ||
| category = "advisory" | ||
| } | ||
|
|
||
| module Banned2Package { | ||
| Query unscopedEnumerationsShouldNotBeDeclaredQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `unscopedEnumerationsShouldNotBeDeclared` query | ||
| TQueryCPP(TBanned2PackageQuery(TUnscopedEnumerationsShouldNotBeDeclaredQuery())) | ||
| } | ||
| } | ||
26 changes: 26 additions & 0 deletions
26
cpp/common/src/codingstandards/cpp/exclusions/cpp/Banned3.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
| import cpp | ||
| import RuleMetadata | ||
| import codingstandards.cpp.exclusions.RuleMetadata | ||
|
|
||
| newtype Banned3Query = TUnscopedEnumWithoutFixedUnderlyingTypeUsedQuery() | ||
|
|
||
| predicate isBanned3QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
| query = | ||
| // `Query` instance for the `unscopedEnumWithoutFixedUnderlyingTypeUsed` query | ||
| Banned3Package::unscopedEnumWithoutFixedUnderlyingTypeUsedQuery() and | ||
| queryId = | ||
| // `@id` for the `unscopedEnumWithoutFixedUnderlyingTypeUsed` query | ||
| "cpp/misra/unscoped-enum-without-fixed-underlying-type-used" and | ||
| ruleId = "RULE-10-2-3" and | ||
| category = "required" | ||
| } | ||
|
|
||
| module Banned3Package { | ||
| Query unscopedEnumWithoutFixedUnderlyingTypeUsedQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `unscopedEnumWithoutFixedUnderlyingTypeUsed` query | ||
| TQueryCPP(TBanned3PackageQuery(TUnscopedEnumWithoutFixedUnderlyingTypeUsedQuery())) | ||
| } | ||
| } |
26 changes: 26 additions & 0 deletions
26
cpp/common/src/codingstandards/cpp/exclusions/cpp/Banned4.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
| import cpp | ||
| import RuleMetadata | ||
| import codingstandards.cpp.exclusions.RuleMetadata | ||
|
|
||
| newtype Banned4Query = TUnnamedNamespacesInHeaderFilesQuery() | ||
|
|
||
| predicate isBanned4QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
| query = | ||
| // `Query` instance for the `unnamedNamespacesInHeaderFiles` query | ||
| Banned4Package::unnamedNamespacesInHeaderFilesQuery() and | ||
| queryId = | ||
| // `@id` for the `unnamedNamespacesInHeaderFiles` query | ||
| "cpp/misra/unnamed-namespaces-in-header-files" and | ||
| ruleId = "RULE-10-3-1" and | ||
| category = "advisory" | ||
| } | ||
|
|
||
| module Banned4Package { | ||
| Query unnamedNamespacesInHeaderFilesQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `unnamedNamespacesInHeaderFiles` query | ||
| TQueryCPP(TBanned4PackageQuery(TUnnamedNamespacesInHeaderFilesQuery())) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
35 changes: 35 additions & 0 deletions
35
cpp/misra/src/rules/RULE-10-2-2/UnscopedEnumerationsShouldNotBeDeclared.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /** | ||
| * @id cpp/misra/unscoped-enumerations-should-not-be-declared | ||
| * @name RULE-10-2-2: Unscoped enumerations should not be declared | ||
| * @description An unscoped enumeration should not be used outside of a class/struct scope; use | ||
| * 'enum class' instead to prevent name clashes and implicit conversions to integral | ||
| * types. | ||
| * @kind problem | ||
| * @precision very-high | ||
| * @problem.severity warning | ||
| * @tags external/misra/id/rule-10-2-2 | ||
| * scope/single-translation-unit | ||
| * correctness | ||
| * maintainability | ||
| * external/misra/enforcement/decidable | ||
| * external/misra/obligation/advisory | ||
| */ | ||
|
|
||
| import cpp | ||
| import codingstandards.cpp.misra | ||
|
|
||
| class NestedUnscopedEnum extends Enum, NestedEnum { | ||
| NestedUnscopedEnum() { not this instanceof ScopedEnum } | ||
| } | ||
|
|
||
| from Enum enum, string message | ||
| where | ||
| not isExcluded(enum, Banned2Package::unscopedEnumerationsShouldNotBeDeclaredQuery()) and | ||
| not (enum instanceof ScopedEnum or enum instanceof NestedUnscopedEnum) and | ||
| ( | ||
| if enum.isAnonymous() | ||
| then | ||
| message = "This unnamed enumeration is an unscoped and not enclosed in a class or a struct." | ||
| else message = "This enumeration $@ is an unscoped enum not enclosed in a class or a struct." | ||
| ) | ||
| select enum, message, enum, enum.toString() |
239 changes: 239 additions & 0 deletions
239
cpp/misra/src/rules/RULE-10-2-3/UnscopedEnumWithoutFixedUnderlyingTypeUsed.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| /** | ||
| * @id cpp/misra/unscoped-enum-without-fixed-underlying-type-used | ||
| * @name RULE-10-2-3: The numeric value of an unscoped enumeration with no fixed underlying type shall not be used | ||
| * @description Treating unscoped enumeration without a fixed underlying type as an integral type is | ||
| * not portable and might cause unintended behaviors. | ||
| * @kind problem | ||
| * @precision very-high | ||
| * @problem.severity error | ||
| * @tags external/misra/id/rule-10-2-3 | ||
| * scope/single-translation-unit | ||
| * correctness | ||
| * readability | ||
| * external/misra/enforcement/decidable | ||
| * external/misra/obligation/required | ||
| */ | ||
|
|
||
| import cpp | ||
| import codingstandards.cpp.misra | ||
|
|
||
| private predicate isUnscopedEnum(Enum enum) { not enum instanceof ScopedEnum } | ||
|
|
||
| private predicate withoutFixedUnderlyingType(Enum enum) { not enum.hasExplicitUnderlyingType() } | ||
|
|
||
| private predicate isUnscopedEnumWithoutFixedUnderlyingType(Enum enum) { | ||
| isUnscopedEnum(enum) and withoutFixedUnderlyingType(enum) | ||
| } | ||
|
|
||
| class ArithmeticBitwiseLogicalBinaryOperation extends BinaryOperation { | ||
| ArithmeticBitwiseLogicalBinaryOperation() { | ||
| this instanceof BinaryArithmeticOperation or | ||
| this instanceof BinaryBitwiseOperation or | ||
| this instanceof BinaryLogicalOperation | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * ``` C++ | ||
| * static_cast<int>(u) == static_cast<int>(s); // COMPLIANT: comparing ints | ||
| * ``` | ||
| * ^^^ To solve this, we use `getExplicitlyConverted`: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Yes. Conversions are annoying and this is exactly the correct approach 👍 👍 👍 (I didn't know |
||
| * `binOp.getLeftOperand().getExplicitlyConverted()` gives `int`. | ||
| */ | ||
| predicate arithmeticBitwiseLogicalOperationUsesUnscopedUnfixedEnum( | ||
| ArithmeticBitwiseLogicalBinaryOperation binOp, Enum enum | ||
| ) { | ||
| /* | ||
| * We want to strip explicit casts and not implicit ones. Without the | ||
| * stripping of explicit casts, our query would raise a false alarm on | ||
| * cases such as below. | ||
| * | ||
| * ``` C++ | ||
| * static_cast<int>(u) + 1 // COMPLIANT | ||
| * ``` | ||
| */ | ||
|
|
||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| ( | ||
| enum = binOp.getLeftOperand().getExplicitlyConverted().getUnderlyingType() or | ||
| enum = binOp.getRightOperand().getExplicitlyConverted().getUnderlyingType() | ||
| ) | ||
| } | ||
|
|
||
| class RelationalEqualityBinaryOperation extends BinaryOperation { | ||
| RelationalEqualityBinaryOperation() { | ||
| this instanceof RelationalOperation or | ||
| this instanceof EqualityOperation | ||
| } | ||
| } | ||
|
|
||
| predicate relationalEqualityOperationUsesUnscopedUnfixedEnum( | ||
| RelationalEqualityBinaryOperation binOp, Enum enum | ||
| ) { | ||
| exists(Type leftOperandType, Type rightOperandType | | ||
| /* | ||
| * We want to strip explicit casts and not implicit ones. Without the | ||
| * stripping of explicit casts, our query would raise a false alarm on | ||
| * cases such as below. | ||
| * | ||
| * ``` C++ | ||
| * static_cast<int>(u) == 1 // COMPLIANT | ||
| * ``` | ||
| */ | ||
|
|
||
| leftOperandType = binOp.getLeftOperand().getExplicitlyConverted().getUnderlyingType() and | ||
| rightOperandType = binOp.getRightOperand().getExplicitlyConverted().getUnderlyingType() | ||
| | | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| ( | ||
| enum = leftOperandType or | ||
| enum = rightOperandType | ||
| ) and | ||
| leftOperandType != rightOperandType | ||
| ) | ||
| } | ||
|
|
||
| class ArithmeticBitwiseCompoundAssignment extends AssignOperation { | ||
| ArithmeticBitwiseCompoundAssignment() { | ||
| this instanceof AssignArithmeticOperation or | ||
| this instanceof AssignBitwiseOperation | ||
| } | ||
| } | ||
|
|
||
| predicate compoundAssignmentUsesUnscopedUnfixedEnum( | ||
| ArithmeticBitwiseCompoundAssignment compoundAssignment, Enum enum | ||
| ) { | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| enum = compoundAssignment.getAnOperand().getUnderlyingType() | ||
| } | ||
|
|
||
| /** | ||
| * Gets the minimum number of bits required to hold all values of enum `e`. | ||
| */ | ||
| int enumMinBits(Enum e, boolean signed) { | ||
| exists(QlBuiltins::BigInt minVal, QlBuiltins::BigInt maxVal | | ||
| minVal = min(EnumConstant c | c.getDeclaringEnum() = e | c.getValue().toBigInt()) and | ||
| maxVal = max(EnumConstant c | c.getDeclaringEnum() = e | c.getValue().toBigInt()) | ||
| | | ||
| // 8 bits: signed [-128, 127] or unsigned [0, 255] | ||
| if minVal >= "-128".toBigInt() and maxVal <= "127".toBigInt() | ||
| then result = 8 and signed = true | ||
| else | ||
| if minVal >= "0".toBigInt() and maxVal <= "255".toBigInt() | ||
| then ( | ||
| result = 8 and signed = false | ||
| ) else | ||
| // 16 bits: signed [-32768, 32767] or unsigned [0, 65535] | ||
| if minVal >= "-32768".toBigInt() and maxVal <= "32767".toBigInt() | ||
| then ( | ||
| result = 16 and signed = true | ||
| ) else | ||
| if minVal >= "0".toBigInt() and maxVal <= "65535".toBigInt() | ||
| then ( | ||
| result = 16 and signed = false | ||
| ) else | ||
| // 32 bits: signed [-2147483648, 2147483647] or unsigned [0, 4294967295] | ||
| if minVal >= "-2147483648".toBigInt() and maxVal <= "2147483647".toBigInt() | ||
| then ( | ||
| result = 32 and signed = true | ||
| ) else | ||
| if minVal >= "0".toBigInt() and maxVal <= "4294967295".toBigInt() | ||
| then ( | ||
| result = 32 and signed = false | ||
| ) else ( | ||
| // 64 bits: everything else | ||
| result = 64 and signed = [true, false] | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Holds if the enum `e` can fit in an integral type `type`. | ||
| */ | ||
| predicate enumFitsInType(Enum e, IntegralType type) { | ||
| exists(int minBits, boolean signed | minBits = enumMinBits(e, signed) | | ||
| /* If it has exactly the minimum number of bits, then check its signedness. */ | ||
| type.getSize() * 8 = minBits and | ||
| ( | ||
| signed = true and type.isSigned() | ||
| or | ||
| signed = false and type.isUnsigned() | ||
| ) | ||
| or | ||
| /* If it exceeds the minimum number of bits, signedness doesn't matter. */ | ||
| type.getSize() * 8 > minBits | ||
| ) | ||
| } | ||
|
|
||
| predicate assignmentSourceIsUnscopedUnfixedEnum(AssignExpr assign, Enum enum, Type targetType) { | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| enum = assign.getRValue().getUnderlyingType() and | ||
| targetType = assign.getLValue().getUnderlyingType() and | ||
| not enumFitsInType(enum, targetType) and | ||
| not enum = targetType | ||
| } | ||
|
|
||
| predicate staticCastSourceIsUnscopedUnfixedEnumVariant(StaticCast cast, Enum enum, Type targetType) { | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| enum = cast.getExpr().getUnderlyingType() and | ||
| targetType = cast.getUnderlyingType() and | ||
| not enumFitsInType(enum, targetType) and | ||
| not enum = targetType | ||
| } | ||
|
|
||
| predicate switchConditionIsAnUnfixedEnumVariant(SwitchStmt switch, Enum enum, SwitchCase invalidCase) { | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| enum = switch.getExpr().getType() and | ||
| invalidCase = switch.getASwitchCase() and | ||
| invalidCase.getExpr().getUnderlyingType() != enum | ||
| } | ||
|
|
||
| /** | ||
| * Holds if a `static_cast` expression has an unscoped enum without fixed | ||
| * underlying type as the target type. | ||
| */ | ||
| predicate staticCastTargetIsUnscopedUnfixedEnumVariant(StaticCast cast, Enum enum) { | ||
| isUnscopedEnumWithoutFixedUnderlyingType(enum) and | ||
| enum = cast.getType() and | ||
| not cast.getExpr().getType() = enum | ||
| } | ||
|
|
||
| from Element x, Enum enum, string message | ||
| where | ||
| not isExcluded(x, Banned3Package::unscopedEnumWithoutFixedUnderlyingTypeUsedQuery()) and | ||
| ( | ||
| arithmeticBitwiseLogicalOperationUsesUnscopedUnfixedEnum(x, enum) and | ||
| message = | ||
| "Arithmetic, bitwise, or logical operation uses unscoped enum $@ without fixed underlying type." | ||
| or | ||
| relationalEqualityOperationUsesUnscopedUnfixedEnum(x, enum) and | ||
| message = | ||
| "Relational or equality operation compares unscoped enum $@ without fixed underlying type to a different type." | ||
| or | ||
| compoundAssignmentUsesUnscopedUnfixedEnum(x, enum) and | ||
| message = "Compound assignment uses unscoped enum $@ without fixed underlying type." | ||
| or | ||
| exists(Type targetType | | ||
| assignmentSourceIsUnscopedUnfixedEnum(x, enum, targetType) and | ||
| message = | ||
| "Assignment from unscoped enum $@ without fixed underlying type to '" + targetType.getName() | ||
| + "' which may not be large enough." | ||
| ) | ||
| or | ||
| exists(Type targetType | | ||
| staticCastSourceIsUnscopedUnfixedEnumVariant(x, enum, targetType) and | ||
| message = | ||
| "Static cast from unscoped enum $@ without fixed underlying type to '" + | ||
| targetType.getName() + "' which may not be large enough." | ||
| ) | ||
| or | ||
| exists(SwitchStmt switch | | ||
| switchConditionIsAnUnfixedEnumVariant(switch, enum, x) and | ||
| message = | ||
| "Switch on unscoped enum $@ without fixed underlying type has case not of the same enum type." | ||
| ) | ||
| or | ||
| staticCastTargetIsUnscopedUnfixedEnumVariant(x, enum) and | ||
| message = "Static cast to unscoped enum $@ without fixed underlying type." | ||
| ) | ||
| select x, message, enum, enum.getName() | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, we don't need to split rules into their own packages unless they're being split into different PRs. In this case, these can all be part of
Banned2.Also, in this case, three PRs would likely be preferable over one, since the three rules don't have any shared imports in common/ or updates to test/includes/, etc. It is especially valuable to separate 10-2-2, 10-3-1, which are quick to review and approve, from 10-2-3 which has over 200LOC and might take a few back-and-forths to land.
Even the small queries, such as 10-2-2 and 10-3-1, are probably easier to review in isolation going forward because they often a lot of test code each. All in all, one review of 1300loc is harder to find time for than three reviews of ~430loc each, and will take longer to land.
No action required in this case, but to keep in mind going forward 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. I'll isolate shared queries moving forward.