review: feature: VariableReferenceFunction, VariableScopeFunction

[pvojtechovsky]

This is the query, which returns all the references to the CtVariable based elements.
Your opinion?

[pvojtechovsky]

Can I fix such Travis problem ? May be something is broken?

[surli]

Can I fix such Travis problem ? May be something is broken?

We just change the pom.xml to now fail on JDK8-related javadoc errors, apparently you have an error in your doc:

error: unterminated signature
[ERROR] *  @see QueryFactory#createQuery(Object))

[pvojtechovsky]

I need VariableReferenceQuery in #1005 to search for all occurrences of variable name, which has to be renamed.
But refactoring should do also check whether it can be applied. For example refactoring should throw an exception instead of rename of variable x to y if there is already another variable y in scope of variable 'x'.
Such check needs part of VariableReferenceQuery behavior - to be able to search in all model elements in scope of to be renamed variable.
I am just looking for an design for that. I will come with something, but your ideas are welcome!

[monperrus]

rebase to have a clean diff?

[pvojtechovsky]

I see these topics:

1. Looking at VariableScopeQuery I am not sure whether it is good idea to implement it as one query, which handles all types of variables. May be the code would be more readable if it would be split to

• LocalVariableScopeQuery

• FieldScopeQuery

• ParameterScopeQuery

• CatchVariableScopeQuery

  and finally there would be

• VariableScopeQuery
  which would be just facade of 4 queries above.

2. Another question is whether it should have method VariableScopeQuery#setFilter, which allows filtering of elements in scope of a variable. It is no problem to add filter later as next query step, using existing query methods.

3. Are the names of these queries understandable? May be

• VariableScopeScanningQuery
• VariableScopeScanner
• AllElementsInVariableScope
  ... I do not know ...

May be we can try

1. to collect list of all interesting queries, which somebody might need in future
2. having whole list in mind to give these queries understandable names -> so we would have dictionary of query names and their short description
3. to implement query, following short description, when somebody needs one.

[monperrus]

to collect list of all interesting queries, which somebody might need in future

I'd be more pragmatic: to implement, test and document what you need now, as long as it's generic enough for others to use it as well.

May be the code would be more readable if it would be split to LocalVariableScopeQuery FieldScopeQuery ParameterScopeQuery CatchVariableScopeQuery

Yes! let's go for 4 separate baby PRs which will be easy to discuss, review and merge.

[pvojtechovsky]

Which java package we will use for new queries?
A) spoon.reflect.visitor.filter
B) spoon.reflect.visitor.query
C) ?

[monperrus]

A) spoon.reflect.visitor.filter

+1

[pvojtechovsky]

Depends on PRs #1136, #1141, #1144, #1145. After they are merged then it will be compilable and test will pass. Till then we can discuss the implementation of this PR.

[pvojtechovsky]

Idea - May be this PR and all related #1136, #1141, #1144, #1145. are wrong approach to solve the problem, which might be solved by change in spoon model or by new - more abstract - model.

This PR expects that Variable reference and Variable are not connected by direct java model pointer, but they are connected just logically/virtually - by an algorithm, which detects which Variable declaration belongs to current Variable reference.

May be it would be more efficient if each variable reference would have direct java pointer to its variable declaration. Then clients does not need to search the model, but can use that reference.

I see that current Spoon model models java code on some level of abstraction. I can imagine that having another related model on higher level of abstraction might be helpful to solve SOME refactoring and analyzing tasks in easier way.

So may be the solution of this PR is to design and implement spoon model on higher abstract level - for example the one, which allows doing data flow and control flow analyze ...

Is there already some spoon related project about that?

Your opinion?

[monperrus]

May be it would be more efficient if each variable reference would have direct java pointer to its variable declaration.

We've already discussed about this a number of times within the team. This is doable, we just have to be careful of dangling / incorrect references when refactoring and maintenance overhead. The performance improvement really depends on the analysis and transformation workload.

[monperrus]

So may be the solution of this PR is to design and implement spoon model on higher abstract level - for example the one, which allows doing data flow and control flow analyze ...

yes, for instance, @marcelinorc <https://github.com/marcelinorc>does this in one of his projects (https://github.com/DIVERSIFY-project/autojmh-source-code?). Those features would be welcome in spoon-core in the mid-term.

[msteinbeck]

May be it would be more efficient if each variable reference would have direct java pointer to its
variable declaration.

We've already discussed about this a number of times within the team. This is doable, we just have
to be careful of dangling / incorrect references when refactoring and maintenance overhead. The
performance improvement really depends on the analysis and transformation workload.

I'm incrementally updating particular AST nodes by removing existing one and replacing them with "refreshed" versions. Having a direct pointer to the, for instance, declaring node of a reference may completely break my approach as I need to update unmodified nodes as well.

[msteinbeck]

How about adding a thread safe cache to the model that allows to fetch already resolved references (we are doing this by our own to enhance performance )? That way, resolving references becomes very fast. By providing a method to invalidate a model's entire cache, updating subsets of the AST is still possible because references will store the new node in cache while resolving. It is important that this cache is thread safe because we are using several thread to traverse the AST in parallel.

[pvojtechovsky]

Having a direct pointer to the, for instance, declaring node of a reference may completely break my approach as I need to update unmodified nodes as well.

that's good point. May be this update might be done automatically, by the method which replaces old by new element.

[pvojtechovsky]

adding a thread safe cache to the model

I am doing something like this too. It would be nice to be able to register listener for a model modification events. Such listeners might then clean the obsolete cache entries. But current spoon model fires no such events.

[msteinbeck]

that's good point. May be this update might be done automatically, by the method which replaces old by new element.

In order to do so, I need to find all references to the old node. That may require a whole lot of time so that I'm afraid that updating the AST takes more time than recreation.

[pvojtechovsky]

There might be bidirectional reference. The reference points to declaration and declaration has a list of all references. Then it is fast. But of course it is extra effort to keep these references consistent. And these references cannot be created at model creation time (because model of declaration may not exist at time when some references are created), but only later. So it is kind of model extension - not the core of model... kind of cache

[pvojtechovsky]

And if references are not bound to declarations at model creation/modification time, then we need this and related PRs to found them.

[pvojtechovsky]

May be it would be more efficient if each variable reference would have direct java pointer to its
variable declaration.

Martin wrote: We've already discussed about this a number of times within the team.

Are there some written results of this discussion? A concept? Issue/PR with some details? Is there some place in this project, where to write/collect such information?

[monperrus]

Are there some written results of this discussion?

No, that was informal oral discussions.

Is there some place in this project, where to write/collect such information?

what about creating an issue for this?

…

--Martin

[pvojtechovsky]

ok, I will make issue for that.

What about review of this PR and related #1136, #1141, #1144, #1145 PRs? If you have no time, then no problem, I am not in hurry with that. It would be just pity if you would be just waiting for me and I for You :-)

[monperrus]

If they're ready for review, could you replace the [WIP] by [review]? Thanks.

[monperrus]

travis fails here?

[pvojtechovsky]

No, this PR is preview of changes which become compilable after other PRs are merged in and rebased

[pvojtechovsky]

VariableScopeFunction was added too. I will need it in change local variable name refactoring ... future PR.

This PR is still not compilable - as expected - because it waits for rebase after #1141 is merged

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170130.234600-20

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

Detected changes: 0.

[surli]

Thanks @pvojtechovsky, it's very clear and elegant now. Ready to merge, wdyt @tdurieux @monperrus ?

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170131.234706-21

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

Detected changes: 0.