Skip to content

Commit 9c9df75

Browse files
knewbury01knewbury01
authored
Merge pull request #549 from rvermeulen/rvermeulen/fix-392
Fix 392
2 parents 92e31d5 + b214af4 commit 9c9df75

File tree

7 files changed

+153
-39
lines changed

7 files changed

+153
-39
lines changed

‎change_notes/2024-03-01-fix-fp-a12-4-1-and-a12-8-6.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
- `A12-4-1` - `DestructorOfABaseClassNotPublicVirtual.ql`:
2+
- Fix FP reported in #392. Improve base class detection for template classes.
3+
- Update the alert message to prevent duplicate alerts for base classes that are both derived and abstract.
4+
- `A12-8-6` - `CopyAndMoveNotDeclaredProtected.ql`:
5+
- Fix FP reported in #392. Improve base class detection for template classes.
6+
- Update the alert message to prevent duplicate alerts for base classes that are both derived and abstract.

‎cpp/autosar/src/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.ql

+3-3
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ predicate isProtectedNonVirtual(Destructor d) { d.isProtected() and not d.isVirt
2929
from Destructor d
3030
where
3131
not isExcluded(d, VirtualFunctionsPackage::destructorOfABaseClassNotPublicVirtualQuery()) and
32-
isPossibleBaseClass(d.getDeclaringType(), _) and
32+
d.getDeclaringType() instanceof BaseClass and
3333
(not isPublicOverride(d) and not isProtectedNonVirtual(d) and not isPublicVirtual(d))
3434
// Report the declaration entry in the class body, as that is where the access specifier should be set
3535
select getDeclarationEntryInClassDeclaration(d),
36-
"Destructor of base class " + d.getDeclaringType() +
37-
" is not declared as public virtual, public override, or protected non-virtual."
36+
"Destructor of base class '" + d.getDeclaringType().getQualifiedName() +
37+
"' is not declared as public virtual, public override, or protected non-virtual."

‎cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql

+34-12
Original file line numberDiff line numberDiff line change
@@ -16,35 +16,57 @@ import cpp
1616
import codingstandards.cpp.autosar
1717
import codingstandards.cpp.Class
1818

19-
predicate isInvalidConstructor(Constructor f, string constructorType) {
19+
predicate isInvalidConstructor(Constructor f, string constructorType, string missingAction) {
2020
not f.isDeleted() and
2121
not f.isProtected() and
2222
(
23-
f instanceof MoveConstructor and constructorType = "Move constructor"
23+
f instanceof MoveConstructor and
24+
if f.isCompilerGenerated()
25+
then constructorType = "Implicit move constructor" and missingAction = "deleted"
26+
else (
27+
constructorType = "Move constructor" and missingAction = "protected"
28+
)
2429
or
25-
f instanceof CopyConstructor and constructorType = "Copy constructor"
30+
f instanceof CopyConstructor and
31+
if f.isCompilerGenerated()
32+
then constructorType = "Implicit copy constructor" and missingAction = "deleted"
33+
else (
34+
constructorType = "Copy constructor" and missingAction = "protected"
35+
)
2636
)
2737
}
2838

29-
predicate isInvalidAssignment(Operator f, string operatorType) {
39+
predicate isInvalidAssignment(Operator f, string operatorType, string missingAction) {
3040
not f.isDeleted() and
3141
(
32-
f instanceof CopyAssignmentOperator and operatorType = "Copy assignment operator"
42+
f instanceof MoveAssignmentOperator and
43+
if f.isCompilerGenerated()
44+
then operatorType = "Implicit move assignment operator" and missingAction = "deleted"
45+
else (
46+
operatorType = "Move assignment operator" and missingAction = "protected"
47+
)
3348
or
34-
f instanceof MoveAssignmentOperator and operatorType = "Move assignment operator"
49+
f instanceof CopyAssignmentOperator and
50+
if f.isCompilerGenerated()
51+
then operatorType = "Implicit copy assignment operator" and missingAction = "deleted"
52+
else (
53+
operatorType = "Copy assignment operator" and missingAction = "protected"
54+
)
3555
) and
3656
not f.hasSpecifier("protected")
3757
}
3858

39-
from MemberFunction mf, string type, string baseReason
59+
from BaseClass baseClass, MemberFunction mf, string type, string missingAction
4060
where
4161
not isExcluded(mf, OperatorsPackage::copyAndMoveNotDeclaredProtectedQuery()) and
4262
(
43-
isInvalidConstructor(mf, type)
63+
isInvalidConstructor(mf, type, missingAction)
4464
or
45-
isInvalidAssignment(mf, type)
65+
isInvalidAssignment(mf, type, missingAction)
4666
) and
47-
isPossibleBaseClass(mf.getDeclaringType(), baseReason)
67+
baseClass = mf.getDeclaringType()
68+
// To avoid duplicate alerts due to inaccurate location information in the database we don't use the location of the base class.
69+
// This for example happens if multiple copies of the same header file are present in the database.
4870
select getDeclarationEntryInClassDeclaration(mf),
49-
type + " for base class " + mf.getDeclaringType().getQualifiedName() + " (" + baseReason +
50-
") is not declared protected or deleted."
71+
type + " for base class '" + baseClass.getQualifiedName() + "' is not declared " + missingAction +
72+
"."

‎cpp/autosar/test/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.expected

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| test.cpp:4:3:4:4 | definition of ~A | Destructor of base class A is not declared as public virtual, public override, or protected non-virtual. |
2-
| test.cpp:30:3:30:4 | definition of ~E | Destructor of base class E is not declared as public virtual, public override, or protected non-virtual. |
1+
| test.cpp:4:3:4:4 | definition of ~A | Destructor of base class 'A' is not declared as public virtual, public override, or protected non-virtual. |
2+
| test.cpp:30:3:30:4 | definition of ~E | Destructor of base class 'E' is not declared as public virtual, public override, or protected non-virtual. |

‎cpp/autosar/test/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.expected

+26-14
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,26 @@
1-
| test.cpp:4:3:4:12 | declaration of BaseClass1 | Copy constructor for base class BaseClass1 (a derived class exists) is not declared protected or deleted. |
2-
| test.cpp:5:3:5:12 | declaration of BaseClass1 | Move constructor for base class BaseClass1 (a derived class exists) is not declared protected or deleted. |
3-
| test.cpp:6:15:6:23 | declaration of operator= | Copy assignment operator for base class BaseClass1 (a derived class exists) is not declared protected or deleted. |
4-
| test.cpp:7:15:7:23 | declaration of operator= | Move assignment operator for base class BaseClass1 (a derived class exists) is not declared protected or deleted. |
5-
| test.cpp:15:7:15:7 | declaration of operator= | Copy assignment operator for base class BaseClass2 (a derived class exists) is not declared protected or deleted. |
6-
| test.cpp:15:7:15:7 | declaration of operator= | Move assignment operator for base class BaseClass2 (a derived class exists) is not declared protected or deleted. |
7-
| test.cpp:55:3:55:12 | declaration of BaseClass5 | Copy constructor for base class BaseClass5 (a derived class exists) is not declared protected or deleted. |
8-
| test.cpp:56:3:56:12 | declaration of BaseClass5 | Move constructor for base class BaseClass5 (a derived class exists) is not declared protected or deleted. |
9-
| test.cpp:57:15:57:23 | declaration of operator= | Copy assignment operator for base class BaseClass5 (a derived class exists) is not declared protected or deleted. |
10-
| test.cpp:58:15:58:23 | declaration of operator= | Move assignment operator for base class BaseClass5 (a derived class exists) is not declared protected or deleted. |
11-
| test.cpp:75:3:75:12 | declaration of BaseClass6 | Copy constructor for base class BaseClass6 (the class is abstract) is not declared protected or deleted. |
12-
| test.cpp:76:3:76:12 | declaration of BaseClass6 | Move constructor for base class BaseClass6 (the class is abstract) is not declared protected or deleted. |
13-
| test.cpp:77:15:77:23 | declaration of operator= | Copy assignment operator for base class BaseClass6 (the class is abstract) is not declared protected or deleted. |
14-
| test.cpp:78:15:78:23 | declaration of operator= | Move assignment operator for base class BaseClass6 (the class is abstract) is not declared protected or deleted. |
1+
| test.cpp:4:3:4:12 | declaration of BaseClass1 | Copy constructor for base class 'BaseClass1' is not declared protected. |
2+
| test.cpp:5:3:5:12 | declaration of BaseClass1 | Move constructor for base class 'BaseClass1' is not declared protected. |
3+
| test.cpp:6:15:6:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass1' is not declared protected. |
4+
| test.cpp:7:15:7:23 | declaration of operator= | Move assignment operator for base class 'BaseClass1' is not declared protected. |
5+
| test.cpp:15:7:15:7 | declaration of operator= | Implicit copy assignment operator for base class 'BaseClass2' is not declared deleted. |
6+
| test.cpp:15:7:15:7 | declaration of operator= | Implicit move assignment operator for base class 'BaseClass2' is not declared deleted. |
7+
| test.cpp:55:3:55:12 | declaration of BaseClass5 | Copy constructor for base class 'BaseClass5' is not declared protected. |
8+
| test.cpp:56:3:56:12 | declaration of BaseClass5 | Move constructor for base class 'BaseClass5' is not declared protected. |
9+
| test.cpp:57:15:57:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass5' is not declared protected. |
10+
| test.cpp:58:15:58:23 | declaration of operator= | Move assignment operator for base class 'BaseClass5' is not declared protected. |
11+
| test.cpp:75:3:75:12 | declaration of BaseClass6 | Copy constructor for base class 'BaseClass6' is not declared protected. |
12+
| test.cpp:76:3:76:12 | declaration of BaseClass6 | Move constructor for base class 'BaseClass6' is not declared protected. |
13+
| test.cpp:77:15:77:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass6' is not declared protected. |
14+
| test.cpp:78:15:78:23 | declaration of operator= | Move assignment operator for base class 'BaseClass6' is not declared protected. |
15+
| test.cpp:85:3:85:12 | declaration of BaseClass7 | Copy constructor for base class 'BaseClass7<T1>' is not declared protected. |
16+
| test.cpp:86:3:86:12 | declaration of BaseClass7 | Move constructor for base class 'BaseClass7<T1>' is not declared protected. |
17+
| test.cpp:87:15:87:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass7<T1>' is not declared protected. |
18+
| test.cpp:88:15:88:23 | declaration of operator= | Move assignment operator for base class 'BaseClass7<T1>' is not declared protected. |
19+
| test.cpp:108:3:108:12 | declaration of BaseClass8 | Copy constructor for base class 'BaseClass8' is not declared protected. |
20+
| test.cpp:109:3:109:12 | declaration of BaseClass8 | Move constructor for base class 'BaseClass8' is not declared protected. |
21+
| test.cpp:110:15:110:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass8' is not declared protected. |
22+
| test.cpp:111:15:111:23 | declaration of operator= | Move assignment operator for base class 'BaseClass8' is not declared protected. |
23+
| test.cpp:124:26:124:26 | declaration of BaseClass9 | Implicit copy constructor for base class 'BaseClass9<int>' is not declared deleted. |
24+
| test.cpp:124:26:124:26 | declaration of BaseClass9 | Implicit move constructor for base class 'BaseClass9<int>' is not declared deleted. |
25+
| test.cpp:124:26:124:26 | declaration of operator= | Implicit copy assignment operator for base class 'BaseClass9<int>' is not declared deleted. |
26+
| test.cpp:124:26:124:26 | declaration of operator= | Implicit move assignment operator for base class 'BaseClass9<int>' is not declared deleted. |

‎cpp/autosar/test/rules/A12-8-6/test.cpp

+64-1
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,67 @@ class BaseClass6 {
7777
BaseClass6 &operator=(BaseClass6 const &) = default; // NON_COMPLIANT
7878
BaseClass6 &operator=(BaseClass6 &&) = default; // NON_COMPLIANT
7979
virtual void test() = 0; // pure virtual function, making this abstract
80-
};
80+
};
81+
82+
template <class T1> class BaseClass7 {
83+
public:
84+
BaseClass7() {}
85+
BaseClass7(BaseClass7 const &) = default; // NON_COMPLIANT
86+
BaseClass7(BaseClass7 &&) = default; // NON_COMPLIANT
87+
BaseClass7 &operator=(BaseClass7 const &) = default; // NON_COMPLIANT
88+
BaseClass7 &operator=(BaseClass7 &&) = default; // NON_COMPLIANT
89+
int operator=(int i); // COMPLIANT - not an assignment operator
90+
}; // COMPLIANT
91+
92+
template <class T>
93+
class DerivedClass7 // COMPLIANT - not a base class itself
94+
: public BaseClass7<T> {
95+
public:
96+
DerivedClass7() {}
97+
};
98+
99+
class DerivedClass8 // COMPLIANT - not a base class itself
100+
: public BaseClass7<int> {
101+
public:
102+
DerivedClass8() {}
103+
};
104+
105+
class BaseClass8 {
106+
public:
107+
BaseClass8() {}
108+
BaseClass8(BaseClass8 const &) = default; // NON_COMPLIANT
109+
BaseClass8(BaseClass8 &&) = default; // NON_COMPLIANT
110+
BaseClass8 &operator=(BaseClass8 const &) = default; // NON_COMPLIANT
111+
BaseClass8 &operator=(BaseClass8 &&) = default; // NON_COMPLIANT
112+
};
113+
114+
template <class T>
115+
class DerivedClass9 // COMPLIANT - not a base class itself
116+
: public BaseClass8 {
117+
public:
118+
DerivedClass9() {}
119+
120+
private:
121+
T t;
122+
};
123+
124+
template <class T> class BaseClass9 { // NON_COMPLIANT
125+
126+
public:
127+
BaseClass9() {}
128+
};
129+
130+
template <class T>
131+
class DerivedClass10 // COMPLIANT - not a base class itself
132+
: public BaseClass9<T> {
133+
public:
134+
DerivedClass10() {}
135+
};
136+
137+
void test() {
138+
BaseClass7<int> b;
139+
DerivedClass7<int> d;
140+
DerivedClass9<int> e;
141+
BaseClass9<int> f;
142+
DerivedClass10<int> g;
143+
}

‎cpp/common/src/codingstandards/cpp/Class.qll

+18-7
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,26 @@
55
import cpp
66
import codingstandards.cpp.Expr
77

8+
private Class getADerivedClass(Class c) {
9+
result = c.getADerivedClass()
10+
or
11+
exists(ClassTemplateInstantiation instantiation |
12+
instantiation.getADerivedClass() = result and c = instantiation.getTemplate()
13+
)
14+
}
15+
816
/**
9-
* Holds if we believe that `c` is used or intended to be used as a base class.
17+
* A class that is used or intended to be used as a base class.
1018
*/
11-
predicate isPossibleBaseClass(Class c, string reason) {
12-
// There exists a derivation in this database
13-
exists(c.getADerivedClass()) and reason = "a derived class exists"
14-
or
15-
// The class must be extended at some point
16-
c.isAbstract() and reason = "the class is abstract"
19+
class BaseClass extends Class {
20+
BaseClass() {
21+
exists(getADerivedClass(this))
22+
or
23+
this.isAbstract()
24+
}
25+
26+
// We don't override `getADerivedClass` because that introduces a non-monotonic recursion.
27+
Class getASubClass() { result = getADerivedClass(this) }
1728
}
1829

1930
/**

0 commit comments

Comments
 (0)