bug in printing of Implicit access to static field

[pvojtechovsky]

Added test to reproduce bug in printing of Implicit access to static field

[pvojtechovsky]

Hi Martin,
I have committed the change, which fixes the problem for me, but I am really not sure if that code is correct. My spoon knowledge is not good enough to be sure.
Best regards,
Pavel

[pvojtechovsky]

This solution is different. It does not try to use implicit access, but it instead forces to use qualified name in case of conflict of Class name and static field name. It has less impact on printed sources, so it is more compatible with existing tests.

I see these remaining problems:

1. I would like to see the new private method DefaultJavaPrettyPrinter.getDeclareddOrInheritedField as public method on CtType. Use a better name of course. I can make a PR if you like that idea. Just give me some better name.
2. performance of pretty-printing is 5 times worse then before... It all the time checks whether there are conflict fields. Before the pretty-printing time was about 10s and now it needs 40-60s! :-(

May be we can try to improve performance by replacing of CtTypeImpl.typeMembers list by a LinkedHashMap?

[pvojtechovsky]

The method on CtType.getDeclaredOrInheritedField would be nice, but the correct solution for the performance problem is to store set of all names of localy visible fields and methods in Printer context. Then we will resolve that list only once when we enter the scope of some class/interface and just check it whenever we need to print type access.

[tdurieux]

getDeclaredOrInheritedField is very similar to CtTypeInformation#getAllFields().
The performance issue comes from the method getTypeDeclaration() that may use the reflection to retrieve the information of the Type.
The PR #898 may resolve your performance issue.

[monperrus]

merged #898.

Before the pretty-printing time was about 10s and now it needs 40-60s! :-(

can you rebase and measure again?

[pvojtechovsky]

Windows is bad system to measure performance... it does so many things on the background ... :-(
I measured this configurations:
#898
printed in: 8584, 9214, 11176 ms

#898 + this PR
printed in: 29257, 18295 ms

this PR without #898
printed in: 36987, 136324 ms ... so different results. I hate MS...

Results:

1. The Fix #867: perf shadow #898 really improves performance
2. This PR even with Fix #867: perf shadow #898 is still 2-3 times slower then origin code.

[tdurieux]

getDeclareddOrInheritedField -> getDeclaredOrInheritedField (double dd)

We agree to move it to CtType.
We must also handle fields from interfaces.

[pvojtechovsky]

I just noticed there is new comment...
What will be the name of that new method on CtType?

We must also handle fields from interfaces.

I agree

[tdurieux]

You can keep getDeclareddOrInheritedField is long but it is explicit

[pvojtechovsky]

Do you agree that CtType.getDeclaredOrInheritedField(fieldName) should return also fields from classes, which are not in CtModel? ... the fields found by java reflection.
So I should add

interface CtTypeReference {
CtFieldReference<?> getField(name);
CtFieldReference<?> getDeclaredOrInheritedField(name);
}

because if we would ignore such fields, then bug #913 would be still reproducible on classes inherited from classes which are not part of the model.

[monperrus]

Do you agree that |CtType.getDeclaredOrInheritedField(fieldName)|
should return also fields from classes, which are not in CtModel? ...
the fields found by java reflection.
Yes.

CtFieldReference<?> getDeclaredOrInheritedField(name);
That could go in CtType, or even better in CtTypeInformation.

--Martin

[pvojtechovsky]

My latest changes fails because of bug #959. After this bug is fixed, then it should pass the tests too.

[pvojtechovsky]

It now performs well. It is back on 10s. The currentThis type field names are now cached in PrintingContext, so they does not have to be resolved again and again.

I am not using new method getDeclaredOrInheritedField, but existing CtTypeInformation.getAllFields(). The difference is that CtTypeInformation.getAllFields() is not checking fields on interfaces. So may be we should change getAllFields by way it gets fields from interfaces too.

May be the new getDeclaredOrInheritedField should be removed if it is not used? But some client might appreciate it.

It still fails on bug #959
And it fails on code snippet, which has not set class path?

[monperrus]

The difference is that CtTypeInformation.getAllFields() is not checking fields on interfaces.

This is a bug, would you propose a PR? Thanks.

[monperrus]

May be the new getDeclaredOrInheritedField should be removed if it is not used?

Yes, you can remove it from here. However, I like the idea, we could you move it to another PR.

[pvojtechovsky]

The getDeclaredOrInheritedField is moved to #967
The getAllFields() now collects fields of interfaces too. I have prepared the test too, but it is in conflict with PR #967, so please merge #967 first.

I had to fix SnippetCompilationHelper. Now it prints the Wrapper helper class, which is made to wrap the snippet ,before Wrapper class is removed from the CtModel of factory. It avoids ClassNotFoundException.

[pvojtechovsky]

Looks like might be merged after bug #959 is fixed

[monperrus]

According to @tdurieux 's comment, the only solution seems to be to make a minimal change to ImportTest.testSpoonWithImports so that all tests pass.

[tdurieux]

Unfortunately we cannot change testSpoonWithImports it tests some tricky access path in java.
This test was created because an user encounter a compilation error in this specific scenario.

The solution is not removing the difficulties.

[monperrus]

your last commit makes sense.

I would propose to also change CtType<?> t = getDeclaration(); by CtType<?> t = getTypeDeclaration(); (use of shadow classes if possible), and then we can rid of return RtHelper.getAllFields(getActualClass(), getFactory());.(but you would still catch the exception and do the hack-of-hack.

[pvojtechovsky]

access path problem - the hack

OK, I did not know before, that "access path" is used to reference super type. But may be I have now an idea, how to fix that without need of deeper change of spoon model.
Where we can discuss that?
A) spoon mailing
B) make a issue or PR for that
C) we do not need/want to discuss it with you.... even if it is not the answer I want to hear, I will really appreciate it, comparing to letting me write some ideas, which nobody is interested in :-) ... because you have already a better concept. I will then focus on other interesting parts of spoon and trust You! ;-)

[monperrus]

Where we can discuss that?

issue or PR is perfect (PR is even better, you can start one with an
empty commit)

[pvojtechovsky]

Thanks for the tip with CtType<?> t = getDeclaration(), the code looks better now :-) ... even if it is quite dirty ;p
:-)))

[monperrus]

thanks a lot for this long and hard PR!

[pvojtechovsky]

You are welcome!