-
Notifications
You must be signed in to change notification settings - Fork 76
Narrowly focused implementation of RULE 15-0-1 #1121
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
MichaelRFairhurst
wants to merge
17
commits into
main
Choose a base branch
from
michaelrfairhurst/classes-3-take-2-rule-15-0-1
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
17 commits
Select commit
Hold shift + click to select a range
a31cf70
First round tests
MichaelRFairhurst ba6c2ca
First implementation
MichaelRFairhurst a31aac3
All tests passing for main query
MichaelRFairhurst 057e33b
Implement audit query
MichaelRFairhurst 3584a15
Add some cases
jeongsoolee09 d37827a
Use correct audit tag
MichaelRFairhurst 5000f27
Just omit functions by hand that's supposed to not be declared
jeongsoolee09 184da86
Merge branch 'michaelrfairhurst/classes-3-take-2-rule-15-0-1' of gith…
jeongsoolee09 e63c088
Fix RuleMetadata.qll (package from a different rule/branch)
MichaelRFairhurst 658571a
clang format
MichaelRFairhurst 6e9dde1
Change the label of a case and break it down
jeongsoolee09 d04d4df
Merge branch 'michaelrfairhurst/classes-3-take-2-rule-15-0-1' of gith…
jeongsoolee09 96e2ed0
Some formatting, make copy-enabled class no longer copy-assignable
MichaelRFairhurst ce43802
Handle suppressed move operations
MichaelRFairhurst a1c1a9e
Updates to expected file
MichaelRFairhurst 2813136
Address feedback
MichaelRFairhurst 51985b9
Give code examples for when copy operations masquerade as moves.
MichaelRFairhurst 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
44 changes: 44 additions & 0 deletions
44
cpp/common/src/codingstandards/cpp/exclusions/cpp/Classes3.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,44 @@ | ||
| //** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
| import cpp | ||
| import RuleMetadata | ||
| import codingstandards.cpp.exclusions.RuleMetadata | ||
|
|
||
| newtype Classes3Query = | ||
| TImproperlyProvidedSpecialMemberFunctionsQuery() or | ||
| TImproperlyProvidedSpecialMemberFunctionsAuditQuery() | ||
|
|
||
| predicate isClasses3QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
| query = | ||
| // `Query` instance for the `improperlyProvidedSpecialMemberFunctions` query | ||
| Classes3Package::improperlyProvidedSpecialMemberFunctionsQuery() and | ||
| queryId = | ||
| // `@id` for the `improperlyProvidedSpecialMemberFunctions` query | ||
| "cpp/misra/improperly-provided-special-member-functions" and | ||
| ruleId = "RULE-15-0-1" and | ||
| category = "required" | ||
| or | ||
| query = | ||
| // `Query` instance for the `improperlyProvidedSpecialMemberFunctionsAudit` query | ||
| Classes3Package::improperlyProvidedSpecialMemberFunctionsAuditQuery() and | ||
| queryId = | ||
| // `@id` for the `improperlyProvidedSpecialMemberFunctionsAudit` query | ||
| "cpp/misra/improperly-provided-special-member-functions-audit" and | ||
| ruleId = "RULE-15-0-1" and | ||
| category = "required" | ||
| } | ||
|
|
||
| module Classes3Package { | ||
| Query improperlyProvidedSpecialMemberFunctionsQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `improperlyProvidedSpecialMemberFunctions` query | ||
| TQueryCPP(TClasses3PackageQuery(TImproperlyProvidedSpecialMemberFunctionsQuery())) | ||
| } | ||
|
|
||
| Query improperlyProvidedSpecialMemberFunctionsAuditQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `improperlyProvidedSpecialMemberFunctionsAudit` query | ||
| TQueryCPP(TClasses3PackageQuery(TImproperlyProvidedSpecialMemberFunctionsAuditQuery())) | ||
| } | ||
| } |
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
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,258 @@ | ||||||||||
| /** | ||||||||||
| * Provides predicates and classes to perform a precursor analysis of which classes the rule can | ||||||||||
| * potentially analyze, or should be excluded and instead selected by the audit query. | ||||||||||
| * | ||||||||||
| * For example, the class `AnalyzableClass` resolves all of the special member functions that we | ||||||||||
| * must have in order to determine rule compliance. | ||||||||||
| * | ||||||||||
| * Part of what this module does is perform a very approximate analysis of which classes will | ||||||||||
| * produce a value of true in `std::is_(copy|move)_(constructible|assignable)_v`. | ||||||||||
| * | ||||||||||
| * Fully predicting these standard type traits requires performing a very thorough overload | ||||||||||
| * resolution analysis, including value category propagation and reference binding and user defined | ||||||||||
| * conversion operators and standard conversions and promotions and ranking viable candidates and | ||||||||||
| * properly handling ambiguous overloads. | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| import cpp | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * For `std::is_(copy|move)_(constructible|assignable)_v` to return true for a given class, the | ||||||||||
| * member must exist, it must not be deleted, and it must be publicly accessible. | ||||||||||
| * | ||||||||||
| * This is a very coarse approximation of true behavior of the standard type traits. | ||||||||||
| */ | ||||||||||
| private predicate isUsable(MemberFunction f) { | ||||||||||
| not f.isDeleted() and | ||||||||||
| f.isPublic() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if the member function `f` has been "customized" by the user, e.g., they explicitly wrote | ||||||||||
| * the implementation of the function. | ||||||||||
| */ | ||||||||||
| private predicate isMemberCustomized(MemberFunction f) { | ||||||||||
| exists(f.getDefinition()) and | ||||||||||
| not f.isDefaulted() and | ||||||||||
| not f.isDeleted() and | ||||||||||
| not f.isCompilerGenerated() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if the user declared the member function `f`, as opposed to it being implicitly declared | ||||||||||
| * by the compiler. | ||||||||||
| * | ||||||||||
| * Note that `T(T&&) = default` and `T(T&&) = delete` are both user declared. This is not to be | ||||||||||
| * confused with "user defined." | ||||||||||
| */ | ||||||||||
| private predicate isUserDeclared(MemberFunction f) { not f.isCompilerGenerated() } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if the implicit move constructor or move assignment operator of the class `c` will not be | ||||||||||
| * declared. | ||||||||||
| * | ||||||||||
| * See [class.copy]/8 and [class.copy] | ||||||||||
|
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.
Suggested change
|
||||||||||
| */ | ||||||||||
| private predicate implicitMoveIsSuppressed(Class c) { | ||||||||||
| isUserDeclared(c.getAConstructor().(CopyConstructor)) | ||||||||||
| or | ||||||||||
| isUserDeclared(c.getAMemberFunction().(CopyAssignmentOperator)) | ||||||||||
| or | ||||||||||
| isUserDeclared(c.getDestructor()) | ||||||||||
|
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. It may be missing a branch.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Returns the move constructor of the class `c` if it exists, or the copy constructor if it does | ||||||||||
| * not exist and the implicit definition was suppressed by the compiler. | ||||||||||
| * | ||||||||||
| * For example: | ||||||||||
| * ```cpp | ||||||||||
| * class OnlyCopyCtor { | ||||||||||
| * public: | ||||||||||
| * OnlyCopyCtor(const OnlyCopyCtor &) = default; | ||||||||||
| * }; | ||||||||||
| * | ||||||||||
| * static_assert(std::is_copy_constructible_v<OnlyCopyCtor>); // Succeeds | ||||||||||
| * static_assert(std::is_move_constructible_v<OnlyCopyCtor>); // Also succeeds | ||||||||||
| * ``` | ||||||||||
| * | ||||||||||
| * Note that without the declared copy constructor, the compiler may define an implicit move | ||||||||||
| * constructor. | ||||||||||
| * | ||||||||||
| * Additionally note that if the move constructor was declared as `= delete;`, then the second | ||||||||||
| * assertion in the above example would fail. | ||||||||||
| */ | ||||||||||
| private Constructor getMoveConstructor(Class c) { | ||||||||||
| if | ||||||||||
| not exists(MoveConstructor mc | mc = c.getAConstructor() and isUserDeclared(mc)) and | ||||||||||
| implicitMoveIsSuppressed(c) | ||||||||||
| then result = c.getAConstructor().(CopyConstructor) | ||||||||||
| else result = c.getAConstructor().(MoveConstructor) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Returns the move assignment operator of the class `c` if it exists, or the copy assignment | ||||||||||
| * operator if it does not exist and the implicit definition was suppressed by the compiler. | ||||||||||
| * | ||||||||||
| * For example: | ||||||||||
| * ```cpp | ||||||||||
| * class OnlyCopyAssign { | ||||||||||
| * public: | ||||||||||
| * OnlyCopyAssign& operator=(const OnlyCopyAssign &) = default; | ||||||||||
| * }; | ||||||||||
| * | ||||||||||
| * static_assert(std::is_copy_assignable_v<OnlyCopyAssign>); // Succeeds | ||||||||||
| * static_assert(std::is_move_assignable_v<OnlyCopyAssign>); // Also succeeds | ||||||||||
| * ``` | ||||||||||
| * | ||||||||||
| * Note that without the declared copy assignment operator, the compiler may define an implicit move | ||||||||||
| * assignment operator. | ||||||||||
| * | ||||||||||
| * Additionally note that if the move assignment operator was declared as `= delete;`, then the second | ||||||||||
| * assertion in the above example would fail. | ||||||||||
| */ | ||||||||||
| private Operator getMoveAssign(Class c) { | ||||||||||
| if | ||||||||||
| not exists(MoveAssignmentOperator mc | mc = c.getAMemberFunction() and isUserDeclared(mc)) and | ||||||||||
| implicitMoveIsSuppressed(c) | ||||||||||
| then result = c.getAMemberFunction().(CopyAssignmentOperator) | ||||||||||
| else result = c.getAMemberFunction().(MoveAssignmentOperator) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * The types of special member functions that the `AnalyzableClass` tracks and analyzes. | ||||||||||
| */ | ||||||||||
| newtype TSpecialMember = | ||||||||||
| TMoveConstructor() or | ||||||||||
| TMoveAssignmentOperator() or | ||||||||||
| TCopyConstructor() or | ||||||||||
| TCopyAssignmentOperator() or | ||||||||||
| TDestructor() | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * A class for which we can see all special member functions, including implicitly declared ones, | ||||||||||
| * and therefore we can attempt to analyze it in the current rule. | ||||||||||
| * | ||||||||||
| * If one of the special member functions cannot be found, we cannot know if it is missing because | ||||||||||
| * it should not have been generated, or if EDG did not emit a definition for it. For instance, EDG | ||||||||||
| * may not generate these functions if they are trivial, or if they are delete, or not ODR used. We, | ||||||||||
| * the authors of this project, do not know the exact conditions we have to consider in this case. | ||||||||||
| * | ||||||||||
| * Determining for ourselves whether a certain constructor would be implicitly declared, and with | ||||||||||
| * what signature, and whether it is deleted, requires implementing a significant portion of the C++ | ||||||||||
| * language rules regarding special member function generation, including a significant portion of | ||||||||||
| * C++ overload resolution rules which are non-trivial. | ||||||||||
| * | ||||||||||
| * Therefore we must find a definition for each special member in the database to proceed. The only | ||||||||||
| * exception we allow is certain missing `MoveConstructor` or `MoveAssignmentOperator` members; if | ||||||||||
| * the class defines copy operations or the destructor, we expect these to be missing, and typically | ||||||||||
| * this means the corresponding copy operation acts in place of the equivalent move. | ||||||||||
| * | ||||||||||
| * The last difficulty in analysis that this class attempts to handle is the values of the type | ||||||||||
| * traits `std::is_(copy|move)_(constructible|assignable)`. These type traits are defined as true if | ||||||||||
| * certain C++ expressions, such as `T(declval<T>())` or `declval<T>() = declval<T>()`, are | ||||||||||
| * well-formed. We cannot correctly determine this in all cases without implementing a significant | ||||||||||
| * portion of the C++ language rules for reference binding and overload resolution. | ||||||||||
| * | ||||||||||
| * To handle these type traits, we take a very rough approximation. If the corresponding special | ||||||||||
| * member function is public and not deleted, then we assume the type trait will evaluate to true. | ||||||||||
| * We also handle the case where a user declared copy operation suppresses the implicit move | ||||||||||
| * operations, which typically means overload resolution selects the copy operation. (This is not | ||||||||||
| * the case when the move operations are declared as deleted). We handle this by treating the copy | ||||||||||
| * operation as effectively acting in place of the move operation for the purposes of evaluating | ||||||||||
| * the type traits. | ||||||||||
| */ | ||||||||||
| class AnalyzableClass extends Class { | ||||||||||
| CopyConstructor copyCtor; | ||||||||||
| // The move constructor may be suppressed, and the copy constructor may be used during moves. | ||||||||||
|
MichaelRFairhurst marked this conversation as resolved.
|
||||||||||
| Constructor moveCtor; | ||||||||||
| CopyAssignmentOperator copyAssign; | ||||||||||
| // The move assignment operator may be suppressed, and the copy assignment operator may be used during moves. | ||||||||||
| Operator moveAssign; | ||||||||||
| Destructor dtor; | ||||||||||
|
|
||||||||||
| AnalyzableClass() { | ||||||||||
| copyCtor = this.getAConstructor() and | ||||||||||
| moveCtor = getMoveConstructor(this) and | ||||||||||
| copyAssign = this.getAMemberFunction() and | ||||||||||
| moveAssign = getMoveAssign(this) and | ||||||||||
| dtor = this.getDestructor() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds `std::is_move_constructible_v<T>` is likely true for this class. | ||||||||||
| * | ||||||||||
| * Specifically this holds if there's a non-deleted public move constructor available for this | ||||||||||
| * class, or if there is a non-deleted public copy constructor that acts as the move constructor. | ||||||||||
| */ | ||||||||||
| predicate moveConstructible() { isUsable(moveCtor) } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds `std::is_copy_constructible_v<T>` is likely true for this class. | ||||||||||
| * | ||||||||||
| * Specifically this holds if there's a non-deleted public copy constructor available for this | ||||||||||
| * class. | ||||||||||
| */ | ||||||||||
| predicate copyConstructible() { isUsable(copyCtor) } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds `std::is_move_assignable_v<T>` is likely true for this class. | ||||||||||
| * | ||||||||||
| * Specifically this holds if there's a non-deleted public move assignment operator available for | ||||||||||
| * this class, or if there is a non-deleted public copy assignment operator that acts as the move | ||||||||||
| * assignment operator. | ||||||||||
| */ | ||||||||||
| predicate moveAssignable() { isUsable(moveAssign) } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds `std::is_copy_assignable_v<T>` is likely true for this class. | ||||||||||
| * | ||||||||||
| * Specifically this holds if there's a non-deleted public copy assignment operator available for | ||||||||||
| * this class. | ||||||||||
| */ | ||||||||||
| predicate copyAssignable() { isUsable(copyAssign) } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if the given special member function `s` is customized for this class. | ||||||||||
| * | ||||||||||
| * For most cases, this checks that the given special member function `s` has a user-provided | ||||||||||
| * body (other than `= default;` or `= delete;`). | ||||||||||
| * | ||||||||||
| * If the class has copy operations that act in place of the move operations, that means the | ||||||||||
| * corresponding move operation was not declared, so we say this predicate does not hold for the | ||||||||||
| * given move operation `s`. | ||||||||||
| */ | ||||||||||
| predicate isCustomized(TSpecialMember s) { | ||||||||||
| s instanceof TMoveConstructor and | ||||||||||
| isMemberCustomized(moveCtor) and | ||||||||||
| declaresMoveConstructor() | ||||||||||
| or | ||||||||||
| s instanceof TMoveAssignmentOperator and | ||||||||||
| isMemberCustomized(moveAssign) and | ||||||||||
| declaresMoveAssignmentOperator() | ||||||||||
| or | ||||||||||
| s instanceof TCopyConstructor and isMemberCustomized(copyCtor) | ||||||||||
| or | ||||||||||
| s instanceof TCopyAssignmentOperator and isMemberCustomized(copyAssign) | ||||||||||
| or | ||||||||||
| s instanceof TDestructor and isMemberCustomized(dtor) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if this class declares a move constructor. | ||||||||||
| * | ||||||||||
| * This will be true if move constructor resolution found a non-implicit constructor that is not | ||||||||||
| * the copy constructor masquerading as a move constructor. | ||||||||||
| */ | ||||||||||
| predicate declaresMoveConstructor() { not moveCtor = copyCtor and isUserDeclared(moveCtor) } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Holds if this class declares a move assignment operator. | ||||||||||
| * | ||||||||||
| * This will be true if move assignment resolution found a non-implicit operator that is not | ||||||||||
| * the copy assignment operator masquerading as a move assignment operator. | ||||||||||
| */ | ||||||||||
| predicate declaresMoveAssignmentOperator() { | ||||||||||
| not moveAssign = copyAssign and isUserDeclared(moveAssign) | ||||||||||
| } | ||||||||||
| } | ||||||||||
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.
Uh oh!
There was an error while loading. Please reload this page.