2

Suppose we have some virtual class Callable, (not abstract, because we cannot use abstract classes as type of std::optional<T>), and some function (call_option) that takes an std::optional of base class. Why in that case always called base class implementation Callable::call(), not overriden Square::call() - as in call_as_pointer() ?

This behavior was discovered during porting code from Rust into C++ :D I know, that it's better to use pointers here, instead of std::option, to make it work as expected. I fixed my code to avoid that unexpected behavior, but the question is open - why it behave so?

#include <optional>

#include <cstdio>

struct Callable
{
    virtual int call(int x) const
    {
        printf("default Callable::call()\n");
        return x;
    }
};

void call_option(const std::optional<Callable>& measurable)
{
    if(measurable.has_value()) {
        measurable.value().call(42);
    }
}

void call_as_pointer(const Callable* measurable)
{
    if(measurable != nullptr) {
        measurable->call(42);
    }
}

struct Square
    : Callable
{
    int call(int x) const override
    {
        printf("overriden Square::call()\n");
        return x * x;
    }
};

int main()
{
    const Square square;
    const std::optional<Square> option(square);

    call_option(option);      // Output: default Callable::call() <-- UNEXPECTED

    call_as_pointer(&square); // Output: overriden Square::call() <-- EXPECTED

    return 0;
}

--> Compiler Explorer <--

Inobelar
  • 59
  • 8
  • 17
    `std::optional` and `std::optional` are two different types. `call_option(option)` converts the `Square` into a `Callable`. You are in effect [slicing the object](https://stackoverflow.com/questions/274626/what-is-object-slicing) – NathanOliver May 15 '23 at 20:36
  • 2
    `optional` isn't an optional reference to the `Square` object, it holds a value, not a reference. In this case an incomplete copy of the `Square` that is only the `Callable` interface. This is why constructors of abstract base classes should be `protected`. That would have caught the issue by preventing the `optional` from existing – Homer512 May 15 '23 at 20:40
  • @NathanOliver thanks! Looks reasonable! What confuses me is that the compiler didn't issue any warnings in this case :D – Inobelar May 15 '23 at 20:47
  • 3
    Use a `std::unique_ptr` and you can keep the logic to check if it's populated or not. [example](https://godbolt.org/z/E3zEf48rz) – Ted Lyngmo May 15 '23 at 21:02
  • @Homer512 -- note, however, that `Callable` is not an abstract base class. – Pete Becker May 15 '23 at 21:53
  • @PeteBecker true, which is part of the problem. IMHO `Callable` should either declare its dtor as virtual abstract or make its ctors and dtor protected – Homer512 May 15 '23 at 22:02
  • 1
    I disagree with the use of `unique_ptr` for this since it has needless overhead through dynamic allocation and indirection (reference to `unique_ptr` is effectively a pointer to a pointer). A simple raw pointer argument that is understood to be `nullptr` if it's optional is perfectly fine here. Borrowing a nullable object is one of the few remaining uses for raw pointers – Homer512 May 15 '23 at 22:08
  • @Homer512 -- no, not being abstract has nothing to do with the problem. If `Callable::call` was pure virtual calling it would make the behavior of the program undefined. As written it's well defined and easily understood. – Pete Becker May 15 '23 at 22:11
  • @PeteBecker I agree that it is well defined and understood. However, in my opinion the design of the class structure is not great. `Callable` to me seems to be designed with the intent of being an interface-style class, not a class of objects that should stand on their own. Plus, slicing (by having public copy-ctors that accept sub-class objects) is a well-known risk and as seen here can lead to confusing bugs. Which is why style guides (can't remember whether GotW or Scott Meyer) recommend building classes so that base classes cannot be instantiated accidentally – Homer512 May 15 '23 at 22:20
  • @Homer512 — your latest comment has nothing to do with what I said. Don’t know why you addressed it to me. – Pete Becker May 15 '23 at 22:50
  • 2
    @Inobelar - The reason for no warning from the compiler - apart from the standard requiring no diagnostic - is that copying part of an object (aka object slicing) is a perfectly reasonable thing to do in some use cases (e.g. passing the "base part and its data" of an object by value) - in which case a warning is not desirable. You happen to have a (commonly occurring) use case [seeking virtual dispatch] where slicing is not so perfectly reasonable. To manage that, you need to make the act of slicing a diagnosable error (e.g. access control of constructors and destructors). – Peter May 16 '23 at 03:00

1 Answers1

3

std::optional<T> contains an object of type T by value. call_option(option) invokes a constructor intended for converting two optional types. This in turn calls a constructor Callable(const Square&) which, since Square inherits from Callable, becomes the implicitly defined copy constructor Callable(const Callable&).

As has been pointed out by others in comments, this is called object slicing. The new object is a pure Callable object, initialized from the Callable "part" of a Square object. In this case it is somewhat benign but it can have much worse effects because it copies (or worse, moves) part of an object while ignoring others.

Preventing object slicing

To prevent this from accidentally happening in the first place, there are two rather simple solutions:

  1. Make base classes abstract
struct Callable
{
    virtual int call(int x) const
    {
        printf("default Callable::call()\n");
        return x;
    }
    virtual ~Callable() = 0;
};
Callable::~Callable() = default;

This prevents compilation with an error like "cannot declare field std::_Optional_payload_base<[…]> to be of abstract type Callable". To be clear, any virtual abstract method would work. Adding a virtual destructor has the added bonus that now it is safe to delete Square objects through pointers to Callable. In your code something like std::unique_ptr<Callable> c = std::make_unique<Square>() would have called the wrong destructor, now it works.

  1. Make base class constructors, assignment and destructors protected
struct Callable
{
    virtual int call(int x) const
    {
        printf("default Callable::call()\n");
        return x;
    }
protected:
    Callable() = default;
    Callable(const Callable&) = default;
    ~Callable() = default;
    Callable& operator=(const Callable&) = default;
    // add move constructors and assignment as needed
};

This likewise prevents the creation of objects of this type and it prevents destroying Squares through the wrong destructor (unique_ptr<Callable> doesn`t compile). This pattern is mostly useful if the base class doesn't have a virtual method and you want to avoid the cost of adding one.

Using either of these patterns is a bit of a coding style question. As such it is opinion-based. However, in my opinion all classes that are designed to be inherited from should be made functionally abstract in this pattern to prevent exactly this issue of happening.

Declaring optional function arguments

Back to the point of how void call_option(const std::optional<Callable>& measurable) should be declared instead.

In my opinion your call_as_pointer is exactly right. Any form of smart pointer such as unique_ptr is pointless, slow and restrictive if all you do is "optionally borrow" a reference. Borrowing objects is one of the last remaining uses of raw pointers.

Even if you hold your Square objects in unique_ptrs, you should probably pass them as raw pointers in instances like this here to 1) avoid the extra level of indirection, 2) make it clear that the interest is the object itself, not the pointer to it 3) avoid restricting yourself to only unique_ptrs in case you want stack-allocated or shared_ptr objects on some call sites.

One might think that this is a good alternative: call_option(std::optional<const Callable*> measurable). However, there is nothing that prevents the measurable from "having a value" and that value being nullptr. In fact, call_option(nullptr) would probably not do what you want.

Another school of thought is that nullptr values and/or explicit checks for object existence should be avoided. Instead, a do-nothing Callable may be defined.

Homer512
  • 9,144
  • 2
  • 8
  • 25