groovy
  1. groovy
  2. GROOVY-5011

@Override should be allowed (and verified) on properties that implement an interface

    Details

    • Type: Improvement Improvement
    • Status: Open Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Compiler
    • Labels:
      None
    • Number of attachments :
      0

      Description

      I would like @Override to be allowed (and verified) on fields that implement an interface.

      For instance, this is valid Groovy code:

      interface Parent {
          String getSetting()
      }
      class Child implements Parent {
          final String setting ='my setting'
      }
      

      I'd like to add the @Override to the field and have the compiler verify that it does override the getter in the parent. It should work on boolean getters too, I suppose.

        Issue Links

          Activity

          Hide
          Roshan Dawrani added a comment -

          Isn't @Override defined by Java and allowed only on methods?

          Show
          Roshan Dawrani added a comment - Isn't @Override defined by Java and allowed only on methods?
          Hide
          Paul King added a comment -

          Hamlet, is it correct that you are really wanting this only on properties? I.e. it is on methods as per Java but allowing the property short-hand to specify the methods?

          Show
          Paul King added a comment - Hamlet, is it correct that you are really wanting this only on properties? I.e. it is on methods as per Java but allowing the property short-hand to specify the methods?
          Hide
          Hamlet D'Arcy added a comment -

          @Override is defined by Java, but Groovy could always loosen that restriction.

          I do want it on properties. If "Object getFoo()" is generated, and the object implements an interface where that method is defined, then I'd like to add the @Override annotation. Same goes for "void setFoo(Object)"

          Are there other uses cases you had in mind?

          Show
          Hamlet D'Arcy added a comment - @Override is defined by Java, but Groovy could always loosen that restriction. I do want it on properties. If "Object getFoo()" is generated, and the object implements an interface where that method is defined, then I'd like to add the @Override annotation. Same goes for "void setFoo(Object)" Are there other uses cases you had in mind?
          Hide
          Paul King added a comment -

          Well, for interfaces, the only fields are constants so I don't think it makes sense to use @Override with them. But for one class inheriting from another case, you could check that the super class also had a field definition if you had @Override on a field in the child class.

          Show
          Paul King added a comment - Well, for interfaces, the only fields are constants so I don't think it makes sense to use @Override with them. But for one class inheriting from another case, you could check that the super class also had a field definition if you had @Override on a field in the child class.
          Hide
          Roshan Dawrani added a comment -

          >> @Override is defined by Java, but Groovy could always loosen that restriction.

          Loosening the restriction just from the Groovy compiler point of view was not my concern. We control it from text to bytecode, so allowing it on properties and shifting the annotations to getters/setters before annotations are validated is a small trick.

          My concern was how such loosening of restrictions will play out with IDEs. They spend a lot of effort on giving early feedback and it may come in the way unless they also come at par with Groovy compiler.

          Show
          Roshan Dawrani added a comment - >> @Override is defined by Java, but Groovy could always loosen that restriction. Loosening the restriction just from the Groovy compiler point of view was not my concern. We control it from text to bytecode, so allowing it on properties and shifting the annotations to getters/setters before annotations are validated is a small trick. My concern was how such loosening of restrictions will play out with IDEs. They spend a lot of effort on giving early feedback and it may come in the way unless they also come at par with Groovy compiler.
          Hide
          Paul King added a comment -

          change title s/fields/properties/ as I believe that is what is intended.

          Show
          Paul King added a comment - change title s/fields/properties/ as I believe that is what is intended.
          Hide
          Paul King added a comment -

          So my understanding of the requirement is that if you write (using the example above):

          ...
          class Child implements Parent {
              @Override
              final String setting ='my setting'
          }
          

          It should end up being equivalent to:

          ...
          class Child implements Parent {
              @Override
              String getSetting() {
                  'my setting'
              }
          }
          

          Give or take using a backing field etc. which I will ignore here.

          Show
          Paul King added a comment - So my understanding of the requirement is that if you write (using the example above): ... class Child implements Parent { @Override final String setting ='my setting' } It should end up being equivalent to: ... class Child implements Parent { @Override String getSetting() { 'my setting' } } Give or take using a backing field etc. which I will ignore here.
          Hide
          Paul King added a comment - - edited

          Just some notes about how this currently would need to be implemented.

          The Verifier adds getter and setter methods for properties. Currently annotations on the property are left on the backing field and not copied over to the generated getter and setter methods.

          The ExtendedVerifier checks for misplaced annotations (e.g. currently produces "Annotation @java.lang.Override is not allowed on element FIELD") and also does Override checking.

          Both these run in the classgen phase. Luckily the Verifier runs before the ExtendedVerifier. So, to implement this feature the Verifier would need to remove the annotation from the field and add it to the getter (and, if generated, the setter). In general, this might not be desirable behavior for all annotations, so we could support some small list of annotations which would be supported or we would need a way to define some metadata about which annotations should be copied and to where. In general, this metadata might need to be augmented by user-specified annotations. Such a feature could become quite complex quite quickly. Perhaps it might be limited to a module by module list of annotations to process.

          Show
          Paul King added a comment - - edited Just some notes about how this currently would need to be implemented. The Verifier adds getter and setter methods for properties. Currently annotations on the property are left on the backing field and not copied over to the generated getter and setter methods. The ExtendedVerifier checks for misplaced annotations (e.g. currently produces "Annotation @java.lang.Override is not allowed on element FIELD") and also does Override checking. Both these run in the classgen phase. Luckily the Verifier runs before the ExtendedVerifier. So, to implement this feature the Verifier would need to remove the annotation from the field and add it to the getter (and, if generated, the setter). In general, this might not be desirable behavior for all annotations, so we could support some small list of annotations which would be supported or we would need a way to define some metadata about which annotations should be copied and to where. In general, this metadata might need to be augmented by user-specified annotations. Such a feature could become quite complex quite quickly. Perhaps it might be limited to a module by module list of annotations to process.

            People

            • Assignee:
              Unassigned
              Reporter:
              Hamlet D'Arcy
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated: