feature: new query architecture with support for Java8 queries #1018

pvojtechovsky · 2016-11-30T21:02:40Z

Requirements for a new query architecture:

map functions with arbitrary return types (incl. that are not CtElement)
support for queries with Java 8 lambdas
chained queries ( "chaining" means writing filterChildren(x).filterChildren(y)... or map(z).map(t)... or even filterChildren(p).map(q)...)
lazy evaluation

With this PR you can write code like

CtMethod<?> method = ...
List<CtMethod<?>> overridenMethods = Query.getElements(new OverriddenMethodFilter(method));

Which makes usage of complex filters more reliable, because these filters can automatically define correct searching context. For example

OverriddenMethodFilter will search in root package
local variable reference filter will search in scope of its CtBock element
member variable reference filter will search in scope of its Class and depending on visibility in all inherited class, in current package or in root package
...

or you can write

CtMethod<?> method = ...
List<CtMethod<?>> overridenMethods = method.getElements(new OverriddenMethodFilter())

which 1) is shortest code and 2) which will let us chain filters (later) into complex filters chains, which are able to browse spoon model in sequence of searching steps. I will probably need something like this for refactoring methods, I am preparing.

tdurieux · 2016-11-30T22:11:35Z

Nice. 👍

One day we will able to write modern code that uses spoon :D

monperrus · 2016-12-01T08:11:57Z

Excellent indeed! Could you add a presentation of this new feature in doc/filter.md?

pvojtechovsky · 2016-12-01T08:19:33Z

documentation

ok, but I would first like to finish real chaining of filters using a new QueryBuilder and PR #1017. The combination of these two things will start to be interesting. I will then document it together.

pvojtechovsky · 2016-12-01T19:50:49Z

Documentation added.

tdurieux · 2016-12-01T19:54:50Z

Documentation added.

Thanks

Do you know that you can use java 8 features in the tests?

pvojtechovsky · 2016-12-01T20:16:42Z

java 8 features in the tests

yes, I found it when I added spoon into Eclipse. The spoon code is java 7 and tests are java 8 compatible

pvojtechovsky · 2016-12-01T20:27:20Z

I have found one problem of this PR. It is not backward compatible with existing filters. For example
this code of FilterTest.testFieldAccessFilter():

CtFieldReference<?> ref = ...
List<CtFieldAccess<?>> expressions = someClass.getElements(new FieldAccessFilter(ref));

will behave different when we improve FieldAccessFilter by way it implements ScopeAwareFilter and ChainableFilter. Because then the correct call is:

CtFieldReference<?> ref = ...
List<CtFieldAccess<?>> expressions = ref.getElements(new FieldAccessFilter());

How to solve that?
A) to not use CtElement#getElements, but new one like CtElement#query or searchFor or ...?
B) to keep old filters as they are and to implement new filters ... 👎
C) ? any ideas ?

By the way, Travis failed. Should I change something?

pvojtechovsky · 2016-12-01T20:47:11Z

I have rolled back CtElement#getElements, because it is not backward compatible.

I would actually prefer solution like this:

CtFieldReference<?> ref = ...
List<CtFieldAccess<?>> expressions = ref.search(new FieldAccessFilter()).list();
//or optionally this should be supported too
ref.search(new FieldAccessFilter()).forEach(CtFieldAccess<?>fa->{...});

Note that CtElement#search(Filter) method would return new QueryBuilder object, which can be used to build even more complex queries using fluent API. When query is built, then there are two ways how to start the query
A) QueryBuilder#list() which processes query and returns List of found elements
B) QueryBuilder#forEach(ElementConsumer consumer) which processes query and calls consumer for each found element.

If you like this approach, then it is question how to make a PR for that?
A) add QueryBuilder into this PR
B) you will merge this PR as it is and I will make new PR with QueryBuilder

pvojtechovsky · 2016-12-01T22:50:13Z

I have added QueryBuilder to this PR. The QueryBuilder is now used in Query API too, so it is tested by all test code which uses Queries.

The documentation does not contain QueryBuilder yet. I will update it probably tomorrow.

This old code

CtFieldReference<?> ref = ...
List<CtFieldAccess<?>> expressions = someClass.getElements(new FieldAccessFilter(ref));

can be written this way:

CtFieldReference<?> ref = ...
List<CtFieldAccess<?>> expressions = ref.query(new FieldAccessFilter()).list();

after FieldAccessFilter will implement ChainableFilter in future PRs.

pvojtechovsky · 2016-12-03T00:13:56Z

This new code needs further refactoring and comments, test, etc. But the idea is like this:
Query builder connects ElementPipes into chain, which can be used process several queries on the model and to collect several data from these nodes. See this example

	class Context {
		CtMethod method;
		CtMethod overridenMethod;
	}
		
	Context context = new Context();

	launcher.getFactory().Package().getRootPackage().query(new TypeFilter(CtMethod.class))
		.then(new ElementPipeImpl<CtMethod,CtMethod>(){
			@Override
			protected void onAccept(CtMethod element) {
				context.method = element;
			}
		}).then(new OverriddenMethodFilter())
		.then(new ElementPipeImpl<CtMethod,CtMethod>(){
			@Override
			protected void onAccept(CtMethod element) {
				context.overridenMethod = element;
                                ...
			}
		}).list();

it searches for all methods and then for all overriden methods of found methods. The Context class is used just as example how client can collect data from the middle of the query processing.

pvojtechovsky · 2016-12-04T22:38:14Z

This PR now contains support for query step chain. I implemented it again since beginning, so please forget olders concepts and check if they are still used in this PR or not.

See the first draft of documentation in doc/Filter.md.

The code is not finished yet.

it needs to be commented
I don't like CtElement#query() but how to do it better?
write jUnit test - the code is really not tested yet

Please review the names of the new Classes, packages and methods. Let me know if something is not understandable or confusing. Do you have a better name for AsyncFunction?
Are the QueryStep build methods then, scan and matches understandable?
Any other feedback is welcome too.

pvojtechovsky · 2016-12-05T20:43:21Z

I am still not finished, but query like this works now. Do you know what it does??

		launcher.getFactory().Package().getRootPackage().query().scan((CtClass<?> c)->{return true;})
			.then((CtClass<?> c)->c.getSuperInterfaces())
			.then((CtTypeReference<?> iface)->iface.getTypeDeclaration())
			.then((CtType<?> iface)->iface.getAllMethods())
			.then((CtMethod<?> method)->method.getSimpleName().equals("make"))
			.then((CtMethod<?> m)->m.getType())
			.then((CtTypeReference<?> t)->t.getTypeDeclaration())
			.forEach((CtInterface<?> c)->{
				assertEquals("ITostada", c.getSimpleName());
			});

It scans AST from root package, accepts CtClasses, then it navigates to all super interfaces of found classes and searches there for each method whose name is equal to "make". If an method is found then it takes its return type and checks that it has found expected value.

I have removed QueryStep#matches. It actually uses then(Function) and the QueryStep detects that returned value is Boolean and in such case behaves as predicate.

monperrus

it looks very promising, but the Javadoc is too scarce to 100% grasp the design (one line per class describing the responsibility and key collaborating classes -- callers, callees, hierarchy -- is usually enough is single responsibility is applied).

monperrus · 2016-12-05T21:03:08Z

src/main/java/spoon/reflect/visitor/chain/AsyncFunction.java

+ * @param <T> the type of the input to the function
+ * @param <R> the type of the result of the function
+ */
+public interface AsyncFunction<T, R> {


name and javadoc not clear enough to convey the core design intuition and contract

why async? executed asynchronously? or do you mean deferred?

I know that Async is not good name for that. Deferred has a time meaning too, but it is better then Async. The purpose of this function is to produce zero, one or more elements from one input element. May be names like

ChainableFunction

CallbackFunction

ProductFunction

...
I like no one :-(

ChainableFunction is better IMHO

monperrus · 2016-12-05T21:03:47Z

src/main/java/spoon/reflect/visitor/chain/AsyncFunctionQueryStep.java

+
+import spoon.support.util.SafeInvoker;
+
+public class AsyncFunctionQueryStep<O> extends QueryStep<O> {


monperrus · 2016-12-05T21:04:23Z

src/main/java/spoon/reflect/visitor/chain/Function.java

+ * @param <T> the type of the input to the function
+ * @param <R> the type of the result of the function
+ */
+public interface Function<T, R> {


monperrus · 2016-12-05T21:04:42Z

src/main/java/spoon/reflect/visitor/chain/Predicate.java

+/**
+ * @param <T> the type of the input to the function
+ */
+public interface Predicate<T> {


the same as Filter? (but the generic T?)

Yes generic.

QueryStep chain can be used to process elements which are not inherited from CtElement. I am not sure whether we needs that.

Spoon Filter has already old meaning like Filtering of children produced by CtScanner, I wanted something more generic, what can be used to decide whether input element should be sent to output or not.

OK, could you put this explanation in the Javadoc?

monperrus · 2016-12-05T21:06:58Z

src/main/java/spoon/reflect/visitor/chain/QueryStep.java

+ * @param <I> - input type
+ * @param <O> - output type
+ */
+public abstract class QueryStep<O> implements Consumer<Object> {


what about having an interface QueryStep? (in Spoon, the design is very interface-oriented: most key concepts and key methods are defined in interfaces). The method then seems indeed essential for the design.

monperrus · 2016-12-05T21:07:46Z

src/main/java/spoon/reflect/visitor/filter/Scann.java

+import spoon.reflect.visitor.chain.AsyncFunction;
+import spoon.reflect.visitor.chain.Consumer;
+
+public class Scann extends CtScanner implements AsyncFunction<CtElement, CtElement> {


what does Scann mean?

It can be used in the QueryStep chain to visit all the children of chain input element.
It is used automatically In combination with filter in method QuertStep#scan(Filter), but I guess it can be use independent using ...query().then(new Scann())....

and why this strange name?

monperrus · 2016-12-05T21:08:39Z

src/main/java/spoon/support/util/RtHelper.java

 	 */
-	public static Class<?>[] getMethodParameterTypes(Class<?> clazz, String methodName, int numParams) {
+	public static Method getMethod(Class<?> clazz, String methodName, int numParams) {


non backward-compatible change on public method

The method getMethodParameterTypes is there only few days. I guess nobody is using yet. And because nobody needs it, I removed it to minimize amount of code.
But if you like this method, then I will put it back. No problem

OK if it's a recent one.

monperrus · 2016-12-05T21:09:05Z

src/main/java/spoon/support/util/SafeInvoker.java

+import spoon.SpoonException;
+
+/**
+ * @param <T> the Type of the delegate object, which is target of method invocation


contract? responsibility?

monperrus · 2016-12-06T18:00:22Z

src/test/java/spoon/test/filters/FilterTest.java

@@ -332,6 +335,8 @@ public void testOverriddenMethodFromAbstractClass() throws Exception {
 		final CtClass<AbstractTostada> aClass = launcher.getFactory().Class().get(AbstractTostada.class);

 		assertEquals(0, Query.getElements(launcher.getFactory(), new OverriddenMethodFilter(aClass.getMethodsByName("prepare").get(0))).size());
+		assertEquals(0, Query.query().then(new OverriddenMethodFilter()).list(aClass.getMethodsByName("prepare").get(0)).size());


hard to understand to start with a call to "then" with no element.

Query.query().then(...)

yes, I do not like it too. But it was minimum code at the beginning. If you have better idea, then tell me please. Otherwise I suggest to wait little bit until I (or somebody else) start to use it and come with real experience and needs... I am also looking for a better way ...

monperrus · 2016-12-06T18:01:46Z

src/test/java/spoon/test/filters/FilterTest.java

+		QueryStep<CtClass<?>> l_qv = launcher.getFactory().getModel().getRootPackage().query().scan(new TypeFilter<>(CtClass.class));
+
+		assertEquals(0, context.counter);
+		l_qv.forEach(cls->{


I like the foreach very much

I would like to see foreach more often in Spoon API - even in core model ... I will make other PR. But later ...

monperrus · 2016-12-06T18:03:05Z

src/test/java/spoon/test/filters/FilterTest.java

+
+		launcher.getFactory().Package().getRootPackage().query().scan(new TypeFilter<CtMethod<?>>(CtMethod.class))
+		.thenConsume((CtMethod<?> method) -> {context.method = method;})
+		.then(new OverriddenMethodFilter())


what you call then is a functional map (https://en.wikipedia.org/wiki/Map_(higher-order_function))?

Thanks! I need suggestions and links like that! I do not have theoretic training in these things

monperrus · 2016-12-06T18:03:25Z

src/test/java/spoon/test/filters/FilterTest.java

+		Context context = new Context();
+
+		launcher.getFactory().Package().getRootPackage().query().scan(new TypeFilter<CtMethod<?>>(CtMethod.class))
+		.thenConsume((CtMethod<?> method) -> {context.method = method;})


what's the difference between then and thenConsume?

The problem is that we must visibly distinguish between
A) then(...a function which produces elements for next step...)
B) thenConsume(... a function which just eats elements and produces nothing....)

We cannot let this distinction on the java compiler, because it would be confusing. Example:
(element)->element.doSomething()
Is that Consumer, which produces nothing or is it a function which produces result? Sometime the client wants to call function which returns values as consumer too.

If you write code like
...query().then(X).then(Y).then(Z)
then it processes X and the result of X is sent to Y and result of Y is sent to Z

But if you write code like
...query().then(X).thenConsume(Y).then(Z)
then it processes X and the result of X is sent to Y, which produces nothing and result of X is sent to Z

I am again open to change function name to anything better. May be I should learn some RxJava API and take over the names from that. Is there anybody who knows that API and can suggest names which are generally used and understandable?

monperrus · 2016-12-06T20:38:45Z

A) then(...a function which produces elements for next step...) B) thenConsume(... a function which just eats elements and produces nothing....)

A) map and B) forEach (or "iter")?

But if you write code like |...query().then(X).thenConsume(Y).then(Z)| then it processes X and the result of X is sent to Y, which produces nothing and result of *X* is sent to Z

That's quite counter-intuitive.to me. Do we need/want this behavior?

pvojtechovsky · 2016-12-06T21:04:30Z

I will replace then by map.

thenConsume(Consumer) ... That's quite counter-intuitive.to me. Do we need/want this behavior?

I wanted that to be able to collect the intermediate results of the query (model browsing). But it can be replaced by

query().then((element)->{
   ...do required consuming...
  return element;
})

so we have one query "command" (thenConsume) less => simpler API and it is easy to get same behavior with existing then (map) command. I will remove it. Thanks for feedback! I want to have it simple and intuitive too.

thenConsume ... forEach (or "iter")?

thenConsume(Consumer) was intended for use in the middle of the query chain. The QueryStep#forEach(Consumer) is already there. The difference to thenConsume(Consumer) is that forEach executes the query, while thenConsume only add a QueryStep, but do not execute the query.

forEach or iter

I prefer forEach, because there is java 8 Iterable#forEach(Consumer). So I think this name will be intuitive for Java 8 developers. Or do you think it is something else and we should use different name?

pvojtechovsky · 2016-12-07T21:21:03Z

... still not ready for merge. I have pushed it here to show the current state and to see whether it pass the tests. Feedback is welcome, of course.
I am mainly not sure with new CtElement methods map(...) and scan(Filter), which replaces ugly query().

monperrus · 2016-12-08T09:52:32Z

OK

pvojtechovsky · 2016-12-08T21:32:49Z

I have commented all the new code and did final simplifications and renames. I am satisfied with that version, but if you have any suggestions, or I forgot something then let me know please.

The test cases are still missing. Please make a review of new API so I can finally document it in Filter.md. I will do that probably on Saturday evening.

monperrus · 2016-12-09T20:10:25Z

src/main/java/spoon/reflect/declaration/CtElement.java

@@ -35,7 +36,7 @@
 * element).
 */
 @Root
-public interface CtElement extends FactoryAccessor, CtVisitable, Cloneable {
+public interface CtElement extends FactoryAccessor, CtVisitable, Cloneable, QueryComposer {


QueryComposer contains all the methods, which can begin with creation of some query. It is the replacement for CtElement#query(). With QueryComposer you can easily start to query spoon model starting from any element.
Note: QueryComposer is implemented by QueryStep too.

OK, let's first agree on this. I'm not fan on this because you cannot really say that a CtElement is a QueryComposer. However, I understand the idea of having an interface here because you want to have a starting point from any element, and the methods could also be used for chaining.
So we have to find a better name that grasps the responsibility. What do you think of QueryStartingPoint? (a CtElement is a query starting point).

Yes, I fully agree with you. New name QueryStartingPoint is good for CtElement. But it is not so good for QueryStep, which will inherit it too and will use these methods for building of next query step.
What about
class CtElement extends QueryStartingPoint
class QueryStartingPoint extends QueryComposer
class QuerStep extends QueryComposer

... it is a kins of compromise. The best would be to found a name which is good for both interfaces.

One related idea... There is Query class, which has actualy static methods. It would be may be nice to change them to instance methods and to let Query extend QueryComposer too. So the name of the interface should fit to that idea too. But let's duscuss it later. This PR is complex enough...

monperrus · 2016-12-09T20:14:10Z

src/main/java/spoon/reflect/visitor/chain/QueryComposer.java

+ * It is implemented 1) by {@link CtElement} to allow creation new query and its first step
+ * 2) by {@link QueryStep} to allow creation of next query step
+ */
+public interface QueryComposer {


I'm not sure about the name, what do you think of QueryResult?

It is there to compose first or next queryStep. It is really not about result/evaluation of the query.

If you do not like QueryComposer, then alternatives might be

QueryCreator

QueryStepBuilder

QueryFactory

... ?

monperrus · 2016-12-09T20:15:45Z

src/main/java/spoon/reflect/visitor/chain/QueryStep.java

+ *
+ * @param <O> the type of the element produced by this QueryStep
+ */
+public interface QueryStep<O> extends QueryComposer, Consumer<Object> {


The role and methods of QueryStep and QueryComposer seem close. Does it make sense to merge them in a single interface?

I think no. QueryComposer has only methods, which can create query step. But QueryStep has methods which can evaluate query too. It makes no sense to evaluate query on CtElement (which implements QueryComposer) directly, because there is no query yet. First you have to create one or more query steps (using QueryComposer) and then you can evaluate the query using methods, which are only on QueryStep only.

monperrus · 2016-12-09T20:18:36Z

src/main/java/spoon/reflect/visitor/chain/ChainableFunction.java

+ * @param <R> the type of the result produced by this function
+ */
+public interface ChainableFunction<T, R> {
+	void apply(T input, Consumer<R> output);


what about extending Function (intuitive for ChainableFunction), and put the consumer as return type: Consumer<R> apply(T input?

It is not possible. It is different contract. Have a look at new class
src/main/java/spoon/reflect/visitor/filter/Scann.java
It implements ChainableFunction. How you would implement such thing using Consumer<R> apply(T input) ?

Looking at Scann, basically Consumer is the implicit Function. What if Consumer extends Function (accept becomes apply) and then you only have one method map?

monperrus · 2016-12-10T16:42:21Z

src/main/java/spoon/reflect/visitor/chain/QueryComposer.java

+	 * @param queryStep
+	 * @return the entered queryStep, which is now the last step of the query
+	 */
+	<R> QueryStep<R> map(QueryStep<R> queryStep);


all map functions are really close (same name, same return type). It suggests a common abstraction (and the proximity of Function and ChainableFunction goes in the same direction).

Is it possible to have a single map function with an appropriate common interface as parameter?

Is it possible to have a single map function with an appropriate common interface as parameter?

I do not know how it would look like. Please describe your idea in more details.

Why three functions are problem?

I need all three functions for these cases:

map(QueryStep) - it let's us to combine query chains together

map(Function) - it let's us to make a query based on standard functions of CtModel elements. It simply gets the input element and returned value is sent to next step. See javadoc description for details about 4 possible usages.

map(ChainableFunction) it let's us implement clever Filters, which automatically computes the scanning scope. For example FieldReferencesFilter, which searches for references to an field depending on field visibility.

Note: we cannot merge map(Function) and map(ChainableFunction) into one. Because Function#apply has 1 input parameter, while ChainableFunction#apply has 2 parameters. And if I would make one Interface, which would contains both apply functions, then it would be not possible to implement it using java 8 lambda expression, because it expects only one non default method in the interface.

Why three functions are problem?

because it's not elegant and hints to a design issue.

monperrus · 2016-12-17T16:23:42Z

added the requirements for this new feature at the top of this thread.

pvojtechovsky · 2016-12-17T20:34:34Z

Thanks for Your PR! It helped me to understand Your thinking. Now I can try to clarify the misunderstanding, which is there. You are trying to implement different feature then me.
My PR contains these features:
F1 possibility to implement CQS like FieldReferencesQuery. I need that for another PR about refactoring.
F2 chaining of query steps
F3 the query steps based on CtFunction
Your PR contains only F2 and F3. It is not sufficient for me. OK, I know that you preffer small steps and my PR was too big step.

So I will comment your PR now and then merge it.

pvojtechovsky · 2016-12-18T07:56:33Z

Please see my comments in your PR, but do not fix anything in your PR. I will takeover it as it is and then, I will simplify it even more. So we will have first small step on which we both can agree on.

What I will do?
I like the idea of FieldReferenceFilter as Query. So we will have no query steps. Just queries which can be composed from other queries.
I will try to give a chance the idea of replacing Consumer by CtFunction<T,Void>
I will remove CtQueryable from the CtElement. The problem of that approach is that Query has predefined input (source element) since beginning and it makes things more complicated then is needed. So after that change we will evaluate query on an inputElementthis way:

factory.Query().map(new FieldReferenceQuery()).forEach(resultConsumingFunction, inputElement
//OR
List list=factory.Query().map(new FieldReferenceQuery()).list(inputElement)
//OR
factory.Query().map(new FieldReferenceQuery()).map(resultConsumingFunction).list(inputElement)

This approach is not so nice like current one, but it is simplest code and the most powerful and flexible way. Later we can use that low level code and to write some nicer facade about that, so the usability will improve too.
I am also not sure about factory.Query, forEach and list method name... etc.

One more idea: If we will not merge code of OverridenMethodFilter with OverridenMethodQuery, then we can even merge map function with filterChildren, because the algoritm and client will know what to do by looking at implemented interface or class name. But may be filterChildren is good name, which intuitivelly explains what such query step will do, so we can keep it.

monperrus · 2016-12-18T15:13:16Z

I will takeover it as it is and then, I will simplify it even more. So we will have first small step on which we both can agree on.

OK, excellent.

Just queries which can be composed from other queries.

Sounds a cool idea

But may be filterChildren is good name, which intuitivelly explains what such query step will do, so we can keep it.

The big advantage of filterChildren right now, is that it's a natural explanation and transition step from old-style filters no new-style queries.

I will remove CtQueryable from the CtElement.

Are you sure? It's a really meaningful, concise and coherent interface, perfectly fitting into CtElement.

pvojtechovsky · 2016-12-19T09:01:33Z

I will remove CtQueryable from the CtElement.

Are you sure? It's a really meaningful, concise and coherent interface, perfectly fitting into CtElement.

I agree with you, but it is kind of facade over the internal query engine. And I have learned from this PR, that too much steps at once causes misunderstanding. So I want to have first low level query engine merged in Spoon and then we can discuss how to integrate it into CtElement.
Why that? The interface like CtQueryStep#forEach(Consumer out, CtElement input) is the low level and I NEED it to build other features. But if I directly implement CtElement facade, then it looks like this interface is not needed and You tends to delete it. ;-)

monperrus · 2016-12-19T09:07:32Z

And I have learned from this PR, that too much steps at once causes misunderstanding.

I learned that too. And also that writing requirements upfront is useful.

So I want to have first low level query engine merged in Spoon and then we can discuss how to integrate it into CtElement.

OK.

pvojtechovsky · 2016-12-19T21:00:45Z

I am not able to implement next version, because each idea ends with the progress killing thought: "Martin doesn't like CtConsumer, CtQueryStep, forEach"....
... and I do not like list(), which will need a lot of memory for queries in my project where I need to process 6000 classes * 20 methods * 5 parameter * ?? elements * in queries with about 5 to 10 query steps.
... and which produces not nice code like:

List list = ...query.list();
for(item:list) {
 ... do something with each item
}

spoon is already full of such code. Just search for usages of CtElement#getElements()
So I am giving up. Any idea how to proceed?

pvojtechovsky · 2016-12-20T07:22:38Z

Here is the exact definition what avoids future progress from my side:
Your PR on my PR deletes OverriddenMethodFilter#forEach(CtConsumer<CtMethod> output, CtMethod input), which is the example of usage of MAIN feature of my PR. With code of your PR it is impossible for me to implement such method effectively. Show me how to implement it using Your approach. Then
A) I will understand a way, which is evident for You, but not for me.
B) You will understand that CtQueryStep#forEach(Consumer, input) and CtConsumer are good and will not try to remove it from the design.

monperrus · 2016-12-20T08:44:32Z

Let's focus on requirements and API. It seems we now agree on a basic set of requirements (listed here) and API (in pvojtechovsky#4). Why about merging it first? What prevents this to your opinion? For implementing scope-based query, we will probably either add new methods in the query interfaces or new interfaces, but again, let's first agree on the core design here.

query: design + javadoc

pvojtechovsky · 2016-12-20T10:19:25Z

I merged your PR.

What prevents this to your opinion?

Query composition is not possible now with your PR.

For implementing scope-based query, we will probably either add new
methods in the query interfaces or new interfaces,

ok, I suggest that You continue on future query development, until it gets at least to state, where it is possible to implement scope based queries.

but again, let's first agree on the core design here.

this core design is quite limited now. I agree with anything what will cause a progress with this PR. But I will spent my effort on another Spoon features now, which are based on the state of this PR before your PR was merged.

monperrus · 2016-12-20T10:51:21Z

Query composition is not possible now with your PR.

renamed the title. do you agree that the four listed requirements are well supported with this PR?

pvojtechovsky · 2016-12-20T11:04:12Z

map functions with arbitrary return types (incl. that are not CtElement)

yes

chained queries

partially. You can chain implementations of CtFunction, but you cannot chain queries.

lazy evaluation

partially. The CtFunction is evaluated lazily for each input element, but it is not possible the apply lazy evaluation to output of that function, because function must return results using return statement and cannot call next query step by Consumer.apply(element). Therefore usage of such queries is limited to smaller result sets.

support for queries with Java 8 lambdas

yes

But I have no problem if you merge it like it is and then continue with scope-based query.

And before merge, I suggest you fix the little problems in your PR. See my comments in your PR.

monperrus · 2016-12-20T11:11:05Z

But I have no problem if you merge it like it

The idea is to be 100% on the same wavelength before merging

chained queries

partially. You can chain implementations of CtFunction, but you cannot chain queries.

to me, "chaining" means writing filterChildren(x).filterChildren(y)... or map(z).map(t)... or even filterChildren(p).map(q)...

This is completely supported, right?

pvojtechovsky · 2016-12-20T11:43:47Z

yes this is supported

monperrus · 2016-12-20T11:47:55Z

On lazy evaluationn you say

The CtFunction is evaluated lazily for each input element, but it is not possible the apply lazy evaluation to output of that function, because function must return results using return statement and cannot call next query step by Consumer.apply(element). Therefore usage of such queries is limited to smaller result sets.

I don't understand. Could you elaborate?

To me, in the current implementation, all queries are only performed by calling list() on the last query method. Correct?

pvojtechovsky · 2016-12-20T12:07:21Z

it is not possible the apply lazy evaluation to output of that function

I don't understand. Could you elaborate?

The implementation of ChildrenFilteringWrapper is correct, because it calls

if (matches) {
	//send input to output, because Fitler.matches returned true
	next.accept(element);
}

So it will work effectively for large amount of elements too.

The implementation of FunctionWrapper is correct too, because CtFunction is intended for query steps, which will return relatively small number of results.

So what is wrong? There is no way how to extend Query by way it works well for functions, which produces large result set. You mentioned it will be next step. OK. But as I mentioned until this feature is back again, I cannot continue, so I will continue on my old branch in the mean time.

To me, in the current implementation, all queries are only performed by calling list() on the last query method. Correct?

Correct.

Anyway thanks for Your effort Martin, to try to make it/keep it understandable!

doc: improves doc of new Query API

monperrus · 2016-12-20T19:48:36Z

@surli this PR is now ready for your review. there is no need to read and understand the whole discussion, the PR itself (doc, javadoc, tests) should now be self-contained.

surli · 2016-12-21T09:30:56Z

src/main/java/spoon/reflect/visitor/QueryVisitor.java

@@ -42,8 +46,8 @@ public QueryVisitor(Filter<T> filter) {
 		if (filter instanceof AbstractFilter) {
 			filteredType =  ((AbstractFilter<T>) filter).getType();
 		} else {
-			Class<?>[] params = RtHelper.getMethodParameterTypes(filter.getClass(), "matches", 1);
-			filteredType = (Class<T>) params[0];
+			Method method = RtHelper.getMethod(filter.getClass(), "matches", 1);


You did not check here that method can be null as you did in AbstractFilter: it's on purpose because you assume it can never happens or is it missing?

I cannot imagine situation when "matches" method is missing, because Filter interface has such method.
But I can add check there. Thank you

surli · 2016-12-21T09:40:04Z

src/test/java/spoon/test/filters/FilterTest.java

+			}).list();
+		assertTrue(context.count==1);
+		assertTrue(context.totalCount>1);
+


Not sure about what you're testing there, compared to the previous test.
Why don't you use a NameFilter here?

You are right this test brings nothing new, The testFunctionQueryStep already tests the same. I will remove this test

I have deleted the test

surli · 2016-12-21T09:46:12Z

src/main/java/spoon/reflect/visitor/chain/CtQueryImpl.java

+	 *
+	 * @param &lt;T> - the type of accepted elements
+	 */
+	private interface CtConsumer<T> {


What's the purpose exactly to distinguish this interface from Step? From my point of view, each time you use a CtConsumer it's in fact a Step you're using, am I wrong?

I need CtConsumer as public interface later. But Martin made it private in this first step, because after his simplifications/deleting of functionality this interface is not needed.

pvojtechovsky force-pushed the searchScopeAwareFilter branch from 606b470 to 5679c8d Compare December 1, 2016 19:49

pvojtechovsky mentioned this pull request Dec 1, 2016

functional ElementConsumer in queries #1017

Closed

pvojtechovsky force-pushed the searchScopeAwareFilter branch from 5c4d19c to 80c7dc1 Compare December 4, 2016 21:29

pvojtechovsky changed the title ~~filters may define searching context and can be used by CtElement.getElements~~ Query can be built from steps where the optionally used filters may influence scanning context Dec 5, 2016

monperrus reviewed Dec 5, 2016

View reviewed changes

monperrus reviewed Dec 6, 2016

View reviewed changes

pvojtechovsky force-pushed the searchScopeAwareFilter branch from 17ffb07 to c191d5d Compare December 7, 2016 21:19

pvojtechovsky force-pushed the searchScopeAwareFilter branch from c191d5d to 10abd98 Compare December 8, 2016 21:22

monperrus reviewed Dec 9, 2016

View reviewed changes

monperrus reviewed Dec 10, 2016

View reviewed changes

up

62f1483

Merge pull request #4 from monperrus/searchScopeAwareFilter

8e17150

query: design + javadoc

monperrus changed the title ~~feature: new query architecture with fine-grain control and query composition~~ feature: new query architecture with support for Java8 queries Dec 20, 2016

monperrus and others added 2 commits December 20, 2016 20:40

doc: improves doc of new Query API

ef2bc14

Merge pull request #5 from monperrus/searchScopeAwareFilter

23b81fb

doc: improves doc of new Query API

surli reviewed Dec 21, 2016

View reviewed changes

remove useless test and test method!=null

2c74d61

surli merged commit adaa3f5 into INRIA:master Dec 21, 2016

pvojtechovsky mentioned this pull request Dec 22, 2016

feature: all you want for queries #1070

Closed

tdurieux mentioned this pull request Jan 4, 2017

Release 5.5.0 planned for Wednesday Jan 11th 2017 #1081

Closed

pvojtechovsky deleted the searchScopeAwareFilter branch January 21, 2017 12:02


		import spoon.support.util.SafeInvoker;

		public class AsyncFunctionQueryStep<O> extends QueryStep<O> {

feature: new query architecture with support for Java8 queries #1018

feature: new query architecture with support for Java8 queries #1018

Conversation

pvojtechovsky commented Nov 30, 2016 • edited by monperrus Loading

tdurieux commented Nov 30, 2016

monperrus commented Dec 1, 2016

pvojtechovsky commented Dec 1, 2016

pvojtechovsky commented Dec 1, 2016

tdurieux commented Dec 1, 2016 • edited Loading

pvojtechovsky commented Dec 1, 2016

pvojtechovsky commented Dec 1, 2016

pvojtechovsky commented Dec 1, 2016

pvojtechovsky commented Dec 1, 2016

pvojtechovsky commented Dec 3, 2016

pvojtechovsky commented Dec 4, 2016

pvojtechovsky commented Dec 5, 2016

monperrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Dec 6, 2016 via email

pvojtechovsky commented Dec 6, 2016

pvojtechovsky commented Dec 7, 2016 • edited Loading

monperrus commented Dec 8, 2016 via email

pvojtechovsky commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Dec 17, 2016

pvojtechovsky commented Dec 17, 2016

pvojtechovsky commented Dec 18, 2016

monperrus commented Dec 18, 2016

pvojtechovsky commented Dec 19, 2016

monperrus commented Dec 19, 2016 via email

pvojtechovsky commented Dec 19, 2016

pvojtechovsky commented Dec 20, 2016

monperrus commented Dec 20, 2016 via email

pvojtechovsky commented Dec 20, 2016

monperrus commented Dec 20, 2016 via email

pvojtechovsky commented Dec 20, 2016 • edited Loading

monperrus commented Dec 20, 2016 • edited Loading

pvojtechovsky commented Dec 20, 2016

monperrus commented Dec 20, 2016

pvojtechovsky commented Dec 20, 2016 • edited Loading

monperrus commented Dec 20, 2016

Choose a reason for hiding this comment

pvojtechovsky commented Nov 30, 2016 •

edited by monperrus

Loading

tdurieux commented Dec 1, 2016 •

edited

Loading

pvojtechovsky commented Dec 7, 2016 •

edited

Loading

pvojtechovsky commented Dec 20, 2016 •

edited

Loading

monperrus commented Dec 20, 2016 •

edited

Loading

pvojtechovsky commented Dec 20, 2016 •

edited

Loading