[GitHub] [groovy] sonatype-lift[bot] commented on a diff in pull request #1714: GROOVY-10570: `@AnnotationCollector`: better error for missing `value()`

[GitBox <gi...@apache.org>]

sonatype-lift[bot] commented on code in PR #1714:
URL: https://github.com/apache/groovy/pull/1714#discussion_r851384163


##########
src/main/java/org/codehaus/groovy/transform/AnnotationCollectorTransform.java:
##########
@@ -268,36 +270,43 @@ private static void copyMembers(final Map<String, Expression> members, final Ann
         }
     }
 
-    private static List<AnnotationNode> getTargetListFromClass(ClassNode alias) {
-        alias = getSerializeClass(alias);
-        Class<?> c = alias.getTypeClass();
+    private static List<AnnotationNode> getTargetListFromClass(final ClassNode alias) {
+        ClassNode cn = getSerializeClass(alias);
+        Class<?> c = cn.getTypeClass();
         Object[][] data;
         try {
             Method m = c.getMethod("value");
+            if (!Modifier.isStatic(m.getModifiers()))
+                throw new NoSuchMethodException("non-static value()");
+
             data = (Object[][]) m.invoke(null);
+            return makeListOfAnnotations(data);
+        } catch (NoSuchMethodException | ClassCastException e) {
+            throw new GroovyRuntimeException("Expecting static method `Object[][] value()`" +
+                    " in " + cn.toString(false) + ". Was it compiled from a Java source?");
         } catch (Exception e) {
             throw new GroovyBugError(e);
         }
-        return makeListOfAnnotations(data);
     }
 
     // 2.5.3 and above gets from annotation attribute otherwise self
-    private static ClassNode getSerializeClass(ClassNode alias) {
-        List<AnnotationNode> annotations = alias.getAnnotations(ClassHelper.make(AnnotationCollector.class));
-        if (!annotations.isEmpty()) {
-            AnnotationNode annotationNode = annotations.get(0);
-            Expression member = annotationNode.getMember("serializeClass");
-            if (member instanceof ClassExpression) {
-                ClassExpression ce = (ClassExpression) member;
-                if (!ce.getType().getName().equals(AnnotationCollector.class.getName())) {
-                    alias = ce.getType();
+    private static ClassNode getSerializeClass(final ClassNode alias) {
+        List<AnnotationNode> collectors = alias.getAnnotations(ClassHelper.make(AnnotationCollector.class));
+        if (!collectors.isEmpty()) {
+            assert collectors.size() == 1;
+            AnnotationNode collectorNode = collectors.get(0);
+            Expression serializeClass = collectorNode.getMember("serializeClass");
+            if (serializeClass instanceof ClassExpression) {
+                ClassNode serializeClassType = serializeClass.getType();
+                if (!serializeClassType.getName().equals(AnnotationCollector.class.getName())) {
+                    return serializeClassType;
                 }
             }
         }
         return alias;
     }
 
-    private static List<AnnotationNode> makeListOfAnnotations(Object[][] data) {
+    private static List<AnnotationNode> makeListOfAnnotations(final Object[][] data) {

Review Comment:
   *MixedMutabilityReturnType:*  This method returns both mutable and immutable collections or maps from different paths. This may be confusing for users of the method. [(details)](https://errorprone.info/bugpattern/MixedMutabilityReturnType)
   
   
   ```suggestion
       private static ImmutableList<AnnotationNode> makeListOfAnnotations(final Object[][] data) {
   ```
   
   
   
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
   
   ---
   
   Was this a good recommendation?
   [ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=192715095&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=192715095&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=192715095&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=192715095&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=192715095&lift_comment_rating=5) ]



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org