jcodemodel icon indicating copy to clipboard operation
jcodemodel copied to clipboard

[feature] copying JCodeModel

Open glelouet opened this issue 5 years ago • 34 comments

I propose to add a new method in JCodeModel : copy(). This method returns a new CopiedModel that contains recursive copy of all the elements of the parent model. This CopiedModel also allows to translate the elements of the parent model into its own elements.

This method could be used with the preprocessors : instead of working on the original model, processors would be applied on the copy. If no processor is used, then no copy is done.

Working on it.

glelouet avatar Jan 22 '21 19:01 glelouet

That means a deep-copy of all objects and expressions... That's going to be a big one

phax avatar Jan 22 '21 19:01 phax

yes.

glelouet avatar Jan 22 '21 20:01 glelouet

Okay, than can we have quick chat on how to do it? I don't like Object.clone because you always need to cast. Can you do something with copy constructors? Or do your prefer a custom clone like in https://github.com/phax/ph-commons/blob/master/ph-commons/src/main/java/com/helger/commons/lang/ICloneable.java ?

phax avatar Jan 22 '21 20:01 phax

I'll make a branch.

I was more asking if there is a visible impossibility for this, as to avoid doing "a big one" for nothing.

The method in JCodemodel is just copy(), the ModelCopy extends JCodemodel and add various translate(..) to get the translation.

glelouet avatar Jan 22 '21 20:01 glelouet

One thing that strikes me, is that the basic types (like VOID or BOOLEAN) depend on the JCodeModel instance and all usages of those must also be transfered. That means that the new CM must be passed through quite deep

phax avatar Jan 22 '21 20:01 phax

The issue I have, is that some constructor eg Jatom() are protected.

glelouet avatar Jan 22 '21 20:01 glelouet

Just make them public. Or create factory methods

phax avatar Jan 22 '21 20:01 phax

NAh I put my ModelCopy in the same package.

The less modifications, the better.

glelouet avatar Jan 22 '21 20:01 glelouet

https://github.com/guiguilechat/jcodemodel/blob/copying/src/main/java/com/helger/jcodemodel/ModelCopy.java

glelouet avatar Jan 22 '21 20:01 glelouet

I though you add a copy-constructor or clone method in every class? Do I get you right - you manually copy the structure from the "outside" of the relevant class?

phax avatar Jan 22 '21 20:01 phax

yes, I just add one class that makes the copy in itself.

glelouet avatar Jan 22 '21 20:01 glelouet

Ah okay. But isnt't there a certain danger that you forget stuff?

phax avatar Jan 22 '21 20:01 phax

indeed.

glelouet avatar Jan 22 '21 20:01 glelouet

Than maybe for the expression stuff could you go for a solution like this: https://github.com/phax/jcodemodel/commit/ead1645d882ff301a530480c26e1e6d7b2c1348d

phax avatar Jan 22 '21 20:01 phax

No, because we need to translate all references .

glelouet avatar Jan 22 '21 20:01 glelouet

Okay, and what if IJObject gets a new method getClone (JCodeModel new) ?

phax avatar Jan 22 '21 20:01 phax

They must be able to translate their internal fields too. So they need to be getClone(ModelCopy). Also then their children may inherit the method and forget to duplicate their added field. Maybe I should use reflect.

glelouet avatar Jan 22 '21 21:01 glelouet

Reflection makes it even more difficult, especially if some byte code modifying frameworks like ByteBuddy or the Spring stuff is used, you might get weird errors. I would really prefer a clean, explicit way. Ideally with the respective code in each class separately. There are not too many derivations.

What could be an issue is the order of cloning of classes - because the references between created classes can only be copied properly, if the order is maintained. Maybe a separate "creationOrder" field is needed for classes?

phax avatar Jan 22 '21 21:01 phax

anyhow reflect can't be used since there is no empty constructor.

Not sure what you mean about issue of cloning classes. I cache the classes, when the class is missing I create it, cache it, then start building it. So there should not be a recursion issue.

I try to avoid making weird stuff in other classes, like creating code that is not necessarily relevant. If later the cloning costs too much dev time, we can just deprecate it ? I try to throw exceptions when I can to tell that a case is not caught.

glelouet avatar Jan 22 '21 21:01 glelouet

Okay - that sounds like a fair compromise. I like throwing exceptions :)

phax avatar Jan 22 '21 21:01 phax

I had to add

public Class <?> getReferencedClass ()
	{
		return m_aClass;
	}

in JreferencedClass, it had no public access.

glelouet avatar Jan 22 '21 21:01 glelouet

Such getters are always great. This is one of the big differences of this project to the original codemodel. If you find more of those - just add them.

phax avatar Jan 22 '21 21:01 phax

I did part of the translation of types. Still a lot to do ^^ https://github.com/guiguilechat/jcodemodel/blob/copying/src/main/java/com/helger/jcodemodel/ModelCopy.java

I use instanceof when I know the delegated method makes a hard check on the sub types, getClass()== when checking for a specific type to generate. This way if classes are added exception will be thrown.

glelouet avatar Jan 22 '21 22:01 glelouet

Another issue I got along is the caching of elements. I started by only caching the classes, for a deep copying recursive issue (what if a field refer to the class being copied ?) . But then I realized another issue.

Basically we want two things for the copying from a SRC model to a CP model:

  1. once copied, no modification applied to SRC will impact CP, and reciprocally. eg renaming a variable, a method, deleting, creating. NO change at all.
  2. once copied, any modification applied to CP will have the same exact result as when applied to SRC, and reciprocally.

The first part implies to copy everything, even the JAtom. The second part implies that the model"tree" is exactly the same, so if a node is present at two places, then its copy must also be present at two places. The only way to do that is to cache the nodes being copied. Since there will be deep copy that can produce recursion issue, the caching of a an IJObject copy must be done in a very strict way, that is always create a shallow copy, before storing it, then deep copy it so that the deep copy can reference it. It's a potential issue for items that have a constructor that needs a reference to another IJObject though.

glelouet avatar Feb 17 '21 09:02 glelouet

basically the translate(parametertype source) code is following :

  • If the parameter type is interface, make a series of instanceof to delegate to a more precise subtype. end with throw exception in case new subtype was added.
  • If the parameter type is abstract, same as interface
  • If the parameter type is concrete class, then still try to delegate to subclass if any, then check for copy presence to return, if absent check same exact class, create a shallow copy, store the copy, make deep copying, return the copy. deep copy is made in separated methods because that can be shared for different part of code.

Then the "tree" of the method starts with interface IJObject, and create the whole interface subclasses translates (therefore only instanceof ) . Then another part of code is for each abstract class, becaue abstract classes belong to a tree (no diamond like interfaces) then I can create a tree from the highest non interface class that implement IJObject.

glelouet avatar Feb 17 '21 10:02 glelouet

https://github.com/guiguilechat/jcodemodel/commit/3851843c9b20fe68c7da8c6f094ac4d10496cfa8

Here I show how it is with the caching.

What remains is … well every todo ^^

glelouet avatar Feb 17 '21 10:02 glelouet

yes, to build the same structure you somehow need a state that know about all objects and if they are already cloned or not. So I think coming back to a clone (CloneState) method per object is the safest way to go. Or make everything Serializable - than write to a byte array and back to an Object - than you also get different instances...

phax avatar Feb 17 '21 10:02 phax

cloneState is the ModelCopy class.

I think you are right, that Serializable would be faster, more bug proof. But I'm not sure how it would handle the cases of static instances. Also I still need to have the translate() method, that translates an item from the source model into the copy model.

Also : In order to test the copy() method, I need to have a way to convert the model into a string (or at least a comparable item) . Do you think there is already such a method I could use ?

Typically the test would be (pseudo code)

cm=createCM();
copy = cm.copy();
assertEquals(cm.representString(), copy.representString());
for(Consumer<Model> m : modifications) {
  m.accept(cm);
  assertNotEquals(cm.representString, copy.representString);
}
for(Consumer<Model> m : modifications) {
 m.accept(copy);
}
assertEquals(cm.representString, copy.representString);

glelouet avatar Feb 17 '21 11:02 glelouet

java code for the test.

The interesting part is that it can be subclassed to test various implementations of copy and represent. Obviously it will fail with the toString() and return source; implementations ^^


public class ModelCopyTest
{

	@Test
	public void testCopy ()
	{
		JCodeModel cm = createCM ();
		JCodeModel copy = copy (cm);
		Assert.assertEquals (represent (cm), represent (copy));
		for (Consumer <JCodeModel> m : modifications ())
		{
			m.accept (cm);
			Assert.assertNotEquals (represent (cm), represent (copy));
		}
		for (Consumer <JCodeModel> m : modifications ())
			m.accept (copy);
		Assert.assertEquals (represent (cm), represent (copy));
	}

	JCodeModel createCM ()
	{
		JCodeModel ret = new JCodeModel ();
		return ret;
	}

	JCodeModel copy (JCodeModel source)
	{
		return source;
	}

	String represent (JCodeModel target)
	{
		return target.toString ();
	}

	public Collection <Consumer <JCodeModel>> modifications ()
	{
		return Arrays.asList (cm ->
		{
			try
			{
				cm._class (JMod.PUBLIC, "MyClass");
			}
			catch (JCodeModelException e)
			{
				throw new UnsupportedOperationException ("catch this", e);
			}
		});
	}

}

glelouet avatar Feb 17 '21 11:02 glelouet

In last patch I made that method abstract, and made a simple implementation with serialization to copy ; and a new StringCodeWriter extends AbstractCodeWriter for which toString() return the whole content of the files created from a JCMWriter.build(), appended with the name of the file to start.

When I try to make a more complex model, I get this error : Caused by: java.io.NotSerializableException: com.helger.jcodemodel.util.FSName

glelouet avatar Feb 17 '21 13:02 glelouet