Is C++'s default copy-constructor inherently unsafe? Are iterators fundamentally unsafe too?

Question

I used to think C++'s object model is very robust when best practices are followed.
Just a few minutes ago, though, I had a realization that I hadn't had before.

Consider this code:

class Foo
{
    std::set<size_t> set;
    std::vector<std::set<size_t>::iterator> vector;
    // ...
    // (assume every method ensures p always points to a valid element of s)
};

I have written code like this. And until today, I hadn't seen a problem with it.

But, thinking about it a more, I realized that this class is very broken:
Its copy-constructor and copy-assignment copy the iterators inside the vector, which implies that they will still point to the old set! The new one isn't a true copy after all!

In other words, I must manually implement the copy-constructor even though this class isn't managing any resources (no RAII)!

This strikes me as astonishing. I've never come across this issue before, and I don't know of any elegant way to solve it. Thinking about it a bit more, it seems to me that copy construction is unsafe by default -- in fact, it seems to me that classes should not be copyable by default, because any kind of coupling between their instance variables risks rendering the default copy-constructor invalid.

Are iterators fundamentally unsafe to store? Or, should classes really be non-copyable by default?

The solutions I can think of, below, are all undesirable, as they don't let me take advantage of the automatically-generated copy constructor:

1. Manually implement a copy constructor for every nontrivial class I write. This is not only error-prone, but also painful to write for a complicated class.
2. Never store iterators as member variables. This seems severely limiting.
3. Disable copying by default on all classes I write, unless I can explicitly prove they are correct. This seems to run entirely against C++'s design, which is for most types to have value semantics, and thus be copyable.

Is this a well-known problem, and if so, does it have an elegant/idiomatic solution?

Answer 1

"Inherently unsafe"

No, the features you mention are not inherently unsafe; the fact that you thought of three possible safe solutions to the problem is evidence that there is no "inherent" lack of safety here, even though you think the solutions are undesirable.

And yes, there is RAII here: the containers (set and vector) are managing resources. I think your point is that the RAII is "already taken care of" by the std containers. But you need to then consider the container instances themselves to be "resources", and in fact your class is managing them. You're correct that you're not directly managing heap memory, because this aspect of the management problem is taken care of for you by the standard library. But there's more to the management problem, which I'll talk a bit more about below.

"Magic" default behavior

The problem is that you are apparently hoping that you can trust the default copy constructor to "do the right thing" in a non-trivial case such as this. I'm not sure why you expected the right behavior--perhaps you're hoping that memorizing rules-of-thumb such as the "rule of 3" will be a robust way to ensure that you don't shoot yourself in the foot? Certainly that would be nice (and, as pointed out in another answer, Rust goes much further than other low-level languages toward making foot-shooting much harder), but C++ simply isn't designed for "thoughtless" class design of that sort, nor should it be.

Conceptualizing constructor behavior

I'm not going to try to address the question of whether this is a "well-known problem", because I don't really know how well-characterized the problem of "sister" data and iterator-storing is. But I hope that I can convince you that, if you take the time to think about copy-constructor-behavior for every class you write that can be copied, this shouldn't be a surprising problem.

In particular, when deciding to use the default copy-constructor, you must think about what the default copy-constructor will actually do: namely, it will call the copy-constructor of each non-primitive, non-union member (i.e. members that have copy-constructors) and bitwise-copy the rest.

When copying your vector of iterators, what does std::vector's copy-constructor do? It performs a "deep copy", i.e., the data inside the vector is copied. Now, if the vector contains iterators, how does that affect the situation? Well, it's simple: the iterators are the data stored by the vector, so the iterators themselves will be copied. What does an iterator's copy-constructor do? I'm not going to actually look this up, because I don't need to know the specifics: I just need to know that iterators are like pointers in this (and other respect), and copying a pointer just copies the pointer itself, not the data pointed to. I.e., iterators and pointers do not have deep-copying by default.

Note that this is not surprising: of course iterators don't do deep-copying by default. If they did, you'd get a different, new set for each iterator being copied. And this makes even less sense than it initially appears: for instance, what would it actually mean if uni-directional iterators made deep-copies of their data? Presumably you'd get a partial copy, i.e., all the remaining data that's still "in front of" the iterator's current position, plus a new iterator pointing to the "front" of the new data structure.

Now consider that there is no way for a copy-constructor to know the context in which it's being called. For instance, consider the following code:

using iter = std::set<size_t>::iterator;  // use typedef pre-C++11
std::vector<iter> foo = getIters();  // get a vector of iterators
useIters(foo);    // pass vector by value

When getIters is called, the return value might be moved, but it might also be copy-constructed. The assignment to foo also invokes a copy-constructor, though this may also be elided. And unless useIters takes its argument by reference, then you've also got a copy constructor call there.

In any of these cases, would you expect the copy constructor to change which std::set is pointed to by the iterators contained by the std::vector<iter>? Of course not! So naturally std::vector's copy-constructor can't be designed to modify the iterators in that particular way, and in fact std::vector's copy-constructor is exactly what you need in most cases where it will actually be used.

However, suppose std::vector could work like this: suppose it had a special overload for "vector-of-iterators" that could re-seat the iterators, and that the compiler could somehow be "told" only to invoke this special constructor when the iterators actually need to be re-seated. (Note that the solution of "only invoke the special overload when generating a default constructor for a containing class that also contains an instance of the iterators' underlying data type" wouldn't work; what if the std::vector iterators in your case were pointing at a different standard set, and were being treated simply as a reference to data managed by some other class? Heck, how is the compiler supposed to know whether the iterators all point to the same std::set?) Ignoring this problem of how the compiler would know when to invoke this special constructor, what would the constructor code look like? Let's try it, using _Ctnr<T>::iterator as our iterator type (I'll use C++11/14isms and be a bit sloppy, but the overall point should be clear):

template <typename T, typename _Ctnr>
std::vector< _Ctnr<T>::iterator> (const std::vector< _Ctnr<T>::iterator>& rhs)
  : _data{ /* ... */ } // initialize underlying data...
{
    for (auto i& : rhs)
    {
        _data.emplace_back( /* ... */ );  // What do we put here?
    }
}

Okay, so we want each new, copied iterator to be re-seated to refer to a different instance of _Ctnr<T>. But where would this information come from? Note that the copy-constructor can't take the new _Ctnr<T> as an argument: then it would no longer be a copy-constructor. And in any case, how would the compiler know which _Ctnr<T> to provide? (Note, too, that for many containers, finding the "corresponding iterator" for the new container may be non-trivial.)

Resource management with std:: containers

This isn't just an issue of the compiler not being as "smart" as it could or should be. This is an instance where you, the programmer, have a specific design in mind that requires a specific solution. In particular, as mentioned above, you have two resources, both std:: containers. And you have a relationship between them. Here we get to something that most of the other answers have stated, and which by this point should be very, very clear: related class members require special care, since C++ does not manage this coupling by default. But what I hope is also clear by this point is that you shouldn't think of the problem as arising specifically because of data-member coupling; the problem is simply that default-construction isn't magic, and the programmer must be aware of the requirements for correctly copying a class before deciding to let the implicitly-generated constructor handle copying.

The elegant solution

...And now we get to aesthetics and opinions. You seem to find it inelegant to be forced to write a copy-constructor when you don't have any raw pointers or arrays in your class that must be manually managed.

But user-defined copy constructors are elegant; allowing you to write them is C++'s elegant solution to the problem of writing correct non-trivial classes.

Admittedly, this seems like a case where the "rule of 3" doesn't quite apply, since there's a clear need to either =delete the copy-constructor or write it yourself, but there's no clear need (yet) for a user-defined destructor. But again, you can't simply program based on rules of thumb and expect everything to work correctly, especially in a low-level language such as C++; you must be aware of the details of (1) what you actually want and (2) how that can be achieved.

So, given that the coupling between your std::set and your std::vector actually creates a non-trivial problem, solving the problem by wrapping them together in a class that correctly implements (or simply deletes) the copy-constructor is actually a very elegant (and idiomatic) solution.

Explicitly defining versus deleting

You mention a potential new "rule of thumb" to follow in your coding practices: "Disable copying by default on all classes I write, unless I can explicitly prove they are correct." While this might be a safer rule of thumb (at least in this case) than the "rule of 3" (especially when your criterion for "do I need to implement the 3" is to check whether a deleter is required), my above caution against relying on rules of thumb still applies.

But I think the solution here is actually simpler than the proposed rule of thumb. You don't need to formally prove the correctness of the default method; you simply need to have a basic idea of what it would do, and what you need it to do.

Above, in my analysis of your particular case, I went into a lot of detail--for instance, I brought up the possibility of "deep-copying iterators". You don't need to go into this much detail to determine whether or not the default copy-constructor will work correctly. Instead, simply imagine what your manually-created copy constructor will look like; you should be able to tell pretty quickly how similar your imaginary explicitly-defined constructor is to the one the compiler would generate.

For example, a class Foo containing a single vector data will have a copy constructor that looks like this:

Foo::Foo(const Foo& rhs)
  : data{rhs.data}
{}

Without even writing that out, you know that you can rely on the implicitly-generated one, because it's exactly the same as what you'd have written above.

Now, consider the constructor for your class Foo:

Foo::Foo(const Foo& rhs)
  : set{rhs.set}
  , vector{ /* somehow use both rhs.set AND rhs.vector */ }  // ...????
{}

Right away, given that simply copying vector's members won't work, you can tell that the default constructor won't work. So now you need to decide whether your class needs to be copyable or not.

Answer 2

Is this a well-known problem?

Well, it is known, but I would not say well-known. Sibling pointers do not occur often, and most implementations I have seen in the wild were broken in the exact same way than yours is.

I believe the problem to be infrequent enough to have escaped most people's notice; interestingly, as I follow more Rust than C++ nowadays, it crops up there quite often because of the strictness of the type system (ie, the compiler refuses those programs, prompting questions).

does it have an elegant/idiomatic solution?

There are many types of sibling pointers situations, so it really depends, however I know of two generic solutions:

• keys
• shared elements

Let's review them in order.

Pointing to a class-member, or pointing into an indexable container, then one can use an offset or key rather than an iterator. It is slightly less efficient (and might require a look-up) however it is a fairly simple strategy. I have seen it used to great effect in shared-memory situation (where using pointers is a no-no since the shared-memory area may be mapped at different addresses).

The other solution is used by Boost.MultiIndex, and consists in an alternative memory layout. It stems from the principle of the intrusive container: instead of putting the element into the container (moving it in memory), an intrusive container uses hooks already inside the element to wire it at the right place. Starting from there, it is easy enough to use different hooks to wire a single elements into multiple containers, right?

Well, Boost.MultiIndex kicks it two steps further:

1. It uses the traditional container interface (ie, move your object in), but the node to which the object is moved in is an element with multiple hooks
2. It uses various hooks/containers in a single entity

You can check various examples and notably Example 5: Sequenced Indices looks a lot like your own code.

Answer 3

C++ copy/move ctor/assign are safe for regular value types. Regular value types behave like integers or other "regular" values.

They are also safe for pointer semantic types, so long as the operation does not change what the pointer "should" point to. Pointing to something "within yourself", or another member, is an example of where it fails.

They are somewhat safe for reference semantic types, but mixing pointer/reference/value semantics in the same class tends to be unsafe/buggy/dangerous in practice.

The rule of zero is that you make classes that behave like either regular value types, or pointer semantic types that don't need to be reseated on copy/move. Then you don't have to write copy/move ctors.

Iterators follow pointer semantics.

The idiomatic/elegant around this is to tightly couple the iterator container with the pointed-into container, and block or write the copy ctor there. They aren't really separate things once one contains pointers into the other.

Answer 4

The assertion that Foo is not managing any resources is false.

Copy-constructor aside, if a element of set is removed, there must be code in Foo that manages vector so that the respective iterator is removed.

I think the idiomatic solution is to just use one container, a vector<size_t>, and check that the count of an element is zero before inserting. Then the copy and move defaults are fine.

Answer 5

Is this a well-known problem

Yes. Any time you have a class that contains pointers, or pointer-like data like an iterator, you have to implement your own copy-constructor and assignment-operator to ensure the new object has valid pointers/iterators.

and if so, does it have an elegant/idiomatic solution?

Maybe not as elegant as you might like, and probably is not the best in performance (but then, copies sometimes are not, which is why C++11 added move semantics), but maybe something like this would work for you (assuming the std::vector contains iterators into the std::set of the same parent object):

class Foo
{
private:
    std::set<size_t> s;
    std::vector<std::set<size_t>::iterator> v;

    struct findAndPushIterator
    {
        Foo &foo;
        findAndPushIterator(Foo &f) : foo(f) {}

        void operator()(const std::set<size_t>::iterator &iter)
        {
            std::set<size_t>::iterator found = foo.s.find(*iter);
            if (found != foo.s.end())
                foo.v.push_back(found);
        }
    };

public:
    Foo() {}

    Foo(const Foo &src)
    {
        *this = src;
    }

    Foo& operator=(const Foo &rhs)
    {
        v.clear();
        s = rhs.s;

        v.reserve(rhs.v.size());
        std::for_each(rhs.v.begin(), rhs.v.end(), findAndPushIterator(*this));

        return *this;
    }

    //...
};

Or, if using C++11:

class Foo
{
private:
    std::set<size_t> s;
    std::vector<std::set<size_t>::iterator> v;

public:
    Foo() {}

    Foo(const Foo &src)
    {
        *this = src;
    }

    Foo& operator=(const Foo &rhs)
    {
        v.clear();
        s = rhs.s;

        v.reserve(rhs.v.size());
        std::for_each(rhs.v.begin(), rhs.v.end(),
            [this](const std::set<size_t>::iterator &iter)
            {
                std::set<size_t>::iterator found = s.find(*iter);
                if (found != s.end())
                   v.push_back(found);
            } 
        );

        return *this;
    }

    //...
};

Answer 6

Yes, of course it's a well-known problem.

If your class stored pointers, as an experienced developer you would intuitively know that the default copy behaviours may not be sufficient for that class.

Your class stores iterators and, since they are also "handles" to data stored elsewhere, the same logic applies.

This is hardly "astonishing".

Answer 7

Yes, this is a well known "problem" -- whenever you store pointers in an object, you're probably going to need some kind of custom copy constructor and assignment operator to ensure that the pointers are all valid and point at the expected things.

Since iterators are just an abstraction of collection element pointers, they have the same issue.