[cli] clone method in Option should use super.clone()

• Key: CLI-21
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Nathan McDonald
• Component/s: CLI-1.x
• Affects Version/s: 1.0
• Fix Version/s: 1.1

Environment

Operating System: other
Platform: Other

Description

In
org.apache.commons.cli.Option
public method clone is implemented by creating a new instance through one of
the class Constructors, and then assigning values as required through the
setter methods.

This means that any subclasses of the Option class will not return a true
clone, but a new Option instance instead.

A proper implementation of clone should use super.clone() to create a new
instance, rather than calling the class constructor. This allows shallows
clones to propogate correctly down to subclasses.

Comments

[Henri Yandell]

+1 on the implementation not correctly supporting subclassing.

Is there even a use case for cloning an Option I wonder. Nothing else can be cloned - why would an
Option be special.

[Sebb]

An alternative is to make the class final.

[Henri Yandell]

clone() showed up in a commit from jkeyes, whose comment doesn't indicate any
reason for the addition:

http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/cli/trunk/src/java/org/apache/commons/cli/Option.java?rev=129799&r1=129796&r2=129799

[Henri Yandell]

Removed clone() from Option - thus resolving this bug.

[Brian Egge]

Properly supporting Cloneable is fairly trivial. Previously I submitted a patch which includes a proper implementation and test case. See:

https://issues.apache.org/jira/secure/attachment/12358085/Cloneable.patch

There's some other code in the patch which does not need to be included. In practice usually the CLI library is only invoked once at startup, so it doesn't matter that some classes aren't quite immutable. However, in unit testing, the options may be reused.

I have some code which looks like this:

abstract class CommandLine {
protected static final Option URL = new Option("u", "url", true, "url");
}

I'll then share this option in various subclasses. In my unit tests, it would be best if I don't alter the original static option. In this situation having a clone option might be helpful. The better solution would probably just be not to make it static and forget about cloning altogether.

I'm not opposed to removing clone, but I don't see a strong enough reason here to break compatibility.

[Ben Speakmon]

My comment from the mailing list thread:

Implementing clone() properly is a pain and hard to get right. If someone really needs distinct copies of Option classes (why?!), I'd suggest providing a copy constructor instead. If compatibility is a really big deal, you could change clone() to call Object.clone(), which throws an exception. Think of it as a gentle hint to the user not to use the method anymore – and the exception message could also point out the new copy constructor.

[Brian Egge]

Add clone method back to Option. Includes test case with subclass.

[Henri Yandell]

Patch applied - thanks for doing that Brian.

One question - when cloning, should the effects of addValue/processValue be undone?

ie) Rather than:

option.values = new ArrayList(values);

it should be:

option.values = new ArrayList();

Or should we just recommend in the javadoc for clone() that people will probably want to immediatley call clearValues()?

[Brian Egge]

I think clearing the values would generally be useful, but I think copying them follows the principle of least surprise. I think we should update the JavaDoc to recommend calling clearValues.

[Henri Yandell]

Another thought - as I kick myself to keep momentum on the CLI release. Need to find out if a change from:

public Object clone()

to

protected Object clone() throws CloneNotSupportedException

will be voted down. I presume it will.

[Henri Yandell]

Here's the clirr error:

ERROR: 7009: org.apache.commons.cli.Option: Accessibility of method 'public java.lang.Object clone()' has been decreased from public to protected

Solutionwise, I guess we make it public again, drop the exception and throw a RuntimeException if it's thrown. Will attach a patch to that end.

I am confused that Clirr doesn't seem to care about the exception being added.

[Henri Yandell]

Patch making the clone method public again, and dropping the exception.