review: refactor: CtScanner to report CtRole of scanned element

[pvojtechovsky]

This is an alternative to #1631 which directly modifies CtScanner ... to see how much Spoon will be broken...

[pvojtechovsky]

Only these tests fails:

CtGenerationTest.testGenerateCloneVisitor:113 » ClassCast spoon.support.reflec...
CtGenerationTest.testGenerateCtBiScanner:75 » ClassCast spoon.support.reflect....
CtGenerationTest.testGenerateReplacementVisitor:43 » ClassCast spoon.support.r...

It is fixable. WDYT? Will we go this way?

[monperrus]

I really like this solution, which can probably be 100% backward compatible, without any API change (incl the one detected by Revapi).

[pvojtechovsky]

I am glad to hear that we are on the same side ;-)

The little problem is that refactored CtScanner is harder for maintenance, because we have to actually manually take care about consistency of CtRole and method call pairs.

It would be cool if CtScanner might be generated from Spoon model. But actually there are missing annotations, which would define scanning order. Let's discuss it in #1634

[monperrus]

The little problem is that refactored CtScanner is harder for maintenance, because we have to actually manually take care about consistency of CtRole and method call pairs.

Yes, but once this is done, this evolves very little. I don't see this as a big problem.

[pvojtechovsky]

I agree, it is not big problem ... it can be covered by test, which checks that CtRole is matching method.
I will implement this test soon.

[pvojtechovsky]

I have implemented the test which checks whether correct role is user in CtScanner. But that test does not work well, because it is based on existing test CtScannerTest#testScannerCallsAllProperties, which is buggy - some methods are not checked. For example CtActualTypeContainer#getActualTypeArguments()

@monperrus Could you please have a look at it and fix the origin test?

[pvojtechovsky]

1. The CtBiScanner knows CtRole now too - @monperrus WDYT?

2. there are some problems with definition of @derived properties in Spoon model. The problems are visible after the bug in SpoonTestHelpers is fixed.
   A) CtScannerTest is wrong
   B) Spoon model is missing some @derived annotations
   C) A+B ;-)

There is also bug in CtScanner#visitCtAnnotationFieldAccess, which calls derived scan(CtRole.TYPE, annotationFieldAccess.getType());

[pvojtechovsky]

It actually fails on inconsistency in @DerivedProperty.

Problem:
Till now the detection of whether property is derived or not was based mainly on interfaces. But that is not correct, because implementation class can inherit from more super interfaces and each of them can declare @DerivedProperty in different way.
I guess the correct is to check annotations on the method which really implements the behavior.
But in the case of CtBlockImpl#getLastStatement() there is missing @DerivedProperty annotation.

WDYT? should check @DerivedProperty on implementation classes and not on intefaces?
If yes, then we can fix CtBlockImpl#getLastStatement(). If not, then what do you suggest?

[pvojtechovsky]

This PR now uses new SpoonMetaModel and it's detection of isDerived field.
I have fixed some inconsistencies in spoon model.

@monperrus, Now I stuck on scanning of COMMENT field. It is missing many CtScanner.visitXxx methods. Is it intentional???

[pvojtechovsky]

ping @monperrus I need your answer to continue here. Thanks! ;-)

[tdurieux]

@monperrus, Now I stuck on scanning of COMMENT field. It is missing many CtScanner.visitXxx methods. Is it intentional???

I don't remember there was any discussion, I had very quickly the support of comment, I probably forgot to add some scanner.
@surli also added some elements in CtComment.

[surli]

@surli also added some elements in CtComment.

Actually I started to work on it but I stopped and I never created any PR, so I'm not sure I can really help here :)

[monperrus]

Now I stuck on scanning of COMMENT field. It is missing many CtScanner.visitXxx methods. Is it intentional???

Yes, it is. As far as I remember, the main idea is that comments are not part of equality checking. Since EqualVisitor is derived from CtScanner we explicitly exclude all calls to getComments

[pvojtechovsky]

we explicitly exclude all calls to getComments

You are not right, or it must be a misunderstanding, because there are already 65 calls of getComments and only 8 are missing.

[pvojtechovsky]

Something bad happened to CI? Please check it.

[tdurieux]

there is a problem with travis: https://www.traviscistatus.com/incidents/4vdl52d28hz3

[pvojtechovsky]

This PR is ready for merge from my point of view

[monperrus]

Is it possible to keep spoon.reflect.visitor.CtScanner::scan(java.lang.Object) to be 100% backward compatible?

[pvojtechovsky]

Done. It should work like this. I am done here

[pvojtechovsky]

I have found that this PR contained some changes in SpoonTestHelpers, which are not needed for this PR. I have rolled back these changes. I am done here.

[monperrus]

thanks Pavel!