Uploaded image for project: 'Commons CLI'
  1. Commons CLI
  2. CLI-21

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 1.1
    • Component/s: CLI-1.x
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

    • Bugzilla Id:
      29908

      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.

      1. bug21.patch
        3 kB
        Brian Egge
      2. CLI-21.patch
        0.8 kB
        Henri Yandell

        Activity

        Hide
        bayard Henri Yandell added a comment -

        +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.

        Show
        bayard Henri Yandell added a comment - +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.
        Hide
        sebb@apache.org Sebb added a comment -

        An alternative is to make the class final.

        Show
        sebb@apache.org Sebb added a comment - An alternative is to make the class final.
        Hide
        bayard Henri Yandell added a comment -

        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

        Show
        bayard Henri Yandell added a comment - 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
        Hide
        bayard Henri Yandell added a comment -

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

        Show
        bayard Henri Yandell added a comment - Removed clone() from Option - thus resolving this bug.
        Hide
        brianegge Brian Egge added a comment -

        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.

        Show
        brianegge Brian Egge added a comment - 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.
        Hide
        bens Ben Speakmon added a comment -

        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.

        Show
        bens Ben Speakmon added a comment - 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.
        Hide
        brianegge Brian Egge added a comment -

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

        Show
        brianegge Brian Egge added a comment - Add clone method back to Option. Includes test case with subclass.
        Hide
        bayard Henri Yandell added a comment -

        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()?

        Show
        bayard Henri Yandell added a comment - 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()?
        Hide
        brianegge Brian Egge added a comment -

        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.

        Show
        brianegge Brian Egge added a comment - 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.
        Hide
        bayard Henri Yandell added a comment -

        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.

        Show
        bayard Henri Yandell added a comment - 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.
        Hide
        bayard Henri Yandell added a comment -

        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.

        Show
        bayard Henri Yandell added a comment - 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.
        Hide
        bayard Henri Yandell added a comment -

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

        Show
        bayard Henri Yandell added a comment - Patch making the clone method public again, and dropping the exception.

          People

          • Assignee:
            Unassigned
            Reporter:
            nathan.mcdonald@optusnet.com.au Nathan McDonald
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development