Skip to content

Commit 2813136

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
Address feedback
1 parent a1c1a9e commit 2813136

5 files changed

Lines changed: 161 additions & 54 deletions

File tree

‎cpp/misra/src/rules/RULE-15-0-1/AnalyzableClass.qll‎

Lines changed: 132 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,50 @@
1+
/**
2+
* Provides predicates and classes to perform a precursor analysis of which classes the rule can
3+
* potentially analyze, or should be excluded and instead selected by the audit query.
4+
*
5+
* For example, the class `AnalyzableClass` resolves all of the special member functions that we
6+
* must have in order to determine rule compliance.
7+
*
8+
* Part of what this module does is perform a very approximate analysis of which classes will
9+
* produce a value of true in `std::is_(copy|move)_(constructible|assignable)_v`.
10+
*
11+
* Fully predicting these standard type traits requires performing a very thorough overload
12+
* resolution analysis, including value category propagation and reference binding and user defined
13+
* conversion operators and standard conversions and promotions and ranking viable candidates and
14+
* properly handling ambiguous overloads.
15+
*/
16+
117
import cpp
218

19+
/**
20+
* For `std::is_(copy|move)_(constructible|assignable)_v` to return true for a given class, the
21+
* member must exist, it must not be deleted, and it must be publicly accessible.
22+
*
23+
* This is a very coarse approximation of true behavior of the standard type traits.
24+
*/
325
private predicate isUsable(MemberFunction f) {
426
not f.isDeleted() and
527
f.isPublic()
628
}
729

30+
/**
31+
* Holds if the member function `f` has been "customized" by the user, e.g., they explicitly wrote
32+
* the implementation of the function.
33+
*/
834
private predicate isMemberCustomized(MemberFunction f) {
935
exists(f.getDefinition()) and
1036
not f.isDefaulted() and
1137
not f.isDeleted() and
1238
not f.isCompilerGenerated()
1339
}
1440

41+
/**
42+
* Holds if the user declared the member function `f`, as opposed to it being implicitly declared
43+
* by the compiler.
44+
*
45+
* Note that `T(T&&) = default` and `T(T&&) = delete` are both user declared. This is not to be
46+
* confused with "user defined."
47+
*/
1548
private predicate isUserDeclared(MemberFunction f) { not f.isCompilerGenerated() }
1649

1750
/**
@@ -23,34 +56,78 @@ private predicate isUserDeclared(MemberFunction f) { not f.isCompilerGenerated()
2356
private predicate implicitMoveIsSuppressed(Class c) {
2457
isUserDeclared(c.getAConstructor().(CopyConstructor))
2558
or
26-
isUserDeclared(c.getAConstructor().(CopyAssignmentOperator))
59+
isUserDeclared(c.getAMemberFunction().(CopyAssignmentOperator))
2760
or
2861
isUserDeclared(c.getDestructor())
2962
}
3063

31-
Constructor getMoveConstructor(Class c) {
64+
/**
65+
* Returns the move constructor of the class `c` if it exists, or the copy constructor if it does
66+
* not exist and the implicit definition was suppressed by the compiler.
67+
*/
68+
private Constructor getMoveConstructor(Class c) {
3269
if
3370
not exists(MoveConstructor mc | mc = c.getAConstructor() and isUserDeclared(mc)) and
3471
implicitMoveIsSuppressed(c)
3572
then result = c.getAConstructor().(CopyConstructor)
3673
else result = c.getAConstructor().(MoveConstructor)
3774
}
3875

39-
Operator getMoveAssign(Class c) {
76+
/**
77+
* Returns the move assignment operator of the class `c` if it exists, or the copy assignment
78+
* operator if it does not exist and the implicit definition was suppressed by the compiler.
79+
*/
80+
private Operator getMoveAssign(Class c) {
4081
if
4182
not exists(MoveAssignmentOperator mc | mc = c.getAMemberFunction() and isUserDeclared(mc)) and
4283
implicitMoveIsSuppressed(c)
4384
then result = c.getAMemberFunction().(CopyAssignmentOperator)
4485
else result = c.getAMemberFunction().(MoveAssignmentOperator)
4586
}
4687

88+
/**
89+
* The types of special member functions that the `AnalyzableClass` tracks and analyzes.
90+
*/
4791
newtype TSpecialMember =
4892
TMoveConstructor() or
4993
TMoveAssignmentOperator() or
5094
TCopyConstructor() or
5195
TCopyAssignmentOperator() or
5296
TDestructor()
5397

98+
/**
99+
* A class for which we can see all special member functions, including implicitly declared ones,
100+
* and therefore we can attempt to analyze it in the current rule.
101+
*
102+
* If one of the special member functions cannot be found, we cannot know if it is missing because
103+
* it should not have been generated, or if EDG did not emit a definition for it. For instance, EDG
104+
* may not generate these functions if they are trivial, or if they are delete, or not ODR used. We,
105+
* the authors of this project, do not know the exact conditions we have to consider in this case.
106+
*
107+
* Determining for ourselves whether a certain constructor would be implicitly declared, and with
108+
* what signature, and whether it is deleted, requires implementing a significant portion of the C++
109+
* language rules regarding special member function generation, including a significant portion of
110+
* C++ overload resolution rules which are non-trivial.
111+
*
112+
* Therefore we must find a definition for each special member in the database to proceed. The only
113+
* exception we allow is certain missing `MoveConstructor` or `MoveAssignmentOperator` members; if
114+
* the class defines copy operations or the destructor, we expect these to be missing, and typically
115+
* this means the corresponding copy operation acts in place of the equivalent move.
116+
*
117+
* The last difficulty in analysis that this class attempts to handle is the values of the type
118+
* traits `std::is_(copy|move)_(constructible|assignable)`. These type traits are defined as true if
119+
* certain C++ expressions, such as `T(declval<T>())` or `declval<T>() = declval<T>()`, are
120+
* well-formed. We cannot correctly determine this in all cases without implementing a significant
121+
* portion of the C++ language rules for reference binding and overload resolution.
122+
*
123+
* To handle these type traits, we take a very rough approximation. If the corresponding special
124+
* member function is public and not deleted, then we assume the type trait will evaluate to true.
125+
* We also handle the case where a user declared copy operation suppresses the implicit move
126+
* operations, which typically means overload resolution selects the copy operation. (This is not
127+
* the case when the move operations are declared as deleted). We handle this by treating the copy
128+
* operation as effectively acting in place of the move operation for the purposes of evaluating
129+
* the type traits.
130+
*/
54131
class AnalyzableClass extends Class {
55132
CopyConstructor copyCtor;
56133
// The move constructor may be suppressed, and the copy constructor may be used during moves.
@@ -68,28 +145,49 @@ class AnalyzableClass extends Class {
68145
dtor = this.getDestructor()
69146
}
70147

71-
predicate exposes(TSpecialMember m) {
72-
m instanceof TMoveConstructor and moveConstructible()
73-
or
74-
m instanceof TMoveAssignmentOperator and moveAssignable()
75-
or
76-
m instanceof TCopyConstructor and copyConstructible()
77-
or
78-
m instanceof TCopyAssignmentOperator and copyAssignable()
79-
or
80-
m instanceof TDestructor and destructible()
81-
}
82-
148+
/**
149+
* Holds `std::is_move_constructible_v<T>` is likely true for this class.
150+
*
151+
* Specifically this holds if there's a non-deleted public move constructor available for this
152+
* class, or if there is a non-deleted public copy constructor that acts as the move constructor.
153+
*/
83154
predicate moveConstructible() { isUsable(moveCtor) }
84155

156+
/**
157+
* Holds `std::is_copy_constructible_v<T>` is likely true for this class.
158+
*
159+
* Specifically this holds if there's a non-deleted public copy constructor available for this
160+
* class.
161+
*/
85162
predicate copyConstructible() { isUsable(copyCtor) }
86163

164+
/**
165+
* Holds `std::is_move_assignable_v<T>` is likely true for this class.
166+
*
167+
* Specifically this holds if there's a non-deleted public move assignment operator available for
168+
* this class, or if there is a non-deleted public copy assignment operator that acts as the move
169+
* assignment operator.
170+
*/
87171
predicate moveAssignable() { isUsable(moveAssign) }
88172

173+
/**
174+
* Holds `std::is_copy_assignable_v<T>` is likely true for this class.
175+
*
176+
* Specifically this holds if there's a non-deleted public copy assignment operator available for
177+
* this class.
178+
*/
89179
predicate copyAssignable() { isUsable(copyAssign) }
90180

91-
predicate destructible() { isUsable(dtor) }
92-
181+
/**
182+
* Holds if the given special member function `s` is customized for this class.
183+
*
184+
* For most cases, this checks that the given special member function `s` has a user-provided
185+
* body (other than `= default;` or `= delete;`).
186+
*
187+
* If the class has copy operations that act in place of the move operations, that means the
188+
* corresponding move operation was not declared, so we say this predicate does not hold for the
189+
* given move operation `s`.
190+
*/
93191
predicate isCustomized(TSpecialMember s) {
94192
s instanceof TMoveConstructor and
95193
isMemberCustomized(moveCtor) and
@@ -106,7 +204,21 @@ class AnalyzableClass extends Class {
106204
s instanceof TDestructor and isMemberCustomized(dtor)
107205
}
108206

109-
predicate declaresMoveConstructor() { not moveCtor = copyCtor }
110-
111-
predicate declaresMoveAssignmentOperator() { not moveAssign = copyAssign }
207+
/**
208+
* Holds if this class declares a move constructor.
209+
*
210+
* This will be true if move constructor resolution found a non-implicit constructor that is not
211+
* the copy constructor masquerading as a move constructor.
212+
*/
213+
predicate declaresMoveConstructor() { not moveCtor = copyCtor and isUserDeclared(moveCtor) }
214+
215+
/**
216+
* Holds if this class declares a move assignment operator.
217+
*
218+
* This will be true if move assignment resolution found a non-implicit operator that is not
219+
* the copy assignment operator masquerading as a move assignment operator.
220+
*/
221+
predicate declaresMoveAssignmentOperator() {
222+
not moveAssign = copyAssign and isUserDeclared(moveAssign)
223+
}
112224
}

‎cpp/misra/src/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctionsAudit.ql‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import cpp
1919
import codingstandards.cpp.misra
2020
import AnalyzableClass
21+
import qtil.Qtil
2122

2223
string missingKind(Class c) {
2324
not c.getAConstructor() instanceof MoveConstructor and
@@ -45,5 +46,5 @@ where
4546
not c.isPod() and
4647
kinds = missingKinds(c)
4748
select c,
48-
"Class '" + c.getName() + "' is not analyzable because the " + kinds +
49-
" are not present in the CodeQL database."
49+
"Class '" + c.getName() + "' is not analyzable because the " + kinds + " " +
50+
Qtil::plural("is", "are", count(missingKind(c))) + " not present in the CodeQL database."

‎cpp/misra/test/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctions.expected‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@
2525
| test.cpp:258:7:258:43 | CopyEnabledCustomizedDtorNonCompliant | Class 'CopyEnabledCustomizedDtorNonCompliant' has customized the destructor, but does not customize the copy constructor. |
2626
| test.cpp:258:7:258:43 | CopyEnabledCustomizedDtorNonCompliant | Class 'CopyEnabledCustomizedDtorNonCompliant' has customized the destructor, but does not customize the move assignment operator. |
2727
| test.cpp:258:7:258:43 | CopyEnabledCustomizedDtorNonCompliant | Class 'CopyEnabledCustomizedDtorNonCompliant' has customized the destructor, but does not customize the move constructor. |
28-
| test.cpp:307:7:307:47 | CopyAssignableCustomizedDtorNonCompliant1 | Class 'CopyAssignableCustomizedDtorNonCompliant1' has customized the destructor, but does not customize the move assignment operator. |
29-
| test.cpp:317:7:317:47 | CopyAssignableCustomizedDtorNonCompliant2 | Class 'CopyAssignableCustomizedDtorNonCompliant2' has customized the destructor, but does not customize the move constructor. |
30-
| test.cpp:327:7:327:47 | CopyAssignableCustomizedDtorNonCompliant3 | Class 'CopyAssignableCustomizedDtorNonCompliant3' does not fall into a valid category (isUnmovable, move-only, or copy-enabled). |
31-
| test.cpp:337:7:337:47 | CopyAssignableCustomizedDtorNonCompliant4 | Class 'CopyAssignableCustomizedDtorNonCompliant4' has customized the destructor, but does not customize the move assignment operator. |
32-
| test.cpp:337:7:337:47 | CopyAssignableCustomizedDtorNonCompliant4 | Class 'CopyAssignableCustomizedDtorNonCompliant4' has customized the destructor, but does not customize the move constructor. |
33-
| test.cpp:347:7:347:47 | CopyAssignableCustomizedDtorNonCompliant5 | Class 'CopyAssignableCustomizedDtorNonCompliant5' has customized the destructor, but does not customize the move assignment operator. |
34-
| test.cpp:347:7:347:47 | CopyAssignableCustomizedDtorNonCompliant5 | Class 'CopyAssignableCustomizedDtorNonCompliant5' has customized the destructor, but does not customize the move constructor. |
35-
| test.cpp:357:7:357:33 | UnmovableBaseNonvirtualDtor | Class 'UnmovableBaseNonvirtualDtor' violates inheritance requirements for special member functions. |
36-
| test.cpp:378:7:378:33 | UnmovablePrivateVirtualDtor | Class 'UnmovablePrivateVirtualDtor' violates inheritance requirements for special member functions. |
37-
| test.cpp:405:7:405:30 | BaseVirtualProtectedDtor | Class 'BaseVirtualProtectedDtor' violates inheritance requirements for special member functions. |
28+
| test.cpp:296:7:296:47 | CopyAssignableCustomizedDtorNonCompliant1 | Class 'CopyAssignableCustomizedDtorNonCompliant1' has customized the destructor, but does not customize the move assignment operator. |
29+
| test.cpp:306:7:306:47 | CopyAssignableCustomizedDtorNonCompliant2 | Class 'CopyAssignableCustomizedDtorNonCompliant2' has customized the destructor, but does not customize the move constructor. |
30+
| test.cpp:316:7:316:47 | CopyAssignableCustomizedDtorNonCompliant3 | Class 'CopyAssignableCustomizedDtorNonCompliant3' does not fall into a valid category (isUnmovable, move-only, or copy-enabled). |
31+
| test.cpp:326:7:326:47 | CopyAssignableCustomizedDtorNonCompliant4 | Class 'CopyAssignableCustomizedDtorNonCompliant4' has customized the destructor, but does not customize the move assignment operator. |
32+
| test.cpp:326:7:326:47 | CopyAssignableCustomizedDtorNonCompliant4 | Class 'CopyAssignableCustomizedDtorNonCompliant4' has customized the destructor, but does not customize the move constructor. |
33+
| test.cpp:336:7:336:47 | CopyAssignableCustomizedDtorNonCompliant5 | Class 'CopyAssignableCustomizedDtorNonCompliant5' has customized the destructor, but does not customize the move assignment operator. |
34+
| test.cpp:336:7:336:47 | CopyAssignableCustomizedDtorNonCompliant5 | Class 'CopyAssignableCustomizedDtorNonCompliant5' has customized the destructor, but does not customize the move constructor. |
35+
| test.cpp:346:7:346:33 | UnmovableBaseNonvirtualDtor | Class 'UnmovableBaseNonvirtualDtor' violates inheritance requirements for special member functions. |
36+
| test.cpp:369:7:369:33 | UnmovablePrivateVirtualDtor | Class 'UnmovablePrivateVirtualDtor' violates inheritance requirements for special member functions. |
37+
| test.cpp:398:7:398:30 | BaseVirtualProtectedDtor | Class 'BaseVirtualProtectedDtor' violates inheritance requirements for special member functions. |

‎cpp/misra/test/rules/RULE-15-0-1/ImproperlyProvidedSpecialMemberFunctionsAudit.expected‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
| test.cpp:363:7:363:32 | UnmovableNonvirtualDerived | Class 'UnmovableNonvirtualDerived' is not analyzable because the destructor and move assignment operator and move constructor are not present in the CodeQL database. |
2-
| test.cpp:375:7:375:39 | UnmovableDerivedPublicVirtualDtor | Class 'UnmovableDerivedPublicVirtualDtor' is not analyzable because the move assignment operator and move constructor are not present in the CodeQL database. |
3-
| test.cpp:389:7:389:40 | UnmovablePrivateVirtualDtorDerived | Class 'UnmovablePrivateVirtualDtorDerived' is not analyzable because the move assignment operator and move constructor are not present in the CodeQL database. |
4-
| test.cpp:403:7:403:26 | ProtectedDtorDerived | Class 'ProtectedDtorDerived' is not analyzable because the destructor and move assignment operator are not present in the CodeQL database. |
5-
| test.cpp:416:7:416:33 | VirtualProtectedDtorDerived | Class 'VirtualProtectedDtorDerived' is not analyzable because the move assignment operator are not present in the CodeQL database. |
6-
| test.cpp:429:8:429:19 | TrivialClass | Class 'TrivialClass' is not analyzable because the destructor and move assignment operator and move constructor are not present in the CodeQL database. |
7-
| test.cpp:435:7:435:21 | NonTrivialClass | Class 'NonTrivialClass' is not analyzable because the copy constructor and move assignment operator and move constructor are not present in the CodeQL database. |
8-
| test.cpp:445:7:445:14 | CopyOnly | Class 'CopyOnly' is not analyzable because the destructor are not present in the CodeQL database. |
1+
| test.cpp:353:7:353:32 | UnmovableNonvirtualDerived | Class 'UnmovableNonvirtualDerived' is not analyzable because the destructor and move assignment operator and move constructor are not present in the CodeQL database. |
2+
| test.cpp:366:7:366:39 | UnmovableDerivedPublicVirtualDtor | Class 'UnmovableDerivedPublicVirtualDtor' is not analyzable because the move assignment operator and move constructor are not present in the CodeQL database. |
3+
| test.cpp:381:7:381:40 | UnmovablePrivateVirtualDtorDerived | Class 'UnmovablePrivateVirtualDtorDerived' is not analyzable because the move assignment operator and move constructor are not present in the CodeQL database. |
4+
| test.cpp:396:7:396:26 | ProtectedDtorDerived | Class 'ProtectedDtorDerived' is not analyzable because the destructor and move assignment operator are not present in the CodeQL database. |
5+
| test.cpp:410:7:410:33 | VirtualProtectedDtorDerived | Class 'VirtualProtectedDtorDerived' is not analyzable because the move assignment operator is not present in the CodeQL database. |
6+
| test.cpp:423:8:423:19 | TrivialClass | Class 'TrivialClass' is not analyzable because the destructor and move assignment operator and move constructor are not present in the CodeQL database. |
7+
| test.cpp:429:7:429:21 | NonTrivialClass | Class 'NonTrivialClass' is not analyzable because the copy constructor and move assignment operator and move constructor are not present in the CodeQL database. |
8+
| test.cpp:439:7:439:14 | CopyOnly | Class 'CopyOnly' is not analyzable because the destructor is not present in the CodeQL database. |

0 commit comments

Comments
 (0)