Skip to content

Commit

Permalink
Modify rule S1699: Improve rule description for CPP-5114 (#3949)
Browse files Browse the repository at this point in the history
  • Loading branch information
@marco-antognini-sonarsource
marco-antognini-sonarsource authored Jun 4, 2024
1 parent 32e5481 commit 8c2eb7d
Show file tree
Hide file tree
Showing 2 changed files with 245 additions and 86 deletions.
1 change: 0 additions & 1 deletion rules/S1699/cfamily/metadata.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
],
"securityStandards": {
"CERT": [
"MET05-J.",
"OOP50-CPP."
]
}
Expand Down
330 changes: 245 additions & 85 deletions rules/S1699/cfamily/rule.adoc
View file
Original file line number Diff line number Diff line change
@@ -1,127 +1,287 @@
== Why is this an issue?

When constructing an object of a derived class, the sub-object of the base class is constructed first, and only then the constructor of the derived class is called. When there are multiple levels of inheritance, the process is the same, from the most base class to the most derived class. Along this construction process, the dynamic type of the object evolves and is the type of the sub-object under construction.

When constructing an object of a derived class, the sub-object of the base class is constructed first, and only then is the derived class's constructor called.
This process remains the same when there are multiple levels of inheritance, from the most base class to the most derived class.
During construction, the object's dynamic type evolves to become the type of the sub-object under construction.
The destruction of the object follows the same process in reverse order.

As a consequence, when calling a virtual function from a constructor or a destructor, the actual function being called is not necessarily the version from the most-derived type, as some developers may believe, but the version that matches the level under construction.
These rules for {cpp} mean that invoking a virtual function from a constructor (or a destructor) selects the override that matches the level under construction (or destruction).
This is not necessarily the override from the most derived type, contrary to what developers familiar with other programming languages might expect.

Additionally, the behavior is undefined when the selected override is a pure virtual function.

We illustrate {cpp}'s behavior in the following example.
[source,cpp]
----
struct A {
virtual void f();
virtual void g();
virtual void h() = 0;
struct Base {
virtual std::string getPrefix() { return "Default"; }
virtual std::string getClassName() { return "Base"; }
};
struct B : public A {
B() {
f();
g();
struct Derived : Base {
std::string getClassName() override { return "Derived"; }
Derived() {
std::cout << getPrefix() << " - " << getClassName() << '\n';
}
void f() override;
};

struct C : public B {
void f() override;
void g() override;
void h() override;
struct Subderived : Derived {
std::string getPrefix() override { return "Custom"; }
std::string getClassName() final { return "Subderived"; }
};
----

When constructing an object of type C, the following occurs:
Constructing an object of type `Subderived` prints _Default - Derived_.
In detail, the following occurs:

* The sub-object of type `A` is constructed.
* The sub-object of type `B` is constructed.
** The constructor ``++B::B()++`` is called, during this call, ``++*this++`` is considered as being of type `B`.
** The function `B::f()` is called.
** The function `A::g()` is called.
* The object of type `C` is constructed.
. The sub-object of type `Base` is constructed using the compiler-generated constructor.
. The sub-object of type `Derived` is constructed using the user-provided constructor.
.. This constructor considers ``++*this++`` as being of type `Derived`.
.. The function `Base::getPrefix()` is called.
.. The function `Derived::getClassName()` is called.
. Finally, the object of type `Subderived` is constructed.

The fact that ``Subderived``'s methods are declared with the `override` and `final` keywords does not play any role.
This surprising behavior can be even worse: If there is no implementation for a virtual function (in the example, if the constructor attempted to call `h()` which is still a pure virtual function) the behavior is undefined.
This rule raises an issue when a non-final virtual function is called from a constructor or a destructor.
If you want to perform virtual calls during object construction that will consider the actual type of the object, the best way is probably to defer those calls right after the object is constructed, by using a factory function:
=== What is the potential impact?
[source,cpp]
In the best-case scenario, the selected override is the desired one.
However, this may not be obvious to everyone reading the code and can cause needless confusion.
Secondly, this reduces the software's adaptability: changing the class hierarchy may break the current assumption and lead to bugs if the wrong override is selected.
Another likely scenario is that the wrong overload is selected.
Since the difference between the program's expected and actual behavior can be very small, you may spend a significant amount of time identifying and fixing the problem.
The problem is even more challenging to identify when virtual functions are called indirectly through another function.
Finally, since the behavior is undefined if the chosen override is a pure virtual function, the program might crash or produce obvious incorrect results.
Or it might seem to work fine on the surface, yet lead to bigger problems down the line that are not identified by tests.
== How to fix it
There are essentially three scenarios:
1. If subclasses can exist and may customize the program's behavior, there is a bug. +
You should refactor the code to remove reliance on virtual function calls from constructors and destructors.
2. If subclasses don't exist or shouldn't customize the behavior, the intent is unclear. +
You should redesign the class hierarchy and let the compiler enforce these constraints.
3. If subclasses can exist but the current behavior is deemed correct, the intent remains unclear. +
You should explicitly reflect your intention in the code.
We will explore solutions for these cases below with a couple of examples.
Please keep in mind that these examples focus on the problem of virtual dispatch and nothing else.
For simplicity, they are not perfect: for example, they lack proper virtual destructors (S1235), they do not take into account encapsulation, etc...
=== Defer virtual function calls
If you want to perform virtual calls during the object construction process that consider the actual type of the object, you should defer those calls after the object constructor has finished with a factory function.
==== Noncompliant code example
Let's take a simple example that does not perform the expected action:
[source,cpp,diff-id=1,diff-type=noncompliant]
----
std::unique_ptr<Base> createObjectOfDerivedType(parameters) {
auto result = ...;
result->callVirtualFunction();
return result;
struct Base {
virtual std::string getClassName() { return "Base"; }
void printInfo() {
std::cout << getClassName() << "\n";
}
Base() {
printInfo(); // Noncompliant: Base::getClassName() is always selected.
}
};
struct Derived : Base {
virtual std::string getClassName() { return "Derived"; }
};
std::unique_ptr<Base> factory() {
auto ptr = std::make_unique<Derived>();
return ptr;
}
----
The `Derived` object created in `factory()` prints _Base_ instead of _Derived_ because `printInfo()` only considers ``Base``'s override of `getClassName`.
==== Compliant solution
This rule raises an issue when a non-final virtual function is called from a constructor or a destructor, therefore avoiding all surprising behavior.
The following solution prints _Derived_ by moving the virtual dispatch after the constructor of `Base` and `Derived` have finished.
[source,cpp,diff-id=1,diff-type=compliant]
----
struct Base {
virtual std::string getClassName() { return "Base"; }
=== Noncompliant code example
void printInfo() {
std::cout << getClassName() << "\n";
}
[source,cpp]
Base() {
// No direct & indirect calls to virtual functions.
}
};
struct Derived : Base {
virtual std::string getClassName() { return "Derived"; }
};
std::unique_ptr<Base> factory() {
auto ptr = std::make_unique<Derived>();
ptr->printInfo(); // Virtual function calls happen after the constructor.
return ptr;
}
----
==== Pitfalls
For this solution to properly work, you have to ensure every `Derived` object in your program is created via `factory()` to guarantee that `printInfo()` is systematically called when a new object is created.
There are multiple ways to enforce this.
For example, you can declare `factory()` as a static member function of `Derived` and mark the relevant constructors as `protected`.
However, you will have to deal with ``++std::make_unique++`` inability to access the protected constructor.
https://seanmiddleditch.github.io/enabling-make-unique-with-private-constructors/[The passkey idiom] is usually a good solution for this.
// https://abseil.io/tips/134 has some nice tips, but not focused enough.
When done rigorously, the compiler will emit a compilation error if you attempt to construct an object without relying on `factory()`.
==== Dealing with destructors
A similar solution can be applied to deal with this issue in destructors:
Instead of calling the virtual functions inside the destructors, you can call them before them.
This also requires to be careful and ensure each object is consistently destroyed.
For example, with the `factory()` function from above, you could use a custom deleter for ``++std::unique_ptr++``.
=== Mark functions or classes as final
Assuming you know that subclasses don't exist or shouldn't customize the behavior, you can use {cpp}11 `final` specifier to ask the compiler to enforce this design decision.
This solution is applicable for virtual calls from constructors and destructors.
==== Noncompliant code example
Consider this example:
[source,cpp,diff-id=2,diff-type=noncompliant]
----
class Parent {
public:
Parent() {
f1();
f2(); // Noncompliant; confusing because Parent::f2() will always be called even if it is overridden
}
virtual ~Parent() {
f3(); // Noncompliant; undefined behavior
}
private:
int f1();
virtual void f2();
virtual void f3() = 0; // pure virtual
};
class Child : public Parent {
public:
Child() { // leads to a call to Parent::f2(), not Child::f2()
f3(); // Noncompliant; Child::f3() might be further overridden
}
protected:
void f2() override;
void f3() override;
struct Widget {
virtual void addChild(Widget* child) = 0;
};
struct Text : Widget {
// ...
};
struct Button : Widget {
void addChild(Widget* child) override;
Button(std::string text) {
addChild(new Text(text)); // Noncompliant: Button::addChild is always selected.
}
};
----
=== Compliant solution
==== Compliant solution
[source,cpp]
In your design, if it does not make sense for `Button` to have subclasses, you can mark it as `final`.
Or you can allow subclasses as long as they do not override `addChild` by marking this function as `final`.
You can also combine both to be explicit about each individual design decision.
[source,cpp,diff-id=2,diff-type=compliant]
----
class Parent {
public:
Parent() {
f1();
Parent::f2(); // acceptable but poor design
}
virtual ~Parent() {
// call to pure virtual function removed
}
protected:
void f1();
virtual void f2();
virtual void f3() = 0;
};
class Child : public Parent {
public:
Child() {
}
virtual ~Child() {
f3(); // // Compliant - Well defined and predictable, a final function cannot be further overridden
}
protected:
void f2() override;
void f3() final;
struct Widget {
virtual void addChild(Widget* child) = 0;
};
struct Text : Widget {
// ...
};
struct Button final : Widget {
void addChild(Widget* child) final;
Button(std::string text) {
addChild(new Text(text)); // Compliant: the intent is clear.
}
};
----
=== Qualify function calls
On some occasions, you may decide to keep the current class hierarchy without prohibiting the existence of subclasses or further overrides.
Therefore, the `final` specifier is not appropriate.
However, it is still possible to make the intent clear.
==== Noncompliant code example
Consider this example:
[source,cpp,diff-id=3,diff-type=noncompliant]
----
struct FileStream {
virtual void sync();
};
struct BufferedFileStream : FileStream {
void sync() override;
~BufferedFileStream() {
sync(); // Noncompliant: BufferedFileStream::sync() is always selected.
}
};
----
==== Compliant solution
If you designed your classes to not rely on subclass override of `sync`, you can make the intention explicit by https://en.cppreference.com/w/cpp/language/qualified_lookup[qualifying the call].
[source,cpp,diff-id=3,diff-type=compliant]
----
struct FileStream {
virtual void sync();
};
struct BufferedFileStream : FileStream {
void sync() override;
~BufferedFileStream() {
BufferedFileStream::sync(); // Compliant: the intent is clear.
}
};
----
While this solution clarifies the intent, it is often brittle since introducing a new class in the hierarchy may silently break the assumption made in the implementation.
When creating a new class or modifying an existing one, you have to know the details of other classes to be sure not to break their behavior.
In other words, this solution tends to break the encapsulation of your classes.
=== Going the extra mile
When facing the problem covered by this rule, you may realize inheritance as used is not the right solution for your design.
Although they usually require larger changes, you can consider alternatives such as preferring composition over inheritance, using the decorator pattern, etc.
A good rule of thumb is to keep the "depth" of class hierarchies small and many design patterns allow you to do that.
Selecting the right one highly depends on your application.
== Resources
* https://wiki.sei.cmu.edu/confluence/x/6ns-BQ[CERT, OOP50-CPP.] - Do not invoke virtual functions from constructors or destructors
=== Documentation
* {cpp} reference - https://en.cppreference.com/w/cpp/language/qualified_lookup[Qualified name lookup]
* {cpp} reference - https://en.cppreference.com/w/cpp/language/final[`final` specifier]
=== Standards
* CERT - https://wiki.sei.cmu.edu/confluence/x/6ns-BQ[OOP50-CPP. Do not invoke virtual functions from constructors or destructors]
=== External coding guidelines
* {cpp} Core Guidelines - https://github.com/isocpp/CppCoreGuidelines/blob/e49158a/CppCoreGuidelines.md#c50-use-a-factory-function-if-you-need-virtual-behavior-during-initialization[C.50: Use a factory function if you need "virtual behavior" during initialization]
include::../rspecator.adoc[]

0 comments on commit 8c2eb7d

Please sign in to comment.