-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: main
Are you sure you want to change the base?
Changes from 14 commits
a31cf70
ba6c2ca
a31aac3
057e33b
3584a15
d37827a
5000f27
184da86
e63c088
658571a
6e9dde1
d04d4df
96e2ed0
ce43802
a1c1a9e
2813136
51985b9
61a4dd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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())) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,112 @@ | ||||||||||
| import cpp | ||||||||||
|
|
||||||||||
| private predicate isUsable(MemberFunction f) { | ||||||||||
| not f.isDeleted() and | ||||||||||
| f.isPublic() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private predicate isMemberCustomized(MemberFunction f) { | ||||||||||
| exists(f.getDefinition()) and | ||||||||||
| not f.isDefaulted() and | ||||||||||
| not f.isDeleted() and | ||||||||||
| not f.isCompilerGenerated() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| 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
Collaborator
Author
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. Fixed in 61a4dd9! |
||||||||||
| */ | ||||||||||
| private predicate implicitMoveIsSuppressed(Class c) { | ||||||||||
| isUserDeclared(c.getAConstructor().(CopyConstructor)) | ||||||||||
| or | ||||||||||
| isUserDeclared(c.getAConstructor().(CopyAssignmentOperator)) | ||||||||||
|
MichaelRFairhurst marked this conversation as resolved.
Outdated
|
||||||||||
| 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
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| 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) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| 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) | ||||||||||
| } | ||||||||||
|
jeongsoolee09 marked this conversation as resolved.
Outdated
|
||||||||||
|
|
||||||||||
| newtype TSpecialMember = | ||||||||||
| TMoveConstructor() or | ||||||||||
| TMoveAssignmentOperator() or | ||||||||||
| TCopyConstructor() or | ||||||||||
| TCopyAssignmentOperator() or | ||||||||||
| TDestructor() | ||||||||||
|
|
||||||||||
| 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() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| predicate exposes(TSpecialMember m) { | ||||||||||
| m instanceof TMoveConstructor and moveConstructible() | ||||||||||
| or | ||||||||||
| m instanceof TMoveAssignmentOperator and moveAssignable() | ||||||||||
| or | ||||||||||
| m instanceof TCopyConstructor and copyConstructible() | ||||||||||
| or | ||||||||||
| m instanceof TCopyAssignmentOperator and copyAssignable() | ||||||||||
| or | ||||||||||
| m instanceof TDestructor and destructible() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| predicate moveConstructible() { isUsable(moveCtor) } | ||||||||||
|
|
||||||||||
| predicate copyConstructible() { isUsable(copyCtor) } | ||||||||||
|
|
||||||||||
| predicate moveAssignable() { isUsable(moveAssign) } | ||||||||||
|
|
||||||||||
| predicate copyAssignable() { isUsable(copyAssign) } | ||||||||||
|
|
||||||||||
| predicate destructible() { isUsable(dtor) } | ||||||||||
|
|
||||||||||
| 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) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| predicate declaresMoveConstructor() { not moveCtor = copyCtor } | ||||||||||
|
|
||||||||||
| predicate declaresMoveAssignmentOperator() { not moveAssign = copyAssign } | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| /** | ||
| * @id cpp/misra/improperly-provided-special-member-functions | ||
| * @name RULE-15-0-1: Special member functions shall be provided appropriately | ||
| * @description Incorrect provision of special member functions can lead to unexpected or undefined | ||
| * behavior when objects of the class are copied, moved, or destroyed. | ||
| * @kind problem | ||
| * @precision medium | ||
| * @problem.severity warning | ||
| * @tags external/misra/id/rule-15-0-1 | ||
| * scope/single-translation-unit | ||
| * correctness | ||
| * maintainability | ||
| * external/misra/enforcement/decidable | ||
| * external/misra/obligation/required | ||
| */ | ||
|
|
||
| import cpp | ||
| import codingstandards.cpp.misra | ||
| import AnalyzableClass | ||
|
|
||
| predicate isCopyEnabled(AnalyzableClass c) { | ||
| c.moveConstructible() and | ||
| not c.moveAssignable() and | ||
| c.copyConstructible() and | ||
| not c.copyAssignable() | ||
| or | ||
| c.moveConstructible() and | ||
| c.moveAssignable() and | ||
| c.copyConstructible() and | ||
| c.copyAssignable() | ||
| } | ||
|
|
||
| predicate isMoveOnly(AnalyzableClass c) { | ||
| c.moveConstructible() and | ||
| not c.moveAssignable() and | ||
| not c.copyConstructible() and | ||
| not c.copyAssignable() | ||
| or | ||
| c.moveConstructible() and | ||
| c.moveAssignable() and | ||
| not c.copyConstructible() and | ||
| not c.copyAssignable() | ||
| } | ||
|
|
||
| predicate isUnmovable(AnalyzableClass c) { | ||
| not c.moveConstructible() and | ||
| not c.moveAssignable() and | ||
| not c.copyConstructible() and | ||
| not c.copyAssignable() | ||
| } | ||
|
|
||
| predicate isValidCategory(AnalyzableClass c) { | ||
| isCopyEnabled(c) or | ||
| isMoveOnly(c) or | ||
| isUnmovable(c) | ||
| } | ||
|
|
||
| string specialMemberName(TSpecialMember f) { | ||
| f = TCopyConstructor() and result = "copy constructor" | ||
| or | ||
| f = TMoveConstructor() and result = "move constructor" | ||
| or | ||
| f = TCopyAssignmentOperator() and result = "copy assignment operator" | ||
| or | ||
| f = TMoveAssignmentOperator() and result = "move assignment operator" | ||
| } | ||
|
|
||
| predicate violatesCustomizedMoveOrCopyRequirements(AnalyzableClass c, string reason) { | ||
| not c.isCustomized(TDestructor()) and | ||
| exists(string concatenated | | ||
| concatenated = | ||
| strictconcat(TSpecialMember f | | ||
| not f = TDestructor() and | ||
| c.isCustomized(f) | ||
| | | ||
| specialMemberName(f), ", " | ||
| ) and | ||
| reason = "has customized " + concatenated + ", but does not customize the destructor." | ||
| ) | ||
| } | ||
|
|
||
| private predicate undeclaredMoveException(AnalyzableClass c) { | ||
| // A copy-enabled class may have an undeclared move constructor. | ||
| isCopyEnabled(c) and | ||
| not c.moveAssignable() and | ||
|
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. Why is this condition needed?
Collaborator
Author
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. This (and the next comment) are attempting to check if the class is copy-assignable vs copy-enabled. If a class is copyEnabled, we can check for the existence of either assignment operator to distinguish copy-enabled from copy-assignable, because a copy-assignable class will have both and a copy-enabled class will have neither. It took me too long to remember that that's why I wrote it. I'll write a helper predicate that makes this clearer.
Collaborator
Author
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. Ah, I see. Yes, you're right. I forgot that Fixed in 61a4dd9! |
||
| not c.declaresMoveConstructor() | ||
| or | ||
| // A copy-assignable class may leave both move operations undeclared. | ||
| isCopyEnabled(c) and | ||
| c.moveAssignable() and | ||
|
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. Shouldn't this be
Collaborator
Author
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. Fixed in 61a4dd9! |
||
| not c.declaresMoveConstructor() and | ||
| not c.declaresMoveAssignmentOperator() | ||
| } | ||
|
|
||
| predicate violatesCustomizedDestructorRequirements(AnalyzableClass c, string reason) { | ||
| c.isCustomized(TDestructor()) and | ||
| ( | ||
| c.moveConstructible() and | ||
| not c.isCustomized(TMoveConstructor()) and | ||
| not undeclaredMoveException(c) and | ||
| reason = "has customized the destructor, but does not customize the move constructor." | ||
| or | ||
| c.moveAssignable() and | ||
| not undeclaredMoveException(c) and | ||
| not c.isCustomized(TMoveAssignmentOperator()) and | ||
| reason = "has customized the destructor, but does not customize the move assignment operator." | ||
| or | ||
| c.copyConstructible() and | ||
| not c.isCustomized(TCopyConstructor()) and | ||
| reason = "has customized the destructor, but does not customize the copy constructor." | ||
| or | ||
| c.copyAssignable() and | ||
| not c.isCustomized(TCopyAssignmentOperator()) and | ||
| reason = "has customized the destructor, but does not customize the copy assignment operator." | ||
| ) | ||
| } | ||
|
Comment on lines
+82
to
+115
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 was a bit hard for me to follow the logic of this requirement check. Since these come in the form of wouldn't it be simpler to check it using by taking the negation of the above? For example, the last requirement translates to c.copyAssignable() implies (
c.isCustomized(TCopyAssignmentOperator()) and
(
c.isCustomized(TMoveConstructor()) and c.isCustomized(TMoveAssignmentOperator()) or
not isUserDeclared(getMoveConstructor(c)) and not isUserDeclared(getMoveAssign(c))
)
)So the check becomes: c.copyAssignable() and not (
c.isCustomized(TCopyAssignmentOperator()) and
(
c.isCustomized(TMoveConstructor()) and c.isCustomized(TMoveAssignmentOperator()) or
not isUserDeclared(getMoveConstructor(c)) and not isUserDeclared(getMoveAssign(c))
)
)Rewriting above gives an arguably more readable version: c.copyAssignable() and (
/* 1. The copy assignment operator is not customized. */
not c.isCustomized(TCopyAssignmentOperator()) or
(
/* 2. Any one (or both) of the move operations is not customized. */
not c.isCustomized(TMoveConstructor() or not c.isCustomized(TMoveAssignmentOperator()) and
/* 3. Any one (or both) of the move operations are user-declared. */
(isUserDeclared(getMoveConstructor(c)) or isUserDeclared(getMoveAssign(c)))
)
)
Collaborator
Author
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. If I'm understanding correctly, I think this is simpler. The bullet points 2 and 3 can essentially be rephrased as:
The only exceptions are:
This matches my implementation above. We can prove each item:
If it is copy constructible, it can only be copy enabled or copy assignable. Copy enabled classes must have a customized copy constructor, and copy assignable classes must have a customized copy constructor. Erg, all copy constructible classes must have a customized copy constructor. Inversely, if it must have a customized copy constructor, it must be either copy constructible or copy assignable -- the other three categories have no restrictions on the copy constructor.
This one is a direct quote: "if it is copy-assignable it shall also have a customized copy assignment operator." No other categories place any requirements on the copy assignment operator.
If it's move constructible, it can belong to any category except unmovable. If it is move-only or move-assignable, it must have a customized move constructor (bullet 2). If it is copy-enabled, it may be undeclared (exception 1) or customized. If it is copy-assignable, both move operation may be undeclared (exception 2) or the the move constructor must be customized along with the copy constructor Ergo, all move constructible classes must fulfill either exception 1 or exception 2 or have a customized move constructor. Inversely, the unmovable category places no requirements on the move constructor.
if its move assignable, it may be move-only. Move only and assignable classes must have a customized move assignment operator. If it is copy-enabled, the move assignment operator may be undeclared if the move constructor is (exception 2). Otherwise, it has to be customized. Ergo, all move assignable classes must fulfill exception 2 or have a customized move assignment operator. Inversely, a class may be:
brief pause ...To me, this is just much simpler overall. Not only is easier to remember that "x-operatable requires x operation is customized." But it also makes pragmatic sense. A customized destructor indicates that you're managing a resource. Copying and moving a class requires care, during both construction and during assignment. In the above code, the default copy operations will copy the handle, and the resource will be freed twice (once by the original, once by the copy). The default move operations will move the handle, which will typically have copy semantics. It isn't enough to customize just the move constructor, or just the move assignment operator. Every available operation must be either customized or deleted....with one exception! If you leave the move operations undeclared, then moving will call your copy operations, so its ok to leave move operations undeclared and instead customize your copy operations.
Collaborator
Author
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. I think you're proposing something closer to this, right? This is more verbose, but its absolutely possible to make the case that it has better error messages. It is also a pretty clear 1:1 mapping to the misra spec. The above version passes our current tests (just with different alert messages). We shouldn't keep the old version unless we can clarify it. I don't know if its enough, but we could add a comment like this: Hopefully this comment:
Collaborator
Author
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. Oh, one last thought, and I don't really know which method this supports. The original/current version will report errors for cases that are not one of the valid categories (not unmovable, or move-only, or copy-enabled), while the rewrite I posted above will not. Usually this is a bad thing. We don't want to give two alerts when we could give one. In this case, you could argue the reverse, that a class with a customized destructor and an otherwise defaulted operation should always get a warning. If a user intentionally writes a class that doesn't fit one of the categories (unmovable, move-only, copy-enabled), they might ignore that alert, and then they'll never get a warning that they forgot to customize their move assignment constructor. Interesting to consider. Maybe not an issue in practice. On qlx, these two versions give the exact same results. |
||
|
|
||
| predicate isPublicBase(AnalyzableClass c) { | ||
| exists(ClassDerivation d | | ||
| d.getBaseClass() = c and | ||
| d.hasSpecifier("public") | ||
| ) | ||
| } | ||
|
|
||
| predicate satisfiesInheritanceRequirements(AnalyzableClass c) { | ||
| not isPublicBase(c) | ||
| or | ||
| isUnmovable(c) and | ||
| c.getDestructor().isPublic() and | ||
| c.getDestructor().isVirtual() | ||
| or | ||
| c.getDestructor().isProtected() and | ||
| not c.getDestructor().isVirtual() | ||
| } | ||
|
|
||
| from AnalyzableClass c, string message | ||
| where | ||
| not isExcluded(c, Classes3Package::improperlyProvidedSpecialMemberFunctionsQuery()) and | ||
| ( | ||
| not isValidCategory(c) and | ||
| message = "does not fall into a valid category (isUnmovable, move-only, or copy-enabled)." | ||
| or | ||
| violatesCustomizedMoveOrCopyRequirements(c, message) | ||
| or | ||
| violatesCustomizedDestructorRequirements(c, message) | ||
| or | ||
| not satisfiesInheritanceRequirements(c) and | ||
| message = "violates inheritance requirements for special member functions." | ||
| ) | ||
| select c, "Class '" + c.getName() + "' " + message | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /** | ||
| * @id cpp/misra/improperly-provided-special-member-functions-audit | ||
| * @name RULE-15-0-1: Special member functions shall be provided appropriately, Audit | ||
| * @description Audit: incorrect provision of special member functions can lead to unexpected or | ||
| * undefined behavior when objects of the class are copied, moved, or destroyed. | ||
| * @kind problem | ||
| * @precision low | ||
| * @problem.severity warning | ||
| * @tags external/misra/id/rule-15-0-1 | ||
| * scope/single-translation-unit | ||
| * correctness | ||
| * external/misra/audit | ||
| * maintainability | ||
| * external/misra/enforcement/decidable | ||
| * external/misra/obligation/required | ||
| */ | ||
|
|
||
| import cpp | ||
| import codingstandards.cpp.misra | ||
| import AnalyzableClass | ||
|
|
||
| string missingKind(Class c) { | ||
| not c.getAConstructor() instanceof MoveConstructor and | ||
| result = "move constructor" | ||
| or | ||
| not c.getAMemberFunction() instanceof MoveAssignmentOperator and | ||
| result = "move assignment operator" | ||
| or | ||
| not c.getAConstructor() instanceof CopyConstructor and | ||
| result = "copy constructor" | ||
| or | ||
| not c.getAMemberFunction() instanceof CopyAssignmentOperator and | ||
| result = "copy assignment operator" | ||
| or | ||
| not c.getAMemberFunction() instanceof Destructor and | ||
| result = "destructor" | ||
| } | ||
|
|
||
| string missingKinds(Class c) { result = concat(missingKind(c), " and ") } | ||
|
MichaelRFairhurst marked this conversation as resolved.
|
||
|
|
||
| from Class c, string kinds | ||
| where | ||
| not isExcluded(c, Classes3Package::improperlyProvidedSpecialMemberFunctionsAuditQuery()) and | ||
| not c instanceof AnalyzableClass and | ||
| not c.isPod() and | ||
| kinds = missingKinds(c) | ||
| select c, | ||
| "Class '" + c.getName() + "' is not analyzable because the " + kinds + | ||
| " are not present in the CodeQL database." | ||
|
MichaelRFairhurst marked this conversation as resolved.
Outdated
|
||
Uh oh!
There was an error while loading. Please reload this page.