groovy
  1. groovy
  2. GROOVY-3713

@Immutable should not override toString() if provided in source

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7-beta-1
    • Fix Version/s: 1.6.5, 1.7-beta-2
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      1

      Description

      @Immutable class MyClass {
          String toString() { 'some return value' }
      }
      new MyClass()
      

      throws

      Repetitive method name/signature for method 'java.lang.String toString()' in class 'MyClass'.
       at line: 2, column: 5
      
      Repetitive method name/signature for method 'java.lang.String toString()' in class 'MyClass'.
       at line: -1, column: -1
      

        Activity

        Hide
        Roshan Dawrani added a comment -

        Did you mean?

        String toString() { 'some return value' }
        
        Show
        Roshan Dawrani added a comment - Did you mean? String toString() { 'some return value' }
        Hide
        Dierk König added a comment -

        changed return type void to String

        Show
        Dierk König added a comment - changed return type void to String
        Hide
        Roshan Dawrani added a comment -

        ImmutableASTTransformation also adds equals() and hashCode() too. Should they also not be added if they are already provided in source.

        I understand that from Class Format point of view, they should not be added but does anyone see any conflict it would create with the @Immutable functionality? If there is some issue there, then some other solution may have to be adopted.

        Show
        Roshan Dawrani added a comment - ImmutableASTTransformation also adds equals() and hashCode() too. Should they also not be added if they are already provided in source. I understand that from Class Format point of view, they should not be added but does anyone see any conflict it would create with the @Immutable functionality? If there is some issue there, then some other solution may have to be adopted.
        Hide
        Guillaume Laforge added a comment -

        Immutability comes partly from the fact equals() and hashCode() work the way they're implemented by the @Immutable transformation.
        So someone could break the immutability if he provided a wrong equals() and hashCode() himself – but toString() should be okay if a user provide his own.
        Instead, I think we should throw a compilation error if someone provides their own equals() and hashCode().

        Paul King may want to add some thoughts there, as @Immutable is his baby

        Show
        Guillaume Laforge added a comment - Immutability comes partly from the fact equals() and hashCode() work the way they're implemented by the @Immutable transformation. So someone could break the immutability if he provided a wrong equals() and hashCode() himself – but toString() should be okay if a user provide his own. Instead, I think we should throw a compilation error if someone provides their own equals() and hashCode(). Paul King may want to add some thoughts there, as @Immutable is his baby
        Hide
        Roshan Dawrani added a comment -

        I will try the toString() change in the meanwhile and wait for more comments on whether users can be told up-front that they should not provide hashCode() / equals() with @Immutable. Some change there is needed anyway because if they provide, class generated will run into ClassFormatError due to repetitive hashCode() / equals() methods.

        Show
        Roshan Dawrani added a comment - I will try the toString() change in the meanwhile and wait for more comments on whether users can be told up-front that they should not provide hashCode() / equals() with @Immutable. Some change there is needed anyway because if they provide, class generated will run into ClassFormatError due to repetitive hashCode() / equals() methods.
        Hide
        Roshan Dawrani added a comment -

        Attaching a patch with

        1) throwing errors for overridden equals() / hashCode()

        2) allowing users to override toString() by not adding the generated toString() if it is provided.

        Show
        Roshan Dawrani added a comment - Attaching a patch with 1) throwing errors for overridden equals() / hashCode() 2) allowing users to override toString() by not adding the generated toString() if it is provided.
        Hide
        blackdrag blackdrag added a comment -

        Roshan, why don't you use cNode.getDeclaredMethods("equals") and such?

        Show
        blackdrag blackdrag added a comment - Roshan, why don't you use cNode.getDeclaredMethods("equals") and such?
        Hide
        Paul King added a comment -

        Just getting back into work after returning from Agile 2009. I'll take a look at this today. Patch looks good - though I am in two minds about whether to allow equals/hashCode methods to be overridden as well.

        Show
        Paul King added a comment - Just getting back into work after returning from Agile 2009. I'll take a look at this today. Patch looks good - though I am in two minds about whether to allow equals/hashCode methods to be overridden as well.
        Hide
        Roshan Dawrani added a comment -

        Attaching a newer patch that uses getDeclaredMethods() and has a test case.

        Show
        Roshan Dawrani added a comment - Attaching a newer patch that uses getDeclaredMethods() and has a test case.
        Hide
        Paul King added a comment -

        I ended up allowing equals and hashCode to be overridden as well since we allow additional (non-property) state variables to be added which up until now we could only ignore - now we can allow code for most "properties" to be automatically generated but have an extensible way to handle some properties manually.

        Show
        Paul King added a comment - I ended up allowing equals and hashCode to be overridden as well since we allow additional (non-property) state variables to be added which up until now we could only ignore - now we can allow code for most "properties" to be automatically generated but have an extensible way to handle some properties manually.
        Hide
        Dierk König added a comment -

        superb, thanks
        Dierk

        Show
        Dierk König added a comment - superb, thanks Dierk

          People

          • Assignee:
            Paul King
            Reporter:
            Dierk König
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: