review 1: feature: search for CtVariableReference in defined scope

[pvojtechovsky]

There are two things:

• The classes XxxScopeFunction have constructor with CtScannerListener parameter.
• The classes XxxReferenceFunction have constructor with var parameter. Such functions will search for references of var in children of mapping function input CtElement

The classes LocalVariableReferenceFunction , CatchVariableReferenceFunction, ParameterReferenceFunction has similar algorithm, so they inherit from AbstractVariableReferenceFunction. The FieldReferenceFunction is implemented individually, because java field references behaves different.

These improvements are needed by #1005.

[monperrus]

This PR looks good to me, it's a big refactoring of recent code.

Could you document the expectations and contracts for the implementors in the Javadoc of abstract method createScopeQuery?

[pvojtechovsky]

Documentation was updated.
I have also improved the signature of createScopeQuery method and made one class private, so there is less internal dependencies.
I am done with that. Please review

[monperrus]

I have a concern with the design of the abstract method createScopeQuery, which is the core of this PR.

While I understand well the CtQuery createScopeQuery(CtVariable<?> scope), it seems to me that the scannerListener parameter is a problem:

• we have no way to enforce or verify that the scanner listener methods are called in a correct manner
• it implicitly assumes that the returned query will use a scanner, but a query can be implemented in another way, since it is an abstraction, not directly tied to scanners
• method apply passes a very specific CtScannerListner, a context, and reuses this object after having passed it to createScopeQuery, so apply does not protect and encapsulates its internal objects.

My proposal would thus be that the context is an internal object, under the responsibility of apply only, and to have a simpler CtQuery createScopeQuery(CtVariable<?> scope).

What do you think?

[pvojtechovsky]

I probably understand the problem you see. I would agree with You in case when the AbstractVariableReferenceFunction would be extended by the Spoon clients. But it is not the case!
AbstractVariableReferenceFunction is package protected. All the contracts it declares are internal and propriatary to implement limited and final set of classes :CatchVariableReferenceFunction, LocalVariableReferenceFunction and ParameterReferenceFunction. Because of that, I think it is correct to accept these theoretical design flaws. In this case it is OK, that implementation of AbstractVariableReferenceFunction expects an specific and matching implementation of their children.

My proposal would thus be that the context is an internal object, under the responsibility of apply only, and to have a simpler CtQuery createScopeQuery(CtVariable<?> scope).

It cannot work, because it really needs to register that listener. And the only code which can register it is the implementation of createScopeQuery.

[pvojtechovsky]

Ideas:
I1) To move all implementations of createScopeQuery to AbstractVariableReferenceFunction and to make it private there. It will be code like
``java
if(variable instanceof CtLocalVariable) {
return ... variable query
} else if(variable instanceof CtCatchVariable)
return ... catch query
...
I2) optinaly to rename AbstractVariableReferenceFunction to some name which represents LocalVariable, CatchVariable, Parameter, but does not mean Field. And make it public, so it can be used directly...

I3) optinally Then we might remove all 3 child classes, because they are nearly empty now. Or we can keep them empty...

[monperrus]

That's a good idea, thanks Pavel for the constructive proposition. I prefer strong encapsulation over questionable design (even if it's internal). To avoid the not-so-nice sequence of instanceof, we can use a custom visitor, and then simply write `variable.accept(new VariableReferenceVisitor())`

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170315.164827-83

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 8.

Change 1

Name	Element
Old	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
New	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
Code	java.class.nonFinalClassInheritsFromNewClass
Description	Non-final class now inherits from 'spoon.reflect.visitor.filter.LocalVariableReferenceFunction'.
Breaking	binary: potentially_breaking, source: potentially_breaking

Change 1

Name	Element
Old	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
New	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtCatchVariable<?>>'.
Breaking	binary: breaking, source: breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtField<?>>'.
Breaking	binary: breaking, source: breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: non_breaking, source: non_breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtField<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: potentially_breaking, source: potentially_breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtLocalVariable<?>>'.
Breaking	binary: breaking, source: breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: non_breaking, source: non_breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtLocalVariable<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: potentially_breaking, source: potentially_breaking

Change 4

Name	Element
Old	class spoon.reflect.visitor.filter.ParameterReferenceFunction
New	class spoon.reflect.visitor.filter.ParameterReferenceFunction
Code	java.class.nonFinalClassInheritsFromNewClass
Description	Non-final class now inherits from 'spoon.reflect.visitor.filter.LocalVariableReferenceFunction'.
Breaking	binary: potentially_breaking, source: potentially_breaking

Change 4

Name	Element
Old	class spoon.reflect.visitor.filter.ParameterReferenceFunction
New	class spoon.reflect.visitor.filter.ParameterReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtParameter<?>>'.
Breaking	binary: breaking, source: breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtVariable<?>>'.
Breaking	binary: breaking, source: breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: non_breaking, source: non_breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtVariable<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	binary: potentially_breaking, source: potentially_breaking

Change 6

Name	Element
Old	parameter void spoon.reflect.visitor.filter.FieldReferenceFunction::apply(===spoon.reflect.declaration.CtField<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.FieldReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.CtField<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	binary: breaking, source: potentially_breaking

Change 7

Name	Element
Old	parameter void spoon.reflect.visitor.filter.LocalVariableReferenceFunction::apply(===spoon.reflect.code.CtLocalVariable<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.LocalVariableReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.code.CtLocalVariable<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	binary: breaking, source: potentially_breaking

Change 8

Name	Element
Old	parameter void spoon.reflect.visitor.filter.VariableReferenceFunction::apply(===spoon.reflect.declaration.CtVariable<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.VariableReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.CtVariable<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	binary: breaking, source: potentially_breaking

[pvojtechovsky]

What about this implementation?
I moved the code from abstract class to the LocalVariableReferenceFunction, which contains package protected constructors, which are now used by CatchVariableReferenceFunction and ParameterReferenceFunction. It is not correct from conceptual point of view, because CatchVariableReferenceFunction is not special case of LocalVariableReferenceFunction, but it is the way how to make all code private, without creating an extra abstract class...

[monperrus]

I like this better, thanks. To me it seems ready for merge.

It is not correct from conceptual point of view, because CatchVariableReferenceFunction is not special case of LocalVariableReferenceFunction

Why? I would say it is correct

[pvojtechovsky]

To me it seems ready for merge.

for me too :-)

I can proceed faster with less opened PRs :-) Rebasing is not so funny like improving awesome spoon code.

Next PR for review is #1005 please.