review: introduction of CtScannerFunction

[pvojtechovsky]

As I mentioned in #1005 it is sometime needed to

• R1 to know when query step of type filterChildren enter/exit an element on one selected query step.
• R2 to be able to skip processing of children of current element
• R3 to be able to skip processing of siblings and children of current elements
• R4 to be able to make mapping functions aware of query processing state (actually mainly whether query is terminated)

There is already a Query Execution Context (QEC) impelemented by class CtQueryImpl$CurrentStep.

There are needed these two things to implement this PR:

1. to get access to the QEC, or to provide own extension of that context
2. to define API of that QEC, which can be used to
   • drive execution of query
   • or to get query processing state.
     • the list of all Functions of the query
     • the index of actually processed Function
     • the depth of current node in the tree of nodes visited by this query

Access to QEC

The client might get access to QEC, by these ways:
Access1) CtQuery#createQueryContext()
By creation of QEC before query is executed. Then that instance can be used in functions of query to drive query

//1) create instance of QEC
 CtQueryContext cc = anElement.map(...).createQueryContext();
//2) use QEC to process query and to drive query, etc.
cc.forEach(...)

Access2) CtQuery#setQueryContext()
By creating an own instance of QEC and setting it to query

//1) create instance of QEC
 CtQueryContext cc = new QEC();
//assign that instance to query
anElement.map(...).setQueryContext(cc).forEach(...)

Access3) (CtQueryContext)outputConsumer
By conversion of outputConsumer of CtConsumableFunction(Object,CtConsumer) to QEC.

anElement.map((input, output)->{
CtQueryContext cc = (CtQueryContext)output;
//use cc to drive query
}).forEach((output)->{
CtQueryContext cc = (CtQueryContext)output;
//use the same cc instance like above to drive query
});

Access4) CtQuery extends CtQueryContext
CtQuery extends CtQueryContext, so all the methods are available through query instance.

API of QEC

There might be these methods on QEC:

• setOutputConsumer(CtConsumer) - sets the CtConsumer which will get results of this query execution
• accept(Object input) - executes the query with input.
• setTerminated(boolean) - terminates whole query
• boolean isTerminated() - returns true if query is terminated
• void onEnter(CtQueryListener) - register listener which is called before element is sent to mapping function
• void onEnter(Filter, CtQueryListener) - some like onEnter, but calls listener only for elements which match the filter.
• void onExit(CtQueryListener) - register listener which is called after element is sent to mapping function
• void onExit(Filter, CtQueryListener) - same like onExit, but but calls listener only for elements which match the filter.
• void onExitOf(Object, CtQueryListener) - same like onExit, but calls listener only when element is same like provided object.
• void skipElement(CtElement); - skips element and all children. The mapping function is not called for such element and it's children
• void setSkipMode(SkipMode); - skips elements depending mode until next call of setSkipMode or end of query.

interface CtQueryListener<T> {
   void onElement(T element, CtQueryContext QEC);
}

Example

//create query listener which skips all children of method
CtQueryListener ql = new CtQueryListener() {
  boolean skipping = false;
   //return false to skip current element
   boolean onElement(CtElement e, CtQueryContext context) {
       if(skipping) return false;
       if(e instanceof CtMethod) {
          //if client wants to be called on exit of this element, then he can register listener for that
          context.onExitOf(e, (e, context)->{
             skipping = false;
          });
          return false;
       }
       return true;
   }
}
anElement.map(...)..onEnter(ql).forEach(...);

...to be continued...
Feedbacks and suggestions are welcome

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170209.143117-32

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

Detected changes: 0.

[pvojtechovsky]

It is still not finished, but API will be like this.

public interface CtQueryListener {
	/**
	 * Called when new object enters query step, before the object is processed by mapping function of that step.
	 * <br>
	 * The returning false or throwing an exception causes that processing of input object and all it's children is skipped.
	 * The returning true causes that input object is normally processed, and exit is called for that object.
	 * @param element the processed element
	 * @param context context the query execution context
	 * @return true to continue processing this step and it's children,
	 * false to skip this step and it's children.
	 */
	boolean enter(Object element, CtQueryContext context);
	/**
	 * The exit is called always only after `enter` returns true and input element and all children are processed or they throw an exception.
	 * @param element the processed element
	 * @param context the query execution context
	 * @param exception null or an exception if the processing of the mapping function failed
	 */
	void exit(Object element, CtQueryContext context, Throwable exception);
}

and the listener can be assigned to the CtQuery using new methods CtQuery#setListener(CtQueryListener)

Please note that there is also new interface CtQueryContext, which is sent to both CtQueryListener methods. The CtQueryContext can be actually used to

• terminate the query
• to send the new input to the query - recursive execution of query (I do not know if it makes sense)
• to change the output of the query - (I do not know if it makes sense)

In future it might be used to get access to the:

• the depth of current step in query execution
• the instance of actually executed CtQuery
• the instance of current mapping function

The one query step and processing of all it's children can be terminated by returning false from CtQueryListener#enter method too, so it actually looks like that whole CtQueryContext is not needed and may stay private.

[monperrus]

Hi Pavel,
That's an interesting design. However, it feels much more complex than the previous one.

The previous one had two new protected methods in an implementation class "enter" and "exit", and I liked it very much.

Here we have two new interfaces CtQuetyListener and CtQueryContext. I'm afraid that this more complex design will be more difficult to document, maintain and understand for new users.

WDYT?

[pvojtechovsky]

Hi Martin, thanks for feedback. I will try to make CtQueryContext private again.

the previous design.

It was just in the middle of work. It was no real solution. There were protected methods on private class and there was no way how use that extended class, even if it would be protected. But I will think about that solution too.

I will experiment with current design from client's point of view. I will try to use it in refactoring code and get some usability experience.

[pvojtechovsky]

Do we need multi-threading support in CtQuery?

I mean to create one instance of CtQuery and then evaluate it parallel in multiple threads? Actually it is partially possible (depending on used mapping function).

But that support, brings some extra complexity, which influences design of this PR too.

So do you agree to make CtQuery and it's future API design single threading?

[pvojtechovsky]

So do you agree to make CtQuery and it's future API design single threading?

I know you like KISS, so your answer is 99.9% yes.

So now the CtQueryImpl is refactored and the query evaluation context is directly part of the CtQuery.
It means:

1. new methods:
   • CtQuery#isTerminated()
   • CtQuery#setTerminated(boolean)
2. we do not need CtQueryContext, but we can directly use CtQuery instead. So there is:

public interface CtQueryListener {
	boolean enter(Object element, CtQuery context);
	void exit(Object element, CtQuery context, Throwable exception);
}

I have added CtQueryImpl$QueryConsumer here too, which is used in this PR by ParentFunction. It is optionally second baby step in this PR. Let's discuss it later.

[monperrus]

So do you agree to make CtQuery and it's future API design single threading? I know you like KISS, so your answer is 99.9% yes.

Correct :-) I'm fundamentally KISS.

[monperrus]

What's parameter "exception" used for?.

[pvojtechovsky]

to detect whether processing of the children passed correctly or failed. I do not need it now, because my code does not throw exceptions, but it is API so it should be generic enough

[pvojtechovsky]

actually it is used like this:

listener.enter(item, query);
try {
 ... process children
} catch (Throwable e) {
  listener.exit(item, query, e);
  throw e;
}
listener.exit(item, query, null);

so the exit is called always, and you can detect in exit whether it failed or not by checking exception parameter.

Optionally we might implement it like this

listener.enter(item, query);
try {
 ... process children
} catch (Throwable e) {
  listener.failureExit(item, query, e);
  throw e;
}
listener.exit(item, query);

where failureExit method might added to the API later, after somebody needs it.
In such case we should provide an default implementation of listener, so the clients can extend that default implementation and will be not forced to change their code later, after we add method failureExit into API.

[monperrus]

I do not need it now

Then, I propose to remove it. Unchecked exceptions can still be thrown if necessary.

[pvojtechovsky]

During experimenting with CtQueryListener, I have found I do not want to listen for query steps. I need to listen to enter/exit of AST nodes during CtQuery#filterChildren. Therefore I have completely redesigned this PR. It is no more about query context, but about scanning context.

What I did?

BS 1. The CtQuery (represents query context too) now holds state of the query execution. The new methods CtQuery#isTerminated() and CtQuery#setTerminated(boolean). The CtQueryImpl was refactored to hold query context directly.

BS 2. The CtQuery can be passed now to any mapping function or Filter. The mapping function or Filter, which needs that information has to implement CtQueryAware and CtQueryAware#setQuery(CtQuery) will be called once before the mapping function is executed. See ParentFunction as an example of usage.

BS 3. The existing private class CtQueryImpl$ChildrenFilteringFunction was moved to own file and named FilterChildrenFunction.

BS 4. The new FilterChildrenFunction mapping function accepts the CtScanningListener (it is the renamed CtQueryListener). This listener can influence processing/skipping children elements during AST scanning process.

(The BS means Baby Step ;-) ... I know there are 4, but may be they are acceptable in this one PR...)

[pvojtechovsky]

IntercessionTest.testSettersAreAllGood:215 Your setter setTerminated in CtQuery don't have a generic type for its return type.

@monperrus Is it problem of my implementation or problem of IntercessionTest?

[monperrus]

Thanks for the proposal. The BS you propose seem reasonable, even if 4 BS may hinder a quick understanding and converging. Now, I feel a bit lost now. Before discussing further, I'd like to understand the relation between the BS and the requirements of this PR (at the top of this thread):

1. to know when query enter/exit an element during filterChildren on any query step level
2. to be able to skip processing of children of current element
3. to be able to skip processing of siblings and children of current elements

Do you still address them all? Do you address new ones?

[pvojtechovsky]

I still address them. With this little change

old: to know when query enter/exit an element during filterChildren on any query step level
new: to know when query enter/exit an element during chosen filterChildren mapping

They are covered by BS4, which needs BS3, which needs BS2 and BS1

I address new:
4. to be able to make mapping functions aware of query processing state (actually mainly whether query is terminated) - covered by BS1, BS2

[monperrus]

OK, let's discuss on BS1 and BS2 since since the others depend on it.

BS1: OK.

BS2: I'm not convinced because: 1) the responsibility of CtQueryAware is trivial, it's not worth introducing a new interface for this. 2) nobody expects a CtQueryAware (I didn't find it) 3) it introduces some state in function, which is essentially a stateless concept (and stateless is better). Am I missing something?

[pvojtechovsky]

1. the responsibility of CtQueryAware is trivial,

yes, it is trivial. I learned that concept by Spring framework. It is an easy way, how to declare, that some object needs a knowledge about something outside.

it's not worth introducing a new interface for this.

I am different opinion :-). But do not care. The question is: Do you have an alternative solution for: How to deliver CtQuery into instance of Filter, CtFunction or CtConsumableFunction?

2. nobody expects a CtQueryAware (I didn't find it)

I do not understand this note. Do you mean nobody needs that? It is used by ParentFunction (just little optimization) and it is needed by FilterChildrenFunction which was introduced in this PR. And I have feeling like many more complex mapping functions will need to be able to early terminate processing, so they actually need to know CtQuery.

3. it introduces some state in function, which is essentially a stateless concept (and stateless is better).

I also wanted stateless at the beginning. But that costs something ... higher complexity ... less KISS.
And note: the CtScanner, which is used in core of most of mapping functions, is already NOT stateless.

By this question:

So do you agree to make CtQuery and it's future API design single threading?

I asked for the agreement to leave stateless concept.
I like stateless too, and if you want to invest effort and pay in form of more complex client's API, and more complex implementation of mapping functions (new CtScanner, etc.), then let's discuss What advantage stateless brings? Do we really need that?

[pvojtechovsky]

The question is: Do you have an alternative solution for: How to deliver CtQuery into instance of Filter, CtFunction or CtConsumableFunction?

I was wrong. The context is needed in CConsumableFunction only. Other two mapping functions are not called when query is terminated. And CtConsumableFunction gets second parameter CtConsumer, which might be change or cast to something what can return the query.

But it is true that current implementation of mapping functions is usually no stateless.

[pvojtechovsky]

another change of my opinion :-)

The question is: Do you have an alternative solution for: How to deliver CtQuery into instance of Filter, CtFunction or CtConsumableFunction?

This is correct question, because CtFunction and Filter sometime might need context (CtQuery) to be able to terminate the query. I already found this use case in implementation of PR #1005

[pvojtechovsky]

This PR is finished from my side.

Or do you have a better - stateless - way how to deliver CtQuery to Filter, CtFunction or CtConsumableFunction?
Or do you agree that stateless is too expensive in this case and Spoon mapping functions will be stateful?
Most of mapping functions which I made till now are stateful anyway.

[monperrus]

We make great progress, thanks! one more question about FilterChildrenFunction.

What I understand now is that it provides the capability of CtScanner in the context of queries, efficiently.

Now, do we really need to have a filter there? Since the query architecture is step-based, and lazily evaluated, having the filter as the next step would be functionally equivalent and similarly efficient.

Correct? Can we we remove the filtering responsibility from FilterChildrenFunction?

[pvojtechovsky]

Can we we remove the filtering responsibility from FilterChildrenFunction?

probably yes. It is an interesting idea. I see only one (little?) problem
The CtQueryImpl#filterChildren will be implemented like this:

	public <R extends CtElement> CtQueryImpl filterChildren(Filter<R> filter) {
		map(new FilterChildrenFunction()); //1st step
		select(filter); //2nd step
		return this;
	}

it means CtQuery#filterChildren will internally add 2 query steps, while client might expect only one query step.

Then the CtQuery#name(String), will give name only the select step. The first FilterChildrenFunction step cannot be named.

I vote for doing that.

And then we have to rename FilterChildrenFunction to ?
A) CtScannerFunction
B) ?

[monperrus]

it means CtQuery#filterChildren will internally add 2 query steps, while client might expect only one query step.

To me it's not a problem, steps are internal, they are not part of the API.

And then we have to rename FilterChildrenFunction to CtScannerFunction?

Yes. We're along the same line.

We're done for the core design decisions. Thanks.

Now the final steps are documentation and tests. I'll do the documentation part. Can you add the relevant tests?

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170307.234505-66

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

Detected changes: 2.

Change 1

Name	Element
Old	none
New	method boolean spoon.reflect.visitor.chain.CtQuery::isTerminated()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, source: breaking, semantic: potentially_breaking

Change 2

Name	Element
Old	none
New	method void spoon.reflect.visitor.chain.CtQuery::terminate()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, source: breaking, semantic: potentially_breaking

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170307.234505-66

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

Detected changes: 2.

Change 1

Name	Element
Old	none
New	method boolean spoon.reflect.visitor.chain.CtQuery::isTerminated()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, source: breaking, semantic: potentially_breaking

Change 2

Name	Element
Old	none
New	method void spoon.reflect.visitor.chain.CtQuery::terminate()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking, source: breaking, semantic: potentially_breaking

[pvojtechovsky]

What about return value of CtScannerListener#enter. Actually it returns boolean

• true - process element and children normally
• false - skip element and all children

May be we might change boolean to enum SCANNING_MODE {NORMAL, SKIP_CHILDREN, SKIP_ALL}

What do you think?

[monperrus]

May be we might change boolean to enum SCANNING_MODE {NORMAL, SKIP_CHILDREN, SKIP_ALL}

Excellent idea! It's more readable and allows for future changes and extension.

[pvojtechovsky]

#1180, #1211 and #1210 are updated and are using new ScanningMode enum. Please review

[monperrus]

merged #1210, you can rebase this one, and I'll pass over the doc.

[pvojtechovsky]

rebased

[monperrus]

Doc PR on this one: pvojtechovsky#14

[pvojtechovsky]

Looks like it is ready for merge. I will then rebase other PRs

[monperrus]

Thanks a lot for the effort put in this very well-done contribution.

[pvojtechovsky]

You are welcome! 👍