Should it be considered a BC break to return clone instead of new self?

Question

(This question applies to the equivalent code in both Java and PHP)

I have a class like this:

class Foo {
    private int $bar;

    public function __construct(int $bar) {
        $this->bar = $bar;
    }

    public function add(int $bar) : Foo {
        return new Foo($this->bar + $bar);
    }
}

Due to certain reasons (the reason is irrelevant to this question), cloning the object is a faster implementation than calling new Foo, so I can change the add function to this:

public function add(int $bar) : Foo {
    $foo = clone $this; // equivalently, this.clone() in Java
    $foo->bar += $bar;
    return $foo;
}

If Foo was final, this should not result in any BC issues. Unfortunately, this is not the case. In fact, there is a subclass of Foo in the API:

class HeavyFoo extends Foo {
    private SomeHeavyObject $object;
}

SomeHeavyObject is a heavy class (as in it references lots of other data), and it should be garbage-collected as soon as unneeded to keep the memory usage low.

Meanwhile, there are lots of users writing this kind of code:

$added = getHeavyFoo()->add(3);
$someLongLivingStorage->insert($added);

With the assumption that Foo.add will return a light (only containing an int field) object, users insert the result of add to a storage that lives much longer than HeavyObject would. This is not exactly a memory leak, but greatly increases the memory consumption anyway.

However, according to OOP principles, returning a subclass instead of the superclass is totally fine.

Should I bump my semver major version because of this change?

(For those wondering why change to clone: I wrote a PHP extension such that if(ref_count_for($this) > 1) { $this = clone $this; }, so no need to allocate a new object if $this is a temporary that isn't referenced anywhere else, e.g. the intermediates in $foo->add(1)->add(2)->add(3))

Answer 1

You are changing what an existing API call does, so I'd say that yes, you need a major version bump.

Moreover, you say that there is a performance improvement but also a potential memory issue, so whether the change is a net improvement or not depends on the user use case.

If this performance gain is significant for your project I'd suggest that you add to the API instead.

You could add a new method called selfAdd (or something similar) or, if this behaviour is present in several different methods, a better solution might be to make it a different implementation of the original class'interface. This still lets the user choose the implementation better suited to their needs without cluttering the class interface.

Answer 2

I will assume that it is unspecified if HeavyFoo::add returns (a reference to) a HeavyFoo or a Foo object.

If that is the case, then technically you haven't changed the interface and don't need to bump the major version. The users that are relying on getting the lightweight object are relying on undocumented behavior.

On the other hand, if that undocumented behavior has been present for a significant time, a large proportion of your users depend on it and the change causes them a real inconvenience, then it would be a good idea to bump the major version anyway as a signal to your users that they must pay attention when upgrading to the new version.

Answer 3

Depending on the state of the object to clone, i would say yes. Since the constructor of a object may only fill some of the fields of the class. This will cause breakage in scenarios where fields get lazy loaded (caching for example). If thats not the case, as in the amount of fields in that class is equal to the amount of arguments in the constructor and thus all fields are final there is no breakage to be considered.

Another thing is to check for hooking implementations. PHP for example has __clone which some developers may have used for other purposes than you initially intended. Due to the nature of the planned change this hook would be executed and lead to undesired behaviour.

For the java end that shouldn't be a problem since you can hide the clone method from your implementation and thus hide it for overriding (which may cause further breakage though)