Java 16 Support

[veltrup]

A call to the constructor throws an exception with java 16.

new com.rits.cloning.Cloner()

java.lang.reflect.InaccessibleObjectException: Unable to make field private transient java.util.NavigableMap java.util.TreeSet.m accessible: module java.base does not "opens java.util" to unnamed module @511baa65
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at com.rits.cloning.Cloner.addAll(Cloner.java:530)
	at com.rits.cloning.Cloner.allFields(Cloner.java:544)
	at com.rits.cloning.Cloner.registerStaticFields(Cloner.java:180)
	at com.rits.cloning.Cloner.registerKnownConstants(Cloner.java:166)
	at com.rits.cloning.Cloner.init(Cloner.java:83)
	at com.rits.cloning.Cloner.<init>(Cloner.java:54)
        ...

I think the reason is the JEP 396

A Workarround is to use the launcher option --illegal-access=permit

[tweimer]

@veltrup I fixed the issue with FastClonerTreeSet in #96. Anyway, you should use --illegal-access=permit for cloning other objects.

[chadlwilson]

There seem also to be issues with statics and nullable transients due to

cloning/src/main/java/com/rits/cloning/Cloner.java

Lines 580 to 592 in 6cea78f

	for (final Field f : fs) {
	if (!f.isAccessible()) {
	f.setAccessible(true);
	}
	int modifiers = f.getModifiers();
	if (!Modifier.isStatic(modifiers)) {
	if (!(nullTransient && Modifier.isTransient(modifiers)) && !isFieldNullInsteadBecauseOfAnnotation(f)) {
	l.add(f);
	boolean shouldClone = (cloneSynthetics || !f.isSynthetic()) && (cloneAnonymousParent || !isAnonymousParent(f));
	shouldCloneList.add(shouldClone);
	}
	}
	}

I wonder if this code needs to setAccessible for these fields too even if they don't end up in the fields list for cloning? Maybe that'd just move the issues elsewhere though.

[tweimer]

@chadlwilson Looks good to me.

@kostaskougios What do you think?

[kostaskougios]

@chadlwilson , @tweimer
Which classes would be affected, can we have a small example?

Is it just a class like class A { public static int x ; } ?

[chadlwilson]

Yeah, that should do it. In the specific case I came across in the wild it was a class extending ArrayList (without a custom fastcloner) and dying on the static final serialversionUID.

I can create a proper reprod later, however it was more curious to me as (without knowing all the details of how this works) I thought it ignored statics so it was a bit confusing that it was failing on a static field.

Of course whether libraries like this have a chance on Java 16 without allowing illegal access or --add-opens all over the place is still something I am trying to figure out.

[kostaskougios]

@chadlwilson I've created a test case but it passes:

public class StaticTransient implements Serializable {
    private static final long serialVersionUID = 1234567L;
}

...

    public void testStaticTransient() {
        cloner.deepClone(new StaticTransient());
    }

Any ideas on how to make it fail?

[chadlwilson]

@kostaskougios Ahh, okay - that won't fail because the static is defined within a class belonging to something not defined as a module per se; and likely "open" to the calling code. However, if you debug the code, you will see it is still (potentially unnecessarily) calling setAccessible on the serialVersionUID field which would trigger a checkCanSetAccessible.

So I guess the challenge here is when the static is defined by a JDK class in a JDK module (since these are now closed by default), or to other types where the library defines module boundaries to be enforced by the JVM.

So the simplest reprod, similar to the original case I found, would be

public static class StaticTransient extends ArrayList<String> { }

@Test
public void testStaticTransient() {
    new Cloner().deepClone(new StaticTransient());
}

...yielding

java.lang.reflect.InaccessibleObjectException: Unable to make field private static final long java.util.ArrayList.serialVersionUID accessible: module java.base does not "opens java.util" to unnamed module @7382f612

	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at com.rits.cloning.Cloner$CloneObjectCloner.<init>(Cloner.java:590)
	at com.rits.cloning.Cloner.findDeepCloner(Cloner.java:485)
	at com.rits.cloning.Cloner.cloneInternal(Cloner.java:449)
	at com.rits.cloning.Cloner.deepClone(Cloner.java:348)

Alternatively, define StaticTransient in its own module with a module-info.java that is not open to the module where the test code is run.

[kostaskougios]

@chadlwilson ok, this code fails of --illegal-access=permit is not defined, but succeeds if it is defined:

    public static class StaticTransient extends ArrayList<String> {};

    public void testStaticTransient() {
        cloner.deepClone(new StaticTransient());
    }

can't you define --illegal-access=permit ?

[tweimer]

@kostaskougios We use that flag, but this still causes a warning. Can't you move the {{setAccessible}} flag inside that if statement?

[kostaskougios]

Ok after a few attempts with @tweimer , it seems the setAccessible is required.
The code now doesn't call it for statics but still the warning will appear in the first non-static field i.e. ArrayList.elementData

[chadlwilson]

Thanks @kostaskougios - that should at least keep the focus for users on the pieces/internals they shouldn't be relying upon access to for deep cloning, so they can register their own fastcloner.

[chadlwilson]

Hi @tweimer and @kostaskougios - could we please get a new 1.11.0 release pushed to Maven with this fix (and others) in it? Seems there hasn't been a Maven release since 1.10.3 in March 2020 it seems.

Looks like the diff would be this.

In Java 17 it is necessary to specifically open each module required for such reflective access with --add-opens=java.base/java.util=ALL-UNNAMED etc as the JVM-wide --illegal-access=permit has been dropped from the JVM.

This fix to avoid touching statics would help reduce the "noise" from use of the cloner so users can focus on only opening the necessary packages for the non-static fields requiring cloning, or potentially adding their own fast cloners which don't require the reflective access.

If there is any help required to cut a release, I can possibly pitch in.

[kostaskougios]

Sorry @chadlwilson , I tried a week ago to deploy but there was a problem with my gpg, I created one and pushed it to a gpg registry but it didn't fix the issue. Effectively I am locked out of maven central. There must be a way to gain access again but nowdays I don't have time to maintain the lib anymore or work on this.

[chadlwilson]

Thanks for the reply @kostaskougios . Appreciate the effort - and sorry to hear that.

I understand if you don't have the time/energy, however there is some stuff at https://central.sonatype.org/publish/requirements/gpg/#dealing-with-expired-keys which might be helpful if you hadn't seen it, and want to have another go at it. My random guess is potentially an issue with primary key vs subkey signing (seems they only support primary key signing) or assuming you can still authenticate with Maven Central, perhaps the new key not being adequately linked to your account somehow.

If not possible to unblock this, I wonder if @tweimer is interested in taking over or forking somehow. 🤔

[tweimer]

I don't have any plans to take this project over.

[vogella]

Looks like an update is available under a different name #117 (comment)