You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2018/12/26 20:20:04 UTC

[GitHub] asfgit closed pull request #843: GROOVY-7233: Configurable Access Modifier for Log AST Transformations

asfgit closed pull request #843: GROOVY-7233: Configurable Access Modifier for Log AST Transformations
URL: https://github.com/apache/groovy/pull/843
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/src/main/groovy/groovy/util/logging/Commons.java b/src/main/groovy/groovy/util/logging/Commons.java
index d4e5ad652c..6b145b4ba1 100644
--- a/src/main/groovy/groovy/util/logging/Commons.java
+++ b/src/main/groovy/groovy/util/logging/Commons.java
@@ -19,6 +19,7 @@
 package groovy.util.logging;
 
 import groovy.lang.GroovyClassLoader;
+import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.expr.ArgumentListExpression;
@@ -30,7 +31,6 @@
 import org.codehaus.groovy.ast.expr.TernaryExpression;
 import org.codehaus.groovy.transform.GroovyASTTransformationClass;
 import org.codehaus.groovy.transform.LogASTTransformation;
-import org.objectweb.asm.Opcodes;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -65,9 +65,15 @@
 public @interface Commons {
     String value() default "log";
     String category() default LogASTTransformation.DEFAULT_CATEGORY_NAME;
+
+    /**
+     * If specified, must match the "id" attribute in a VisibilityOptions annotation to enable a custom visibility.
+     */
+    String visibilityId() default Undefined.STRING;
+
     Class<? extends LogASTTransformation.LoggingStrategy> loggingStrategy() default CommonsLoggingStrategy.class;
 
-    public  static class CommonsLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategy {
+    public  static class CommonsLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategyV2 {
 
         private static final String LOGGER_NAME = "org.apache.commons.logging.Log";
         private static final String LOGGERFACTORY_NAME = "org.apache.commons.logging.LogFactory";
@@ -76,9 +82,10 @@ protected CommonsLoggingStrategy(final GroovyClassLoader loader) {
             super(loader);
         }
 
-        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName) {
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName, int fieldModifiers) {
             return classNode.addField(logFieldName,
-                    Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE,
+                    fieldModifiers,
                     classNode(LOGGER_NAME),
                     new MethodCallExpression(
                             new ClassExpression(classNode(LOGGERFACTORY_NAME)),
diff --git a/src/main/groovy/groovy/util/logging/Log.java b/src/main/groovy/groovy/util/logging/Log.java
index 1a4e554b7d..f99d2789f6 100644
--- a/src/main/groovy/groovy/util/logging/Log.java
+++ b/src/main/groovy/groovy/util/logging/Log.java
@@ -19,6 +19,7 @@
 package groovy.util.logging;
 
 import groovy.lang.GroovyClassLoader;
+import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
@@ -33,7 +34,6 @@
 import org.codehaus.groovy.transform.GroovyASTTransformationClass;
 import org.codehaus.groovy.transform.LogASTTransformation;
 import org.codehaus.groovy.transform.LogASTTransformation.LoggingStrategy;
-import org.objectweb.asm.Opcodes;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -70,12 +70,19 @@
 public @interface Log {
     String value() default "log";
     String category() default LogASTTransformation.DEFAULT_CATEGORY_NAME;
+
+    /**
+     * If specified, must match the "id" attribute in a VisibilityOptions annotation to enable a custom visibility.
+     * @since 3.0.0
+     */
+    String visibilityId() default Undefined.STRING;
+
     Class<? extends LoggingStrategy> loggingStrategy() default JavaUtilLoggingStrategy.class;
 
     /**
      * This class contains the logic of how to weave a Java Util Logging logger into the host class.
      */
-    public static class JavaUtilLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategy {
+    public static class JavaUtilLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategyV2 {
 
         private static final ClassNode LOGGER_CLASSNODE = ClassHelper.make(java.util.logging.Logger.class);
         private static final ClassNode LEVEL_CLASSNODE = ClassHelper.make(java.util.logging.Level.class);
@@ -84,9 +91,10 @@ protected JavaUtilLoggingStrategy(final GroovyClassLoader loader) {
             super(loader);
         }
 
-        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName) {
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName, int fieldModifiers) {
             return classNode.addField(logFieldName,
-                        Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE,
+                        fieldModifiers,
                         LOGGER_CLASSNODE,
                         new MethodCallExpression(
                                 new ClassExpression(LOGGER_CLASSNODE),
diff --git a/src/main/groovy/groovy/util/logging/Log4j.java b/src/main/groovy/groovy/util/logging/Log4j.java
index 28633b5536..1db7bf9b05 100644
--- a/src/main/groovy/groovy/util/logging/Log4j.java
+++ b/src/main/groovy/groovy/util/logging/Log4j.java
@@ -19,6 +19,7 @@
 package groovy.util.logging;
 
 import groovy.lang.GroovyClassLoader;
+import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.expr.ArgumentListExpression;
@@ -31,7 +32,6 @@
 import org.codehaus.groovy.ast.expr.TernaryExpression;
 import org.codehaus.groovy.transform.GroovyASTTransformationClass;
 import org.codehaus.groovy.transform.LogASTTransformation;
-import org.objectweb.asm.Opcodes;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -66,9 +66,16 @@
 public @interface Log4j {
     String value() default "log";
     String category() default LogASTTransformation.DEFAULT_CATEGORY_NAME;
+
+    /**
+     * If specified, must match the "id" attribute in a VisibilityOptions annotation to enable a custom visibility.
+     * @since 3.0.0
+     */
+    String visibilityId() default Undefined.STRING;
+
     Class<? extends LogASTTransformation.LoggingStrategy> loggingStrategy() default Log4jLoggingStrategy.class;
 
-    public static class Log4jLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategy {
+    public static class Log4jLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategyV2 {
         private static final String LOGGER_NAME = "org.apache.log4j.Logger";
         private static final String PRIORITY_NAME = "org.apache.log4j.Priority";
 
@@ -76,9 +83,10 @@ protected Log4jLoggingStrategy(final GroovyClassLoader loader) {
             super(loader);
         }
 
-        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName) {
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName, int fieldModifiers) {
             return classNode.addField(logFieldName,
-                    Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE,
+                    fieldModifiers,
                     classNode(LOGGER_NAME),
                     new MethodCallExpression(
                             new ClassExpression(classNode(LOGGER_NAME)),
diff --git a/src/main/groovy/groovy/util/logging/Log4j2.java b/src/main/groovy/groovy/util/logging/Log4j2.java
index a53874c6d1..d9a8a8a43a 100644
--- a/src/main/groovy/groovy/util/logging/Log4j2.java
+++ b/src/main/groovy/groovy/util/logging/Log4j2.java
@@ -19,6 +19,7 @@
 package groovy.util.logging;
 
 import groovy.lang.GroovyClassLoader;
+import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.expr.ArgumentListExpression;
@@ -30,7 +31,6 @@
 import org.codehaus.groovy.ast.expr.TernaryExpression;
 import org.codehaus.groovy.transform.GroovyASTTransformationClass;
 import org.codehaus.groovy.transform.LogASTTransformation;
-import org.objectweb.asm.Opcodes;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -65,9 +65,16 @@
 public @interface Log4j2 {
     String value() default "log";
     String category() default LogASTTransformation.DEFAULT_CATEGORY_NAME;
+
+    /**
+     * If specified, must match the "id" attribute in a VisibilityOptions annotation to enable a custom visibility.
+     * @since 3.0.0
+     */
+    String visibilityId() default Undefined.STRING;
+
     Class<? extends LogASTTransformation.LoggingStrategy> loggingStrategy() default Log4j2LoggingStrategy.class;
 
-    public static class Log4j2LoggingStrategy extends LogASTTransformation.AbstractLoggingStrategy {
+    public static class Log4j2LoggingStrategy extends LogASTTransformation.AbstractLoggingStrategyV2 {
         private static final String LOGGER_NAME = "org.apache.logging.log4j.core.Logger";
         private static final String LOG_MANAGER_NAME = "org.apache.logging.log4j.LogManager";
 
@@ -75,9 +82,10 @@ protected Log4j2LoggingStrategy(final GroovyClassLoader loader) {
             super(loader);
         }
 
-        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName) {
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName, int fieldModifiers) {
             return classNode.addField(logFieldName,
-                    Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE,
+                    fieldModifiers,
                     classNode(LOGGER_NAME),
                     new MethodCallExpression(
                             new ClassExpression(classNode(LOG_MANAGER_NAME)),
diff --git a/src/main/groovy/groovy/util/logging/Slf4j.java b/src/main/groovy/groovy/util/logging/Slf4j.java
index 7561cf735c..4f7da0743e 100644
--- a/src/main/groovy/groovy/util/logging/Slf4j.java
+++ b/src/main/groovy/groovy/util/logging/Slf4j.java
@@ -19,6 +19,7 @@
 package groovy.util.logging;
 
 import groovy.lang.GroovyClassLoader;
+import groovy.transform.Undefined;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.expr.ArgumentListExpression;
@@ -30,7 +31,6 @@
 import org.codehaus.groovy.ast.expr.TernaryExpression;
 import org.codehaus.groovy.transform.GroovyASTTransformationClass;
 import org.codehaus.groovy.transform.LogASTTransformation;
-import org.objectweb.asm.Opcodes;
 
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -65,9 +65,16 @@
 public @interface Slf4j {
     String value() default "log";
     String category() default LogASTTransformation.DEFAULT_CATEGORY_NAME;
+
+    /**
+     * If specified, must match the "id" attribute in a VisibilityOptions annotation to enable a custom visibility.
+     * @since 3.0.0
+     */
+    String visibilityId() default Undefined.STRING;
+
     Class<? extends LogASTTransformation.LoggingStrategy> loggingStrategy() default Slf4jLoggingStrategy.class;
 
-    public static class Slf4jLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategy {
+    public static class Slf4jLoggingStrategy extends LogASTTransformation.AbstractLoggingStrategyV2 {
         private static final String LOGGER_NAME = "org.slf4j.Logger";
         private static final String FACTORY_NAME = "org.slf4j.LoggerFactory";
 
@@ -75,9 +82,10 @@ protected Slf4jLoggingStrategy(final GroovyClassLoader loader) {
             super(loader);
         }
 
-        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName) {
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String logFieldName, String categoryName, int fieldModifiers) {
             return classNode.addField(logFieldName,
-                    Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE,
+                    fieldModifiers,
                     classNode(LOGGER_NAME),
                     new MethodCallExpression(
                             new ClassExpression(classNode(FACTORY_NAME)),
diff --git a/src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java
index 1895062645..c2505ffa33 100644
--- a/src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java
@@ -46,6 +46,10 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 
+import org.objectweb.asm.Opcodes;
+
+import static org.apache.groovy.ast.tools.VisibilityUtils.getVisibility;
+
 /**
  * This class provides an AST Transformation to add a log field to a class.
  */
@@ -58,6 +62,8 @@
      */
     public static final String DEFAULT_CATEGORY_NAME = "##default-category-name##";
 
+    public static final String DEFAULT_ACCESS_MODIFIER = "private";
+
     private CompilationUnit compilationUnit;
 
     @Override
@@ -74,6 +80,8 @@ public void visit(ASTNode[] nodes, final SourceUnit source) {
 
         final String categoryName = lookupCategoryName(logAnnotation);
 
+        final int logFieldModifiers = lookupLogFieldModifiers(targetClass, logAnnotation);
+
         if (!(targetClass instanceof ClassNode))
             throw new GroovyBugError("Class annotation " + logAnnotation.getClassNode().getName() + " annotated no Class, this must not happen.");
 
@@ -107,7 +115,13 @@ public void visitClass(ClassNode node) {
                 } else if (logField != null && !Modifier.isPrivate(logField.getModifiers())) {
                     addError("Class annotated with Log annotation cannot have log field declared because the field exists in the parent class: " + logField.getOwner().getName(), logField);
                 } else {
-                    logNode = loggingStrategy.addLoggerFieldToClass(node, logFieldName, categoryName);
+                    if (loggingStrategy instanceof LoggingStrategyV2) {
+                        LoggingStrategyV2 loggingStrategyV2 = (LoggingStrategyV2) loggingStrategy;
+                        logNode = loggingStrategyV2.addLoggerFieldToClass(node, logFieldName, categoryName, logFieldModifiers);
+                    } else {
+                        // support the old style but they won't be as configurable
+                        logNode = loggingStrategy.addLoggerFieldToClass(node, logFieldName, categoryName);
+                    }
                 }
                 super.visitClass(node);
             }
@@ -189,6 +203,11 @@ private static String lookupCategoryName(AnnotationNode logAnnotation) {
         return DEFAULT_CATEGORY_NAME;
     }
 
+    private int lookupLogFieldModifiers(AnnotatedNode targetClass, AnnotationNode logAnnotation) {
+        int modifiers = getVisibility(logAnnotation, targetClass, ClassNode.class, Opcodes.ACC_PRIVATE);
+        return Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT | Opcodes.ACC_STATIC | modifiers;
+    }
+
     private static LoggingStrategy createLoggingStrategy(AnnotationNode logAnnotation, GroovyClassLoader loader) {
 
         String annotationName = logAnnotation.getClassNode().getName();
@@ -214,10 +233,23 @@ private static LoggingStrategy createLoggingStrategy(AnnotationNode logAnnotatio
             throw new RuntimeException("Could not find default value of method named loggingStrategy on class named " + annotationName);
         }
 
-        if (!LoggingStrategy.class.isAssignableFrom((Class) defaultValue)) {
+        if (!LoggingStrategy.class.isAssignableFrom((Class) defaultValue)
+                && !LoggingStrategyV2.class.isAssignableFrom((Class) defaultValue)) {
             throw new RuntimeException("Default loggingStrategy value on class named " + annotationName + " is not a LoggingStrategy");
         }
 
+        // try V2 configurable logging strategy
+        try {
+            Class<? extends LoggingStrategyV2> strategyClass = (Class<? extends LoggingStrategyV2>) defaultValue;
+            if (AbstractLoggingStrategy.class.isAssignableFrom(strategyClass)) {
+                return DefaultGroovyMethods.newInstance(strategyClass, new Object[]{loader});
+            } else {
+                return strategyClass.newInstance();
+            }
+        } catch (Exception e) {
+        }
+
+        // try legacy logging strategy
         try {
             Class<? extends LoggingStrategy> strategyClass = (Class<? extends LoggingStrategy>) defaultValue;
             if (AbstractLoggingStrategy.class.isAssignableFrom(strategyClass)) {
@@ -228,6 +260,7 @@ private static LoggingStrategy createLoggingStrategy(AnnotationNode logAnnotatio
         } catch (Exception e) {
             return null;
         }
+
     }
 
 
@@ -255,6 +288,40 @@ private static LoggingStrategy createLoggingStrategy(AnnotationNode logAnnotatio
         Expression wrapLoggingMethodCall(Expression logVariable, String methodName, Expression originalExpression);
     }
 
+    /**
+     * A LoggingStrategy defines how to wire a new logger instance into an existing class.
+     * It is meant to be used with the @Log family of annotations to allow you to
+     * write your own Log annotation provider.
+     */
+    public interface LoggingStrategyV2 extends LoggingStrategy {
+        /**
+         * In this method, you are given a ClassNode, a field name and a category name, and you must add a new Field
+         * onto the class. Return the result of the ClassNode.addField operations.
+         *
+         * @param classNode      the class that was originally annotated with the Log transformation.
+         * @param fieldName      the name of the logger field
+         * @param categoryName   the name of the logging category
+         * @param fieldModifiers the modifiers (private, final, et. al.) of the logger field
+         * @return the FieldNode instance that was created and added to the class
+         */
+        FieldNode addLoggerFieldToClass(ClassNode classNode, String fieldName, String categoryName, int fieldModifiers);
+    }
+
+    public abstract static class AbstractLoggingStrategyV2 extends AbstractLoggingStrategy implements LoggingStrategyV2 {
+        protected AbstractLoggingStrategyV2(final GroovyClassLoader loader) {
+            super(loader);
+        }
+
+        protected AbstractLoggingStrategyV2() {
+            this(null);
+        }
+
+        @Override
+        public FieldNode addLoggerFieldToClass(ClassNode classNode, String fieldName, String categoryName) {
+            throw new UnsupportedOperationException("This logger requires a later version of Groovy");
+        }
+    }
+
     public abstract static class AbstractLoggingStrategy implements LoggingStrategy {
         protected final GroovyClassLoader loader;
 
diff --git a/src/test/groovy/util/logging/CommonsTest.groovy b/src/test/groovy/util/logging/CommonsTest.groovy
index b8d5acc8a0..643defd59d 100644
--- a/src/test/groovy/util/logging/CommonsTest.groovy
+++ b/src/test/groovy/util/logging/CommonsTest.groovy
@@ -56,6 +56,80 @@ class CommonsTest extends GroovyTestCase {
         }
     }
 
+    void testExplicitPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PRIVATE)
+            @groovy.util.logging.Commons
+            class MyClass {
+            }
+        ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPackagePrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PACKAGE_PRIVATE)
+            @groovy.util.logging.Commons
+            class MyClass {
+            }
+        ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    !Modifier.isPrivate(field.getModifiers()) &&
+                    !Modifier.isProtected(field.getModifiers()) &&
+                    !Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testProtectedFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PROTECTED)
+            @groovy.util.logging.Commons
+            class MyClass {
+            }
+        ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isProtected(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPublicFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PUBLIC)
+            @groovy.util.logging.Commons
+            class MyClass {
+            }
+        ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
     void testPrivateFinalStaticNamedLogFieldAppears() {
         Class clazz = new GroovyClassLoader().parseClass('''
               @groovy.util.logging.Commons('logger')
@@ -110,7 +184,7 @@ class CommonsTest extends GroovyTestCase {
             new MyClass().loggingMethod() ''')
 
         clazz.newInstance().run()
-        
+
         String log = redirectedSystemOut.toString()
         assert log.contains("error called")
         assert log.contains("warn called")
@@ -129,7 +203,7 @@ class CommonsTest extends GroovyTestCase {
             MyClass.loggingMethod()""")
 
         clazz.newInstance().run()
-        
+
         String log = redirectedSystemOut.toString()
         assert log.contains("(static) info called")
     }
@@ -149,7 +223,7 @@ class CommonsTest extends GroovyTestCase {
             new MyClass().loggingMethod() ''')
 
         clazz.newInstance().run()
-        
+
         String log = redirectedSystemOut.toString()
         assert log.contains("error called")
         assert log.contains("warn called")
diff --git a/src/test/groovy/util/logging/Log4j2Test.groovy b/src/test/groovy/util/logging/Log4j2Test.groovy
index 3d7b24c69f..0c09975702 100644
--- a/src/test/groovy/util/logging/Log4j2Test.groovy
+++ b/src/test/groovy/util/logging/Log4j2Test.groovy
@@ -38,7 +38,7 @@ class Log4j2Test extends GroovyTestCase {
         List<Map> events
         boolean isLogGuarded = true
 
-        Log4j2InterceptingAppender(String name, Filter filter, Layout<String> layout){
+        Log4j2InterceptingAppender(String name, Filter filter, Layout<String> layout) {
             super(name, filter, layout)
             this.events = new ArrayList<Map>()
         }
@@ -94,6 +94,91 @@ class Log4j2Test extends GroovyTestCase {
         }
     }
 
+    void testExplicitPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PRIVATE)
+            @groovy.util.logging.Log4j2
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPackagePrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PACKAGE_PRIVATE)
+            @groovy.util.logging.Log4j2
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    !Modifier.isPrivate(field.getModifiers()) &&
+                    !Modifier.isProtected(field.getModifiers()) &&
+                    !Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testProtectedFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PROTECTED)
+            @groovy.util.logging.Log4j2
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isProtected(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPublicFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PUBLIC)
+            @groovy.util.logging.Log4j2
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testUnknownAccessPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            @groovy.util.logging.Log4j2
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
     void testClassAlreadyHasLogField() {
         shouldFail(RuntimeException) {
             Class clazz = new GroovyClassLoader().parseClass('''
@@ -259,7 +344,7 @@ class Log4j2Test extends GroovyTestCase {
                       log.error("error called")
                     }
                 }""")
-        
+
         clazz.newInstance().loggingMethod()
 
         assert appenderForCustomCategory.getEvents().size() == 1
diff --git a/src/test/groovy/util/logging/Log4jTest.groovy b/src/test/groovy/util/logging/Log4jTest.groovy
index febc05fc69..3ed8c25b6e 100644
--- a/src/test/groovy/util/logging/Log4jTest.groovy
+++ b/src/test/groovy/util/logging/Log4jTest.groovy
@@ -62,6 +62,76 @@ class Log4jTest extends GroovyTestCase {
         }
     }
 
+    void testExplicitPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PRIVATE)
+            @groovy.util.logging.Log4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPackagePrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PACKAGE_PRIVATE)
+            @groovy.util.logging.Log4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    !Modifier.isPrivate(field.getModifiers()) &&
+                    !Modifier.isProtected(field.getModifiers()) &&
+                    !Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testProtectedFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PROTECTED)
+            @groovy.util.logging.Log4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isProtected(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPublicFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PUBLIC)
+            @groovy.util.logging.Log4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
     void testClassAlreadyHasLogField() {
 
         shouldFail {
diff --git a/src/test/groovy/util/logging/LogTest.groovy b/src/test/groovy/util/logging/LogTest.groovy
index a52788a811..57f7929e64 100644
--- a/src/test/groovy/util/logging/LogTest.groovy
+++ b/src/test/groovy/util/logging/LogTest.groovy
@@ -61,6 +61,76 @@ class LogTest extends GroovyTestCase {
         }
     }
 
+    void testExplicitPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PRIVATE)
+            @groovy.util.logging.Log
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPackagePrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PACKAGE_PRIVATE)
+            @groovy.util.logging.Log
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    !Modifier.isPrivate(field.getModifiers()) &&
+                    !Modifier.isProtected(field.getModifiers()) &&
+                    !Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testProtectedFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PROTECTED)
+            @groovy.util.logging.Log
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isProtected(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPublicFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PUBLIC)
+            @groovy.util.logging.Log
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
     void testClassAlreadyHasLogField() {
 
         shouldFail {
@@ -312,7 +382,8 @@ class LogTest extends GroovyTestCase {
     }
 }
 
-@groovy.transform.PackageScope class LoggerSpy extends Logger {
+@groovy.transform.PackageScope
+class LoggerSpy extends Logger {
 
     String severeParameter = null
     String warningParameter = null
diff --git a/src/test/groovy/util/logging/Slf4jTest.groovy b/src/test/groovy/util/logging/Slf4jTest.groovy
index 6a6324b888..52e113f183 100644
--- a/src/test/groovy/util/logging/Slf4jTest.groovy
+++ b/src/test/groovy/util/logging/Slf4jTest.groovy
@@ -74,6 +74,76 @@ class Slf4jTest extends GroovyTestCase {
         }
     }
 
+    void testExplicitPrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PRIVATE)
+            @groovy.util.logging.Slf4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPrivate(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPackagePrivateFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PACKAGE_PRIVATE)
+            @groovy.util.logging.Slf4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    !Modifier.isPrivate(field.getModifiers()) &&
+                    !Modifier.isProtected(field.getModifiers()) &&
+                    !Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testProtectedFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PROTECTED)
+            @groovy.util.logging.Slf4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isProtected(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
+    void testPublicFinalStaticLogFieldAppears() {
+        Class clazz = new GroovyClassLoader().parseClass('''
+            import static groovy.transform.options.Visibility.*
+            @groovy.transform.VisibilityOptions(value = PUBLIC)
+            @groovy.util.logging.Slf4j
+            class MyClass {
+            } ''')
+
+        assert clazz.declaredFields.find { Field field ->
+            field.name == "log" &&
+                    Modifier.isPublic(field.getModifiers()) &&
+                    Modifier.isStatic(field.getModifiers()) &&
+                    Modifier.isTransient(field.getModifiers()) &&
+                    Modifier.isFinal(field.getModifiers())
+        }
+    }
+
     void testPrivateFinalStaticNamedLogFieldAppears() {
         Class clazz = new GroovyClassLoader().parseClass('''
                 @groovy.util.logging.Slf4j('logger')


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services