Changing methods in interfaces

[drdanz]

Working on #2587, I came up with this idea to deprecate the old interfaces and replace them gradually with new ones, and I'm trying to figure out if this will work:

Assuming we have an interface IFoo

class IFoo
{
public:
    virtual ~IFoo() = default;
    virtual bool bar() = 0;
    virtual bool deprecatedMethod() = 0;
};

If we add an IFoo2 interface that derives from IFoo where old pure virtual methods are declared private and final and implemented inline using the new methods, for example:

class IFoo2 :
    public IFoo
{
public:
    ~IFoo2() override = default;

    virtual bool newMethod() const = 0;

private:
    bool deprecatedMethod() final
    {
        return const_cast<const IFoo*>(this)->newMethod();
    }
};

• Devices implementing the IFoo interface will see no differences (perhaps they will get a deprecated warning for implementing a deprecated interface)
• Devices using the IFoo interface will be able to use any device implementing IFoo or IFoo2 (and perhaps will a deprecated warning when using the deprecated methods)
• Devices implementing IFoo2 will automatically implement also IFoo. They will not be able to implement the deprecated methods, since they are declared final
• Devices using IFoo2 will be able to use only the devices implementing the new interface, it will not be able to use the deprecated methods, since they are private for the IFoo2 interface, but they can eventually try to use the IFoo2, and switch to the IFoo interface, if the new one is not available.

The only problem that I can see right now, is that we are back to the "2" interfaces... But in this way, we don't have to add methods that are not pure virtual to the interfaces, and we do not break the API/ABI for changing method signatures or adding new ones

Is there any drawback I'm not seeing right now? Is there a reason why we are not already doing this in this way right now?

---

Edit: One extra drawback is when we want to disable the deprecated parts in YARP (i.e. YARP_NO_DEPRECATED), the IFoo interface will disapperar, and the IFoo2 interface will stay, so all the methods that are not explicitly declared in IFoo2 will not exist, therefore they should be declared explicitly in the #else branch of the #ifndef YARP_NO_DEPRECATED. Also when we allow breaking changes for major releases, we cannot just remove deprecated part, we also have to take care of the 2 in the names.

class IFoo2() :
    public IFoo
{
public:
    ~IFoo2() override = default;

    virtual bool newMethod() const = 0;
#ifndef YARP_NO_DEPRECATED
private:
    bool deprecatedMethod() final
    {
        return const_cast<const IFoo*>(this)->newMethod();
    }
#else // YARP_NO_DEPRECATED

    virtual bool bar() = 0;

#endif // YARP_NO_DEPRECATED
};

---

Edit 2: Another drawback is that, if you change the signature of the method, and keep the same name, when you implement a class that inherits from the new interface, we will get a -Woverloaded-virtual warning, because the old method is hidden. This is an annoying issue, because it means that the user will have to add using to hide this warning for methods that are private and final.

---

Edit 3: Yet another drawback. If a method is changed from non-const to const, and the non-const version is private, the method can be called only from a const object (fix: in this case it should have the same visibility as the non-const version)

[PeterBowman]

Is there a reason why we are not already doing this in this way right now?

This syntax requires C++11, it might not have been available by the time IPositionControl2 and others were conceived (I'm just guessing).

(...) when we want to disable the deprecated parts in YARP (i.e. YARP_NO_DEPRECATED), the IFoo interface will disappear (...)

Isn't this going to follow the usual deprecation path as it was the case with IPositionControl2 et al.? Like so:

• IFoo exists.
• We want to deprecate IFoo::deprecatedMethod, perhaps add new methods as well.
• IFoo::deprecatedMethod is marked as deprecated via compiler attribute, IFoo2 is created featuring new methods.
• After a longer while, IFoo::deprecatedMethod is removed for good, IFoo2 is merged into IFoo, IFoo2 is an alias for IFoo.
• Finally, IFoo2 is deleted.

As I see it, YARP_NO_DEPRECATED would only surround IFoo::deprecatedMethod instead of whole IFoo.

(...) so all the methods that are not explicitly declared in IFoo2 will not exist, therefore they should be declared explicitly in the #else branch of the #ifndef YARP_NO_DEPRECATED.

Assuming I'm wrong in the previous point and IFoo would indeed disappear, I think you can safely re-declare pure-virtual methods in IFoo2, i.e. bar may stay outside the YARP_NO_DEPRECATED block. Note the destructor should be virtual and defaulted as in IFoo, too, and override would not work (there would be no base destructor).

(...) we also have to take care of the 2 in the names

As an alternative to the 2 suffix, @jgvictores and I have been thinking about placing the new interface in a nested namespace:

class IFoo { /* stuff */ };

namespace future {
    class IFoo { /* stuff */ };
}

Note the same class name is preserved. Alternatives for said namespace: incoming, staging, novel, experimental.

[drdanz]

Actually this made me realize that another very interesting option would be to use inline namespaces, I'll investigate this

[drdanz]

A tiny example that shows all issues found so far:

Source code:

#include <iostream>

/*inline*/ namespace v1
{

class IFoo
{
public:
    virtual ~IFoo() = default;
    virtual void dofoo() = 0;
    virtual void deprecatedMethod() = 0;
    virtual void changed1() = 0;
    virtual void changed2() = 0;

};

}

namespace v2
{

class IFoo :
    public v1::IFoo
{
public:
    ~IFoo() override = default;

    virtual void newMethod() const = 0;
    virtual void changed1() const = 0;
    virtual void changed2() const = 0;

    virtual void changed1() final
    {
        std::cout << __PRETTY_FUNCTION__ << '\n';
        return const_cast<const IFoo*>(this)->changed1();
    }
private:
    void deprecatedMethod() final
    {
        std::cout << __PRETTY_FUNCTION__ << '\n';
        return const_cast<const IFoo*>(this)->newMethod();
    }

    virtual void changed2() final
    {
        std::cout << __PRETTY_FUNCTION__ << '\n';
        return const_cast<const IFoo*>(this)->changed2();
    }
};
}


class Bar1 : public v1::IFoo
{
    void dofoo() override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void deprecatedMethod() override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void changed1() override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void changed2() override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }
};

class Bar2 : public v2::IFoo
{
    void dofoo() override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void newMethod() const override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void changed1() const override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

    void changed2() const override
    {
        std::cout << __PRETTY_FUNCTION__ << "\n\n";
    }

//     void deprecatedMethod() override {} // Error (OK: method is final)
//     void changed1() override {} // Error (OK: method is final)
//     void changed2() override {} // Error (OK: method is final)
};



int main()
{
    Bar1 bar1;
    Bar2 bar2;

    {
        std::cout << "--- BEGIN Test 1\n";
        auto* x = dynamic_cast<v1::IFoo*>(&bar1);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
            x->dofoo();
            x->deprecatedMethod();
            x->changed1();
            x->changed2();
        }
        std::cout << "--- END Test 1\n";
    }

    {
        std::cout << "--- BEGIN Test 2\n";
        const auto* x = dynamic_cast<v1::IFoo*>(&bar1);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
//             x->dofoo(); // Cannot call (OK: non-const method)
//             x->deprecatedMethod(); // Cannot call (OK: non-const method)
//             x->changed1(); // Cannot call (OK: non-const method)
//             x->changed2(); // Cannot call (OK: non-const method)
        }
        std::cout << "--- END Test 2\n";
    }

    {
        std::cout << "--- BEGIN Test 3\n";
        auto* x = dynamic_cast<v2::IFoo*>(&bar2);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
            x->dofoo();
            x->newMethod();
//             x->deprecatedMethod(); // Cannot call (OK: method is not available in the new interface)
            x->changed1(); // Runtime error (BAD!!! recursive call!)
//             x->changed2(); // Cannot call (BAD!!! cannot call on a non-const object)
        }
        std::cout << "--- END Test 3\n";
    }

    {
        std::cout << "--- BEGIN Test 4\n";
        const auto* x = dynamic_cast<const v2::IFoo*>(&bar2);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
//             x->dofoo(); // Cannot call (OK: non-const method)
            x->newMethod();
//             x->deprecatedMethod(); // Cannot call (method is not available in the new interface)
            x->changed1();
            x->changed2();
        }
        std::cout << "--- END Test 4\n";
    }

    {
        std::cout << "--- BEGIN Test 5\n";
        auto* x = dynamic_cast<v1::IFoo*>(&bar2);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
            x->dofoo();
            x->deprecatedMethod();
            x->changed1();
            x->changed2();
        }
        std::cout << "--- END Test 5\n";
    }

    {
        std::cout << "--- BEGIN Test 6\n";
        const auto* x = dynamic_cast<v1::IFoo*>(&bar2);
        if (!x) {
            std::cout << "dynamic cast failed\n";
        } else {
//             x->dofoo(); // Cannot call (OK: non-const method)
//             x->deprecatedMethod(); // Cannot call (OK: non-const method)
//             x->changed1(); // Cannot call (OK: non-const method)
//             x->changed2(); // Cannot call (OK: non-const method)
        std::cout << "--- END Test 6\n";
        }
    }

}

Compiler output (compiled with `-Wall -Wextra -Woverloaded-virtual`):

/tmp/test.cpp:44:18: warning: ‘virtual void v2::IFoo::changed2()’ was hidden [-Woverloaded-virtual]
   44 |     virtual void changed2() final
      |                  ^~~~~~~~
/tmp/test.cpp:93:10: note:   by ‘virtual void Bar2::changed2() const’
   93 |     void changed2() const override
      |          ^~~~~~~~
/tmp/test.cpp:32:18: warning: ‘virtual void v2::IFoo::changed1()’ was hidden [-Woverloaded-virtual]
   32 |     virtual void changed1() final
      |                  ^~~~~~~~
/tmp/test.cpp:88:10: note:   by ‘virtual void Bar2::changed1() const’
   88 |     void changed1() const override
      |          ^~~~~~~~

Execution output:

--- BEGIN Test 1
virtual void Bar1::dofoo()

virtual void Bar1::deprecatedMethod()

virtual void Bar1::changed1()

virtual void Bar1::changed2()

--- END Test 1
--- BEGIN Test 2
--- END Test 2
--- BEGIN Test 3
virtual void Bar2::dofoo()

virtual void Bar2::newMethod() const

virtual void v2::IFoo::changed1()
virtual void Bar2::changed1() const

--- END Test 3
--- BEGIN Test 4
virtual void Bar2::newMethod() const

virtual void Bar2::changed1() const

virtual void Bar2::changed2() const

--- END Test 4
--- BEGIN Test 5
virtual void Bar2::dofoo()

virtual void v2::IFoo::deprecatedMethod()
virtual void Bar2::newMethod() const

virtual void v2::IFoo::changed1()
virtual void Bar2::changed1() const

virtual void v2::IFoo::changed2()
virtual void Bar2::changed2() const

--- END Test 5
--- BEGIN Test 6
--- END Test 6

Unfortunately the -Woverloaded-virtual issue is not easily solved, when implementing Bar2 it would be possible to hide the warning with a couple of using statements, but this means that who implements the IFoo2 interface needs to know about the IFoo, and moreover he needs to add a check on YARP_NO_DEPRECATED, because the using would become an error if the IFoo interface does not exist.