You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by su...@apache.org on 2020/09/12 16:26:26 UTC
[groovy] 13/19: minor refactor
This is an automated email from the ASF dual-hosted git repository.
sunlan pushed a commit to branch GROOVY_3_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 33e1277b81ce8cdcaf2e87e792fc72435ea96b8e
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sat Aug 29 12:35:30 2020 -0500
minor refactor
(cherry picked from commit 3ae9083dbbee0d6a3bcbd5683668ac77501a9a49)
---
.../codehaus/groovy/reflection/CachedField.java | 64 ++++++++--------
.../groovy/reflection/ReflectionUtils.java | 86 ++++++++++-----------
src/test/groovy/IllegalAccessScenariosTest.groovy | 41 ----------
src/test/groovy/IllegalAccessTests.groovy | 88 ++++++++++++++++++++++
.../sc/FieldsAndPropertiesStaticCompileTest.groovy | 8 +-
5 files changed, 170 insertions(+), 117 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/reflection/CachedField.java b/src/main/java/org/codehaus/groovy/reflection/CachedField.java
index c2b5dd4..6c666aa 100644
--- a/src/main/java/org/codehaus/groovy/reflection/CachedField.java
+++ b/src/main/java/org/codehaus/groovy/reflection/CachedField.java
@@ -28,34 +28,44 @@ import java.lang.reflect.Modifier;
import static org.codehaus.groovy.reflection.ReflectionUtils.makeAccessibleInPrivilegedAction;
public class CachedField extends MetaProperty {
- private final Field cachedField;
+ private final Field field;
- public CachedField(Field field) {
- super (field.getName(), field.getType());
- this.cachedField = field;
+ public CachedField(final Field field) {
+ super(field.getName(), field.getType());
+ this.field = field;
}
- public boolean isStatic() {
- return Modifier.isStatic(getModifiers());
+ public Field getCachedField() {
+ makeAccessibleIfNecessary();
+ return field;
+ }
+
+ public Class getDeclaringClass() {
+ return field.getDeclaringClass();
+ }
+
+ @Override
+ public int getModifiers() {
+ return field.getModifiers();
}
public boolean isFinal() {
return Modifier.isFinal(getModifiers());
}
- public int getModifiers() {
- return cachedField.getModifiers();
+ public boolean isStatic() {
+ return Modifier.isStatic(getModifiers());
}
/**
* @return the property of the given object
* @throws RuntimeException if the property could not be evaluated
*/
+ @Override
public Object getProperty(final Object object) {
makeAccessibleIfNecessary();
- AccessPermissionChecker.checkAccessPermission(cachedField);
try {
- return cachedField.get(object);
+ return field.get(object);
} catch (IllegalAccessException e) {
throw new GroovyRuntimeException("Cannot get the property '" + name + "'.", e);
}
@@ -68,36 +78,26 @@ public class CachedField extends MetaProperty {
* @param newValue the new value of the property
* @throws RuntimeException if the property could not be set
*/
- public void setProperty(final Object object, Object newValue) {
- makeAccessibleIfNecessary();
- AccessPermissionChecker.checkAccessPermission(cachedField);
- final Object goalValue = DefaultTypeTransformation.castToType(newValue, cachedField.getType());
-
+ @Override
+ public void setProperty(final Object object, final Object newValue) {
if (isFinal()) {
throw new GroovyRuntimeException("Cannot set the property '" + name + "' because the backing field is final.");
}
+ makeAccessibleIfNecessary();
+ Object goalValue = DefaultTypeTransformation.castToType(newValue, field.getType());
try {
- cachedField.set(object, goalValue);
- } catch (IllegalAccessException ex) {
- throw new GroovyRuntimeException("Cannot set the property '" + name + "'.", ex);
+ field.set(object, goalValue);
+ } catch (IllegalAccessException e) {
+ throw new GroovyRuntimeException("Cannot set the property '" + name + "'.", e);
}
}
- public Class getDeclaringClass() {
- return cachedField.getDeclaringClass();
- }
-
- public Field getCachedField() {
- makeAccessibleIfNecessary();
- AccessPermissionChecker.checkAccessPermission(cachedField);
- return cachedField;
- }
-
- private boolean makeAccessibleDone = false;
+ private transient boolean madeAccessible;
private void makeAccessibleIfNecessary() {
- if (!makeAccessibleDone) {
- makeAccessibleInPrivilegedAction(cachedField);
- makeAccessibleDone = true;
+ if (!madeAccessible) {
+ makeAccessibleInPrivilegedAction(field);
+ madeAccessible = true;
}
+ AccessPermissionChecker.checkAccessPermission(field);
}
}
diff --git a/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java b/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java
index b9027d1..780e7a7 100644
--- a/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java
+++ b/src/main/java/org/codehaus/groovy/reflection/ReflectionUtils.java
@@ -42,32 +42,37 @@ import java.util.function.Function;
* current class to multiple levels of depth. Calls used to handle the
* groovy MOP are excluded from the level counting.
*/
+@SuppressWarnings("rawtypes")
public class ReflectionUtils {
- // these are packages in the call stack that are only part of the groovy MOP
- private static final Set<String> IGNORED_PACKAGES = new HashSet<String>();
private static final VMPlugin VM_PLUGIN = VMPluginFactory.getPlugin();
+ /** The packages in the call stack that are only part of the Groovy MOP. */
+ private static final Set<String> IGNORED_PACKAGES;
static {
- //IGNORED_PACKAGES.add("java.lang.reflect");
- IGNORED_PACKAGES.add("groovy.lang");
- IGNORED_PACKAGES.add("org.codehaus.groovy.reflection");
- IGNORED_PACKAGES.add("org.codehaus.groovy.runtime.callsite");
- IGNORED_PACKAGES.add("org.codehaus.groovy.runtime.metaclass");
- IGNORED_PACKAGES.add("org.codehaus.groovy.runtime");
- IGNORED_PACKAGES.add("sun.reflect");
- IGNORED_PACKAGES.add("java.security");
- IGNORED_PACKAGES.add("java.lang.invoke");
- IGNORED_PACKAGES.add("org.codehaus.groovy.vmplugin.v5");
- IGNORED_PACKAGES.add("org.codehaus.groovy.vmplugin.v6");
- IGNORED_PACKAGES.add("org.codehaus.groovy.vmplugin.v7");
- IGNORED_PACKAGES.add("org.codehaus.groovy.vmplugin.v8");
- IGNORED_PACKAGES.add("org.codehaus.groovy.vmplugin.v9");
+ Set<String> set = new HashSet<>();
+
+ set.add("groovy.lang");
+ set.add("sun.reflect");
+ set.add("java.security");
+ set.add("java.lang.invoke");
+ //set.add("java.lang.reflect");
+ set.add("org.codehaus.groovy.reflection");
+ set.add("org.codehaus.groovy.runtime");
+ set.add("org.codehaus.groovy.runtime.callsite");
+ set.add("org.codehaus.groovy.runtime.metaclass");
+ set.add("org.codehaus.groovy.vmplugin.v5");
+ set.add("org.codehaus.groovy.vmplugin.v6");
+ set.add("org.codehaus.groovy.vmplugin.v7");
+ set.add("org.codehaus.groovy.vmplugin.v8");
+ set.add("org.codehaus.groovy.vmplugin.v9");
+
+ IGNORED_PACKAGES = Collections.unmodifiableSet(set);
}
private static final ClassContextHelper HELPER = new ClassContextHelper();
/**
- * Determine whether or not the getCallingClass methods will return
+ * Determines whether or not the getCallingClass methods will return
* any sensible results. On JVMs that are not Sun derived i.e.
* (gcj, Harmony) this will likely return false. When not available
* all getCallingClass methods will return null.
@@ -80,7 +85,7 @@ public class ReflectionUtils {
}
/**
- * Get the immediate calling class, ignoring MOP frames.
+ * Gets the immediate calling class, ignoring MOP frames.
*
* @return The Class of the caller
*/
@@ -89,7 +94,7 @@ public class ReflectionUtils {
}
/**
- * Get the called that is matchLevel stack frames before the call,
+ * Gets the called that is matchLevel stack frames before the call,
* ignoring MOP frames.
*
* @param matchLevel how may call stacks down to look.
@@ -97,12 +102,12 @@ public class ReflectionUtils {
* @return The Class of the matched caller, or null if there aren't
* enough stackframes to satisfy matchLevel
*/
- public static Class getCallingClass(int matchLevel) {
+ public static Class getCallingClass(final int matchLevel) {
return getCallingClass(matchLevel, Collections.emptySet());
}
/**
- * Get the called that is matchLevel stack frames before the call,
+ * Gets the called that is matchLevel stack frames before the call,
* ignoring MOP frames and desired exclude packages.
*
* @param matchLevel how may call stacks down to look.
@@ -112,32 +117,31 @@ public class ReflectionUtils {
* @return The Class of the matched caller, or null if there aren't
* enough stackframes to satisfy matchLevel
*/
- public static Class getCallingClass(int matchLevel, Collection<String> extraIgnoredPackages) {
+ public static Class getCallingClass(final int matchLevel, final Collection<String> extraIgnoredPackages) {
Class[] classContext = HELPER.getClassContext();
-
- int depth = 0;
+ int depth = 0, level = matchLevel;
try {
Class c;
do {
do {
c = classContext[depth++];
} while (classShouldBeIgnored(c, extraIgnoredPackages));
- } while (c != null && matchLevel-- > 0 && depth<classContext.length);
+ } while (c != null && level-- > 0 && depth < classContext.length);
return c;
- } catch (Throwable t) {
+ } catch (Throwable ignore) {
return null;
}
}
- public static List<Method> getDeclaredMethods(Class<?> type, String name, Class<?>... parameterTypes) {
+ public static List<Method> getDeclaredMethods(final Class<?> type, final String name, final Class<?>... parameterTypes) {
return doGetMethods(type, name, parameterTypes, Class::getDeclaredMethods);
}
- public static List<Method> getMethods(Class<?> type, String name, Class<?>... parameterTypes) {
+ public static List<Method> getMethods(final Class<?> type, final String name, final Class<?>... parameterTypes) {
return doGetMethods(type, name, parameterTypes, Class::getMethods);
}
- private static List<Method> doGetMethods(Class<?> type, String name, Class<?>[] parameterTypes, Function<? super Class<?>, ? extends Method[]> f) {
+ private static List<Method> doGetMethods(final Class<?> type, final String name, final Class<?>[] parameterTypes, final Function<? super Class<?>, ? extends Method[]> f) {
List<Method> methodList = new LinkedList<>();
out:
@@ -151,7 +155,7 @@ public class ReflectionUtils {
continue;
}
- for (int i = 0, n = methodParameterTypes.length; i < n; i++) {
+ for (int i = 0, n = methodParameterTypes.length; i < n; i += 1) {
Class<?> parameterType = TypeUtil.autoboxType(parameterTypes[i]);
if (null == parameterType) {
continue out;
@@ -169,21 +173,20 @@ public class ReflectionUtils {
return methodList;
}
- public static boolean checkCanSetAccessible(AccessibleObject accessibleObject, Class<?> caller) {
+ public static boolean checkCanSetAccessible(final AccessibleObject accessibleObject, final Class<?> caller) {
return VM_PLUGIN.checkCanSetAccessible(accessibleObject, caller);
}
- public static boolean checkAccessible(Class<?> callerClass, Class<?> declaringClass, int memberModifiers, boolean allowIllegalAccess) {
+ public static boolean checkAccessible(final Class<?> callerClass, final Class<?> declaringClass, final int memberModifiers, final boolean allowIllegalAccess) {
return VM_PLUGIN.checkAccessible(callerClass, declaringClass, memberModifiers, allowIllegalAccess);
}
- public static boolean trySetAccessible(AccessibleObject ao) {
+ public static boolean trySetAccessible(final AccessibleObject ao) {
try {
return VM_PLUGIN.trySetAccessible(ao);
- } catch (Throwable t) {
+ } catch (Throwable ignore) {
// swallow for strict security managers, module systems, android or others
}
-
return false;
}
@@ -193,9 +196,8 @@ public class ReflectionUtils {
// to be run in PrivilegedAction!
public static Optional<AccessibleObject> makeAccessible(final AccessibleObject ao) {
- AccessibleObject[] result = makeAccessible(new AccessibleObject[] { ao });
-
- return Optional.ofNullable(0 == result.length ? null : result[0]);
+ AccessibleObject[] result = makeAccessible(new AccessibleObject[] {ao});
+ return Optional.ofNullable(result.length == 0 ? null : result[0]);
}
// to be run in PrivilegedAction!
@@ -203,10 +205,10 @@ public class ReflectionUtils {
try {
AccessibleObject.setAccessible(aoa, true);
return aoa;
- } catch (Throwable outer) {
+ } catch (Throwable ignore) {
// swallow for strict security managers, module systems, android or others,
// but try one-by-one to get the allowed ones at least
- final List<AccessibleObject> ret = new ArrayList<>(aoa.length);
+ List<AccessibleObject> ret = new ArrayList<>(aoa.length);
for (final AccessibleObject ao : aoa) {
boolean accessible = trySetAccessible(ao);
if (accessible) {
@@ -217,8 +219,8 @@ public class ReflectionUtils {
}
}
- private static boolean classShouldBeIgnored(Class c, Collection<String> extraIgnoredPackages) {
- return ((c != null)
+ private static boolean classShouldBeIgnored(final Class c, final Collection<String> extraIgnoredPackages) {
+ return (c != null
&& (c.isSynthetic()
|| (c.getPackage() != null
&& (IGNORED_PACKAGES.contains(c.getPackage().getName())
diff --git a/src/test/groovy/IllegalAccessScenariosTest.groovy b/src/test/groovy/IllegalAccessScenariosTest.groovy
deleted file mode 100644
index cc1db54..0000000
--- a/src/test/groovy/IllegalAccessScenariosTest.groovy
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package groovy
-
-import groovy.test.GroovyTestCase
-
-import static groovy.test.GroovyAssert.isAtLeastJdk
-import static org.apache.groovy.util.SystemUtil.getBooleanSafe
-
-/**
- * Tests for permissive member access. Typically such access is only allowed in Java via means such
- * as reflection.
- *
- * In JDK versions < 9, Groovy supports permissive access and no warnings are given by the JDK.
- * In JDK versions >= 9, Groovy supports permissive access but the JDK gives illegal access warnings.
- * At some point, the JDK may further restrict permissive access and Groovy's support for this feature may be limited.
- */
-class IllegalAccessScenariosTest extends GroovyTestCase {
- void testPrivateFieldAccess() {
- if (isAtLeastJdk('9.0') && !getBooleanSafe('groovy.force.illegal.access')) return
- def items = [1, 2, 3]
- // size is a private field in ArrayList
- assert items.size == 3
- }
-}
diff --git a/src/test/groovy/IllegalAccessTests.groovy b/src/test/groovy/IllegalAccessTests.groovy
new file mode 100644
index 0000000..f0d1192
--- /dev/null
+++ b/src/test/groovy/IllegalAccessTests.groovy
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy
+
+import org.junit.Before
+import org.junit.Test
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.isAtLeastJdk
+import static org.junit.Assume.assumeTrue
+
+/**
+ * Tests for permissive member access. Typically such access is only allowed in
+ * Java via means such as reflection.
+ *
+ * In JDK versions < 9, Groovy supports permissive access and no warnings are given by the JDK.
+ * In JDK versions >= 9, Groovy supports permissive access but the JDK gives illegal access warnings.
+ * At some point, the JDK may further restrict permissive access and Groovy's support for this feature may be limited.
+ */
+final class IllegalAccessTests {
+
+ @Before
+ void setUp() {
+ assumeTrue(isAtLeastJdk('9.0') && !Boolean.getBoolean('groovy.force.illegal.access'))
+ }
+
+ @Test
+ void testReadPrivateField() {
+ assertScript '''
+ def items = [1, 2, 3]
+ assert items.size == 3 // "size" is private
+ '''
+ }
+
+ @Test
+ void testReadPackageProtectedField() {
+ // TODO: move A to another package
+ assertScript '''
+ class A {
+ @groovy.transform.PackageScope int i
+ }
+ class B extends A {
+ def eye() { super.i }
+ }
+ assert new B().eye() == 0
+ '''
+ }
+
+ @Test // GROOVY-9596
+ void testReadProtectedFieldFromSuperClass() {
+ // in is a protected field in FilterReader
+ assertScript '''
+ class MyFilterReader extends FilterReader {
+ MyFilterReader(Reader reader) {
+ super(new BufferedReader(reader))
+ }
+ String nextLine() {
+ ((BufferedReader) this.in).readLine()?.trim() // "in" is protected
+ }
+ }
+
+ def input =
+ " works \\t\\n" +
+ "hello there \\n" +
+ "hi\\n" +
+ "\\n"
+ def reader = new CharArrayReader(input.toCharArray())
+ reader = new MyFilterReader(reader)
+ assert reader.nextLine() == 'works'
+ '''
+ }
+}
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
index 3150e2b..81c5c83 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
@@ -305,7 +305,8 @@ final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCT
b.m()
assert b.isGetterCalled() == false
'''
- assert astTrees['B'][1].contains('GETFIELD A.x')
+ def b = astTrees['B'][1]
+ assert b.contains('GETFIELD A.x')
}
void testUseGetterFieldAccess() {
@@ -328,7 +329,10 @@ final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCT
b.usingGetter()
assert b.isGetterCalled() == true
'''
- assert astTrees['B'][1].contains('INVOKEVIRTUAL B.getX')
+ def b = astTrees['B'][1]
+ assert !b.contains('GETFIELD A.x')
+ assert !b.contains('GETFIELD B.x')
+ assert b.contains('INVOKEVIRTUAL B.getX')
}
void testUseAttributeExternal() {