Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review: feature: VariableReferenceFunction, VariableScopeFunction #1114

Merged
merged 6 commits into from
Feb 1, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

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

@pvojtechovsky
Copy link
Collaborator Author

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

@surli
Copy link
Collaborator

surli commented Jan 12, 2017

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 pvojtechovsky changed the title Variable reference query WIP[incubation] Variable reference query Jan 12, 2017
@pvojtechovsky
Copy link
Collaborator Author

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
Copy link
Collaborator

rebase to have a clean diff?

@pvojtechovsky
Copy link
Collaborator Author

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.

  1. 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.

  2. 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
Copy link
Collaborator

monperrus commented Jan 18, 2017

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
Copy link
Collaborator Author

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

@monperrus
Copy link
Collaborator

monperrus commented Jan 19, 2017 via email

@pvojtechovsky
Copy link
Collaborator Author

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 pvojtechovsky changed the title WIP[incubation] Variable reference query WIP VariableReferenceFunction Jan 22, 2017
@pvojtechovsky
Copy link
Collaborator Author

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
Copy link
Collaborator

monperrus commented Jan 24, 2017 via email

@monperrus
Copy link
Collaborator

monperrus commented Jan 24, 2017 via email

@msteinbeck
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@pvojtechovsky
Copy link
Collaborator Author

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
Copy link
Collaborator

monperrus commented Jan 25, 2017 via email

@pvojtechovsky
Copy link
Collaborator Author

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
Copy link
Collaborator

monperrus commented Jan 25, 2017 via email

@pvojtechovsky pvojtechovsky changed the title WIP VariableReferenceFunction review VariableReferenceFunction Jan 25, 2017
@monperrus
Copy link
Collaborator

travis fails here?

@pvojtechovsky
Copy link
Collaborator Author

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

@pvojtechovsky pvojtechovsky changed the title review VariableReferenceFunction WiP: VariableReferenceFunction, VariableScopeFunction Jan 30, 2017
@pvojtechovsky
Copy link
Collaborator Author

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

@pvojtechovsky pvojtechovsky changed the title WiP: VariableReferenceFunction, VariableScopeFunction review: feature: VariableReferenceFunction, VariableScopeFunction Jan 31, 2017
@spoon-bot
Copy link
Collaborator

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
Copy link
Collaborator

surli commented Feb 1, 2017

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

@surli surli merged commit 8c35c98 into INRIA:master Feb 1, 2017
@spoon-bot
Copy link
Collaborator

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.

@pvojtechovsky pvojtechovsky deleted the VariableReferenceQuery branch February 1, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants