From 77e59955e68ec56aad82dc815772e01ef363e0d8 Mon Sep 17 00:00:00 2001 From: Johannes Link Date: Sat, 11 Nov 2023 18:54:38 +0100 Subject: [PATCH] Before Try lifecycle is now using test instances for accessing fields --- TODO.md | 7 +- .../hooks/lifecycle/BeforeTryMembersHook.java | 10 +-- .../support/JqwikReflectionSupport.java | 67 +++++++++---------- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/TODO.md b/TODO.md index 11093490b..6a96c02b3 100644 --- a/TODO.md +++ b/TODO.md @@ -1,14 +1,13 @@ # 1.8.2 - - Make run with Java 18+. - See https://github.com/jqwik-team/jqwik/issues/528. - This may require a change to how methods are invoked via reflection. - See JqwikReflectionSupport.invokeMethodPotentiallyOuter() will probably need to accept the current property context as parameter. + - Use sourceTarget in GH action matrix - Can large array shrinking be improved? See https://github.com/jqwik-team/jqwik/issues/525 and https://github.com/jqwik-team/jqwik/issues/526. + - Summon preconfigured arbitrary. See https://github.com/jqwik-team/jqwik/issues/527 + - Update samples repo - Update jqwik-spring, jqwik-micronaut, jqwik-testcontainers diff --git a/engine/src/main/java/net/jqwik/engine/hooks/lifecycle/BeforeTryMembersHook.java b/engine/src/main/java/net/jqwik/engine/hooks/lifecycle/BeforeTryMembersHook.java index 0df415c21..fda7e486b 100644 --- a/engine/src/main/java/net/jqwik/engine/hooks/lifecycle/BeforeTryMembersHook.java +++ b/engine/src/main/java/net/jqwik/engine/hooks/lifecycle/BeforeTryMembersHook.java @@ -27,25 +27,25 @@ private void beforeTry(TryLifecycleContext context) { } private void initializeFields(List fields, TryLifecycleContext context) { - Object testInstance = context.testInstance(); + List testInstances = context.testInstances(); ThrowableCollector throwableCollector = new ThrowableCollector(ignore -> false); for (Field field : fields) { if (JqwikReflectionSupport.isStatic(field)) { String message = String.format("Static field <%s> must not be annotated with @BeforeTry.", field); throw new JqwikException(message); } - throwableCollector.execute(() -> initializeField(field, testInstance)); + throwableCollector.execute(() -> initializeField(field, testInstances)); } throwableCollector.assertEmpty(); } - private void initializeField(Field field, Object target) { + private void initializeField(Field field, List testInstances) { Store initialFieldValue = Store.getOrCreate( Tuple.of(BeforeTryMembersHook.class, field), Lifespan.PROPERTY, - () -> JqwikReflectionSupport.readFieldPotentiallyOuter(field, target) + () -> JqwikReflectionSupport.readFieldOnContainer(field, testInstances) ); - JqwikReflectionSupport.setFieldPotentiallyOuter(field, initialFieldValue.get(), target); + JqwikReflectionSupport.setFieldOnContainer(field, initialFieldValue.get(), testInstances); } @Override diff --git a/engine/src/main/java/net/jqwik/engine/support/JqwikReflectionSupport.java b/engine/src/main/java/net/jqwik/engine/support/JqwikReflectionSupport.java index 500e9c8ac..cbc767f87 100644 --- a/engine/src/main/java/net/jqwik/engine/support/JqwikReflectionSupport.java +++ b/engine/src/main/java/net/jqwik/engine/support/JqwikReflectionSupport.java @@ -23,28 +23,6 @@ public class JqwikReflectionSupport { private static final Logger LOG = Logger.getLogger(JqwikReflectionSupport.class.getName()); - private static Optional getOuterInstance(Object inner) { - // This is risky since it depends on the name of the field which is nowhere guaranteed - // but has been stable so far in all JDKs. - // Moreover, since Java 18 this no longer works for inner classes that do not reference their outer class somewhere, - // eg through OuterClass.this, because the field has been optimized away :-(. - - return Arrays - .stream(inner.getClass().getDeclaredFields()) - .filter(field -> field.getName().startsWith("this$")) - .findFirst() - .map(field -> { - try { - return makeAccessible(field).get(inner); - } catch (SecurityException | IllegalArgumentException | IllegalAccessException ex) { - String message = String.format("Could not access outer instance of %s." + - "%nReason: %s", inner, ex); - LOG.warning(message); - return Optional.empty(); - } - }); - } - @SuppressWarnings("deprecation") // Deprecated as of Java 9 private static T makeAccessible(T object) { if (!object.isAccessible()) { @@ -164,9 +142,20 @@ public static List findFieldsPotentiallyOuter( return foundFields; } - // TODO: Require all instances to be passed in instead of just the most inner one - public static Object readFieldPotentiallyOuter(Field field, Object target) { + /** + * Read a field's value as in ReflectionSupport.getField(..) but potentially use outer instances if the field belongs to an inner class. + * + * @param field The field to read + * @param targetInstances The container instances to read the field from, from outermost to innermost + * @return The value of the field + */ + public static Object readFieldOnContainer(Field field, List targetInstances) { makeAccessible(field); + return readField(field, new ArrayDeque<>(targetInstances)); + } + + private static Object readField(Field field, Deque instances) { + Object target = instances.removeLast(); List declaredFields = Arrays.stream(target.getClass().getDeclaredFields()).collect(toList()); if (declaredFields.contains(field)) { try { @@ -175,19 +164,28 @@ public static Object readFieldPotentiallyOuter(Field field, Object target) { return JqwikExceptionSupport.throwAsUncheckedException(exception); } } else { - Optional outer = getOuterInstance(target); - if (outer.isPresent()) { - return readFieldPotentiallyOuter(field, outer.get()); - } else { + if (instances.isEmpty()) { String message = String.format("Cannot access value of field %s", field); throw new JqwikException(message); } + return readField(field, instances); } } - // TODO: Require all instances to be passed in instead of just the most inner one - public static void setFieldPotentiallyOuter(Field field, Object value, Object target) { + /** + * Set a field's value as in ReflectionSupport.setField(..) but potentially use outer instances if the field belongs to an inner class. + * + * @param field The field to set + * @param value The value to set in the field + * @param targetInstances The container instances to set the field on, from outermost to innermost + */ + public static void setFieldOnContainer(Field field, Object value, List targetInstances) { makeAccessible(field); + setField(field, value, new ArrayDeque(targetInstances)); + } + + private static void setField(Field field, Object value, Deque instances) { + Object target = instances.removeLast(); List declaredFields = Arrays.stream(target.getClass().getDeclaredFields()).collect(toList()); if (declaredFields.contains(field)) { try { @@ -201,13 +199,11 @@ public static void setFieldPotentiallyOuter(Field field, Object value, Object ta JqwikExceptionSupport.throwAsUncheckedException(exception); } } else { - Optional outer = getOuterInstance(target); - if (outer.isPresent()) { - setFieldPotentiallyOuter(field, value, outer.get()); - } else { + if (instances.isEmpty()) { String message = String.format("Cannot set value of field %s", field); throw new JqwikException(message); } + setField(field, value, instances); } } @@ -244,7 +240,8 @@ private static Object invokeMethod(Method method, Deque instances, Objec return ReflectionSupport.invokeMethod(method, target, args); } else { if (instances.isEmpty()) { - throw new IllegalArgumentException(String.format("Method [%s] cannot be invoked on target [%s].", method, target)); + String message = String.format("Method [%s] cannot be invoked on target [%s].", method, target); + throw new IllegalArgumentException(message); } return invokeMethod(method, instances, args); }