CtExecutableReference in cloned method should refer to the clone, not to the method being cloned. #1869
Comments
|
I have two concerns with this. First, there is no clearly better semantics. One can object that the cloned should call the same methods as before. Both semantics can be confusing depending on one's assumption. Second, it is impossible, since reference resolving is name-based and the method name after cloning is the same, so it would resolve to the same declaration. That's normal. One solution would be that in the |
|
the issue title says For |
|
It just produces wrong AST, later transformations with it are going to break. It's not about calling (or otherwise referencing, in Spoon's AST model) other methods, it's about referencing itself. It's not only the problem with |
|
Also I think if This issue might be solved generically in A-la identity tracking in the Java's built-in serialization mechanism. |
monperrus
changed the title
It seems that there is no problem with parameter references, they refer well to the clone, as verified from your example per methodClone.accept(new CtScanner() {
@Override
public <T> void visitCtParameterReference(CtParameterReference<T> reference) {
assertSame(methodClone, reference.getDeclaration().getParent(CtMethod.class));
super.visitCtParameterReference(reference);
}
});Changed the title of the issue to refer to CtExecutableReference instead. |
|
So what you are saying that that cloning should comply with the following contract: I agree, this generic contract could probably be enforced in Is that what you mean? |
|
@monperrus yes, if I understand it right. And it's not about just one cloned object x, as in the given example of |
|
The problem if we tailor the cloning code like this is that we break the contract See failing tests in https://travis-ci.org/monperrus/spoon/jobs/345684253 of branch master...monperrus:refactor-clone-relative-3 |
|
Equality of arbitrary ASTs is a tricky question. I understand equality of constructs (expressions, primarily) within an execution block, that could be used for implementing optimizations such as CSE. The change in the clone() behaviour won't affect this, because we never do CSE optimizations across methods (once they are cloned). On the level of methods and above, I don't think equiality makes much sense. Each declaration is inherently unique, so inheriting Object's equality seems more reasonable to me. |
|
In Spoon, we are interested in syntactic equality, not behavioral equality (as for CSE). Currently, the beauty is that we generate the code of |
|
What's practical application of syntactic equiality of methods and classes? For example, in my code, I always use |
|
Here is a wrap-up. We study whether cloned recursive methods can point to themselves. For this, we can change the model, the cloning code or both.
Overall, I'm considering either Option 2 or WONTFIX. |
|
For me, it's definitely 4. Could you give a specific example when people do compare for equality large execution blocks and declarations such as methods and classes? |
|
Well, I don't want to go over, for instance, the 10,226 occurrences of "equals" in astor to show you exactly the problem. |
|
Here is a 5th solution: master...monperrus:refactor-clone-relative-4 The idea is to define special cloning methods which 1) fit this interesting use case we discuss here and 2) keep full backward compatibility and all contracts. Note that it goes beyond methods and CtExecutableReferences, because it has support for cloning and rewriring whole classes, it rewires type references, exec references, and field references (see test testCloneRewireLargeClass). WDYT? |
|
Current clone produces just broken AST, with unpredictable behaviour if you try to apply later transformations, refactorings, or code analysis. I discovered this issue when I doing method dependency analysis, and have seen dependencies that "shouldn't be there", while analysing methods, that were cloned several processing stages before that. I spend several hours chasing for this bug. Current clone() behaviour ought to be fixed. |
|
Well, a separate method like duplicate() will work for me, but think about other users.. |
|
master...monperrus:refactor-clone-relative-4 contains this special method, not called "duplicate" but "cloneAndRewire" |
|
I would say that your debug session was because current cloning went against your implicit assumptions, not because it is broken. To me, it's not broken, because cloning is currently very regular and systematic. The general question we are discussing here is whether clones should include:
|
|
It's not about "pretty printing" it's about producing independent entities. Let's discuss for what cloning is used? I use cloning to produce several slight modifications of one method, to later have them all in one class, instead of writing repetitive code by hand. Could you give an example when the current semantics of clone are needed? |
|
yes, this is what I mean, "minimal" means not linked to anything else, hence independent. what you need/expect seems to be such a minimal / light clone.
If you have a class A that uses methods of a class B, and you want that all calls in C=clone(A) to still resolve to B. With a minimal clone, you wouldn't resolve to them (but you would get a pretty-printable and compilable C). |
Spoon has Templates for that purpose. The templates works like this
I suggest to keep current Spoon cloning light/simple/stupid because it is predicatable and if you need more clever cloning, then implement a layer above the current clonning, which does what you need. PR is highly welcome. Note: Current Spoon templating engine is quite powerfull, but it has some limitations. So I am already improving that. |
|
It's an interesting idea to use the substitution engine for this use case. We may even implement methods How would you write the equivalent of |
yes, it works like this in #1686 Pattern pattern = PatternBuilder.create(factory, SomeClass.class,
templateBuilder -> templateBuilder.setTypeMember("someMethodName"))
.build();
CtMethod method = pattern.applyToType(targetType, CtMethod.class, Collections.emptyMap());The pattern above created |
|
master...monperrus:refactor-clone-relative-4 now clearly covers your use case (see test cases). It goes even beyond since is support copying types as well. I think those new methods can be very useful for many transformations, starting with those done by @nharrand WDYT? |
|
These copy methods should not generate name on it´s own, but it should be input parametr - so the copy has directly required name. Otherwise client has to process same algorithm again to change the names. WDYT? |
I actually "went over them" by adding There is actually a performance bug in To fix this performance bug, you should add methods to a hash set with custom equality, either by adding a wrapper of CtMethod (https://stackoverflow.com/a/5453268/648955), or using a custom library (see http://fastutil.di.unimi.it/docs/it/unimi/dsi/fastutil/objects/Object2ObjectOpenCustomHashMap.html) Those two cases boil down to construction of AST and adding newly constructed methods to a Could you provide another example of valid use of |
In this case, the proposed change to So with regard to Also interesting to note, that my use of Spoon, by which I discovered this issue (I mean the beginning of this topic, #1869), is not new. It used to use Spoon 5.2.0 and worked fine. Recently I tried to update to Spoon 6.1.0 and this issue arose. And I don't see any notes about the change in clone() behaviour that could be relevant in release notes of the versions of Spoon in between. Probably it's just because of #1875 bug that was introduced at some point, but it's also possible that |
|
Could you please answer to my last two messages? |
What do you mean by "valid use"? |
|
When the users actually need those methods as they are implemented today. Rather than actually needing only signature equality, as in the example above. |
|
It's an interesting point. There are points to consider when you provide a library. First, for any non-trivial library providing complex behaviors, it is impossible to envision all possible usages. Beyond that, I think that it is success criterion when a library starts to be used in an opportunistic manner, outside the planned and designed usage space. Second, it is well known that there is always somebody relying on a specific behavior (sometimes called @hwright Hyrum's law, well represented in the XKCD below). Consequently, humbly, I don't pretend mastering all possible ways of using the clone API, and the Spoon API in general. |
|
Consider the whole picture:
With regard to this, I also want to call a citation from "Effective Java": "Don't override equals() unless you really have to: in many cases, the implementation inherited from Object is exactly what you want". Especially considering controversy of this definition: does it compare Java AST objects, or text representations as they are pretty-printed (what about optional spaces and braces in this case?), or code semantics?
To me, this is a disservice of your users: in order not to break the code for imaginary users, you serve all users with a performance trap, and force all users to carefully write some boilerplate code. You are free to do that, but could you please provide users a utility that replaces |
Current definition of clone is simple: clone each element and attribute recursivelly. No extra mapping.
I read this issue and I do not understand what is broken. I understand that you would like that type references to type
I also do not like current CtMethod/CtClass.equals(). I always had a feeling that it is the cause of performance problems in Spoon. And the stacktrace of
if you use templates instead clonning, then this problem is no more relevant. And from my point of view this contract is not interesting.
I think spoon template is that utility. The legacy templates are already quite usable and we work on even more powerful implementation. |
|
@pvojtechovsky could you please provide the piece of code, using Spoon templates, that does what I want to do? Given a void foo() {
class Bar {
void bar() {
bar();
foo();
}
}
return new Bar();
} |
|
Nice example, I will try that. @leventov One question: is your real method which you need to clone |
|
@pvojtechovsky my method is a real Java method of a fully working real Java class, that is not written with the idea that it's going to be cloned in the future. It's not a complete "black box" method, though, I write them myself. They couldn't probably contain local classes, which are exotic, but easily anonymous classes and lambdas. |
So then current spoon Templates are not a solution for you. But #1686 is the tool, which will do what you need. I have just added the tests . See test cases in #1686 in
@leventov, please check if these tests correctly checks that all references and executables are mapped well - as you would expect. Thank You The code for clonning of any method with correct mapping of references looks like this: //create a spoon template for the method foo of `sourceType`
Pattern pattern = PatternBuilder.create(
(CtMethod) sourceType.getMethodsByName("foo").get(0))
.build();
//generate a new method (you call it clone, but it is not a clone) using that spoon template
//the method is added the targetType automatically in this case too
pattern.applyToType(targetType, CtMethod.class, Collections.emptyMap()); |
|
@pvojtechovsky could it be boiled down to a single method, that returns another method? Like |
such method would not work, because you at least need to specify reference to target type. But usually the target type reference is not the only parameter and one needs to change the generated method name, and other things... ... or I still do not understand your use case... why do you need to copy whole methods/types ? Do you change anything after copying? If yes, then spoon template can again support you and to generate directly the adapted/modified copy... so you need more parameters anyway. And if you think that PatternBuilder API and Pattern#apply API is too complicated, then I invite you into discussion in #1686. Now is the best time to improve that API - there are no clients yet, so no problems with backward compatibility ;-) |
|
[Issue Cleaning Marathon] No more activity here, closing the issue. |
Diff of the test which fails:
The text was updated successfully, but these errors were encountered: