review ParameterReferenceFunction, ParameterScopeFunction

[pvojtechovsky]

First query of the set of PRs, which will be produced from #1114. It is the simplest one - good one to discuss basic topics.

Please decide:
Q1) Is the package spoon.reflect.visitor.query OK?

Q2) I am not sure with the filter field, which does filtering of elements in scope of parameter visibility. It would be simpler to have query, which simply returns all elements in scope of parameter. And if somebody wants to filter them then s/he can apply CtQuery#map in next query step. But it is true that actually I need filtering in all use cases of ParameterScopeQuery, so may be it is correct.

Any other feedback/idea?

[monperrus]

what's the role of the filter?

[pvojtechovsky]

Now I have committed the ParameterReferenceQuery, which extends ParameterScopeQuery and returns all the references to the input CtParameter.

It is useful when somebody needs to check, whether method parameter is used. If not used then it is adept for remove - code cleaning refactoring.

[msteinbeck]

It is useful when somebody needs to check, whether method parameter is used. If not used then it is adept for remove - code cleaning refactoring.

I'm using exactly this for an unused code detector that allows to find different elements that are not in use.

[pvojtechovsky]

what's the role of the filter?

See ParameterReferenceQuery. So filter has a meaning ...

But technically it actually cannot be implemented without filter, because I need to call CtQuery#filterChildren(filter), which needs a filter :-(

Solutions:
A) to implement PassAllFilter, which always return true; and to put it there as constant.
B) to implement AllChildrenQuery, which is used like this:

list = element.map(new AllChildrenQuery()).list();

which is equivalent to this old code

list = element.filterChildren(e->{return true;}).list();

[msteinbeck]

But since I need a fast visitor I'm using the EarlyTerminationScanner. However, for very long methods it is not fast enough because there are too many elements to evaluate.

[pvojtechovsky]

unused code detector
EarlyTerminationScanner

@msteinbeck which project contains that code and class? Is it accessible somewhere? I do not want to reinvent wheel :-)

[msteinbeck]

Here you go: https://github.com/INRIA/spoon/blob/e5e98127df72cab06817547e4f084bcd7545bfc7/src/main/java/spoon/reflect/visitor/EarlyTerminatingScanner.java

Or do you mean the unused code detector?

[pvojtechovsky]

However, for very long methods it is not fast enough

idea: I needed to found all the classes which inherits from class A. I needed that for many classes A. For such use cases it is better to scan model once and to store backreferenes using CtElement#putMetadata. Thanks to this you can then very fast solve some problems, because the result is cached in element metadata.

idea 2: measure performance of "evaluation of element", may be there is some bottle neck. I do not believe that plain scanning of elements needs long time. I guess the evaluation is the problem.

[pvojtechovsky]

Or do you mean the unused code detector?

yes, both. I am interested in everything what helps might me :-)

[msteinbeck]

idea: I needed to found all the classes which inherits from class A. I needed that for many classes A. For such use cases it is better to scan model once and to store backreferenes using CtElement#putMetadata. Thanks to this you can then very fast solve some problems, because the result is cached in element metadata.

I'm using two sets for this. The first one stores the scanned elements I'm interested in and the second one stores all referenced elements. For the sake of performance, I'm using String HashSets. Each entry contains the qualified name of an element. At the end I'm using https://docs.oracle.com/javase/7/docs/api/java/util/AbstractSet.html#removeAll%28java.util.Collection%29 to extract all elements I'm interested in but are not referenced. That is very fast, though, I'm hashcoding and comparing Strings.

[msteinbeck]

yes, both. I am interested in everything what helps might me :-)

I am sorry, I can't give you the full implementation now because we are working on a journal paper that wants to introduce enhanced techniques for automated code smell detection (and implementing a fast an precise unused code detector was very tough). Once it is published, I'll work on publishing the code :)

[msteinbeck]

It is worth mentioning that I'm using Spoon's references instead of resolving the actual types since type resolution takes quite some time. That is, if a type reference is visited, I'm using the qualified name returned by the reference rather than extracting the type declaration and using its qualified name. This lowered the detection time from 8 minutes to 4 seconds :D (for very large software systems).

[monperrus]

I have a problem with ParameterScopeQuery implements CtConsumableFunction, it's a bad smell that a query implements a function and not CtQuery itself.

It's either a naming problem or a design problem.

What do you think?

[pvojtechovsky]

I understand what you mean.
I understand it as naming problem. I understand the ParameterScopeQuery as mapping function, which can be used in CtQuery#map. So I vote for finding of better name for such mapping functions.

[monperrus]

OK, let's go for ParameterScopeFunction then.
Now about the filer. I understand that you need a kind of PassAllFilter, which always return true. Why does this filter have to be configurable in the constructor and even in a setter?

[pvojtechovsky]

PassAllFilter

I came to simpler solution. We just have to make this running

element.filterChildren(null)

and optionally we might make it nicer with these 2 new mapping methods:

• CtQuery#filter(Filter)
• CtQuery#allChildren()
  but I am not sure if it is good idea.

Why does this filter have to be configurable in the constructor and even in a setter?
We can remove it from Constructor.
It needs to be in setter because of ParameterReferenceQuery needs it actually - it can be changed of course, but it seems to be quite straight forward solution.

[monperrus]

* CtQuery#filter(Filter)

We already have "map(Filter)"?

* CtQuery#allChildren()

This is an easy function. Should it include the input element?

It needs to be in setter because of ParameterReferenceQuery needs it actually

When and why?

[pvojtechovsky]

"map(Filter)"?
no we have not. We have map(CtFunction) which can behave like Filter, but existing Filter instances cannot be directly used there.

CtQuery#allChildren()

This is an easy function. Should it include the input element?

no it is without input parameters. May be allChildren(boolean includingSelf), might be interesting.

When and why?

see sources of this PR. They are short.

[monperrus]

I see, it's for extension.

Since the reused behavior is one single line, I'd prefer to have no filter field, no setFilter method, and no inheritance, for sake of simplicity and understandability.

[pvojtechovsky]

OK, I agree. I will make another PR

[pvojtechovsky]

But if we will not allow setFilter, then we should think about CtQuery#filter(Filter) method.

Filter filter = new ...;
element.map(new ParameterScopeQuery).filter(filter)...

otherwise the filtering is possible only using this not intuitive code

Filter filter = new ...;
element.map(new ParameterScopeQuery).map(e->filter.matches(e))...

And it will be even longer without java 8 lambdas.
I can live with that code, but it is not nice...

[monperrus]

Filter filter = new ...;
element.map(new ParameterScopeQuery).map(e->filter.matches(e))...

I like it, esp with lambdas, it's clear and explicit. Let's first aim at this simplest version.

[pvojtechovsky]

ok

[pvojtechovsky]

Baby step 1 done:

• renamed xxxQuery to xxxFilter
• removed filter, setFilter and inheritance. It is much simpler now. Thanks for that idea!

Note: I have to add tests too. But let's agree on design first.

[pvojtechovsky]

And this PR needs PR #1140, otherwise it fails with NullPointerException

[pvojtechovsky]

I have added test of ParameterReferenceFunction. This test is originally designed to test VariableReferenceFunction, therefore test class contains parameters, fields, local variables and catch variables. May be it is hard to understand how this test works, but it efficiently checks that ParameterReferenceFunction

• really finds all parameter references
• does not by mistake returns fields, local variables or catch variables.
  and it is easy for me to add any other case when parameter reference might occur.

It depends on these PRs

• CtQuery#filter(Filter) feature CtQuery#select(Filter) #1142
• CtQuery#filterChildren(null) feature CtQuery#filterChildren(null) #1140

[monperrus]

it looks good to me.

[surli]

Ok for me, indeed the tests are the most difficult part of this PR! Thanks @pvojtechovsky :)